USB: serial: console: Fix potential use-after-free in usb_console_setup()

PATCH In review Liang He 3 participants 6 messages Sep 16, 2022 – Sep 19, 2022

Subsystems (2)

USB USB serial

lore

Share & tools

lore

Liang He

author

2022-09-16 07:36 UTC #0 lore raw

PATCH 1/1 USB: serial: console: Fix potential use-after-free in usb_console_setup()

In usb_console_setup(), if we goto error_get_interface and the usb_serial_put() may finally call kfree(serial). However, the next line will call 'mutex_unlock(&serial->disc_mutex)' which can cause a potential UAF bug.

Fixes 7bd032dc2793 ("USB serial: update the console driver")

Signed-off-by Liang He <[email protected]>

---

I don't know if the refcount can be zero here, so if it cannot be zero,
this code is safe and please ignore my patch.

 drivers/usb/serial/console.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

drivers/usb/serial/console.c +3 -2

index b97aa40ca4d1..21ac2dd6baca 100644
@@ -62,6 +62,7 @@ static int usb_console_setup(struct console *co, char *options)
 	int cflag = CREAD | HUPCL | CLOCAL;
 	char *s;
 	struct usb_serial *serial;
+	struct mutex *s_mutex;
 	struct usb_serial_port *port;
 	int retval;
 	struct tty_struct *tty = NULL;

@@ -116,7 +117,7 @@ static int usb_console_setup(struct console *co, char *options)
 		return -ENODEV;
 	}
 	serial = port->serial;
-
+	s_mutex = &serial->disc_mutex;
 	retval = usb_autopm_get_interface(serial->interface);
 	if (retval)
 		goto error_get_interface;

@@ -190,7 +191,7 @@ static int usb_console_setup(struct console *co, char *options)
 	usb_autopm_put_interface(serial->interface);
  error_get_interface:
 	usb_serial_put(serial);
-	mutex_unlock(&serial->disc_mutex);
+	mutex_unlock(s_mutex);
 	return retval;
 }

Signature · 1 line

2.25.1

Alan Stern

2022-09-16 15:04 UTC #1 lore raw

Quoted 4 lines · Liang He

In usb_console_setup(), if we goto error_get_interface and the usb_serial_put() may finally call kfree(serial). However, the next line will call 'mutex_unlock(&serial->disc_mutex)' which can cause a potential UAF bug.

Why not just move the mutex_unlock() call up one line, before the usb_serial_put()?

Quoted 32 lines

Fixes 7bd032dc2793 ("USB serial: update the console driver")

Signed-off-by Liang He <[email protected]>

---

I don't know if the refcount can be zero here, so if it cannot be zero,
this code is safe and please ignore my patch.

 drivers/usb/serial/console.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

drivers/usb/serial/console.c +2 -1

index b97aa40ca4d1..21ac2dd6baca 100644
@@ -62,6 +62,7 @@ static int usb_console_setup(struct console *co, char *options)
 	int cflag = CREAD | HUPCL | CLOCAL;
 	char *s;
 	struct usb_serial *serial;
+	struct mutex *s_mutex;
 	struct usb_serial_port *port;
 	int retval;
 	struct tty_struct *tty = NULL;

@@ -116,7 +117,7 @@ static int usb_console_setup(struct console *co, char *options)
 		return -ENODEV;
 	}
 	serial = port->serial;
-
+	s_mutex = &serial->disc_mutex;
 	retval = usb_autopm_get_interface(serial->interface);
 	if (retval)
 		goto error_get_interface;

drivers/usb/serial/console.c static int usb_console_setup(struct console *co, char *options) · line 194

Quoted 5 lines

drivers/usb/serial/console.c +1 -1

@@ -190,7 +191,7 @@ static int usb_console_setup(struct console *co, char *options)
 	usb_autopm_put_interface(serial->interface);
  error_get_interface:
 	usb_serial_put(serial);
-	mutex_unlock(&serial->disc_mutex);
+	mutex_unlock(s_mutex);

If the old code was unsafe then so is this, because s_mutex points to a mutex that is embedded within the serial structure. If the structure was deallocated by usb_serial_put() then so was the mutex.

Alan Stern

Quoted 6 lines

drivers/usb/serial/console.c +0 -0
 	return retval;
 }
 
Signature · 1 line
2.25.1

Liang He

author

2022-09-16 15:21 UTC #2 lore raw

Quoted 51 lines · Alan Stern

