| Message ID | 20210316122609.6523-2-marek.behun@nic.cz |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | U-Boot LTO (Sandbox + Some ARM boards) | expand |
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@nic.cz> > Reviewed-by: Simon Glass <sjg@chromium.org> > Reviewed-by: Heiko Schocher <hs@denx.de> > Reviewed-by: Bin Meng <bmeng.cn@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 >
On Tue, 16 Mar 2021 19:28:46 +0530 Pratyush Yadav <p.yadav@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@nic.cz> > > Reviewed-by: Simon Glass <sjg@chromium.org> > > Reviewed-by: Heiko Schocher <hs@denx.de> > > Reviewed-by: Bin Meng <bmeng.cn@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
On 16/03/21 03:15PM, Marek Behun wrote: > On Tue, 16 Mar 2021 19:28:46 +0530 > Pratyush Yadav <p.yadav@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@nic.cz> > > > Reviewed-by: Simon Glass <sjg@chromium.org> > > > Reviewed-by: Heiko Schocher <hs@denx.de> > > > Reviewed-by: Bin Meng <bmeng.cn@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. Message-ID: <CAPnjgZ3rE-UGjBk+sHasZy=aFbPRg3yxyG8OyAihX6yRUG3tgg@mail.gmail.com> The quoting is off but look at the last line, it says "applied to u-boot-dm/next".
On Tue, 16 Mar 2021 20:59:55 +0530 Pratyush Yadav <p.yadav@ti.com> wrote: > On 16/03/21 03:15PM, Marek Behun wrote: > > On Tue, 16 Mar 2021 19:28:46 +0530 > > Pratyush Yadav <p.yadav@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@nic.cz> > > > > Reviewed-by: Simon Glass <sjg@chromium.org> > > > > Reviewed-by: Heiko Schocher <hs@denx.de> > > > > Reviewed-by: Bin Meng <bmeng.cn@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. > > Message-ID: <CAPnjgZ3rE-UGjBk+sHasZy=aFbPRg3yxyG8OyAihX6yRUG3tgg@mail.gmail.com> > > The quoting is off but look at the last line, it says "applied to > u-boot-dm/next". > Ah, my mail client failed to parse that mail. Simon, will you drop the patch or shall I write a fix? Marek
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); + + 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); }