Re: [PATCH] md: do overflow check for sb->bblog_shift in super_1_load()

> 2026年7月20日 19:35，[email protected] 写道：
> 
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] The newly added bounds check for `sb->bblog_shift` incorrectly allows values up to 63, failing to prevent 32-bit integer overflows and undefined behavior which can lead to a divide-by-zero kernel panic.
> --
> 
> --- Patch [1]: [PATCH] md: do overflow check for sb->bblog_shift in super_1_load() ---
> commit 36f9915dbd10f1c3803610b32e46390e9cb7ec7c
> Author: Coly Li <[email protected]>
> 
> md: do overflow check for sb->bblog_shift in super_1_load()
> 
> This commit adds a bounds check for sb->bblog_shift in super_1_load() to
> prevent possible overflows in the badblocks API. It limits the shift value
> to be less than BITS_PER_TYPE(sector_t).
> 
>> diff --git a/drivers/md/md.c b/drivers/md/md.c
>> index d1465bcd86c81..67f8edf4db368 100644
>> --- a/drivers/md/md.c
>> +++ b/drivers/md/md.c
>> @@ -1920,6 +1920,13 @@ static int super_1_load(struct md_rdev *rdev, struct md_rdev *refdev, int minor_
>>  rdev->bb_page, REQ_OP_READ, true))
>> return -EIO;
>> bbp = (__le64 *)page_address(rdev->bb_page);
>> +
>> + /* check for badblocks api. */
>> + if (sb->bblog_shift >= BITS_PER_TYPE(sector_t)) {
>                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> [Severity: High]
> This is a pre-existing issue, but does this bounds check still allow values
> that can cause undefined behavior and integer overflows?
> 
> Since BITS_PER_TYPE(sector_t) is 64, this allows sb->bblog_shift to be up
> to 63.
> 
> However, count is declared as a 32-bit signed int in super_1_load(), so the
> following operation:
> 
>    count <<= sb->bblog_shift;
> 
> will cause undefined behavior if the shift is >= 31.
> 
> Additionally, downstream in block/badblocks.c (such as in badblocks_check()
> and _badblocks_set()), the shift is used with a 32-bit literal:
> 
>    rounddown(s, 1 << bb->shift);
> 
> If sb->bblog_shift is >= 32, this can result in undefined behavior and
> yield 0, leading to a divide-by-zero panic in the rounddown() macro during
> array IO.
> 
> Should the maximum shift be restricted to a smaller value?


With an overflow check patch from Ramesh Adhikari <[email protected]>, 
only checking sb->bblog_shift >= BITS_PER_TYPE(sector_t) is enough. The rested overflow
check will be performed inside badblocks API.

And if variables are overflowed before calling badblocks API, this is not the badblocks API
issue, should not belong to the badblocks API check fixes.

Also I don’t feel it is necessary to do restricted overflow checking in super block loading stage,
if the invalid values are rejected, this super block will fail to load and won’t introduce any run time
problem.

Therefore, current checking with (sb->bblog_shift >= BITS_PER_TYPE(sector_t) is sufficient.


> 
>> + pr_err("md: %pg: bogus bblog_shift %u for badblocks.\n",
>> + rdev->bdev, sb->bblog_shift);
>> + return -EINVAL;
>> + }
>> rdev->badblocks.shift = sb->bblog_shift;
>> for (i = 0 ; i < (sectors << (9-3)) ; i++, bbp++) {
>> u64 bb = le64_to_cpu(*bbp);
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1