| Message ID | alpine.LNX.2.00.1102132206390.18930@swampdragon.chaosbits.net |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Jesper Juhl <jj@chaosbits.net> Date: Sun, 13 Feb 2011 22:15:35 +0100 (CET) > In drivers/net/usb/hso.c::hso_create_bulk_serial_device() we have this > code: > ... > serial = kzalloc(sizeof(*serial), GFP_KERNEL); > if (!serial) > goto exit; > ... > exit: > hso_free_tiomget(serial); > ... > hso_free_tiomget() directly dereferences its argument, which in the > example above is a NULL pointer, ouch. > I could just add a 'if (serial)' test at the 'exit' label, but since most > freeing functions in the kernel accept NULL pointers (and it seems like > this was also assumed here) I opted to instead change 'hso_free_tiomget()' > so that it is safe to call it with a NULL argument. I also modified the > function to get rid of a pointles conditional before the call to > 'usb_free_urb()' since that function already tests for NULL itself - > besides fixing the NULL deref this change also buys us a few bytes in > size. > Before: > $ size drivers/net/usb/hso.o > text data bss dec hex filename > 32200 592 9960 42752 a700 drivers/net/usb/hso.o > After: > $ size drivers/net/usb/hso.o > text data bss dec hex filename > 32196 592 9960 42748 a6fc drivers/net/usb/hso.o > > Signed-off-by: Jesper Juhl <jj@chaosbits.net> Applied. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c index bed8fce..6d83812 100644 --- a/drivers/net/usb/hso.c +++ b/drivers/net/usb/hso.c @@ -2628,15 +2628,15 @@ exit: static void hso_free_tiomget(struct hso_serial *serial) { - struct hso_tiocmget *tiocmget = serial->tiocmget; + struct hso_tiocmget *tiocmget; + if (!serial) + return; + tiocmget = serial->tiocmget; if (tiocmget) { - if (tiocmget->urb) { - usb_free_urb(tiocmget->urb); - tiocmget->urb = NULL; - } + usb_free_urb(tiocmget->urb); + tiocmget->urb = NULL; serial->tiocmget = NULL; kfree(tiocmget); - } }
In drivers/net/usb/hso.c::hso_create_bulk_serial_device() we have this code: ... serial = kzalloc(sizeof(*serial), GFP_KERNEL); if (!serial) goto exit; ... exit: hso_free_tiomget(serial); ... hso_free_tiomget() directly dereferences its argument, which in the example above is a NULL pointer, ouch. I could just add a 'if (serial)' test at the 'exit' label, but since most freeing functions in the kernel accept NULL pointers (and it seems like this was also assumed here) I opted to instead change 'hso_free_tiomget()' so that it is safe to call it with a NULL argument. I also modified the function to get rid of a pointles conditional before the call to 'usb_free_urb()' since that function already tests for NULL itself - besides fixing the NULL deref this change also buys us a few bytes in size. Before: $ size drivers/net/usb/hso.o text data bss dec hex filename 32200 592 9960 42752 a700 drivers/net/usb/hso.o After: $ size drivers/net/usb/hso.o text data bss dec hex filename 32196 592 9960 42748 a6fc drivers/net/usb/hso.o Signed-off-by: Jesper Juhl <jj@chaosbits.net> --- hso.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)