[PATCH 0/2] Console/stdio use after free

[PATCH 0/2] Console/stdio use after free

[Nicolas Saenz Julienne]

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

---

Nicolas Saenz Julienne (2):
  stdio: Introduce stdio_valid()
  console: Don't start/stop console if stdio device invalid

 common/console.c    |  3 +++
 common/stdio.c      | 11 +++++++++++
 include/stdio_dev.h |  1 +
 3 files changed, 15 insertions(+)

-- 
2.30.0

[PATCH 1/2] stdio: Introduce stdio_valid()

[Nicolas Saenz Julienne]

stdio_valid() will confirm that a struct stdio_dev pointer is indeed
valid.

Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
---
 common/stdio.c      | 11 +++++++++++
 include/stdio_dev.h |  1 +
 2 files changed, 12 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..f341439b03 100644
--- a/include/stdio_dev.h
+++ b/include/stdio_dev.h
@@ -97,6 +97,7 @@ int stdio_deregister_dev(struct stdio_dev *dev, int force);
 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);
+bool stdio_valid(struct stdio_dev *dev);
 
 int drv_lcd_init(void);
 int drv_video_init(void);
-- 
2.30.0

[PATCH 2/2] console: Don't start/stop console if stdio device invalid

[Nicolas Saenz Julienne]

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")
---
 common/console.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/common/console.c b/common/console.c
index f3cc45cab5..5c6b74b351 100644
--- a/common/console.c
+++ b/common/console.c
@@ -252,6 +252,9 @@ static bool console_needs_start_stop(int file, struct stdio_dev *sdev)
 {
 	int i, j;
 
+	if (!stdio_valid(sdev))
+		return false;
+
 	for (i = 0; i < ARRAY_SIZE(cd_count); i++) {
 		if (i == file)
 			continue;
-- 
2.30.0

[PATCH 0/2] Console/stdio use after free

[Simon Glass]

Hi Nicolas,

On Wed, 20 Jan 2021 at 07:04, Nicolas Saenz Julienne
<nsaenzjulienne@suse.de> 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.

Your 'from' address is coming through as just your email. Could you
please update it to include your name as well?

Regards,
Simon

[PATCH 0/2] Console/stdio use after free

[Nicolas Saenz Julienne]

On Wed, 2021-01-20 at 07:18 -0700, Simon Glass wrote:
> Hi Nicolas,
> 
> On Wed, 20 Jan 2021 at 07:04, Nicolas Saenz Julienne
> <nsaenzjulienne@suse.de> 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.
> 
> Your 'from' address is coming through as just your email. Could you
> please update it to include your name as well?

OK, do you want me to re-send the series?


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: This is a digitally signed message part
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210120/a65fbb52/attachment.sig>

[PATCH 0/2] Console/stdio use after free

[Simon Glass]

Hi Nicolas,

On Wed, 20 Jan 2021 at 07:44, Nicolas Saenz Julienne
<nsaenzjulienne@suse.de> wrote:
>
> On Wed, 2021-01-20 at 07:18 -0700, Simon Glass wrote:
> > Hi Nicolas,
> >
> > On Wed, 20 Jan 2021 at 07:04, Nicolas Saenz Julienne
> > <nsaenzjulienne@suse.de> 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.
> >
> > Your 'from' address is coming through as just your email. Could you
> > please update it to include your name as well?
>
> OK, do you want me to re-send the series?

Not just for that, no.

Regards,
Simon

[PATCH 0/2] Console/stdio use after free

[Andy Shevchenko]

On Wed, Jan 20, 2021 at 4:05 PM Nicolas Saenz Julienne
<nsaenzjulienne@suse.de> 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.

Thanks for the report and indeed this sounds like a papering over the
real issue somewhere else.
If we have a device in the console_list, IOMUX may access it. So,
whenever we drop device, we must update console_list accordingly.


-- 
With Best Regards,
Andy Shevchenko

[PATCH 0/2] Console/stdio use after free

[Pratyush Yadav]

On 20/01/21 07:18AM, Simon Glass wrote:
> Hi Nicolas,
> 
> On Wed, 20 Jan 2021 at 07:04, Nicolas Saenz Julienne
> <nsaenzjulienne@suse.de> 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.
> 
> Your 'from' address is coming through as just your email. Could you
> please update it to include your name as well?

From shows the full name on my email client. For everybody apart from 
Nicholas it shows just the email, but for Nicholas I can see full name 
in both From and Cc.

Maybe something wrong with your email client settings.
 
> Regards,
> Simon

-- 
Regards,
Pratyush Yadav
Texas Instruments India

[PATCH 0/2] Console/stdio use after free

[Simon Glass]

Hi Pratyush,

On Wed, 20 Jan 2021 at 11:59, Pratyush Yadav <p.yadav@ti.com> wrote:
>
> On 20/01/21 07:18AM, Simon Glass wrote:
> > Hi Nicolas,
> >
> > On Wed, 20 Jan 2021 at 07:04, Nicolas Saenz Julienne
> > <nsaenzjulienne@suse.de> 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.
> >
> > Your 'from' address is coming through as just your email. Could you
> > please update it to include your name as well?
>
> From shows the full name on my email client. For everybody apart from
> Nicholas it shows just the email, but for Nicholas I can see full name
> in both From and Cc.
>
> Maybe something wrong with your email client settings.

Yes, perhaps you are right. Looking at the list I also have this
problem for  SkyLake Huang <SkyLake.Huang@mediatek.com>

Regards,
Simon

[PATCH 1/2] stdio: Introduce stdio_valid()

[Simon Glass]

Hi Nicolas,

On Wed, 20 Jan 2021 at 07:05, 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>
> ---
>  common/stdio.c      | 11 +++++++++++
>  include/stdio_dev.h |  1 +
>  2 files changed, 12 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..f341439b03 100644
> --- a/include/stdio_dev.h
> +++ b/include/stdio_dev.h
> @@ -97,6 +97,7 @@ int stdio_deregister_dev(struct stdio_dev *dev, int force);
>  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);
> +bool stdio_valid(struct stdio_dev *dev);

Please add a full function comment and explain what valid means.

>
>  int drv_lcd_init(void);
>  int drv_video_init(void);
> --
> 2.30.0
>

Regards,
Simon

[PATCH 2/2] console: Don't start/stop console if stdio device invalid

[Simon Glass]

On Wed, 20 Jan 2021 at 07:05, 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")
> ---
>  common/console.c | 3 +++
>  1 file changed, 3 insertions(+)
>

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH 0/2] Console/stdio use after free

