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

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 07/19/2018 11:18 AM, Isaila Alexandru wrote:
> On Mi, 2018-07-18 at 15:33 +0000, George Dunlap wrote:
>>
>>>
>>> On Jul 2, 2018, at 8:42 AM, Alexandru Isaila <aisaila@bitdefender.c
>>> om> wrote:
>>>
>>> From: Isaila Alexandru <aisaila@xxxxxxxxxxxxxxx>
>>>
>>> This patch adds access rights for the NPT pages. The access rights
>>> are
>>> saved in a radix tree with the root saved in p2m_domain. The rights
>>> are manipulated through p2m_set_access()
>>> and p2m_get_access() functions.
>>> The patch follows the ept logic.
>> This description needs to be much more complete.  Something like
>> this:
>>
>> ---
>> This patch adds access control for NPT mode.  
>>
>> There aren’t enough extra bits to store the access rights in the NPT
>> p2m table, so we add a radix tree to store the rights.  For
>> efficiency, remove entries which match the default permissions rather
>> than continuing to store them.
>>
>> Modify p2m-pt.c:p2m_type_to_flags() to mirror the ept version: taking
>> an access, and removing / adding RW or NX flags as appropriate.
>> ---
>>
> I will update the patch description.
>>>
>>>
>>> Note: It was tested with xen-access write
>>>
>>> Signed-off-by: Alexandru Isaila <aisaila@xxxxxxxxxxxxxxx>
>>
>>>
>>>
>>> ---
>>> Changes since V2:
>>>     - Delete blak line
>>>     - Add return if p2m_access_rwx = a
>>>     - Delete the comment from p2m_pt_get_entry()
>>>     - Moved radix_tree_init() to arch_monitor_init_domain().
>>> ---
>>> xen/arch/x86/mm/mem_access.c     |   3 ++
>>> xen/arch/x86/mm/p2m-pt.c         | 109
>>> ++++++++++++++++++++++++++++++++++-----
>>> xen/arch/x86/mm/p2m.c            |   6 +++
>>> xen/arch/x86/monitor.c           |  13 +++++
>>> xen/include/asm-x86/mem_access.h |   2 +-
>>> xen/include/asm-x86/p2m.h        |   6 +++
>>> 6 files changed, 124 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/xen/arch/x86/mm/mem_access.c
>>> b/xen/arch/x86/mm/mem_access.c
>>> index c0cd017..d78c82c 100644
>>> --- a/xen/arch/x86/mm/mem_access.c
>>> +++ b/xen/arch/x86/mm/mem_access.c
>>> @@ -221,7 +221,10 @@ bool p2m_mem_access_check(paddr_t gpa,
>>> unsigned long gla,
>>>         {
>>>             req->u.mem_access.flags |= MEM_ACCESS_GLA_VALID;
>>>             req->u.mem_access.gla = gla;
>>> +        }
>>>
>>> +        if ( npfec.gla_valid || cpu_has_svm )
>>> +        {
>> I can’t immediately tell what this is about, which means it needs a
>> comment.
>>
>> It may also be (depending on why this is here) that “cpu_has_svm”
>> makes this more fragile — if this is really about having a radix
>> tree, for instance, then you should probably check for a radix tree.
> 
> This is about the different npfec on SVN. The gla in never valid so the
> fault flag will not be set.

Specifically, this is what svm_do_nested_pgfault() does:

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);

so gla_valid is never non-0 on SVM.


Thanks,
Razvan

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