[PATCH 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
---
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(+)
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);
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
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
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.
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;
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
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
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?
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
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
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
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.
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
participants (5)
-
Andy Shevchenko -
Nicolas Saenz Julienne -
Pratyush Yadav -
Simon Glass -
Tom Rini