| Message ID | 20210316150740.29878-1-marek.behun@nic.cz |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | [u-boot,v3.1,01/39] regmap: fix a serious pointer casting bug | expand |
Simon, Heiko, Bin,
Pratyush discovered that the solution implemented by the patch
regmap: fix a serious pointer casting bug
is wrong. The cpu_to_le32() / le32_to_cpu() shifts data to the correct
position, but on big endian machines it also reverses byte order.
Somehow this went right through my head when I thought this up.
I have sent a new version, with subject
[PATCH u-boot v3.1 01/39] regmap: fix a serious pointer casting bug
The new solution utilizes an union { u8; u16; u32; u64; }, since all
members of an union start at the same address.
Could you please review this? Thanks.
Marek
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@nic.cz> > Cc: Simon Glass <sjg@chromium.org> > Cc: Heiko Schocher <hs@denx.de> > Cc: Bin Meng <bmeng.cn@gmail.com> > Cc: Pratyush Yadav <p.yadav@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 >
On Tue, 16 Mar 2021 22:04:17 +0530 Pratyush Yadav <p.yadav@ti.com> wrote: > > + 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. If only these two options are possible, then the first option is better. The regmap_read function ought to be simple, so no void * pointer. There is another option: if a value greater than ULONG_MAX is read, the regmap_read() function will return -ERANGE. This way this function will remain simple. ulong is 64-bit wide on 64-bit devices, so this only is a problem when a 64-bit wide regmap is used on a 32-bit device. I think we should keep regmap_read() simple, and for people who use a 64-bit wide regmap on 32-bit device, there is regmap_raw_read(). But I think this problem should be solved (however we decide) in a follow-up patch. This patch fixes the pointer casting bug, and commits should remain atomic (fixing one thing). Marek
Hi Marek, On Wed, 17 Mar 2021 at 04:19, Marek Behun <marek.behun@nic.cz> wrote: > > Simon, Heiko, Bin, > > Pratyush discovered that the solution implemented by the patch > regmap: fix a serious pointer casting bug > is wrong. The cpu_to_le32() / le32_to_cpu() shifts data to the correct > position, but on big endian machines it also reverses byte order. > Somehow this went right through my head when I thought this up. > > I have sent a new version, with subject > [PATCH u-boot v3.1 01/39] regmap: fix a serious pointer casting bug > > The new solution utilizes an union { u8; u16; u32; u64; }, since all > members of an union start at the same address. > > Could you please review this? Thanks. You already have my review tag on that. I dropped it from u-boot-dm/next. The problem was that it v1 was in my queue, but not later versions. Regards, Simon
On Thu, 25 Mar 2021 13:38:13 +1300 Simon Glass <sjg@chromium.org> wrote: > Hi Marek, > > On Wed, 17 Mar 2021 at 04:19, Marek Behun <marek.behun@nic.cz> wrote: > > > > Simon, Heiko, Bin, > > > > Pratyush discovered that the solution implemented by the patch > > regmap: fix a serious pointer casting bug > > is wrong. The cpu_to_le32() / le32_to_cpu() shifts data to the correct > > position, but on big endian machines it also reverses byte order. > > Somehow this went right through my head when I thought this up. > > > > I have sent a new version, with subject > > [PATCH u-boot v3.1 01/39] regmap: fix a serious pointer casting bug > > > > The new solution utilizes an union { u8; u16; u32; u64; }, since all > > members of an union start at the same address. > > > > Could you please review this? Thanks. > > You already have my review tag on that. > > I dropped it from u-boot-dm/next. The problem was that it v1 was in my > queue, but not later versions. > > Regards, > Simon I don't have your review for version v3.1 :) to which you just replied. I had your review tag for v3, and sent v3.1 as a reply to Pratyush'scomplains on v3. Marek
eHi Marek, On Thu, 25 Mar 2021 at 13:46, Marek Behun <marek.behun@nic.cz> wrote: > > On Thu, 25 Mar 2021 13:38:13 +1300 > Simon Glass <sjg@chromium.org> wrote: > > > Hi Marek, > > > > On Wed, 17 Mar 2021 at 04:19, Marek Behun <marek.behun@nic.cz> wrote: > > > > > > Simon, Heiko, Bin, > > > > > > Pratyush discovered that the solution implemented by the patch > > > regmap: fix a serious pointer casting bug > > > is wrong. The cpu_to_le32() / le32_to_cpu() shifts data to the correct > > > position, but on big endian machines it also reverses byte order. > > > Somehow this went right through my head when I thought this up. > > > > > > I have sent a new version, with subject > > > [PATCH u-boot v3.1 01/39] regmap: fix a serious pointer casting bug > > > > > > The new solution utilizes an union { u8; u16; u32; u64; }, since all > > > members of an union start at the same address. > > > > > > Could you please review this? Thanks. > > > > You already have my review tag on that. > > > > I dropped it from u-boot-dm/next. The problem was that it v1 was in my > > queue, but not later versions. > > > > Regards, > > Simon > > I don't have your review for version v3.1 :) to which you just replied. > > I had your review tag for v3, and sent v3.1 as a reply to > Pratyush'scomplains on v3. Fractional versions? In that case I'm going to hold out for version 3.125. Regards, Simon
On Thu, 25 Mar 2021 15:41:55 +1300 Simon Glass <sjg@chromium.org> wrote: > eHi Marek, > > On Thu, 25 Mar 2021 at 13:46, Marek Behun <marek.behun@nic.cz> wrote: > > > > On Thu, 25 Mar 2021 13:38:13 +1300 > > Simon Glass <sjg@chromium.org> wrote: > > > > > Hi Marek, > > > > > > On Wed, 17 Mar 2021 at 04:19, Marek Behun <marek.behun@nic.cz> wrote: > > > > > > > > Simon, Heiko, Bin, > > > > > > > > Pratyush discovered that the solution implemented by the patch > > > > regmap: fix a serious pointer casting bug > > > > is wrong. The cpu_to_le32() / le32_to_cpu() shifts data to the correct > > > > position, but on big endian machines it also reverses byte order. > > > > Somehow this went right through my head when I thought this up. > > > > > > > > I have sent a new version, with subject > > > > [PATCH u-boot v3.1 01/39] regmap: fix a serious pointer casting bug > > > > > > > > The new solution utilizes an union { u8; u16; u32; u64; }, since all > > > > members of an union start at the same address. > > > > > > > > Could you please review this? Thanks. > > > > > > You already have my review tag on that. > > > > > > I dropped it from u-boot-dm/next. The problem was that it v1 was in my > > > queue, but not later versions. > > > > > > Regards, > > > Simon > > > > I don't have your review for version v3.1 :) to which you just replied. > > > > I had your review tag for v3, and sent v3.1 as a reply to > > Pratyush'scomplains on v3. > > Fractional versions? In that case I'm going to hold out for version 3.125. :) I sent it as a reply for v3, and did not want to call it v4 because for v4 I am going to send the whole series again. Sorry. Anyway I probably will send v4 next week, there are still some problems and I am working on something different now. Marek
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; + 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)
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@nic.cz> Cc: Simon Glass <sjg@chromium.org> Cc: Heiko Schocher <hs@denx.de> Cc: Bin Meng <bmeng.cn@gmail.com> Cc: Pratyush Yadav <p.yadav@ti.com> --- drivers/core/regmap.c | 59 +++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 57 insertions(+), 2 deletions(-)