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