Re: [PATCH 5.10 60/76] f2fs: fix to do sanity check on last xattr entry in __f2fs_setxattr()

[Salvatore Bonaccorso <carnil@debian.org>]

Hi,

On Mon, Dec 27, 2021 at 04:31:15PM +0100, Greg Kroah-Hartman wrote:
> From: Chao Yu <chao@kernel.org>
> 
> commit 5598b24efaf4892741c798b425d543e4bed357a1 upstream.
> 
> As Wenqing Liu reported in bugzilla:
> 
> https://bugzilla.kernel.org/show_bug.cgi?id=215235
> 
> - Overview
> page fault in f2fs_setxattr() when mount and operate on corrupted image
> 
> - Reproduce
> tested on kernel 5.16-rc3, 5.15.X under root
> 
> 1. unzip tmp7.zip
> 2. ./single.sh f2fs 7
> 
> Sometimes need to run the script several times
> 
> - Kernel dump
> loop0: detected capacity change from 0 to 131072
> F2FS-fs (loop0): Found nat_bits in checkpoint
> F2FS-fs (loop0): Mounted with checkpoint version = 7548c2ee
> BUG: unable to handle page fault for address: ffffe47bc7123f48
> RIP: 0010:kfree+0x66/0x320
> Call Trace:
>  __f2fs_setxattr+0x2aa/0xc00 [f2fs]
>  f2fs_setxattr+0xfa/0x480 [f2fs]
>  __f2fs_set_acl+0x19b/0x330 [f2fs]
>  __vfs_removexattr+0x52/0x70
>  __vfs_removexattr_locked+0xb1/0x140
>  vfs_removexattr+0x56/0x100
>  removexattr+0x57/0x80
>  path_removexattr+0xa3/0xc0
>  __x64_sys_removexattr+0x17/0x20
>  do_syscall_64+0x37/0xb0
>  entry_SYSCALL_64_after_hwframe+0x44/0xae
> 
> The root cause is in __f2fs_setxattr(), we missed to do sanity check on
> last xattr entry, result in out-of-bound memory access during updating
> inconsistent xattr data of target inode.
> 
> After the fix, it can detect such xattr inconsistency as below:
> 
> F2FS-fs (loop11): inode (7) has invalid last xattr entry, entry_size: 60676
> F2FS-fs (loop11): inode (8) has corrupted xattr
> F2FS-fs (loop11): inode (8) has corrupted xattr
> F2FS-fs (loop11): inode (8) has invalid last xattr entry, entry_size: 47736
> 
> Cc: stable@vger.kernel.org
> Reported-by: Wenqing Liu <wenqingliu0120@gmail.com>
> Signed-off-by: Chao Yu <chao@kernel.org>
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>  fs/f2fs/xattr.c |   11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> --- a/fs/f2fs/xattr.c
> +++ b/fs/f2fs/xattr.c
> @@ -680,8 +680,17 @@ static int __f2fs_setxattr(struct inode
>  	}
>  
>  	last = here;
> -	while (!IS_XATTR_LAST_ENTRY(last))
> +	while (!IS_XATTR_LAST_ENTRY(last)) {
> +		if ((void *)(last) + sizeof(__u32) > last_base_addr ||
> +			(void *)XATTR_NEXT_ENTRY(last) > last_base_addr) {
> +			f2fs_err(F2FS_I_SB(inode), "inode (%lu) has invalid last xattr entry, entry_size: %zu",
> +					inode->i_ino, ENTRY_SIZE(last));
> +			set_sbi_flag(F2FS_I_SB(inode), SBI_NEED_FSCK);
> +			error = -EFSCORRUPTED;
> +			goto exit;
> +		}
>  		last = XATTR_NEXT_ENTRY(last);
> +	}
>  
>  	newsize = XATTR_ALIGN(sizeof(struct f2fs_xattr_entry) + len + size);

It looks this commit while it was applied to several stable series
(TTBOMK in 5.15.12, 5.10.89, 5.4.169, 4.19.223 and 4.14.260) it is
still missing from mainline, Chao, or anyone else, do you know what
happened here?

Regards,
Salvatore