Re: [PATCH v7 00/14] arm/mem_access: Walk guest page tables in SW if mem_access is active

[Sergej Proskurin <proskurin@sec.in.tum.de>]

Hi Julien,


On 08/04/2017 11:15 AM, Sergej Proskurin wrote:
> Hi Julien,
>
> Sorry for the late reply.
>
> On 07/31/2017 04:38 PM, Julien Grall wrote:
>>
>> On 18/07/17 13:24, Sergej Proskurin wrote:
>>> Hi all,
>> Hi,
>>
>>> The function p2m_mem_access_check_and_get_page is called from the function
>>> get_page_from_gva if mem_access is active and the hardware-aided translation of
>>> the given guest virtual address (gva) into machine address fails. That is, if
>>> the stage-2 translation tables constrain access to the guests's page tables,
>>> hardware-assisted translation will fail. The idea of the function
>>> p2m_mem_access_check_and_get_page is thus to translate the given gva and check
>>> the requested access rights in software. However, as the current implementation
>>> of p2m_mem_access_check_and_get_page makes use of the hardware-aided gva to ipa
>>> translation, the translation might also fail because of reasons stated above
>>> and will become equally relevant for the altp2m implementation on ARM.  As
>>> such, we provide a software guest translation table walk to address the above
>>> mentioned issue.
>>>
>>> The current version of the implementation supports translation of both the
>>> short-descriptor as well as the long-descriptor translation table format on
>>> ARMv7 and ARMv8 (AArch32/AArch64).
>>>
>>> This revised version incorporates the comments of the previous patch series. In
>>> this patch version we refine the definition of PAGE_SIZE_GRAN and
>>> PAGE_MASK_GRAN. In particular, we use PAGE_SIZE_GRAN to define PAGE_MASK_GRAN
>>> and thus avoid these defines to have a differing type. We also changed the
>>> previously introduced macro BITS_PER_LONG_LONG to BITS_PER_LLONG. Further
>>> changes comprise minor adjustments in comments and renaming of macros and
>>> function parameters. Some additional changes comprising code readability and
>>> correct type usage have been made and stated in the individual commits.
>>>
>>> The following patch series can be found on Github[0].
>> I tried this series today with the change [1] in Xen to check the translation
>> is valid. However, I got a failure when booting non-LPAE arm32 Dom0:
>>
> That's odd.. Thanks for the information. I will investigate this issue
> next week, as soon as I have access to our ARMv7 board.
>
>> (XEN) Loading kernel from boot module @ 0000000080008000
>> (XEN) Allocating 1:1 mappings totalling 512MB for dom0:
>> (XEN) BANK[0] 0x000000a0000000-0x000000c0000000 (512MB)
>> (XEN) Grant table range: 0x000000ffe00000-0x000000ffe6a000
>> (XEN) Loading zImage from 0000000080008000 to 00000000a7800000-00000000a7f50e28
>> (XEN) Allocating PPI 16 for event channel interrupt
>> (XEN) Loading dom0 DTB to 0x00000000a8000000-0x00000000a8001f8e
>> (XEN) Std. Loglevel: All
>> (XEN) Guest Loglevel: All
>> (XEN) guest_walk_tables: gva 0xffeff018 pipa 0x1c090018
>> (XEN) access_guest_memory_by_ipa: gpa 0xa0207ff8
>> (XEN) access_guest_memory_by_ipa: gpa 0xffffffffa13aebfc
>> (XEN) d0: guestcopy: failed to get table entry.
>> (XEN) Xen BUG at traps.c:2737
>> (XEN) ----[ Xen-4.10-unstable  arm32  debug=y   Not tainted ]----
>> (XEN) CPU:    0
>> (XEN) PC:     00264dc0 do_trap_guest_sync+0x161c/0x1804
>> (XEN) CPSR:   a000005a MODE:Hypervisor
>> (XEN)      R0: ffffffea R1: 00000000 R2: 00000000 R3: 0000004a
>> (XEN)      R4: 93830007 R5: 47fcff58 R6: 93830007 R7: 00000007
>> (XEN)      R8: 1c090000 R9: 00000000 R10:00000000 R11:47fcff54 R12:ffffffea
>> (XEN) HYP: SP: 47fcfee4 LR: 00258dec
>> (XEN) 
>> (XEN)   VTCR_EL2: 80003558
>> (XEN)  VTTBR_EL2: 00010008f3ffc000
>> (XEN) 
>> (XEN)  SCTLR_EL2: 30cd187f
>> (XEN)    HCR_EL2: 000000000038663f
>> (XEN)  TTBR0_EL2: 00000000fff02000
>> (XEN) 
>> (XEN)    ESR_EL2: 00000000
>> (XEN)  HPFAR_EL2: 00000000001c0900
>> (XEN)      HDFAR: ffeff018
>> (XEN)      HIFAR: 00000000
>> (XEN) 
>> (XEN) Xen stack trace from sp=47fcfee4:
>> (XEN)    00000000 47fcff34 00256008 47fcfefc 47fcfefc 200000da 00000004 47fd48f4
>> (XEN)    002d5ef0 00000004 002d1f00 00000004 00000000 002d1f00 c163f740 93830007
>> (XEN)    ffeff018 1c090018 00000000 47fcff44 c15e70ac 0000005b c15e70ac c074400c
>> (XEN)    00000031 00000000 c0743ff8 47fcff58 00268ce0 c15e70ac 0000005b 00000031
>> (XEN)    ffeff000 c15e70ac 0000005b c15e70ac c074400c 00000031 00000000 c0743ff8
>> (XEN)    00000000 0000001f ffffffff 00000000 c074401c 200001d3 93830007 00000000
>> (XEN)    c161cac0 c161cac0 c1501de0 c0735640 c161cacc c161cacc c161cad8 c161cad8
>> (XEN)    00000000 00000000 00000000 00000000 00000000 c161cae4 c161cae4 400001d3
>> (XEN)    00000000 00000000 00000000 00000000 00000000 dfdfdfcf cfdfdfdf
>> (XEN) Xen call trace:
>> (XEN)    [<00264dc0>] do_trap_guest_sync+0x161c/0x1804 (PC)
>> (XEN)    [<00258dec>] access_guest_memory_by_ipa+0x25c/0x284 (LR)
>> (XEN)    [<00268ce0>] entry.o#return_from_trap+0/0x4
>> (XEN) 
>> (XEN) 
>> (XEN) ****************************************
>> (XEN) Panic on CPU 0:
>> (XEN) Xen BUG at traps.c:2737
>> (XEN) ****************************************
>> (XEN) 
>> (XEN) Reboot in five seconds...
>>
>> The IPA 0xffffffffa13aebfc is not valid for the domain.
>>
>> Cheers,
>>
>> [1]
>> diff --git a/xen/arch/arm/guestcopy.c b/xen/arch/arm/guestcopy.c
>> index 4ee07fcea3..89c5ebf3cf 100644
>> --- a/xen/arch/arm/guestcopy.c
>> +++ b/xen/arch/arm/guestcopy.c
>> @@ -139,6 +139,8 @@ int access_guest_memory_by_ipa(struct domain *d, paddr_t gpa, void *buf,
>>          return -EINVAL;
>>      }
>>  
>> +    printk("%s: gpa 0x%llx\n", __FUNCTION__, gpa);
>> +
>>      page = get_page_from_gfn(d, paddr_to_pfn(gpa), &p2mt, P2M_ALLOC);
>>      if ( !page )
>>      {
>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
>> index c07999b518..904abafcae 100644
>> --- a/xen/arch/arm/traps.c
>> +++ b/xen/arch/arm/traps.c
>> @@ -2688,6 +2688,8 @@ static bool try_map_mmio(gfn_t gfn)
>>      return !map_regions_p2mt(d, gfn, 1, mfn, p2m_mmio_direct_c);
>>  }
>>  
>> +#include <asm/guest_walk.h>
>> +
>>  static void do_trap_data_abort_guest(struct cpu_user_regs *regs,
>>                                       const union hsr hsr)
>>  {
>> @@ -2725,6 +2727,17 @@ static void do_trap_data_abort_guest(struct cpu_user_regs *regs,
>>              return; /* Try again */
>>      }
>>  
>> +    {
>> +        paddr_t ipa, pipa;
>> +        rc = gva_to_ipa(info.gva, &info.ipa, GV2M_READ);

There is no ipa field in mmio_info_t. But even if you used info.gpa
instead, the test that you have provided is unfortunately flawed:

>> +        BUG_ON(rc);
>> +        printk("guest_walk_tables: gva 0x%x pipa 0x%llx\n",
>> +               info.gva, pipa);
>> +        rc = guest_walk_tables(current, info.gva, &ipa, NULL);
>> +        BUG_ON(rc);
>> +        BUG_ON(ipa != pipa);

In your test-case you don't initialize pipa at all, however you test for
it in BUG_ON, which is the reason why it fails. I have adopted your test
case and it runs on ARMv7 (non-LPAE guest) and ARMv8 (LPAE guest)
without any issues. It would be great if you would verify this behaviour
by applying the following patch to the arm-gpt-walk-v7 patch [0] as before:



From a28db6321780c442b1c97aa78883dccbd84de7dd Mon Sep 17 00:00:00 2001
From: Sergej Proskurin <proskurin@sec.in.tum.de>
Date: Tue, 8 Aug 2017 13:30:00 +0200
Subject: [PATCH] Julien Grall's test case

---
 xen/arch/arm/guestcopy.c |  2 ++
 xen/arch/arm/traps.c     | 13 +++++++++++++
 2 files changed, 15 insertions(+)

diff --git a/xen/arch/arm/guestcopy.c b/xen/arch/arm/guestcopy.c
index 4ee07fcea3..f2758ebd45 100644
--- a/xen/arch/arm/guestcopy.c
+++ b/xen/arch/arm/guestcopy.c
@@ -139,6 +139,8 @@ int access_guest_memory_by_ipa(struct domain *d,
paddr_t gpa, void *buf,
         return -EINVAL;
     }

+    printk("%s: gpa 0x%"PRIpaddr"\n", __FUNCTION__, gpa);
+
     page = get_page_from_gfn(d, paddr_to_pfn(gpa), &p2mt, P2M_ALLOC);
     if ( !page )
     {
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index c07999b518..9b0b79a3fe 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -2688,6 +2688,8 @@ static bool try_map_mmio(gfn_t gfn)
     return !map_regions_p2mt(d, gfn, 1, mfn, p2m_mmio_direct_c);
 }

+#include <asm/guest_walk.h>
+
 static void do_trap_data_abort_guest(struct cpu_user_regs *regs,
                                      const union hsr hsr)
 {
@@ -2725,6 +2727,17 @@ static void do_trap_data_abort_guest(struct
cpu_user_regs *regs,
             return; /* Try again */
     }

+    {
+        paddr_t ipa;
+        rc = gva_to_ipa(info.gva, &info.gpa, GV2M_READ);
+        BUG_ON(rc);
+        printk("guest_walk_tables: gva 0x%"PRIvaddr" pipa 0x%"PRIpaddr"\n",
+               info.gva, info.gpa);
+        rc = guest_walk_tables(current, info.gva, &ipa, NULL);
+        BUG_ON(rc);
+        BUG_ON(ipa != info.gpa);
+    }
+
     switch ( fsc )
     {
     case FSC_FLT_PERM:
--
2.13.3




Thanks,
~Sergej

[0] https://github.com/sergej-proskurin/xen (branch arm-gpt-walk-v7)


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel