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