Re: [PATCH] crypto: amlogic: Fix IRQ handler return value and fallthrough logic

[sashiko-bot@kernel.org]

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

New issues:
- [High] Unreachable fallback and masking of spurious interrupts.

Pre-existing issues:
- [Critical] Hardware DMA engine continues running after timeout, leading to silent memory corruption and Use-After-Free.
- [High] DMA mappings are leaked on scatterlist mapping failure paths.
- [High] Incorrect devres registration order creates a NULL pointer dereference on probe and a Use-After-Free window on remove.
- [High] Un-ratelimited dev_err() in the interrupt handler causes console lockup during spurious interrupts.
--

commit cb3932b36aa0f1b16d26166261f56135df7d5e86
Author: Mohamad Raizudeen <raizudeen.kerneldev@gmail.com>

crypto: amlogic: Fix IRQ handler return value and fallthrough logic

In irqreturn_t_meson_irq_handler(), when an interrupt matches a flow but the
status register is empty, the driver prints an error but falls through the loop
and incorrectly prints an unknown irq message. This commit fixes it by returning
IRQ_HANDLED immediately after the error print. Additionally, it returns IRQ_NONE
instead of IRQ_HANDLED for genuinely unknown interrupts to prevent masking them.

> diff --git a/drivers/crypto/amlogic/amlogic-gxl-core.c b/drivers/crypto/amlogic/amlogic-gxl-core.c
> index 169c6eeb51e51..f8b1d094cb188 100644
> --- a/drivers/crypto/amlogic/amlogic-gxl-core.c
> +++ b/drivers/crypto/amlogic/amlogic-gxl-core.c
> @@ -38,11 +38,12 @@ static irqreturn_t meson_irq_handler(int irq, void *data)
>  				return IRQ_HANDLED;
>  			}
>  			dev_err(mc->dev, "%s %d Got irq for flow %d but ctrl is empty\n", __func__, irq, flow);

[Severity: High]
This is a pre-existing issue, but does this un-ratelimited dev_err() in
meson_irq_handler() lock up the console during a spurious interrupt storm? If
the device status register is empty (p == 0), it prints an error. Given the
high frequency of interrupt storms, this could overwhelm the console driver.

> +			return IRQ_HANDLED; 

[Severity: High]
Does explicitly returning IRQ_HANDLED for an empty device status mask spurious
interrupts? This defeats the kernel's spurious interrupt detector in
note_interrupt(), which relies on IRQ_NONE to disable stuck IRQ lines. Could
this lead to an unrecoverable interrupt storm and CPU lockup?

>  		}
>  	}
>  
>  	dev_err(mc->dev, "%s %d from unknown irq\n", __func__, irq);
> -	return IRQ_HANDLED;
> +	return IRQ_NONE;

[Severity: High]
Is this intended fix in meson_irq_handler() unreachable? The driver registers
its interrupt without the IRQF_SHARED flag, meaning the kernel will only invoke
this handler with an irq matching mc->irqs[flow]. Because of this, the loop
above will always find a match and return early, making this IRQ_NONE return
entirely unreachable dead code.

>  }
>  
>  static struct meson_alg_template mc_algs[] = {

[Severity: High]
This is a pre-existing issue, but does the devres registration order in
meson_crypto_probe() create a NULL pointer dereference on probe and a
use-after-free window on remove?

The IRQ is requested here, enabling interrupts before the necessary structures
are allocated:

drivers/crypto/amlogic/amlogic-gxl-core.c:meson_crypto_probe() {
    ...
    err = devm_request_irq(&pdev->dev, mc->irqs[i], meson_irq_handler, 0,
                           "gxl-crypto", mc);
    ...
}

Then mc->chanlist is allocated here:

drivers/crypto/amlogic/amlogic-gxl-core.c:meson_crypto_probe() {
    ...
    err = meson_allocate_chanlist(mc);
    ...
}

Because devres frees resources in the reverse order of their registration,
mc->chanlist will be freed before the IRQ handler is unregistered during driver
removal. If an interrupt fires during module unbind after devres has freed
mc->chanlist but before it unregisters the IRQ, could the interrupt handler
access freed memory when it dereferences mc->chanlist[flow].status?


[Severity: High]
This is a pre-existing issue, but are DMA mappings leaked on scatterlist
mapping failure paths in meson_cipher()?

If dma_map_single() succeeds for phykeyiv, but a subsequent dma_map_sg() fails,
the code jumps to the theend label:

drivers/crypto/amlogic/amlogic-gxl-cipher.c:meson_cipher() {
    ...
        if (!nr_sgd || nr_sgd > MAXDESC - 3) {
            dev_err(mc->dev, "Invalid SG count %d\n", nr_sgd);
            err = -EINVAL;
            goto theend;
        }
    ...
}

The cleanup label fails to call dma_unmap_single() and dma_unmap_sg():

drivers/crypto/amlogic/amlogic-gxl-cipher.c:meson_cipher() {
    ...
theend:
    kfree_sensitive(bkeyiv);
    kfree_sensitive(backup_iv);

    return err;
}

Could repeated failures exhaust IOMMU space or bounce buffers, permanently
leaking the mapping resources?


[Severity: Critical]
This is a pre-existing issue, but does the hardware DMA engine continue running
after a timeout in meson_cipher(), leading to silent memory corruption and
use-after-free?

The driver waits for the hardware:

drivers/crypto/amlogic/amlogic-gxl-cipher.c:meson_cipher() {
    ...
    wait_for_completion_interruptible_timeout(&mc->chanlist[flow].complete,
                                              msecs_to_jiffies(500));
    ...
}

If the hardware takes longer than 500ms to process a request or the wait is
interrupted by a signal, it times out and proceeds to tear down mappings:

drivers/crypto/amlogic/amlogic-gxl-cipher.c:meson_cipher() {
    ...
    dma_unmap_single(mc->dev, phykeyiv, keyivlen, DMA_TO_DEVICE);
    ...
}

DMA resources are unmapped without stopping the hardware. Could the still-running
hardware DMA engine read from or write to unmapped physical memory in the
background against the now-freed memory, causing memory corruption or overwriting
unrelated kernel structures?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260908160120.4805-1-raizudeen.kerneldev@gmail.com?part=1

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic