Re: [PATCH v9 15/16] xen/arm: vpci: check guest range

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

On 9/22/23 04:44, Roger Pau Monné wrote:
> On Tue, Aug 29, 2023 at 11:19:47PM +0000, Volodymyr Babchuk wrote:
>> From: Stewart Hildebrand <stewart.hildebrand@xxxxxxx>
>>
>> Skip mapping the BAR if it is not in a valid range.
>>
>> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@xxxxxxx>
>> ---
>>  xen/drivers/vpci/header.c | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>>
>> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
>> index 1d243eeaf9..dbabdcbed2 100644
>> --- a/xen/drivers/vpci/header.c
>> +++ b/xen/drivers/vpci/header.c
>> @@ -345,6 +345,15 @@ static int modify_bars(const struct pci_dev *pdev, 
>> uint16_t cmd, bool rom_only)
>>               bar->enabled == !!(cmd & PCI_COMMAND_MEMORY) )
>>              continue;
>>
>> +#ifdef CONFIG_ARM
>> +        if ( !is_hardware_domain(pdev->domain) )
>> +        {
>> +            if ( (start_guest < PFN_DOWN(GUEST_VPCI_MEM_ADDR)) ||
>> +                 (end_guest >= PFN_DOWN(GUEST_VPCI_MEM_ADDR + 
>> GUEST_VPCI_MEM_SIZE)) )
>> +                continue;
>> +        }
>> +#endif
> 
> Hm, I think this should be in a hook similar to pci_check_bar() that
> can be implemented per-arch.
> 
> IIRC at least on x86 we allow the guest to place the BARs whenever it
> wants, would such placement cause issues to the hypervisor on Arm?

Hm. I wrote this patch in a hurry to make v9 of this series work on ARM. In my 
haste I also forgot about the prefetchable range starting at 
GUEST_VPCI_PREFETCH_MEM_ADDR, but that won't matter as we can probably throw 
this patch out.

Now that I've had some more time to investigate, I believe the check in this 
patch is more or less redundant to the existing check in map_range() added in 
baa6ea700386 ("vpci: add permission checks to map_range()").

The issue is that during initialization bar->guest_addr is zeroed, and this 
initial value of bar->guest_addr will fail the permissions check in map_range() 
and crash the domain. When the guest writes a new valid BAR, the old invalid 
address remains in the rangeset to be mapped. If we simply remove the old 
invalid BAR from the rangeset, that seems to fix the issue. So something like 
this:

diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
index d4629a14f26b..732be26f0d2d 100644
--- a/xen/drivers/vpci/header.c
+++ b/xen/drivers/vpci/header.c
@@ -638,6 +638,16 @@ static void cf_check guest_bar_write(const struct pci_dev 
*pdev,
         return;
     }

+    if ( (val != 0xfffffff0U) &&
+         (bar->guest_addr != (0xfffffff0ULL & ~(bar->size - 1))) &&
+         (bar->guest_addr != (0xfffffffffffffff0ULL & ~(bar->size - 1))) )
+    {
+        if ( rangeset_remove_range(bar->mem, PFN_DOWN(bar->guest_addr),
+                                   PFN_DOWN(bar->guest_addr + bar->size) - 1) )
+            gprintk(XENLOG_WARNING, "%pp failed to remove old BAR range\n",
+                    &pdev->sbdf);
+    }
+
     bar->guest_addr = guest_addr;
 }