On Mon, Sep 5, 2022 at 6:26 PM Markus Armbruster <arm...@redhat.com> wrote: > > 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? >
When the capabilities overlap, vfio_std_cap_max_size() and vfio_ext_cap_max_size(), called by vfio_add_std_cap(), vfio_add_ext_cap() should clip the size of capabilities. Comments in vfio_add_std_cap() and vfio_add_ext_cap() say: "Since QEMU doesn't actually handle many of the config accesses, exact size doesn't seem worthwhile." Regards, Akihiko Odaki
