Re: [PATCH v2] pci: Assert that capabilities never overlap

[Akihiko Odaki]

On Mon, Sep 5, 2022 at 6:26 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> Akihiko Odaki <akihiko.odaki@daynix.com> writes:
>
> > On Fri, Sep 2, 2022 at 7:23 PM Markus Armbruster <armbru@redhat.com> wrote:
> >>
> >> Akihiko Odaki <akihiko.odaki@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.odaki@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