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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] nvmem: stm32: broken empty zone handling in stm32_tamp_nvram_bkpreg_get_zone()
--

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

nvmem: stm32: add TAMP nvmem driver

This commit adds an nvmem provider driver for the STMicroelectronics STM32
TAMP backup registers on STM32MP1 and STM32MP25 SoCs, checking hardware
protection zones and compartment isolation rights before accessing the
backup registers.

> diff --git a/drivers/nvmem/stm32-tamp-nvram.c b/drivers/nvmem/stm32-tamp-nvram.c
> new file mode 100644
> index 0000000000000..81096a5d02829
> --- /dev/null
> +++ b/drivers/nvmem/stm32-tamp-nvram.c
> @@ -0,0 +1,680 @@

[ ... ]

> +static int stm32_tamp_nvram_bkpreg_get_zone(struct stm32_tamp_nvram_priv *priv, int reg)
> +{
> +	int *idx_bkpreg_zones_end = priv->idx_bkpreg_zones_end;
> +	int nb_zones = priv->data->nb_zones;
> +	int protection_zone_idx;
> +
> +	if (reg < 0)
> +		return -1; /* negative reg is the boundary of an empty zone */
> +
> +	for (protection_zone_idx = 0; protection_zone_idx < nb_zones; protection_zone_idx++) {
> +		if (reg <= idx_bkpreg_zones_end[protection_zone_idx])
> +			break;
> +	}
> +
> +	if (protection_zone_idx >= nb_zones)
> +		return -1; /* the reg is not a part of any zone */
> +
> +	return protection_zone_idx;
> +}

[ ... ]

> +static int *stm32_tamp_nvram_get_backup_zones(struct stm32_tamp_nvram_priv *priv)
> +{
> +	struct device *dev = priv->dev;
> +	struct regmap *tamp_regmap = priv->config_regmap;
> +	u32 offset_field;
> +	int nb_zones = priv->data->nb_zones;
> +	int zone_idx;
> +	int *idx_bkpreg_zones_end;
> +
> +	idx_bkpreg_zones_end = devm_kcalloc(dev,
> +					    nb_zones,
> +					    sizeof(*idx_bkpreg_zones_end),
> +					    GFP_KERNEL);
> +	if (!idx_bkpreg_zones_end) {
> +		dev_err(dev, "Can't allocate registers zones\n");
> +		return ERR_PTR(-ENOMEM);
> +	}
> +
> +	/* Get the n-1 frontiers of zone within the tamp configuration registers */
> +	for (zone_idx = 0; zone_idx < nb_zones - 1; zone_idx++) {
> +		const struct reg_field reg_field = priv->data->config_reg_fields[zone_idx];
> +		struct regmap_field *field;
> +		int ret;
> +
> +		field = devm_regmap_field_alloc(dev, tamp_regmap, reg_field);
> +		if (IS_ERR(field)) {
> +			dev_err(dev, "Can't allocate registers zones\n");
> +			devm_kfree(dev, idx_bkpreg_zones_end);
> +			return ERR_CAST(field);
> +		}
> +
> +		ret = regmap_field_read(field, &offset_field);
> +		devm_regmap_field_free(dev, field);
> +		if (ret) {
> +			dev_err(dev, "Can't read field for registers zones\n");
> +			devm_kfree(dev, idx_bkpreg_zones_end);
> +			return ERR_PTR(ret);
> +		}
> +
> +		idx_bkpreg_zones_end[zone_idx] = offset_field - 1;
> +	}
> +
> +	/* The last zone end is defined by the number of registers in TAMP */
> +	idx_bkpreg_zones_end[zone_idx] = priv->data->nb_regs - 1;
> +
> +	return idx_bkpreg_zones_end;
> +}

[ ... ]

