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, <rcojoc...@bitdefender.com> wrote:
>>> On 07/19/2018 11:30 AM, Jan Beulich wrote:
>>>>
>>>>>
>>>>>>
>>>>>>>
>>>>>>> On 19.07.18 at 10:18, <aisa...@bitdefender.com> 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
