[PATCH v2] usb: gadget: f_phonet: fix use-after-free in pn_bind

pn_bind() and phonet_free_inst() race on opts->bound and opts->net.
If configfs removes the function instance while pn_bind() is between
the !bound check and setting bound = true, free_inst() frees opts->net
and pn_bind() then writes to net->dev.parent via gphonet_set_gadget().

The existing "no race condition" comment was wrong: configfs_rmdir()
can run independently of the composite bind sequence.

Add a mutex to f_phonet_opts and use scoped_guard(mutex) in both
pn_bind() and phonet_free_inst() to serialize access to ->bound and
->net. Add a kernel-doc comment describing what the lock protects,
and destroy the mutex before freeing opts.

Fixes: 00a2430ff07d ("usb: gadget: Gadget directory cleanup - group usb functions")
Reported-by: [email protected]
Closes: https://syzkaller.appspot.com/bug?extid=098999e05b6b877c01b3
Signed-off-by: Nguyen Quang Le Kien <[email protected]>
---
v2:
 - use scoped_guard(mutex) instead of open-coded lock/unlock
 - add kernel-doc comment on struct f_phonet_opts describing what
   @lock protects
 - add explicit #include <linux/mutex.h>
 - call mutex_destroy() before kfree(opts)
 - remove stale "no race condition" comment; explain why it was wrong
   in the commit message
---
 drivers/usb/gadget/function/f_phonet.c | 34 +++++++++++++-------------
 drivers/usb/gadget/function/u_phonet.h | 10 ++++++++
 2 files changed, 27 insertions(+), 17 deletions(-)

diff --git a/drivers/usb/gadget/function/f_phonet.c b/drivers/usb/gadget/function/f_phonet.c
index b1ee9a7c2..350579747 100644
--- a/drivers/usb/gadget/function/f_phonet.c
+++ b/drivers/usb/gadget/function/f_phonet.c
@@ -12,6 +12,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/device.h>
+#include <linux/mutex.h>
 
 #include <linux/netdevice.h>
 #include <linux/if_ether.h>
@@ -499,19 +500,14 @@ static int pn_bind(struct usb_configuration *c, struct usb_function *f)
 
 	phonet_opts = container_of(f->fi, struct f_phonet_opts, func_inst);
 
-	/*
-	 * in drivers/usb/gadget/configfs.c:configfs_composite_bind()
-	 * configurations are bound in sequence with list_for_each_entry,
-	 * in each configuration its functions are bound in sequence
-	 * with list_for_each_entry, so we assume no race condition
-	 * with regard to phonet_opts->bound access
-	 */
-	if (!phonet_opts->bound) {
-		gphonet_set_gadget(phonet_opts->net, gadget);
-		status = gphonet_register_netdev(phonet_opts->net);
-		if (status)
-			return status;
-		phonet_opts->bound = true;
+	scoped_guard(mutex, &phonet_opts->lock) {
+		if (!phonet_opts->bound) {
+			gphonet_set_gadget(phonet_opts->net, gadget);
+			status = gphonet_register_netdev(phonet_opts->net);
+			if (status)
+				return status;
+			phonet_opts->bound = true;
+		}
 	}
 
 	/* Reserve interface IDs */
@@ -621,10 +617,13 @@ static void phonet_free_inst(struct usb_function_instance *f)
 	struct f_phonet_opts *opts;
 
 	opts = container_of(f, struct f_phonet_opts, func_inst);
-	if (opts->bound)
-		gphonet_cleanup(opts->net);
-	else
-		free_netdev(opts->net);
+	scoped_guard(mutex, &opts->lock) {
+		if (opts->bound)
+			gphonet_cleanup(opts->net);
+		else
+			free_netdev(opts->net);
+	}
+	mutex_destroy(&opts->lock);
 	kfree(opts);
 }
 
@@ -636,6 +635,7 @@ static struct usb_function_instance *phonet_alloc_inst(void)
 	if (!opts)
 		return ERR_PTR(-ENOMEM);
 
+	mutex_init(&opts->lock);
 	opts->func_inst.free_func_inst = phonet_free_inst;
 	opts->net = gphonet_setup_default();
 	if (IS_ERR(opts->net)) {
diff --git a/drivers/usb/gadget/function/u_phonet.h b/drivers/usb/gadget/function/u_phonet.h
index ff62ca22c..54fadfe64 100644
--- a/drivers/usb/gadget/function/u_phonet.h
+++ b/drivers/usb/gadget/function/u_phonet.h
@@ -8,11 +8,21 @@
 #ifndef __U_PHONET_H
 #define __U_PHONET_H
 
+#include <linux/mutex.h>
 #include <linux/usb/composite.h>
 #include <linux/usb/cdc.h>
 
+/**
+ * struct f_phonet_opts - Phonet function instance options
+ * @func_inst: USB function instance
+ * @lock: protects @bound and @net against concurrent access from
+ *        pn_bind() vs phonet_free_inst() during configfs teardown
+ * @bound: true once pn_bind() has successfully registered @net
+ * @net: net_device owned by this function instance
+ */
 struct f_phonet_opts {
 	struct usb_function_instance func_inst;
+	struct mutex lock;
 	bool bound;
 	struct net_device *net;
 };
-- 
2.34.1