Thread (5 messages) 5 messages, 3 authors, 2021-04-07

Re: [PATCH] net: hso: fix null-ptr-deref during tty device unregistration

flat view

From: Greg KH <gregkh@linuxfoundation.org>
Date: 2021-04-06 14:06:04
Also in: linux-usb, lkml

On Tue, Apr 06, 2021 at 06:13:59PM +0530, Anirudh Rayabharam wrote:
quoted hunk ↗ jump to hunk
Multiple ttys try to claim the same the minor number causing a double
unregistration of the same device. The first unregistration succeeds
but the next one results in a null-ptr-deref.

The get_free_serial_index() function returns an available minor number
but doesn't assign it immediately. The assignment is done by the caller
later. But before this assignment, calls to get_free_serial_index()
would return the same minor number.

Fix this by modifying get_free_serial_index to assign the minor number
immediately after one is found to be and rename it to obtain_minor()
to better reflect what it does. Similary, rename set_serial_by_index()
to release_minor() and modify it to free up the minor number of the
given hso_serial. Every obtain_minor() should have corresponding
release_minor() call.

Reported-by: syzbot+c49fe6089f295a05e6f8@syzkaller.appspotmail.com
Tested-by: syzbot+c49fe6089f295a05e6f8@syzkaller.appspotmail.com

Signed-off-by: Anirudh Rayabharam <redacted>
---
 drivers/net/usb/hso.c | 32 ++++++++++++--------------------
 1 file changed, 12 insertions(+), 20 deletions(-)
diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
index 31d51346786a..295ca330e70c 100644
--- a/drivers/net/usb/hso.c
+++ b/drivers/net/usb/hso.c
@@ -611,7 +611,7 @@ static struct hso_serial *get_serial_by_index(unsigned index)
 	return serial;
 }
 
-static int get_free_serial_index(void)
+static int obtain_minor(struct hso_serial *serial)
 {
 	int index;
 	unsigned long flags;
@@ -619,8 +619,10 @@ static int get_free_serial_index(void)
 	spin_lock_irqsave(&serial_table_lock, flags);
 	for (index = 0; index < HSO_SERIAL_TTY_MINORS; index++) {
 		if (serial_table[index] == NULL) {
+			serial_table[index] = serial->parent;
+			serial->minor = index;
 			spin_unlock_irqrestore(&serial_table_lock, flags);
-			return index;
+			return 0;
Minor note, you might want to convert this to use an idr structure in
the future, this "loop and find a free minor" isn't really needed now
that we have a data structure that does this all for us :)

But that's not going to fix this issue, that's for future changes.

thanks,

greg k-h
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help