Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Lee Jones <lee@kernel.org>
To:     lee@kernel.org, balbi@kernel.org, gregkh@linuxfoundation.org
Cc:     linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org
Subject: [PATCH 1/1] usb: gadget: f_hid: Conduct proper refcounting on shared f_hidg pointer
Date:   Thu, 17 Nov 2022 12:08:13 +0000
Message-Id: <20221117120813.1257583-1-lee@kernel.org>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

A reference to struct f_hidg is shared with this driver's associated
character device handling component without provision for life-time
handling.  In some circumstances, this can lead to unwanted
behaviour depending on the order in which things are torn down.

Utilise, the reference counting functionality already provided by the
implanted character device structure to ensure the struct f_hidg's
memory is only freed once the character device handling has finished
with it.

Signed-off-by: Lee Jones <lee@kernel.org>
---
 drivers/usb/gadget/function/f_hid.c | 47 +++++++++++++++++++++++------
 1 file changed, 37 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c
index ca0a7d9eaa34e..5da8f44d47d9d 100644
--- a/drivers/usb/gadget/function/f_hid.c
+++ b/drivers/usb/gadget/function/f_hid.c
@@ -77,6 +77,8 @@ struct f_hidg {
 
 	struct usb_ep			*in_ep;
 	struct usb_ep			*out_ep;
+
+	bool				gc;
 };
 
 static inline struct f_hidg *func_to_hidg(struct usb_function *f)
@@ -84,6 +86,11 @@ static inline struct f_hidg *func_to_hidg(struct usb_function *f)
 	return container_of(f, struct f_hidg, func);
 }
 
+static inline bool f_hidg_is_open(struct f_hidg *hidg)
+{
+	return !!kref_read(&hidg->cdev.kobj.kref);
+}
+
 /*-------------------------------------------------------------------------*/
 /*                           Static descriptors                            */
 
@@ -273,6 +280,18 @@ static struct usb_gadget_strings *ct_func_strings[] = {
 	NULL,
 };
 
+static void hidg_free_resources(struct f_hidg *hidg)
+{
+       struct f_hid_opts *opts = container_of(hidg->func.fi, struct f_hid_opts, func_inst);
+
+       mutex_lock(&opts->lock);
+       kfree(hidg->report_desc);
+       kfree(hidg->set_report_buf);
+       kfree(hidg);
+       --opts->refcnt;
+       mutex_unlock(&opts->lock);
+}
+
 /*-------------------------------------------------------------------------*/
 /*                              Char Device                                */
 
@@ -539,7 +558,16 @@ static __poll_t f_hidg_poll(struct file *file, poll_table *wait)
 
 static int f_hidg_release(struct inode *inode, struct file *fd)
 {
+	struct f_hidg *hidg  = fd->private_data;
+
+	if (hidg->gc) {
+		/* Gadget has already been disconnected and we are the last f_hidg user */
+		cdev_del(&hidg->cdev);
+		hidg_free_resources(hidg);
+	}
+
 	fd->private_data = NULL;
+
 	return 0;
 }
 
@@ -1239,17 +1267,16 @@ static struct usb_function_instance *hidg_alloc_inst(void)
 
 static void hidg_free(struct usb_function *f)
 {
-	struct f_hidg *hidg;
-	struct f_hid_opts *opts;
+	struct f_hidg *hidg = func_to_hidg(f);
 
-	hidg = func_to_hidg(f);
-	opts = container_of(f->fi, struct f_hid_opts, func_inst);
-	kfree(hidg->report_desc);
-	kfree(hidg->set_report_buf);
-	kfree(hidg);
-	mutex_lock(&opts->lock);
-	--opts->refcnt;
-	mutex_unlock(&opts->lock);
+	if (f_hidg_is_open(hidg))
+		/*
+		 * Gadget disconnected whilst an open dev node exists.
+		 * Delay freeing resources until it closes.
+		 */
+		hidg->gc = true;
+	else
+		hidg_free_resources(hidg);
 }
 
 static void hidg_unbind(struct usb_configuration *c, struct usb_function *f)
-- 
2.38.1.431.g37b22c650d-goog