[PATCH 3/3] usb: gadget: f_ncm: align net_device lifecycle with bind/unbind

Kuen-Han Tsai posted 3 patches 1 month, 1 week ago
[PATCH 3/3] usb: gadget: f_ncm: align net_device lifecycle with bind/unbind
Posted by Kuen-Han Tsai 1 month, 1 week ago
Currently, the net_device is allocated in ncm_alloc_inst() and freed in
ncm_free_inst(). This ties the network interface's lifetime to the
configuration instance rather than the USB connection (bind/unbind).

This decoupling causes issues when the USB gadget is disconnected where
the underlying gadget device is removed. The net_device can outlive its
parent, leading to dangling sysfs links and NULL pointer dereferences
when accessing the freed gadget device.

Problem 1: NULL pointer dereference on disconnect
 Unable to handle kernel NULL pointer dereference at virtual address
 0000000000000000
 Call trace:
   __pi_strlen+0x14/0x150
   rtnl_fill_ifinfo+0x6b4/0x708
   rtmsg_ifinfo_build_skb+0xd8/0x13c
   rtmsg_ifinfo+0x50/0xa0
   __dev_notify_flags+0x4c/0x1f0
   dev_change_flags+0x54/0x70
   do_setlink+0x390/0xebc
   rtnl_newlink+0x7d0/0xac8
   rtnetlink_rcv_msg+0x27c/0x410
   netlink_rcv_skb+0x134/0x150
   rtnetlink_rcv+0x18/0x28
   netlink_unicast+0x254/0x3f0
   netlink_sendmsg+0x2e0/0x3d4

Problem 2: Dangling sysfs symlinks
 console:/ # ls -l /sys/class/net/ncm0
 lrwxrwxrwx ... /sys/class/net/ncm0 ->
 /sys/devices/platform/.../gadget.0/net/ncm0
 console:/ # ls -l /sys/devices/platform/.../gadget.0/net/ncm0
 ls: .../gadget.0/net/ncm0: No such file or directory

Move the net_device allocation to ncm_bind() and deallocation to
ncm_unbind(). This ensures the network interface exists only when the
gadget function is actually bound to a configuration.

To support pre-bind configuration (e.g., setting interface name or MAC
address via configfs), cache user-provided options in f_ncm_opts
using the gether_opts structure. Apply these cached settings to the
net_device upon creation in ncm_bind().

Preserve the use-after-free fix from commit 6334b8e4553c ("usb: gadget:
f_ncm: Fix UAF ncm object at re-bind after usb ep transport error").
Check opts->net in ncm_set_alt() and ncm_disable() to ensure
gether_disconnect() runs only if a connection was established.

Fixes: 40d133d7f542 ("usb: gadget: f_ncm: convert to new function interface with backward compatibility")
Cc: stable@kernel.org
Signed-off-by: Kuen-Han Tsai <khtsai@google.com>
---
 drivers/usb/gadget/function/f_ncm.c | 128 ++++++++++++++++++------------------
 drivers/usb/gadget/function/u_ncm.h |   4 +-
 2 files changed, 66 insertions(+), 66 deletions(-)

diff --git a/drivers/usb/gadget/function/f_ncm.c b/drivers/usb/gadget/function/f_ncm.c
index 0e38330271d5ac40f7f46b7e8d565c1f0d41e4b8..e23adc132f8865f6bbce6c88c8b5f3f06110faaa 100644
--- a/drivers/usb/gadget/function/f_ncm.c
+++ b/drivers/usb/gadget/function/f_ncm.c
@@ -83,6 +83,11 @@ static inline struct f_ncm *func_to_ncm(struct usb_function *f)
 	return container_of(f, struct f_ncm, port.func);
 }
 
