usb: kbd: Fix dangling pointers on probe fail
PATCH 1/1 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'.
---
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(-)
common/usb_kbd.c +24 -6
@@ -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
Signature · 1 line
2.13.5
PATCH 1/1 usb: kbd: Fix dangling pointers on probe fail
Quoted 4 lines · Rob Clark
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 <[email protected]>
Tested on RPi3 and a couple of other devices.
Quoted 67 lines
---
I finally got around to debugging why things explode so badly without fixingusb_kbdvs. 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(-)common/usb_kbd.c +24
-7@@ -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 at lists.denx.de
https://lists.denx.de/listinfo/u-boot
PATCH 1/1 usb: kbd: Fix dangling pointers on probe fail
In reply to Rob ClarkHi Rob,
Quoted 19 lines · Rob Clark
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 <[email protected]>---
I finally got around to debugging why things explode so badly without fixingusb_kbdvs. 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(-)common/usb_kbd.c +0
-0@@ -535,22 +535,40 @@ static int probe_usb_keyboard(struct usb_device *dev) error =iomux_doenv (stdin, stdinname);
common/usb_kbd.c static int probe_usb_keyboard(struct usb_device *dev) · line 542
Quoted 10 lines
common/usb_kbd.c +3
-3free (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?
Quoted 13 lines
common/usb_kbd.c +6
-2+ 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)
common/usb_kbd.c static int probe_usb_keyboard(struct usb_device *dev) · line 562
Quoted 10 lines
common/usb_kbd.c +6
-1-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
common/usb_kbd.c static int probe_usb_keyboard(struct usb_device *dev) · line 570
Quoted 8 lines
common/usb_kbd.c +8
-0+* 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...
common/usb_kbd.c static int probe_usb_keyboard(struct usb_device *dev)
Quoted 5 lines
common/usb_kbd.c +1
-1+#endif }#ifndef CONFIG_DM_USB --
Regards,
Bin
PATCH 1/1 usb: kbd: Fix dangling pointers on probe fail
Quoted 26 lines · Bin Meng
Hi Rob,
Quoted 23 lines · Rob Clark
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 <[email protected]>---
I finally got around to debugging why things explode so badly without fixingusb_kbdvs. 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(-)common/usb_kbd.c +1
-1@@ -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;
Quoted 8 lines
common/usb_kbd.c static int probe_usb_keyboard(struct usb_device *dev) · line 542
Quoted 6 lines
common/usb_kbd.c +2
-2}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
Quoted 26 lines
Quoted 13 lines
common/usb_kbd.c +6
-2+ 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)common/usb_kbd.c static int probe_usb_keyboard(struct usb_device *dev) · line 562
Quoted 10 lines
common/usb_kbd.c +6
-1-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
Quoted 10 lines
common/usb_kbd.c static int probe_usb_keyboard(struct usb_device *dev) · line 570
Quoted 8 lines
common/usb_kbd.c +8
-0+* 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
Quoted 8 lines
common/usb_kbd.c static int probe_usb_keyboard(struct usb_device *dev)
Quoted 5 lines
common/usb_kbd.c +1
-1+#endif }#ifndef CONFIG_DM_USB --Regards,
Bin