diff mbox series

nilfs2: fix use-after-free bug of nilfs_root in nilfs_evict_inode()

Message ID 20230509152956.8313-1-konishi.ryusuke@gmail.com
State New
Headers
Series nilfs2: fix use-after-free bug of nilfs_root in nilfs_evict_inode() |

Commit Message

Ryusuke Konishi May 9, 2023, 3:29 p.m. UTC 
  During unmount process of nilfs2, nothing holds nilfs_root structure after
nilfs2 detaches its writer in nilfs_detach_log_writer().  However, since
nilfs_evict_inode() uses nilfs_root for some cleanup operations, it may
cause use-after-free read if inodes are left in "garbage_list" and
released by nilfs_dispose_list() at the end of nilfs_detach_log_writer().

Fix this issue by modifying nilfs_evict_inode() to only clear inode
without additional metadata changes that use nilfs_root if the file system
is degraded to read-only or the writer is detached.

Signed-off-by: Ryusuke Konishi <konishi.ryusuke@gmail.com>
Reported-by: syzbot+78d4495558999f55d1da@syzkaller.appspotmail.com
Closes: https://lkml.kernel.org/r/00000000000099e5ac05fb1c3b85@google.com
Tested-by: Ryusuke Konishi <konishi.ryusuke@gmail.com>
Cc: stable@vger.kernel.org
---
 fs/nilfs2/inode.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

Comments

Andrew Morton May 12, 2023, 11:44 p.m. UTC | #1 
On Wed, 10 May 2023 00:29:56 +0900 Ryusuke Konishi <konishi.ryusuke@gmail.com> wrote:

> During unmount process of nilfs2, nothing holds nilfs_root structure after
> nilfs2 detaches its writer in nilfs_detach_log_writer().  However, since
> nilfs_evict_inode() uses nilfs_root for some cleanup operations, it may
> cause use-after-free read if inodes are left in "garbage_list" and
> released by nilfs_dispose_list() at the end of nilfs_detach_log_writer().
> 
> Fix this issue by modifying nilfs_evict_inode() to only clear inode
> without additional metadata changes that use nilfs_root if the file system
> is degraded to read-only or the writer is detached.
> 
> ...
>
> --- a/fs/nilfs2/inode.c
> +++ b/fs/nilfs2/inode.c
> @@ -917,6 +917,7 @@ void nilfs_evict_inode(struct inode *inode)
>  	struct nilfs_transaction_info ti;
>  	struct super_block *sb = inode->i_sb;
>  	struct nilfs_inode_info *ii = NILFS_I(inode);
> +	struct the_nilfs *nilfs;
>  	int ret;

`the_nilfs' is unused - I'll zap it.
Andrew Morton May 12, 2023, 11:46 p.m. UTC | #2 
On Fri, 12 May 2023 16:44:24 -0700 Andrew Morton <akpm@linux-foundation.org> wrote:

> On Wed, 10 May 2023 00:29:56 +0900 Ryusuke Konishi <konishi.ryusuke@gmail.com> wrote:
> 
> > During unmount process of nilfs2, nothing holds nilfs_root structure after
> > nilfs2 detaches its writer in nilfs_detach_log_writer().  However, since
> > nilfs_evict_inode() uses nilfs_root for some cleanup operations, it may
> > cause use-after-free read if inodes are left in "garbage_list" and
> > released by nilfs_dispose_list() at the end of nilfs_detach_log_writer().
> > 
> > Fix this issue by modifying nilfs_evict_inode() to only clear inode
> > without additional metadata changes that use nilfs_root if the file system
> > is degraded to read-only or the writer is detached.
> > 
> > ...
> >
> > --- a/fs/nilfs2/inode.c
> > +++ b/fs/nilfs2/inode.c
> > @@ -917,6 +917,7 @@ void nilfs_evict_inode(struct inode *inode)
> >  	struct nilfs_transaction_info ti;
> >  	struct super_block *sb = inode->i_sb;
> >  	struct nilfs_inode_info *ii = NILFS_I(inode);
> > +	struct the_nilfs *nilfs;
> >  	int ret;
> 
> `the_nilfs' is unused - I'll zap it.

argh, sorry, brainfart.
diff mbox series

Patch

diff --git a/fs/nilfs2/inode.c b/fs/nilfs2/inode.c
index 1310d2d5feb3..a8ce522ac747 100644
--- a/fs/nilfs2/inode.c
+++ b/fs/nilfs2/inode.c
@@ -917,6 +917,7 @@  void nilfs_evict_inode(struct inode *inode)
 	struct nilfs_transaction_info ti;
 	struct super_block *sb = inode->i_sb;
 	struct nilfs_inode_info *ii = NILFS_I(inode);
+	struct the_nilfs *nilfs;
 	int ret;
 
 	if (inode->i_nlink || !ii->i_root || unlikely(is_bad_inode(inode))) {
@@ -929,6 +930,23 @@  void nilfs_evict_inode(struct inode *inode)
 
 	truncate_inode_pages_final(&inode->i_data);
 
+	nilfs = sb->s_fs_info;
+	if (unlikely(sb_rdonly(sb) || !nilfs->ns_writer)) {
+		/*
+		 * If this inode is about to be disposed after the file system
+		 * has been degraded to read-only due to file system corruption
+		 * or after the writer has been detached, do not make any
+		 * changes that cause writes, just clear it.
+		 * Do this check after read-locking ns_segctor_sem by
+		 * nilfs_transaction_begin() in order to avoid a race with
+		 * the writer detach operation.
+		 */
+		clear_inode(inode);
+		nilfs_clear_inode(inode);
+		nilfs_transaction_abort(sb);
+		return;
+	}
+
 	/* TODO: some of the following operations may fail.  */
 	nilfs_truncate_bmap(ii, 0);
 	nilfs_mark_inode_dirty(inode);