ata: libata-core: Ignore forced transfer modes the host does not support
PATCH 1/1 ata: libata-core: Ignore forced transfer modes the host does not support
The libata.force parameter can force a transfer mode (pio0-6, mwdma0-4, udma0-7) for a port or device. ata_force_xfermask() applies it in ata_do_set_mode() right after ata_dev_xfermask() has limited the device masks to what the host supports. It replaces those masks with the forced mode without checking the host masks, so a mode the controller does not support is passed to the driver's ->set_piomode() or ->set_dmamode().
Several PATA drivers rely on the host masks to keep the mode number in range when indexing their timing tables. ata_piix, pata_efar, pata_it8213, pata_oldpiix, pata_radisys and pata_rdc declare ATA_PIO4 and ATA_MWDMA12_ONLY, index a five-entry PIO timing table with pio_mode - XFER_PIO_0, and index a three-entry needed_pio[] table with dma_mode - XFER_MW_DMA_0. Booting with libata.force=pio5, pio6, mwdma3 or mwdma4 makes them read past those tables, use the values read as further indices into the timing table, and write the result to the controller's timing registers.
Make ata_force_xfermask() ignore a forced transfer mode that is not in the port's PIO, MWDMA or UDMA mask, and warn about it. Forcing a mode the host supports still works as before, including a mode that ata_dev_xfermask() removed for the device or cable.
Found by Linux Verification Center (linuxtesting.org) with SVACE.
--- drivers/ata/libata-core.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
drivers/ata/libata-core.c +12 -0
@@ -434,6 +434,7 @@ static void ata_force_link_limits(struct ata_link *link)*/ static void ata_force_xfermask (struct ata_device *dev) { +struct ata_port *ap = dev->link->ap;int devno = dev->link->pmp + dev->devno;int alt_devno = devno;int i;
@@ -458,6 +459,17 @@ static void ata_force_xfermask(struct ata_device *dev)ata_unpack_xfermask (fe->param.xfer_mask, &pio_mask, &mwdma_mask, &udma_mask); + +/* Never force a transfer mode the host does not support */ +if ((pio_mask & ~ap->pio_mask) || + (mwdma_mask & ~ap->mwdma_mask) || + (udma_mask & ~ap->udma_mask)) { +ata_dev_warn (dev, +"FORCE: %s not supported by the host, ignored\n" , + fe->param.name); +return ; + } +if (udma_mask) dev->udma_mask = udma_mask;else if (mwdma_mask) {
base-commit: 4056b3c9d3c92ea45955641dbafeb9b960e146ef
Signature · 1 line
2.39.5
[email protected]
AI reviewThank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] ata: libata-core: early return bypasses valid fallback force configs --
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] ata: libata-core: early return bypasses valid fallback force configs --
commit b2b8699db9f1c2c52ee37c5d556e07a3dde1ab2e
Author: Rand Deeb <[email protected]>
ata: libata-core: Ignore forced transfer modes the host does not support
This commit adds a check to ignore forced transfer modes that the host controller does not support. This prevents out-of-bounds reads in driver timing tables when an unsupported mode is forced.
Quoted 7 lines
drivers/ata/libata-core.c +0
-0@@ -458,6 +459,17 @@ static void ata_force_xfermask(struct ata_device *dev) ata_unpack_xfermask(fe->param.xfer_mask,
drivers/ata/libata-core.c static void ata_force_xfermask(struct ata_device *dev) · line 470
Quoted 10 lines
drivers/ata/libata-core.c +9
-0&pio_mask, &mwdma_mask, &udma_mask); + +/* Never force a transfer mode the host does not support */ +if ((pio_mask & ~ap->pio_mask) || + (mwdma_mask & ~ap->mwdma_mask) || + (udma_mask & ~ap->udma_mask)) { +ata_dev_warn (dev, +"FORCE: %s not supported by the host, ignored\n" , + fe->param.name); +return ;
[Severity: Medium]
When a specific override is rejected by this new host capability check in ata_force_xfermask(), does using return prematurely abort the loop?
The libata.force parameter allows stacking multiple hierarchical overrides (e.g., libata.force=pio4,1.0:pio5), which are evaluated backwards so the most specific match wins.
By returning instead of using continue, the code appears to silently bypass earlier valid fallback overrides like the port-wide pio4 in the example above.
This would cause the device to fall back to its auto-negotiated mask instead of the valid user-specified fallback, which might cause issues if the user was relying on the fallback constraint to disable broken DMA.
Quoted 5 lines
drivers/ata/libata-core.c +2
-0+ } +if (udma_mask) dev->udma_mask = udma_mask;else if (mwdma_mask) {
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
Damien Le Moal
maintainerQuoted 46 lines · Rand Deeb
The libata.force parameter can force a transfer mode (pio0-6, mwdma0-4, udma0-7) for a port or device.
ata_force_xfermask()applies it inata_do_set_mode()right afterata_dev_xfermask()has limited the device masks to what the host supports. It replaces those masks with the forced mode without checking the host masks, so a mode the controller does not support is passed to the driver's ->set_piomode() or ->set_dmamode().Several PATA drivers rely on the host masks to keep the mode number in range when indexing their timing tables.
ata_piix,pata_efar,pata_it8213,pata_oldpiix,pata_radisysandpata_rdcdeclareATA_PIO4andATA_MWDMA12_ONLY, index a five-entry PIO timing table withpio_mode-XFER_PIO_0, and index a three-entry needed_pio[] table withdma_mode-XFER_MW_DMA_0. Booting with libata.force=pio5, pio6, mwdma3 or mwdma4 makes them read past those tables, use the values read as further indices into the timing table, and write the result to the controller's timing registers.Make
ata_force_xfermask()ignore a forced transfer mode that is not in the port's PIO, MWDMA or UDMA mask, and warn about it. Forcing a mode the host supports still works as before, including a mode thatata_dev_xfermask()removed for the device or cable.Found by Linux Verification Center (linuxtesting.org) with SVACE.
Fixes 332673257056 ("libata: implement libata.force module parameter")Signed-off-by Rand Deeb <[email protected]>--- drivers/ata/libata-core.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)drivers/ata/libata-core.c +1
-0@@ -434,6 +434,7 @@ static void ata_force_link_limits(struct ata_link *link)*/ static void ata_force_xfermask (struct ata_device *dev) { +struct ata_port *ap = dev->link->ap;int devno = dev->link->pmp + dev->devno;int alt_devno = devno;int i;@@ -458,6 +459,17 @@ static void ata_force_xfermask(struct ata_device *dev)ata_unpack_xfermask (fe->param.xfer_mask, &pio_mask, &mwdma_mask, &udma_mask);
drivers/ata/libata-core.c static void ata_force_xfermask(struct ata_device *dev) · line 471
Quoted 10 lines
drivers/ata/libata-core.c +10
-0+ +/* Never force a transfer mode the host does not support */ +if ((pio_mask & ~ap->pio_mask) || + (mwdma_mask & ~ap->mwdma_mask) || + (udma_mask & ~ap->udma_mask)) { +ata_dev_warn (dev, +"FORCE: %s not supported by the host, ignored\n" , + fe->param.name); +return ; + }
I would rewrite this as:
/* Make sure that the port supports the specified xfer modes. */
if (!(pio_mask & ap->pio_mask) &&
!(mwdma_mask & ap->mwdma_mask) &&
!(udma_mask & ap->udma_mask)) {
ata_dev_warn(dev,
"FORCE: ignoring unsupported xfer_mask %s\n",
fe->param.name);
continue;
}as that is easier to understand. The "continue" here should address Sashiko comment.
Quoted 6 lines
drivers/ata/libata-core.c +1
-0+if (udma_mask) dev->udma_mask = udma_mask;else if (mwdma_mask) {base-commit:
4056b3c9d3c92ea45955641dbafeb9b960e146ef
Signature · 2 lines
Damien Le Moal Western Digital Research