linux-kernel - Re: [PATCH] tty: Fix data race in tty_insert_flip_string_fixed

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20180425153948.7be2bc57@alans-desktop>
Date:   Wed, 25 Apr 2018 15:39:48 +0100
From:   Alan Cox <gnomes@...rguk.ukuu.org.uk>
To:     DaeRyong Jeong <threeearcat@...il.com>
Cc:     gregkh@...uxfoundation.org, jslaby@...e.com,
        byoungyoung@...due.edu, kt0755@...il.com, bammanag@...due.edu,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] tty: Fix data race in tty_insert_flip_string_fixed_flag

On Wed, 25 Apr 2018 22:20:50 +0900
DaeRyong Jeong <threeearcat@...il.com> wrote:

> tty_insert_flip_string_fixed_flag() copies chars to the buffer indicated
> by th->used and updates tb->used.
> But tty_insert_flip_string_fixed_flag() can be executed concurrently and
> tb->used can be updated improperly.

The tty input layer does not work if it can be executed concurrently. If
that is happening in the pty code then the pty code is buggy and that
needs serializing on the inbound path.


> -static void tty_write_unlock(struct tty_struct *tty)
> +void tty_write_unlock(struct tty_struct *tty, int wakeup)
>  {
>  	mutex_unlock(&tty->atomic_write_lock);
> -	wake_up_interruptible_poll(&tty->write_wait, EPOLLOUT);
> +	if (wakeup) {
> +		wake_up_interruptible_poll(&tty->write_wait, EPOLLOUT);
> +	}

And this may cause deadlocks.

You don't actually need any of the wakeup changes in your code

> diff --git a/drivers/tty/tty_ioctl.c b/drivers/tty/tty_ioctl.c
> index d9b561d89432..a54ab91aec90 100644
> --- a/drivers/tty/tty_ioctl.c
> +++ b/drivers/tty/tty_ioctl.c
> @@ -911,12 +911,17 @@ int n_tty_ioctl_helper(struct tty_struct *tty, struct file *file,
>  			spin_unlock_irq(&tty->flow_lock);
>  			break;
>  		case TCOON:
> +			if (tty_write_lock(tty, 0) < 0)
> +				return -ERESTARTSYS;
> +
>  			spin_lock_irq(&tty->flow_lock);
>  			if (tty->flow_stopped) {
>  				tty->flow_stopped = 0;
>  				__start_tty(tty);
>  			}
>  			spin_unlock_irq(&tty->flow_lock);
> +
> +			tty_write_unlock(tty, 0);

If you just used these unmodified it would be simpler and as good,
however it won't actually fix anything. The pty layer is broken not this
code.

The tty layer rule for all the input buffer handling is that you may not
call *any* of it from multiple threads at once. This works fine for
normal serial because the IRQ layer or the polling logic has that
property.

The bug looks real, your diagnosis looks right, your fix sort of works
but isn't sufficient.

So NAK.

Alan