At 2022-09-16 23:36:47, "Alan Stern" <stern@xxxxxxxxxxxxxxxxxxx> wrote:
>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
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
