On Tue, Nov 15, 2022 at 08:52:02PM +0800, jiantao zhang wrote: > In the process of switching USB config from rndis to other config, > if the hardware does not support the ->pullup callback, or the > hardware encounters a low probability fault, both of them may cause > the ->pullup callback to fail, which will then cause a system panic > (use after free). > > The gadget drivers sometimes need to be unloaded regardless of the > hardware's behavior. > > Analysis as follows: > ======================================================================= > (1) write /config/usb_gadget/g1/UDC "none" > > gether_disconnect+0x2c/0x1f8 > rndis_disable+0x4c/0x74 > composite_disconnect+0x74/0xb0 > configfs_composite_disconnect+0x60/0x7c > usb_gadget_disconnect+0x70/0x124 > usb_gadget_unregister_driver+0xc8/0x1d8 > gadget_dev_desc_UDC_store+0xec/0x1e4 > > (2) rm /config/usb_gadget/g1/configs/b.1/f1 > > rndis_deregister+0x28/0x54 > rndis_free+0x44/0x7c > usb_put_function+0x14/0x1c > config_usb_cfg_unlink+0xc4/0xe0 > configfs_unlink+0x124/0x1c8 > vfs_unlink+0x114/0x1dc > > (3) rmdir /config/usb_gadget/g1/functions/rndis.gs4 > > panic+0x1fc/0x3d0 > do_page_fault+0xa8/0x46c > do_mem_abort+0x3c/0xac > el1_sync_handler+0x40/0x78 > 0xffffff801138f880 > rndis_close+0x28/0x34 > eth_stop+0x74/0x110 > dev_close_many+0x48/0x194 > rollback_registered_many+0x118/0x814 > unregister_netdev+0x20/0x30 > gether_cleanup+0x1c/0x38 > rndis_attr_release+0xc/0x14 > kref_put+0x74/0xb8 > configfs_rmdir+0x314/0x374 > > If gadget->ops->pullup() return an error, function rndis_close() will be > called, then it will causes a use-after-free problem. > ======================================================================= > > Fixes: 0a55187a1ec8 ("USB: gadget core: Issue ->disconnect() callback from > usb_gadget_disconnect()") > Signed-off-by: Jiantao Zhang <water.zhangjiantao@xxxxxxxxxx> > Signed-off-by: TaoXue <xuetao09@xxxxxxxxxx> > --- Acked-by: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> It looks like you accidentally typed a ' ' character at the start of the second line of the patch, the line that says: ret = gadget->ops->pullup(gadget, 0); This would explain why Greg said the patch was corrupted. Alan Stern > V2 -> V3: Solved the format issues of Fixes and backtraces. > V1 -> V2: V1 will affect the original function, V2 just move the callback > after "if" statement, so that the original function will not be affected. > And fixed formatting issues. > > drivers/usb/gadget/udc/core.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c > index c63c0c2cf649..bf9878e1a72a 100644 > --- a/drivers/usb/gadget/udc/core.c > +++ b/drivers/usb/gadget/udc/core.c > @@ -734,13 +734,13 @@ int usb_gadget_disconnect(struct usb_gadget *gadget) > } > ret = gadget->ops->pullup(gadget, 0); > - if (!ret) { > + if (!ret) > gadget->connected = 0; > - mutex_lock(&udc_lock); > - if (gadget->udc->driver) > - gadget->udc->driver->disconnect(gadget); > - mutex_unlock(&udc_lock); > - } > + > + mutex_lock(&udc_lock); > + if (gadget->udc->driver) > + gadget->udc->driver->disconnect(gadget); > + mutex_unlock(&udc_lock); > out: > trace_usb_gadget_disconnect(gadget, ret); > -- > 2.17.1 >