[PATCH] USB: gadget: Fix CFI failure during usb config switch.

[PATCH] USB: gadget: Fix CFI failure during usb config switch.

[Jiantao Zhang]

This reverts commit 0a55187a1ec8c

In the process of switching USB config from rndis to other config, if function gadget->ops->pullup() return an error,it will inevitably cause a CFI failure(Linux version 5.10.43).

analysis as follows:
======================================================
(1) write /config/usb_gadget/g1/UDC "none"   (init.usb.configfs.rc:2)

gether_disconnect+0x2c/0x1f8     (dev->port_usb = NULL)
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

in function usb_gadget_disconnect(),gadget->udc->driver->disconnect()
will not be called when gadget->ops->pullup() return an error, therefore, pointer dev->port will not be set to NULL.

(2) rm /config/usb_gadget/g1/configs/b.1/f1    (init.usb.configfs.rc:8)
    (f1 -> ../../../../usb_gadget/g1/functions/rndis.gs4)

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
    (init.usb.configfs.rc:11)

CFI failure (target: [<ffffff814bc20c00>] 0000000068f50078):
CPU: 2 PID: 1 Comm: init VIP: 00 Tainted: G   W  O   5.10.43 #1
Call trace:
 __cfi_slowpath+0x300/0x3bc
 rndis_signal_disconnect+0x1e0/0x204
 rndis_close+0x24/0x2c
 eth_stop+0xd0/0x234           (if dev->port_usb != NULL, call rndis_close)
 __dev_close_many+0x204/0x2d4
 dev_close_many+0x48/0x2c8
 rollback_registered_many+0x184/0xdac
 unregister_netdevice_queue+0xf8/0x24c
 rndis_free_inst+0x78/0xc8
 rndis_attr_release+0x3c/0x84
 config_item_release+0x6c/0x180
 configfs_rmdir+0x7e0/0xca0

Since the rndis memory has been freed in step2, function rndis_close cannot be called here. In function eth_stop(), if pointer dev->port_usb is NULL, function rndis_close() will not be called. So, if
gadget->ops->pullup() return an error in step1, a CFI failure will be
caused here.
======================================================
Through above analysis, i think gadget->udc->driver->disconnect() need to be called even if gadget->udc->driver->disconnect() return an error.

Signed-off-by: Jiantao Zhang <water.zhangjiantao@huawei.com>
Signed-off-by: TaoXue <xuetao09@huawei.com>
---
 drivers/usb/gadget/udc/core.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c index c63c0c2cf649..b502b2ac4824 100644
--- a/drivers/usb/gadget/udc/core.c
+++ b/drivers/usb/gadget/udc/core.c
@@ -707,9 +707,6 @@ EXPORT_SYMBOL_GPL(usb_gadget_connect);
  * as a disconnect (when a VBUS session is active).  Not all systems
  * support software pullup controls.
  *
- * Following a successful disconnect, invoke the ->disconnect() callback
- * for the current gadget driver so that UDC drivers don't need to.
- *
  * Returns zero on success, else negative errno.
  */
 int usb_gadget_disconnect(struct usb_gadget *gadget) @@ -734,13 +731,8 @@ 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);
-	}
 
 out:
 	trace_usb_gadget_disconnect(gadget, ret); @@ -1532,6 +1524,11 @@ static void gadget_unbind_driver(struct device *dev)
 	kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE);
 
 	usb_gadget_disconnect(gadget);
+
+	mutex_lock(&udc_lock);
+	udc->driver->disconnect(udc->gadget);
+	mutex_unlock(&udc_lock);
+
 	usb_gadget_disable_async_callbacks(udc);
 	if (gadget->irq)
 		synchronize_irq(gadget->irq);