[Nicolas Saenz Julienne]

Hi Andy, Simon

On Wed, 2021-01-20 at 17:57 +0200, Andy Shevchenko wrote:
> On Wed, Jan 20, 2021 at 4:05 PM Nicolas Saenz Julienne
> <nsaenzjulienne@suse.de> 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.
> 
> Thanks for the report and indeed this sounds like a papering over the
> real issue somewhere else.
> If we have a device in the console_list, IOMUX may access it. So,
> whenever we drop device, we must update console_list accordingly.

Sorry, but I don't have time to address this ATM. If someone else can it'd be
nice.

Regards,
Nicolas

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: This is a digitally signed message part
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210125/4c4d4567/attachment.sig>

[PATCH 1/2] stdio: Introduce stdio_valid()

[Nicolas Saenz Julienne]

On Sat, 2021-01-23 at 19:03 -0700, Simon Glass wrote:
> Hi Nicolas,
> 
> On Wed, 20 Jan 2021 at 07:05, 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>
> > ---
> > ?common/stdio.c      | 11 +++++++++++
> > ?include/stdio_dev.h |  1 +
> > ?2 files changed, 12 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..f341439b03 100644
> > --- a/include/stdio_dev.h
> > +++ b/include/stdio_dev.h
> > @@ -97,6 +97,7 @@ int stdio_deregister_dev(struct stdio_dev *dev, int force);
> > ?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);
> > +bool stdio_valid(struct stdio_dev *dev);
> 
> Please add a full function comment and explain what valid means.

As discussed with Andy, this is a workaround that doesn't address the
underlying issue. If it's good enough for the time being I'll be happy to send
a v2.

I'll leave a comment stating that it's something to fix.

Regards,
Nicolas

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: This is a digitally signed message part
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210125/dd853625/attachment.sig>

[PATCH 1/2] stdio: Introduce stdio_valid()

[Tom Rini]

On Mon, Jan 25, 2021 at 05:34:05PM +0100, Nicolas Saenz Julienne wrote:
> On Sat, 2021-01-23 at 19:03 -0700, Simon Glass wrote:
> > Hi Nicolas,
> > 
> > On Wed, 20 Jan 2021 at 07:05, 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>
> > > ---
> > > ?common/stdio.c      | 11 +++++++++++
> > > ?include/stdio_dev.h |  1 +
> > > ?2 files changed, 12 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..f341439b03 100644
> > > --- a/include/stdio_dev.h
> > > +++ b/include/stdio_dev.h
> > > @@ -97,6 +97,7 @@ int stdio_deregister_dev(struct stdio_dev *dev, int force);
> > > ?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);
> > > +bool stdio_valid(struct stdio_dev *dev);
> > 
> > Please add a full function comment and explain what valid means.
> 
> As discussed with Andy, this is a workaround that doesn't address the
> underlying issue. If it's good enough for the time being I'll be happy to send
> a v2.
> 
> I'll leave a comment stating that it's something to fix.

Please do, 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/20210127/c3fe2f1e/attachment.sig>