[PATCH u-boot v3.1 01/39] regmap: fix a serious pointer casting bug

[Pratyush Yadav]

Hi Marek,

On 16/03/21 04:07PM, Marek Behún wrote:
> There is a serious bug in regmap_read() and regmap_write() functions
> where an uint pointer is cast to (void *) which is then cast to (u8 *),
> (u16 *), (u32 *) or (u64 *), depending on register width of the map.
> 
> For example given a regmap with 16-bit register width the code
> 	int val = 0x12340000;
> 	regmap_read(map, 0, &val);
> only changes the lower 16 bits of val on little-endian machines.
> The upper 16 bits will remain 0x1234.
> 
> Nobody noticed this probably because this bug can be triggered with
> regmap_write() only on big-endian architectures (which are not used by
> many people anymore), and on little endian this bug has consequences
> only if register width is 8 or 16 bits and also the memory place to
> which regmap_read() should store it's result has non-zero upper bits,
> which it seems doesn't happen anywhere in U-Boot normally. CI managed to
> trigger this bug in unit test of dm_test_devm_regmap_field when compiled
> for sandbox_defconfig using LTO.
> 
> Fix this by utilizing an union { u8; u16; u32; u64; } and reading data
> into this union / writing data from this union.
> 
> Signed-off-by: Marek Behún <marek.behun at nic.cz>
> Cc: Simon Glass <sjg at chromium.org>
> Cc: Heiko Schocher <hs at denx.de>
> Cc: Bin Meng <bmeng.cn at gmail.com>
> Cc: Pratyush Yadav <p.yadav at ti.com>
> ---
>  drivers/core/regmap.c | 59 +++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 57 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/core/regmap.c b/drivers/core/regmap.c
> index b51ce108c1..3206f3d112 100644
> --- a/drivers/core/regmap.c
> +++ b/drivers/core/regmap.c
> @@ -435,7 +435,36 @@ int regmap_raw_read(struct regmap *map, uint offset, void *valp, size_t val_len)
>  
>  int regmap_read(struct regmap *map, uint offset, uint *valp)
>  {
> -	return regmap_raw_read(map, offset, valp, map->width);
> +	union {
> +		u8 v8;
> +		u16 v16;
> +		u32 v32;
> +		u64 v64;
> +	} u;
> +	int res;
> +
> +	res = regmap_raw_read(map, offset, &u, map->width);
> +	if (res)
> +		return res;
> +
> +	switch (map->width) {
> +	case REGMAP_SIZE_8:
> +		*valp = u.v8;
> +		break;
> +	case REGMAP_SIZE_16:
> +		*valp = u.v16;
> +		break;
> +	case REGMAP_SIZE_32:
> +		*valp = u.v32;
> +		break;
> +	case REGMAP_SIZE_64:
> +		*valp = u.v64;
> +		break;

I think this should fix the problem you are trying to solve.

But I see another problem with this code. What if someone wants to read 
8 bytes? IIUC, since valp points to a uint, *valp = u.v64 will result in 
4 bytes being truncated from the result.

I see two options:

- Change the uint pointer to u64 pointer and update every driver to use 
  a u64 when using the regmap.

- Change the uint pointer to void pointer and expect every driver to 
  pass a container with exactly the required size, based on map->width. 
  Update the ones that don't follow this.

I prefer the latter option.

> +	default:
> +		unreachable();
> +	}
> +
> +	return 0;
>  }
>  
>  static inline void __write_8(u8 *addr, const u8 *val,
> @@ -546,7 +575,33 @@ int regmap_raw_write(struct regmap *map, uint offset, const void *val,
>  
>  int regmap_write(struct regmap *map, uint offset, uint val)
>  {
> -	return regmap_raw_write(map, offset, &val, map->width);
> +	union {
> +		u8 v8;
> +		u16 v16;
> +		u32 v32;
> +		u64 v64;
> +	} u;
> +
> +	switch (map->width) {
> +	case REGMAP_SIZE_8:
> +		u.v8 = val;
> +		break;
> +	case REGMAP_SIZE_16:
> +		u.v16 = val;
> +		break;
> +	case REGMAP_SIZE_32:
> +		u.v32 = val;
> +		break;
> +	case REGMAP_SIZE_64:
> +		u.v64 = val;
> +		break;
> +	default:
> +		debug("%s: regmap size %zu unknown\n", __func__,
> +		      (size_t)map->width);
> +		return -EINVAL;
> +	}
> +
> +	return regmap_raw_write(map, offset, &u, map->width);
>  }
>  
>  int regmap_update_bits(struct regmap *map, uint offset, uint mask, uint val)
> -- 
> 2.26.2
> 

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.