Re: [PATCH 02/12] crypto: amlogic - Remove redundant dev_err()

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

Pre-existing issues:
- [High] IRQs are requested via devm_request_irq before the device clock is enabled and the channel list is allocated.
- [High] The driver modifies a shared static global array of algorithm templates during probe, preventing multiple device instances and corrupting crypto API state.
- [High] DMA mappings are leaked in `meson_cipher` when scatterlist mapping fails or exceeds the maximum descriptor count.
- [Medium] The IRQ handler unconditionally returns IRQ_HANDLED even for spurious interrupts.
--

--- Patch [2]: [PATCH 02/12] crypto: amlogic - Remove redundant dev_err() ---
commit 82c5002d2a0028a21e2688dd2106e4d410487f9e
Author: Pan Chuang <[email protected]>

crypto: amlogic - Remove redundant dev_err()

Since commit 55b48e23f5c4 ("genirq/devres: Add error handling in
devm_request_*_irq()"), devm_request_irq() automatically logs
detailed error messages on failure. Remove the now-redundant
driver-specific dev_err() calls.

> diff --git a/drivers/crypto/amlogic/amlogic-gxl-core.c b/drivers/crypto/amlogic/amlogic-gxl-core.c
> index 6cb33949915fe..169c6eeb51e51 100644
> --- a/drivers/crypto/amlogic/amlogic-gxl-core.c
> +++ b/drivers/crypto/amlogic/amlogic-gxl-core.c
> @@ -257,10 +257,8 @@ static int meson_crypto_probe(struct platform_device *pdev)
>  
>  		err = devm_request_irq(&pdev->dev, mc->irqs[i], meson_irq_handler, 0,
>  				       "gxl-crypto", mc);
> -		if (err < 0) {
> -			dev_err(mc->dev, "Cannot request IRQ for flow %d\n", i);
> +		if (err < 0)
>  			return err;

[Severity: High]
This is a pre-existing issue, but does requesting the interrupt at this point
risk a crash if an interrupt fires immediately?

If an interrupt fires between devm_request_irq() and the subsequent calls to
clk_prepare_enable() or meson_allocate_chanlist(), the handler will run before
resources are initialized.

In drivers/crypto/amlogic/amlogic-gxl-core.c:meson_irq_handler():

    p = readl(mc->base + ((0x04 + flow) << 2));
    if (p) {
        writel_relaxed(0xF, mc->base + ((0x4 + flow) << 2));
        mc->chanlist[flow].status = 1;

The handler performs a readl() on unclocked device registers, which can cause
a synchronous external abort or bus hang. It also accesses mc->chanlist
without checking if it is allocated, which could cause a null pointer
dereference.

[Severity: Medium]
This is a pre-existing issue, but does meson_irq_handler() unconditionally
return IRQ_HANDLED even for spurious interrupts?

Looking at drivers/crypto/amlogic/amlogic-gxl-core.c:meson_irq_handler():

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

If an unhandled interrupt occurs, returning IRQ_HANDLED prevents the core
interrupt subsystem's "nobody cared" logic from disabling a stuck interrupt
line. Could this result in an interrupt storm locking up a processor if the
hardware malfunctions?

> -		}
>  	}
>  
>  	err = clk_prepare_enable(mc->busclk);

[Severity: High]
This is a pre-existing issue, but does the driver modify a shared static
global array during probe?

Looking at meson_register_algs() which is called shortly after in
meson_crypto_probe():

In drivers/crypto/amlogic/amlogic-gxl-core.c:meson_register_algs():

    mc_algs[i].mc = mc;
    switch (mc_algs[i].type) {
    case CRYPTO_ALG_TYPE_SKCIPHER:
        err = crypto_engine_register_skcipher(&mc_algs[i].alg.skcipher);

Modifying the shared global static array mc_algs with device-specific pointers
prevents multiple device instances from functioning correctly. Probing multiple
instances or unbinding and rebinding the device will mutate this shared state,
which might corrupt the linked lists within the kernel's crypto subsystem.

[Severity: High]
This is a pre-existing issue, but does meson_cipher() leak direct memory access
mappings when scatterlist mapping fails?

In drivers/crypto/amlogic/amlogic-gxl-cipher.c:meson_cipher():

    if (!nr_sgs || nr_sgs > MAXDESC - 3) {
        dev_err(mc->dev, "Invalid SG count %d\n", nr_sgs);
        err = -EINVAL;
        goto theend;
    }

And in the error path:

    theend:
        kfree_sensitive(bkeyiv);
        kfree_sensitive(backup_iv);

        return err;

The error path frees memory but does not call dma_unmap_single() for the
previously mapped phykeyiv, nor dma_unmap_sg() for the already mapped
areq->src. Can this permanently leak mapping resources until exhaustion if
fragmented scatterlists are submitted?

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

_______________________________________________
linux-amlogic mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-amlogic