lkml.org 
[lkml]   [2020]   [Jun]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] USB: gadget: serial: fix null pointer access in tty_wakeup()
From
Date
On 09. 06. 20, 7:00, Kyungtae Kim wrote:
> FuzzUSB (a variant of syzkaller) found an illegal memory access
> while finalizing an enumeration for a serial (acm) gadget.
>
> Reference: https://lkml.org/lkml/2020/6/7/3
>
> The bug arises because of accessing null instance of tty_struct
> during USB enumeration phase i.e., set_config().
>
> Although port->port.tty in gs_start_io() becomes null after gs_close() in
> a separate syscall context, this tries to accesss its field (tty->flags)
> in the following tty_wakeup(), which triggers the corruption.
>
> The fix checks if port->port.tty is still valid before being used
> in tty_wakeup().
...> Signed-off-by: Kyungtae Kim <kt0755@gmail.com>
> Reported-by: Kyungtae Kim <kt0755@gmail.com>
>
> ---
> drivers/usb/gadget/function/u_serial.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c
> index 8167d379e115..cf4f876783d3 100644
> --- a/drivers/usb/gadget/function/u_serial.c
> +++ b/drivers/usb/gadget/function/u_serial.c
> @@ -561,7 +561,7 @@ static int gs_start_io(struct gs_port *port)
> port->n_read = 0;
> started = gs_start_rx(port);
>
> - if (started) {
> + if (started && port->port.tty != NULL) {

Could you explain, what prevents port->port.tty to be non-NULL on the
lines below? As far as I can see, port->port.count is set to zero at the
same place as "port->port.tty = NULL;" in gs_close. And port->port.count
is tested in the gs_start_io's caller, i.e. in gserial_connect. All this
happens under port->port_lock.

> gs_start_tx(port);
> /* Unblock any pending writes into our circular buffer, in case
> * we didn't in gs_start_tx() */

If it is a race, it will still race, only the race window would be
smaller. Or am I missing something?

The whole use of port->port.tty looks suspicious in the driver. It might
work as port->port_lock at places. But converting the uses to
tty_port_tty_set & tty_port_tty_get should fix also this problem. And
tty_port_tty_wakeup in this particular situation you are fixing. (And to
tty_port_open/close in the long run.)

thanks,
--
js
suse labs

\
 
 \ /
  Last update: 2020-06-09 07:46    [W:0.109 / U:0.048 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site