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?
Thanks again,
Liang
