It's possible that NULLDEV can be disabled while it makes leftovers, move entire device under #if.

Signed-off-by: Andy Shevchenko andriy.shevchenko@linux.intel.com --- common/stdio.c | 33 +++++++++++++++++++++------------ 1 file changed, 21 insertions(+), 12 deletions(-)

diff --git a/common/stdio.c b/common/stdio.c index e3e24f0dbf89..2935d0d9ba8a 100644 --- a/common/stdio.c +++ b/common/stdio.c @@ -28,6 +28,7 @@ static struct stdio_dev devs; struct stdio_dev *stdio_devices[] = { NULL, NULL, NULL }; char *stdio_names[MAX_FILES] = { "stdin", "stdout", "stderr" };

+#if CONFIG_IS_ENABLED(SYS_DEVICE_NULLDEV) static void nulldev_putc(struct stdio_dev *dev, const char c) { /* nulldev is empty! */ @@ -44,6 +45,25 @@ static int nulldev_input(struct stdio_dev *dev) return 0; }

+static void nulldev_register(void) +{ + struct stdio_dev dev; + + memset(&dev, '\0', sizeof(dev)); + + strcpy(dev.name, "nulldev"); + dev.flags = DEV_FLAGS_OUTPUT | DEV_FLAGS_INPUT; + dev.putc = nulldev_putc; + dev.puts = nulldev_puts; + dev.getc = nulldev_input; + dev.tstc = nulldev_input; + + stdio_register(&dev); +} +#else +static inline void nulldev_register(void) {} +#endif /* SYS_DEVICE_NULLDEV */ + static void stdio_serial_putc(struct stdio_dev *dev, const char c) { serial_putc(c); @@ -83,18 +103,7 @@ static void drv_system_init (void) dev.tstc = stdio_serial_tstc; stdio_register (&dev);

- if (CONFIG_IS_ENABLED(SYS_DEVICE_NULLDEV)) { - memset(&dev, '\0', sizeof(dev)); - - strcpy(dev.name, "nulldev"); - dev.flags = DEV_FLAGS_OUTPUT | DEV_FLAGS_INPUT; - dev.putc = nulldev_putc; - dev.puts = nulldev_puts; - dev.getc = nulldev_input; - dev.tstc = nulldev_input; - - stdio_register(&dev); - } + nulldev_register(); }

/**************************************************************************

Let's deduplicate existing copies by splitting off to a new helper.

Signed-off-by: Andy Shevchenko andriy.shevchenko@linux.intel.com --- common/stdio.c | 13 +++++++++++++ include/stdio_dev.h | 2 ++ 2 files changed, 15 insertions(+)

diff --git a/common/stdio.c b/common/stdio.c index 2935d0d9ba8a..aa003b308a21 100644 --- a/common/stdio.c +++ b/common/stdio.c @@ -28,6 +28,19 @@ static struct stdio_dev devs; struct stdio_dev *stdio_devices[] = { NULL, NULL, NULL }; char *stdio_names[MAX_FILES] = { "stdin", "stdout", "stderr" };

+int stdio_file_to_flags(const int file) +{ + switch (file) { + case stdin: + return DEV_FLAGS_INPUT; + case stdout: + case stderr: + return DEV_FLAGS_OUTPUT; + default: + return -EINVAL; + } +} + #if CONFIG_IS_ENABLED(SYS_DEVICE_NULLDEV) static void nulldev_putc(struct stdio_dev *dev, const char c) { diff --git a/include/stdio_dev.h b/include/stdio_dev.h index 109a68d06409..8fb9a12dd876 100644 --- a/include/stdio_dev.h +++ b/include/stdio_dev.h @@ -18,6 +18,8 @@ #define DEV_FLAGS_OUTPUT 0x00000002 /* Device can be used as output console */ #define DEV_FLAGS_DM 0x00000004 /* Device priv is a struct udevice * */

