linux-kernel - Re: [PATCH] USB: gadget: serial: fix null pointer access in tty

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Message-ID: <5db498dc-bfa7-a1eb-7d09-390a7fc3c248@suse.cz>
Date:   Tue, 9 Jun 2020 07:44:58 +0200
From:   Jiri Slaby <jslaby@...e.cz>
To:     Kyungtae Kim <kt0755@...il.com>,
        Greg KH <gregkh@...uxfoundation.org>
Cc:     USB list <linux-usb@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        syzkaller <syzkaller@...glegroups.com>,
        Dave Tian <dave.jing.tian@...il.com>
Subject: Re: [PATCH] USB: gadget: serial: fix null pointer access in
 tty_wakeup()

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@...il.com>
> Reported-by: Kyungtae Kim <kt0755@...il.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