[PATCH 0/2] usb: gadget: u_ether: Fix NULL pointer dereferences after gadget unbind

[PATCH 0/2] usb: gadget: u_ether: Fix NULL pointer dereferences after gadget unbind

[Kuen-Han Tsai]

Hi all,

This series fixes two NULL pointer dereference issues in u_ether after
the gadget is unbound and dev->gadget is cleared by
gether_detach_gadget():

- Patch 1 replaces the composite.h DBG()/VDBG()/ERROR()/INFO() macros in
  u_ether.c with netdev_*() helpers so debug logging does not 
  dereference dev->gadget when the net_device is detached from the
  gadget.
- Patch 2 extends dev->lock to serialize dev->gadget accesses across
  eth_get_drvinfo(), rx_submit(), gether_set_gadget(), and
  gether_detach_gadget(), closing a check-then-use race in
  eth_get_drvinfo().

Signed-off-by: Kuen-Han Tsai <khtsai@google.com>
---
Kuen-Han Tsai (2):
      usb: gadget: u_ether: Fix NULL pointer deref in debug logging
      usb: gadget: u_ether: Protect dev->gadget access with dev->lock

 drivers/usb/gadget/function/u_ether.c | 84 ++++++++++++++++++-----------------
 1 file changed, 44 insertions(+), 40 deletions(-)
---
base-commit: abc36cbda29d8f19cf3a580cd86ca9e865186a41
change-id: 20260923-u-ether-gadget-npe-2fddafb37f23

Best regards,
-- 
Kuen-Han Tsai <khtsai@google.com>

[PATCH 1/2] usb: gadget: u_ether: Fix NULL pointer deref in debug logging

[Kuen-Han Tsai]

Commit ec35c1969650 ("usb: gadget: f_ncm: Fix net_device lifecycle with
device_move") and its counterparts reparent the net_device to
/sys/devices/virtual during unbind and clear dev->gadget. However, the
DBG(), VDBG(), ERROR(), and INFO() macros from <linux/usb/composite.h>
dereference &dev->gadget->dev.

When dynamic debug or CONFIG_USB_GADGET_DEBUG is enabled, any logging on
the surviving net_device after unbind causes a NULL pointer dereference,
such as in eth_stop() during function instance teardown:

  Unable to handle kernel NULL pointer dereference
  Call trace:
   dev_driver_string from __dynamic_dev_dbg+0x8c/0x118
   __dynamic_dev_dbg from eth_stop+0x70/0x134 [u_ether]
   ...
   unregister_netdev from gether_cleanup+0x14/0x28 [u_ether]
   gether_cleanup [u_ether] from rndis_free_inst+0x2c/0x48 [usb_f_rndis]

Replace the composite.h logging macros in u_ether.c with the standard
netdev_*() helpers. Because dev->net remains valid for the entire
lifetime of struct eth_dev and netdev_printk() natively handles
unparented network devices, messages are logged safely both when
attached and when detached from the gadget.

Reported-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
Closes: https://lore.kernel.org/all/89e19e6e-7ee7-4bb0-abd6-60971b7fd601@gmail.com/
Fixes: ec35c1969650 ("usb: gadget: f_ncm: Fix net_device lifecycle with device_move")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Kuen-Han Tsai <khtsai@google.com>
---
 drivers/usb/gadget/function/u_ether.c | 61 +++++++++++++++++------------------
 1 file changed, 30 insertions(+), 31 deletions(-)

diff --git a/drivers/usb/gadget/function/u_ether.c b/drivers/usb/gadget/function/u_ether.c
index 59d85d6a84a8..043b4ec80808 100644
--- a/drivers/usb/gadget/function/u_ether.c
+++ b/drivers/usb/gadget/function/u_ether.c
@@ -135,9 +135,9 @@ static void defer_kevent(struct eth_dev *dev, int flag)
 	if (test_and_set_bit(flag, &dev->todo))
 		return;
 	if (!schedule_work(&dev->work))
-		ERROR(dev, "kevent %d may have been dropped\n", flag);
+		netdev_err(dev->net, "kevent %d may have been dropped\n", flag);
 	else
-		DBG(dev, "kevent %d scheduled\n", flag);
+		netdev_dbg(dev->net, "kevent %d scheduled\n", flag);
 }
 
 static void rx_complete(struct usb_ep *ep, struct usb_request *req);
