Re: [PATCH v2 1/7] ata: don't keep pci_device_id

["Gary Guo" <gary@garyguo.net>]

On Tue Jun 30, 2026 at 12:59 PM BST, Niklas Cassel wrote:
> Hello Gary,
>
> On Tue, Jun 30, 2026 at 12:09:01PM +0100, Gary Guo wrote:
>> pci_device_id is not guaranteed to live longer than probe due to presence
>> of dynamic ID. All information apart from driver_data can be easily
>> retrieved from pci_dev, so just store driver_data.
>> 
>> Signed-off-by: Gary Guo <gary@garyguo.net>
>
> Please write a proper commit message.
>
> The commit message should be detailed enough for someone to realize what
> is going on without reading your cover-letter (as information in the cover
> letter in not part of the accepted commit).
>
> 1) Explain how to reproduce.
>
> 2) Explain the problem.
>
> 3) Explain the consequences of the problem. UAF? Crash?
>
> 4) Explain how you fix it.

Hi Niklas,

I see this as a contract mismatch between pci core and drivers, hence the commit
message just mentions the problem (lifetime of pci_device_id pointer is
restricted to probe only) and the fix (don't store it).

Currently as you said, the way that this becomes a problem is when dynamic ID is
involved. So the following sequence will cause issue:

    echo "vendor device" > /sys/bus/pci/drivers/your_driver/new_id
    # PCI core calls probe which stores the ID (e.g. ata)
    echo "vendor device" > /sys/bus/pci/drivers/your_driver/remove_id
    # Driver uses the stored ID (UAF)

However, the gist here is that due to the presence of dynamic ID, pci_device_id
in probe is not guaranteed to live longer than the probe function (in fact, it
currently is not guaranteed to be alive at all, which is what this series is
trying to address).

Exactly how long the ID is going to live should be up to the PCI core and be
transparent to drivers, so I intentionally left this out from driver fix
patches, this should be implementation detail of PCI core. In fact, in patch 7
I changed to be unconditionally invalid upon return regardless if it is dynamic
ID or not.

At the end of this series I changed the documentation to explicitly state this
contract. So even without having the reproducer, the commit message still makes
sense because it fixes a contract violation and reader can connect it with the
documentation.

Best,
Gary

>
>
> AFAICT, this is somehow related to pci_add_dynid(), which is called when
> user-space is doing something like:
>
> $ echo "vendor device" > /sys/bus/pci/drivers/your_driver/new_id
>
>
> Kind regards,
> Niklas