* Re: [PATCH v3] USB: gadget: Fix use-after-free during usb config switch
2022-11-15 12:52 ` [PATCH v3] USB: gadget: Fix use-after-free during usb config switch jiantao zhang
@ 2022-11-15 13:40 ` Greg KH
2022-11-15 15:45 ` Alan Stern
1 sibling, 0 replies; 3+ messages in thread
From: Greg KH @ 2022-11-15 13:40 UTC (permalink / raw)
To: jiantao zhang
Cc: stern, jakobkoschel, geert+renesas, colin.i.king,
薛涛, linux-usb, linux-kernel, Caiyadong, xuhaiyang
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@huawei.com>
> Signed-off-by: TaoXue <xuetao09@huawei.com>
> ---
> 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.
This patch is really corrupted and can not be applied. How did you
generate it?
The email is also showing up as unauthenticated and seems like it might
be spoofed from your huawei.com domain, which is not good.
Please fix up both of those issues if you wish for this to be accepted.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH v3] USB: gadget: Fix use-after-free during usb config switch
2022-11-15 12:52 ` [PATCH v3] USB: gadget: Fix use-after-free during usb config switch jiantao zhang
2022-11-15 13:40 ` Greg KH
@ 2022-11-15 15:45 ` Alan Stern
1 sibling, 0 replies; 3+ messages in thread
From: Alan Stern @ 2022-11-15 15:45 UTC (permalink / raw)
To: jiantao zhang
Cc: gregkh, jakobkoschel, geert+renesas, colin.i.king,
薛涛, linux-usb, linux-kernel, Caiyadong, xuhaiyang
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@huawei.com>
> Signed-off-by: TaoXue <xuetao09@huawei.com>
> ---
Acked-by: Alan Stern <stern@rowland.harvard.edu>
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
>
^ permalink raw reply [flat|nested] 3+ messages in thread