* [PATCH] USB: serial: console: Fix potential use-after-free in usb_console_setup()
@ 2022-09-16 7:35 Liang He
2022-09-16 15:04 ` Alan Stern
0 siblings, 1 reply; 6+ messages in thread
From: Liang He @ 2022-09-16 7:35 UTC (permalink / raw)
To: johan, gregkh, linux-usb; +Cc: windhl
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(-)
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;
}
--
2.25.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] USB: serial: console: Fix potential use-after-free in usb_console_setup()
2022-09-16 7:35 [PATCH] USB: serial: console: Fix potential use-after-free in usb_console_setup() Liang He
@ 2022-09-16 15:04 ` Alan Stern
2022-09-16 15:20 ` Liang He
0 siblings, 1 reply; 6+ messages in thread
From: Alan Stern @ 2022-09-16 15:04 UTC (permalink / raw)
To: Liang He; +Cc: johan, gregkh, linux-usb
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re:Re: [PATCH] USB: serial: console: Fix potential use-after-free in usb_console_setup()
2022-09-16 15:04 ` Alan Stern
@ 2022-09-16 15:20 ` Liang He
2022-09-16 15:36 ` Alan Stern
0 siblings, 1 reply; 6+ messages in thread
From: Liang He @ 2022-09-16 15:20 UTC (permalink / raw)
To: Alan Stern; +Cc: johan, gregkh, linux-usb
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Re: [PATCH] USB: serial: console: Fix potential use-after-free in usb_console_setup()
2022-09-16 15:20 ` Liang He
@ 2022-09-16 15:36 ` Alan Stern
2022-09-19 1:59 ` Liang He
0 siblings, 1 reply; 6+ messages in thread
From: Alan Stern @ 2022-09-16 15:36 UTC (permalink / raw)
To: Liang He; +Cc: johan, gregkh, linux-usb
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re:Re: Re: [PATCH] USB: serial: console: Fix potential use-after-free in usb_console_setup()
2022-09-16 15:36 ` Alan Stern
@ 2022-09-19 1:59 ` Liang He
2022-09-19 6:51 ` Johan Hovold
0 siblings, 1 reply; 6+ messages in thread
From: Liang He @ 2022-09-19 1:59 UTC (permalink / raw)
To: Alan Stern; +Cc: johan, gregkh, linux-usb
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Re: Re: [PATCH] USB: serial: console: Fix potential use-after-free in usb_console_setup()
2022-09-19 1:59 ` Liang He
@ 2022-09-19 6:51 ` Johan Hovold
0 siblings, 0 replies; 6+ messages in thread
From: Johan Hovold @ 2022-09-19 6:51 UTC (permalink / raw)
To: Liang He; +Cc: Alan Stern, gregkh, linux-usb
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-09-19 6:51 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-16 7:35 [PATCH] USB: serial: console: Fix potential use-after-free in usb_console_setup() Liang He
2022-09-16 15:04 ` Alan Stern
2022-09-16 15:20 ` Liang He
2022-09-16 15:36 ` Alan Stern
2022-09-19 1:59 ` Liang He
2022-09-19 6:51 ` Johan Hovold
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.