| From 0aee4c6e17f0862e8684281dd5c28914537ff8c9 Mon Sep 17 00:00:00 2001 |
| From: "Eric W. Biederman" <ebiederm@xmission.com> |
| Date: Fri, 12 Jun 2020 09:42:03 -0500 |
| Subject: [PATCH] proc: Use new_inode not new_inode_pseudo |
| |
| commit ef1548adada51a2f32ed7faef50aa465e1b4c5da upstream. |
| |
| Recently syzbot reported that unmounting proc when there is an ongoing |
| inotify watch on the root directory of proc could result in a use |
| after free when the watch is removed after the unmount of proc |
| when the watcher exits. |
| |
| Commit 69879c01a0c3 ("proc: Remove the now unnecessary internal mount |
| of proc") made it easier to unmount proc and allowed syzbot to see the |
| problem, but looking at the code it has been around for a long time. |
| |
| Looking at the code the fsnotify watch should have been removed by |
| fsnotify_sb_delete in generic_shutdown_super. Unfortunately the inode |
| was allocated with new_inode_pseudo instead of new_inode so the inode |
| was not on the sb->s_inodes list. Which prevented |
| fsnotify_unmount_inodes from finding the inode and removing the watch |
| as well as made it so the "VFS: Busy inodes after unmount" warning |
| could not find the inodes to warn about them. |
| |
| Make all of the inodes in proc visible to generic_shutdown_super, |
| and fsnotify_sb_delete by using new_inode instead of new_inode_pseudo. |
| The only functional difference is that new_inode places the inodes |
| on the sb->s_inodes list. |
| |
| I wrote a small test program and I can verify that without changes it |
| can trigger this issue, and by replacing new_inode_pseudo with |
| new_inode the issues goes away. |
| |
| Cc: stable@vger.kernel.org |
| Link: https://lkml.kernel.org/r/000000000000d788c905a7dfa3f4@google.com |
| Reported-by: syzbot+7d2debdcdb3cb93c1e5e@syzkaller.appspotmail.com |
| Fixes: 0097875bd415 ("proc: Implement /proc/thread-self to point at the directory of the current thread") |
| Fixes: 021ada7dff22 ("procfs: switch /proc/self away from proc_dir_entry") |
| Fixes: 51f0885e5415 ("vfs,proc: guarantee unique inodes in /proc") |
| Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> |
| Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com> |
| |
| diff --git a/fs/proc/inode.c b/fs/proc/inode.c |
| index 5f8d215b3fd0..df6a5c0656df 100644 |
| --- a/fs/proc/inode.c |
| +++ b/fs/proc/inode.c |
| @@ -441,7 +441,7 @@ const struct inode_operations proc_link_inode_operations = { |
| |
| struct inode *proc_get_inode(struct super_block *sb, struct proc_dir_entry *de) |
| { |
| - struct inode *inode = new_inode_pseudo(sb); |
| + struct inode *inode = new_inode(sb); |
| |
| if (inode) { |
| inode->i_ino = de->low_ino; |
| diff --git a/fs/proc/self.c b/fs/proc/self.c |
| index 57c0a1047250..32af065397f8 100644 |
| --- a/fs/proc/self.c |
| +++ b/fs/proc/self.c |
| @@ -43,7 +43,7 @@ int proc_setup_self(struct super_block *s) |
| inode_lock(root_inode); |
| self = d_alloc_name(s->s_root, "self"); |
| if (self) { |
| - struct inode *inode = new_inode_pseudo(s); |
| + struct inode *inode = new_inode(s); |
| if (inode) { |
| inode->i_ino = self_inum; |
| inode->i_mtime = inode->i_atime = inode->i_ctime = current_time(inode); |
| diff --git a/fs/proc/thread_self.c b/fs/proc/thread_self.c |
| index f61ae53533f5..fac9e50b33a6 100644 |
| --- a/fs/proc/thread_self.c |
| +++ b/fs/proc/thread_self.c |
| @@ -43,7 +43,7 @@ int proc_setup_thread_self(struct super_block *s) |
| inode_lock(root_inode); |
| thread_self = d_alloc_name(s->s_root, "thread-self"); |
| if (thread_self) { |
| - struct inode *inode = new_inode_pseudo(s); |
| + struct inode *inode = new_inode(s); |
| if (inode) { |
| inode->i_ino = thread_self_inum; |
| inode->i_mtime = inode->i_atime = inode->i_ctime = current_time(inode); |
| -- |
| 2.27.0 |
| |