@@ -190,7 +190,7 @@ rx_submit(struct eth_dev *dev, struct usb_request *req, gfp_t gfp_flags)
 
 	skb = __netdev_alloc_skb(dev->net, size + NET_IP_ALIGN, gfp_flags);
 	if (skb == NULL) {
-		DBG(dev, "no rx skb\n");
+		netdev_dbg(dev->net, "no rx skb\n");
 		goto enomem;
 	}
 
@@ -211,7 +211,7 @@ rx_submit(struct eth_dev *dev, struct usb_request *req, gfp_t gfp_flags)
 enomem:
 		defer_kevent(dev, WORK_RX_MEMORY);
 	if (retval) {
-		DBG(dev, "rx submit --> %d\n", retval);
+		netdev_dbg(dev->net, "rx submit --> %d\n", retval);
 		if (skb)
 			dev_kfree_skb_any(skb);
 		spin_lock_irqsave(&dev->req_lock, flags);
@@ -258,7 +258,7 @@ static void rx_complete(struct usb_ep *ep, struct usb_request *req)
 					|| skb2->len > GETHER_MAX_ETH_FRAME_LEN) {
 				dev->net->stats.rx_errors++;
 				dev->net->stats.rx_length_errors++;
-				DBG(dev, "rx length %d\n", skb2->len);
+				netdev_dbg(dev->net, "rx length %d\n", skb2->len);
 				dev_kfree_skb_any(skb2);
 				goto next_frame;
 			}
@@ -278,12 +278,12 @@ static void rx_complete(struct usb_ep *ep, struct usb_request *req)
 	/* software-driven interface shutdown */
 	case -ECONNRESET:		/* unlink */
 	case -ESHUTDOWN:		/* disconnect etc */
-		VDBG(dev, "rx shutdown, code %d\n", status);
+		netdev_vdbg(dev->net, "rx shutdown, code %d\n", status);
 		goto quiesce;
 
 	/* for hardware automagic (such as pxa) */
 	case -ECONNABORTED:		/* endpoint reset */
-		DBG(dev, "rx %s reset\n", ep->name);
+		netdev_dbg(dev->net, "rx %s reset\n", ep->name);
 		defer_kevent(dev, WORK_RX_MEMORY);
 quiesce:
 		dev_kfree_skb_any(skb);
@@ -296,7 +296,7 @@ static void rx_complete(struct usb_ep *ep, struct usb_request *req)
 
 	default:
 		dev->net->stats.rx_errors++;
-		DBG(dev, "rx status %d\n", status);
+		netdev_dbg(dev->net, "rx status %d\n", status);
 		break;
 	}
 
@@ -365,7 +365,7 @@ static int alloc_requests(struct eth_dev *dev, struct gether *link, unsigned n)
 		goto fail;
 	goto done;
 fail:
-	DBG(dev, "can't alloc requests\n");
+	netdev_dbg(dev->net, "can't alloc requests\n");
 done:
 	spin_unlock(&dev->req_lock);
 	return status;
@@ -403,7 +403,7 @@ static void eth_work(struct work_struct *work)
 	}
 
 	if (dev->todo)
-		DBG(dev, "work done, flags = 0x%lx\n", dev->todo);
+		netdev_dbg(dev->net, "work done, flags = 0x%lx\n", dev->todo);
 }
 
 static void tx_complete(struct usb_ep *ep, struct usb_request *req)
