[PATCH] btrfs: Avoid potential integer overflow when left-shifting 32-bit int

[PATCH] btrfs: Avoid potential integer overflow when left-shifting 32-bit int

[Nur Hussein]

In scrub_stripe(), the 32-bit signed value returned by the
nr_data_stripes(map) function call should be cast to u64
before being shifted left by BTRFS_STRIPE_LEN_SHIFT (16),
as a cautionary measure to avoid potential overflows. We
then assign it to a u64 value anyway, so a cast before a
shift seems prudent.

Signed-off-by: Nur Hussein <hussein@unixcat.org>
---
 fs/btrfs/scrub.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
index ccb4f58ae307..4de1665fcd52 100644
--- a/fs/btrfs/scrub.c
+++ b/fs/btrfs/scrub.c
@@ -2187,7 +2187,7 @@ static noinline_for_stack int scrub_stripe(struct scrub_ctx *sctx,
 
 	/* Initialize @offset in case we need to go to out: label */
 	get_raid56_logic_offset(physical, stripe_index, map, &offset, NULL);
-	increment = nr_data_stripes(map) << BTRFS_STRIPE_LEN_SHIFT;
+	increment = (u64)nr_data_stripes(map) << BTRFS_STRIPE_LEN_SHIFT;
 
 	/*
 	 * Due to the rotation, for RAID56 it's better to iterate each stripe
-- 
2.34.1

Re: [PATCH] btrfs: Avoid potential integer overflow when left-shifting 32-bit int

[Qu Wenruo]

On 2023/4/7 03:24, Nur Hussein wrote:
> In scrub_stripe(), the 32-bit signed value returned by the
> nr_data_stripes(map) function call should be cast to u64
> before being shifted left by BTRFS_STRIPE_LEN_SHIFT (16),
> as a cautionary measure to avoid potential overflows. We
> then assign it to a u64 value anyway, so a cast before a
> shift seems prudent.

I'd say it's a little overkilled.

For nr_data_stripes(), it's at most hundreds of stripes (which is 
already insane).
Even with 16 bits left shift, we need to get 2 ** 16 stripes to overflow 
32bits.

Thanks,
Qu
> 
> Signed-off-by: Nur Hussein <hussein@unixcat.org>
> ---
>   fs/btrfs/scrub.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
> index ccb4f58ae307..4de1665fcd52 100644
> --- a/fs/btrfs/scrub.c
> +++ b/fs/btrfs/scrub.c
> @@ -2187,7 +2187,7 @@ static noinline_for_stack int scrub_stripe(struct scrub_ctx *sctx,
>   
>   	/* Initialize @offset in case we need to go to out: label */
>   	get_raid56_logic_offset(physical, stripe_index, map, &offset, NULL);
> -	increment = nr_data_stripes(map) << BTRFS_STRIPE_LEN_SHIFT;
> +	increment = (u64)nr_data_stripes(map) << BTRFS_STRIPE_LEN_SHIFT;
>   
>   	/*
>   	 * Due to the rotation, for RAID56 it's better to iterate each stripe

Re: [PATCH] btrfs: Avoid potential integer overflow when left-shifting 32-bit int

[Nur Hussein]

On Fri, Apr 07, 2023 at 08:35:40AM +0800, Qu Wenruo wrote:
> 
> 
> On 2023/4/7 03:24, Nur Hussein wrote:
> > In scrub_stripe(), the 32-bit signed value returned by the
> > nr_data_stripes(map) function call should be cast to u64
> > before being shifted left by BTRFS_STRIPE_LEN_SHIFT (16),
> > as a cautionary measure to avoid potential overflows. We
> > then assign it to a u64 value anyway, so a cast before a
> > shift seems prudent.
> 
> I'd say it's a little overkilled.
> 
> For nr_data_stripes(), it's at most hundreds of stripes (which is already
> insane).
> Even with 16 bits left shift, we need to get 2 ** 16 stripes to overflow
> 32bits.

Perhaps so, but it was flagged by Coverity, and it's a little safer with
the cast, with no cost. It's up to y'all if you want it though.

- Nur

Re: [PATCH] btrfs: Avoid potential integer overflow when left-shifting 32-bit int

[David Sterba]

On Fri, Apr 07, 2023 at 08:35:40AM +0800, Qu Wenruo wrote:
> On 2023/4/7 03:24, Nur Hussein wrote:
> > In scrub_stripe(), the 32-bit signed value returned by the
> > nr_data_stripes(map) function call should be cast to u64
> > before being shifted left by BTRFS_STRIPE_LEN_SHIFT (16),
> > as a cautionary measure to avoid potential overflows. We
> > then assign it to a u64 value anyway, so a cast before a
> > shift seems prudent.
> 
> I'd say it's a little overkilled.
> 
> For nr_data_stripes(), it's at most hundreds of stripes (which is 
> already insane).
> Even with 16 bits left shift, we need to get 2 ** 16 stripes to overflow 
> 32bits.

I don't like adding casts unless really necessary. That the values won't
overflow is what we know because there are other constraints.

In this case I'd rather switch the return type of nr_data_stripes to u32
as the return value from the helper is used for shifts by
BTRFS_STRIPE_LEN_SHIFT in several places.

For the struct map_lookup we should use u32 for all values that simply
count something and there's no semantic value for -1 (like uninitialzed
or invalid). I did a quick grep and the values are assigned from
unsigned types in most if not all cases.