Re: [PATCH 3/3] riscv: dts: sophgo: sg2044: Add eFUSE device

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] nvmem: sophgo: heap buffer overflow in sg2044_efuses_read()
- [High] nvmem: sophgo-efuse: uninitialized mutex usage
- [High] nvmem: sophgo-efuse: missing reset line deassertion
- [Low] patch: undocumented driver code modification in a Device Tree patch
- [Low] nvmem: sophgo-efuse: sg2044_efuses_read_strip() return type mismatch
--

commit 9dc44262e764574361fe929e8bb24ed2e5ee7f12
Author: Inochi Amaoto <inochiama@gmail.com>

riscv: dts: sophgo: sg2044: Add eFUSE device

Add eFUSE controller node for SG2044.

> diff --git a/arch/riscv/boot/dts/sophgo/sg2044.dtsi b/arch/riscv/boot/dts/sophgo/sg2044.dtsi
> --- a/arch/riscv/boot/dts/sophgo/sg2044.dtsi
> +++ b/arch/riscv/boot/dts/sophgo/sg2044.dtsi

[ ... ]

> @@ -408,6 +408,18 @@ sd: mmc@703000b000 {
>  			status = "disabled";
>  		};
>  
> +		efuse0: efuse@7040000000 {
> +			compatible = "sophgo,sg2044-efuse";
> +			reg = <0x70 0x40000000 0x0 0x1000>;
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			clocks = <&clk CLK_GATE_EFUSE>,
> +				 <&clk CLK_GATE_APB_EFUSE>;
> +			clock-names = "core", "apb";
> +			resets = <&rst RST_EFUSE0>;

[Severity: High]
Does the driver need to deassert this reset line during probe?

The device tree node specifies a reset line, indicating the hardware requires
it.

However, looking at the probe function:

drivers/nvmem/sophgo-efuse.c:sophgo_efuses_probe() {
	...
	efuse->num_clks = devm_clk_bulk_get_all_enabled(&pdev->dev, &efuse->clks);
	if (efuse->num_clks < 0)
		return dev_err_probe(dev, efuse->num_clks, "failed to get clocks\n");

	config.priv = efuse;
	...
}

It seems the reset control is omitted entirely.

If the bootloader leaves the device in a reset state, subsequent MMIO accesses
in sg2044_efuse_wait_mode() could cause a bus hang or timeout.

> +			status = "disabled";
> +		};
> +
>  		i2c0: i2c@7040005000 {

[ ... ]

> diff --git a/drivers/nvmem/sophgo-efuse.c b/drivers/nvmem/sophgo-efuse.c
> --- a/drivers/nvmem/sophgo-efuse.c
> +++ b/drivers/nvmem/sophgo-efuse.c
> @@ -14,6 +14,7 @@
>  #include <linux/mutex.h>
>  #include <linux/nvmem-provider.h>
>  #include <linux/platform_device.h>
> +#include <linux/slab.h>

[Severity: Low]
This isn't a bug, but is it intended to include an undocumented driver
modification in a Device Tree patch?

The patch is scoped as a Device Tree update for the eFUSE DT node, but
silently modifies drivers/nvmem/sophgo-efuse.c to add this include.

Separating driver modifications from DTS patches helps maintain clean subsystem
boundaries and facilitates bisection.

[Severity: High]
Does this code overflow the allocation for the read buffer?

When sg2044_efuses_read() calculates start and end indices:

drivers/nvmem/sophgo-efuse.c:sg2044_efuses_read() {
	...
	start /= SG2044_EFUSE_ALIGN;
	end /= SG2044_EFUSE_ALIGN;
	...
	buf = kzalloc(end - start, GFP_KERNEL);
	...
}

It converts start and end from byte offsets to word counts, but then allocates
the buffer using the word count rather than multiplying by 4.

Later in the loop:

drivers/nvmem/sophgo-efuse.c:sg2044_efuses_read() {
	...
	for (i = start; i < end; i++) {
		ret = sg2044_efuses_read_strip(efuse, i, &value);
		if (ret)
			goto failed;

		memcpy(&buf[(i - start) * 4], &value, SG2044_EFUSE_ALIGN);
	}
	...
}

The memcpy writes 4 bytes per word into the under-allocated buffer.

Could this write past the end of the allocation and cause memory corruption?

[Severity: High]
Is the mutex inside the efuse structure properly initialized?

In the probe function, the structure is allocated and zero-initialized:

drivers/nvmem/sophgo-efuse.c:sophgo_efuses_probe() {
	...
	efuse = devm_kzalloc(dev, sizeof(*efuse), GFP_KERNEL);
	if (!efuse)
		return -ENOMEM;
	...
}

However, it appears devm_mutex_init() or mutex_init() is never called for
efuse->mutex.

When the sysfs read callback executes:

drivers/nvmem/sophgo-efuse.c:sg2044_efuses_read_strip() {
	...
	guard(mutex)(&efuse->mutex);
	...
}

Could locking this uninitialized mutex trigger lockdep warnings or system
crashes due to the uninitialized internal wait lock and wait list?

[Severity: Low]
This isn't a bug, but could the return type of sg2044_efuses_read_strip()
cause issues if negative error codes are implicitly cast?

The function is declared to return a u32:

drivers/nvmem/sophgo-efuse.c:
static u32 sg2044_efuses_read_strip(struct sophgo_efuses *efuse,
				    unsigned int offset, u32 *strip)

But it returns negative error codes directly:

	ret = sg2044_efuse_set_mode(efuse, SG2044_EFUSE_MODE_READ);
	if (ret < 0)
		return ret;

While the single caller sg2044_efuses_read() assigns this back to an int and
checks it correctly, returning negative errors as unsigned integers might lead
to unexpected behavior if future callers check for less than zero.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261014074516.240247-1-inochiama@gmail.com?part=3