> +	if (priv->data->nb_zones == NB_ZONES_STM32MP2) {
> +		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"
> +			"Zone 1-RIF2 %3d - %3d %c%c\n"
> +			"Zone 2-RIF1 %3d - %3d %c%c\n"
> +			"Zone 2-RIF2 %3d - %3d %c%c\n"
> +			"Zone 3-RIF1 %3d - %3d %c%c\n"
> +			"Zone 3-RIF0 %3d - %3d %c%c\n"
> +			"Zone 3-RIF2 %3d - %3d %c%c\n",
> +			0,
> +			zones_end[BKPREG_PROTECTION_ZONE_1_RIF1],
> +			stm32_tamp_nvram_rights(priv,
> +						zones_end[BKPREG_PROTECTION_ZONE_1_RIF1],
> +						true) ? 'R' : '-',
> +			stm32_tamp_nvram_rights(priv,
> +						zones_end[BKPREG_PROTECTION_ZONE_1_RIF1],
> +						false) ? 'W' : '-',
> +			zones_end[BKPREG_PROTECTION_ZONE_1_RIF1] + 1,
> +			zones_end[BKPREG_PROTECTION_ZONE_1_RIF2],
> +			stm32_tamp_nvram_rights(priv,
> +						zones_end[BKPREG_PROTECTION_ZONE_1_RIF2],
> +						true) ? 'R' : '-',

[Severity: Medium]
Can stm32_tamp_nvram_get_backup_zones() and
stm32_tamp_nvram_bkpreg_get_zone() misclassify backup registers or misreport
zone boundaries when one or more TAMP protection zones or RIF subzones are
empty?

In STM32MP1 and STM32MP25 TAMP hardware, the zone boundary fields in
TAMP_SECCFGR and TAMP_BKPRIFRx (BKPWRIF1, BKPRWSEC, BKPWRIF2, BKPWSEC,
BKPW3RIF1, BKPW3RIF0) define cumulative register offsets from TAMP_BKP0R.

A zone k > 0 is empty when its offset equals the preceding zone's offset (so
zones_end[k] == zones_end[k - 1] >= 0), or when a subzone offset field is
left at 0 (so zones_end[k] == -1 while zones_end[k - 1] >= 0). In addition,
on STM32MP25, BKPWRIF1 only subdivides Zone 1 (bounded by BKPRWSEC) and
BKPWRIF2 only subdivides Zone 2 (bounded by BKPWSEC); if BKPWRIF1 >=
BKPRWSEC or BKPWRIF2 >= BKPWSEC, Zone 1-RIF2 or Zone 2-RIF2 is disabled and
Zone 1 or Zone 2 remains capped at BKPRWSEC or BKPWSEC.

In stm32_tamp_nvram_get_backup_zones(), idx_bkpreg_zones_end[zone_idx] is set
to offset_field - 1 without clamping BKPWRIF1 to BKPRWSEC or BKPWRIF2 to
BKPWSEC and without normalizing empty zones.

If BKPWRIF1 > BKPRWSEC or BKPWRIF2 > BKPWSEC,
stm32_tamp_nvram_bkpreg_get_zone() checks
idx_bkpreg_zones_end[BKPREG_PROTECTION_ZONE_1_RIF1] or
idx_bkpreg_zones_end[BKPREG_PROTECTION_ZONE_2_RIF1] before the parent zone
end (BKPRWSEC - 1 or BKPWSEC - 1), which would place registers beyond
BKPRWSEC or BKPWSEC into Zone 1-RIF1 or Zone 2-RIF1 and apply the wrong
access rights during stm32_tamp_nvram_read() and stm32_tamp_nvram_write().

Furthermore, stm32_tamp_nvram_bkpreg_get_zone() assumes an empty zone always
has a negative end index (reg < 0). When zone k > 0 is empty because
zones_end[k] == zones_end[k - 1] >= 0, calling
stm32_tamp_nvram_rights(priv, zones_end[k], ...) in stm32_tamp_nvram_probe()
matches zone k - 1 (the first zone with idx_bkpreg_zones_end >= reg) rather
than treating zone k as empty. Conversely, if a subzone k > 0 has
offset_field == 0 (zones_end[k] == -1) after a non-empty zone k - 1,
stm32_tamp_nvram_probe() logs the start of zone k + 1 as zones_end[k] + 1
(0) instead of zones_end[k - 1] + 1.

Should stm32_tamp_nvram_get_backup_zones() clamp the RIF1 subzone offsets to
their parent zone boundaries, and should empty zones be tracked explicitly
rather than relying on zones_end[k] < 0?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105-upstream_nvram_reworked-v5-0-ddf7dbf1b298@foss.st.com?part=3