Re: [PATCH] USB: console: fix potential use after free
On Wed, 2015-02-11 at 14:55 +0800, Johan Hovold wrote: On Tue, Feb 10, 2015 at 08:39:26PM +, Ben Hutchings wrote: On Mon, 2015-01-05 at 16:04 +0100, Johan Hovold wrote: Use tty kref to release the fake tty in usb_console_setup to avoid use after free if the underlying serial driver has acquired a reference. Note that using the tty destructor release_one_tty requires some more state to be initialised. [...] --- a/drivers/usb/serial/console.c +++ b/drivers/usb/serial/console.c [...] @@ -137,14 +139,17 @@ static int usb_console_setup(struct console *co, char *options) goto reset_open_count; } kref_init(tty-kref); - tty_port_tty_set(port-port, tty); tty-driver = usb_serial_tty_driver; tty-index = co-index; init_ldsem(tty-ldisc_sem); + INIT_LIST_HEAD(tty-tty_files); + kref_get(tty-driver-kref); + tty-ops = usb_console_fake_tty_ops; [...] Do we also need: __module_get(tty-driver-owner); or am I missing something? When the console setup code is running, and while the console is open, everything is pinned through a reference to the usb-serial-driver module (the tty layer also pins usb-serial core directly). We should be holding a reference to the usb-serial driver module (which in turns pins usb-serial core, which is the tty driver) for when the console isn't open as well, although that is not directly related to the fix in question. Sure, but release_one_tty() expects that the tty has a reference to the module and it will drop that reference. (Normally tty_init_dev() takes that reference.) I think that means that for a USB serial console we take one reference to the module when setting up but drop two references when cleaning up. Ben. Yes, the USB console is a bit of a hack. -- Ben Hutchings Never attribute to conspiracy what can adequately be explained by stupidity. signature.asc Description: This is a digitally signed message part
Re: [PATCH] USB: console: fix potential use after free
On Mon, Feb 16, 2015 at 03:49:17AM +, Ben Hutchings wrote: On Wed, 2015-02-11 at 14:55 +0800, Johan Hovold wrote: On Tue, Feb 10, 2015 at 08:39:26PM +, Ben Hutchings wrote: On Mon, 2015-01-05 at 16:04 +0100, Johan Hovold wrote: Use tty kref to release the fake tty in usb_console_setup to avoid use after free if the underlying serial driver has acquired a reference. Note that using the tty destructor release_one_tty requires some more state to be initialised. [...] --- a/drivers/usb/serial/console.c +++ b/drivers/usb/serial/console.c [...] @@ -137,14 +139,17 @@ static int usb_console_setup(struct console *co, char *options) goto reset_open_count; } kref_init(tty-kref); - tty_port_tty_set(port-port, tty); tty-driver = usb_serial_tty_driver; tty-index = co-index; init_ldsem(tty-ldisc_sem); + INIT_LIST_HEAD(tty-tty_files); + kref_get(tty-driver-kref); + tty-ops = usb_console_fake_tty_ops; [...] Do we also need: __module_get(tty-driver-owner); or am I missing something? When the console setup code is running, and while the console is open, everything is pinned through a reference to the usb-serial-driver module (the tty layer also pins usb-serial core directly). We should be holding a reference to the usb-serial driver module (which in turns pins usb-serial core, which is the tty driver) for when the console isn't open as well, although that is not directly related to the fix in question. Sure, but release_one_tty() expects that the tty has a reference to the module and it will drop that reference. (Normally tty_init_dev() takes that reference.) I think that means that for a USB serial console we take one reference to the module when setting up but drop two references when cleaning up. Ah, but the tty-driver owner will be null as usb-serial has to be compiled in to enable the usb console (and therefore the point I made above about a missing reference is also moot). So nothing is leaking, but I should probably add a comment or a call to __module_get() nonetheless to make this clear. Yes, the USB console is a bit of a hack. Thanks, Johan -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] USB: console: fix potential use after free
On Mon, 2015-01-05 at 16:04 +0100, Johan Hovold wrote: Use tty kref to release the fake tty in usb_console_setup to avoid use after free if the underlying serial driver has acquired a reference. Note that using the tty destructor release_one_tty requires some more state to be initialised. [...] --- a/drivers/usb/serial/console.c +++ b/drivers/usb/serial/console.c [...] @@ -137,14 +139,17 @@ static int usb_console_setup(struct console *co, char *options) goto reset_open_count; } kref_init(tty-kref); - tty_port_tty_set(port-port, tty); tty-driver = usb_serial_tty_driver; tty-index = co-index; init_ldsem(tty-ldisc_sem); + INIT_LIST_HEAD(tty-tty_files); + kref_get(tty-driver-kref); + tty-ops = usb_console_fake_tty_ops; [...] Do we also need: __module_get(tty-driver-owner); or am I missing something? Ben. -- Ben Hutchings I say we take off; nuke the site from orbit. It's the only way to be sure. signature.asc Description: This is a digitally signed message part
Re: [PATCH] USB: console: fix potential use after free
On Tue, Feb 10, 2015 at 08:39:26PM +, Ben Hutchings wrote: On Mon, 2015-01-05 at 16:04 +0100, Johan Hovold wrote: Use tty kref to release the fake tty in usb_console_setup to avoid use after free if the underlying serial driver has acquired a reference. Note that using the tty destructor release_one_tty requires some more state to be initialised. [...] --- a/drivers/usb/serial/console.c +++ b/drivers/usb/serial/console.c [...] @@ -137,14 +139,17 @@ static int usb_console_setup(struct console *co, char *options) goto reset_open_count; } kref_init(tty-kref); - tty_port_tty_set(port-port, tty); tty-driver = usb_serial_tty_driver; tty-index = co-index; init_ldsem(tty-ldisc_sem); + INIT_LIST_HEAD(tty-tty_files); + kref_get(tty-driver-kref); + tty-ops = usb_console_fake_tty_ops; [...] Do we also need: __module_get(tty-driver-owner); or am I missing something? When the console setup code is running, and while the console is open, everything is pinned through a reference to the usb-serial-driver module (the tty layer also pins usb-serial core directly). We should be holding a reference to the usb-serial driver module (which in turns pins usb-serial core, which is the tty driver) for when the console isn't open as well, although that is not directly related to the fix in question. Yes, the USB console is a bit of a hack. Johan -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
