[PATCH] ata: libata-sata: fix ata_scsi_lpm_supported() iteration

[PATCH] ata: libata-sata: fix ata_scsi_lpm_supported() iteration

[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.

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

Cc: stable@vger.kernel.org
Fixes: 0060beec0bfa ("ata: libata-sata: Add link_power_management_supported sysfs attribute")
Reported-by: Sashiko <sashiko-bot@kernel.org>
Link: https://lore.kernel.org/linux-ide/20260728112200.B99F21F000E9@smtp.kernel.org/
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/ata/libata-sata.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
index 2ee54d60ea4b..5138b4e500e5 100644
--- a/drivers/ata/libata-sata.c
+++ b/drivers/ata/libata-sata.c
@@ -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;
 		}
-- 
2.55.0

Re: [PATCH] ata: libata-sata: fix ata_scsi_lpm_supported() iteration

[Niklas Cassel]

On Tue, Jul 28, 2026 at 01:38:42PM +0200, Niklas Cassel wrote:
> The inner loop of ata_scsi_lpm_supported() user the wrong variable when

s/user/uses/

Re: [PATCH] ata: libata-sata: fix ata_scsi_lpm_supported() iteration

[sashiko-bot]

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

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/20260728112200.B99F21F000E9@smtp.kernel.org/

> diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
> index b0706c30da052..ad40f516c6d43 100644
> --- a/drivers/ata/libata-sata.c
> +++ b/drivers/ata/libata-sata.c
> @@ -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?

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260728113841.727256-2-cassel@kernel.org?part=1

Re: [PATCH] ata: libata-sata: fix ata_scsi_lpm_supported() iteration

[Niklas Cassel]

On Tue, Jul 28, 2026 at 11:53:36AM +0000, sashiko-bot@kernel.org 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 <cassel@kernel.org>
> 
> 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/20260728112200.B99F21F000E9@smtp.kernel.org/
> 
> > diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
> > index b0706c30da052..ad40f516c6d43 100644
> > --- a/drivers/ata/libata-sata.c
> > +++ b/drivers/ata/libata-sata.c
> > @@ -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.

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

Re: [PATCH] ata: libata-sata: fix ata_scsi_lpm_supported() iteration

[Niklas Cassel]

On Tue, Jul 28, 2026 at 02:16:30PM +0200, Niklas Cassel wrote:
> On Tue, Jul 28, 2026 at 11:53:36AM +0000, sashiko-bot@kernel.org 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 <cassel@kernel.org>
> > 
> > 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/20260728112200.B99F21F000E9@smtp.kernel.org/
> > 
> > > diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
> > > index b0706c30da052..ad40f516c6d43 100644
> > > --- a/drivers/ata/libata-sata.c
> > > +++ b/drivers/ata/libata-sata.c
> > > @@ -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.
> 
> 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.

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