[PATCH v2 0/2] Console/stdio use after free
With today's master, 70c2525c0d3c ('IOMUX: Stop dropped consoles') introduces a use after free in usb_kbd_remove():
- usbkbd's stdio device is de-registered with stdio_deregister_dev(), the struct stdio_dev is freed.
- iomux_doenv() is called, usbkbd removed from the console list, and console_stop() is called on the struct stdio_dev pointer that no longer exists.
This series mitigates this by making sure the pointer is really a stdio device prior performing the stop operation. It's not ideal, but I couldn't figure out a nicer way to fix this.
Regards, Nicolas
---
Changes since v1: - Comment new function - Add comment stating this needs a proper fix
Nicolas Saenz Julienne (2): stdio: Introduce stdio_valid() console: Don't start/stop console if stdio device invalid
common/console.c | 9 +++++++++ common/stdio.c | 11 +++++++++++ include/stdio_dev.h | 11 +++++++++++ 3 files changed, 31 insertions(+)
stdio_valid() will confirm that a struct stdio_dev pointer is indeed valid.
Signed-off-by: Nicolas Saenz Julienne nsaenzjulienne@suse.de
---
Changes since v1: - Properly document function
common/stdio.c | 11 +++++++++++ include/stdio_dev.h | 11 +++++++++++ 2 files changed, 22 insertions(+)
diff --git a/common/stdio.c b/common/stdio.c index abf9b1e915..69b7d2692d 100644 --- a/common/stdio.c +++ b/common/stdio.c @@ -157,6 +157,17 @@ static int stdio_probe_device(const char *name, enum uclass_id id, return 0; }
+bool stdio_valid(struct stdio_dev *dev) +{ + struct stdio_dev *sdev; + + list_for_each_entry(sdev, &devs.list, list) + if (sdev == dev) + return true; + + return false; +} + struct stdio_dev *stdio_get_by_name(const char *name) { struct list_head *pos; diff --git a/include/stdio_dev.h b/include/stdio_dev.h index 48871a6a22..e9d610a840 100644 --- a/include/stdio_dev.h +++ b/include/stdio_dev.h @@ -98,6 +98,17 @@ struct list_head *stdio_get_list(void); struct stdio_dev *stdio_get_by_name(const char *name); struct stdio_dev *stdio_clone(struct stdio_dev *dev);
+/** + * stdio_valid() - Validate stdio device exists + * + * Verifies that dev is contained in the global list of stdio devices. + * + * @dev: Stdio device pointer + * + * returns 'true' when valid, 'false' otherwise + */ +bool stdio_valid(struct stdio_dev *dev); + int drv_lcd_init(void); int drv_video_init(void); int drv_keyboard_init(void);
On Thu, 28 Jan 2021 at 06:12, Nicolas Saenz Julienne nsaenzjulienne@suse.de wrote:
stdio_valid() will confirm that a struct stdio_dev pointer is indeed valid.
Signed-off-by: Nicolas Saenz Julienne nsaenzjulienne@suse.de
Changes since v1:
- Properly document function
common/stdio.c | 11 +++++++++++ include/stdio_dev.h | 11 +++++++++++ 2 files changed, 22 insertions(+)
Reviewed-by: Simon Glass sjg@chromium.org
Don't start/stop an stdio device that might have been already freed.
Signed-off-by: Nicolas Saenz Julienne nsaenzjulienne@suse.de Fixes: 70c2525c0d3c ("IOMUX: Stop dropped consoles")
--- Changes since v1: - Add comment stating this should be properly fixed
common/console.c | 9 +++++++++ 1 file changed, 9 insertions(+)
diff --git a/common/console.c b/common/console.c index f3cc45cab5..570f26d149 100644 --- a/common/console.c +++ b/common/console.c @@ -252,6 +252,15 @@ static bool console_needs_start_stop(int file, struct stdio_dev *sdev) { int i, j;
+ /* + * TODO: This is a workaround to avoid accessing freed memory: + * console_stop() might be called on an stdio_dev that has already been + * de-registered, due to the fact that stdio_deregister_dev() + * doesn't update the global console_devices array. + */ + if (!stdio_valid(sdev)) + return false; + for (i = 0; i < ARRAY_SIZE(cd_count); i++) { if (i == file) continue;
On Thu, Jan 28, 2021 at 02:12:40PM +0100, Nicolas Saenz Julienne wrote:
Don't start/stop an stdio device that might have been already freed.
Signed-off-by: Nicolas Saenz Julienne nsaenzjulienne@suse.de Fixes: 70c2525c0d3c ("IOMUX: Stop dropped consoles")
...
- /*
* TODO: This is a workaround to avoid accessing freed memory:* console_stop() might be called on an stdio_dev that has already been* de-registered, due to the fact that stdio_deregister_dev()* doesn't update the global console_devices array.*/
I see now. I think I have experienced this issue from time to time. I will look at it. Tom, Simon, please hold on applying these for a while.
- if (!stdio_valid(sdev))
return false;
On 28/01/2021 16:52, Andy Shevchenko wrote:
On Thu, Jan 28, 2021 at 02:12:40PM +0100, Nicolas Saenz Julienne wrote:
Don't start/stop an stdio device that might have been already freed.
Signed-off-by: Nicolas Saenz Julienne nsaenzjulienne@suse.de Fixes: 70c2525c0d3c ("IOMUX: Stop dropped consoles")
...
- /*
* TODO: This is a workaround to avoid accessing freed memory:* console_stop() might be called on an stdio_dev that has already been* de-registered, due to the fact that stdio_deregister_dev()* doesn't update the global console_devices array.*/I see now. I think I have experienced this issue from time to time. I will look at it. Tom, Simon, please hold on applying these for a while.
Just for the notes, the failing tests hold back Nicolas series to support RPi400/CM4 [1] as it does not run the new tests added successfully. If it takes a long time to fix the test environment, I'd vote to take this series as a stop-gap so that we can support that HW in the next release.
Regards, Matthias
[1] https://patchwork.ozlabs.org/project/uboot/list/?series=223890
On Fri, Jan 29, 2021 at 09:50:50AM +0100, Matthias Brugger wrote:
On 28/01/2021 16:52, Andy Shevchenko wrote:
On Thu, Jan 28, 2021 at 02:12:40PM +0100, Nicolas Saenz Julienne wrote:
Don't start/stop an stdio device that might have been already freed.
Signed-off-by: Nicolas Saenz Julienne nsaenzjulienne@suse.de Fixes: 70c2525c0d3c ("IOMUX: Stop dropped consoles")
...
- /*
* TODO: This is a workaround to avoid accessing freed memory:* console_stop() might be called on an stdio_dev that has already been* de-registered, due to the fact that stdio_deregister_dev()* doesn't update the global console_devices array.*/I see now. I think I have experienced this issue from time to time. I will look at it. Tom, Simon, please hold on applying these for a while.
Just for the notes, the failing tests hold back Nicolas series to support RPi400/CM4 [1] as it does not run the new tests added successfully. If it takes a long time to fix the test environment, I'd vote to take this series as a stop-gap so that we can support that HW in the next release.
Andy, since you're working on a better solution, do you want more time for that or should I pick this series up for now and you can revert it as part of your better fix? Thanks!
On Mon, Feb 1, 2021 at 9:29 PM Tom Rini trini@konsulko.com wrote:
On Fri, Jan 29, 2021 at 09:50:50AM +0100, Matthias Brugger wrote:
On 28/01/2021 16:52, Andy Shevchenko wrote:
On Thu, Jan 28, 2021 at 02:12:40PM +0100, Nicolas Saenz Julienne wrote:
Don't start/stop an stdio device that might have been already freed.
Signed-off-by: Nicolas Saenz Julienne nsaenzjulienne@suse.de Fixes: 70c2525c0d3c ("IOMUX: Stop dropped consoles")
...
- /*
- TODO: This is a workaround to avoid accessing freed memory:
- console_stop() might be called on an stdio_dev that has already been
- de-registered, due to the fact that stdio_deregister_dev()
- doesn't update the global console_devices array.
- */
I see now. I think I have experienced this issue from time to time. I will look at it. Tom, Simon, please hold on applying these for a while.
Just for the notes, the failing tests hold back Nicolas series to support RPi400/CM4 [1] as it does not run the new tests added successfully. If it takes a long time to fix the test environment, I'd vote to take this series as a stop-gap so that we can support that HW in the next release.
Andy, since you're working on a better solution, do you want more time for that or should I pick this series up for now and you can revert it as part of your better fix? Thanks!
Sorry for the delayed reply. Give me a couple of days, and if I will come up without any (good) solution, apply this series.
On Wed, Feb 03, 2021 at 11:50:53AM +0200, Andy Shevchenko wrote:
On Mon, Feb 1, 2021 at 9:29 PM Tom Rini trini@konsulko.com wrote:
On Fri, Jan 29, 2021 at 09:50:50AM +0100, Matthias Brugger wrote:
On 28/01/2021 16:52, Andy Shevchenko wrote:
On Thu, Jan 28, 2021 at 02:12:40PM +0100, Nicolas Saenz Julienne wrote:
Don't start/stop an stdio device that might have been already freed.
Signed-off-by: Nicolas Saenz Julienne nsaenzjulienne@suse.de Fixes: 70c2525c0d3c ("IOMUX: Stop dropped consoles")
...
- /*
- TODO: This is a workaround to avoid accessing freed memory:
- console_stop() might be called on an stdio_dev that has already been
- de-registered, due to the fact that stdio_deregister_dev()
- doesn't update the global console_devices array.
- */
I see now. I think I have experienced this issue from time to time. I will look at it. Tom, Simon, please hold on applying these for a while.
Just for the notes, the failing tests hold back Nicolas series to support RPi400/CM4 [1] as it does not run the new tests added successfully. If it takes a long time to fix the test environment, I'd vote to take this series as a stop-gap so that we can support that HW in the next release.
Andy, since you're working on a better solution, do you want more time for that or should I pick this series up for now and you can revert it as part of your better fix? Thanks!
Sorry for the delayed reply. Give me a couple of days, and if I will come up without any (good) solution, apply this series.
Okay, as promised, I prepared a branch [1] with new approach, but while I will be busy with other stuff, I would like you to test on real hardware and tell if it helps. At least it passes test cases.
If it works, I would like to get a Tested-by tag and will prepare and submit the formal series.
[1]: https://github.com/andy-shev/u-boot/tree/iomux
On Fri, Feb 05, 2021 at 07:06:54PM +0200, Andy Shevchenko wrote:
On Wed, Feb 03, 2021 at 11:50:53AM +0200, Andy Shevchenko wrote:
On Mon, Feb 1, 2021 at 9:29 PM Tom Rini trini@konsulko.com wrote:
On Fri, Jan 29, 2021 at 09:50:50AM +0100, Matthias Brugger wrote:
On 28/01/2021 16:52, Andy Shevchenko wrote:
On Thu, Jan 28, 2021 at 02:12:40PM +0100, Nicolas Saenz Julienne wrote:
Don't start/stop an stdio device that might have been already freed.
Signed-off-by: Nicolas Saenz Julienne nsaenzjulienne@suse.de Fixes: 70c2525c0d3c ("IOMUX: Stop dropped consoles")
...
- /*
- TODO: This is a workaround to avoid accessing freed memory:
- console_stop() might be called on an stdio_dev that has already been
- de-registered, due to the fact that stdio_deregister_dev()
- doesn't update the global console_devices array.
- */
I see now. I think I have experienced this issue from time to time. I will look at it. Tom, Simon, please hold on applying these for a while.
Just for the notes, the failing tests hold back Nicolas series to support RPi400/CM4 [1] as it does not run the new tests added successfully. If it takes a long time to fix the test environment, I'd vote to take this series as a stop-gap so that we can support that HW in the next release.
Andy, since you're working on a better solution, do you want more time for that or should I pick this series up for now and you can revert it as part of your better fix? Thanks!
Sorry for the delayed reply. Give me a couple of days, and if I will come up without any (good) solution, apply this series.
Okay, as promised, I prepared a branch [1] with new approach, but while I will be busy with other stuff, I would like you to test on real hardware and tell if it helps. At least it passes test cases.
If it works, I would like to get a Tested-by tag and will prepare and submit the formal series.
I reliably get:
========================================== FAILURES =========================================== _____________________________ test_ut[ut_dm_fdt_livetree_writing] _____________________________
u_boot_console = <u_boot_console_sandbox.ConsoleSandbox object at 0x7fe251aeca90> ut_subtest = 'dm fdt_livetree_writing'
def test_ut(u_boot_console, ut_subtest): """Execute a "ut" subtest.
The subtests are collected in function generate_ut_subtest() from linker generated lists by applying a regular expression to the lines of file u-boot.sym. The list entries are created using the C macro UNIT_TEST().
Strict naming conventions have to be followed to match the regular expression. Use UNIT_TEST(foo_test_bar, _flags, foo_test) for a test bar in test suite foo that can be executed via command 'ut foo bar' and is implemented in C function foo_test_bar().
Args: u_boot_console (ConsoleBase): U-Boot console ut_subtest (str): test to be executed via command ut, e.g 'foo bar' to execute command 'ut foo bar' """
output = u_boot_console.run_command('ut ' + ut_subtest)
assert output.endswith('Failures: 0')
E AssertionError: assert False E + where False = <built-in method endswith of str object at 0x7fe2516c5da0>('Failures: 0') E + where <built-in method endswith of str object at 0x7fe2516c5da0> = 'Test: dm_test_fdt_livetree_writing: test-fdt.c\r\r\ntest/dm/test-fdt.c:837, dm_test_fdt_livetree_writing(): FDT_ADDR_...\r\nTest: dm_test_fdt_livetree_writing: test-fdt.c (flat tree)\r\r\nLive tree not active; ignore test\r\r\nFailures: 1'.endswith
test/py/tests/test_ut.py:43: AssertionError ------------------------------------ Captured stdout call ------------------------------------- => ut dm fdt_livetree_writing Test: dm_test_fdt_livetree_writing: test-fdt.c test/dm/test-fdt.c:837, dm_test_fdt_livetree_writing(): FDT_ADDR_T_NONE == dev_read_addr(dev): Expected 0xffffffff (-1), got 0x42 (66) Test: dm_test_fdt_livetree_writing: test-fdt.c (flat tree) Live tree not active; ignore test Failures: 1 =>
On sandbox. I'm going to take sandbox out of my testing loop for the moment and see what Pi and a few others do.
On Fri, Feb 05, 2021 at 12:50:56PM -0500, Tom Rini wrote:
On Fri, Feb 05, 2021 at 07:06:54PM +0200, Andy Shevchenko wrote:
On Wed, Feb 03, 2021 at 11:50:53AM +0200, Andy Shevchenko wrote:
On Mon, Feb 1, 2021 at 9:29 PM Tom Rini trini@konsulko.com wrote:
On Fri, Jan 29, 2021 at 09:50:50AM +0100, Matthias Brugger wrote:
On 28/01/2021 16:52, Andy Shevchenko wrote:
On Thu, Jan 28, 2021 at 02:12:40PM +0100, Nicolas Saenz Julienne wrote: > Don't start/stop an stdio device that might have been already freed. > > Signed-off-by: Nicolas Saenz Julienne nsaenzjulienne@suse.de > Fixes: 70c2525c0d3c ("IOMUX: Stop dropped consoles")
...
> + /* > + * TODO: This is a workaround to avoid accessing freed memory: > + * console_stop() might be called on an stdio_dev that has already been > + * de-registered, due to the fact that stdio_deregister_dev() > + * doesn't update the global console_devices array. > + */
I see now. I think I have experienced this issue from time to time. I will look at it. Tom, Simon, please hold on applying these for a while.
Just for the notes, the failing tests hold back Nicolas series to support RPi400/CM4 [1] as it does not run the new tests added successfully. If it takes a long time to fix the test environment, I'd vote to take this series as a stop-gap so that we can support that HW in the next release.
Andy, since you're working on a better solution, do you want more time for that or should I pick this series up for now and you can revert it as part of your better fix? Thanks!
Sorry for the delayed reply. Give me a couple of days, and if I will come up without any (good) solution, apply this series.
Okay, as promised, I prepared a branch [1] with new approach, but while I will be busy with other stuff, I would like you to test on real hardware and tell if it helps. At least it passes test cases.
If it works, I would like to get a Tested-by tag and will prepare and submit the formal series.
I reliably get:
========================================== FAILURES =========================================== _____________________________ test_ut[ut_dm_fdt_livetree_writing] _____________________________
u_boot_console = <u_boot_console_sandbox.ConsoleSandbox object at 0x7fe251aeca90> ut_subtest = 'dm fdt_livetree_writing'
def test_ut(u_boot_console, ut_subtest): """Execute a "ut" subtest. The subtests are collected in function generate_ut_subtest() from linker generated lists by applying a regular expression to the lines of file u-boot.sym. The list entries are created using the C macro UNIT_TEST(). Strict naming conventions have to be followed to match the regular expression. Use UNIT_TEST(foo_test_bar, _flags, foo_test) for a test bar in test suite foo that can be executed via command 'ut foo bar' and is implemented in C function foo_test_bar(). Args: u_boot_console (ConsoleBase): U-Boot console ut_subtest (str): test to be executed via command ut, e.g 'foo bar' to execute command 'ut foo bar' """ output = u_boot_console.run_command('ut ' + ut_subtest)assert output.endswith('Failures: 0')E AssertionError: assert False E + where False = <built-in method endswith of str object at 0x7fe2516c5da0>('Failures: 0') E + where <built-in method endswith of str object at 0x7fe2516c5da0> = 'Test: dm_test_fdt_livetree_writing: test-fdt.c\r\r\ntest/dm/test-fdt.c:837, dm_test_fdt_livetree_writing(): FDT_ADDR_...\r\nTest: dm_test_fdt_livetree_writing: test-fdt.c (flat tree)\r\r\nLive tree not active; ignore test\r\r\nFailures: 1'.endswith
test/py/tests/test_ut.py:43: AssertionError ------------------------------------ Captured stdout call ------------------------------------- => ut dm fdt_livetree_writing Test: dm_test_fdt_livetree_writing: test-fdt.c test/dm/test-fdt.c:837, dm_test_fdt_livetree_writing(): FDT_ADDR_T_NONE == dev_read_addr(dev): Expected 0xffffffff (-1), got 0x42 (66) Test: dm_test_fdt_livetree_writing: test-fdt.c (flat tree) Live tree not active; ignore test Failures: 1 =>
On sandbox. I'm going to take sandbox out of my testing loop for the moment and see what Pi and a few others do.
As expected, the rest of the tests (which did pass in sandbox) also pass on Pi 3 and a few other platforms I have here. Didn't test USB, etc, etc, just pytest.
On Fri, Feb 05, 2021 at 01:07:02PM -0500, Tom Rini wrote:
On Fri, Feb 05, 2021 at 12:50:56PM -0500, Tom Rini wrote:
On Fri, Feb 05, 2021 at 07:06:54PM +0200, Andy Shevchenko wrote:
On Wed, Feb 03, 2021 at 11:50:53AM +0200, Andy Shevchenko wrote:
...
Okay, as promised, I prepared a branch [1] with new approach, but while I will be busy with other stuff, I would like you to test on real hardware and tell if it helps. At least it passes test cases.
If it works, I would like to get a Tested-by tag and will prepare and submit the formal series.
I reliably get:
It's one of the my patches revealed this, I reverted for now. Please, retest.
On Fri, Feb 05, 2021 at 09:31:00PM +0200, Andy Shevchenko wrote:
On Fri, Feb 05, 2021 at 01:07:02PM -0500, Tom Rini wrote:
On Fri, Feb 05, 2021 at 12:50:56PM -0500, Tom Rini wrote:
On Fri, Feb 05, 2021 at 07:06:54PM +0200, Andy Shevchenko wrote:
On Wed, Feb 03, 2021 at 11:50:53AM +0200, Andy Shevchenko wrote:
...
Okay, as promised, I prepared a branch [1] with new approach, but while I will be busy with other stuff, I would like you to test on real hardware and tell if it helps. At least it passes test cases.
If it works, I would like to get a Tested-by tag and will prepare and submit the formal series.
I reliably get:
It's one of the my patches revealed this, I reverted for now. Please, retest.
pytest now passes, thanks.
Hi Nicolas,
On Thu, 28 Jan 2021 at 06:12, Nicolas Saenz Julienne nsaenzjulienne@suse.de wrote:
Don't start/stop an stdio device that might have been already freed.
Signed-off-by: Nicolas Saenz Julienne nsaenzjulienne@suse.de Fixes: 70c2525c0d3c ("IOMUX: Stop dropped consoles")
Changes since v1:
- Add comment stating this should be properly fixed
common/console.c | 9 +++++++++ 1 file changed, 9 insertions(+)
Reviewed-by: Simon Glass sjg@chromium.org
Since this says it is a stopgap, when does the real fix come?
Regards, Simon
On Thu, Jan 28, 2021 at 02:12:38PM +0100, Nicolas Saenz Julienne wrote:
With today's master, 70c2525c0d3c ('IOMUX: Stop dropped consoles') introduces a use after free in usb_kbd_remove():
usbkbd's stdio device is de-registered with stdio_deregister_dev(), the struct stdio_dev is freed.
iomux_doenv() is called, usbkbd removed from the console list, and console_stop() is called on the struct stdio_dev pointer that no longer exists.
This series mitigates this by making sure the pointer is really a stdio device prior performing the stop operation. It's not ideal, but I couldn't figure out a nicer way to fix this.
I have just sent another approach, can you test it instead, please?
participants (6)
-
Andy Shevchenko -
Andy Shevchenko -
Matthias Brugger -
Nicolas Saenz Julienne -
Simon Glass -
Tom Rini