lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <87bkg6o4o9.fsf@doe.com>
Date:   Thu, 20 Jul 2023 23:18:06 +0530
From:   Ritesh Harjani (IBM) <ritesh.list@...il.com>
To:     Baokun Li <libaokun1@...wei.com>, linux-ext4@...r.kernel.org
Cc:     tytso@....edu, adilger.kernel@...ger.ca, jack@...e.cz,
        ojaswin@...ux.ibm.com, linux-kernel@...r.kernel.org,
        yi.zhang@...wei.com, yangerkun@...wei.com, yukuai3@...wei.com,
        libaokun1@...wei.com
Subject: Re: [PATCH 1/4] ext4: add two helper functions fex_end() and pa_end()

Baokun Li <libaokun1@...wei.com> writes:

> When we use lstart + len to calculate the end of free extent or prealloc
> space, it may exceed the maximum value of 4294967295(0xffffffff) supported
> by ext4_lblk_t and cause overflow, which may lead to various problems.
>
> Therefore, we add two helper functions, fex_end() and pa_end(), to limit
> the type of end to loff_t, and also convert lstart to loff_t for
> calculation to avoid overflow.
>
> Signed-off-by: Baokun Li <libaokun1@...wei.com>
> ---
>  fs/ext4/mballoc.c |  4 ++--
>  fs/ext4/mballoc.h | 13 +++++++++++++
>  2 files changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index 456150ef6111..eb7f5d35ef96 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -4432,7 +4432,7 @@ ext4_mb_normalize_request(struct ext4_allocation_context *ac,
>  
>  	/* first, let's learn actual file size
>  	 * given current request is allocated */
> -	size = ac->ac_o_ex.fe_logical + EXT4_C2B(sbi, ac->ac_o_ex.fe_len);
> +	size = fex_end(sbi, &ac->ac_o_ex, NULL);
>  	size = size << bsbits;
>  	if (size < i_size_read(ac->ac_inode))
>  		size = i_size_read(ac->ac_inode);
> @@ -5665,7 +5665,7 @@ static void ext4_mb_group_or_file(struct ext4_allocation_context *ac)
>  
>  	group_pa_eligible = sbi->s_mb_group_prealloc > 0;
>  	inode_pa_eligible = true;
> -	size = ac->ac_o_ex.fe_logical + EXT4_C2B(sbi, ac->ac_o_ex.fe_len);
> +	size = fex_end(sbi, &ac->ac_o_ex, NULL);
>  	isize = (i_size_read(ac->ac_inode) + ac->ac_sb->s_blocksize - 1)
>  		>> bsbits;
>  
> diff --git a/fs/ext4/mballoc.h b/fs/ext4/mballoc.h
> index df6b5e7c2274..fe6bf381f30a 100644
> --- a/fs/ext4/mballoc.h
> +++ b/fs/ext4/mballoc.h
> @@ -233,6 +233,19 @@ static inline ext4_fsblk_t ext4_grp_offs_to_block(struct super_block *sb,
>  		(fex->fe_start << EXT4_SB(sb)->s_cluster_bits);
>  }
>  
> +static inline loff_t fex_end(struct ext4_sb_info *sbi,
> +			     struct ext4_free_extent *fex, ext4_grpblk_t *fe_len)
> +{
> +	return (loff_t)fex->fe_logical +
> +		EXT4_C2B(sbi, fe_len ? *fe_len : fex->fe_len);
> +}
> +
> +static inline loff_t pa_end(struct ext4_sb_info *sbi,
> +			    struct ext4_prealloc_space *pa)
> +{
> +	return (loff_t)pa->pa_lstart + EXT4_C2B(sbi, pa->pa_len);
> +}
> +

I didn't find anything else where we could hit any overflow due to
fex->fe_logical + xxx. 
So, looks good to me except these two helper functions.
I think we can directly just typecast those in place. There are many
other places in ext4 code where careful arithmatic of either adding or
bit shifting has been done. These helpers specially fex_end() is
really confusing.

-ritesh

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