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

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?

> +			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