Quoted 4 lines · Liang He
In usb_console_setup(), if we goto error_get_interface and the usb_serial_put() may finally call kfree(serial). However, the next line will call 'mutex_unlock(&serial->disc_mutex)' which can cause a potential UAF bug.
Why not just move the mutex_unlock() call up one line, before the usb_serial_put()?
Quoted 32 lines
Fixes 7bd032dc2793 ("USB serial: update the console driver")
Signed-off-by Liang He <[email protected]>
---
I don't know if the refcount can be zero here, so if it cannot be zero,
this code is safe and please ignore my patch.
 drivers/usb/serial/console.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/usb/serial/console.c +2 -1
index b97aa40ca4d1..21ac2dd6baca 100644
@@ -62,6 +62,7 @@ static int usb_console_setup(struct console *co, char *options)
 	int cflag = CREAD | HUPCL | CLOCAL;
 	char *s;
 	struct usb_serial *serial;
+	struct mutex *s_mutex;
 	struct usb_serial_port *port;
 	int retval;
 	struct tty_struct *tty = NULL;
@@ -116,7 +117,7 @@ static int usb_console_setup(struct console *co, char *options)
 		return -ENODEV;
 	}
 	serial = port->serial;
-
+	s_mutex = &serial->disc_mutex;
 	retval = usb_autopm_get_interface(serial->interface);
 	if (retval)
 		goto error_get_interface;
drivers/usb/serial/console.c static int usb_console_setup(struct console *co, char *options) · line 194
Quoted 5 lines
drivers/usb/serial/console.c +1 -1
@@ -190,7 +191,7 @@ static int usb_console_setup(struct console *co, char *options)
 	usb_autopm_put_interface(serial->interface);
  error_get_interface:
 	usb_serial_put(serial);
-	mutex_unlock(&serial->disc_mutex);
+	mutex_unlock(s_mutex);
If the old code was unsafe then so is this, because s_mutex points to a mutex that is embedded within the serial structure. If the structure was deallocated by usb_serial_put() then so was the mutex.

Quoted 8 lines

Alan Stern
Quoted 6 lines
drivers/usb/serial/console.c +0 -0
 	return retval;
 }
 
Signature · 1 line
2.25.1

Hi, Alan Stern,

Thanks for your review and this patch is indeed wrong!

But I am not sure if we can safely move the usb_serial_put() out of mutex_unlock().

If it is safe, I can give a new version of patch very soon.

Can you help me confirm if it is safe?

Thanks again,

Liang

Alan Stern

2022-09-16 15:36 UTC #3 lore raw

Quoted 65 lines · Liang He

Quoted 51 lines · Alan Stern
Quoted 4 lines · Liang He
In usb_console_setup(), if we goto error_get_interface and the usb_serial_put() may finally call kfree(serial). However, the next line will call 'mutex_unlock(&serial->disc_mutex)' which can cause a potential UAF bug.
Why not just move the mutex_unlock() call up one line, before the usb_serial_put()?
Quoted 32 lines
Fixes 7bd032dc2793 ("USB serial: update the console driver")
Signed-off-by Liang He <[email protected]>
---
I don't know if the refcount can be zero here, so if it cannot be zero,
this code is safe and please ignore my patch.
 drivers/usb/serial/console.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/usb/serial/console.c +2 -1
index b97aa40ca4d1..21ac2dd6baca 100644
@@ -62,6 +62,7 @@ static int usb_console_setup(struct console *co, char *options)
 	int cflag = CREAD | HUPCL | CLOCAL;
 	char *s;
 	struct usb_serial *serial;
+	struct mutex *s_mutex;
 	struct usb_serial_port *port;
 	int retval;
 	struct tty_struct *tty = NULL;
@@ -116,7 +117,7 @@ static int usb_console_setup(struct console *co, char *options)
 		return -ENODEV;
 	}
 	serial = port->serial;
-
+	s_mutex = &serial->disc_mutex;
 	retval = usb_autopm_get_interface(serial->interface);
 	if (retval)
 		goto error_get_interface;
drivers/usb/serial/console.c static int usb_console_setup(struct console *co, char *options) · line 194
Quoted 5 lines
drivers/usb/serial/console.c +1 -1
@@ -190,7 +191,7 @@ static int usb_console_setup(struct console *co, char *options)
 	usb_autopm_put_interface(serial->interface);
  error_get_interface:
 	usb_serial_put(serial);
-	mutex_unlock(&serial->disc_mutex);
+	mutex_unlock(s_mutex);
If the old code was unsafe then so is this, because s_mutex points to a mutex that is embedded within the serial structure. If the structure was deallocated by usb_serial_put() then so was the mutex.
Quoted 8 lines
Alan Stern
Quoted 6 lines
drivers/usb/serial/console.c +0 -0
 	return retval;
 }
 
Signature · 1 line
2.25.1
Hi, Alan Stern,

Quoted 8 lines

