| Message ID | a11e31ab-6ffc-453f-ba6a-b7f6e512c55e@I-love.SAKURA.ne.jp |
|---|---|
| State | New |
| Headers | show |
| Series | tty: tty_io: fix race between tty_fops and hung_up_tty_fops | expand |
On Tue 23-07-24 07:20:34, Tetsuo Handa wrote: > On 2024/07/23 1:24, Greg Kroah-Hartman wrote: > > 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. > > https://lkml.kernel.org/r/18a58415-4aa9-4cba-97d2-b70384407313@I-love.SAKURA.ne.jp was a patch > that does not replace f_op, and > https://lkml.kernel.org/r/CAHk-=wgSOa_g+bxjNi+HQpC=6sHK2yKeoW-xOhb0-FVGMTDWjg@mail.gmail.com > was a comment from Linus. OK, thanks for references. After doing some light audit of tty, I didn't find a way how switching f_op could break the system. Still it is giving me some creeps because usually there's some god-forgotten place somewhere that caches some decision based on f_op content and when f_op change, things go out of sync with strange results. And the splice operations enabled by tty are complex enough to hide some potential problems. In fact I'm not sure why tty switches f_op at all. The reliable check for tty being hung up seems to be fetching ldisc pointer under a ldisc_lock and most operations do this and handle it appropriately -> no need for special f_op for hung up state for them. .ioctl notably might need some love but overall it doesn't seem too hard to completely avoid changing f_op for tty. Honza
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
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 . Reported-by: syzbot <syzbot+b7c3ba8cdc2f6cf83c21@syzkaller.appspotmail.com> Closes: https://syzkaller.appspot.com/bug?extid=b7c3ba8cdc2f6cf83c21 Suggested-by: Linus Torvalds <torvalds@linux-foundation.org> Cc: Dmitry Vyukov <dvyukov@google.com> Cc: Marco Elver <elver@google.com> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> --- 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@I-love.SAKURA.ne.jp . 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(-)