On Fri, Sep 16, 2022 at 11:20:23PM +0800, Liang He wrote: > > > At 2022-09-16 23:04:02, "Alan Stern" <stern@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxx> > >> --- > >> > >> 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