All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 0/2] Console/stdio use after free
@ 2021-01-28 13:12 Nicolas Saenz Julienne
  2021-01-28 13:12 ` [PATCH v2 1/2] stdio: Introduce stdio_valid() Nicolas Saenz Julienne
                   ` (2 more replies)
  0 siblings, 3 replies; 15+ messages in thread
From: Nicolas Saenz Julienne @ 2021-01-28 13:12 UTC (permalink / raw)
  To: u-boot

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(+)

-- 
2.30.0

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v2 1/2] stdio: Introduce stdio_valid()
  2021-01-28 13:12 [PATCH v2 0/2] Console/stdio use after free Nicolas Saenz Julienne
@ 2021-01-28 13:12 ` Nicolas Saenz Julienne
  2021-02-01 20:43   ` Simon Glass
  2021-01-28 13:12 ` [PATCH v2 2/2] console: Don't start/stop console if stdio device invalid Nicolas Saenz Julienne
  2021-01-28 16:55 ` [PATCH v2 0/2] Console/stdio use after free Andy Shevchenko
  2 siblings, 1 reply; 15+ messages in thread
From: Nicolas Saenz Julienne @ 2021-01-28 13:12 UTC (permalink / raw)
  To: u-boot

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);
-- 
2.30.0

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v2 2/2] console: Don't start/stop console if stdio device invalid
  2021-01-28 13:12 [PATCH v2 0/2] Console/stdio use after free Nicolas Saenz Julienne
  2021-01-28 13:12 ` [PATCH v2 1/2] stdio: Introduce stdio_valid() Nicolas Saenz Julienne
@ 2021-01-28 13:12 ` Nicolas Saenz Julienne
  2021-01-28 15:52   ` Andy Shevchenko
  2021-02-01 20:18   ` Simon Glass
  2021-01-28 16:55 ` [PATCH v2 0/2] Console/stdio use after free Andy Shevchenko
  2 siblings, 2 replies; 15+ messages in thread
From: Nicolas Saenz Julienne @ 2021-01-28 13:12 UTC (permalink / raw)
  To: u-boot

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;
-- 
2.30.0

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v2 2/2] console: Don't start/stop console if stdio device invalid
  2021-01-28 13:12 ` [PATCH v2 2/2] console: Don't start/stop console if stdio device invalid Nicolas Saenz Julienne
@ 2021-01-28 15:52   ` Andy Shevchenko
  2021-01-29  8:50     ` Matthias Brugger
  2021-02-01 20:18   ` Simon Glass
  1 sibling, 1 reply; 15+ messages in thread
From: Andy Shevchenko @ 2021-01-28 15:52 UTC (permalink / raw)
  To: u-boot

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;

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v2 0/2] Console/stdio use after free
  2021-01-28 13:12 [PATCH v2 0/2] Console/stdio use after free Nicolas Saenz Julienne
  2021-01-28 13:12 ` [PATCH v2 1/2] stdio: Introduce stdio_valid() Nicolas Saenz Julienne
  2021-01-28 13:12 ` [PATCH v2 2/2] console: Don't start/stop console if stdio device invalid Nicolas Saenz Julienne
@ 2021-01-28 16:55 ` Andy Shevchenko
  2 siblings, 0 replies; 15+ messages in thread
From: Andy Shevchenko @ 2021-01-28 16:55 UTC (permalink / raw)
  To: u-boot

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?

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v2 2/2] console: Don't start/stop console if stdio device invalid
  2021-01-28 15:52   ` Andy Shevchenko
@ 2021-01-29  8:50     ` Matthias Brugger
  2021-02-01 19:29       ` Tom Rini
  0 siblings, 1 reply; 15+ messages in thread
From: Matthias Brugger @ 2021-01-29  8:50 UTC (permalink / raw)
  To: u-boot



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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v2 2/2] console: Don't start/stop console if stdio device invalid
  2021-01-29  8:50     ` Matthias Brugger
@ 2021-02-01 19:29       ` Tom Rini
  2021-02-03  9:50         ` Andy Shevchenko
  0 siblings, 1 reply; 15+ messages in thread
From: Tom Rini @ 2021-02-01 19:29 UTC (permalink / raw)
  To: u-boot

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!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210201/686390a0/attachment.sig>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v2 2/2] console: Don't start/stop console if stdio device invalid
  2021-01-28 13:12 ` [PATCH v2 2/2] console: Don't start/stop console if stdio device invalid Nicolas Saenz Julienne
  2021-01-28 15:52   ` Andy Shevchenko
