Re: [PATCH v4 2/3] regulator: core: Support regulators where the voltage selector spans two registers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] regulator: core: NULL deref on cfg->regmap in regulator_register()
- [High] regulator: core: false -EINVAL when vsel_reg is 0 in regulator_register()
--

commit d7169bf1fc7df63ad755917c095641a4d50ecedf
Author: Paul Louvel <paul.louvel@bootlin.com>

regulator: core: Support regulators where the voltage selector spans two registers

This commit adds the vsel_reg_oversize flag to struct regulator_desc to
support regulators whose voltage selector spans two consecutive 8-bit
registers, reading and writing them in bulk as little-endian 16-bit values.

> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index 14422cfe80e2d..31f3d1ac42d09 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -6060,6 +6060,14 @@ regulator_register(struct device *dev,
>  		goto rinse;
>  	}
>  
> +	if (regulator_desc->vsel_reg_oversize) {
> +		if (regmap_get_val_bytes(cfg->regmap) != 1 ||

[Severity: High]
Can this dereference a NULL pointer when cfg->regmap is not explicitly set by
the caller?

In struct regulator_config, regmap is optional when dev_get_regmap() can
provide it, or when regulator_desc->of_parse_cb populates config->regmap
later during regulator_of_get_init_data(). In regulator_register(),
rdev->regmap is only resolved further down:

	if (config->regmap)
		rdev->regmap = config->regmap;
	else if (dev_get_regmap(dev, NULL))
		rdev->regmap = dev_get_regmap(dev, NULL);
	else if (dev->parent)
		rdev->regmap = dev_get_regmap(dev->parent, NULL);

However, regmap_get_val_bytes() unconditionally dereferences its map
argument:

int regmap_get_val_bytes(struct regmap *map)
{
	if (map->format.format_write)
		return -EINVAL;

	return map->format.val_bytes;
}

Should this check be moved after rdev->regmap is initialized, and verify that
rdev->regmap is non-NULL before calling regmap_get_val_bytes(rdev->regmap)?

> +		    regulator_desc->vsel_reg == regulator_desc->vsel_range_reg) {
> +			ret = -EINVAL;
> +			goto rinse;
> +		}
> +	}

[Severity: High]
Will this reject valid regulators with -EINVAL when pickable ranges are not
used and vsel_reg is at register offset 0?

For regulators using standard regulator_get_voltage_sel_regmap() and
regulator_set_voltage_sel_regmap() rather than pickable ranges,
vsel_range_reg and vsel_range_mask are unused and left zero-initialized in
struct regulator_desc. If such a regulator has vsel_reg_oversize set and its
voltage selector starts at register 0, regulator_desc->vsel_reg ==
regulator_desc->vsel_range_reg evaluates 0 == 0 and fails registration.

Additionally, when pickable ranges are used, an oversized vsel_reg spans both
vsel_reg and vsel_reg + 1, so vsel_range_reg could also collide with the
second register at vsel_reg + 1.

Should this check be gated on pickable ranges being used (for example,
regulator_desc->vsel_range_mask != 0) and also check for overlap with
vsel_reg + 1?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104-paul-lm51772-v4-0-494eab808581@bootlin.com?part=2