[PATCH 0/8] Fix misc ASAN reports
I've been experimenting with ASAN on sandbox and turned up a few issues that are fixed in this series.
Basic ASAN was easy to turn on, but integrating with dlmalloc was messier and fairly intrusive. Even when I had it working, there was only a small redzone between allocations which limits the usefulness.
I saw another series on the list by Sean Anderson to enable valgrind which was finding a different set of issues, though there was one overlap that Sean is fixing with "[PATCH] IOMUX: Fix access past end of console_devices".
With these issues fixed, I was able to run the dm tests without any ASAN issues. There are a couple of leaks reported at the end, but that's for another day.
Andrew Scull (8): doc: Correct position of gdb '--args' parameter acpi: Fix buffer overflow in do_acpi_dump() x86: sandbox: Add missing PCI bar to barinfo usb: sandbox: Check for string end in copy_to_unicode() usb: sandbox: Bounds check read from buffer sound: Fix buffer overflow in square wave generation test: Fix pointer overrun in dm_test_devm_regmap() test: dm: devres: Remove use-after-free
cmd/acpi.c | 2 +- doc/develop/tests_sandbox.rst | 2 +- drivers/power/acpi_pmc/pmc_emul.c | 1 + drivers/sound/sound.c | 6 ++---- drivers/usb/emul/sandbox_flash.c | 2 ++ drivers/usb/emul/usb-emul-uclass.c | 5 ++--- test/dm/devres.c | 5 +---- test/dm/regmap.c | 9 ++++----- 8 files changed, 14 insertions(+), 18 deletions(-)
The '--args' parameter to gdb comes before the binary that the debugger will be attached to rather than after the binary and before the arguments. Fix that in the docs.
Signed-off-by: Andrew Scull ascull@google.com Cc: Simon Glass sjg@chromium.org --- doc/develop/tests_sandbox.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/doc/develop/tests_sandbox.rst b/doc/develop/tests_sandbox.rst index 84608dcb84..40cf8ecdd7 100644 --- a/doc/develop/tests_sandbox.rst +++ b/doc/develop/tests_sandbox.rst @@ -103,7 +103,7 @@ running with -D will produce different results.
You can easily use gdb on these tests, without needing --gdbserver::
- $ gdb u-boot --args -T -c "ut dm gpio" + $ gdb --args u-boot -T -c "ut dm gpio" ... (gdb) break dm_test_gpio Breakpoint 1 at 0x1415bd: file test/dm/gpio.c, line 37.
On Sun, 3 Apr 2022 at 04:39, Andrew Scull ascull@google.com wrote:
The '--args' parameter to gdb comes before the binary that the debugger will be attached to rather than after the binary and before the arguments. Fix that in the docs.
Signed-off-by: Andrew Scull ascull@google.com Cc: Simon Glass sjg@chromium.org
doc/develop/tests_sandbox.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Simon Glass sjg@chromium.org
On Sun, Apr 03, 2022 at 10:39:08AM +0000, Andrew Scull wrote:
The '--args' parameter to gdb comes before the binary that the debugger will be attached to rather than after the binary and before the arguments. Fix that in the docs.
Signed-off-by: Andrew Scull ascull@google.com Cc: Simon Glass sjg@chromium.org Reviewed-by: Simon Glass sjg@chromium.org
Applied to u-boot/master, thanks!
When do_acpi_dump() converts the table name to upper case, pass the actual size of the output buffer so that the null terminator doesn't get written beyond the end of the buffer.
Signed-off-by: Andrew Scull ascull@google.com Cc: Simon Glass sjg@chromium.org Cc: Wolfgang Wallner wolfgang.wallner@br-automation.com Cc: Bin Meng bmeng.cn@gmail.com --- cmd/acpi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/cmd/acpi.c b/cmd/acpi.c index c543f1e3c2..0e473b415d 100644 --- a/cmd/acpi.c +++ b/cmd/acpi.c @@ -178,7 +178,7 @@ static int do_acpi_dump(struct cmd_tbl *cmdtp, int flag, int argc, printf("Table name '%s' must be four characters\n", name); return CMD_RET_FAILURE; } - str_to_upper(name, sig, -1); + str_to_upper(name, sig, ACPI_NAME_LEN); ret = dump_table_name(sig); if (ret) { printf("Table '%.*s' not found\n", ACPI_NAME_LEN, sig);
On Sun, 3 Apr 2022 at 04:39, Andrew Scull ascull@google.com wrote:
When do_acpi_dump() converts the table name to upper case, pass the actual size of the output buffer so that the null terminator doesn't get written beyond the end of the buffer.
Signed-off-by: Andrew Scull ascull@google.com Cc: Simon Glass sjg@chromium.org Cc: Wolfgang Wallner wolfgang.wallner@br-automation.com Cc: Bin Meng bmeng.cn@gmail.com
cmd/acpi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Simon Glass sjg@chromium.org
On Sun, Apr 03, 2022 at 10:39:09AM +0000, Andrew Scull wrote:
When do_acpi_dump() converts the table name to upper case, pass the actual size of the output buffer so that the null terminator doesn't get written beyond the end of the buffer.
Signed-off-by: Andrew Scull ascull@google.com Cc: Simon Glass sjg@chromium.org Cc: Wolfgang Wallner wolfgang.wallner@br-automation.com Cc: Bin Meng bmeng.cn@gmail.com Reviewed-by: Simon Glass sjg@chromium.org
Applied to u-boot/master, thanks!
There are expecte to be bars 0 through 5, but the last of these was missing leading to an read beyond the buffer. Add the missing element with zero values.
Signed-off-by: Andrew Scull ascull@google.com Cc: Simon Glass sjg@chromium.org Cc: Bin Meng bmeng.cn@gmail.com --- drivers/power/acpi_pmc/pmc_emul.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/power/acpi_pmc/pmc_emul.c b/drivers/power/acpi_pmc/pmc_emul.c index a61eb5bd85..8015031da8 100644 --- a/drivers/power/acpi_pmc/pmc_emul.c +++ b/drivers/power/acpi_pmc/pmc_emul.c @@ -37,6 +37,7 @@ static struct pci_bar { { 0, 0 }, { 0, 0 }, { PCI_BASE_ADDRESS_SPACE_IO, 256 }, + { 0, 0 }, };
struct pmc_emul_priv {
On Sun, 3 Apr 2022 at 04:39, Andrew Scull ascull@google.com wrote:
There are expecte to be bars 0 through 5, but the last of these was missing leading to an read beyond the buffer. Add the missing element with zero values.
Signed-off-by: Andrew Scull ascull@google.com Cc: Simon Glass sjg@chromium.org Cc: Bin Meng bmeng.cn@gmail.com
drivers/power/acpi_pmc/pmc_emul.c | 1 + 1 file changed, 1 insertion(+)
Reviewed-by: Simon Glass sjg@chromium.org
On Sun, Apr 03, 2022 at 10:39:10AM +0000, Andrew Scull wrote:
There are expecte to be bars 0 through 5, but the last of these was missing leading to an read beyond the buffer. Add the missing element with zero values.
Signed-off-by: Andrew Scull ascull@google.com Cc: Simon Glass sjg@chromium.org Cc: Bin Meng bmeng.cn@gmail.com Reviewed-by: Simon Glass sjg@chromium.org
Applied to u-boot/master, thanks!
When copying the string in copy_to_unicode(), check for the null terminator in each position, not just at the start, to avoid reading beyond the end of the string.
Signed-off-by: Andrew Scull ascull@google.com Cc: Simon Glass sjg@chromium.org Cc: Marek Vasut marex@denx.de --- drivers/usb/emul/usb-emul-uclass.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/emul/usb-emul-uclass.c b/drivers/usb/emul/usb-emul-uclass.c index 05f6d3d9e2..b31dc950e3 100644 --- a/drivers/usb/emul/usb-emul-uclass.c +++ b/drivers/usb/emul/usb-emul-uclass.c @@ -15,13 +15,12 @@ static int copy_to_unicode(char *buff, int length, const char *str) { int ptr; - int i;
if (length < 2) return 0; buff[1] = USB_DT_STRING; - for (ptr = 2, i = 0; ptr + 1 < length && *str; i++, ptr += 2) { - buff[ptr] = str[i]; + for (ptr = 2; ptr + 1 < length && *str; str++, ptr += 2) { + buff[ptr] = *str; buff[ptr + 1] = 0; } buff[0] = ptr;
On Sun, 3 Apr 2022 at 04:39, Andrew Scull ascull@google.com wrote:
When copying the string in copy_to_unicode(), check for the null terminator in each position, not just at the start, to avoid reading beyond the end of the string.
Signed-off-by: Andrew Scull ascull@google.com Cc: Simon Glass sjg@chromium.org Cc: Marek Vasut marex@denx.de
drivers/usb/emul/usb-emul-uclass.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
On Sun, Apr 03, 2022 at 10:39:11AM +0000, Andrew Scull wrote:
When copying the string in copy_to_unicode(), check for the null terminator in each position, not just at the start, to avoid reading beyond the end of the string.
Signed-off-by: Andrew Scull ascull@google.com Cc: Simon Glass sjg@chromium.org Cc: Marek Vasut marex@denx.de Reviewed-by: Simon Glass sjg@chromium.org
Applied to u-boot/master, thanks!
The buffer is 512 bytes but read requests can be 800 bytes. Limit the request to the size of the buffer.
Signed-off-by: Andrew Scull ascull@google.com Cc: Simon Glass sjg@chromium.org Cc: Marek Vasut marex@denx.de --- drivers/usb/emul/sandbox_flash.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/usb/emul/sandbox_flash.c b/drivers/usb/emul/sandbox_flash.c index edabc1b3a7..cc80f67133 100644 --- a/drivers/usb/emul/sandbox_flash.c +++ b/drivers/usb/emul/sandbox_flash.c @@ -345,6 +345,8 @@ static int sandbox_flash_bulk(struct udevice *dev, struct usb_device *udev, } else { if (priv->alloc_len && len > priv->alloc_len) len = priv->alloc_len; + if (len > sizeof(priv->buff)) + len = sizeof(priv->buff); memcpy(buff, priv->buff, len); priv->phase = PHASE_STATUS; }
On Sun, 3 Apr 2022 at 04:39, Andrew Scull ascull@google.com wrote:
The buffer is 512 bytes but read requests can be 800 bytes. Limit the request to the size of the buffer.
Signed-off-by: Andrew Scull ascull@google.com Cc: Simon Glass sjg@chromium.org Cc: Marek Vasut marex@denx.de
drivers/usb/emul/sandbox_flash.c | 2 ++ 1 file changed, 2 insertions(+)
Reviewed-by: Simon Glass sjg@chromium.org
On Sun, Apr 03, 2022 at 10:39:12AM +0000, Andrew Scull wrote:
The buffer is 512 bytes but read requests can be 800 bytes. Limit the request to the size of the buffer.
Signed-off-by: Andrew Scull ascull@google.com Cc: Simon Glass sjg@chromium.org Cc: Marek Vasut marex@denx.de Reviewed-by: Simon Glass sjg@chromium.org
Applied to u-boot/master, thanks!
Data is written for each channel but is only tracked as having one channel written. This resulted in a buffer overflow and corruption of the allocator's metadata which caused further problems when the buffer was later freed. This could be observed with sandbox unit tests.
Resolve the overflow by tracking the writes for each channel.
Fixes: f987177db9 ("dm: sound: Use the correct number of channels for sound") Signed-off-by: Andrew Scull ascull@google.com Cc: Simon Glass sjg@chromium.org --- drivers/sound/sound.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/sound/sound.c b/drivers/sound/sound.c index b0eab23391..041dfdccfe 100644 --- a/drivers/sound/sound.c +++ b/drivers/sound/sound.c @@ -25,13 +25,11 @@ void sound_create_square_wave(uint sample_rate, unsigned short *data, int size, int i, j;
for (i = 0; size && i < half; i++) { - size -= 2; - for (j = 0; j < channels; j++) + for (j = 0; size && j < channels; j++, size -= 2) *data++ = amplitude; } for (i = 0; size && i < period - half; i++) { - size -= 2; - for (j = 0; j < channels; j++) + for (j = 0; size && j < channels; j++, size -= 2) *data++ = -amplitude; } }
On Sun, 3 Apr 2022 at 04:39, Andrew Scull ascull@google.com wrote:
Data is written for each channel but is only tracked as having one channel written. This resulted in a buffer overflow and corruption of the allocator's metadata which caused further problems when the buffer was later freed. This could be observed with sandbox unit tests.
Resolve the overflow by tracking the writes for each channel.
Fixes: f987177db9 ("dm: sound: Use the correct number of channels for sound") Signed-off-by: Andrew Scull ascull@google.com Cc: Simon Glass sjg@chromium.org
drivers/sound/sound.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
On Sun, Apr 03, 2022 at 10:39:13AM +0000, Andrew Scull wrote:
Data is written for each channel but is only tracked as having one channel written. This resulted in a buffer overflow and corruption of the allocator's metadata which caused further problems when the buffer was later freed. This could be observed with sandbox unit tests.
Resolve the overflow by tracking the writes for each channel.
Fixes: f987177db9 ("dm: sound: Use the correct number of channels for sound") Signed-off-by: Andrew Scull ascull@google.com Cc: Simon Glass sjg@chromium.org Reviewed-by: Simon Glass sjg@chromium.org
Applied to u-boot/master, thanks!
This tests calls regmap_read() which takes a uint pointer as an output parameter. The test was passing a pointer to a u16 which resulted in an overflow when the output was written. Fix this by following the regmap_read() API and passing a uint pointer instead.
Signed-off-by: Andrew Scull ascull@google.com Cc: Simon Glass sjg@chromium.org Cc: Heinrich Schuchardt xypron.glpk@gmx.de Cc: Jean-Jacques Hiblot jjhiblot@ti.com Cc: Pratyush Yadav p.yadav@ti.com --- test/dm/regmap.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/test/dm/regmap.c b/test/dm/regmap.c index 04bb1645d1..8560f2afc2 100644 --- a/test/dm/regmap.c +++ b/test/dm/regmap.c @@ -286,8 +286,7 @@ U_BOOT_DRIVER(regmap_test) = { static int dm_test_devm_regmap(struct unit_test_state *uts) { int i = 0; - u16 val; - void *valp = &val; + uint val; u16 pattern[REGMAP_TEST_BUF_SZ]; u16 *buffer; struct udevice *dev; @@ -311,7 +310,7 @@ static int dm_test_devm_regmap(struct unit_test_state *uts) ut_assertok(regmap_write(priv->cfg_regmap, i, pattern[i])); } for (i = 0; i < REGMAP_TEST_BUF_SZ; i++) { - ut_assertok(regmap_read(priv->cfg_regmap, i, valp)); + ut_assertok(regmap_read(priv->cfg_regmap, i, &val)); ut_asserteq(val, buffer[i]); ut_asserteq(val, pattern[i]); } @@ -319,9 +318,9 @@ static int dm_test_devm_regmap(struct unit_test_state *uts) ut_asserteq(-ERANGE, regmap_write(priv->cfg_regmap, REGMAP_TEST_BUF_SZ, val)); ut_asserteq(-ERANGE, regmap_read(priv->cfg_regmap, REGMAP_TEST_BUF_SZ, - valp)); + &val)); ut_asserteq(-ERANGE, regmap_write(priv->cfg_regmap, -1, val)); - ut_asserteq(-ERANGE, regmap_read(priv->cfg_regmap, -1, valp)); + ut_asserteq(-ERANGE, regmap_read(priv->cfg_regmap, -1, &val));
return 0; }
On Sun, 3 Apr 2022 at 04:39, Andrew Scull ascull@google.com wrote:
This tests calls regmap_read() which takes a uint pointer as an output parameter. The test was passing a pointer to a u16 which resulted in an overflow when the output was written. Fix this by following the regmap_read() API and passing a uint pointer instead.
Signed-off-by: Andrew Scull ascull@google.com Cc: Simon Glass sjg@chromium.org Cc: Heinrich Schuchardt xypron.glpk@gmx.de Cc: Jean-Jacques Hiblot jjhiblot@ti.com Cc: Pratyush Yadav p.yadav@ti.com
test/dm/regmap.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
On Sun, Apr 03, 2022 at 10:39:14AM +0000, Andrew Scull wrote:
This tests calls regmap_read() which takes a uint pointer as an output parameter. The test was passing a pointer to a u16 which resulted in an overflow when the output was written. Fix this by following the regmap_read() API and passing a uint pointer instead.
Signed-off-by: Andrew Scull ascull@google.com Cc: Simon Glass sjg@chromium.org Cc: Heinrich Schuchardt xypron.glpk@gmx.de Cc: Jean-Jacques Hiblot jjhiblot@ti.com Cc: Pratyush Yadav p.yadav@ti.com Reviewed-by: Simon Glass sjg@chromium.org
Applied to u-boot/master, thanks!
Use-after-free shouldn't be used, even in tests. It's bad practice and makes the test brittle.
Signed-off-by: Andrew Scull ascull@google.com Cc: Simon Glass sjg@chromium.org --- test/dm/devres.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/test/dm/devres.c b/test/dm/devres.c index 4f959d11da..524114c833 100644 --- a/test/dm/devres.c +++ b/test/dm/devres.c @@ -178,11 +178,8 @@ static int dm_test_devres_phase(struct unit_test_state *uts) ut_asserteq(1, stats.allocs); ut_asserteq(TEST_DEVRES_SIZE, stats.total_size);
- /* Unbinding removes the other. Note this access a freed pointer */ + /* Unbinding removes the other. */ device_unbind(dev); - devres_get_stats(dev, &stats); - ut_asserteq(0, stats.allocs); - ut_asserteq(0, stats.total_size);
return 0; }
On Sun, 3 Apr 2022 at 04:39, Andrew Scull ascull@google.com wrote:
Use-after-free shouldn't be used, even in tests. It's bad practice and makes the test brittle.
Signed-off-by: Andrew Scull ascull@google.com Cc: Simon Glass sjg@chromium.org
test/dm/devres.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
On Sun, Apr 03, 2022 at 10:39:15AM +0000, Andrew Scull wrote:
Use-after-free shouldn't be used, even in tests. It's bad practice and makes the test brittle.
Signed-off-by: Andrew Scull ascull@google.com Cc: Simon Glass sjg@chromium.org Reviewed-by: Simon Glass sjg@chromium.org
Applied to u-boot/master, thanks!
On 4/3/22 6:39 AM, Andrew Scull wrote:
I've been experimenting with ASAN on sandbox and turned up a few issues that are fixed in this series.
Basic ASAN was easy to turn on, but integrating with dlmalloc was messier and fairly intrusive. Even when I had it working, there was only a small redzone between allocations which limits the usefulness.
Do you have any patches for this?
--Sean
I saw another series on the list by Sean Anderson to enable valgrind which was finding a different set of issues, though there was one overlap that Sean is fixing with "[PATCH] IOMUX: Fix access past end of console_devices".
With these issues fixed, I was able to run the dm tests without any ASAN issues. There are a couple of leaks reported at the end, but that's for another day.
Andrew Scull (8): doc: Correct position of gdb '--args' parameter acpi: Fix buffer overflow in do_acpi_dump() x86: sandbox: Add missing PCI bar to barinfo usb: sandbox: Check for string end in copy_to_unicode() usb: sandbox: Bounds check read from buffer sound: Fix buffer overflow in square wave generation test: Fix pointer overrun in dm_test_devm_regmap() test: dm: devres: Remove use-after-free
cmd/acpi.c | 2 +- doc/develop/tests_sandbox.rst | 2 +- drivers/power/acpi_pmc/pmc_emul.c | 1 + drivers/sound/sound.c | 6 ++---- drivers/usb/emul/sandbox_flash.c | 2 ++ drivers/usb/emul/usb-emul-uclass.c | 5 ++--- test/dm/devres.c | 5 +---- test/dm/regmap.c | 9 ++++----- 8 files changed, 14 insertions(+), 18 deletions(-)
On Wed, 6 Apr 2022 at 19:31, Sean Anderson seanga2@gmail.com wrote:
On 4/3/22 6:39 AM, Andrew Scull wrote:
I've been experimenting with ASAN on sandbox and turned up a few issues that are fixed in this series.
Basic ASAN was easy to turn on, but integrating with dlmalloc was messier and fairly intrusive. Even when I had it working, there was only a small redzone between allocations which limits the usefulness.
Do you have any patches for this?
They're becoming less of a mess as we speak, except the dlmalloc part which I don't have a good idea how to make neater. I'm actually doing it as a prerequisite for fuzzing, which I've almost got an acceptable feeling framework for and hope to be able to share fairly soon. But I could post just the ASAN parts separately if there's interest for that.
If you're using gcc, you can just add -fsanitize=address to the compiler and linker commands; clang requires some more work to rename symbols that begin with '.'.
--Sean
I saw another series on the list by Sean Anderson to enable valgrind which was finding a different set of issues, though there was one overlap that Sean is fixing with "[PATCH] IOMUX: Fix access past end of console_devices".
With these issues fixed, I was able to run the dm tests without any ASAN issues. There are a couple of leaks reported at the end, but that's for another day.
Andrew Scull (8): doc: Correct position of gdb '--args' parameter acpi: Fix buffer overflow in do_acpi_dump() x86: sandbox: Add missing PCI bar to barinfo usb: sandbox: Check for string end in copy_to_unicode() usb: sandbox: Bounds check read from buffer sound: Fix buffer overflow in square wave generation test: Fix pointer overrun in dm_test_devm_regmap() test: dm: devres: Remove use-after-free
cmd/acpi.c | 2 +- doc/develop/tests_sandbox.rst | 2 +- drivers/power/acpi_pmc/pmc_emul.c | 1 + drivers/sound/sound.c | 6 ++---- drivers/usb/emul/sandbox_flash.c | 2 ++ drivers/usb/emul/usb-emul-uclass.c | 5 ++--- test/dm/devres.c | 5 +---- test/dm/regmap.c | 9 ++++----- 8 files changed, 14 insertions(+), 18 deletions(-)
participants (4)
-
Andrew Scull -
Sean Anderson -
Simon Glass -
Tom Rini