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. Why can't the hangup just be recorded in tty_file_private and then checked in the corresponding f_op->$method()? And if that's not possible for some reason I'd be willing to sacrifice one of the FMODE_* bits I recently freed up and add e.g., FMODE_TTY_HANGUP instead of this f_op raciness (Why wasn't this using replace_fops anyway so it'd be easy to grep for it?). Something like the completely untested below: diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c index bc9aebcb873f..219bf6391fed 100644 --- a/drivers/tty/tty_io.c +++ b/drivers/tty/tty_io.c @@ -583,6 +583,26 @@ static struct file *tty_release_redirect(struct tty_struct *tty) return f; } +static noinline void noinline tty_mark_hungup(struct file *file) +{ + fmode_t fmode = READ_ONCE(file->f_mode); + + do { + if (fmode & FMODE_TTY_HUNGUP) + break; + } while (!try_cmpxchg(&file->f_mode, &fmode, fmode | FMODE_TTY_HUNGUP)); +} + +static noinline void noinline tty_clear_hungup(struct file *file) +{ + fmode_t fmode = READ_ONCE(file->f_mode); + + do { + if (!(fmode & FMODE_TTY_HUNGUP)) + break; + } while (!try_cmpxchg(&file->f_mode, &fmode, fmode & ~FMODE_TTY_HUNGUP)); +} + /** * __tty_hangup - actual handler for hangup events * @tty: tty device @@ -652,7 +672,7 @@ static void __tty_hangup(struct tty_struct *tty, int exit_session) continue; closecount++; __tty_fasync(-1, filp, 0); /* can't block */ - filp->f_op = &hung_up_tty_fops; + tty_mark_hungup(filp); } spin_unlock(&tty->files_lock); @@ -776,7 +796,7 @@ void tty_vhangup_session(struct tty_struct *tty) */ int tty_hung_up_p(struct file *filp) { - return (filp && filp->f_op == &hung_up_tty_fops); + return (filp && (READ_ONCE(filp->f_mode) & FMODE_TTY_HUNGUP)); } EXPORT_SYMBOL(tty_hung_up_p); @@ -2204,7 +2224,7 @@ static int tty_open(struct inode *inode, struct file *filp) * Need to reset f_op in case a hangup happened. */ if (tty_hung_up_p(filp)) - filp->f_op = &tty_fops; + tty_clear_hungup(filp); goto retry_open; } clear_bit(TTY_HUPPED, &tty->flags); @@ -2718,6 +2738,10 @@ long tty_ioctl(struct file *file, unsigned int cmd, unsigned long arg) int retval; struct tty_ldisc *ld; + /* Check whether the tty hung up. */ + if (tty_hung_up_p(file)) + return hung_up_tty_ioctl(file, cmd, arg); + if (tty_paranoia_check(tty, file_inode(file), "tty_ioctl")) return -EINVAL; diff --git a/include/linux/fs.h b/include/linux/fs.h index ea3df718c53e..e96b86bab356 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -128,7 +128,8 @@ typedef int (dio_iodone_t)(struct kiocb *iocb, loff_t offset, /* File supports atomic writes */ #define FMODE_CAN_ATOMIC_WRITE ((__force fmode_t)(1 << 7)) -/* FMODE_* bit 8 */ +/* File driver hung up (tty specific) */ +#define FMODE_TTY_HUNGUP ((__force fmode_t)(1 << 8)) /* 32bit hashes as llseek() offset (for directories) */ #define FMODE_32BITHASH ((__force fmode_t)(1 << 9)) > > Reported-by: syzbot <syzbot+b7c3ba8cdc2f6cf83c21@xxxxxxxxxxxxxxxxxxxxxxxxx> > Closes: https://syzkaller.appspot.com/bug?extid=b7c3ba8cdc2f6cf83c21 > Suggested-by: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> > Cc: Dmitry Vyukov <dvyukov@xxxxxxxxxx> > Cc: Marco Elver <elver@xxxxxxxxxx> > Signed-off-by: Tetsuo Handa <penguin-kernel@xxxxxxxxxxxxxxxxxxx> > --- > This patch has been tested using linux-next tree via my tomoyo tree since 20240611, > and there was no response on > [fs] Are you OK with updating "struct file"->f_op value dynamically? > at https://lkml.kernel.org/r/b221d2cf-7dc0-4624-a040-85c131ed72a1@xxxxxxxxxxxxxxxxxxx . > Thus, I guess we can go with this approach. > > drivers/tty/tty_io.c | 34 ++++++++++++++++++++++++++++++++++ > include/linux/fs.h | 2 +- > 2 files changed, 35 insertions(+), 1 deletion(-) > > diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c > index 407b0d87b7c1..bc9aebcb873f 100644 > --- a/drivers/tty/tty_io.c > +++ b/drivers/tty/tty_io.c > @@ -430,6 +430,24 @@ static ssize_t hung_up_tty_write(struct kiocb *iocb, struct iov_iter *from) > return -EIO; > } > > +static ssize_t hung_up_copy_splice_read(struct file *in, loff_t *ppos, > + struct pipe_inode_info *pipe, > + size_t len, unsigned int flags) > +{ > + return -EINVAL; > +} > + > +static ssize_t hung_up_iter_file_splice_write(struct pipe_inode_info *pipe, struct file *out, > + loff_t *ppos, size_t len, unsigned int flags) > +{ > + return -EINVAL; > +} > + > +static int hung_up_no_open(struct inode *inode, struct file *file) > +{ > + return -ENXIO; > +} > + > /* No kernel lock held - none needed ;) */ > static __poll_t hung_up_tty_poll(struct file *filp, poll_table *wait) > { > @@ -462,6 +480,12 @@ static void tty_show_fdinfo(struct seq_file *m, struct file *file) > } > > static const struct file_operations tty_fops = { > + /* > + * WARNING: You must implement all callbacks defined in tty_fops in > + * hung_up_tty_fops, for tty_fops and hung_up_tty_fops are toggled > + * after "struct file" is published. Failure to synchronize has a risk > + * of NULL pointer dereference bug. > + */ > .llseek = no_llseek, > .read_iter = tty_read, > .write_iter = tty_write, > @@ -491,14 +515,24 @@ static const struct file_operations console_fops = { > }; > > static const struct file_operations hung_up_tty_fops = { > + /* > + * WARNING: You must implement all callbacks defined in hung_up_tty_fops > + * in tty_fops, for tty_fops and hung_up_tty_fops are toggled after > + * "struct file" is published. Failure to synchronize has a risk of > + * NULL pointer dereference bug. > + */ > .llseek = no_llseek, > .read_iter = hung_up_tty_read, > .write_iter = hung_up_tty_write, > + .splice_read = hung_up_copy_splice_read, > + .splice_write = hung_up_iter_file_splice_write, > .poll = hung_up_tty_poll, > .unlocked_ioctl = hung_up_tty_ioctl, > .compat_ioctl = hung_up_tty_compat_ioctl, > + .open = hung_up_no_open, > .release = tty_release, > .fasync = hung_up_tty_fasync, > + .show_fdinfo = tty_show_fdinfo, > }; > > static DEFINE_SPINLOCK(redirect_lock); > diff --git a/include/linux/fs.h b/include/linux/fs.h > index 0283cf366c2a..636bcc59a3f5 100644 > --- a/include/linux/fs.h > +++ b/include/linux/fs.h > @@ -1008,7 +1008,7 @@ struct file { > struct file_ra_state f_ra; > struct path f_path; > struct inode *f_inode; /* cached value */ > - const struct file_operations *f_op; > + const struct file_operations *__data_racy f_op; > > u64 f_version; > #ifdef CONFIG_SECURITY > -- > 2.43.5 >