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
>> /*
