Re: [PATCH v18 1/2] xen/arm: translate virtual PCI bus topology for guests

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

On 3/30/25 17:59, Julien Grall wrote:
> Hi Stewart,
> 
> I realize this series is at v18, but there are a few bits security-wise
> that concerns me. They don't have to be handled now because vPCI is
> still in tech preview. But we probably want to keep track of any issues
> if this hasn't yet been done in the code.

No worries, we want to get this right. Thank you for taking a look.

> On 25/03/2025 17:27, Stewart Hildebrand wrote:
>> diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
>> index 1e6aa5d799b9..49c9444195d7 100644
>> --- a/xen/drivers/vpci/vpci.c
>> +++ b/xen/drivers/vpci/vpci.c
>> @@ -81,6 +81,32 @@ static int assign_virtual_sbdf(struct pci_dev *pdev)
>>       return 0;
>>   }
>>   +/*
>> + * Find the physical device which is mapped to the virtual device
>> + * and translate virtual SBDF to the physical one.
>> + */
>> +bool vpci_translate_virtual_device(struct domain *d, pci_sbdf_t *sbdf)
>> +{
>> +    const struct pci_dev *pdev;
>> +
>> +    ASSERT(!is_hardware_domain(d));
>> +    read_lock(&d->pci_lock);
>> +
>> +    for_each_pdev ( d, pdev )
> 
> I can't remember whether this was discussed before (there is no
> indication in the commit message). What's the maximum number of PCI
> devices supported? Do we limit it?
> 
> Asking because iterating through the list could end up to be quite
> expensive.

32. See xen/include/xen/vpci.h:
#define VPCI_MAX_VIRT_DEV       (PCI_SLOT(~0) + 1)

> Also, not a change for this patch, but it seems vcpi_{read,write} will
> also do another lookup. This is quite innefficient. We should consider
> return a pdev and use it for vcpi_{read,write}.

Hm. Right. Indeed, a future consideration.

>> +    {
>> +        if ( pdev->vpci && (pdev->vpci->guest_sbdf.sbdf == sbdf->sbdf) )
>> +        {
>> +            /* Replace guest SBDF with the physical one. */
>> +            *sbdf = pdev->sbdf;
>> +            read_unlock(&d->pci_lock);
>> +            return true;
>> +        }
>> +    }
>> +
>> +    read_unlock(&d->pci_lock);
> 
> At the point this is unlocked, what prevent the sbdf to be detached from
> the domain?

Nothing.

> If nothing, would this mean the domain can access something it should
> not?

Yep. I have a patch in the works to acquire the lock in the I/O
handlers. I would prefer doing this in a pre-patch so that we don't
temporarily introduce the race condition.

>> +    return false;> +}
>> +
>>   #endif /* CONFIG_HAS_VPCI_GUEST_SUPPORT */
>>     void vpci_deassign_device(struct pci_dev *pdev)
>> diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
>> index 807401b2eaa2..e355329913ef 100644
>> --- a/xen/include/xen/vpci.h
>> +++ b/xen/include/xen/vpci.h
>> @@ -311,6 +311,18 @@ static inline int __must_check vpci_reset_device(struct 
>> pci_dev *pdev)
>>       return vpci_assign_device(pdev);
>>   }
>>   +#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
>> +bool vpci_translate_virtual_device(struct domain *d, pci_sbdf_t *sbdf);
>> +#else
>> +static inline bool vpci_translate_virtual_device(struct domain *d,
>> +                                                 pci_sbdf_t *sbdf)
>> +{
>> +    ASSERT_UNREACHABLE();
>> +
>> +    return false;
>> +}
>> +#endif
>> +
>>   #endif
>>     /*