| Message ID | 20230406192406.2300379-1-hussein@unixcat.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: Avoid potential integer overflow when left-shifting 32-bit int | expand |
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
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
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.
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
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(-)