| Message ID | 20220916073552.4093048-1-windhl@126.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | USB: serial: console: Fix potential use-after-free in usb_console_setup() | expand |
On Fri, Sep 16, 2022 at 03:35:52PM +0800, Liang He wrote: > In usb_console_setup(), if we goto error_get_interface and the > usb_serial_put() may finally call kfree(serial). However, the next > line will call 'mutex_unlock(&serial->disc_mutex)' which can cause > a potential UAF bug. Why not just move the mutex_unlock() call up one line, before the usb_serial_put()? > Fixes: 7bd032dc2793 ("USB serial: update the console driver") > Signed-off-by: Liang He <windhl@126.com> > --- > > I don't know if the refcount can be zero here, so if it cannot be zero, > this code is safe and please ignore my patch. > > drivers/usb/serial/console.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/usb/serial/console.c b/drivers/usb/serial/console.c > index b97aa40ca4d1..21ac2dd6baca 100644 > --- a/drivers/usb/serial/console.c > +++ b/drivers/usb/serial/console.c > @@ -62,6 +62,7 @@ static int usb_console_setup(struct console *co, char *options) > int cflag = CREAD | HUPCL | CLOCAL; > char *s; > struct usb_serial *serial; > + struct mutex *s_mutex; > struct usb_serial_port *port; > int retval; > struct tty_struct *tty = NULL; > @@ -116,7 +117,7 @@ static int usb_console_setup(struct console *co, char *options) > return -ENODEV; > } > serial = port->serial; > - > + s_mutex = &serial->disc_mutex; > retval = usb_autopm_get_interface(serial->interface); > if (retval) > goto error_get_interface; > @@ -190,7 +191,7 @@ static int usb_console_setup(struct console *co, char *options) > usb_autopm_put_interface(serial->interface); > error_get_interface: > usb_serial_put(serial); > - mutex_unlock(&serial->disc_mutex); > + mutex_unlock(s_mutex); If the old code was unsafe then so is this, because s_mutex points to a mutex that is embedded within the serial structure. If the structure was deallocated by usb_serial_put() then so was the mutex. Alan Stern > return retval; > } > > -- > 2.25.1 >
At 2022-09-16 23:04:02, "Alan Stern" <stern@rowland.harvard.edu> wrote: >On Fri, Sep 16, 2022 at 03:35:52PM +0800, Liang He wrote: >> In usb_console_setup(), if we goto error_get_interface and the >> usb_serial_put() may finally call kfree(serial). However, the next >> line will call 'mutex_unlock(&serial->disc_mutex)' which can cause >> a potential UAF bug. > >Why not just move the mutex_unlock() call up one line, before the >usb_serial_put()? > >> Fixes: 7bd032dc2793 ("USB serial: update the console driver") >> Signed-off-by: Liang He <windhl@126.com> >> --- >> >> I don't know if the refcount can be zero here, so if it cannot be zero, >> this code is safe and please ignore my patch. >> >> drivers/usb/serial/console.c | 5 +++-- >> 1 file changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/usb/serial/console.c b/drivers/usb/serial/console.c >> index b97aa40ca4d1..21ac2dd6baca 100644 >> --- a/drivers/usb/serial/console.c >> +++ b/drivers/usb/serial/console.c >> @@ -62,6 +62,7 @@ static int usb_console_setup(struct console *co, char *options) >> int cflag = CREAD | HUPCL | CLOCAL; >> char *s; >> struct usb_serial *serial; >> + struct mutex *s_mutex; >> struct usb_serial_port *port; >> int retval; >> struct tty_struct *tty = NULL; >> @@ -116,7 +117,7 @@ static int usb_console_setup(struct console *co, char *options) >> return -ENODEV; >> } >> serial = port->serial; >> - >> + s_mutex = &serial->disc_mutex; >> retval = usb_autopm_get_interface(serial->interface); >> if (retval) >> goto error_get_interface; >> @@ -190,7 +191,7 @@ static int usb_console_setup(struct console *co, char *options) >> usb_autopm_put_interface(serial->interface); >> error_get_interface: >> usb_serial_put(serial); >> - mutex_unlock(&serial->disc_mutex); >> + mutex_unlock(s_mutex); > >If the old code was unsafe then so is this, because s_mutex points to a >mutex that is embedded within the serial structure. If the structure >was deallocated by usb_serial_put() then so was the mutex. > >Alan Stern > >> return retval; >> } >> >> -- >> 2.25.1 >> Hi, Alan Stern, Thanks for your review and this patch is indeed wrong! But I am not sure if we can safely move the usb_serial_put() out of mutex_unlock(). If it is safe, I can give a new version of patch very soon. Can you help me confirm if it is safe? Thanks again, Liang
On Fri, Sep 16, 2022 at 11:20:23PM +0800, Liang He wrote: > > > At 2022-09-16 23:04:02, "Alan Stern" <stern@rowland.harvard.edu> wrote: > >On Fri, Sep 16, 2022 at 03:35:52PM +0800, Liang He wrote: > >> In usb_console_setup(), if we goto error_get_interface and the > >> usb_serial_put() may finally call kfree(serial). However, the next > >> line will call 'mutex_unlock(&serial->disc_mutex)' which can cause > >> a potential UAF bug. > > > >Why not just move the mutex_unlock() call up one line, before the > >usb_serial_put()? > > > >> Fixes: 7bd032dc2793 ("USB serial: update the console driver") > >> Signed-off-by: Liang He <windhl@126.com> > >> --- > >> > >> I don't know if the refcount can be zero here, so if it cannot be zero, > >> this code is safe and please ignore my patch. > >> > >> drivers/usb/serial/console.c | 5 +++-- > >> 1 file changed, 3 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/usb/serial/console.c b/drivers/usb/serial/console.c > >> index b97aa40ca4d1..21ac2dd6baca 100644 > >> --- a/drivers/usb/serial/console.c > >> +++ b/drivers/usb/serial/console.c > >> @@ -62,6 +62,7 @@ static int usb_console_setup(struct console *co, char *options) > >> int cflag = CREAD | HUPCL | CLOCAL; > >> char *s; > >> struct usb_serial *serial; > >> + struct mutex *s_mutex; > >> struct usb_serial_port *port; > >> int retval; > >> struct tty_struct *tty = NULL; > >> @@ -116,7 +117,7 @@ static int usb_console_setup(struct console *co, char *options) > >> return -ENODEV; > >> } > >> serial = port->serial; > >> - > >> + s_mutex = &serial->disc_mutex; > >> retval = usb_autopm_get_interface(serial->interface); > >> if (retval) > >> goto error_get_interface; > >> @@ -190,7 +191,7 @@ static int usb_console_setup(struct console *co, char *options) > >> usb_autopm_put_interface(serial->interface); > >> error_get_interface: > >> usb_serial_put(serial); > >> - mutex_unlock(&serial->disc_mutex); > >> + mutex_unlock(s_mutex); > > > >If the old code was unsafe then so is this, because s_mutex points to a > >mutex that is embedded within the serial structure. If the structure > >was deallocated by usb_serial_put() then so was the mutex. > > > >Alan Stern > > > >> return retval; > >> } > >> > >> -- > >> 2.25.1 > >> > > Hi, Alan Stern, > > Thanks for your review and this patch is indeed wrong! > > But I am not sure if we can safely move the usb_serial_put() > out of mutex_unlock(). > > If it is safe, I can give a new version of patch very soon. > > Can you help me confirm if it is safe? I cannot. You need to ask Johan (the USB-serial maintainer). Alan Stern
At 2022-09-16 23:36:47, "Alan Stern" <stern@rowland.harvard.edu> wrote: >On Fri, Sep 16, 2022 at 11:20:23PM +0800, Liang He wrote: >> >> >> At 2022-09-16 23:04:02, "Alan Stern" <stern@rowland.harvard.edu> wrote: >> >On Fri, Sep 16, 2022 at 03:35:52PM +0800, Liang He wrote: >> >> In usb_console_setup(), if we goto error_get_interface and the >> >> usb_serial_put() may finally call kfree(serial). However, the next >> >> line will call 'mutex_unlock(&serial->disc_mutex)' which can cause >> >> a potential UAF bug. >> > >> >Why not just move the mutex_unlock() call up one line, before the >> >usb_serial_put()? >> > >> >> Fixes: 7bd032dc2793 ("USB serial: update the console driver") >> >> Signed-off-by: Liang He <windhl@126.com> >> >> --- >> >> >> >> I don't know if the refcount can be zero here, so if it cannot be zero, >> >> this code is safe and please ignore my patch. >> >> >> >> drivers/usb/serial/console.c | 5 +++-- >> >> 1 file changed, 3 insertions(+), 2 deletions(-) >> >> >> >> diff --git a/drivers/usb/serial/console.c b/drivers/usb/serial/console.c >> >> index b97aa40ca4d1..21ac2dd6baca 100644 >> >> --- a/drivers/usb/serial/console.c >> >> +++ b/drivers/usb/serial/console.c >> >> @@ -62,6 +62,7 @@ static int usb_console_setup(struct console *co, char *options) >> >> int cflag = CREAD | HUPCL | CLOCAL; >> >> char *s; >> >> struct usb_serial *serial; >> >> + struct mutex *s_mutex; >> >> struct usb_serial_port *port; >> >> int retval; >> >> struct tty_struct *tty = NULL; >> >> @@ -116,7 +117,7 @@ static int usb_console_setup(struct console *co, char *options) >> >> return -ENODEV; >> >> } >> >> serial = port->serial; >> >> - >> >> + s_mutex = &serial->disc_mutex; >> >> retval = usb_autopm_get_interface(serial->interface); >> >> if (retval) >> >> goto error_get_interface; >> >> @@ -190,7 +191,7 @@ static int usb_console_setup(struct console *co, char *options) >> >> usb_autopm_put_interface(serial->interface); >> >> error_get_interface: >> >> usb_serial_put(serial); >> >> - mutex_unlock(&serial->disc_mutex); >> >> + mutex_unlock(s_mutex); >> > >> >If the old code was unsafe then so is this, because s_mutex points to a >> >mutex that is embedded within the serial structure. If the structure >> >was deallocated by usb_serial_put() then so was the mutex. >> > >> >Alan Stern >> > >> >> return retval; >> >> } >> >> >> >> -- >> >> 2.25.1 >> >> >> >> Hi, Alan Stern, >> >> Thanks for your review and this patch is indeed wrong! >> >> But I am not sure if we can safely move the usb_serial_put() >> out of mutex_unlock(). >> >> If it is safe, I can give a new version of patch very soon. >> >> Can you help me confirm if it is safe? > >I cannot. You need to ask Johan (the USB-serial maintainer). > >Alan Stern Still thanks! And from a recent similar commit, I think we can move mutex_unlock above the usb_serial_put(): https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=v6.0-rc5&id=6c53b45c71b4920b5e62f0ea8079a1da382b9434 Johan, please confirm if this can be accepted. Thanks, Liang
On Mon, Sep 19, 2022 at 09:59:17AM +0800, Liang He wrote: > At 2022-09-16 23:36:47, "Alan Stern" <stern@rowland.harvard.edu> wrote: > >On Fri, Sep 16, 2022 at 11:20:23PM +0800, Liang He wrote: > >> At 2022-09-16 23:04:02, "Alan Stern" <stern@rowland.harvard.edu> wrote: > >> >On Fri, Sep 16, 2022 at 03:35:52PM +0800, Liang He wrote: > >> >> In usb_console_setup(), if we goto error_get_interface and the > >> >> usb_serial_put() may finally call kfree(serial). However, the next > >> >> line will call 'mutex_unlock(&serial->disc_mutex)' which can cause > >> >> a potential UAF bug. > >> > > >> >Why not just move the mutex_unlock() call up one line, before the > >> >usb_serial_put()? > >> >If the old code was unsafe then so is this, because s_mutex points to a > >> >mutex that is embedded within the serial structure. If the structure > >> >was deallocated by usb_serial_put() then so was the mutex. > >> Thanks for your review and this patch is indeed wrong! > >> > >> But I am not sure if we can safely move the usb_serial_put() > >> out of mutex_unlock(). > >> > >> If it is safe, I can give a new version of patch very soon. > >> > >> Can you help me confirm if it is safe? > > > >I cannot. You need to ask Johan (the USB-serial maintainer). > Johan, please confirm if this can be accepted. Yes, we should unlock before dropping the reference as Alan suggested. Note however that there is no use-after-free here as USB serial core holds another reference when the console is registered. Johan
diff --git a/drivers/usb/serial/console.c b/drivers/usb/serial/console.c index b97aa40ca4d1..21ac2dd6baca 100644 --- a/drivers/usb/serial/console.c +++ b/drivers/usb/serial/console.c @@ -62,6 +62,7 @@ static int usb_console_setup(struct console *co, char *options) int cflag = CREAD | HUPCL | CLOCAL; char *s; struct usb_serial *serial; + struct mutex *s_mutex; struct usb_serial_port *port; int retval; struct tty_struct *tty = NULL; @@ -116,7 +117,7 @@ static int usb_console_setup(struct console *co, char *options) return -ENODEV; } serial = port->serial; - + s_mutex = &serial->disc_mutex; retval = usb_autopm_get_interface(serial->interface); if (retval) goto error_get_interface; @@ -190,7 +191,7 @@ static int usb_console_setup(struct console *co, char *options) usb_autopm_put_interface(serial->interface); error_get_interface: usb_serial_put(serial); - mutex_unlock(&serial->disc_mutex); + mutex_unlock(s_mutex); return retval; }
In usb_console_setup(), if we goto error_get_interface and the usb_serial_put() may finally call kfree(serial). However, the next line will call 'mutex_unlock(&serial->disc_mutex)' which can cause a potential UAF bug. Fixes: 7bd032dc2793 ("USB serial: update the console driver") Signed-off-by: Liang He <windhl@126.com> --- I don't know if the refcount can be zero here, so if it cannot be zero, this code is safe and please ignore my patch. drivers/usb/serial/console.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)