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

[Marek Behun]

On Tue, 16 Mar 2021 19:28:46 +0530
Pratyush Yadav <p.yadav at ti.com> wrote:

> On 16/03/21 01:25PM, 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 simply by taking into account that regmap_raw_read() and
> > regmap_raw_write() behave as if the data given to these functions were
> > in little-endian format, i.e. use cpu_to_le32() / le32_to_cpu(). In
> > regmap_read() also zero out the space so that we don't get invalid
> > result if regmap_raw_read() does not fill the whole object.
> > 
> > Signed-off-by: Marek Behún <marek.behun at nic.cz>
> > Reviewed-by: Simon Glass <sjg at chromium.org>
> > Reviewed-by: Heiko Schocher <hs at denx.de>
> > Reviewed-by: Bin Meng <bmeng.cn at gmail.com>
> > ---
> >  drivers/core/regmap.c | 13 ++++++++++++-
> >  1 file changed, 12 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/core/regmap.c b/drivers/core/regmap.c
> > index b51ce108c1..5d37006fff 100644
> > --- a/drivers/core/regmap.c
> > +++ b/drivers/core/regmap.c
> > @@ -435,7 +435,16 @@ 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);
> > +	int res;
> > +
> > +	*valp = 0;
> > +	res = regmap_raw_read(map, offset, valp, map->width);
> > +	if (res)
> > +		return res;
> > +
> > +	*valp = le32_to_cpu(*valp);  
> 
> Looks like I'm a bit late to the party and Simon has already applied 
> this patch.

Where did he apply? I don't see it applied in u-boot-dm.

> Anyway, I don't see why this is correct. regmap_raw_read() 
> calls regmap_raw_read_range(), which calls the helpers __read_16(), 
> __read_32() and so on.
> 
> Take __read_16() for example. It takes the regmap's endianness and then 
> based on that calls in_le16() or in_be16(). These calls translate to 
> le16_to_cpu(__raw_readw(a)) or be16_to_cpu(__raw_readw(a)). Or the 
> regmap is native endian in which case it is a simple readw(a).
> 
> In all 3 cases the value returned is in cpu endianness. But you claim 
> that "regmap_raw_read() and regmap_raw_write() behave as if the data 
> given to these functions were in little-endian format".
> 
> This is fine on a little endian cpu but on a big endian cpu you would 
> reverse the byte order, no? Same for writes.

I made a mistake.

Somehow I thought that le32_to_cpu() will fix this, because it will
move the read bytes into the correct position. But somehow I forgot that
it will also reverse the byte order /o\ :D

I shall fix this and send a new version :D