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

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] [day] [month] [year] [list]

Date:   Tue, 2 May 2023 18:11:38 +0200
From:   David Sterba <dsterba@...e.cz>
To:     Qu Wenruo <quwenruo.btrfs@....com>
Cc:     Nur Hussein <hussein@...xcat.org>, clm@...com,
        josef@...icpanda.com, dsterba@...e.com,
        linux-btrfs@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] btrfs: Avoid potential integer overflow when
 left-shifting 32-bit int

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.