Re: [PATCH 4/4] arm64: dts: rockchip: Add crypto node to rk3588-base

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The shared interrupt handler accesses device registers without ensuring the device's clocks are enabled, which will trigger an asynchronous bus fault (SError) if a shared or spurious interrupt occurs while suspended.
- [High] The driver dereferences the `areq->iv` byte array as a `u32 *`, which can cause an alignment fault.
--

commit 7f6218b80827dfe5983e50e2564e2a7bc74c81f8
Author: Dawid Olesinski <dawidro@gmail.com>

arm64: dts: rockchip: Add crypto node to rk3588-base

This commit adds the device tree node for the V2 cryptographic hardware
accelerator on the RK3588. The node is disabled by default and uses the ARM
SCMI interface for clocks and resets because the IP is in the secure domain.

> diff --git a/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi b/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi
> index 4fb8888c281c8..4f336741d11ff 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi
> @@ -2257,6 +2257,18 @@ rng@fe378000 {
>  		resets = <&scmi_reset SCMI_SRST_H_TRNG_NS>;
>  	};
>  
> +	crypto: crypto@fe370000 {
> +		compatible = "rockchip,rk3588-crypto";

[Severity: High]
This isn't a bug introduced by this patch, but enabling this device exposes an
alignment fault issue in the driver.

The driver dereferences the areq->iv byte array as a u32 pointer, which can
cause an alignment fault. The block cipher algorithms set .base.cra_alignmask
to 0, meaning the crypto subsystem makes no guarantees about the memory
alignment of the areq->iv buffer. However, in rk2_cipher_run()
(drivers/crypto/rockchip/rk2_crypto_skcipher.c):

	writel(cpu_to_be32(riv[i]),
	       rkc->reg + RK2_CRYPTO_CH0_IV_0 + i * 4);

the driver casts areq->iv to a u32 pointer and reads from it directly via
riv[i]. If the IV buffer is unaligned, this dereference is undefined behavior
in C. On ARM64, if the compiler assumes alignment and emits an instruction that
strictly requires it, this will trigger an alignment fault exception and crash
the kernel.

Should the driver be updated to either use get_unaligned() to read the IV bytes
or set cra_alignmask = 3 to enforce 4-byte alignment?

> +		reg = <0x0 0xfe370000 0x0 0x2000>;
> +		interrupts = <GIC_SPI 209 IRQ_TYPE_LEVEL_HIGH 0>;

[Severity: High]
This isn't a bug introduced by this patch, but enabling this device via device
tree will expose a shared interrupt handling issue in the driver.

The shared interrupt handler accesses device registers without ensuring the
device's clocks are enabled.

The driver requests the interrupt with IRQF_SHARED while placing the device into
runtime suspend (disabling its clocks and asserting reset) when idle. If another
device on the same IRQ line fires, or a spurious interrupt occurs,
rk2_crypto_irq_handle() in drivers/crypto/rockchip/rk2_crypto.c will execute:

	v = readl(rkc->reg + RK2_CRYPTO_DMA_INT_ST);
	if (!v)
		return IRQ_NONE;

Accessing an unclocked APB/AHB target on Rockchip SoCs triggers an asynchronous
bus fault (SError), crashing the kernel.

Could the handler verify the device is active (e.g., via a driver-specific
software flag) before touching hardware registers?

> +		clocks = <&scmi_clk SCMI_CRYPTO_CORE>, <&scmi_clk SCMI_ACLK_SECURE_NS>,
> +			 <&scmi_clk SCMI_HCLK_SECURE_NS>;
> +		clock-names = "core", "aclk", "hclk";
> +		resets = <&scmi_reset SCMI_SRST_CRYPTO_CORE>;
> +		reset-names = "core";
> +		status = "disabled";
> +	};

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