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

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

On 9/26/23 04:07, Roger Pau Monné wrote:
> On Mon, Sep 25, 2023 at 05:49:00PM -0400, Stewart Hildebrand wrote:
>> 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:
> 
> It does seem to me we are missing a proper cleanup of the rangeset
> contents in some paths then.  In the above paragraph you mention "the
> old invalid address remains in the rangeset to be mapped", how does it
> get in there in the first place, and why is the rangeset not emptied
> if the mapping failed?

Back in ("vpci/header: handle p2m range sets per BAR") I added a v->domain == 
pdev->domain check near the top of vpci_process_pending() as you appropriately 
suggested.

+    if ( v->domain != pdev->domain )
+    {
+        read_unlock(&v->domain->pci_lock);
+        return false;
+    }

I have also reverted this patch ("xen/arm: vpci: check guest range").

The sequence of events leading to the old value remaining in the rangeset are:

# xl pci-assignable-add 01:00.0
drivers/vpci/vpci.c:vpci_deassign_device()
    deassign 0000:01:00.0 from d0
# grep pci domu.cfg
pci = [ "01:00.0" ]
# xl create domu.cfg
drivers/vpci/vpci.c:vpci_deassign_device()
    deassign 0000:01:00.0 from d[IO]
drivers/vpci/vpci.c:vpci_assign_device()
    assign 0000:01:00.0 to d1
    bar->guest_addr is initialized to zero because of the line: pdev->vpci = 
xzalloc(struct vpci);
drivers/vpci/header.c:init_bars()
drivers/vpci/header.c:modify_bars()
    BAR0: start 0xe0000, end 0xe000f, start_guest 0x0, end_guest 0xf
    The range { 0-f } is added to the BAR0 rangeset for d1
drivers/vpci/header.c:defer_map()
    raise_softirq(SCHEDULE_SOFTIRQ);
drivers/vpci/header.c:vpci_process_pending()
    vpci_process_pending() returns because v->domain != pdev->domain (i.e. d0 
!= d1)
    BAR0 rangeset still contains { 0-f }
xl create finishes

Then during domU boot, guest initializes BAR0:

drivers/vpci/header.c:guest_bar_write()
    bar->guest_addr = 0x23000000
drivers/vpci/header.c:modify_bars()
    BAR0: start 0xe0000, end 0xe000f, start_guest 0x23000, end_guest 0x2300f
    The d1 BAR0 rangeset now contains both { 0-f } and { 23000-2300f }
drivers/vpci/header.c:defer_map()
    raise_softirq(SCHEDULE_SOFTIRQ);
drivers/vpci/header.c:vpci_process_pending()
    rangeset_consume_ranges(bar->mem, map_range, &data);
drivers/vpci/header.c:map_range()
    The range { 0-f } fails the permissions check and we crash the domU (back 
in vpci_process_pending)