Re: [PATCH v2 2/4] nvmem: add mt6323 PMIC EFUSE driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Unchecked traversal of device parent pointers causes a NULL pointer dereference.
- [High] Buffer overflow in the NVMEM read callback for unaligned read requests.
--

commit 6d793839a1a7e502029cf9f7b58d011fec9486e7
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..7a0ce4c7f5658
> --- /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)) {
> +		ret = regmap_read(map, MT6323_EFUSE_DOUT_BASE + offset + i, &tmp);
> +		if (ret)
> +			return ret;
> +
> +		*buf++ = tmp;

[Severity: High]
What happens if an internal client requests to read an odd number of bytes?

Since this loop unconditionally writes a 16-bit word on each iteration, an odd
bytes length would cause the loop to execute an extra time and write past the
end of the destination buffer. Because nvmem_device_read() does not enforce
word_size alignment on internal requests, this could corrupt adjacent memory
if the caller allocated a buffer matching the exact requested size.

Could the driver explicitly reject unaligned lengths or safely handle partial
word writes?

> +	}
> +
> +	return 0;
> +}
> +
> +static int mt6323_efuse_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct nvmem_config config = {
> +		.name = "mt6323-efuse",
> +		.stride = 2,
> +		.word_size = 2,
> +		.size = MT6323_EFUSE_SIZE,
> +		.reg_read = mt6323_efuse_read,
> +	};
> +	struct nvmem_device *nvmem;
> +	struct regmap *regmap;
> +
> +	/* efuse -> mfd -> pwrap */
> +	regmap = dev_get_regmap(dev->parent->parent, NULL);

[Severity: High]
Could this lead to a NULL pointer dereference if the device tree hierarchy
is malformed?

If the device is instantiated at the top level or independently of the
expected MFD and PMIC wrapper structure, dev->parent could map to the
platform bus, making dev->parent->parent NULL. Passing a NULL pointer to
dev_get_regmap() will cause a kernel panic when it attempts to lock the
devres lock.

Would it be safer to verify that dev->parent and dev->parent->parent are
non-NULL before passing the grandparent pointer?

> +	if (!regmap)
> +		return dev_err_probe(dev, -ENODEV, "failed to get regmap\n");

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260617-mt6323-nvmem-v2-0-4f30e36aa0f4@protonmail.com?part=2