Akihiko Odaki <akihiko.od...@daynix.com> writes: > On Fri, Sep 2, 2022 at 7:23 PM Markus Armbruster <arm...@redhat.com> wrote: >> >> Akihiko Odaki <akihiko.od...@daynix.com> writes: >> >> > pci_add_capability appears most PCI devices. Its error handling required >> > lots of code, and led to inconsistent behaviors such as: >> > - passing error_abort >> > - passing error_fatal >> > - asserting the returned value >> > - propagating the error to the caller >> > - skipping the rest of the function >> > - just ignoring >> > >> > The code generating errors in pci_add_capability had a comment which >> > says: >> >> Verify that capabilities don't overlap. Note: device assignment >> >> depends on this check to verify that the device is not broken. >> >> Should never trigger for emulated devices, but it's helpful for >> >> debugging these. >> > >> > Indeed vfio has some code that passes capability offsets and sizes from >> > a physical device, but it explicitly pays attention so that the >> > capabilities never overlap. >> >> I can't see that at a glance. Can you give me a clue? >> >> > Therefore, we can always assert that >> > capabilities never overlap when pci_add_capability is called, resolving >> > these inconsistencies. >> > >> > Signed-off-by: Akihiko Odaki <akihiko.od...@daynix.com> >> > > Looking at vfio_add_std_cap(), and vfio_add_ext_cap() it seems that > they are clipping the size of capabilities so that they do not > overlap, if I read it correctly.
If we want to deal gracefully with buggy physical devices, we need to treat pdev->config[] as untrusted input. As far as I can tell: * vfio_add_capabilities() replicates the physical device's capabilities (starting at pdev->config[PCI_CAPABILITY_LIST]) in the virtual device. * vfio_add_std_cap() is a helper to add the tail starting at pdev->config[pos]. Could the physical device's capabilities overlap? If yes, what would happen before and after your series?
