Re: [PATCH v3 2/4] crypto: rockchip: Add RK356x/RK3588 cryptographic offloader driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] In `rk2_hash_run`, the DMA descriptor's source length is programmed with the full scatterlist node length instead of the requested cryptographic payload length, causing a DMA hardware overrun and timeout.
- [High] The driver dynamically allocates its algorithm structures (`rkc->algs`) and explicitly frees them in `rk2_crypto_remove()`, leading to a use-after-free when active TFMs later access the freed memory.
- [High] In the DMA timeout error path, the hardware reset control is asserted without first disabling the device interrupt, causing a race condition with the IRQ handler.
- [Medium] The fallback checker `rk2_ahash_need_fallback` incorrectly evaluates unused scatterlist entries beyond the requested payload length, causing unnecessary software fallbacks.
--

--- Patch [2]: [PATCH v3 2/4] crypto: rockchip: Add RK356x/RK3588 cryptographic offloader driver ---
commit 4f54e41eac56e61deb80f3aed1c19c8ac7cba18a
Author: Dawid Olesinski <dawidro@gmail.com>

crypto: rockchip: Add RK356x/RK3588 cryptographic offloader driver

Add a driver for the second-generation Rockchip cryptographic hardware
accelerator found on RK3568 and RK3588 SoCs. It provides hardware
acceleration for AES block ciphers and hashing functions via a DMA engine,
falling back to software for unsupported requests.

diff --git a/drivers/crypto/rockchip/rk2_crypto.c b/drivers/crypto/rockchip/rk2_crypto.c
--- a/drivers/crypto/rockchip/rk2_crypto.c
+++ b/drivers/crypto/rockchip/rk2_crypto.c
[ ... ]
> static void rk2_crypto_remove(struct platform_device *pdev)
> {
>       struct rk2_crypto_dev *rkc = platform_get_drvdata(pdev);
> 
>       rk2_crypto_unregister(rkc);
[ ... ]
>       dma_free_coherent(rkc->dev, sizeof(struct rk2_crypto_lli) * MAX_LLI,
>                         rkc->tl, rkc->t_phy);
> 
>       kfree(rkc->algs);

[Severity: High]
Could this result in a use-after-free if active TFMs are still referencing
the algorithm structures?

When a user opens a crypto session and allocates a TFM, the crypto_alg
structure is pinned. If the device is unbound via sysfs, rk2_crypto_remove()
is invoked and frees rkc->algs immediately without waiting for active TFMs
to close.

When the session is later closed, the crypto core might access the freed
alg structure, for example in crypto_alg_put(). Should the algorithm
structures be statically allocated or managed differently to avoid this?

> }

diff --git a/drivers/crypto/rockchip/rk2_crypto_ahash.c b/drivers/crypto/rockchip/rk2_crypto_ahash.c
--- a/drivers/crypto/rockchip/rk2_crypto_ahash.c
+++ b/drivers/crypto/rockchip/rk2_crypto_ahash.c
[ ... ]
> static bool rk2_ahash_need_fallback(struct ahash_request *areq)
> {
[ ... ]
>       sg = areq->src;
>       while (sg) {
>               if (!IS_ALIGNED(sg->offset, sizeof(u32))) {
>                       atomic_long_inc(&algt->stat_fb_align);
>                       return true;
>               }
>               if (sg->length % 4) {
>                       atomic_long_inc(&algt->stat_fb_sglen);
>                       return true;
>               }
>               sg = sg_next(sg);
>       }

[Severity: Medium]
Does this loop unnecessarily force a fallback for scatterlist entries that
extend beyond the requested payload length?

If a request fits entirely within the first scatterlist entry, but the chain
contains subsequent unused entries from the network stack that are unaligned
or have lengths not a multiple of 4, this loop will still evaluate them and
trigger a fallback.

Is there a way to limit this check to only the scatterlist length up to
areq->nbytes to avoid performance degradation on hardware-capable requests?

[ ... ]
> int rk2_hash_run(struct crypto_engine *engine, void *breq)
> {
[ ... ]
>       /*
>        * Multi-SG requests are forced into the software fallback path due to
>        * hardware padding limitations (HW_PAD). Therefore, we are guaranteed
>        * to only process a single scatterlist element here.
>        */
>       dd->src_addr = cpu_to_le32(lower_32_bits(sg_dma_address(sgs)));
>       dd->src_len  = cpu_to_le32(sg_dma_len(sgs));

[Severity: High]
Will programming the DMA descriptor source length with the full scatterlist
node length cause a hardware overrun?

If a caller submits an asynchronous hash request where the scatterlist element
is larger than areq->nbytes, the hardware padding engine will expect exactly
areq->nbytes. However, the DMA engine will push the entire sg_dma_len(sgs)
into the FIFO.

Could this cause the DMA engine to stall when the hash engine stops reading,
resulting in a timeout? Should this length be capped at areq->nbytes instead?

[ ... ]
>       timeout = wait_for_completion_timeout(&rkc->complete,
>                                             msecs_to_jiffies(2000));
>       if (!timeout) {
>               dev_err(rkc->dev, "DMA timeout\n");
>               err = -ETIMEDOUT;
>               reset_control_assert(rkc->rst);
>               udelay(10);
>               reset_control_deassert(rkc->rst);
>               synchronize_irq(rkc->irq);
>       }

[Severity: High]
Is there a race condition here between asserting the hardware reset and the
interrupt handler?

If the DMA transfer times out and the hardware asserts an interrupt exactly
as the timeout expires, rk2_crypto_irq_handle() might execute concurrently
on another CPU.

If the interrupt handler attempts to read the RK2_CRYPTO_DMA_INT_ST register
while reset_control_assert() is active, could this crash the system with a
synchronous external abort?

Would it be safer to disable the interrupt before asserting the reset control?

diff --git a/drivers/crypto/rockchip/rk2_crypto_skcipher.c b/drivers/crypto/rockchip/rk2_crypto_skcipher.c
--- a/drivers/crypto/rockchip/rk2_crypto_skcipher.c
+++ b/drivers/crypto/rockchip/rk2_crypto_skcipher.c
[ ... ]
> int rk2_cipher_run(struct crypto_engine *engine, void *async_req)
> {
[ ... ]
>               timeout = wait_for_completion_timeout(&rkc->complete,
>                                                     msecs_to_jiffies(2000));
> 
>               if (!timeout) {
>                       dev_err(rkc->dev, "DMA timeout\n");
>                       err = -ETIMEDOUT;
>                       reset_control_assert(rkc->rst);
>                       udelay(10);
>                       reset_control_deassert(rkc->rst);
>                       synchronize_irq(rkc->irq);
>               }

[Severity: High]
Similar to the timeout path in rk2_hash_run(), does asserting the reset
control before synchronizing and disabling the IRQ risk a race condition
with rk2_crypto_irq_handle() here?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260816194112.552100-1-dawidro@gmail.com?part=2