| Message ID | 20210128131240.13190-3-nsaenzjulienne@suse.de |
|---|---|
| State | Deferred |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | Console/stdio use after free | expand |
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!
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 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. > > [1]: https://github.com/andy-shev/u-boot/tree/iomux 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. > > > > [1]: https://github.com/andy-shev/u-boot/tree/iomux > > 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. > > > > > > [1]: https://github.com/andy-shev/u-boot/tree/iomux > > > > 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. > > > > > > > > [1]: https://github.com/andy-shev/u-boot/tree/iomux > > > > > > I reliably get: > > It's one of the my patches revealed this, I reverted for now. Please, retest. pytest now passes, thanks.
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;
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(+)