linux-kernel - Re: [PATCH] tty: pty: Fix race condition between release_one_tty and pty

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Mon, 11 Feb 2019 08:47:37 +0100
From:   Greg KH <gregkh@...uxfoundation.org>
To:     kpark3469@...il.com
Cc:     linux-kernel@...r.kernel.org, keun-o.park@...kmatter.ae,
        peter@...leysoftware.com, jslaby@...e.com, maninder1.s@...sung.com
Subject: Re: [PATCH] tty: pty: Fix race condition between release_one_tty and
 pty_write

On Mon, Feb 11, 2019 at 11:09:15AM +0400, kpark3469@...il.com wrote:
> From: Sahara <keun-o.park@...kmatter.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@...kmatter.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