Re: [PATCH v2] crypto/amlogic: Use devm APIs for clock management

[sashiko-bot@kernel.org]

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

New issues:
- [High] The patch introduces a Use-After-Free of mc->chanlist in the IRQ handler during driver removal and probe error paths due to devres teardown order.

Pre-existing issues:
- [High] IRQs are requested and enabled before their associated data structures (mc->chanlist) are allocated, leading to a NULL pointer dereference if a pending interrupt fires early during probe.
- [High] Static array state is not cleared on unbind, leading to double unregistration and crypto list corruption if a subsequent probe fails.
- [High] Sysfs unbind causes a Use-After-Free for active Crypto API TFMs.
- [Medium] The IRQ handler unconditionally returns IRQ_HANDLED even for unhandled or spurious interrupts, defeating the kernel's spurious interrupt detection.
--

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

crypto/amlogic: Use devm APIs for clock management

This patch transitions the driver to use devm_clk_get_enabled for managing
the core clock. This change allows the kernel to automatically handle the
clock lifecycle and removes the need for manual cleanup in error paths and
the remove function.

Closes: https://lore.kernel.org/oe-kbuild-all/202608310329.QgxCCMs1-lkp@intel.com/

> diff --git a/drivers/crypto/amlogic/amlogic-gxl-core.c b/drivers/crypto/amlogic/amlogic-gxl-core.c
> index 169c6eeb51e51..6f4c62ddaf896 100644
> --- a/drivers/crypto/amlogic/amlogic-gxl-core.c
> +++ b/drivers/crypto/amlogic/amlogic-gxl-core.c
> @@ -243,10 +243,10 @@ static int meson_crypto_probe(struct platform_device *pdev)
>  	if (IS_ERR(mc->base))
>  		return PTR_ERR(mc->base);
>  
> -	mc->busclk = devm_clk_get(&pdev->dev, "blkmv");
> +	mc->busclk = devm_clk_get_enabled(&pdev->dev, "blkmv");

[Severity: High]
Could this change introduce a use-after-free regression during driver unbind
or probe error paths?

By using devm_clk_get_enabled here, the clock is registered for devres cleanup
before mc->chanlist is allocated with devm_kcalloc. During device removal,
devres cleans up in LIFO order:

1. devm_kcalloc frees mc->chanlist
2. devm_request_irq unbinds the IRQs
3. devm_clk_get_enabled disables the clock

If an interrupt fires between the time mc->chanlist is freed and the IRQs
are unbound, meson_irq_handler will dereference the freed mc->chanlist array.
Would it be safer to ensure mc->chanlist outlives the IRQ bindings?

>  	if (IS_ERR(mc->busclk)) {
>  		err = PTR_ERR(mc->busclk);
> -		dev_err(&pdev->dev, "Cannot get core clock err=%d\n", err);
> +		dev_err(&pdev->dev, "Cannot get/enable core clock err=%d\n", err);
>  		return err;
>  	}
>  
> @@ -261,15 +261,9 @@ static int meson_crypto_probe(struct platform_device *pdev)
>  			return err;
>  	}
>  
> -	err = clk_prepare_enable(mc->busclk);
> -	if (err != 0) {
> -		dev_err(&pdev->dev, "Cannot prepare_enable busclk\n");
> -		return err;
> -	}
> -
>  	err = meson_allocate_chanlist(mc);

[Severity: High]
This is a pre-existing issue, but is there a risk of a null pointer
dereference here if an interrupt arrives early?

In meson_crypto_probe(), devm_request_irq() is called to request and enable
the IRQs right before meson_allocate_chanlist() is called:

meson_crypto_probe() {
    ...
    err = devm_request_irq(&pdev->dev, mc->irqs[i], meson_irq_handler, ...);
    ...
    err = meson_allocate_chanlist(mc);
    ...
}

If the hardware has a pending interrupt from a previous state, the handler
can fire before the mc->chanlist array is allocated, leading to a panic when
meson_irq_handler accesses mc->chanlist. Should the IRQs be requested after
the necessary structures are allocated?

>  	if (err)
> -		goto error_flow;
> +		return err;
>  
>  	err = meson_register_algs(mc);
>  	if (err)
> @@ -290,8 +284,6 @@ static int meson_crypto_probe(struct platform_device *pdev)
>  error_alg:
>  	meson_unregister_algs(mc);

[Severity: High]
This is a pre-existing issue, but does this cleanup path properly reset the
registration state for mc_algs?

When meson_unregister_algs() unregisters the algorithms, it leaves the
mc_algs[i].mc pointer intact instead of clearing it:

meson_unregister_algs() {
    for (i = 0; i < ARRAY_SIZE(mc_algs); i++) {
        if (!mc_algs[i].mc)
            continue;
        ...
        crypto_engine_unregister_skcipher(&mc_algs[i].alg.skcipher);
    }
}

If a subsequent probe fails, the error path will call meson_unregister_algs()
again. Since the global static mc_algs array still holds the stale mc
pointers, it will attempt to unregister the algorithms a second time,
potentially causing list corruption in the core crypto subsystem. Does this
array need to be cleared?

>  	meson_free_chanlist(mc, MAXFLOW - 1);
> -error_flow:
> -	clk_disable_unprepare(mc->busclk);
>  	return err;
>  }
>  
> @@ -306,8 +298,6 @@ static void meson_crypto_remove(struct platform_device *pdev)
>  	meson_unregister_algs(mc);
>  
>  	meson_free_chanlist(mc, MAXFLOW - 1);
> -
> -	clk_disable_unprepare(mc->busclk);
>  }

[Severity: High]
This is a pre-existing issue, but can sysfs unbind cause a use-after-free
for active Crypto API contexts?

The main device context mc is allocated with devm_kzalloc(), which ties it
to the device's sysfs lifecycle. If an administrator unbinds the driver while
unprivileged userspace holds open an AF_ALG socket, the TFMs will outlive
the device.

When userspace subsequently issues an encrypt or decrypt request, operations
like meson_skencrypt() will dereference op->mc, which has already been freed
by devres. Does the driver need to decouple the mc context lifetime from
devres to protect active TFMs?


[Severity: Medium]
This is a pre-existing issue, but does the interrupt handler unconditionally
return IRQ_HANDLED even for unhandled interrupts?

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;
}

Returning IRQ_HANDLED unconditionally for unknown or empty conditions bypasses
the kernel spurious interrupt detection mechanism. If the device misbehaves
and generates an interrupt storm, the kernel will not mask the line, which
could result in a lockup. Should this fallback path return IRQ_NONE instead?

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

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