[PATCH u-boot v3.1 01/39] regmap: fix a serious pointer casting bug
From: Pratyush Yadav <hidden>
Date: 2021-03-16 16:34:17
Hi Marek, On 16/03/21 04:07PM, Marek Beh?n wrote:
quoted hunk ↗ jump to hunk
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 <redacted> Cc: Simon Glass <sjg@chromium.org> Cc: Heiko Schocher <redacted> Cc: Bin Meng <redacted> Cc: Pratyush Yadav <redacted> --- 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.
quoted hunk ↗ jump to hunk
+ 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.