+int stdio_file_to_flags(const int file); + /* Device information */ struct stdio_dev { int flags; /* Device flags: input/output/system */

Deduplicate code by replacing with stdio_file_to_flags() helper.

Signed-off-by: Andy Shevchenko andriy.shevchenko@linux.intel.com --- common/console.c | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/common/console.c b/common/console.c index f3cc45cab548..b1c3ed17cc03 100644 --- a/common/console.c +++ b/common/console.c @@ -854,17 +854,9 @@ int console_assign(int file, const char *devname) struct stdio_dev *dev;

/* Check for valid file */ - switch (file) { - case stdin: - flag = DEV_FLAGS_INPUT; - break; - case stdout: - case stderr: - flag = DEV_FLAGS_OUTPUT; - break; - default: - return -1; - } + flag = stdio_file_to_flags(file); + if (flag < 0) + return flag;

/* Check for valid device name */

console_devices_set() missed the console device counter to be set correctly.

Fixes: 45375adc9799 ("console: add function console_devices_set") Cc: Patrick Delaunay patrick.delaunay@foss.st.com Signed-off-by: Andy Shevchenko andriy.shevchenko@linux.intel.com --- common/console.c | 1 + 1 file changed, 1 insertion(+)

diff --git a/common/console.c b/common/console.c index b1c3ed17cc03..4595376dcc0b 100644 --- a/common/console.c +++ b/common/console.c @@ -235,6 +235,7 @@ int cd_count[MAX_FILES]; static void __maybe_unused console_devices_set(int file, struct stdio_dev *dev) { console_devices[file][0] = dev; + cd_count[file] = 1; }

/**

Logical continuation of the change that brought console_devices_set() is to unify console_setfile() with it and replace in the callers.

Signed-off-by: Andy Shevchenko andriy.shevchenko@linux.intel.com --- common/console.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/common/console.c b/common/console.c index 4595376dcc0b..672b4a1cc678 100644 --- a/common/console.c +++ b/common/console.c @@ -232,7 +232,7 @@ static struct stdio_dev *tstcdev; struct stdio_dev **console_devices[MAX_FILES]; int cd_count[MAX_FILES];

-static void __maybe_unused console_devices_set(int file, struct stdio_dev *dev) +static void console_devices_set(int file, struct stdio_dev *dev) { console_devices[file][0] = dev; cd_count[file] = 1; @@ -369,7 +369,7 @@ static inline void console_doenv(int file, struct stdio_dev *dev) #endif #else

-static void __maybe_unused console_devices_set(int file, struct stdio_dev *dev) +static void console_devices_set(int file, struct stdio_dev *dev) { }

@@ -417,6 +417,12 @@ static inline void console_doenv(int file, struct stdio_dev *dev) #endif #endif /* CONIFIG_IS_ENABLED(CONSOLE_MUX) */

+static void __maybe_unused console_setfile_and_devices(int file, struct stdio_dev *dev) +{ + console_setfile(file, dev); + console_devices_set(file, dev); +} + int console_start(int file, struct stdio_dev *sdev) { int error; @@ -1071,17 +1077,13 @@ int console_init_r(void)

/* Initializes output console first */ if (outputdev != NULL) { - console_setfile(stdout, outputdev); - console_setfile(stderr, outputdev); - console_devices_set(stdout, outputdev); - console_devices_set(stderr, outputdev); + console_setfile_and_devices(stdout, outputdev); + console_setfile_and_devices(stderr, outputdev); }

/* Initializes input console */ - if (inputdev != NULL) { - console_setfile(stdin, inputdev); - console_devices_set(stdin, inputdev); - } + if (inputdev != NULL) + console_setfile_and_devices(stdin, inputdev);

if (!IS_ENABLED(CONFIG_SYS_CONSOLE_INFO_QUIET)) stdio_print_current_devices();

Deduplicate code by replacing with stdio_file_to_flags() helper.

Signed-off-by: Andy Shevchenko andriy.shevchenko@linux.intel.com --- common/iomux.c | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/common/iomux.c b/common/iomux.c index 15bf53388559..5d027561bb6f 100644 --- a/common/iomux.c +++ b/common/iomux.c @@ -75,15 +75,8 @@ int iomux_doenv(const int console, const char *arg) return 1; }

- switch (console) { - case stdin: - io_flag = DEV_FLAGS_INPUT; - break; - case stdout: - case stderr: - io_flag = DEV_FLAGS_OUTPUT; - break; - default: + io_flag = stdio_file_to_flags(console); + if (io_flag < 0) { free(start); free(console_args); free(cons_set);

Deduplicate the code used in a few places by splitting out a common helper.

Signed-off-by: Andy Shevchenko andriy.shevchenko@linux.intel.com --- common/console.c | 7 +++---- common/iomux.c | 27 ++++++++++++++------------- include/iomux.h | 1 + 3 files changed, 18 insertions(+), 17 deletions(-)

diff --git a/common/console.c b/common/console.c index 672b4a1cc678..27e881a46bda 100644 --- a/common/console.c +++ b/common/console.c @@ -251,15 +251,14 @@ static void console_devices_set(int file, struct stdio_dev *dev) */ static bool console_needs_start_stop(int file, struct stdio_dev *sdev) { - int i, j; + int i;

for (i = 0; i < ARRAY_SIZE(cd_count); i++) { if (i == file) continue;

- for (j = 0; j < cd_count[i]; j++) - if (console_devices[i][j] == sdev) - return false; + if (iomux_match_device(console_devices[i], cd_count[i], sdev) >= 0) + return false; } return true; } diff --git a/common/iomux.c b/common/iomux.c index 5d027561bb6f..a8be1ac7d8ab 100644 --- a/common/iomux.c +++ b/common/iomux.c @@ -22,11 +22,21 @@ void iomux_printdevs(const int console) printf("\n"); }

+int iomux_match_device(struct stdio_dev **set, const int n, struct stdio_dev *sdev) +{ + int i; + + for (i = 0; i < n; i++) + if (sdev == set[i]) + return i; + return -ENOENT; +} + /* This tries to preserve the old list if an error occurs. */ int iomux_doenv(const int console, const char *arg) { char *console_args, *temp, **start; - int i, j, k, io_flag, cs_idx, repeat; + int i, j, io_flag, cs_idx, repeat; struct stdio_dev **cons_set, **old_set; struct stdio_dev *dev;

@@ -96,14 +106,8 @@ int iomux_doenv(const int console, const char *arg) /* * Prevent multiple entries for a device. */ - repeat = 0; - for (k = 0; k < cs_idx; k++) { - if (dev == cons_set[k]) { - repeat++; - break; - } - } - if (repeat) + repeat = iomux_match_device(cons_set, cs_idx, dev); + if (repeat >= 0) continue; /* * Try assigning the specified device. @@ -129,10 +133,7 @@ int iomux_doenv(const int console, const char *arg)

/* Stop dropped consoles */ for (i = 0; i < repeat; i++) { - for (j = 0; j < cs_idx; j++) { - if (old_set[i] == cons_set[j]) - break; - } + j = iomux_match_device(cons_set, cs_idx, old_set[i]); if (j == cs_idx) console_stop(console, old_set[i]); } diff --git a/include/iomux.h b/include/iomux.h index da7ff697d218..9c2d5796066c 100644 --- a/include/iomux.h +++ b/include/iomux.h @@ -24,6 +24,7 @@ extern struct stdio_dev **console_devices[MAX_FILES]; */ extern int cd_count[MAX_FILES];

+int iomux_match_device(struct stdio_dev **, const int, struct stdio_dev *); int iomux_doenv(const int, const char *); void iomux_printdevs(const int);

It is not only less lines of code, but also better readability when new macro is being in use. Introduce for_each_console_dev() helper macro and convert current users to it.

Signed-off-by: Andy Shevchenko andriy.shevchenko@linux.intel.com --- common/console.c | 15 +++++---------- common/iomux.c | 4 +--- include/iomux.h | 5 +++++ 3 files changed, 11 insertions(+), 13 deletions(-)

diff --git a/common/console.c b/common/console.c index 27e881a46bda..197b4de23bb4 100644 --- a/common/console.c +++ b/common/console.c @@ -292,8 +292,7 @@ static int console_tstc(int file) int prev;

prev = disable_ctrlc(1); - for (i = 0; i < cd_count[file]; i++) { - dev = console_devices[file][i]; + for_each_console_dev(i, file, dev) { if (dev->tstc != NULL) { ret = dev->tstc(dev); if (ret > 0) { @@ -313,8 +312,7 @@ static void console_putc(int file, const char c) int i; struct stdio_dev *dev;

- for (i = 0; i < cd_count[file]; i++) { - dev = console_devices[file][i]; + for_each_console_dev(i, file, dev) { if (dev->putc != NULL) dev->putc(dev, c); } @@ -333,11 +331,9 @@ static void console_puts_select(int file, bool serial_only, const char *s) int i; struct stdio_dev *dev;

- for (i = 0; i < cd_count[file]; i++) { - bool is_serial; + for_each_console_dev(i, file, dev) { + bool is_serial = console_dev_is_serial(dev);

- dev = console_devices[file][i]; - is_serial = console_dev_is_serial(dev); if (dev->puts && serial_only == is_serial) dev->puts(dev, s); } @@ -353,8 +349,7 @@ static void console_puts(int file, const char *s) int i; struct stdio_dev *dev;

- for (i = 0; i < cd_count[file]; i++) { - dev = console_devices[file][i]; + for_each_console_dev(i, file, dev) { if (dev->puts != NULL) dev->puts(dev, s); } diff --git a/common/iomux.c b/common/iomux.c index a8be1ac7d8ab..5290b13b668b 100644 --- a/common/iomux.c +++ b/common/iomux.c @@ -15,10 +15,8 @@ void iomux_printdevs(const int console) int i; struct stdio_dev *dev;

- for (i = 0; i < cd_count[console]; i++) { - dev = console_devices[console][i]; + for_each_console_dev(i, console, dev) printf("%s ", dev->name); - } printf("\n"); }

diff --git a/include/iomux.h b/include/iomux.h index 9c2d5796066c..bd4a143b1e60 100644 --- a/include/iomux.h +++ b/include/iomux.h @@ -24,6 +24,11 @@ extern struct stdio_dev **console_devices[MAX_FILES]; */ extern int cd_count[MAX_FILES];

+#define for_each_console_dev(i, file, dev) \ + for (i = 0, dev = console_devices[file][i]; \ + i < cd_count[file]; \ + i++, dev = console_devices[file][i]) + int iomux_match_device(struct stdio_dev **, const int, struct stdio_dev *); int iomux_doenv(const int, const char *); void iomux_printdevs(const int);

Some console devices may appear or disappear at run time. In order to support such a hotplug mechanism introduce a new iomux_replace_device() helper to update the list of devices without altering environment.

Signed-off-by: Andy Shevchenko andriy.shevchenko@linux.intel.com --- common/iomux.c | 33 +++++++++++++++++++++++++++++++++ include/iomux.h | 1 + 2 files changed, 34 insertions(+)

diff --git a/common/iomux.c b/common/iomux.c index 5290b13b668b..b9088aa3b58b 100644 --- a/common/iomux.c +++ b/common/iomux.c @@ -139,4 +139,37 @@ int iomux_doenv(const int console, const char *arg) free(old_set); return 0; } + +int iomux_replace_device(const int console, const char *old, const char *new) +{ + struct stdio_dev *dev; + char *arg = NULL; /* Initial empty list */ + int size = 1; /* For NUL terminator */ + int i, ret; + + for_each_console_dev(i, console, dev) { + const char *name = strcmp(dev->name, old) ? dev->name : new; + char *tmp; + + /* Append name with a ',' (comma) separator */ + tmp = realloc(arg, size + strlen(name) + 1); + if (!tmp) { + free(arg); + return -ENOMEM; + } + + strcat(tmp, ","); + strcat(tmp, name); + + arg = tmp; + size = strlen(tmp) + 1; + } + + ret = iomux_doenv(console, arg); + if (ret) + ret = -EINVAL; + + free(arg); + return ret; +} #endif /* CONSOLE_MUX */ diff --git a/include/iomux.h b/include/iomux.h index bd4a143b1e60..37f5f6dee69f 100644 --- a/include/iomux.h +++ b/include/iomux.h @@ -31,6 +31,7 @@ extern int cd_count[MAX_FILES];

int iomux_match_device(struct stdio_dev **, const int, struct stdio_dev *); int iomux_doenv(const int, const char *); +int iomux_replace_device(const int, const char *, const char *); void iomux_printdevs(const int);

#endif /* _IO_MUX_H */

In case of IOMUX enabled it assumes that console devices in the list are available to get them stopped properly via ->stop() callback. However, the USB keyboard driver violates this assumption and tries to play tricks so the device get destroyed while being listed as an active console.

Swap the order of device deregistration and IOMUX update along with converting to use iomux_replace_device() jelper to avoid the use-after-free.

Fixes: 3cbcb2892809 ("usb: Fix usb_kbd_deregister when console-muxing is used") Fixes: 8a8348703081 ("dm: usb: Add a remove() method for USB keyboards") Reported-by: Nicolas Saenz Julienne nsaenzjulienne@suse.de Signed-off-by: Andy Shevchenko andriy.shevchenko@linux.intel.com --- common/usb_kbd.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/common/usb_kbd.c b/common/usb_kbd.c index b316807844b1..60c6027e048d 100644 --- a/common/usb_kbd.c +++ b/common/usb_kbd.c @@ -617,12 +617,12 @@ int usb_kbd_deregister(int force) if (dev) { usb_kbd_dev = (struct usb_device *)dev->priv; data = usb_kbd_dev->privptr; - if (stdio_deregister_dev(dev, force) != 0) - return 1; #if CONFIG_IS_ENABLED(CONSOLE_MUX) - if (iomux_doenv(stdin, env_get("stdin")) != 0) + if (iomux_replace_device(stdin, DEVNAME, force ? "nulldev" : "")) return 1; #endif + if (stdio_deregister_dev(dev, force) != 0) + return 1; #ifdef CONFIG_SYS_USB_EVENT_POLL_VIA_INT_QUEUE destroy_int_queue(usb_kbd_dev, data->intq); #endif @@ -660,16 +660,16 @@ static int usb_kbd_remove(struct udevice *dev) goto err; } data = udev->privptr; - if (stdio_deregister_dev(sdev, true)) { - ret = -EPERM; - goto err; - } #if CONFIG_IS_ENABLED(CONSOLE_MUX) - if (iomux_doenv(stdin, env_get("stdin"))) { + if (iomux_replace_device(stdin, DEVNAME, "nulldev")) { ret = -ENOLINK; goto err; } #endif + if (stdio_deregister_dev(sdev, true)) { + ret = -EPERM; + goto err; + } #ifdef CONFIG_SYS_USB_EVENT_POLL_VIA_INT_QUEUE destroy_int_queue(udev, data->intq); #endif