+static inline struct f_ncm_opts *func_to_ncm_opts(struct usb_function *f)
+{
+	return container_of(f->fi, struct f_ncm_opts, func_inst);
+}
+
 /*-------------------------------------------------------------------------*/
 
 /*
@@ -859,6 +864,7 @@ static int ncm_setup(struct usb_function *f, const struct usb_ctrlrequest *ctrl)
 static int ncm_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
 {
 	struct f_ncm		*ncm = func_to_ncm(f);
+	struct f_ncm_opts	*opts = func_to_ncm_opts(f);
 	struct usb_composite_dev *cdev = f->config->cdev;
 
 	/* Control interface has only altsetting 0 */
@@ -881,12 +887,13 @@ static int ncm_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
 		if (alt > 1)
 			goto fail;
 
-		if (ncm->netdev) {
-			DBG(cdev, "reset ncm\n");
-			ncm->netdev = NULL;
-			gether_disconnect(&ncm->port);
-			ncm_reset_values(ncm);
-		}
+		scoped_guard(mutex, &opts->lock)
+			if (opts->net) {
+				DBG(cdev, "reset ncm\n");
+				opts->net = NULL;
+				gether_disconnect(&ncm->port);
+				ncm_reset_values(ncm);
+			}
 
 		/*
 		 * CDC Network only sends data in non-default altsettings.
@@ -919,7 +926,8 @@ static int ncm_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
 			net = gether_connect(&ncm->port);
 			if (IS_ERR(net))
 				return PTR_ERR(net);
-			ncm->netdev = net;
+			scoped_guard(mutex, &opts->lock)
+				opts->net = net;
 		}
 
 		spin_lock(&ncm->lock);
@@ -1366,14 +1374,16 @@ static int ncm_unwrap_ntb(struct gether *port,
 static void ncm_disable(struct usb_function *f)
 {
 	struct f_ncm		*ncm = func_to_ncm(f);
+	struct f_ncm_opts	*opts = func_to_ncm_opts(f);
 	struct usb_composite_dev *cdev = f->config->cdev;
 
 	DBG(cdev, "ncm deactivated\n");
 
-	if (ncm->netdev) {
-		ncm->netdev = NULL;
-		gether_disconnect(&ncm->port);
-	}
+	scoped_guard(mutex, &opts->lock)
+		if (opts->net) {
+			opts->net = NULL;
+			gether_disconnect(&ncm->port);
+		}
 
 	if (ncm->notify->enabled) {
 		usb_ep_disable(ncm->notify);
@@ -1433,39 +1443,44 @@ static int ncm_bind(struct usb_configuration *c, struct usb_function *f)
 {
 	struct usb_composite_dev *cdev = c->cdev;
 	struct f_ncm		*ncm = func_to_ncm(f);
+	struct f_ncm_opts	*ncm_opts = func_to_ncm_opts(f);
 	struct usb_string	*us;
 	int			status = 0;
 	struct usb_ep		*ep;
-	struct f_ncm_opts	*ncm_opts;
 
 	struct usb_os_desc_table	*os_desc_table __free(kfree) = NULL;
+	struct net_device		*netdev __free(free_gether_netdev) = NULL;
 	struct usb_request		*request __free(free_usb_request) = NULL;
 
 	if (!can_support_ecm(cdev->gadget))
 		return -EINVAL;
 
-	ncm_opts = container_of(f->fi, struct f_ncm_opts, func_inst);
-
 	if (cdev->use_os_string) {
 		os_desc_table = kzalloc(sizeof(*os_desc_table), GFP_KERNEL);
 		if (!os_desc_table)
 			return -ENOMEM;
 	}
 
-	mutex_lock(&ncm_opts->lock);
-	gether_set_gadget(ncm_opts->net, cdev->gadget);
-	if (!ncm_opts->bound) {
-		ncm_opts->net->mtu = (ncm_opts->max_segment_size - ETH_HLEN);
-		status = gether_register_netdev(ncm_opts->net);
+	netdev = gether_setup_default();
+	if (IS_ERR(netdev))
+		return -ENOMEM;
+
+	scoped_guard(mutex, &ncm_opts->lock) {
+		gether_apply_opts(netdev, &ncm_opts->net_opts);
+		netdev->mtu = ncm_opts->max_segment_size - ETH_HLEN;
 	}
-	mutex_unlock(&ncm_opts->lock);
 
+	gether_set_gadget(netdev, cdev->gadget);
+	status = gether_register_netdev(netdev);
 	if (status)
 		return status;
 
-	ncm_opts->bound = true;
-
-	ncm_string_defs[1].s = ncm->ethaddr;
+	/* export host's Ethernet address in CDC format */
+	status = gether_get_host_addr_cdc(netdev, ncm->ethaddr,
+					  sizeof(ncm->ethaddr));
+	if (status < 12)
+		return -EINVAL;
+	ncm_string_defs[STRING_MAC_IDX].s = ncm->ethaddr;
 
 	us = usb_gstrings_attach(cdev, ncm_strings,
 				 ARRAY_SIZE(ncm_string_defs));
