Re: [PATCH 2/3] nvmem: Add Sophgo SG2044 eFuse driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [Critical] mm: heap buffer overflow in sg2044_efuses_read allocation
- [High] nvmem: sophgo-efuse: heap buffer overflow due to word-to-byte calculation error
- [High] nvmem: sophgo-efuse: uninitialized mutex triggers kernel panic or undefined behavior
- [High] locking: uninitialized mutex efuse->mutex causes crash
- [Low] nvmem: sophgo-efuse: sg2044_efuses_read_strip() returns negative error code as u32
- [Low] endianness: byte order reversal on big-endian via memcpy
--

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

nvmem: Add Sophgo SG2044 eFuse driver

This patch adds a new nvmem driver for Sophgo eFuses used on system-on-chips
such as the SG2044. The driver exposes the shadow registers, which are
organized as 32-bit values, as memory-mapped io.

> diff --git a/drivers/nvmem/sophgo-efuse.c b/drivers/nvmem/sophgo-efuse.c
> new file mode 100644
> index 0000000000000..5f90adaf8e4fc
> --- /dev/null
> +++ b/drivers/nvmem/sophgo-efuse.c
[ ... ]
> +static u32 sg2044_efuses_read_strip(struct sophgo_efuses *efuse,
> +				    unsigned int offset, u32 *strip)
> +{
> +	u32 val = FIELD_PREP(SG2044_EFUSE_ADR_ADDR, offset);
> +	int ret;
> +
> +	guard(mutex)(&efuse->mutex);
> +
> +	writel(val, efuse->base + SG2044_EFUSE_ADR);
> +
> +	ret = sg2044_efuse_set_mode(efuse, SG2044_EFUSE_MODE_READ);
> +	if (ret < 0)
> +		return ret;

[Severity: Low]
Does this code return a negative error code as a u32?

sg2044_efuse_set_mode() returns an int (such as -ETIMEDOUT), which gets cast
to a large unsigned integer when returned here. Even though the caller
assigns it back to an int ret, returning negative values via an unsigned type
is poor practice and obscures the logic.

Should the return type of sg2044_efuses_read_strip() be int instead of u32?

[ ... ]
> +static int sg2044_efuses_read(void *context, unsigned int offset, void *val,
> +			      size_t bytes)
> +{
> +	struct sophgo_efuses *efuse = context;
> +	unsigned int start, start_offset, end, i;
> +	u32 value;
> +	u8 *buf;
> +	int ret;
> +
> +	start = rounddown(offset, SG2044_EFUSE_ALIGN);
> +	end = roundup(offset + bytes, SG2044_EFUSE_ALIGN);
> +	start_offset = offset - start;
> +
> +	start /= SG2044_EFUSE_ALIGN;
> +	end /= SG2044_EFUSE_ALIGN;
> +
> +	ret = readl_poll_timeout(efuse->base + SG2044_EFUSE_MD, value,
> +				 (value & SG2044_EFUSE_BOOT_DONE),
> +				 1, SG2044_BOOT_TIMEOUT);
> +	if (ret < 0)
> +		return ret;
> +
> +	buf = kzalloc(end - start, GFP_KERNEL);

[Severity: Critical]
Does this allocate an undersized buffer?

The variables start and end were divided by SG2044_EFUSE_ALIGN (4), changing
them from byte offsets to 32-bit word indices. The allocation size is exactly
end - start bytes, effectively allocating only 1 byte per 4-byte word instead
of (end - start) * SG2044_EFUSE_ALIGN.

> +	if (!buf)
> +		return -ENOMEM;
> +
> +	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);

[Severity: Critical]
Because the buffer allocation size is implicitly treated as words but
allocated as bytes, this loop writes 4 bytes per word iteration into the
undersized buffer, leading to an out-of-bounds heap write.

[Severity: Low]
Additionally, can this cause a byte order reversal on big-endian
architectures?

readl() returns a 32-bit integer in native endianness. The loop then uses
memcpy() to write this native integer's bytes into a byte buffer.

On big-endian systems, this places the most-significant byte at the lowest
address, reversing the intended nvmem byte-stream order.

[ ... ]
> +static int sophgo_efuses_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct sophgo_efuses *efuse;
> +	struct nvmem_config config = {
> +		.dev = &pdev->dev,
> +		.add_legacy_fixed_of_cells = true,
> +		.read_only = true,
> +		.reg_read = sg2044_efuses_read,
> +		.stride = 1,
> +		.word_size = 1,
> +		.name = "sophgo-efuse",
> +		.id = NVMEM_DEVID_AUTO,
> +		.root_only = true,
> +	};
> +
> +	efuse = devm_kzalloc(dev, sizeof(*efuse), GFP_KERNEL);

[Severity: High]
Is efuse->mutex left uninitialized?

This allocates the efuse structure (zero-initialized), but mutex_init() is
never called on efuse->mutex anywhere in the probe function.

Later, reading from the eFuse calls sg2044_efuses_read_strip() which attempts
to acquire the lock:

    guard(mutex)(&efuse->mutex);

Locking a zero-initialized mutex will cause a null-pointer dereference crash
on lock contention because the internal wait lists are not initialized.

Should mutex_init(&efuse->mutex) be added after allocation?

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