Re: [PATCH] ata: ata_generic: Do not bind to devices that are not IDE controllers

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Preexisting Use-After-Free in `generic_set_mode()` when an IDE device is bound via sysfs `new_id` and the ID is later removed.
--

--- Patch [1]: [PATCH] ata: ata_generic: Do not bind to devices that are not IDE controllers ---
commit 6571983234fdb7767d7bb9ad7f55e962d8eec259
Author: Niklas Cassel <[email protected]>

ata: ata_generic: Do not bind to devices that are not IDE controllers

This commit prevents ata_generic from binding to devices that are not IDE
controllers unless explicitly listed in the ID table. It resolves a fatal
synchronous external abort caused by force-binding to a virtio-blk-pci
device.

Link: https://lore.kernel.org/linux-ide/[email protected]/

> diff --git a/drivers/ata/ata_generic.c b/drivers/ata/ata_generic.c
> index e70b6c089cf17..f57db4480ad3f 100644
> --- a/drivers/ata/ata_generic.c
> +++ b/drivers/ata/ata_generic.c
[ ... ]
> @@ -172,6 +175,19 @@ static int ata_generic_init_one(struct pci_dev *dev, const struct pci_device_id
>  	};
>  	const struct ata_port_info *ppi[] = { &info, NULL };
>  
> +	/*
> +	 * A device matched through driver_override or through an ID added
> +	 * with new_id does not come from our ID table, so pci_match_device()
> +	 * hands us a synthetic ID with no driver_data and none of the checks
> +	 * below apply. Probing maps BAR0 and BAR1 as the ATA command and
> +	 * control blocks and writes to the device control register, so only
> +	 * continue for devices which report the IDE class or which we list
> +	 * ourselves.
> +	 */
> +	if (!pci_match_id(ata_generic, dev) &&
> +	    (dev->class >> 8) != PCI_CLASS_STORAGE_IDE)
> +		return -ENODEV;
> +

[Severity: High]
This is a pre-existing issue, but does this bypass still leave the driver
vulnerable to a use-after-free when a dynamic ID is removed?

If a device with PCI_CLASS_STORAGE_IDE is bound via the sysfs new_id
mechanism, the PCI core passes a dynamically allocated struct pci_device_id
pointer to ata_generic_init_one(). 

This bypasses the new -ENODEV check and eventually passes the dynamic ID
pointer down to be stored in the host private data:

drivers/ata/ata_generic.c:ata_generic_init_one() {
    ...
    return ata_pci_bmdma_init_one(dev, ppi, &generic_sht, (void *)id, 0);
}

If root later writes to the remove_id sysfs attribute, the PCI core frees
the dynamic ID without unbinding the device. Subsequently, if an ATA link
reset occurs, the freed memory is dereferenced:

drivers/ata/ata_generic.c:generic_set_mode() {
    ...
    const struct pci_device_id *id = ap->host->private_data;
    ...
    if (id->driver_data & ATA_GEN_FORCE_DMA) {
    ...
}

Could the driver avoid storing the synthetic ID in private data, or copy
the ID contents to prevent accessing freed memory?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1