@@ -414,7 +414,7 @@ static void tx_complete(struct usb_ep *ep, struct usb_request *req)
 	switch (req->status) {
 	default:
 		dev->net->stats.tx_errors++;
-		VDBG(dev, "tx err %d\n", req->status);
+		netdev_vdbg(dev->net, "tx err %d\n", req->status);
 		fallthrough;
 	case -ECONNRESET:		/* unlink */
 	case -ESHUTDOWN:		/* disconnect etc */
@@ -475,7 +475,7 @@ static netdev_tx_t eth_start_xmit(struct sk_buff *skb,
 	}
 
 	if (dev->port_usb && dev->port_usb->is_suspend) {
-		DBG(dev, "Port suspended. Triggering wakeup\n");
+		netdev_dbg(dev->net, "Port suspended. Triggering wakeup\n");
 		netif_stop_queue(net);
 		spin_unlock_irqrestore(&dev->lock, flags);
 		ether_wakeup_host(dev->port_usb);
@@ -579,7 +579,7 @@ static netdev_tx_t eth_start_xmit(struct sk_buff *skb,
 	retval = usb_ep_queue(in, req, GFP_ATOMIC);
 	switch (retval) {
 	default:
-		DBG(dev, "tx queue err %d\n", retval);
+		netdev_dbg(dev->net, "tx queue err %d\n", retval);
 		break;
 	case 0:
 		netif_trans_update(net);
@@ -604,7 +604,7 @@ static netdev_tx_t eth_start_xmit(struct sk_buff *skb,
 
 static void eth_start(struct eth_dev *dev, gfp_t gfp_flags)
 {
-	DBG(dev, "%s\n", __func__);
+	netdev_dbg(dev->net, "%s\n", __func__);
 
 	/* fill the rx queue */
 	rx_fill(dev, gfp_flags);
@@ -619,7 +619,7 @@ static int eth_open(struct net_device *net)
 	struct eth_dev	*dev = netdev_priv(net);
 	struct gether	*link;
 
-	DBG(dev, "%s\n", __func__);
+	netdev_dbg(dev->net, "%s\n", __func__);
 	if (netif_carrier_ok(dev->net))
 		eth_start(dev, GFP_KERNEL);
 
@@ -637,13 +637,12 @@ static int eth_stop(struct net_device *net)
 	struct eth_dev	*dev = netdev_priv(net);
 	unsigned long	flags;
 
-	VDBG(dev, "%s\n", __func__);
+	netdev_vdbg(dev->net, "%s\n", __func__);
 	netif_stop_queue(net);
 
-	DBG(dev, "stop stats: rx/tx %ld/%ld, errs %ld/%ld\n",
-		dev->net->stats.rx_packets, dev->net->stats.tx_packets,
-		dev->net->stats.rx_errors, dev->net->stats.tx_errors
-		);
+	netdev_dbg(dev->net, "stop stats: rx/tx %ld/%ld, errs %ld/%ld\n",
+		   dev->net->stats.rx_packets, dev->net->stats.tx_packets,
+		   dev->net->stats.rx_errors, dev->net->stats.tx_errors);
 
 	/* ensure there are no more active requests */
 	spin_lock_irqsave(&dev->lock, flags);
@@ -669,7 +668,7 @@ static int eth_stop(struct net_device *net)
 		usb_ep_disable(link->in_ep);
 		usb_ep_disable(link->out_ep);
 		if (netif_carrier_ok(net)) {
-			DBG(dev, "host still using in/out endpoints\n");
+			netdev_dbg(dev->net, "host still using in/out endpoints\n");
 			link->in_ep->desc = in;
 			link->out_ep->desc = out;
 			usb_ep_enable(link->in_ep);
@@ -799,8 +798,8 @@ struct eth_dev *gether_setup_name(struct usb_gadget *g,
 		free_netdev(net);
 		dev = ERR_PTR(status);
 	} else {
-		INFO(dev, "MAC %pM\n", net->dev_addr);
-		INFO(dev, "HOST MAC %pM\n", dev->host_mac);
+		netdev_info(net, "MAC %pM\n", net->dev_addr);
+		netdev_info(net, "HOST MAC %pM\n", dev->host_mac);
 
 		/*
 		 * two kinds of host-initiated state changes:
@@ -875,8 +874,8 @@ int gether_register_netdev(struct net_device *net)
 		dev_dbg(&g->dev, "register_netdev failed, %d\n", status);
 		return status;
 	} else {
-		INFO(dev, "HOST MAC %pM\n", dev->host_mac);
-		INFO(dev, "MAC %pM\n", dev->dev_mac);
+		netdev_info(net, "HOST MAC %pM\n", dev->host_mac);
+		netdev_info(net, "MAC %pM\n", dev->dev_mac);
 
 		/* two kinds of host-initiated state changes:
 		 *  - iff DATA transfer is active, carrier is "on"
@@ -1147,16 +1146,16 @@ struct net_device *gether_connect(struct gether *link)
 	link->in_ep->driver_data = dev;
 	result = usb_ep_enable(link->in_ep);
 	if (result != 0) {
-		DBG(dev, "enable %s --> %d\n",
-			link->in_ep->name, result);
+		netdev_dbg(dev->net, "enable %s --> %d\n",
+			   link->in_ep->name, result);
 		goto fail0;
 	}
 
 	link->out_ep->driver_data = dev;
 	result = usb_ep_enable(link->out_ep);
 	if (result != 0) {
-		DBG(dev, "enable %s --> %d\n",
-			link->out_ep->name, result);
+		netdev_dbg(dev->net, "enable %s --> %d\n",
+			   link->out_ep->name, result);
 		goto fail1;
 	}
 
@@ -1167,7 +1166,7 @@ struct net_device *gether_connect(struct gether *link)
 	if (result == 0) {
 		dev->zlp = link->is_zlp_ok;
 		dev->no_skb_reserve = gadget_avoids_skb_reserve(dev->gadget);
-		DBG(dev, "qlen %d\n", qlen(dev->gadget, dev->qmult));
+		netdev_dbg(dev->net, "qlen %d\n", qlen(dev->gadget, dev->qmult));
 
 		dev->header_len = link->header_len;
 		dev->unwrap = link->unwrap;
@@ -1223,7 +1222,7 @@ void gether_disconnect(struct gether *link)
 	if (!dev)
 		return;
 
-	DBG(dev, "%s\n", __func__);
+	netdev_dbg(dev->net, "%s\n", __func__);
 
 	spin_lock(&dev->lock);
 	dev->port_usb = NULL;

-- 
2.55.0.1082.g2b9226bbc0-goog

[PATCH 2/2] usb: gadget: u_ether: Protect dev->gadget access with dev->lock

[Kuen-Han Tsai]

Commit e002e92e88e1 ("usb: gadget: u_ether: Fix NULL pointer deref in
eth_get_drvinfo") added a NULL check for dev->gadget in
eth_get_drvinfo(), but the check and subsequent dereferences are not
serialized against gether_detach_gadget(). If unbind clears dev->gadget
concurrently after the NULL check, eth_get_drvinfo() can still
dereference a NULL or freed gadget pointer.

Extend dev->lock to protect dev->gadget across gether_set_gadget(),
gether_detach_gadget(), eth_get_drvinfo(), and rx_submit(). Also use
gether_set_gadget() in gether_setup_name() for consistency.

Reported-by: Faqiang Zhu <faqiang.zhu@nxp.com>
Fixes: e002e92e88e1 ("usb: gadget: u_ether: Fix NULL pointer deref in eth_get_drvinfo")
Fixes: ec35c1969650 ("usb: gadget: f_ncm: Fix net_device lifecycle with device_move")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Kuen-Han Tsai <khtsai@google.com>
---
 drivers/usb/gadget/function/u_ether.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/gadget/function/u_ether.c b/drivers/usb/gadget/function/u_ether.c
index 043b4ec80808..2a9ae598e612 100644
--- a/drivers/usb/gadget/function/u_ether.c
+++ b/drivers/usb/gadget/function/u_ether.c
@@ -13,6 +13,7 @@
 #include <linux/module.h>
 #include <linux/gfp.h>
 #include <linux/device.h>
+#include <linux/cleanup.h>
 #include <linux/ctype.h>
 #include <linux/etherdevice.h>
 #include <linux/ethtool.h>
@@ -54,8 +55,7 @@
 #define GETHER_MAX_ETH_FRAME_LEN (GETHER_MAX_MTU_SIZE + ETH_HLEN)
 
 struct eth_dev {
-	/* lock is held while accessing port_usb
-	 */
+	/* lock is held while accessing port_usb and gadget */
 	spinlock_t		lock;
 	struct gether		*port_usb;
 
@@ -113,6 +113,8 @@ static void eth_get_drvinfo(struct net_device *net, struct ethtool_drvinfo *p)
 
 	strscpy(p->driver, "g_ether", sizeof(p->driver));
 	strscpy(p->version, UETH__VERSION, sizeof(p->version));
+
+	guard(spinlock_irqsave)(&dev->lock);
 	if (dev->gadget) {
 		strscpy(p->fw_version, dev->gadget->name, sizeof(p->fw_version));
 		strscpy(p->bus_info, dev_name(&dev->gadget->dev), sizeof(p->bus_info));
@@ -145,7 +147,7 @@ static void rx_complete(struct usb_ep *ep, struct usb_request *req);
 static int
 rx_submit(struct eth_dev *dev, struct usb_request *req, gfp_t gfp_flags)
 {
-	struct usb_gadget *g = dev->gadget;
+	struct usb_gadget *g;
 	struct sk_buff	*skb;
 	int		retval = -ENOMEM;
 	size_t		size = 0;
@@ -153,6 +155,7 @@ rx_submit(struct eth_dev *dev, struct usb_request *req, gfp_t gfp_flags)
 	unsigned long	flags;
 
 	spin_lock_irqsave(&dev->lock, flags);
+	g = dev->gadget;
 	if (dev->port_usb)
 		out = dev->port_usb->out_ep;
 	else
@@ -788,8 +791,7 @@ struct eth_dev *gether_setup_name(struct usb_gadget *g,
 	net->min_mtu = ETH_HLEN;
 	net->max_mtu = GETHER_MAX_MTU_SIZE;
 
-	dev->gadget = g;
-	SET_NETDEV_DEV(net, &g->dev);
+	gether_set_gadget(net, g);
 	SET_NETDEV_DEVTYPE(net, &gadget_type);
 
 	status = register_netdev(net);
@@ -890,10 +892,11 @@ EXPORT_SYMBOL_GPL(gether_register_netdev);
 
 void gether_set_gadget(struct net_device *net, struct usb_gadget *g)
 {
-	struct eth_dev *dev;
+	struct eth_dev *dev = netdev_priv(net);
+
+	scoped_guard(spinlock_irqsave, &dev->lock)
+		dev->gadget = g;
 
-	dev = netdev_priv(net);
-	dev->gadget = g;
 	SET_NETDEV_DEV(net, &g->dev);
 }
 EXPORT_SYMBOL_GPL(gether_set_gadget);
@@ -915,8 +918,10 @@ void gether_detach_gadget(struct net_device *net)
 {
 	struct eth_dev *dev = netdev_priv(net);
 
+	scoped_guard(spinlock_irqsave, &dev->lock)
+		dev->gadget = NULL;
+
 	device_move(&net->dev, NULL, DPM_ORDER_NONE);
-	dev->gadget = NULL;
 }
 EXPORT_SYMBOL_GPL(gether_detach_gadget);
 

-- 
2.55.0.1082.g2b9226bbc0-goog