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

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

[Niklas Cassel]

syzbot force-bound ata_generic to 0000:00:03.0 on a QEMU arm64 virt
machine. That device is a virtio-blk-pci device holding the root file
system, and it reports PCI class 0x010000, i.e. PCI_CLASS_STORAGE_SCSI.

QEMU gives the virtio-blk-pci device a legacy virtio I/O BAR0 and a 4 KiB
MSI-X BAR1, so both resources are non-empty, the port is not discarded,
and the device control register ends up in the middle of the MSI-X table.

The emulated device rejects the byte write, and arm64 reports the
resulting bus error as a fatal synchronous external abort:

  Internal error: synchronous external abort: 0000000096000050 [#1]  SMP
  pc : ata_sff_freeze+0x7c/0x90 drivers/ata/libata-sff.c:1606
  Call trace:
   ata_sff_freeze+0x7c/0x90
   ata_eh_freeze_port+0x34/0x5c
   ata_host_start+0x13c/0x228
   ata_pci_sff_activate_host+0x50/0x340
   ata_pci_init_one+0x19c/0x1d8
   ata_pci_bmdma_init_one+0x14/0x20
   ata_generic_init_one+0xc4/0x1ac
   local_pci_probe+0x40/0xa8
   pci_device_probe+0xd8/0x288
   really_probe+0xbc/0x2bc
   device_driver_attach+0x48/0xb4
   bind_store+0x7c/0xd8

Refuse devices which neither report the IDE class nor appear in our ID
table. Table entries keep binding as before, because some of the listed
controllers cannot be assumed to report the IDE class. A controller which
needs ata_generic but does not report the IDE class should get an ID table
entry, which is what the table is for.

Binding a driver to unrelated hardware requires root and is what
driver_override is meant to do, so this does not fix a privilege boundary.

This change only stops ata_generic from binding to a PCI device which it
has no reason to believe to be an IDE controller.

Reported-by: syzbot+891c7b195b408052e519@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/linux-ide/6a82bc54.10853dc7.22f513.001b.GAE@google.com/
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/ata/ata_generic.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/ata/ata_generic.c b/drivers/ata/ata_generic.c
index e70b6c089cf1..f57db4480ad3 100644
--- a/drivers/ata/ata_generic.c
+++ b/drivers/ata/ata_generic.c
@@ -150,6 +150,9 @@ static int is_intel_ider(struct pci_dev *dev)
 	return 1;
 }
 
+/* Forward declaration for the pci_match_id() call in ata_generic_init_one() */
+static const struct pci_device_id ata_generic[];
+
 /**
  *	ata_generic_init_one		-	attach generic IDE
  *	@dev: PCI device found
@@ -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;
+
 	/* Don't use the generic entry unless instructed to do so */
 	if ((id->driver_data & ATA_GEN_CLASS_MATCH) && all_generic_ide == 0)
 		return -ENODEV;
-- 
2.55.0

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

[sashiko-bot]

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 <cassel@kernel.org>

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/6a82bc54.10853dc7.22f513.001b.GAE@google.com/

> 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/20260818094205.2672967-2-cassel@kernel.org?part=1

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

[Damien Le Moal]

