On Sun, May 08, 2022 at 12:08:35PM +0800, Schspa Shi wrote: > Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> writes: > > > > Are you sure that this patch will fix the problem found by syzbot? That > > is, are you sure that the problem really was caused by two threads > > registering the same driver concurrently? > > > > Yes, from the console log from syzbot. > T8324 alloced driver_private was released by T8326. That is a smoking gun. > > The fact that the error was "use after free" suggests that the problem > > might be something else. It looks like one of the threads was trying to > > access the driver structure after the other thread had done something > > that caused it to be deallocated, which suggests an error in reference > > counting. > > > > The direct cause of this place is because of the refcount error, but the > root cause is still caused by multiple registrations > > Please refer to the following scenarios. > > T1 T2 > ------------------------------------------------------------------ > usb_gadget_register_driver_owner > driver_register driver_register > driver_find driver_find > bus_add_driver bus_add_driver > priv alloced <context switch> > drv->p = priv; > <schedule out> > kobject_init_and_add // refcount = 1; > //couldn't find an available UDC or it's busy > <context switch> > priv alloced > drv->priv = priv; > kobject_init_and_add > ---> refcount = 1 <------ > // register success > <context switch> > ===================== another ioctl/process ====================== > driver_register > driver_find > k = kset_find_obj() > ---> refcount = 2 <------ > <context out> > driver_unregister > // drv->p become T2's priv > ---> refcount = 1 <------ > <context switch> > kobject_put(k) > ---> refcount = 0 <------ > return priv->driver; > --------UAF here---------- It looks like you've got T2 calling driver_register and driver_find twice, but the overall idea is pretty clear. > There will be UAF in this scenario. > And all the logs reported by syzbot can be matched to this scenario. And in any case it is obvious that the patch is necessary. (Although I would have put the new state before the RUNNING state, to reflect the actual order in which the states occur.) Acked-by: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> Alan Stern
