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 entry breaks NCQ completion tracking, which the patch fails to address.
- [High] The driver unconditionally reads ap->link.active_tag without checking for ATA_TAG_POISON, causing out-of-bounds array accesses in error and DMA paths.
--

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

ata: sata_dwc_460ex: fix data race on hsdev->sactive_issued in interrupt handler

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_instance)
>  	int handled, port = 0;
>  	uint intpr, sactive, sactive2, tag_mask;
>  	struct sata_dwc_device_port *hsdevp;
> -	hsdev->sactive_issued = 0;
>  
>  	spin_lock_irqsave(&host->lock, flags);
> +	hsdev->sactive_issued = 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 = (0 | sactive) ^ sactive, which always
evaluates to 0 because sactive clears the bit for completed commands.

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

>  
>  	/* Read the interrupt register */
>  	intpr = 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 = ap->link.active_tag;

        dev_err(ap->dev,
                "%s SCR_ERROR=0x%08x intpr=0x%08x status=0x%08x dma_intp=%d pending=%d issued=%d",
                __func__, serror, intpr, status, hsdevp->dma_interrupt_count,
                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()?

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