USB: gadget: serial: fix null pointer access in tty_wakeup()
A message that is not in the archive (sent off-list or not yet received): 20200609050012.GA7907@pizza01
Quoted 8 lines · Kyungtae Kim
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_structduring USB enumeration phase i.e.,set_config().
Quoted 6 lines
Although
port->port.ttyings_start_io()becomes null aftergs_close()in a separate syscall context, this tries to accesss its field (tty->flags) in the followingtty_wakeup(), which triggers the corruption.The fix checks if
port->port.ttyis still valid before being used intty_wakeup().
...> Signed-off-by: Kyungtae Kim <[email protected]>
Quoted 11 lines
Reported-by Kyungtae Kim <[email protected]>--- drivers/usb/gadget/function/u_serial.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/usb/gadget/function/u_serial.c +0
-0
drivers/usb/gadget/function/u_serial.c static int gs_start_io(struct gs_port *port) · line 564
Quoted 5 lines
drivers/usb/gadget/function/u_serial.c +1
-1@@ -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.
drivers/usb/gadget/function/u_serial.c static int gs_start_io(struct gs_port *port) · line 567
Quoted 3 lines
drivers/usb/gadget/function/u_serial.c +0
-0gs_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,
Signature · 2 lines
js suse labs