diff mbox series

[f2fs-dev] f2fs: fix double free of f2fs_sb_info

Message ID 20240113005747.38887-1-ebiggers@kernel.org (mailing list archive)
State Mainlined
Commit c919330dd57835970b37676d377de3eaaea2c1e9
Headers show
Series [f2fs-dev] f2fs: fix double free of f2fs_sb_info | expand

Commit Message

Eric Biggers Jan. 13, 2024, 12:57 a.m. UTC
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>
---
 fs/f2fs/super.c | 1 +
 1 file changed, 1 insertion(+)


base-commit: 38814330fedd778edffcabe0c8cb462ee365782e

Comments

Eric Biggers Jan. 13, 2024, 1:01 a.m. UTC | #1
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
Chao Yu Jan. 13, 2024, 1:14 a.m. UTC | #2
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
Jaegeuk Kim Jan. 13, 2024, 1:28 a.m. UTC | #3
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
Eric Biggers Jan. 13, 2024, 1:32 a.m. UTC | #4
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
Jaegeuk Kim Jan. 13, 2024, 1:46 a.m. UTC | #5
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 mbox series

Patch

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;
 }