ata: libata-sata: fix ata_scsi_lpm_supported() iteration

PATCH Applied Niklas Cassel 3 participants 6 messages Jul 28 – Jul 29

Subsystems (2)

Stable branch libata

lore

Share & tools

lore

Niklas Cassel

author maintainer

2026-07-28 11:38 UTC #0 lore raw

PATCH 1/1 ata: libata-sata: fix ata_scsi_lpm_supported() iteration

The inner loop of ata_scsi_lpm_supported() user the wrong variable when iterating.

It should obviously use the link that we are currently iterating over, rather than always using the host link.

ata_scsi_lpm_supported() is used to control if a user should be allowed to change lpm policy (from the default) via sysfs.

Thus, this bug could potentially disallow users to change the LPM policy for certain SATA devices via sysfs.

Cc [email protected]

Fixes 0060beec0bfa ("ata: libata-sata: Add link_power_management_supported sysfs attribute")

Reported-by Sashiko <[email protected]>

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

Signed-off-by Niklas Cassel <[email protected]>

---
 drivers/ata/libata-sata.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/ata/libata-sata.c +1 -1

index 2ee54d60ea4b..5138b4e500e5 100644
@@ -913,7 +913,7 @@ static bool ata_scsi_lpm_supported(struct ata_port *ap)
 		return false;
 
 	ata_for_each_link(link, ap, EDGE) {
-		ata_for_each_dev(dev, &ap->link, ENABLED) {
+		ata_for_each_dev(dev, link, ENABLED) {
 			if (dev->quirks & ATA_QUIRK_NOLPM)
 				return false;
 		}

Signature · 1 line

2.55.0

Niklas Cassel

author maintainer

2026-07-28 11:40 UTC #1 lore raw

Quoted 1 line · Niklas Cassel

The inner loop of ata_scsi_lpm_supported() user the wrong variable when

s/user/uses/

[email protected]

AI review

2026-07-28 11:53 UTC #2 lore raw

In reply to Niklas Cassel

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

Pre-existing issues:
- [High] ata_scsi_lpm_supported() skips checking devices on the slave link because it uses the ATA_LITER_EDGE iterator. This allows userspace to enable Link Power Management (LPM) on a port where the slave device does not support it (has ATA_QUIRK_NOLPM). --

--- Patch [1]: [PATCH] ata: libata-sata: fix ata_scsi_lpm_supported() iteration --- commit 32bd728de2e1b6025504205f3ade16813231e703
Author: Niklas Cassel <[email protected]>

ata: libata-sata: fix ata_scsi_lpm_supported() iteration

This commit fixes the inner loop of ata_scsi_lpm_supported() to iterate over the specific link being evaluated rather than always using the host link. This prevents the LPM policy from being incorrectly applied or denied.

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

drivers/ata/libata-sata.c static bool ata_scsi_lpm_supported(struct ata_port *ap) · line 916

Quoted 10 lines

drivers/ata/libata-sata.c +1 -1

index b0706c30da052..ad40f516c6d43 100644
@@ -913,7 +913,7 @@ static bool ata_scsi_lpm_supported(struct ata_port *ap)
 		return false;
 
 	ata_for_each_link(link, ap, EDGE) {
-		ata_for_each_dev(dev, &ap->link, ENABLED) {
+		ata_for_each_dev(dev, link, ENABLED) {

[Severity: High]
This is a pre-existing issue, but does using the ATA_LITER_EDGE iterator here in ata_scsi_lpm_supported() skip checking devices on the slave link?

Since the EDGE iterator skips ap->slave_link, it appears this loop misses devices on the slave link that might have the ATA_QUIRK_NOLPM quirk.

If a user or power-management tool writes to the sysfs attribute link_power_management_policy, ata_scsi_lpm_supported() would incorrectly return true. Then ata_port_schedule_eh() triggers Error Handling, and ata_eh_link_set_lpm() might blindly apply the unsupported policy to the slave link.

Can this result in the slave drive becoming unresponsive, leading to command timeouts or dropped devices?

Quoted 3 lines

drivers/ata/libata-sata.c +0 -0

 			if (dev->quirks & ATA_QUIRK_NOLPM)
 				return false;
 		}

Signature · 1 line

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

Niklas Cassel

author maintainer

2026-07-28 12:16 UTC #3 lore raw

Quoted 33 lines · On Tue, Jul 28, 2026 at 11:53:36AM +0000, [email protected] wrote:

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] ata_scsi_lpm_supported() skips checking devices on the slave link because it uses the ATA_LITER_EDGE iterator. This allows userspace to enable Link Power Management (LPM) on a port where the slave device does not support it (has ATA_QUIRK_NOLPM). --
--- Patch [1]: [PATCH] ata: libata-sata: fix ata_scsi_lpm_supported() iteration --- commit 32bd728de2e1b6025504205f3ade16813231e703
Author: Niklas Cassel <[email protected]>
ata: libata-sata: fix ata_scsi_lpm_supported() iteration
This commit fixes the inner loop of ata_scsi_lpm_supported() to iterate over the specific link being evaluated rather than always using the host link. This prevents the LPM policy from being incorrectly applied or denied.
Link https://lore.kernel.org/linux-ide/[email protected]/ open here
drivers/ata/libata-sata.c static bool ata_scsi_lpm_supported(struct ata_port *ap) · line 916
Quoted 10 lines
drivers/ata/libata-sata.c +1 -1
index b0706c30da052..ad40f516c6d43 100644
@@ -913,7 +913,7 @@ static bool ata_scsi_lpm_supported(struct ata_port *ap)
 		return false;
 
 	ata_for_each_link(link, ap, EDGE) {
-		ata_for_each_dev(dev, &ap->link, ENABLED) {
+		ata_for_each_dev(dev, link, ENABLED) {
[Severity: High]
This is a pre-existing issue, but does using the ATA_LITER_EDGE iterator here in ata_scsi_lpm_supported() skip checking devices on the slave link?

Quoted 2 lines

Since the EDGE iterator skips ap->slave_link, it appears this loop misses devices on the slave link that might have the ATA_QUIRK_NOLPM quirk.

Well,

1) slave_link is only for PATA.

Yes, I know that two sata drivers abuse this:

drivers/ata/sata_sis.c:                 rc = ata_slave_link_init(ap);
drivers/ata/sata_via.c:         ata_slave_link_init(host->ports[i]);

2) The call to ata_eh_link_set_lpm() is protected by:

libata-eh.c:ata_eh_recover()

if (link->lpm_policy != ap->target_lpm_policy) {
	rc = ata_eh_link_set_lpm(link, ap->target_lpm_policy, &dev);
}

And the only driver that sets ap->target_lpm_policy is AHCI.

So, I don't think we need to care about the slave link in this case.

Kind regards,
Niklas

Niklas Cassel

author maintainer

2026-07-28 12:37 UTC #4 lore raw

Quoted 50 lines · Niklas Cassel

Quoted 33 lines · On Tue, Jul 28, 2026 at 11:53:36AM +0000, [email protected] wrote:
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] ata_scsi_lpm_supported() skips checking devices on the slave link because it uses the ATA_LITER_EDGE iterator. This allows userspace to enable Link Power Management (LPM) on a port where the slave device does not support it (has ATA_QUIRK_NOLPM). --
--- Patch [1]: [PATCH] ata: libata-sata: fix ata_scsi_lpm_supported() iteration --- commit 32bd728de2e1b6025504205f3ade16813231e703
Author: Niklas Cassel <[email protected]>
ata: libata-sata: fix ata_scsi_lpm_supported() iteration
This commit fixes the inner loop of ata_scsi_lpm_supported() to iterate over the specific link being evaluated rather than always using the host link. This prevents the LPM policy from being incorrectly applied or denied.
Link https://lore.kernel.org/linux-ide/[email protected]/ open here
drivers/ata/libata-sata.c static bool ata_scsi_lpm_supported(struct ata_port *ap) · line 916
Quoted 10 lines
drivers/ata/libata-sata.c +1 -1
index b0706c30da052..ad40f516c6d43 100644
@@ -913,7 +913,7 @@ static bool ata_scsi_lpm_supported(struct ata_port *ap)
 		return false;
 
 	ata_for_each_link(link, ap, EDGE) {
-		ata_for_each_dev(dev, &ap->link, ENABLED) {
+		ata_for_each_dev(dev, link, ENABLED) {
[Severity: High]
This is a pre-existing issue, but does using the ATA_LITER_EDGE iterator here in ata_scsi_lpm_supported() skip checking devices on the slave link?
Quoted 2 lines
Since the EDGE iterator skips ap->slave_link, it appears this loop misses devices on the slave link that might have the ATA_QUIRK_NOLPM quirk.
Well,
1) slave_link is only for PATA.
Yes, I know that two sata drivers abuse this:
drivers/ata/sata_sis.c:                 rc = ata_slave_link_init(ap);
drivers/ata/sata_via.c:         ata_slave_link_init(host->ports[i]);
2) The call to ata_eh_link_set_lpm() is protected by:
libata-eh.c:ata_eh_recover()

Quoted 7 lines

if (link->lpm_policy != ap->target_lpm_policy) {
	rc = ata_eh_link_set_lpm(link, ap->target_lpm_policy, &dev);
}
And the only driver that sets ap->target_lpm_policy is AHCI.
So, I don't think we need to care about the slave link in this case.

I guess theoretically, if someone uses a:

sata_sis or sata_via, and forces LPM enabled via sysfs, even though it is not supported/initialized by any driver other than AHCI... I guess the drive might become unresponsive.

But, looking at these drivers, they don't provide a .set_lpm() callback, so I don't see how that could actually happen.

There is another driver however, that does have a slave_link:
drivers/ata/ata_piix.c: rc = ata_slave_link_init(ap);

And which does have a .set_lpm() callback:
drivers/ata/ata_piix.c: .set_lpm = piix_sidpr_set_lpm,

So I guess perhaps this problem could happen on ata_piix - which seems to be a driver for Intel PATA/SATA controllers.

From a quick look, it seems like
ata_piix.c calls ata_slave_link_init() only if:

if (ap->flags & ATA_FLAG_SLAVE_POSS) {
	rc = ata_slave_link_init(ap);
}

And:
PIIX_PATA_FLAGS = ATA_FLAG_SLAVE_POSS,
PIIX_SATA_FLAGS = ATA_FLAG_SATA | PIIX_FLAG_CHECKINTR,

And all board definitions using PIIX_PATA_FLAGS seems to have pata_* in their name, so I still think we can ignore this Sashiko comment.

Kind regards,
Niklas

Damien Le Moal

maintainer Applied

2026-07-29 01:49 UTC #5 lore raw

In reply to Niklas Cassel

Quoted 9 lines · Niklas Cassel

The inner loop of ata_scsi_lpm_supported() user the wrong variable when iterating.
It should obviously use the link that we are currently iterating over, rather than always using the host link.
ata_scsi_lpm_supported() is used to control if a user should be allowed to change lpm policy (from the default) via sysfs.

Quoted 8 lines

Thus, this bug could potentially disallow users to change the LPM policy for certain SATA devices via sysfs.
Cc [email protected]
Fixes 0060beec0bfa ("ata: libata-sata: Add link_power_management_supported sysfs attribute")
Reported-by Sashiko <[email protected]>
Link https://lore.kernel.org/linux-ide/[email protected]/ open here
Signed-off-by Niklas Cassel <[email protected]>

Applied to for-7.2-fixes, with the message typo fixed. Thanks!

Signature · 2 lines

Damien Le Moal
Western Digital Research