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

Message-ID: <20180425175411.GB15991@dragonet.kaist.ac.kr>
Date:   Thu, 26 Apr 2018 02:54:12 +0900
From:   DaeRyong Jeong <threeearcat@...il.com>
To:     Alan Cox <gnomes@...rguk.ukuu.org.uk>
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, Apr 25, 2018 at 03:39:48PM +0100, Alan Cox wrote:
> 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.

Yes. I thought the wrong way to resolve the issue.

> 
> 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

With your comments, now I know that this patch is incorrect.
Since the bug is real, I will send a new patch with considering all your
comments above.

Thank you a lot for all your comments.

DaeRyong Jeong