Thanks for your review and this patch is indeed wrong!
But I am not sure if we can safely move the usb_serial_put() out of mutex_unlock().
If it is safe, I can give a new version of patch very soon.
Can you help me confirm if it is safe?

I cannot. You need to ask Johan (the USB-serial maintainer).

Alan Stern

Liang He

author

2022-09-19 01:59 UTC #4 lore raw

Quoted 71 lines · Alan Stern

Quoted 70 lines · Liang He
Quoted 51 lines · Alan Stern
Quoted 4 lines · Liang He
In usb_console_setup(), if we goto error_get_interface and the usb_serial_put() may finally call kfree(serial). However, the next line will call 'mutex_unlock(&serial->disc_mutex)' which can cause a potential UAF bug.
Why not just move the mutex_unlock() call up one line, before the usb_serial_put()?
Quoted 32 lines
Fixes 7bd032dc2793 ("USB serial: update the console driver")
Signed-off-by Liang He <[email protected]>
---
I don't know if the refcount can be zero here, so if it cannot be zero,
this code is safe and please ignore my patch.
 drivers/usb/serial/console.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/usb/serial/console.c +2 -1
index b97aa40ca4d1..21ac2dd6baca 100644
@@ -62,6 +62,7 @@ static int usb_console_setup(struct console *co, char *options)
 	int cflag = CREAD | HUPCL | CLOCAL;
 	char *s;
 	struct usb_serial *serial;
+	struct mutex *s_mutex;
 	struct usb_serial_port *port;
 	int retval;
 	struct tty_struct *tty = NULL;
@@ -116,7 +117,7 @@ static int usb_console_setup(struct console *co, char *options)
 		return -ENODEV;
 	}
 	serial = port->serial;
-
+	s_mutex = &serial->disc_mutex;
 	retval = usb_autopm_get_interface(serial->interface);
 	if (retval)
 		goto error_get_interface;
drivers/usb/serial/console.c static int usb_console_setup(struct console *co, char *options) · line 194
Quoted 5 lines
drivers/usb/serial/console.c +1 -1
@@ -190,7 +191,7 @@ static int usb_console_setup(struct console *co, char *options)
 	usb_autopm_put_interface(serial->interface);
  error_get_interface:
 	usb_serial_put(serial);
-	mutex_unlock(&serial->disc_mutex);
+	mutex_unlock(s_mutex);
If the old code was unsafe then so is this, because s_mutex points to a mutex that is embedded within the serial structure. If the structure was deallocated by usb_serial_put() then so was the mutex.
Quoted 8 lines
Alan Stern
Quoted 6 lines
drivers/usb/serial/console.c +0 -0
 	return retval;
 }
 
Signature · 1 line
2.25.1
Hi, Alan Stern,
Thanks for your review and this patch is indeed wrong!
But I am not sure if we can safely move the usb_serial_put() out of mutex_unlock().

Quoted 7 lines

Quoted 3 lines
If it is safe, I can give a new version of patch very soon.
Can you help me confirm if it is safe?
I cannot. You need to ask Johan (the USB-serial maintainer).
Alan Stern

Still thanks!

And from a recent similar commit, I think we can move mutex_unlock above the usb_serial_put():

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=v6.0-rc5&id=6c53b45c71b4920b5e62f0ea8079a1da382b9434

Johan, please confirm if this can be accepted.

Thanks,

Liang

Johan Hovold

maintainer

2022-09-19 06:51 UTC #5 lore raw

Quoted 21 lines · Liang He

Quoted 20 lines · Alan Stern
Quoted 19 lines · Liang He
Quoted 6 lines · Alan Stern
Quoted 4 lines · Liang He
In usb_console_setup(), if we goto error_get_interface and the usb_serial_put() may finally call kfree(serial). However, the next line will call 'mutex_unlock(&serial->disc_mutex)' which can cause a potential UAF bug.
Quoted 6 lines
Why not just move the mutex_unlock() call up one line, before the usb_serial_put()?
If the old code was unsafe then so is this, because s_mutex points to a mutex that is embedded within the serial structure. If the structure was deallocated by usb_serial_put() then so was the mutex.
Thanks for your review and this patch is indeed wrong!
But I am not sure if we can safely move the usb_serial_put() out of mutex_unlock().

Quoted 7 lines

Quoted 5 lines
Quoted 3 lines
If it is safe, I can give a new version of patch very soon.
Can you help me confirm if it is safe?
I cannot. You need to ask Johan (the USB-serial maintainer).
Johan, please confirm if this can be accepted.

Yes, we should unlock before dropping the reference as Alan suggested.

Note however that there is no use-after-free here as USB serial core holds another reference when the console is registered.

Johan