Re: [PATCH v2 3/3] nvmem: stm32: add TAMP nvmem driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] nvmem: stm32: mismatch in DT matching can cause out-of-bounds array read
- [Medium] nvmem: stm32: endianness mismatch between word and byte NVMEM accesses
- [Low] nvmem: stm32: incompatible pointer type assignment causes compiler warning
- [Low] nvmem: stm32: off-by-one error in max_register configuration
--

commit 5c38f8fb5f67e48c161b92fd1e0bd910ac6adfd9
Author: Simeon Marijon <simeon.marijon@foss.st.com>

nvmem: stm32: add TAMP nvmem driver

This commit adds support for exposing the STM32 TAMP backup registers as
nvmem cells. It includes driver implementations for handling register read,
write, and access permissions for different hardware zones on STM32MP1
and STM32MP2.

> diff --git a/drivers/nvmem/stm32-tamp-nvram.c b/drivers/nvmem/stm32-tamp-nvram.c
> new file mode 100644
> index 0000000000000..c10c5ed5d2732
> --- /dev/null
> +++ b/drivers/nvmem/stm32-tamp-nvram.c
[ ... ]
> +static const struct regmap_config stm32mp1_tamp_nvram_regmap_cfg = {
> +	.reg_bits = 32,
> +	.val_bits = 32,
> +	.reg_stride = 4,
> +	.readable_reg = stm32_tamp_nvram_readable_bkpreg,
> +	.writeable_reg = stm32_tamp_nvram_writeable_bkpreg,
> +	.max_register = 0x80,
> +};

[Severity: Low]
Could the max_register be slightly off-by-one here? Since MP15 has 32
registers (128 bytes), the highest valid register offset should be 0x7C.
Setting this to 0x80 theoretically allows regmap to accept accesses one
register beyond the valid range.

> +
> +static const struct regmap_config stm32mp2_tamp_nvram_regmap_cfg = {
> +	.reg_bits = 32,
> +	.val_bits = 32,
> +	.reg_stride = 4,
> +	.readable_reg = stm32_tamp_nvram_readable_bkpreg,
> +	.writeable_reg = stm32_tamp_nvram_writeable_bkpreg,
> +	.max_register = 0x200,
> +};

[Severity: Low]
Similarly, for MP25 with 128 registers (512 bytes), shouldn't the maximum
valid offset be 0x1FC rather than 0x200?

[ ... ]
> +static int stm32_tamp_nvram_read(void *context, unsigned int offset, void *buf, size_t bytes)
> +{
[ ... ]
> +		reg_idx = i / sizeof(u32);
> +		if (i + sizeof(u32) <= total && IS_ALIGNED(i, sizeof(u32))) {
> +			if (!stm32_tamp_nvram_rights(priv, reg_idx, true)) {
> +				dev_dbg(dev, "Backup register %u is not allowed to be read\n",
> +					reg_idx);
> +				temp_u32 = 0;
> +			} else {
> +				ret = regmap_read(priv->bkpregs_regmap, i, &temp_u32);
> +				if (ret)
> +					return ret;
> +			}
> +			memcpy(buf_u8, &temp_u32, sizeof(u32));

[Severity: Medium]
Will this memcpy cause an endianness mismatch on Big Endian architectures? 

The regmap_read() function gives a natively ordered u32, but directly
copying it into the byte buffer will result in byte-swapped data compared to
the byte-aligned fallback path. Do we need an explicit endian conversion
(like cpu_to_le32s) here instead of a raw memcpy?

> +			buf_u8 += sizeof(u32);
> +			i += sizeof(u32);
> +		} else {
[ ... ]
> +static int stm32_tamp_nvram_write(void *context, unsigned int offset, void *buf, size_t bytes)
> +{
[ ... ]
> +	i = offset;
> +	while (i < total)  {
> +		reg_idx = i / sizeof(u32);
> +		if (i + sizeof(u32) <= total && IS_ALIGNED(i, sizeof(u32))) {
> +			if (stm32_tamp_nvram_rights(priv, reg_idx, false)) {
> +				memcpy(&temp_u32, buf_u8, sizeof(u32));
> +
> +				ret = regmap_write(priv->bkpregs_regmap, i, temp_u32);

[Severity: Medium]
Does this have the same endianness concern for Big Endian CPUs? Reading raw
NVMEM buffer bytes directly into a native CPU integer via memcpy might flip
the byte order before the regmap_write().

[ ... ]
> +static int stm32_tamp_nvram_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct stm32_tamp_nvram_priv *priv;
> +	struct resource *res;
> +	struct device_node *of_node;
> +	const struct of_device_id *of_id;
> +	void __iomem *base_addr;
> +	unsigned int *zones_end;
[ ... ]
> +	priv->idx_bkpreg_zones_end = stm32_tamp_nvram_get_backup_zones(priv);
> +	if (IS_ERR(priv->idx_bkpreg_zones_end))
> +		return dev_err_probe(dev, PTR_ERR(priv->idx_bkpreg_zones_end),
> +				     "Can't determine protection zone\n");
> +
> +	zones_end = priv->idx_bkpreg_zones_end;

[Severity: Low]
Can this assignment cause a compiler warning for differing signedness? 

The driver assigns priv->idx_bkpreg_zones_end (an int *) to zones_end
(an unsigned int *) without casting, which might break builds enforcing
-Werror.

[ ... ]
> +	if (of_device_is_compatible(pdev->dev.of_node,
> +				    "st,stm32mp25-tamp-nvram")) {
> +		priv->bkpreg_access = stm32_tamp_get_access_rights_mp2x(priv);
> +
> +		if (IS_ERR(priv->bkpreg_access))
> +			return PTR_ERR(priv->bkpreg_access);
> +
> +		dev_dbg(dev, "\n"
> +			"Zone 1-RIF1 %3d - %3d %c%c\n"
[ ... ]
> +			zones_end[BKPREG_PROTECTION_ZONE_3_RIF0] + 1,
> +			zones_end[BKPREG_PROTECTION_ZONE_3_RIF2],
> +			stm32_tamp_nvram_rights(priv,
> +						zones_end[BKPREG_PROTECTION_ZONE_3_RIF2],
> +						true) ? 'R' : '-',

[Severity: Medium]
Is it possible for a misconfigured device tree to trigger an out-of-bounds
array read here?

If a device tree node contains multiple compatible strings (e.g., both MP15
and MP25), of_match_node() determines priv->data based on the first match in
the driver's list (MP15). This allocates only a 3-element array for
idx_bkpreg_zones_end.

However, if this of_device_is_compatible() check for
"st,stm32mp25-tamp-nvram" also evaluates to true, the driver will branch
here and read up to index 6 of the zones_end array, reading past the end of
the 3-element allocation. 
    
Should the control flow rely on the already resolved priv->data->nb_zones
instead of re-evaluating the compatible string?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260930-upstream_nvram_reworked-v2-0-cfda749ee0b7@foss.st.com?part=3