| Message ID | 20240113005747.38887-1-ebiggers@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | f2fs: fix double free of f2fs_sb_info | expand |
On Fri, Jan 12, 2024 at 04:57:47PM -0800, Eric Biggers wrote: > From: Eric Biggers <ebiggers@google.com> > > kill_f2fs_super() is called even if f2fs_fill_super() fails. > f2fs_fill_super() frees the struct f2fs_sb_info, so it must set > sb->s_fs_info to NULL to prevent it from being freed again. > > Fixes: 275dca4630c1 ("f2fs: move release of block devices to after kill_block_super()") > Reported-by: syzbot+8f477ac014ff5b32d81f@syzkaller.appspotmail.com > Closes: https://lore.kernel.org/r/0000000000006cb174060ec34502@google.com > Signed-off-by: Eric Biggers <ebiggers@google.com> Jaegeuk, I'd be glad to take this through the fscrypt tree since that's where my broken commit came from. But let me know if you want to just take this through the f2fs tree. - Eric
On 2024/1/13 8:57, Eric Biggers wrote: > From: Eric Biggers <ebiggers@google.com> > > kill_f2fs_super() is called even if f2fs_fill_super() fails. > f2fs_fill_super() frees the struct f2fs_sb_info, so it must set > sb->s_fs_info to NULL to prevent it from being freed again. Oh, I missed that case as well during reviewing, my bad. > > Fixes: 275dca4630c1 ("f2fs: move release of block devices to after kill_block_super()") > Reported-by: syzbot+8f477ac014ff5b32d81f@syzkaller.appspotmail.com > Closes: https://lore.kernel.org/r/0000000000006cb174060ec34502@google.com > Signed-off-by: Eric Biggers <ebiggers@google.com> Reviewed-by: Chao Yu <chao@kernel.org> Thanks, > --- > fs/f2fs/super.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c > index d00d21a8b53ad..d45ab0992ae59 100644 > --- a/fs/f2fs/super.c > +++ b/fs/f2fs/super.c > @@ -4873,20 +4873,21 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent) > kfree(F2FS_OPTION(sbi).s_qf_names[i]); > #endif > fscrypt_free_dummy_policy(&F2FS_OPTION(sbi).dummy_enc_policy); > kvfree(options); > free_sb_buf: > kfree(raw_super); > free_sbi: > if (sbi->s_chksum_driver) > crypto_free_shash(sbi->s_chksum_driver); > kfree(sbi); > + sb->s_fs_info = NULL; > > /* give only one another chance */ > if (retry_cnt > 0 && skip_recovery) { > retry_cnt--; > shrink_dcache_sb(sb); > goto try_onemore; > } > return err; > } > > > base-commit: 38814330fedd778edffcabe0c8cb462ee365782e
On 01/12, Eric Biggers wrote: > On Fri, Jan 12, 2024 at 04:57:47PM -0800, Eric Biggers wrote: > > From: Eric Biggers <ebiggers@google.com> > > > > kill_f2fs_super() is called even if f2fs_fill_super() fails. > > f2fs_fill_super() frees the struct f2fs_sb_info, so it must set > > sb->s_fs_info to NULL to prevent it from being freed again. > > > > Fixes: 275dca4630c1 ("f2fs: move release of block devices to after kill_block_super()") > > Reported-by: syzbot+8f477ac014ff5b32d81f@syzkaller.appspotmail.com > > Closes: https://lore.kernel.org/r/0000000000006cb174060ec34502@google.com > > Signed-off-by: Eric Biggers <ebiggers@google.com> > > Jaegeuk, I'd be glad to take this through the fscrypt tree since that's where my Ok, are you heading to push this in -rc1? > broken commit came from. But let me know if you want to just take this through > the f2fs tree. > > - Eric
On Fri, Jan 12, 2024 at 05:28:31PM -0800, Jaegeuk Kim wrote: > On 01/12, Eric Biggers wrote: > > On Fri, Jan 12, 2024 at 04:57:47PM -0800, Eric Biggers wrote: > > > From: Eric Biggers <ebiggers@google.com> > > > > > > kill_f2fs_super() is called even if f2fs_fill_super() fails. > > > f2fs_fill_super() frees the struct f2fs_sb_info, so it must set > > > sb->s_fs_info to NULL to prevent it from being freed again. > > > > > > Fixes: 275dca4630c1 ("f2fs: move release of block devices to after kill_block_super()") > > > Reported-by: syzbot+8f477ac014ff5b32d81f@syzkaller.appspotmail.com > > > Closes: https://lore.kernel.org/r/0000000000006cb174060ec34502@google.com > > > Signed-off-by: Eric Biggers <ebiggers@google.com> > > > > Jaegeuk, I'd be glad to take this through the fscrypt tree since that's where my > > Ok, are you heading to push this in -rc1? > > > broken commit came from. But let me know if you want to just take this through > > the f2fs tree. > > Yes, we should get this into -rc1. - Eric
On 01/12, Eric Biggers wrote: > On Fri, Jan 12, 2024 at 05:28:31PM -0800, Jaegeuk Kim wrote: > > On 01/12, Eric Biggers wrote: > > > On Fri, Jan 12, 2024 at 04:57:47PM -0800, Eric Biggers wrote: > > > > From: Eric Biggers <ebiggers@google.com> > > > > > > > > kill_f2fs_super() is called even if f2fs_fill_super() fails. > > > > f2fs_fill_super() frees the struct f2fs_sb_info, so it must set > > > > sb->s_fs_info to NULL to prevent it from being freed again. > > > > > > > > Fixes: 275dca4630c1 ("f2fs: move release of block devices to after kill_block_super()") > > > > Reported-by: syzbot+8f477ac014ff5b32d81f@syzkaller.appspotmail.com > > > > Closes: https://lore.kernel.org/r/0000000000006cb174060ec34502@google.com > > > > Signed-off-by: Eric Biggers <ebiggers@google.com> > > > > > > Jaegeuk, I'd be glad to take this through the fscrypt tree since that's where my > > > > Ok, are you heading to push this in -rc1? > > > > > broken commit came from. But let me know if you want to just take this through > > > the f2fs tree. > > > > > Yes, we should get this into -rc1. Ok, please do so. > > - Eric
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c index d00d21a8b53ad..d45ab0992ae59 100644 --- a/fs/f2fs/super.c +++ b/fs/f2fs/super.c @@ -4873,20 +4873,21 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent) kfree(F2FS_OPTION(sbi).s_qf_names[i]); #endif fscrypt_free_dummy_policy(&F2FS_OPTION(sbi).dummy_enc_policy); kvfree(options); free_sb_buf: kfree(raw_super); free_sbi: if (sbi->s_chksum_driver) crypto_free_shash(sbi->s_chksum_driver); kfree(sbi); + sb->s_fs_info = NULL; /* give only one another chance */ if (retry_cnt > 0 && skip_recovery) { retry_cnt--; shrink_dcache_sb(sb); goto try_onemore; } return err; }