Re: [PATCH 2/3] ata: sata_dwc_460ex: fix data race on hsdev->sactive_issued in interrupt handler

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

Pre-existing issues:
- [High] Unconditionally clearing hsdev->sactive_issued to 0 on every ISR e=
ntry breaks NCQ completion tracking, which the patch fails to address.
- [High] The driver unconditionally reads ap->link.active_tag without check=
ing for ATA_TAG_POISON, causing out-of-bounds array accesses in error and D=
MA paths.
--

--- Patch [2]: [PATCH 2/3] ata: sata_dwc_460ex: fix data race on hsdev->sac=
tive_issued in interrupt handler ---
commit 2519582df93a7a74dcbe39c71c593d6dad63fb71
Author: Rosen Penev <[email protected]>

ata: sata_dwc_460ex: fix data race on hsdev->sactive_issued in interrupt ha=
ndler

This commit moves the zeroing of hsdev->sactive_issued inside the host
spinlock critical section in sata_dwc_isr() to prevent a data race with
sata_dwc_qc_complete().

> diff --git a/drivers/ata/sata_dwc_460ex.c b/drivers/ata/sata_dwc_460ex.c
> index 8a1d80ac906a7..73bacdfd0bd31 100644
> --- a/drivers/ata/sata_dwc_460ex.c
> +++ b/drivers/ata/sata_dwc_460ex.c
> @@ -465,9 +465,9 @@ static irqreturn_t sata_dwc_isr(int irq, void *dev_in=
stance)
>  	int handled, port =3D 0;
>  	uint intpr, sactive, sactive2, tag_mask;
>  	struct sata_dwc_device_port *hsdevp;
> -	hsdev->sactive_issued =3D 0;
> =20
>  	spin_lock_irqsave(&host->lock, flags);
> +	hsdev->sactive_issued =3D 0;

[Severity: High]
This is a pre-existing issue, but does moving this assignment actually
resolve the underlying problem? Unconditionally clearing
hsdev->sactive_issued to 0 on every ISR entry seems to break NCQ
completion tracking.

When an NCQ completion interrupt fires, sactive_issued will be 0. The
driver then calculates tag_mask =3D (0 | sactive) ^ sactive, which always
evaluates to 0 because sactive clears the bit for completed commands.

With tag_mask =3D=3D 0, will the driver skip the NCQ completion block and
leave the command to time out?

> =20
>  	/* Read the interrupt register */
>  	intpr =3D sata_dwc_readl(&hsdev->sata_dwc_regs->intpr);


[Severity: High]
This is also a pre-existing issue, but I noticed another potential problem
in sata_dwc_error_intr() and the DMA completion paths.

Does this code potentially perform out-of-bounds array accesses?

drivers/ata/sata_dwc_460ex.c:sata_dwc_error_intr() {
        ...
        tag =3D ap->link.active_tag;

        dev_err(ap->dev,
                "%s SCR_ERROR=3D0x%08x intpr=3D0x%08x status=3D0x%08x dma_i=
ntp=3D%d pending=3D%d issued=3D%d",
                __func__, serror, intpr, status, hsdevp->dma_interrupt_coun=
t,
                hsdevp->dma_pending[tag], hsdevp->cmd_issued[tag]);
        ...
}

When an error interrupt occurs while no command is active,
ap->link.active_tag is set to ATA_TAG_POISON (0xfafbfcfdU).

The driver truncates this to a u8 (253) and uses it directly as an index
into the hsdevp->dma_pending and hsdevp->cmd_issued arrays, which are
sized SATA_DWC_QCMD_MAX (33).

Could this result in leaking arbitrary memory up to ~1000 bytes past the
arrays into dmesg via dev_err()?

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260723001208.1469=
[email protected]?part=3D2