On Mon, Sep 19, 2022 at 09:59:17AM +0800, Liang He wrote: > At 2022-09-16 23:36:47, "Alan Stern" <stern@xxxxxxxxxxxxxxxxxxx> wrote: > >On Fri, Sep 16, 2022 at 11:20:23PM +0800, Liang He wrote: > >> At 2022-09-16 23:04:02, "Alan Stern" <stern@xxxxxxxxxxxxxxxxxxx> wrote: > >> >On Fri, Sep 16, 2022 at 03:35:52PM +0800, Liang He wrote: > >> >> In usb_console_setup(), if we goto error_get_interface and the > >> >> usb_serial_put() may finally call kfree(serial). However, the next > >> >> line will call 'mutex_unlock(&serial->disc_mutex)' which can cause > >> >> a potential UAF bug. > >> > > >> >Why not just move the mutex_unlock() call up one line, before the > >> >usb_serial_put()? > >> >If the old code was unsafe then so is this, because s_mutex points to a > >> >mutex that is embedded within the serial structure. If the structure > >> >was deallocated by usb_serial_put() then so was the mutex. > >> Thanks for your review and this patch is indeed wrong! > >> > >> But I am not sure if we can safely move the usb_serial_put() > >> out of mutex_unlock(). > >> > >> If it is safe, I can give a new version of patch very soon. > >> > >> Can you help me confirm if it is safe? > > > >I cannot. You need to ask Johan (the USB-serial maintainer). > Johan, please confirm if this can be accepted. Yes, we should unlock before dropping the reference as Alan suggested. Note however that there is no use-after-free here as USB serial core holds another reference when the console is registered. Johan
