Re: [PATCH] usb: gadget: legacy: ether: fix RNDIS for Windows hosts

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] usb: gadget: resource leak in composite_bind() when OS desc prep fails
--

commit 9ae1a82cebd8ee193e2a0cd2544effdf04549a00
Author: Amine Ayaou <amine-ayaou@hotmail.com>

usb: gadget: legacy: ether: fix RNDIS for Windows hosts

This commit configures Microsoft OS 1.0 descriptors in g_ether when the RNDIS
function is enabled so modern Windows hosts can bind the RNDIS driver to the
legacy gadget.

> diff --git a/drivers/usb/gadget/legacy/ether.c b/drivers/usb/gadget/legacy/ether.c
> index 99c7fc0d1d597..70de686fa56b7 100644
> --- a/drivers/usb/gadget/legacy/ether.c
> +++ b/drivers/usb/gadget/legacy/ether.c
[ ... ]
> @@ -387,6 +390,18 @@ static int eth_bind(struct usb_composite_dev *cdev)
>  		device_desc.idVendor = cpu_to_le16(RNDIS_VENDOR_NUM);
>  		device_desc.idProduct = cpu_to_le16(RNDIS_PRODUCT_NUM);
>  		device_desc.bNumConfigurations = 2;
> +
> +		/* Configure Microsoft OS 1.0 descriptors for RNDIS. */
> +		cdev->os_desc_config = &rndis_config_driver;
> +		cdev->use_os_string = true;

[Severity: Medium]
Can setting cdev->use_os_string in eth_bind() leave eth_unbind() skipped if
composite_os_desc_req_prepare() fails in composite_bind()?

In drivers/usb/gadget/composite.c:composite_bind(), composite->bind(cdev)
runs before the cdev->use_os_string check:

	status = composite->bind(cdev);
	if (status < 0)
		goto fail;

	if (cdev->use_os_string) {
		status = composite_os_desc_req_prepare(cdev, gadget->ep0);
		if (status)
			goto fail;
	}
	...
fail:
	__composite_unbind(gadget, false);
	return status;

When eth_bind() returns 0, it has already registered net via
gether_register_netdev(net) with bound set to true, allocated fi_rndis and
fi_eem/fi_ecm/fi_geth, allocated f_rndis and f_eem/f_ecm/f_geth via
usb_add_config(), and optionally allocated otg_desc[0].

If composite_os_desc_req_prepare() then fails with -ENOMEM, composite_bind()
calls __composite_unbind(gadget, false). In __composite_unbind():

	while (!list_empty(&cdev->configs)) {
		struct usb_configuration	*c;
		c = list_first_entry(&cdev->configs,
				struct usb_configuration, list);
		remove_config(cdev, c);
	}
	if (cdev->driver->unbind && unbind_driver)
		cdev->driver->unbind(cdev);

Because unbind_driver is false, eth_unbind() is not called. While
remove_config() unbinds the functions from each configuration, the function
unbind callbacks do not unregister or free net when bound and borrowed_net
are true. Without eth_unbind() running:

static int eth_unbind(struct usb_composite_dev *cdev)
{
	if (has_rndis()) {
		usb_put_function(f_rndis);
		usb_put_function_instance(fi_rndis);
	}
	if (use_eem) {
		usb_put_function(f_eem);
		usb_put_function_instance(fi_eem);
	} else if (can_support_ecm(cdev->gadget)) {
		usb_put_function(f_ecm);
		usb_put_function_instance(fi_ecm);
	} else {
		usb_put_function(f_geth);
		usb_put_function_instance(fi_geth);
	}
	kfree(otg_desc[0]);
	otg_desc[0] = NULL;

	return 0;
}

wouldn't this leak f_rndis, fi_rndis, f_eem/f_ecm/f_geth,
fi_eem/fi_ecm/fi_geth, and otg_desc[0], as well as leave net registered
after cdev is freed?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/MRVP191MB2786AD66C3EAB9F32E5B84F9EF6D2@MRVP191MB2786.EURP191.PROD.OUTLOOK.COM?part=1