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

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

[colyli]

From: Coly Li <colyli@fygo.io>

In super_1_load(), sb->bblog_shift is an __u8 type value loaded from on-
disk superblock. It is used for badblocks API badblocks_set() by the
following sequence,

 1930   rdev->badblocks.shift = sb->bblog_shift;
 1931   for (i = 0 ; i < (sectors << (9-3)) ; i++, bbp++) {
 1932           u64 bb = le64_to_cpu(*bbp);
 1933           int count = bb & (0x3ff);
 1934           u64 sector = bb >> 10;
 1935           sector <<= sb->bblog_shift;
 1936           count <<= sb->bblog_shift;
 1937           if (bb + 1 == 0)
 1938                   break;
 1939           if (!badblocks_set(&rdev->badblocks, sector, count, 1))
 1940                   return -EINVAL;
 1941   }

bb->bblog_shit is in range of 0-255, variable sector is 64bit width, for
an invalid bb->bblog_shit, it is possible to make sector be overflowed
by the following calculation,
 1935           sector <<= sb->bblog_shift;
Then in turn when call badblocks_set() at line 1939 with the invalid
rdev->badblocks.shift set at line 1930, may result an overflow inside
_badblocks_clear() in block/badblocks.c.

Although there are many places to call badblocks APIs, the non-zero
shift value is only used in super_1_load(), other places always use 0 as
the shift value. Therefore it is unnecessary to do a general shift value
overflow check inside badblock API, and just check here as the caller.

This may avoid unnecessary check, make the badblocks API code more simple
and elegant.

Fixes: 2699b67223aca ("md: load/store badblock list from v1.x metadata")
Fixes: 1726c77467833 ("badblocks: improve badblocks_set() for multiple ranges handling")
Cc: stable@vger.kernel.org
Cc: Ramesh Adhikari <adhikari.resume@gmail.com>
Signed-off-by: Coly Li <colyli@fygo.io>
---
 drivers/md/md.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index d1465bcd86c8..67f8edf4db36 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -1918,10 +1918,17 @@ static int super_1_load(struct md_rdev *rdev, struct md_rdev *refdev, int minor_
 		bb_sector = (long long)offset;
 		if (!sync_page_io(rdev, bb_sector, sectors << 9,
 				  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)) {
+			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);
 			int count = bb & (0x3ff);
 			u64 sector = bb >> 10;
-- 
2.47.3

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

[sashiko-bot]

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 <colyli@fygo.io>

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/20260720111400.2120834-1-colyli@fygo.io?part=1

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

[Coly Li]

> 2026年7月20日 19:35，sashiko-bot@kernel.org 写道：
> 
> 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 <colyli@fygo.io>
> 
> 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 <adhikari.resume@gmail.com>, 
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/20260720111400.2120834-1-colyli@fygo.io?part=1