| Message ID | 20201031010752.23974-3-wqu@suse.com |
|---|---|
| State | Accepted |
| Commit | 3b72612ad191cad29aad3982221ff3355bec798d |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | fs: btrfs: coverity fixes | expand |
On Sat, 31 Oct 2020 09:07:50 +0800 Qu Wenruo <wqu@suse.com> wrote: > In real world, this should not cause problem as we have device number > limit thus it won't go beyond 4G for a single stripe. So we can't run into this overflow in U-Boot because only one device is supported? But in Linux we can run into this issue?
On 2020/11/2 上午7:02, Marek Behun wrote: > On Sat, 31 Oct 2020 09:07:50 +0800 > Qu Wenruo <wqu@suse.com> wrote: > >> In real world, this should not cause problem as we have device number >> limit thus it won't go beyond 4G for a single stripe. > > So we can't run into this overflow in U-Boot because only one device is > supported? But in Linux we can run into this issue? > In kernel, we have device number check, IIRC for default nodesize is just several donzens of devices. And since each stripe is only 64K fixed in btrfs, you can see it won't go beyond 4G even in kernel. Thanks, Qu
On 2020/11/2 上午8:20, Qu Wenruo wrote: > > > On 2020/11/2 上午7:02, Marek Behun wrote: >> On Sat, 31 Oct 2020 09:07:50 +0800 >> Qu Wenruo <wqu@suse.com> wrote: >> >>> In real world, this should not cause problem as we have device number >>> limit thus it won't go beyond 4G for a single stripe. >> >> So we can't run into this overflow in U-Boot because only one device is >> supported? But in Linux we can run into this issue? >> > In kernel, we have device number check, IIRC for default nodesize is > just several donzens of devices. > And since each stripe is only 64K fixed in btrfs, you can see it won't > go beyond 4G even in kernel. Sorry, the 64K stripe_len is only for RAID56, and for kernel, we always use u64 for stripe_len, thus we won't hit the problem. The problem is in btrfs-progs, where the code comes from, we use int, other than u64. Thanks for the hint for the root cause, would related values to be u64 for both btrfs-progs and u-boot to follow the kernel scheme. Thanks, Qu > > Thanks, > Qu >
On Sat, Oct 31, 2020 at 09:07:50AM +0800, Qu Wenruo wrote: > In __btrfs_map_block() we do a int * int and assign it to u64. > This is not safe as the result (int * int) is still evaluated as (int) > thus it can overflow. > > Convert one of the multiplier to u64 to prevent such problem. > > In real world, this should not cause problem as we have device number > limit thus it won't go beyond 4G for a single stripe. > > But it's harder to teach coverity about all these hidden limits, so just > fix the possible overflow. > > Reported-by: Coverity CID 312957 > Reported-by: Coverity CID 312948 > Signed-off-by: Qu Wenruo <wqu@suse.com> Applied to u-boot/master, thanks!
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c index fcf52d4b0ff3..4aaaeab663f5 100644 --- a/fs/btrfs/volumes.c +++ b/fs/btrfs/volumes.c @@ -1030,7 +1030,7 @@ again: */ stripe_nr = stripe_nr / map->stripe_len; - stripe_offset = stripe_nr * map->stripe_len; + stripe_offset = stripe_nr * (u64)map->stripe_len; BUG_ON(offset < stripe_offset); /* stripe_offset is the offset of this block in its stripe*/ @@ -1103,7 +1103,7 @@ again: rot = stripe_nr % map->num_stripes; /* Fill in the logical address of each stripe */ - tmp = stripe_nr * nr_data_stripes(map); + tmp = (u64)stripe_nr * nr_data_stripes(map); for (i = 0; i < nr_data_stripes(map); i++) raid_map[(i+rot) % map->num_stripes] =
In __btrfs_map_block() we do a int * int and assign it to u64. This is not safe as the result (int * int) is still evaluated as (int) thus it can overflow. Convert one of the multiplier to u64 to prevent such problem. In real world, this should not cause problem as we have device number limit thus it won't go beyond 4G for a single stripe. But it's harder to teach coverity about all these hidden limits, so just fix the possible overflow. Reported-by: Coverity CID 312957 Reported-by: Coverity CID 312948 Signed-off-by: Qu Wenruo <wqu@suse.com> --- fs/btrfs/volumes.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)