[PATCH] ata: libahci: Do not use an EM slot for the PMP control port

[PATCH] ata: libahci: Do not use an EM slot for the PMP control port

[Niklas Cassel]

When a Port Multiplier is attached, sata_pmp_attach() sets the pmp
number of the host link to SATA_PMP_CTRL_PORT (15), and every PMP
register access then goes through ahci_qc_issue() -> ahci_sw_activity()
on the host link. ahci_sw_activity() uses link->pmp as an index into
pp->em_priv[], which only has EM_MAX_SLOTS (15) entries, so for the
control port it ends up one past the end of the array.

Commit 1e41e693f458 ("ata: ahci: Match EM_MAX_SLOTS with
SATA_PMP_MAX_PORTS") grew em_priv[] from 8 to 15 entries for a UBSAN
report about this very index, but index 15 is still out of bounds.
UBSAN stopped complaining because &pp->em_priv[15] is now a
one-past-the-end address, which its bounds check accepts, while the
accesses through it are still out of bounds.

These accesses happen when the host link has ATA_LFLAG_SW_ACTIVITY set.
Enabling SW activity via sysfs for a disk attached directly to the port
sets the flag on the host link, and it stays set when the disk is
unplugged. If a PMP is then connected, each PMP register access makes
ahci_sw_activity() increment a counter past the end of em_priv[] and
pass the address of a nonexistent timer_list to timer_pending() and
mod_timer().

The flag cannot simply be cleared when the disk is unplugged: enclosure
LEDs belong to the bay, which is why em_priv[] is indexed by slot and
the blink policy outlives the disk, so a replacement disk in the same
bay should keep blinking.

Fixes: 18f7ba4c2f4b ("libata/ahci: enclosure management support")
Cc: stable@vger.kernel.org
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/ata/libahci.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index 9e970408916d..ef760d81d1d1 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -1033,11 +1033,16 @@ static void ahci_sw_activity(struct ata_link *link)
 {
 	struct ata_port *ap = link->ap;
 	struct ahci_port_priv *pp = ap->private_data;
-	struct ahci_em_priv *emp = &pp->em_priv[link->pmp];
+	struct ahci_em_priv *emp;
 
 	if (!(link->flags & ATA_LFLAG_SW_ACTIVITY))
 		return;
 
+	/* The PMP control port (SATA_PMP_CTRL_PORT) has no EM slot */
+	if (link->pmp >= EM_MAX_SLOTS)
+		return;
+
+	emp = &pp->em_priv[link->pmp];
 	emp->activity++;
 	if (!timer_pending(&emp->timer))
 		mod_timer(&emp->timer, jiffies + msecs_to_jiffies(10));
-- 
2.56.0

Re: [PATCH] ata: libahci: Do not use an EM slot for the PMP control port

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [High] ata: libahci: active timer reinitialization across
  suspend/resume in ahci_init_sw_activity() [1]
--

commit 05fb8ee9732c9d4452a8089e2686c99e78769965
Author: Niklas Cassel <cassel@kernel.org>

ata: libahci: Do not use an EM slot for the PMP control port

This commit skips enclosure management software activity updates when
link->pmp is at or above EM_MAX_SLOTS, avoiding an out-of-bounds access on
pp->em_priv[] for the PMP control port.

> diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
> index 9e970408916d1..ef760d81d1d1f 100644
> --- a/drivers/ata/libahci.c
> +++ b/drivers/ata/libahci.c
> @@ -1033,11 +1033,16 @@ static void ahci_sw_activity(struct ata_link *link)
>  {
>  	struct ata_port *ap = link->ap;
>  	struct ahci_port_priv *pp = ap->private_data;
> -	struct ahci_em_priv *emp = &pp->em_priv[link->pmp];
> +	struct ahci_em_priv *emp;
>  
>  	if (!(link->flags & ATA_LFLAG_SW_ACTIVITY))
>  		return;
>  
> +	/* The PMP control port (SATA_PMP_CTRL_PORT) has no EM slot */
> +	if (link->pmp >= EM_MAX_SLOTS)
> +		return;
> +
> +	emp = &pp->em_priv[link->pmp];
>  	emp->activity++;
>  	if (!timer_pending(&emp->timer))
>  		mod_timer(&emp->timer, jiffies + msecs_to_jiffies(10));

[Severity: High]
This problem wasn't introduced by this patch, but can emp->timer remain
armed across port suspend and be reinitialized while still queued on
resume [1]?

During port suspend, ata_eh_handle_port_suspend() issues a standby command
through ahci_qc_issue() -> ahci_sw_activity(), arming emp->timer, and
ahci_sw_activity_blink() re-arms it for another 100ms on expiration.
ahci_port_suspend() -> ahci_deinit_port() does not cancel emp->timer.

If ahci_port_resume() -> ahci_start_port() -> ahci_init_sw_activity() runs
while emp->timer is still pending, timer_setup() overwrites the queued
timer's hlist linkage:

drivers/ata/libahci.c:ahci_init_sw_activity() {
	...
	emp->saved_activity = emp->activity = 0;
	emp->link = link;
	timer_setup(&emp->timer, ahci_sw_activity_blink, 0);
	...
}

When that timer later expires, __hlist_del() hits a NULL pointer
dereference.  If the timer instead expires while the port is suspended,
ahci_sw_activity_blink() -> ahci_transmit_led_message() ->
ahci_rpm_get_port() calls pm_runtime_get_sync() from timer softirq context,
sleeping in atomic context and touching a powered-down controller.

[1] https://sashiko.dev/#/bug/linux-b496ab4d-84b4-4d7d-8517-70f1a3e845a2

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