[SRU][F][PATCH 1/2] f2fs: check validation of fault attrs in f2fs_build_fault_attr()

[Stefan Bader]

On 23.08.24 14:20, Massimiliano Pellizzer wrote:
> From: Chao Yu <chao at kernel.org>
> 
> [ Upstream commit 4ed886b187f47447ad559619c48c086f432d2b77 ]
> 
> - It missed to check validation of fault attrs in parse_options(),
> let's fix to add check condition in f2fs_build_fault_attr().
> - Use f2fs_build_fault_attr() in __sbi_store() to clean up code.
> 
> Signed-off-by: Chao Yu <chao at kernel.org>
> Signed-off-by: Jaegeuk Kim <jaegeuk at kernel.org>
> Signed-off-by: Sasha Levin <sashal at kernel.org>
> (backported from commit bc84dd2c33e0c10fd90d60f0cfc0bfb504d4692d linux-6.1.y)
> [mpellizzer: solved a merge conflict in sysfs.c due to the usage of a
> macro instead of the full instruction in linux-6.1.y, removed the usage
> of the macro BIT(n) in super.c for consistency]
> CVE-2024-42160
> Signed-off-by: Massimiliano Pellizzer <massimiliano.pellizzer at canonical.com>
> ---
>   fs/f2fs/f2fs.h  | 12 ++++++++----
>   fs/f2fs/super.c | 27 ++++++++++++++++++++-------
>   fs/f2fs/sysfs.c | 14 ++++++++++----
>   3 files changed, 38 insertions(+), 15 deletions(-)
> 

>   static inline bool is_journalled_quota(struct f2fs_sb_info *sbi)
> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> index 7f99b05a43eb..f309e1e4fcd3 100644
> --- a/fs/f2fs/super.c
> +++ b/fs/f2fs/super.c
> @@ -57,21 +57,31 @@ const char *f2fs_fault_name[FAULT_MAX] = {
>   	[FAULT_WRITE_IO]	= "write IO error",
>   };
>   
> -void f2fs_build_fault_attr(struct f2fs_sb_info *sbi, unsigned int rate,
> -							unsigned int type)
> +int f2fs_build_fault_attr(struct f2fs_sb_info *sbi, unsigned long rate,
> +							unsigned long type)
>   {
>   	struct f2fs_fault_info *ffi = &F2FS_OPTION(sbi).fault_info;
>   
>   	if (rate) {
> +		if (rate > INT_MAX)
> +			return -EINVAL;
>   		atomic_set(&ffi->inject_ops, 0);
> -		ffi->inject_rate = rate;
> +		ffi->inject_rate = (int)rate;
>   	}
>   
> -	if (type)
> -		ffi->inject_type = type;
> +	if (type) {
> +		if (type >= (1 << FAULT_MAX))

I would generally be careful with constructs like above. Maybe it is ok 
because type is unsigned long and that gets used by if the right hand 
expression is not explicit. But using "1" iirc defaults to int normally. 
Which is a different size for 32bit and 64bit arches. But at least it is 
signed which could make the shifted expression negative and then apply 
in any case. That is why "1UL << FAULT_MAX" might be the better option here.

> +			return -EINVAL;
> +		ffi->inject_type = (unsigned int)type;
> +	}
>   
>   	if (!rate && !type)
>   		memset(ffi, 0, sizeof(struct f2fs_fault_info));
> +	else
> +		f2fs_info(sbi,
> +			"build fault injection attr: rate: %lu, type: 0x%lx",
> +								rate, type);
> +	return 0;
>   }
>   #endif
>   
> @@ -673,14 +683,17 @@ static int parse_options(struct super_block *sb, char *options)
>   		case Opt_fault_injection:
>   			if (args->from && match_int(args, &arg))
>   				return -EINVAL;
> -			f2fs_build_fault_attr(sbi, arg, F2FS_ALL_FAULT_TYPE);
> +			if (f2fs_build_fault_attr(sbi, arg,
> +					F2FS_ALL_FAULT_TYPE))
> +				return -EINVAL;
>   			set_opt(sbi, FAULT_INJECTION);
>   			break;
>   
>   		case Opt_fault_type:
>   			if (args->from && match_int(args, &arg))
>   				return -EINVAL;
> -			f2fs_build_fault_attr(sbi, 0, arg);
> +			if (f2fs_build_fault_attr(sbi, 0, arg))
> +				return -EINVAL;
>   			set_opt(sbi, FAULT_INJECTION);
>   			break;
>   #else
> diff --git a/fs/f2fs/sysfs.c b/fs/f2fs/sysfs.c
> index 84bb37665093..5e4d58d2b7eb 100644
> --- a/fs/f2fs/sysfs.c
> +++ b/fs/f2fs/sysfs.c
> @@ -254,10 +254,16 @@ static ssize_t __sbi_store(struct f2fs_attr *a,
>   	if (ret < 0)
>   		return ret;
>   #ifdef CONFIG_F2FS_FAULT_INJECTION
> -	if (a->struct_type == FAULT_INFO_TYPE && t >= (1 << FAULT_MAX))
> -		return -EINVAL;
> -	if (a->struct_type == FAULT_INFO_RATE && t >= UINT_MAX)
> -		return -EINVAL;
> +	if (a->struct_type == FAULT_INFO_TYPE) {
> +		if (f2fs_build_fault_attr(sbi, 0, t))
> +			return -EINVAL;
> +		return count;
> +	}
> +	if (a->struct_type == FAULT_INFO_RATE) {
> +		if (f2fs_build_fault_attr(sbi, t, 0))
> +			return -EINVAL;
> +		return count;
> +	}
>   #endif
>   	if (a->struct_type == RESERVED_BLOCKS) {
>   		spin_lock(&sbi->stat_lock);

-- 
- Stefan

-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_0xE8675DEECBEECEA3.asc
Type: application/pgp-keys
Size: 48643 bytes
Desc: OpenPGP public key
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20240826/54b76bb9/attachment-0001.key>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20240826/54b76bb9/attachment-0001.sig>