On 8/18/26 18:42, Niklas Cassel wrote:
> syzbot force-bound ata_generic to 0000:00:03.0 on a QEMU arm64 virt
> machine. That device is a virtio-blk-pci device holding the root file
> system, and it reports PCI class 0x010000, i.e. PCI_CLASS_STORAGE_SCSI.
> 
> QEMU gives the virtio-blk-pci device a legacy virtio I/O BAR0 and a 4 KiB
> MSI-X BAR1, so both resources are non-empty, the port is not discarded,
> and the device control register ends up in the middle of the MSI-X table.
> 
> The emulated device rejects the byte write, and arm64 reports the
> resulting bus error as a fatal synchronous external abort:
> 
>   Internal error: synchronous external abort: 0000000096000050 [#1]  SMP
>   pc : ata_sff_freeze+0x7c/0x90 drivers/ata/libata-sff.c:1606
>   Call trace:
>    ata_sff_freeze+0x7c/0x90
>    ata_eh_freeze_port+0x34/0x5c
>    ata_host_start+0x13c/0x228
>    ata_pci_sff_activate_host+0x50/0x340
>    ata_pci_init_one+0x19c/0x1d8
>    ata_pci_bmdma_init_one+0x14/0x20
>    ata_generic_init_one+0xc4/0x1ac
>    local_pci_probe+0x40/0xa8
>    pci_device_probe+0xd8/0x288
>    really_probe+0xbc/0x2bc
>    device_driver_attach+0x48/0xb4
>    bind_store+0x7c/0xd8
> 
> Refuse devices which neither report the IDE class nor appear in our ID
> table. Table entries keep binding as before, because some of the listed
> controllers cannot be assumed to report the IDE class. A controller which
> needs ata_generic but does not report the IDE class should get an ID table
> entry, which is what the table is for.
> 
> Binding a driver to unrelated hardware requires root and is what
> driver_override is meant to do, so this does not fix a privilege boundary.
> 
> This change only stops ata_generic from binding to a PCI device which it
> has no reason to believe to be an IDE controller.
> 
> Reported-by: syzbot+891c7b195b408052e519@syzkaller.appspotmail.com
> Closes: https://lore.kernel.org/linux-ide/6a82bc54.10853dc7.22f513.001b.GAE@google.com/
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

Looks sensible to me, and very surprising that this problem was not cought before...

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

-- 
Damien Le Moal
Western Digital Research

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

[Niklas Cassel]

On Tue, Aug 18, 2026 at 11:42:05AM +0200, Niklas Cassel wrote:
> syzbot force-bound ata_generic to 0000:00:03.0 on a QEMU arm64 virt
> machine. That device is a virtio-blk-pci device holding the root file
> system, and it reports PCI class 0x010000, i.e. PCI_CLASS_STORAGE_SCSI.
>
> QEMU gives the virtio-blk-pci device a legacy virtio I/O BAR0 and a 4 KiB
> MSI-X BAR1, so both resources are non-empty, the port is not discarded,
> and the device control register ends up in the middle of the MSI-X table.
>
> The emulated device rejects the byte write, and arm64 reports the
> resulting bus error as a fatal synchronous external abort:
>
>   Internal error: synchronous external abort: 0000000096000050 [#1]  SMP
>   pc : ata_sff_freeze+0x7c/0x90 drivers/ata/libata-sff.c:1606
>   Call trace:
>    ata_sff_freeze+0x7c/0x90
>    ata_eh_freeze_port+0x34/0x5c
>    ata_host_start+0x13c/0x228
>    ata_pci_sff_activate_host+0x50/0x340
>    ata_pci_init_one+0x19c/0x1d8
>    ata_pci_bmdma_init_one+0x14/0x20
>    ata_generic_init_one+0xc4/0x1ac
>    local_pci_probe+0x40/0xa8
>    pci_device_probe+0xd8/0x288
>    really_probe+0xbc/0x2bc
>    device_driver_attach+0x48/0xb4
>    bind_store+0x7c/0xd8
>
> Refuse devices which neither report the IDE class nor appear in our ID
> table. Table entries keep binding as before, because some of the listed
> controllers cannot be assumed to report the IDE class. A controller which
> needs ata_generic but does not report the IDE class should get an ID table
> entry, which is what the table is for.
>
> Binding a driver to unrelated hardware requires root and is what
> driver_override is meant to do, so this does not fix a privilege boundary.
>
> This change only stops ata_generic from binding to a PCI device which it
> has no reason to believe to be an IDE controller.
>
> Reported-by: syzbot+891c7b195b408052e519@syzkaller.appspotmail.com
> Closes: https://lore.kernel.org/linux-ide/6a82bc54.10853dc7.22f513.001b.GAE@google.com/
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
> ---
>  drivers/ata/ata_generic.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/drivers/ata/ata_generic.c b/drivers/ata/ata_generic.c
> index e70b6c089cf1..f57db4480ad3 100644
> --- a/drivers/ata/ata_generic.c
> +++ b/drivers/ata/ata_generic.c
> @@ -150,6 +150,9 @@ static int is_intel_ider(struct pci_dev *dev)
>	return 1;
>  }
>
> +/* Forward declaration for the pci_match_id() call in ata_generic_init_one() */
> +static const struct pci_device_id ata_generic[];
> +
>  /**
>   *	ata_generic_init_one		-	attach generic IDE
>   *	@dev: PCI device found
> @@ -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;
> +
>	/* Don't use the generic entry unless instructed to do so */
>	if ((id->driver_data & ATA_GEN_CLASS_MATCH) && all_generic_ide == 0)
>		return -ENODEV;
> --
> 2.55.0
>

Another suggested patch is to something like:


--- a/drivers/ata/libata-sff.c
+++ b/drivers/ata/libata-sff.c
@@ -2115,6 +2115,14 @@ static bool ata_resources_present(struct pci_dev *pdev, int port)
	/* Check the PCI resources for this channel are enabled */
	port *= 2;
	for (i = 0; i < 2; i++) {
+               /*
+                * The command and control blocks of a native mode PCI IDE
+                * controller are I/O resources, as are the legacy mode
+                * resources which the PCI core fabricates for compatibility
+                * mode controllers.  A memory resource is not a taskfile.
+                */
+               if (!(pci_resource_flags(pdev, port + i) & IORESOURCE_IO))
+                       return false;

Since IDE controllers always have BAR0 and BAR1 as IORESOURCE_IO and not
IORESOURCE_MEM.

Verified that:
ata_generic, ata_piix, pata_rdc and the ~40 other pata_* PCI drivers,
plus sata_nv, sata_sis, sata_uli, sata_via (vt6420/vt8251) all use I/O BARs
for the register blocks.


That would still allow someone to use driver_override to bind a PCI device
that has class code != PCI_CLASS_STORAGE_IDE, but would still fail e.g.
a virtio-pci device, which has the BARs of type MEM and not type I/O.


Kind regards,
Niklas

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

[Niklas Cassel]

On Tue, Aug 18, 2026 at 06:52:45PM +0900, Damien Le Moal wrote:
> On 8/18/26 18:42, Niklas Cassel wrote:
> > syzbot force-bound ata_generic to 0000:00:03.0 on a QEMU arm64 virt
> > machine. That device is a virtio-blk-pci device holding the root file
> > system, and it reports PCI class 0x010000, i.e. PCI_CLASS_STORAGE_SCSI.
> > 
> > QEMU gives the virtio-blk-pci device a legacy virtio I/O BAR0 and a 4 KiB
> > MSI-X BAR1, so both resources are non-empty, the port is not discarded,
> > and the device control register ends up in the middle of the MSI-X table.
> > 
> > The emulated device rejects the byte write, and arm64 reports the
> > resulting bus error as a fatal synchronous external abort:
> > 
> >   Internal error: synchronous external abort: 0000000096000050 [#1]  SMP
> >   pc : ata_sff_freeze+0x7c/0x90 drivers/ata/libata-sff.c:1606
> >   Call trace:
> >    ata_sff_freeze+0x7c/0x90
> >    ata_eh_freeze_port+0x34/0x5c
> >    ata_host_start+0x13c/0x228
> >    ata_pci_sff_activate_host+0x50/0x340
> >    ata_pci_init_one+0x19c/0x1d8
> >    ata_pci_bmdma_init_one+0x14/0x20
> >    ata_generic_init_one+0xc4/0x1ac
> >    local_pci_probe+0x40/0xa8
> >    pci_device_probe+0xd8/0x288
> >    really_probe+0xbc/0x2bc
> >    device_driver_attach+0x48/0xb4
> >    bind_store+0x7c/0xd8
> > 
> > Refuse devices which neither report the IDE class nor appear in our ID
> > table. Table entries keep binding as before, because some of the listed
> > controllers cannot be assumed to report the IDE class. A controller which
> > needs ata_generic but does not report the IDE class should get an ID table
> > entry, which is what the table is for.
> > 
> > Binding a driver to unrelated hardware requires root and is what
> > driver_override is meant to do, so this does not fix a privilege boundary.
> > 
> > This change only stops ata_generic from binding to a PCI device which it
> > has no reason to believe to be an IDE controller.
> > 
> > Reported-by: syzbot+891c7b195b408052e519@syzkaller.appspotmail.com
> > Closes: https://lore.kernel.org/linux-ide/6a82bc54.10853dc7.22f513.001b.GAE@google.com/
> > Signed-off-by: Niklas Cassel <cassel@kernel.org>
> 
> Looks sensible to me, and very surprising that this problem was not cought before...
> 
> Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

I mean, syzbot using driver_override like this is kind of a stupid test.

If you try to use the generic ATA driver for a PCI device that is something
completely different, e.g. an Ethernet controller... you kind of get what
you asked for :)

But it is such a small patch, so I think it makes sense, even if all it
does it to stop us from getting more silly reports like this.


Tell me if you prefer the other patch, which checks BAR type instead.
BAR type I/O is very rare, since it is old x86 legacy.
(But such a patch would still allow someone to bind a PCI device with
BARs marked as type I/O, even if that PCI device does not have storage
class IDE.)


I think the main argument for the patch in $subject... is:
if your PCI device is not class IDE... just add an entry to the match table.
(I.e. we don't care that driver_override will not work with a device that
does not have class IDE... If you really want the driver to work with such
a device, you will have to add an explicit entry to the match table.)


Kind regards,
Niklas