@@ -1563,6 +1578,8 @@ static int ncm_bind(struct usb_configuration *c, struct usb_function *f)
 		f->os_desc_n = 1;
 	}
 	ncm->notify_req = no_free_ptr(request);
+	ncm->netdev = no_free_ptr(netdev);
+	ncm->port.ioport = netdev_priv(ncm->netdev);
 
 	DBG(cdev, "CDC Network: IN/%s OUT/%s NOTIFY/%s\n",
 			ncm->port.in_ep->name, ncm->port.out_ep->name,
@@ -1577,19 +1594,19 @@ static inline struct f_ncm_opts *to_f_ncm_opts(struct config_item *item)
 }
 
 /* f_ncm_item_ops */
-USB_ETHERNET_CONFIGFS_ITEM(ncm);
+USB_ETHER_OPTS_ITEM(ncm);
 
 /* f_ncm_opts_dev_addr */
-USB_ETHERNET_CONFIGFS_ITEM_ATTR_DEV_ADDR(ncm);
+USB_ETHER_OPTS_ATTR_DEV_ADDR(ncm);
 
 /* f_ncm_opts_host_addr */
-USB_ETHERNET_CONFIGFS_ITEM_ATTR_HOST_ADDR(ncm);
+USB_ETHER_OPTS_ATTR_HOST_ADDR(ncm);
 
 /* f_ncm_opts_qmult */
-USB_ETHERNET_CONFIGFS_ITEM_ATTR_QMULT(ncm);
+USB_ETHER_OPTS_ATTR_QMULT(ncm);
 
 /* f_ncm_opts_ifname */
-USB_ETHERNET_CONFIGFS_ITEM_ATTR_IFNAME(ncm);
+USB_ETHER_OPTS_ATTR_IFNAME(ncm);
 
 static ssize_t ncm_opts_max_segment_size_show(struct config_item *item,
 					      char *page)
@@ -1655,34 +1672,27 @@ static void ncm_free_inst(struct usb_function_instance *f)
 	struct f_ncm_opts *opts;
 
 	opts = container_of(f, struct f_ncm_opts, func_inst);
