Re: [PATCH v5 2/3] nvmem: add mt6323 PMIC EFUSE driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] nvmem: buffer overflow and unaligned memory access in efuse read loop
- [Medium] nvmem: EFUSE data exposed with host-dependent endianness
- [Medium] nvmem: NULL pointer dereference in probe when parent is missing
- [Medium] nvmem: module autoloading fails without MODULE_ALIAS
--

commit 3543415716c7fda0e6f5e6edb62e4ed761b35afd
Author: Roman Vivchar <rva333@protonmail.com>

nvmem: add mt6323 PMIC EFUSE driver

This commit adds support for the EFUSE controller found in the Mediatek
MT6323 PMIC. The MT6323 EFUSE stores 24 bytes of hardware-related data,
such as thermal sensor calibration values.

diff --git a/drivers/nvmem/mt6323-efuse.c b/drivers/nvmem/mt6323-efuse.c
new file mode 100644
index 0000000000000..de5e30215c22d
--- /dev/null
+++ b/drivers/nvmem/mt6323-efuse.c
[ ... ]
> +static int mt6323_efuse_read(void *context, unsigned int offset, void *val,
> +			     size_t bytes)
> +{
> +	struct regmap *map = context;
> +	u16 *buf = val;
> +	u32 tmp;
> +	int ret;
> +
> +	/*
> +	 * A manual loop using regmap_read is required because PWRAP is not
> +	 * a continuous MMIO space, but rather a FSM that doesn't implement the
> +	 * necessary read callback for the regmap_read_raw and regmap_read_bulk
> +	 * functions.
> +	 */
> +	for (size_t i = 0; i < bytes; i += sizeof(*buf)) {

[Severity: High]
Could this loop overflow the allocated val buffer and cause unaligned memory
accesses?

The NVMEM core API nvmem_device_read() does not enforce that the requested
bytes count is a multiple of word_size. If an in-kernel consumer requests an
odd number of bytes (for example, 1 or 3), the loop will run an extra
iteration and unconditionally write a full 16-bit word. This will overflow
the caller's allocated val buffer by 1 byte.

Additionally, if val is not 16-bit aligned, this causes an unaligned memory
access.

> +		ret = regmap_read(map, MT6323_EFUSE_DOUT_BASE + offset + i, &tmp);
> +		if (ret)
> +			return ret;
> +
> +		*buf++ = tmp;

[Severity: Medium]
Does this expose the EFUSE data with a host-dependent endianness?

The regmap_read() function returns a CPU-endian 32-bit integer, which is
assigned to tmp. Writing this integer into a 16-bit buffer using native CPU
byte order means that on big-endian architectures, the bytes will be swapped
compared to little-endian architectures, breaking the expected NVMEM byte
stream consistency.

> +	}
> +
> +	return 0;
> +}
> +
> +static int mt6323_efuse_probe(struct platform_device *pdev)
> +{
> +	struct mt6397_chip *mt6323 = dev_get_drvdata(pdev->dev.parent);

[Severity: Medium]
Will this cause a NULL pointer dereference if the parent device pointer is
missing?

The driver has an of_match_table. If it is instantiated directly from a root
Device Tree node rather than being instantiated as an MFD child, the parent
device pointer pdev->dev.parent will be NULL. Calling dev_get_drvdata() on a
NULL parent dereferences it immediately without checks, leading to a crash.

[ ... ]
> +static struct platform_driver mt6323_efuse_driver = {
> +	.probe = mt6323_efuse_probe,
> +	.driver = {
> +		.name = "mt6323-efuse",
> +		.of_match_table = mt6323_efuse_of_match,
> +	},
> +};
> +module_platform_driver(mt6323_efuse_driver);
> +
> +MODULE_DESCRIPTION("MediaTek MT6323 PMIC EFUSE driver");
> +MODULE_LICENSE("GPL");

[Severity: Medium]
Will module autoloading fail without a MODULE_ALIAS declaration here?

On systems where the Device Tree lacks the mediatek,mt6323-efuse node, the
MFD core will still instantiate the child device by name, producing a modalias
of platform:mt6323-efuse. Because the driver lacks a MODULE_ALIAS declaration
for platform:mt6323-efuse, udev cannot match the modalias, and the module
might never be automatically loaded on boot.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260921-mt6323-nvmem-v5-0-2f5d308369f0@protonmail.com?part=2