@ 2021-02-01 20:18   ` Simon Glass
  1 sibling, 0 replies; 15+ messages in thread
From: Simon Glass @ 2021-02-01 20:18 UTC (permalink / raw)
  To: u-boot

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v2 1/2] stdio: Introduce stdio_valid()
  2021-01-28 13:12 ` [PATCH v2 1/2] stdio: Introduce stdio_valid() Nicolas Saenz Julienne
@ 2021-02-01 20:43   ` Simon Glass
  0 siblings, 0 replies; 15+ messages in thread
From: Simon Glass @ 2021-02-01 20:43 UTC (permalink / raw)
  To: u-boot

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>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v2 2/2] console: Don't start/stop console if stdio device invalid
  2021-02-01 19:29       ` Tom Rini
@ 2021-02-03  9:50         ` Andy Shevchenko
  2021-02-05 17:06           ` Andy Shevchenko
  0 siblings, 1 reply; 15+ messages in thread
From: Andy Shevchenko @ 2021-02-03  9:50 UTC (permalink / raw)
  To: u-boot

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.


-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v2 2/2] console: Don't start/stop console if stdio device invalid
  2021-02-03  9:50         ` Andy Shevchenko
@ 2021-02-05 17:06           ` Andy Shevchenko
  2021-02-05 17:50             ` Tom Rini
  0 siblings, 1 reply; 15+ messages in thread
From: Andy Shevchenko @ 2021-02-05 17:06 UTC (permalink / raw)
  To: u-boot

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

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v2 2/2] console: Don't start/stop console if stdio device invalid
  2021-02-05 17:06           ` Andy Shevchenko
@ 2021-02-05 17:50             ` Tom Rini
  2021-02-05 18:07               ` Tom Rini
  0 siblings, 1 reply; 15+ messages in thread
From: Tom Rini @ 2021-02-05 17:50 UTC (permalink / raw)
  To: u-boot

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@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@0x7fe2516c5da0>('Failures: 0')
E        +    where <built-in method endswith of str object@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.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210205/5e1cc7a1/attachment.sig>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v2 2/2] console: Don't start/stop console if stdio device invalid
  2021-02-05 17:50             ` Tom Rini
@ 2021-02-05 18:07               ` Tom Rini
  2021-02-05 19:31                 ` Andy Shevchenko
  0 siblings, 1 reply; 15+ messages in thread
From: Tom Rini @ 2021-02-05 18:07 UTC (permalink / raw)
  To: u-boot

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@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@0x7fe2516c5da0>('Failures: 0')
> E        +    where <built-in method endswith of str object@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.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210205/9c61aa40/attachment.sig>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v2 2/2] console: Don't start/stop console if stdio device invalid
  2021-02-05 18:07               ` Tom Rini
@ 2021-02-05 19:31                 ` Andy Shevchenko
  2021-02-05 20:47                   ` Tom Rini
  0 siblings, 1 reply; 15+ messages in thread
From: Andy Shevchenko @ 2021-02-05 19:31 UTC (permalink / raw)
  To: u-boot

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.


-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v2 2/2] console: Don't start/stop console if stdio device invalid
  2021-02-05 19:31                 ` Andy Shevchenko
@ 2021-02-05 20:47                   ` Tom Rini
  0 siblings, 0 replies; 15+ messages in thread
From: Tom Rini @ 2021-02-05 20:47 UTC (permalink / raw)
  To: u-boot

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.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210205/b5aff639/attachment.sig>

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2021-02-05 20:47 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-28 13:12 [PATCH v2 0/2] Console/stdio use after free Nicolas Saenz Julienne
2021-01-28 13:12 ` [PATCH v2 1/2] stdio: Introduce stdio_valid() Nicolas Saenz Julienne
2021-02-01 20:43   ` Simon Glass
2021-01-28 13:12 ` [PATCH v2 2/2] console: Don't start/stop console if stdio device invalid Nicolas Saenz Julienne
2021-01-28 15:52   ` Andy Shevchenko
2021-01-29  8:50     ` Matthias Brugger
2021-02-01 19:29       ` Tom Rini
2021-02-03  9:50         ` Andy Shevchenko
2021-02-05 17:06           ` Andy Shevchenko
2021-02-05 17:50             ` Tom Rini
2021-02-05 18:07               ` Tom Rini
2021-02-05 19:31                 ` Andy Shevchenko
2021-02-05 20:47                   ` Tom Rini
2021-02-01 20:18   ` Simon Glass
2021-01-28 16:55 ` [PATCH v2 0/2] Console/stdio use after free Andy Shevchenko

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.