[PATCH u-boot] regmap: fix a serious pointer casting bug
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 --- 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); + + 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); }
On Wed, 3 Mar 2021 at 08:16, Marek Behún marek.behun@nic.cz 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
drivers/core/regmap.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)
Reviewed-by: Simon Glass sjg@chromium.org
Hello Marek,
On 03.03.21 14:15, 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
drivers/core/regmap.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)
Good catch, thanks!
Reviewed-by: Heiko Schocher hs@denx.de
bye, Heiko
Hello Marek,
On 03.03.21 14:15, 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
drivers/core/regmap.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)
Good catch, thanks!
Reviewed-by: Heiko Schocher hs@denx.de
bye, Heiko
Hi Simon,
On Tue, Mar 16, 2021 at 2:10 PM Simon Glass sjg@chromium.org wrote:
Hello Marek,
On 03.03.21 14:15, 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
drivers/core/regmap.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)
Good catch, thanks!
Reviewed-by: Heiko Schocher hs@denx.de
bye, Heiko -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-52 Fax: +49-8142-66989-80 Email: hs@denx.de
Applied to u-boot-dm/next, thanks!
This patch is included in the LTO series.
Regards, Bin
Hi Bin,
On Tue, 16 Mar 2021 at 21:46, Bin Meng bmeng.cn@gmail.com wrote:
Hi Simon,
On Tue, Mar 16, 2021 at 2:10 PM Simon Glass sjg@chromium.org wrote:
Hello Marek,
On 03.03.21 14:15, 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
drivers/core/regmap.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)
Good catch, thanks!
Reviewed-by: Heiko Schocher hs@denx.de
bye, Heiko -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-52 Fax: +49-8142-66989-80 Email: hs@denx.de
Applied to u-boot-dm/next, thanks!
This patch is included in the LTO series.
OK, I'll drop it.
Regards, Simon
participants (4)
-
Bin Meng -
Heiko Schocher -
Marek Behún -
Simon Glass