[PATCH] tty: pty: Fix race condition between release_one_tty and pty_write

[PATCH] tty: pty: Fix race condition between release_one_tty and pty_write

[kpark3469]

From: Sahara <keun-o.park@darkmatter.ae>

Especially when a linked tty is used such as pty, the linked tty
port's buf works have not been cancelled while master tty port's
buf work has been cancelled. Since release_one_tty and flush_to_ldisc
run in workqueue threads separately, when pty_cleanup happens and
link tty port is freed, flush_to_ldisc tries to access freed port
and port->itty, eventually it causes a panic.
This patch utilizes the magic value with holding the tty_mutex to
check if the tty->link is valid.

Fixes: 2b022ab7542d ("pty: cancel pty slave port buf's work in tty_release")
Signed-off-by: Sahara <keun-o.park@darkmatter.ae>
---
 drivers/tty/pty.c    | 7 +++++++
 drivers/tty/tty_io.c | 3 +++
 2 files changed, 10 insertions(+)

diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
index 00099a84..ef72031 100644
--- a/drivers/tty/pty.c
+++ b/drivers/tty/pty.c
@@ -116,6 +116,12 @@ static int pty_write(struct tty_struct *tty, const unsigned char *buf, int c)
 	if (tty->stopped)
 		return 0;
 
+	mutex_lock(&tty_mutex);
+	if (to->magic != TTY_MAGIC) {
+		mutex_unlock(&tty_mutex);
+		return -EIO;
+	}
+
 	if (c > 0) {
 		spin_lock_irqsave(&to->port->lock, flags);
 		/* Stuff the data into the input queue of the other end */
@@ -125,6 +131,7 @@ static int pty_write(struct tty_struct *tty, const unsigned char *buf, int c)
 			tty_flip_buffer_push(to->port);
 		spin_unlock_irqrestore(&to->port->lock, flags);
 	}
+	mutex_unlock(&tty_mutex);
 	return c;
 }
 
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 23c6fd2..6c4a206 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -1446,10 +1446,13 @@ static void release_one_tty(struct work_struct *work)
 	struct tty_driver *driver = tty->driver;
 	struct module *owner = driver->owner;
 
+	mutex_lock(&tty_mutex);
 	if (tty->ops->cleanup)
 		tty->ops->cleanup(tty);
 
 	tty->magic = 0;
+	mutex_unlock(&tty_mutex);
+
 	tty_driver_kref_put(driver);
 	module_put(owner);
 
-- 
2.7.4

Re: [PATCH] tty: pty: Fix race condition between release_one_tty and pty_write

[Greg KH]

On Mon, Feb 11, 2019 at 11:09:15AM +0400, kpark3469@gmail.com wrote:
> From: Sahara <keun-o.park@darkmatter.ae>

I need a "real" name here, one you use to sign legal documents please.

> Especially when a linked tty is used such as pty, the linked tty
> port's buf works have not been cancelled while master tty port's
> buf work has been cancelled. Since release_one_tty and flush_to_ldisc
> run in workqueue threads separately, when pty_cleanup happens and
> link tty port is freed, flush_to_ldisc tries to access freed port
> and port->itty, eventually it causes a panic.
> This patch utilizes the magic value with holding the tty_mutex to
> check if the tty->link is valid.
> 
> Fixes: 2b022ab7542d ("pty: cancel pty slave port buf's work in tty_release")
> Signed-off-by: Sahara <keun-o.park@darkmatter.ae>
> ---
>  drivers/tty/pty.c    | 7 +++++++
>  drivers/tty/tty_io.c | 3 +++
>  2 files changed, 10 insertions(+)
> 
> diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
> index 00099a84..ef72031 100644
> --- a/drivers/tty/pty.c
> +++ b/drivers/tty/pty.c
> @@ -116,6 +116,12 @@ static int pty_write(struct tty_struct *tty, const unsigned char *buf, int c)
>  	if (tty->stopped)
>  		return 0;
>  
> +	mutex_lock(&tty_mutex);
> +	if (to->magic != TTY_MAGIC) {

Checking the "magic" field is odd, are you sure about this?  Usually
this means that memory that is already freed is still being used, right?

And have you tried the 5.0-rc6 release?  I thought I fixed much this
same issue there already, but for a serial port, not a pty.  Is the same
problem still present?




> +		mutex_unlock(&tty_mutex);
> +		return -EIO;
> +	}
> +
>  	if (c > 0) {
>  		spin_lock_irqsave(&to->port->lock, flags);
>  		/* Stuff the data into the input queue of the other end */
> @@ -125,6 +131,7 @@ static int pty_write(struct tty_struct *tty, const unsigned char *buf, int c)
>  			tty_flip_buffer_push(to->port);
>  		spin_unlock_irqrestore(&to->port->lock, flags);
>  	}
> +	mutex_unlock(&tty_mutex);
>  	return c;
>  }
>  
> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> index 23c6fd2..6c4a206 100644
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
> @@ -1446,10 +1446,13 @@ static void release_one_tty(struct work_struct *work)
>  	struct tty_driver *driver = tty->driver;
>  	struct module *owner = driver->owner;
>  
> +	mutex_lock(&tty_mutex);
>  	if (tty->ops->cleanup)
>  		tty->ops->cleanup(tty);
>  
>  	tty->magic = 0;

Using the "magic" field as a flag isn't the best thing for us to rely
on, because this structure should be freed now, right?

thanks,

greg k-h