-	if (opts->bound)
-		gether_cleanup(netdev_priv(opts->net));
-	else
-		free_netdev(opts->net);
 	kfree(opts->ncm_interf_group);
 	kfree(opts);
 }
 
 static struct usb_function_instance *ncm_alloc_inst(void)
 {
-	struct f_ncm_opts *opts;
+	struct usb_function_instance *ret;
 	struct usb_os_desc *descs[1];
 	char *names[1];
 	struct config_group *ncm_interf_group;
 
-	opts = kzalloc(sizeof(*opts), GFP_KERNEL);
+	struct f_ncm_opts *opts __free(kfree) = kzalloc(sizeof(*opts), GFP_KERNEL);
 	if (!opts)
 		return ERR_PTR(-ENOMEM);
+
+	opts->net = NULL;
 	opts->ncm_os_desc.ext_compat_id = opts->ncm_ext_compat_id;
+	gether_setup_opts_default(&opts->net_opts, "usb");
 
 	mutex_init(&opts->lock);
 	opts->func_inst.free_func_inst = ncm_free_inst;
-	opts->net = gether_setup_default();
-	if (IS_ERR(opts->net)) {
-		struct net_device *net = opts->net;
-		kfree(opts);
-		return ERR_CAST(net);
-	}
 	opts->max_segment_size = ETH_FRAME_LEN;
 	INIT_LIST_HEAD(&opts->ncm_os_desc.ext_prop);
 
@@ -1693,26 +1703,22 @@ static struct usb_function_instance *ncm_alloc_inst(void)
 	ncm_interf_group =
 		usb_os_desc_prepare_interf_dir(&opts->func_inst.group, 1, descs,
 					       names, THIS_MODULE);
-	if (IS_ERR(ncm_interf_group)) {
-		ncm_free_inst(&opts->func_inst);
+	if (IS_ERR(ncm_interf_group))
 		return ERR_CAST(ncm_interf_group);
-	}
 	opts->ncm_interf_group = ncm_interf_group;
 
-	return &opts->func_inst;
+	ret = &opts->func_inst;
+	retain_and_null_ptr(opts);
+	return ret;
 }
 
 static void ncm_free(struct usb_function *f)
 {
-	struct f_ncm *ncm;
-	struct f_ncm_opts *opts;
+	struct f_ncm_opts *opts = func_to_ncm_opts(f);
 
-	ncm = func_to_ncm(f);
-	opts = container_of(f->fi, struct f_ncm_opts, func_inst);
-	kfree(ncm);
-	mutex_lock(&opts->lock);
-	opts->refcnt--;
-	mutex_unlock(&opts->lock);
+	scoped_guard(mutex, &opts->lock)
+		opts->refcnt--;
+	kfree(func_to_ncm(f));
 }
 
 static void ncm_unbind(struct usb_configuration *c, struct usb_function *f)
@@ -1736,13 +1742,15 @@ static void ncm_unbind(struct usb_configuration *c, struct usb_function *f)
 
 	kfree(ncm->notify_req->buf);
 	usb_ep_free_request(ncm->notify, ncm->notify_req);
