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

[Markus Armbruster]

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?