Forwarded: [PATCH] USB: serial: keyspan: fix use-after-free in keyspan_close

[syzbot <syzbot+5fabc1ae99ff40690d84@syzkaller.appspotmail.com>]

For archival purposes, forwarding an incoming command email to
linux-kernel@vger.kernel.org, syzkaller-bugs@googlegroups.com.

***

Subject: [PATCH] USB: serial: keyspan: fix use-after-free in keyspan_close
Author: kartikey406@gmail.com

#syz test: git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master


keyspan_port_remove() frees the port's private data (p_priv) while
keyspan_close() may still be running concurrently on another task,
e.g. triggered by an explicit TIOCVHANGUP ioctl on an already-open
tty racing with device disconnect. This results in keyspan_close()
dereferencing freed memory.

Fix this by adding a mutex to keyspan_serial_private that serializes
keyspan_close() against keyspan_port_remove(): the latter clears the
port's private data pointer under the lock before freeing it, and
the former re-fetches and checks that pointer under the same lock
before use.

Reported-by: syzbot+5fabc1ae99ff40690d84@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=5fabc1ae99ff40690d84
Signed-off-by: Deepanshu Kartikey <kartikey406@gmail.com>
---
 drivers/usb/serial/keyspan.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/serial/keyspan.c b/drivers/usb/serial/keyspan.c
index 4d3746c7a94e..623f15b51a6a 100644
--- a/drivers/usb/serial/keyspan.c
+++ b/drivers/usb/serial/keyspan.c
@@ -541,6 +541,7 @@ struct keyspan_serial_private {
 	struct urb	*glocont_urb;
 	char		*glocont_buf;
 	char		*ctrl_buf;	/* for EP0 control message */
+	struct mutex	lock; /* protects p_priv vs port_remove races */
 };
 
 struct keyspan_port_private {
@@ -1581,8 +1582,15 @@ static void keyspan_close(struct usb_serial_port *port)
 {
 	int			i;
 	struct keyspan_port_private 	*p_priv;
+	struct keyspan_serial_private	*s_priv = usb_get_serial_data(port->serial);
 
+	mutex_lock(&s_priv->lock);
 	p_priv = usb_get_serial_port_data(port);
+	if (!p_priv) {
+		/* port_remove() already ran and freed this */
+		mutex_unlock(&s_priv->lock);
+		return ;
+	}
 
 	p_priv->rts_state = 0;
 	p_priv->dtr_state = 0;
@@ -1599,6 +1607,7 @@ static void keyspan_close(struct usb_serial_port *port)
 		usb_kill_urb(p_priv->in_urbs[i]);
 		usb_kill_urb(p_priv->out_urbs[i]);
 	}
+	mutex_unlock(&s_priv->lock);
 }
 
 /* download the firmware to a pre-renumeration device */
@@ -2794,7 +2803,7 @@ static int keyspan_startup(struct usb_serial *serial)
 	s_priv = kzalloc_obj(struct keyspan_serial_private);
 	if (!s_priv)
 		return -ENOMEM;
-
+	mutex_init(&s_priv->lock);
 	s_priv->instat_buf = kzalloc(INSTAT_BUFLEN, GFP_KERNEL);
 	if (!s_priv->instat_buf)
 		goto err_instat_buf;
@@ -2971,10 +2980,14 @@ static int keyspan_port_probe(struct usb_serial_port *port)
 
 static void keyspan_port_remove(struct usb_serial_port *port)
 {
+	struct keyspan_serial_private *s_priv = usb_get_serial_data(port->serial);
 	struct keyspan_port_private *p_priv;
 	int i;
 
+	mutex_lock(&s_priv->lock);
 	p_priv = usb_get_serial_port_data(port);
+	usb_set_serial_port_data(port, NULL);
+	mutex_unlock(&s_priv->lock);
 
 	usb_kill_urb(p_priv->inack_urb);
 	usb_kill_urb(p_priv->outcont_urb);
-- 
2.43.0