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.be...@nic.cz>
> Reviewed-by: Simon Glass <s...@chromium.org>
> Reviewed-by: Heiko Schocher <h...@denx.de>
> Reviewed-by: Bin Meng <bmeng...@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. 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.
> +
> + return 0;
> }
>
> static inline void __write_8(u8 *addr, const u8 *val,
> @@ -546,6 +555,8 @@ int regmap_raw_write(struct regmap *map, uint offset,
> const void *val,
>
> int regmap_write(struct regmap *map, uint offset, uint val)
> {
> + val = cpu_to_le32(val);
> +
> return regmap_raw_write(map, offset, &val, map->width);
> }
>
> --
> 2.26.2
>
--
Regards,
Pratyush Yadav
Texas Instruments Inc.
