[U-Boot] [PATCH] usb: kbd: Fix dangling pointers on probe fail
If probe fails, we should unregister the stdio device, else we leave dangling pointers to the parent 'struct usb_device'.
Signed-off-by: Rob Clark robdclark@gmail.com --- I finally got around to debugging why things explode so badly without fixing usb_kbd vs. iomux[1] (which this patch applies on top of).
[1] https://patchwork.ozlabs.org/patch/818881/
common/usb_kbd.c | 30 ++++++++++++++++++++++++------ 1 file changed, 24 insertions(+), 6 deletions(-)
diff --git a/common/usb_kbd.c b/common/usb_kbd.c index 4c3ad95fca..82ad93f6ca 100644 --- a/common/usb_kbd.c +++ b/common/usb_kbd.c @@ -535,22 +535,40 @@ static int probe_usb_keyboard(struct usb_device *dev) error = iomux_doenv(stdin, stdinname); free(newstdin); if (error) - return error; + goto unregister_stdio; } else { /* Check if this is the standard input device. */ - if (strcmp(stdinname, DEVNAME)) - return 1; + if (strcmp(stdinname, DEVNAME)) { + error = -1; + goto unregister_stdio; + }
/* Reassign the console */ - if (overwrite_console()) - return 1; + if (overwrite_console()) { + error = -1; + goto unregister_stdio; + }
error = console_assign(stdin, DEVNAME); if (error) - return error; + goto unregister_stdio; }
return 0; + +unregister_stdio: + /* + * If probe fails, the device will be removed.. leaving dangling + * pointers if the stdio device is not unregistered. If u-boot + * is built without stdio_deregister(), just pretend to succeed + * in order to avoid dangling pointers. + */ +#if CONFIG_IS_ENABLED(SYS_STDIO_DEREGISTER) + stdio_deregister(DEVNAME, 1); + return error; +#else + return 0; +#endif }
#ifndef CONFIG_DM_USB
On Tue, Oct 3, 2017 at 6:31 PM, Rob Clark robdclark@gmail.com wrote:
If probe fails, we should unregister the stdio device, else we leave dangling pointers to the parent 'struct usb_device'.
Signed-off-by: Rob Clark robdclark@gmail.com
Tested-by: Peter Robinson pbrobinson@gmail.com
Tested on RPi3 and a couple of other devices.
I finally got around to debugging why things explode so badly without fixing usb_kbd vs. iomux[1] (which this patch applies on top of).
[1] https://patchwork.ozlabs.org/patch/818881/
common/usb_kbd.c | 30 ++++++++++++++++++++++++------ 1 file changed, 24 insertions(+), 6 deletions(-)
diff --git a/common/usb_kbd.c b/common/usb_kbd.c index 4c3ad95fca..82ad93f6ca 100644 --- a/common/usb_kbd.c +++ b/common/usb_kbd.c @@ -535,22 +535,40 @@ static int probe_usb_keyboard(struct usb_device *dev) error = iomux_doenv(stdin, stdinname); free(newstdin); if (error)
return error;
goto unregister_stdio; } else { /* Check if this is the standard input device. */
if (strcmp(stdinname, DEVNAME))return 1;
if (strcmp(stdinname, DEVNAME)) {error = -1;goto unregister_stdio;} /* Reassign the console */
if (overwrite_console())return 1;
if (overwrite_console()) {error = -1;goto unregister_stdio;} error = console_assign(stdin, DEVNAME); if (error)
return error;
goto unregister_stdio; } return 0;+unregister_stdio:
/** If probe fails, the device will be removed.. leaving dangling* pointers if the stdio device is not unregistered. If u-boot* is built without stdio_deregister(), just pretend to succeed* in order to avoid dangling pointers.*/+#if CONFIG_IS_ENABLED(SYS_STDIO_DEREGISTER)
stdio_deregister(DEVNAME, 1);return error;+#else
return 0;+#endif }
#ifndef CONFIG_DM_USB
2.13.5
U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Hi Rob,
On Wed, Oct 4, 2017 at 1:31 AM, Rob Clark robdclark@gmail.com wrote:
If probe fails, we should unregister the stdio device, else we leave dangling pointers to the parent 'struct usb_device'.
Signed-off-by: Rob Clark robdclark@gmail.com
I finally got around to debugging why things explode so badly without fixing usb_kbd vs. iomux[1] (which this patch applies on top of).
[1] https://patchwork.ozlabs.org/patch/818881/
common/usb_kbd.c | 30 ++++++++++++++++++++++++------ 1 file changed, 24 insertions(+), 6 deletions(-)
diff --git a/common/usb_kbd.c b/common/usb_kbd.c index 4c3ad95fca..82ad93f6ca 100644 --- a/common/usb_kbd.c +++ b/common/usb_kbd.c @@ -535,22 +535,40 @@ static int probe_usb_keyboard(struct usb_device *dev) error = iomux_doenv(stdin, stdinname); free(newstdin); if (error)
return error;
goto unregister_stdio; } else { /* Check if this is the standard input device. */
if (strcmp(stdinname, DEVNAME))return 1;
if (strcmp(stdinname, DEVNAME)) {error = -1;
Could we use some meaningful errno?
goto unregister_stdio;} /* Reassign the console */
if (overwrite_console())return 1;
if (overwrite_console()) {error = -1;goto unregister_stdio;} error = console_assign(stdin, DEVNAME); if (error)
return error;
goto unregister_stdio; } return 0;+unregister_stdio:
/** If probe fails, the device will be removed.. leaving dangling* pointers if the stdio device is not unregistered. If u-boot
nits: U-Boot
* is built without stdio_deregister(), just pretend to succeed* in order to avoid dangling pointers.*/+#if CONFIG_IS_ENABLED(SYS_STDIO_DEREGISTER)
stdio_deregister(DEVNAME, 1);return error;+#else
return 0;
Don't understand why 'return 0' here if probe fails...
+#endif }
#ifndef CONFIG_DM_USB
Regards, Bin
On Thu, Oct 5, 2017 at 7:48 PM, Bin Meng bmeng.cn@gmail.com wrote:
Hi Rob,
On Wed, Oct 4, 2017 at 1:31 AM, Rob Clark robdclark@gmail.com wrote:
If probe fails, we should unregister the stdio device, else we leave dangling pointers to the parent 'struct usb_device'.
Signed-off-by: Rob Clark robdclark@gmail.com
I finally got around to debugging why things explode so badly without fixing usb_kbd vs. iomux[1] (which this patch applies on top of).
[1] https://patchwork.ozlabs.org/patch/818881/
common/usb_kbd.c | 30 ++++++++++++++++++++++++------ 1 file changed, 24 insertions(+), 6 deletions(-)
diff --git a/common/usb_kbd.c b/common/usb_kbd.c index 4c3ad95fca..82ad93f6ca 100644 --- a/common/usb_kbd.c +++ b/common/usb_kbd.c @@ -535,22 +535,40 @@ static int probe_usb_keyboard(struct usb_device *dev) error = iomux_doenv(stdin, stdinname); free(newstdin); if (error)
return error;
goto unregister_stdio; } else { /* Check if this is the standard input device. */
if (strcmp(stdinname, DEVNAME))return 1;
if (strcmp(stdinname, DEVNAME)) {error = -1;Could we use some meaningful errno?
suggestions welcome.. I wasn't sure what to pick so I just went with <0
goto unregister_stdio;} /* Reassign the console */
if (overwrite_console())return 1;
if (overwrite_console()) {error = -1;goto unregister_stdio;} error = console_assign(stdin, DEVNAME); if (error)
return error;
goto unregister_stdio; } return 0;+unregister_stdio:
/** If probe fails, the device will be removed.. leaving dangling* pointers if the stdio device is not unregistered. If u-bootnits: U-Boot
* is built without stdio_deregister(), just pretend to succeed* in order to avoid dangling pointers.*/+#if CONFIG_IS_ENABLED(SYS_STDIO_DEREGISTER)
stdio_deregister(DEVNAME, 1);return error;+#else
return 0;Don't understand why 'return 0' here if probe fails...
see above comment, basically we can't fail after we've registered the stdio device unless we can remove it. Suggestions welcome on better wording for the comment if it wasn't clear
BR, -R
+#endif }
#ifndef CONFIG_DM_USB
Regards, Bin
participants (3)
-
Bin Meng -
Peter Robinson -
Rob Clark