Re: [Xen-devel] [PATCH v3] x86/mm: Add mem access rights to NPT

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 07/19/2018 02:08 PM, Isaila Alexandru wrote:
> On Jo, 2018-07-19 at 04:02 -0600, Jan Beulich wrote:
>>>
>>>>
>>>>>
>>>>> On 19.07.18 at 10:43, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>>> On 07/19/2018 11:30 AM, Jan Beulich wrote:
>>>>
>>>>>
>>>>>>
>>>>>>>
>>>>>>> On 19.07.18 at 10:18, <aisaila@xxxxxxxxxxxxxxx> wrote:
>>>>> On Mi, 2018-07-18 at 15:33 +0000, George Dunlap wrote:
>>>>>>
>>>>>>>
>>>>>>> On Jul 2, 2018, at 8:42 AM, Alexandru Isaila <aisaila@bitde
>>>>>>> fender.c 
>>>>>>> +            break;
>>>>>>> +        case p2m_access_x:
>>>>>>> +            flags &= ~_PAGE_RW;
>>>>>>> +            break;
>>>>>>> +        case p2m_access_rwx:
>>>>>>> +        default:
>>>>>>> +            break;
>>>>>>>     }
>>>>>> I think you want another blank line here too.
>>>>>>
>>>>>> Also, this doesn’t seem to capture the ‘r’ part of the
>>>>>> equation —
>>>>>> shouldn’t p2m_access_n end up with a not-present p2m entry?
>>>>> SVM dosen't explicitly provide a read access bit so we treat
>>>>> read and
>>>>> write the same way.
>>>> Read and write can't possibly be treated the same. You ought to
>>>> use
>>>> the present bit to deny read (really: any) access, as also
>>>> implied by
>>>> George's response.
>>> They aren't treated the same as far sending out a vm_event goes.
>>> However, if we understand this correctly, there is no way to cause
>>> only
>>> read, or only write exits for NPT. They are bundled together under
>>> _PAGE_RW.
>>>
>>> So svm_do_nested_pgfault() tries to sort these out:
>>>
>>> 1781     struct npfec npfec = {
>>> 1782         .read_access = !(pfec & PFEC_insn_fetch),
>>> 1783         .write_access = !!(pfec & PFEC_write_access),
>>> 1784         .insn_fetch = !!(pfec & PFEC_insn_fetch),
>>> 1785         .present = !!(pfec & PFEC_page_present),
>>> 1786     };
>>> 1787
>>> 1788     /* These bits are mutually exclusive */
>>> 1789     if ( pfec & NPT_PFEC_with_gla )
>>> 1790         npfec.kind = npfec_kind_with_gla;
>>> 1791     else if ( pfec & NPT_PFEC_in_gpt )
>>> 1792         npfec.kind = npfec_kind_in_gpt;
>>> 1793
>>> 1794     ret = hvm_hap_nested_page_fault(gpa, ~0ul, npfec);
>>>
>>> but a read access is considered to be something that's not an insn
>>> fetch, and we only have a specific bit set for the write.
>>>
>>> Since hvm_hap_nested_page_fault() looks at npfec to decide when to
>>> send
>>> out a vm_event, this takes care of handling reads and writes
>>> differently
>>> at this level; however it's not possible to set separate single
>>> "don't
>>> read" or "don't write" exit-causing flags with NPT.
>> All fine, but George's question was raised in the context of
>> permission
>> conversion from p2m to pte representation.
> 
> I've tried a test with xen access that sets XENMEM_access_n on all the
> pages and in p2m_type_to_flags() remove the _PAGE_PRESENT flag for
> the p2m_access_n case and the guest fails with triple fault. There are
> a lot of checks against _PAGE_PRESENT in p2m-pt and a lot of return
> invalid if the flag is not present. I don't think this is the way to go
> with the p2m_access_n flags.

I will absolutely nack any interface where if the caller says, "Please
remove read permission", the hypervisor says, "OK!" but then allows read
permission anyway -- particularly in one which is allegedly designed for
security tools.

If it's not practical / more work than it's worth doing at the moment to
implement p2m_access_n on NPT, then you should return an error when it's
requested.

The same really should be true for write-only permission as well -- if
it's not possible to allow writes but not reads, then you should return
an error when such permissions are requested.

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel