linux-kernel - Re: [PATCH] tty: tty_io: fix race between tty_fops and hung_up_tty

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: <2024072238-reversing-despise-b555@gregkh>
Date: Mon, 22 Jul 2024 18:24:50 +0200
From: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To: Jan Kara <jack@...e.cz>
Cc: Christian Brauner <brauner@...nel.org>,
	Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Jiri Slaby <jirislaby@...nel.org>,
	Alexander Viro <viro@...iv.linux.org.uk>,
	LKML <linux-kernel@...r.kernel.org>,
	linux-serial <linux-serial@...r.kernel.org>,
	linux-fsdevel <linux-fsdevel@...r.kernel.org>
Subject: Re: [PATCH] tty: tty_io: fix race between tty_fops and
 hung_up_tty_fops

On Mon, Jul 22, 2024 at 06:10:41PM +0200, Jan Kara wrote:
> On Mon 22-07-24 16:41:22, Christian Brauner wrote:
> > On Fri, Jul 19, 2024 at 10:37:47PM GMT, Tetsuo Handa wrote:
> > > syzbot is reporting data race between __tty_hangup() and __fput(), and
> > > Dmitry Vyukov mentioned that this race has possibility of NULL pointer
> > > dereference, for tty_fops implements e.g. splice_read callback whereas
> > > hung_up_tty_fops does not.
> > > 
> > >   CPU0                                  CPU1
> > >   ----                                  ----
> > >   do_splice_read() {
> > >                                         __tty_hangup() {
> > >     // f_op->splice_read was copy_splice_read
> > >     if (unlikely(!in->f_op->splice_read))
> > >       return warn_unsupported(in, "read");
> > >                                           filp->f_op = &hung_up_tty_fops;
> > >     // f_op->splice_read is now NULL
> > >     return in->f_op->splice_read(in, ppos, pipe, len, flags);
> > >                                         }
> > >   }
> > > 
> > > Fix possibility of NULL pointer dereference by implementing missing
> > > callbacks, and suppress KCSAN messages by adding __data_racy qualifier
> > > to "struct file"->f_op .
> > 
> > This f_op replacing without synchronization seems really iffy imho.
> 
> Yeah, when I saw this I was also going "ouch". I was just waiting whether a
> tty maintainer will comment ;)

I really didn't want to :)

> Anyway this replacement of ops in file /
> inode has proven problematic in almost every single case where it was used
> leading to subtle issues.

Yeah, let's not do this.

Let me dig after -rc1 is out and see if there's a better way to handle
this contrived race condition...

thanks,

greg k-h