@@ -1626,6 +1623,11 @@ static ssize_t soft_connect_store(struct device *dev,
 		usb_gadget_connect(udc->gadget);
 	} else if (sysfs_streq(buf, "disconnect")) {
 		usb_gadget_disconnect(udc->gadget);
+
+		mutex_lock(&udc_lock);
+		udc->driver->disconnect(udc->gadget);
+		mutex_unlock(&udc_lock);
+
 		usb_gadget_udc_stop(udc);
 	} else {
 		dev_err(dev, "unsupported command '%s'\n", buf);
--
2.17.1

Re: [PATCH] USB: gadget: Fix CFI failure during usb config switch.

[gregkh]

On Mon, Nov 07, 2022 at 08:56:06AM +0000, Jiantao Zhang wrote:
> This reverts commit 0a55187a1ec8c

Please show the full hash for this line.

> 
> In the process of switching USB config from rndis to other config, if function gadget->ops->pullup() return an error,it will inevitably cause a CFI failure(Linux version 5.10.43).

Please wrap the line properly.

And CFI is not in 5.10, so why is this an issue?

> 
> analysis as follows:
> ======================================================
> (1) write /config/usb_gadget/g1/UDC "none"   (init.usb.configfs.rc:2)
> 
> gether_disconnect+0x2c/0x1f8     (dev->port_usb = NULL)
> 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
> 
> in function usb_gadget_disconnect(),gadget->udc->driver->disconnect()
> will not be called when gadget->ops->pullup() return an error, therefore, pointer dev->port will not be set to NULL.
> 
> (2) rm /config/usb_gadget/g1/configs/b.1/f1    (init.usb.configfs.rc:8)
>     (f1 -> ../../../../usb_gadget/g1/functions/rndis.gs4)
> 
> 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
>     (init.usb.configfs.rc:11)
> 
> CFI failure (target: [<ffffff814bc20c00>] 0000000068f50078):
> CPU: 2 PID: 1 Comm: init VIP: 00 Tainted: G   W  O   5.10.43 #1
> Call trace:
>  __cfi_slowpath+0x300/0x3bc
>  rndis_signal_disconnect+0x1e0/0x204
>  rndis_close+0x24/0x2c
>  eth_stop+0xd0/0x234           (if dev->port_usb != NULL, call rndis_close)
>  __dev_close_many+0x204/0x2d4
>  dev_close_many+0x48/0x2c8
>  rollback_registered_many+0x184/0xdac
>  unregister_netdevice_queue+0xf8/0x24c
>  rndis_free_inst+0x78/0xc8
>  rndis_attr_release+0x3c/0x84
>  config_item_release+0x6c/0x180
>  configfs_rmdir+0x7e0/0xca0
> 
> Since the rndis memory has been freed in step2, function rndis_close cannot be called here. In function eth_stop(), if pointer dev->port_usb is NULL, function rndis_close() will not be called. So, if
> gadget->ops->pullup() return an error in step1, a CFI failure will be
> caused here.
> ======================================================
> Through above analysis, i think gadget->udc->driver->disconnect() need to be called even if gadget->udc->driver->disconnect() return an error.


I really do not understand, sorry.  What does CFI have to do with
anything here?  What functions are mis-matched that CFI trips over it?

And by reverting this change, don't you now have the original problem
that this commit was trying to solve?

> 
> Signed-off-by: Jiantao Zhang <water.zhangjiantao@huawei.com>
> Signed-off-by: TaoXue <xuetao09@huawei.com>

No Fixes: tag?

No cc: stable?

thanks,

greg k-h

Re: [PATCH] USB: gadget: Fix CFI failure during usb config switch.

[gregkh]

On Tue, Nov 08, 2022 at 08:39:52PM +0800, Zhangjiantao (Kirin, nanjing) wrote:

<snip>

Please fix your email client to not send HTML email, otherwise the
mailing list rejects it.

Fix that up and respond and I will be glad to reply.

thanks,

greg k-h

Re: [PATCH] USB: gadget: Fix CFI failure during usb config switch.

[Zhangjiantao (Kirin, nanjing)]

Thanks,

           I will fix my email client and rewrite a new patch.

在 2022/11/8 23:11, gregkh@linuxfoundation.org 写道:
> On Tue, Nov 08, 2022 at 08:39:52PM +0800, Zhangjiantao (Kirin, nanjing) wrote:
>
> <snip>
>
> Please fix your email client to not send HTML email, otherwise the
> mailing list rejects it.
>
> Fix that up and respond and I will be glad to reply.
>
> thanks,
>
> greg k-h
> .