Re: [PATCH v6 2/2] badblocks: validate sector range and shift before rounding

On Thu, Jul 09, 2026 at 06:49:04PM +0800, Ramesh Adhikari wrote:
> _badblocks_set(), _badblocks_clear() and badblocks_check() round
> the caller-supplied [s, s+sectors) range to the current bb->shift
> block size before touching the bad block table. That rounding
> was not defensive against a few cases:
> 
> - s + sectors can overflow sector_t (u64), wrapping the range
>   end before it is ever compared against s.
> 
> - bb->shift is a plain 'int' field, populated in one case
>   (drivers/md/md.c, from the on-disk superblock's bblog_shift)
>   straight from an unvalidated byte with no upper bound. Shifting
>   by an amount >= the width of the shifted type is undefined
>   behaviour in C; "1 << bb->shift" was shifting an int literal,
>   so this was already undefined for bb->shift >= 32, let alone
>   the full 0-255 range bblog_shift allows.
> 
> - round_up()/round_down() rounding a value near ULLONG_MAX can
>   itself wrap back to a small value, so even with a valid shift
>   the rounded end of the range could end up smaller than the
>   rounded start, silently turning a small range into a huge one
>   (in _badblocks_clear()/badblocks_check(), which round the end
>   up) or losing the range entirely.
> 
> Add an explicit s+sectors overflow check, reject any bb->shift
> that is too large to shift a sector_t by, cast the shift operand
> to sector_t so the shift itself is never performed on a 32-bit
> int, and detect post-rounding wrap by comparing the rounded
> result back against the pre-rounding value.
> 
> Suggested-by: Coly Li <[email protected]>
> Fixes: aa511ff8218b ("badblocks: switch to the improved badblock handling code")
> Cc: [email protected]
> Signed-off-by: Ramesh Adhikari <[email protected]>
> ---
>  block/badblocks.c | 52 ++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 45 insertions(+), 7 deletions(-)
> 
> diff --git a/block/badblocks.c b/block/badblocks.c
> index 1f786b193fb..00a59729600 100644
> --- a/block/badblocks.c
> +++ b/block/badblocks.c
> @@ -853,12 +853,23 @@ static bool _badblocks_set(struct badblocks *bb, sector_t s, sector_t sectors,
>  		/* Invalid sectors number */
>  		return false;
>  
> +	if (s > ULLONG_MAX - sectors)
> +		/* Range wraps past the end of sector_t */
> +		return false;
> +

Yes, this is necessary. Hmm, if I write this, maybe I will use,
	if ((s + sectors) < s)
		return fasle;
This makes me more comfortable.

>  	if (bb->shift) {
>  		/* round the start down, and the end up */
>  		sector_t next = s + sectors;
>  
> -		s = round_down(s, 1 << bb->shift);
> -		next = round_up(next, 1 << bb->shift);
> +		if (bb->shift >= BITS_PER_LONG_LONG)
> +			/* Corrupt/unsanitised shift value */
> +			return false;

This bb->shift overflow check can be avoided. See my finally comments.

> +
> +		s = round_down(s, (sector_t)1 << bb->shift);
> +		next = round_up(next, (sector_t)1 << bb->shift);
> +		if (next <= s)
> +			/* Rounding wrapped past the end of sector_t */
> +			return false;
>  		sectors = next - s;
>  	}
>  
> @@ -1061,7 +1072,12 @@ static bool _badblocks_clear(struct badblocks *bb, sector_t s, sector_t sectors)
>  		/* Invalid sectors number */
>  		return false;
>  
> +	if (s > ULLONG_MAX - sectors)
> +		/* Range wraps past the end of sector_t */
> +		return false;
> +

same
	if ((s + sectors) < s)
		return fasle;

>  	if (bb->shift) {
> +		sector_t orig_s = s;
>  		sector_t target;
>  
>  		/* When clearing we round the start up and the end down.
> @@ -1070,10 +1086,21 @@ static bool _badblocks_clear(struct badblocks *bb, sector_t s, sector_t sectors)
>  		 * However it is better the think a block is bad when it
>  		 * isn't than to think a block is not bad when it is.
>  		 */
> +		if (bb->shift >= BITS_PER_LONG_LONG)
> +			/* Corrupt/unsanitised shift value */
> +			return false;
> 

This bb->shift overflow check can be avoided. See my finally comments.

>  		target = s + sectors;
> -		s = round_up(s, 1 << bb->shift);
> -		target = round_down(target, 1 << bb->shift);
> -		sectors = target - s;
> +		s = round_up(s, (sector_t)1 << bb->shift);
> +		target = round_down(target, (sector_t)1 << bb->shift);
> +		if (s < orig_s || target < s)
> +			/* Rounding wrapped, or range collapsed */
> +			sectors = 0;
> +		else
> +			sectors = target - s;
> +
> +		if (sectors == 0)
> +			return false;
>  	}
>  
>  	write_seqlock_irq(&bb->lock);
> @@ -1303,12 +1330,23 @@ int badblocks_check(struct badblocks *bb, sector_t s, sector_t sectors,
>  
>  	WARN_ON(bb->shift < 0 || sectors == 0);
>  
> +	if (s > ULLONG_MAX - sectors)
> +		/* Range wraps past the end of sector_t */
> +		return -EINVAL;
> +

same
	if ((s + sectors) < s)
		return fasle;

But I feel returning -EINVAL doesn't follow badblocks_check() api convention,
and has potential risk to break the caller's code.  IMHO it is better to
return 0.


>  	if (bb->shift > 0) {
>  		/* round the start down, and the end up */
>  		sector_t target = s + sectors;
>  
> -		s = round_down(s, 1 << bb->shift);
> -		target = round_up(target, 1 << bb->shift);
> +		if (bb->shift >= BITS_PER_LONG_LONG)
> +			/* Corrupt/unsanitised shift value */
> +			return -EINVAL;
> +
bb->shift checking here might not be necessary. See my finally comments.
And again, returning -EINVAL might be risky here.



> +		s = round_down(s, (sector_t)1 << bb->shift);
> +		target = round_up(target, (sector_t)1 << bb->shift);
> +		if (target <= s)
> +			/* Rounding wrapped past the end of sector_t */
> +			return 0;
>  		sectors = target - s;
>  	}
> 

I feel you can avoid to check bb->shift here. Except for md code, other places
all set bb->shift to 0. So add a code in md.c to check the shift bits when it
is loaded form on-disk super block, that should be enough. and one line
code comment can be added in badblocks API that caller should take the
responsibility to make sure bb->shift won't result overflow.

Thansk.

Coly Li