+
+	ncm->port.ioport = NULL;
+	gether_cleanup(netdev_priv(ncm->netdev));
 }
 
 static struct usb_function *ncm_alloc(struct usb_function_instance *fi)
 {
 	struct f_ncm		*ncm;
 	struct f_ncm_opts	*opts;
-	int status;
 
 	/* allocate and initialize one new instance */
 	ncm = kzalloc(sizeof(*ncm), GFP_KERNEL);
@@ -1750,22 +1758,12 @@ static struct usb_function *ncm_alloc(struct usb_function_instance *fi)
 		return ERR_PTR(-ENOMEM);
 
 	opts = container_of(fi, struct f_ncm_opts, func_inst);
-	mutex_lock(&opts->lock);
-	opts->refcnt++;
 
-	/* export host's Ethernet address in CDC format */
-	status = gether_get_host_addr_cdc(opts->net, ncm->ethaddr,
-				      sizeof(ncm->ethaddr));
-	if (status < 12) { /* strlen("01234567890a") */
-		kfree(ncm);
-		mutex_unlock(&opts->lock);
-		return ERR_PTR(-EINVAL);
-	}
+	scoped_guard(mutex, &opts->lock)
+		opts->refcnt++;
 
 	spin_lock_init(&ncm->lock);
 	ncm_reset_values(ncm);
-	ncm->port.ioport = netdev_priv(opts->net);
-	mutex_unlock(&opts->lock);
 	ncm->port.is_fixed = true;
 	ncm->port.supports_multi_frame = true;
 
diff --git a/drivers/usb/gadget/function/u_ncm.h b/drivers/usb/gadget/function/u_ncm.h
index 49ec095cdb4b6dcb330fd3149b502840312f78ec..d99330fe31e880f636615774d212062952c31e43 100644
--- a/drivers/usb/gadget/function/u_ncm.h
+++ b/drivers/usb/gadget/function/u_ncm.h
@@ -15,11 +15,13 @@
 
 #include <linux/usb/composite.h>
 
+#include "u_ether.h"
+
 struct f_ncm_opts {
 	struct usb_function_instance	func_inst;
 	struct net_device		*net;
-	bool				bound;
 
+	struct gether_opts		net_opts;
 	struct config_group		*ncm_interf_group;
 	struct usb_os_desc		ncm_os_desc;
 	char				ncm_ext_compat_id[16];

-- 
2.52.0.351.gbe84eed79e-goog
Re: [PATCH 3/3] usb: gadget: f_ncm: align net_device lifecycle with bind/unbind
Posted by Ernest Van Hoecke 1 month ago
On Tue, Dec 30, 2025 at 06:13:16PM +0800, Kuen-Han Tsai wrote:
> Currently, the net_device is allocated in ncm_alloc_inst() and freed in
> ncm_free_inst(). This ties the network interface's lifetime to the
> configuration instance rather than the USB connection (bind/unbind).
> 
> This decoupling causes issues when the USB gadget is disconnected where
> the underlying gadget device is removed. The net_device can outlive its
> parent, leading to dangling sysfs links and NULL pointer dereferences
> when accessing the freed gadget device.
> 
> Problem 1: NULL pointer dereference on disconnect
>  Unable to handle kernel NULL pointer dereference at virtual address
>  0000000000000000
>  Call trace:
>    __pi_strlen+0x14/0x150
>    rtnl_fill_ifinfo+0x6b4/0x708
>    rtmsg_ifinfo_build_skb+0xd8/0x13c
>    rtmsg_ifinfo+0x50/0xa0
>    __dev_notify_flags+0x4c/0x1f0
>    dev_change_flags+0x54/0x70
>    do_setlink+0x390/0xebc
>    rtnl_newlink+0x7d0/0xac8
>    rtnetlink_rcv_msg+0x27c/0x410
>    netlink_rcv_skb+0x134/0x150
>    rtnetlink_rcv+0x18/0x28
>    netlink_unicast+0x254/0x3f0
>    netlink_sendmsg+0x2e0/0x3d4
> 
> Problem 2: Dangling sysfs symlinks
>  console:/ # ls -l /sys/class/net/ncm0
>  lrwxrwxrwx ... /sys/class/net/ncm0 ->
>  /sys/devices/platform/.../gadget.0/net/ncm0
>  console:/ # ls -l /sys/devices/platform/.../gadget.0/net/ncm0
>  ls: .../gadget.0/net/ncm0: No such file or directory
> 
> Move the net_device allocation to ncm_bind() and deallocation to
> ncm_unbind(). This ensures the network interface exists only when the
> gadget function is actually bound to a configuration.
> 
> To support pre-bind configuration (e.g., setting interface name or MAC
> address via configfs), cache user-provided options in f_ncm_opts
> using the gether_opts structure. Apply these cached settings to the
> net_device upon creation in ncm_bind().
> 
> Preserve the use-after-free fix from commit 6334b8e4553c ("usb: gadget:
> f_ncm: Fix UAF ncm object at re-bind after usb ep transport error").
> Check opts->net in ncm_set_alt() and ncm_disable() to ensure
> gether_disconnect() runs only if a connection was established.
> 
> Fixes: 40d133d7f542 ("usb: gadget: f_ncm: convert to new function interface with backward compatibility")
> Cc: stable@kernel.org
> Signed-off-by: Kuen-Han Tsai <khtsai@google.com>

Hi Kuen-Han,

Thank you for all your work on this.

When using the DWC3 IP for USB OTG on an iMX95 with our Aquila iMX95
SoM, USB NCM does not function properly when booting the board with this
USB in host mode.

Your patch series completely solves this issue, I was debugging it
before and saw that there were indeed issues with the relation between
the net device and the gadget.

Tested-by: Ernest Van Hoecke <ernest.vanhoecke@toradex.com> # Aquila iMX95

Kind regards,
Ernest
Re: [PATCH 3/3] usb: gadget: f_ncm: align net_device lifecycle with bind/unbind
Posted by Kuen-Han Tsai 1 month ago
On Fri, Jan 9, 2026 at 6:25 PM Ernest Van Hoecke
<ernestvanhoecke@gmail.com> wrote:
>
> On Tue, Dec 30, 2025 at 06:13:16PM +0800, Kuen-Han Tsai wrote:
> > Currently, the net_device is allocated in ncm_alloc_inst() and freed in
> > ncm_free_inst(). This ties the network interface's lifetime to the
> > configuration instance rather than the USB connection (bind/unbind).
> >
> > This decoupling causes issues when the USB gadget is disconnected where
> > the underlying gadget device is removed. The net_device can outlive its
> > parent, leading to dangling sysfs links and NULL pointer dereferences
> > when accessing the freed gadget device.
> >
> > Problem 1: NULL pointer dereference on disconnect
> >  Unable to handle kernel NULL pointer dereference at virtual address
> >  0000000000000000
> >  Call trace:
> >    __pi_strlen+0x14/0x150
> >    rtnl_fill_ifinfo+0x6b4/0x708
> >    rtmsg_ifinfo_build_skb+0xd8/0x13c
> >    rtmsg_ifinfo+0x50/0xa0
> >    __dev_notify_flags+0x4c/0x1f0
> >    dev_change_flags+0x54/0x70
> >    do_setlink+0x390/0xebc
> >    rtnl_newlink+0x7d0/0xac8
> >    rtnetlink_rcv_msg+0x27c/0x410
> >    netlink_rcv_skb+0x134/0x150
> >    rtnetlink_rcv+0x18/0x28
> >    netlink_unicast+0x254/0x3f0
> >    netlink_sendmsg+0x2e0/0x3d4
> >
> > Problem 2: Dangling sysfs symlinks
> >  console:/ # ls -l /sys/class/net/ncm0
> >  lrwxrwxrwx ... /sys/class/net/ncm0 ->
> >  /sys/devices/platform/.../gadget.0/net/ncm0
> >  console:/ # ls -l /sys/devices/platform/.../gadget.0/net/ncm0
> >  ls: .../gadget.0/net/ncm0: No such file or directory
> >
> > Move the net_device allocation to ncm_bind() and deallocation to
> > ncm_unbind(). This ensures the network interface exists only when the
> > gadget function is actually bound to a configuration.
> >
> > To support pre-bind configuration (e.g., setting interface name or MAC
> > address via configfs), cache user-provided options in f_ncm_opts
> > using the gether_opts structure. Apply these cached settings to the
> > net_device upon creation in ncm_bind().
> >
> > Preserve the use-after-free fix from commit 6334b8e4553c ("usb: gadget:
> > f_ncm: Fix UAF ncm object at re-bind after usb ep transport error").
> > Check opts->net in ncm_set_alt() and ncm_disable() to ensure
> > gether_disconnect() runs only if a connection was established.
> >
> > Fixes: 40d133d7f542 ("usb: gadget: f_ncm: convert to new function interface with backward compatibility")
> > Cc: stable@kernel.org
> > Signed-off-by: Kuen-Han Tsai <khtsai@google.com>
>
> Hi Kuen-Han,
>
> Thank you for all your work on this.
>
> When using the DWC3 IP for USB OTG on an iMX95 with our Aquila iMX95
> SoM, USB NCM does not function properly when booting the board with this
> USB in host mode.
>
> Your patch series completely solves this issue, I was debugging it
> before and saw that there were indeed issues with the relation between
> the net device and the gadget.
>
> Tested-by: Ernest Van Hoecke <ernest.vanhoecke@toradex.com> # Aquila iMX95
>
> Kind regards,
> Ernest

Hi Ernest,

Thank you for the testing and the confirmation on iMX95! I'm glad to
hear the fix is working well for you.

Regards,
Kuen-Han