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
>
