[PATCH RFC] usb: legacy: ncm: Fix potential NPE in gncm_bind

Kuen-Han Tsai posted 1 patch 1 month, 2 weeks ago
drivers/usb/gadget/legacy/ncm.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
[PATCH RFC] usb: legacy: ncm: Fix potential NPE in gncm_bind
Posted by Kuen-Han Tsai 1 month, 2 weeks ago
Commit 56a512a9b410 ("usb: gadget: f_ncm: align net_device lifecycle
with bind/unbind") deferred the allocation of the net_device. This
change can lead to a NULL pointer dereference in the legacy NCM driver
as it attempts to access the net_device before it's fully initialized.

Store the provided qmult, host_addr, and dev_addr into the struct
ncm_opts->net_opts during gncm_bind(). These values will be properly
applied to the net_device when it is allocated and configured later in
the binding process by the NCM function driver.

Fixes: 56a512a9b410 ("usb: gadget: f_ncm: align net_device lifecycle with bind/unbind")
Cc: stable@kernel.org
Signed-off-by: Kuen-Han Tsai <khtsai@google.com>
---
Hi Greg,

I have been working on a series of changes to align the net_device
lifecycle with the bind/unbind process across various USB gadget 
function drivers. The goal is to solve the long-standing "lifetime
mismatch" problem where the network interface could outlive its parent
gadget device, leading to dangling symbolic links.

Recently, Commit 56a512a9b410 ("usb: gadget: f_ncm: align net_device
lifecycle with bind/unbind") was accepted to address this for the NCM
driver. However, during the process of adapting this same architecture
to f_subset, f_eem, f_ecm, and f_rndis, I discovered that this approach
regresses the legacy NCM driver (g_ncm).

Specifically, the legacy driver attempts to access the net_device during
its own binding process before the NCM function driver has been fully
bound. This can result in a NULL pointer dereference.

I am submitting the following patch as a fix for the g_ncm regression by
caching the configuration (qmult, host_addr, dev_addr) in opts_net until
the network device is ready. Please note that while I have verified the
logic and ensured the code builds, I do not have the physical hardware
required to perform a full functional test with the legacy g_ncm driver.

Beyond this specific fix, I would like to request the guidance on how to
proceed with the remaining network function drivers:

1. Deprecation: Can we consider the legacy drivers obsolete?  If so,
  we could prioritize the lifecycle fix for the configfs framework.

2. Compatibility: Should we continue to adapt the lifecycle concept to
  all drivers while strictly maintaining backward compatibility for 
  legacy function drivers?

I would appreciate your feedback on the preferred direction before I
proceed with the patchsets for the other USB function drivers.
---
 drivers/usb/gadget/legacy/ncm.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/gadget/legacy/ncm.c b/drivers/usb/gadget/legacy/ncm.c
index 0f1b45e3abd1a1ead7b2776be10a2a5747960136..bf00f9d76a5323b298869b4210c5bf99b1ab7f9c 100644
--- a/drivers/usb/gadget/legacy/ncm.c
+++ b/drivers/usb/gadget/legacy/ncm.c
@@ -15,8 +15,10 @@
 /* #define DEBUG */
 /* #define VERBOSE_DEBUG */
 
+#include <linux/hex.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/string.h>
 #include <linux/usb/composite.h>
 
 #include "u_ether.h"
@@ -129,6 +131,7 @@ static int gncm_bind(struct usb_composite_dev *cdev)
 	struct usb_gadget	*gadget = cdev->gadget;
 	struct f_ncm_opts	*ncm_opts;
 	int			status;
+	u8			mac[ETH_ALEN];
 
 	f_ncm_inst = usb_get_function_instance("ncm");
 	if (IS_ERR(f_ncm_inst))
@@ -136,11 +139,15 @@ static int gncm_bind(struct usb_composite_dev *cdev)
 
 	ncm_opts = container_of(f_ncm_inst, struct f_ncm_opts, func_inst);
 
-	gether_set_qmult(ncm_opts->net, qmult);
-	if (!gether_set_host_addr(ncm_opts->net, host_addr))
+	ncm_opts->net_opts.qmult = qmult;
+	if (mac_pton(host_addr, mac)) {
+		memcpy(&ncm_opts->net_opts.host_mac, mac, ETH_ALEN);
 		pr_info("using host ethernet address: %s", host_addr);
-	if (!gether_set_dev_addr(ncm_opts->net, dev_addr))
+	}
+	if (mac_pton(dev_addr, mac)) {
+		memcpy(&ncm_opts->net_opts.dev_mac, mac, ETH_ALEN);
 		pr_info("using self ethernet address: %s", dev_addr);
+	}
 
 	/* Allocate string descriptor numbers ... note that string
 	 * contents can be overridden by the composite_dev glue.

---
base-commit: da87d45b195148d670ab995367d52aa9e8a9a1fa
change-id: 20260214-legacy-ncm-8c001295b343

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