[PATCH v6 0/2] badblocks: fix infinite loop and validate sector range/shift

[PATCH v6 0/2] badblocks: fix infinite loop and validate sector range/shift

[Ramesh Adhikari]

This replaces the single-patch v5 with a two-patch series, per Coly's
review.

v1-v4 chased symptoms of the same underlying bug (an RCU stall found
by syzkaller through the nvdimm ioctl path, in _badblocks_check() /
badblocks_check() looping with a non-advancing range) before landing
on the root cause in v4: rounddown()/roundup() don't modify their
argument in place, so 's'/'next'/'target' were never actually
rounded.

v5 folded the round_down()/round_up() fix together with overflow and
zero-length guards into one patch. Coly's review on v5 pointed out
that:

  - the overflow check that was there wasn't sufficient on its own
    (only one of several range-validity conditions), and
  - the round fix and the validation should be separate patches,
    since they're independently useful and one is safe to backport
    on its own.

This series:

  1/2 is exactly the round_down()/round_up() fix, nothing else. This
      is what actually stops the infinite loop and also fixes the
      32-bit build breakage kernel test robot reported on v1
      (rounddown()/roundup() do 64-bit division/modulo on sector_t,
      requiring libgcc helpers not linked into the kernel).

  2/2 adds the range/shift validation Coly asked for: s+sectors
      overflow, bb->shift too large to shift a sector_t by (bb->shift
      is populated in drivers/md/md.c straight from an unvalidated
      on-disk superblock byte), and detecting when round_up()/
      round_down() themselves wrap near ULLONG_MAX.

Both are tagged Fixes: aa511ff8218b ("badblocks: switch to the
improved badblock handling code") and Cc: stable, since that's the
commit that introduced this code path.

Ramesh Adhikari (2):
  badblocks: fix in-place round_up/round_down usage bug
  badblocks: validate sector range and shift before rounding

 block/badblocks.c | 52 ++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 45 insertions(+), 7 deletions(-)

--
2.43.0

[PATCH v6 1/2] badblocks: fix in-place round_up/round_down usage bug

[Ramesh Adhikari]

rounddown() and roundup() do not modify their first argument in
place; they return the rounded value. _badblocks_set(),
_badblocks_clear() and badblocks_check() were calling them as
bare statements and discarding the result, so 's' (and 'next'/
'target') were never actually rounded. Depending on the caller's
alignment this can leave 'sectors' unchanged or, in the reported
case, produce a range whose end never advances, causing
_badblocks_check()/badblocks_check() to loop with a non-advancing
cursor and stall the CPU (RCU stall) when called through the
nvdimm ioctl path via nvdimm_clear_badblocks_region().

rounddown()/roundup() also do division/modulo on the sector_t
(u64) operand, which requires libgcc helpers (__aeabi_uldivmod,
__umoddi3) that are not linked into the kernel on 32-bit builds,
breaking the build on arm/i386 (reported by kernel test robot).

Switch to round_down()/round_up() (include/linux/math.h), which
are mask-based, assign their result back to the variable being
rounded, and require no 64-bit division, fixing both the
non-rounding bug and the 32-bit build breakage.

Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202604301231.IpPh4AiH-lkp@intel.com/
Fixes: aa511ff8218b ("badblocks: switch to the improved badblock handling code")
Cc: stable@vger.kernel.org
Signed-off-by: Ramesh Adhikari <adhikari.resume@gmail.com>
---
 block/badblocks.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/block/badblocks.c b/block/badblocks.c
index ece64e76fe8..1f786b193fb 100644
--- a/block/badblocks.c
+++ b/block/badblocks.c
@@ -857,8 +857,8 @@ static bool _badblocks_set(struct badblocks *bb, sector_t s, sector_t sectors,
 		/* round the start down, and the end up */
 		sector_t next = s + sectors;
 
-		rounddown(s, 1 << bb->shift);
-		roundup(next, 1 << bb->shift);
+		s = round_down(s, 1 << bb->shift);
+		next = round_up(next, 1 << bb->shift);
 		sectors = next - s;
 	}
 
@@ -1071,8 +1071,8 @@ static bool _badblocks_clear(struct badblocks *bb, sector_t s, sector_t sectors)
 		 * isn't than to think a block is not bad when it is.
 		 */
 		target = s + sectors;
-		roundup(s, 1 << bb->shift);
-		rounddown(target, 1 << bb->shift);
+		s = round_up(s, 1 << bb->shift);
+		target = round_down(target, 1 << bb->shift);
 		sectors = target - s;
 	}
 
@@ -1307,8 +1307,8 @@ int badblocks_check(struct badblocks *bb, sector_t s, sector_t sectors,
 		/* round the start down, and the end up */
 		sector_t target = s + sectors;
 
-		rounddown(s, 1 << bb->shift);
-		roundup(target, 1 << bb->shift);
+		s = round_down(s, 1 << bb->shift);
+		target = round_up(target, 1 << bb->shift);
 		sectors = target - s;
 	}
 
-- 
2.43.0

Re: [PATCH v6 1/2] badblocks: fix in-place round_up/round_down usage bug

[Coly Li]

On Thu, Jul 09, 2026 at 06:49:03PM +0800, Ramesh Adhikari wrote:
> rounddown() and roundup() do not modify their first argument in
> place; they return the rounded value. _badblocks_set(),
> _badblocks_clear() and badblocks_check() were calling them as
> bare statements and discarding the result, so 's' (and 'next'/
> 'target') were never actually rounded. Depending on the caller's
> alignment this can leave 'sectors' unchanged or, in the reported
> case, produce a range whose end never advances, causing
> _badblocks_check()/badblocks_check() to loop with a non-advancing
> cursor and stall the CPU (RCU stall) when called through the
> nvdimm ioctl path via nvdimm_clear_badblocks_region().
> 
> rounddown()/roundup() also do division/modulo on the sector_t
> (u64) operand, which requires libgcc helpers (__aeabi_uldivmod,
> __umoddi3) that are not linked into the kernel on 32-bit builds,
> breaking the build on arm/i386 (reported by kernel test robot).
> 
> Switch to round_down()/round_up() (include/linux/math.h), which
> are mask-based, assign their result back to the variable being
> rounded, and require no 64-bit division, fixing both the
> non-rounding bug and the 32-bit build breakage.
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202604301231.IpPh4AiH-lkp@intel.com/
> Fixes: aa511ff8218b ("badblocks: switch to the improved badblock handling code")
> Cc: stable@vger.kernel.org
> Signed-off-by: Ramesh Adhikari <adhikari.resume@gmail.com>

It looks good to me.

Reviewed-by: Coly Li <colyli@fygo.io>

Thanks.

Coly Li

> ---
>  block/badblocks.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/block/badblocks.c b/block/badblocks.c
> index ece64e76fe8..1f786b193fb 100644
> --- a/block/badblocks.c
> +++ b/block/badblocks.c
> @@ -857,8 +857,8 @@ static bool _badblocks_set(struct badblocks *bb, sector_t s, sector_t sectors,
>  		/* round the start down, and the end up */
>  		sector_t next = s + sectors;
>  
> -		rounddown(s, 1 << bb->shift);
> -		roundup(next, 1 << bb->shift);
> +		s = round_down(s, 1 << bb->shift);
> +		next = round_up(next, 1 << bb->shift);
>  		sectors = next - s;
>  	}
>  
> @@ -1071,8 +1071,8 @@ static bool _badblocks_clear(struct badblocks *bb, sector_t s, sector_t sectors)
>  		 * isn't than to think a block is not bad when it is.
>  		 */
>  		target = s + sectors;
> -		roundup(s, 1 << bb->shift);
> -		rounddown(target, 1 << bb->shift);
> +		s = round_up(s, 1 << bb->shift);
> +		target = round_down(target, 1 << bb->shift);
>  		sectors = target - s;
>  	}
>  
> @@ -1307,8 +1307,8 @@ int badblocks_check(struct badblocks *bb, sector_t s, sector_t sectors,
>  		/* round the start down, and the end up */
>  		sector_t target = s + sectors;
>  
> -		rounddown(s, 1 << bb->shift);
> -		roundup(target, 1 << bb->shift);
> +		s = round_down(s, 1 << bb->shift);
> +		target = round_up(target, 1 << bb->shift);
>  		sectors = target - s;
>  	}
>  
> -- 
> 2.43.0

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

[Ramesh Adhikari]

_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 <colyli@fygo.io>
Fixes: aa511ff8218b ("badblocks: switch to the improved badblock handling code")
Cc: stable@vger.kernel.org
Signed-off-by: Ramesh Adhikari <adhikari.resume@gmail.com>
---
 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;
+
 	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;
+
+		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;
+
 	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;
+
 		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;
+
 	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;
+
+		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;
 	}
 
-- 
2.43.0

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

[Coly Li]

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 <colyli@fygo.io>
> Fixes: aa511ff8218b ("badblocks: switch to the improved badblock handling code")
> Cc: stable@vger.kernel.org
> Signed-off-by: Ramesh Adhikari <adhikari.resume@gmail.com>
> ---
>  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

[PATCH v7 0/2] badblocks: fix rounding bug and validate input range

[Ramesh Adhikari]

Two related fixes to block/badblocks.c:

Patch 1/2 fixes the actual reported bug: round_up()/round_down() don't
modify their argument in place, they return the rounded value, and the
callers in _badblocks_set()/_badblocks_clear()/badblocks_check() were
discarding that return value. Depending on caller alignment this could
leave sectors unrounded or, in the reported case, stall the CPU with a
non-advancing cursor when reached via the nvdimm ioctl path. It also
fixes a 32-bit build breakage reported by kernel test robot, since the
old rounddown()/roundup() macros pull in 64-bit division helpers not
linked on 32-bit builds. Unchanged since v6.

Patch 2/2 hardens the same three functions against the input-range and
shift edge cases the round_up/round_down fix exposed: s + sectors
overflow, and rounding itself wrapping past ULLONG_MAX. Revised in v7
per Coly's review of v6 — see the changelog in that patch for details.
The remaining open item, bounding bb->shift where it's populated from
the on-disk MD superblock in drivers/md/md.c, is being sent separately
since it's a different file and a different root cause (missing input
validation, not a rounding bug).

Ramesh Adhikari (2):
  badblocks: fix in-place round_up/round_down usage bug
  badblocks: validate sector range and shift before rounding

 block/badblocks.c         | 40 ++++++++++++++++++++++++++++++++-------
 include/linux/badblocks.h |  6 +++++-
 2 files changed, 38 insertions(+), 8 deletions(-)

-- 
2.43.0

[PATCH v7 1/2] badblocks: fix in-place round_up/round_down usage bug

[Ramesh Adhikari]

rounddown() and roundup() do not modify their first argument in
place; they return the rounded value. _badblocks_set(),
_badblocks_clear() and badblocks_check() were calling them as
bare statements and discarding the result, so 's' (and 'next'/
'target') were never actually rounded. Depending on the caller's
alignment this can leave 'sectors' unchanged or, in the reported
case, produce a range whose end never advances, causing
_badblocks_check()/badblocks_check() to loop with a non-advancing
cursor and stall the CPU (RCU stall) when called through the
nvdimm ioctl path via nvdimm_clear_badblocks_region().

rounddown()/roundup() also do division/modulo on the sector_t
(u64) operand, which requires libgcc helpers (__aeabi_uldivmod,
__umoddi3) that are not linked into the kernel on 32-bit builds,
breaking the build on arm/i386 (reported by kernel test robot).

Switch to round_down()/round_up() (include/linux/math.h), which
are mask-based, assign their result back to the variable being
rounded, and require no 64-bit division, fixing both the
non-rounding bug and the 32-bit build breakage.

Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202604301231.IpPh4AiH-lkp@intel.com/
Fixes: aa511ff8218b ("badblocks: switch to the improved badblock handling code")
Cc: stable@vger.kernel.org
Signed-off-by: Ramesh Adhikari <adhikari.resume@gmail.com>
Reviewed-by: Coly Li <colyli@fygo.io>
---
 block/badblocks.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/block/badblocks.c b/block/badblocks.c
index ece64e76fe8..1f786b193fb 100644
--- a/block/badblocks.c
+++ b/block/badblocks.c
@@ -857,8 +857,8 @@ static bool _badblocks_set(struct badblocks *bb, sector_t s, sector_t sectors,
 		/* round the start down, and the end up */
 		sector_t next = s + sectors;
 
-		rounddown(s, 1 << bb->shift);
-		roundup(next, 1 << bb->shift);
+		s = round_down(s, 1 << bb->shift);
+		next = round_up(next, 1 << bb->shift);
 		sectors = next - s;
 	}
 
@@ -1071,8 +1071,8 @@ static bool _badblocks_clear(struct badblocks *bb, sector_t s, sector_t sectors)
 		 * isn't than to think a block is not bad when it is.
 		 */
 		target = s + sectors;
-		roundup(s, 1 << bb->shift);
-		rounddown(target, 1 << bb->shift);
+		s = round_up(s, 1 << bb->shift);
+		target = round_down(target, 1 << bb->shift);
 		sectors = target - s;
 	}
 
@@ -1307,8 +1307,8 @@ int badblocks_check(struct badblocks *bb, sector_t s, sector_t sectors,
 		/* round the start down, and the end up */
 		sector_t target = s + sectors;
 
-		rounddown(s, 1 << bb->shift);
-		roundup(target, 1 << bb->shift);
+		s = round_down(s, 1 << bb->shift);
+		target = round_up(target, 1 << bb->shift);
 		sectors = target - s;
 	}
 
-- 
2.43.0

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

[Ramesh Adhikari]

_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 and 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.

badblocks.c does not itself bound bb->shift: every caller except
drivers/md/md.c always leaves it at 0, so the one caller that
populates it from untrusted on-disk data is responsible for
bounding it before assigning it, per struct badblocks's shift
field documentation in include/linux/badblocks.h. That md.c-side
bound is being sent as a separate patch.

badblocks_check() returns 0 rather than -EINVAL on the wrap case,
matching its existing "0: no known bad blocks in the range"
return convention instead of introducing a new error path callers
don't expect.

Suggested-by: Coly Li <colyli@fygo.io>
Fixes: aa511ff8218b ("badblocks: switch to the improved badblock handling code")
Cc: stable@vger.kernel.org
Signed-off-by: Ramesh Adhikari <adhikari.resume@gmail.com>
---
Changes in v7 (per Coly Li's review of v6):
 - Simplify the overflow check in all three call sites from
   "s > ULLONG_MAX - sectors" to "(s + sectors) < s".
 - Drop the "bb->shift >= BITS_PER_LONG_LONG" guards in badblocks.c.
   Only drivers/md/md.c ever sets a nonzero bb->shift, so the bound
   belongs where bblog_shift is read off the on-disk superblock, not
   scattered through the badblocks API. Documented the caller
   requirement on struct badblocks.shift in badblocks.h instead; the
   md.c-side bound follows as a separate patch.
 - badblocks_check() now returns 0 instead of -EINVAL on the wrap
   case, consistent with its existing return convention.

 block/badblocks.c         | 40 ++++++++++++++++++++++++++++++++-------
 include/linux/badblocks.h |  6 +++++-
 2 files changed, 38 insertions(+), 8 deletions(-)

diff --git a/block/badblocks.c b/block/badblocks.c
index 1f786b193fb..728b59a1a5d 100644
--- a/block/badblocks.c
+++ b/block/badblocks.c
@@ -853,12 +853,19 @@ static bool _badblocks_set(struct badblocks *bb, sector_t s, sector_t sectors,
 		/* Invalid sectors number */
 		return false;
 
+	if ((s + sectors) < s)
+		/* Range wraps past the end of sector_t */
+		return false;
+
 	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);
+		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 +1068,12 @@ static bool _badblocks_clear(struct badblocks *bb, sector_t s, sector_t sectors)
 		/* Invalid sectors number */
 		return false;
 
+	if ((s + sectors) < s)
+		/* Range wraps past the end of sector_t */
+		return false;
+
 	if (bb->shift) {
+		sector_t orig_s = s;
 		sector_t target;
 
 		/* When clearing we round the start up and the end down.
@@ -1071,9 +1083,16 @@ static bool _badblocks_clear(struct badblocks *bb, sector_t s, sector_t sectors)
 		 * isn't than to think a block is not bad when it is.
 		 */
 		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 +1322,19 @@ int badblocks_check(struct badblocks *bb, sector_t s, sector_t sectors,
 
 	WARN_ON(bb->shift < 0 || sectors == 0);
 
+	if ((s + sectors) < s)
+		/* Range wraps past the end of sector_t */
+		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);
+		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;
 	}
 
diff --git a/include/linux/badblocks.h b/include/linux/badblocks.h
index 996493917f3..5d88992b55a 100644
--- a/include/linux/badblocks.h
+++ b/include/linux/badblocks.h
@@ -34,7 +34,11 @@ struct badblocks {
 				 */
 	int shift;		/* shift from sectors to block size
 				 * a -ve shift means badblocks are
-				 * disabled.*/
+				 * disabled. Callers that set this from
+				 * untrusted/on-disk data are responsible
+				 * for bounding it so 1 << shift does not
+				 * overflow a sector_t.
+				 */
 	u64 *page;		/* badblock list */
 	int changed;
 	seqlock_t lock;
-- 
2.43.0

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

[Coly Li]

On Tue, Jul 21, 2026 at 10:10:24PM +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 and 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.
> 
> badblocks.c does not itself bound bb->shift: every caller except
> drivers/md/md.c always leaves it at 0, so the one caller that
> populates it from untrusted on-disk data is responsible for
> bounding it before assigning it, per struct badblocks's shift
> field documentation in include/linux/badblocks.h. That md.c-side
> bound is being sent as a separate patch.
> 
> badblocks_check() returns 0 rather than -EINVAL on the wrap case,
> matching its existing "0: no known bad blocks in the range"
> return convention instead of introducing a new error path callers
> don't expect.
> 
> Suggested-by: Coly Li <colyli@fygo.io>
> Fixes: aa511ff8218b ("badblocks: switch to the improved badblock handling code")
> Cc: stable@vger.kernel.org
> Signed-off-by: Ramesh Adhikari <adhikari.resume@gmail.com>

The patch looks good to me, thanks.

Reviewed-by: Coly Li <colyli@fygo.io>

BTW, the shift overflow checking in md raid code is accepted by
md maintainer, so you may submit these 2 patches with my Reviewed-by
to Jens. That's enough.

Coly Li

> ---
> Changes in v7 (per Coly Li's review of v6):
>  - Simplify the overflow check in all three call sites from
>    "s > ULLONG_MAX - sectors" to "(s + sectors) < s".
>  - Drop the "bb->shift >= BITS_PER_LONG_LONG" guards in badblocks.c.
>    Only drivers/md/md.c ever sets a nonzero bb->shift, so the bound
>    belongs where bblog_shift is read off the on-disk superblock, not
>    scattered through the badblocks API. Documented the caller
>    requirement on struct badblocks.shift in badblocks.h instead; the
>    md.c-side bound follows as a separate patch.
>  - badblocks_check() now returns 0 instead of -EINVAL on the wrap
>    case, consistent with its existing return convention.
> 
>  block/badblocks.c         | 40 ++++++++++++++++++++++++++++++++-------
>  include/linux/badblocks.h |  6 +++++-
>  2 files changed, 38 insertions(+), 8 deletions(-)
> 
> diff --git a/block/badblocks.c b/block/badblocks.c
> index 1f786b193fb..728b59a1a5d 100644
> --- a/block/badblocks.c
> +++ b/block/badblocks.c
> @@ -853,12 +853,19 @@ static bool _badblocks_set(struct badblocks *bb, sector_t s, sector_t sectors,
>  		/* Invalid sectors number */
>  		return false;
>  
> +	if ((s + sectors) < s)
> +		/* Range wraps past the end of sector_t */
> +		return false;
> +
>  	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);
> +		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 +1068,12 @@ static bool _badblocks_clear(struct badblocks *bb, sector_t s, sector_t sectors)
>  		/* Invalid sectors number */
>  		return false;
>  
> +	if ((s + sectors) < s)
> +		/* Range wraps past the end of sector_t */
> +		return false;
> +
>  	if (bb->shift) {
> +		sector_t orig_s = s;
>  		sector_t target;
>  
>  		/* When clearing we round the start up and the end down.
> @@ -1071,9 +1083,16 @@ static bool _badblocks_clear(struct badblocks *bb, sector_t s, sector_t sectors)
>  		 * isn't than to think a block is not bad when it is.
>  		 */
>  		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 +1322,19 @@ int badblocks_check(struct badblocks *bb, sector_t s, sector_t sectors,
>  
>  	WARN_ON(bb->shift < 0 || sectors == 0);
>  
> +	if ((s + sectors) < s)
> +		/* Range wraps past the end of sector_t */
> +		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);
> +		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;
>  	}
>  
> diff --git a/include/linux/badblocks.h b/include/linux/badblocks.h
> index 996493917f3..5d88992b55a 100644
> --- a/include/linux/badblocks.h
> +++ b/include/linux/badblocks.h
> @@ -34,7 +34,11 @@ struct badblocks {
>  				 */
>  	int shift;		/* shift from sectors to block size
>  				 * a -ve shift means badblocks are
> -				 * disabled.*/
> +				 * disabled. Callers that set this from
> +				 * untrusted/on-disk data are responsible
> +				 * for bounding it so 1 << shift does not
> +				 * overflow a sector_t.
> +				 */
>  	u64 *page;		/* badblock list */
>  	int changed;
>  	seqlock_t lock;
> -- 
> 2.43.0

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

[Coly Li]

> 2026年8月6日 15:22，Coly Li <colyli@fygo.io> 写道：
> 
> On Tue, Jul 21, 2026 at 10:10:24PM +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 and 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.
>> 
>> badblocks.c does not itself bound bb->shift: every caller except
>> drivers/md/md.c always leaves it at 0, so the one caller that
>> populates it from untrusted on-disk data is responsible for
>> bounding it before assigning it, per struct badblocks's shift
>> field documentation in include/linux/badblocks.h. That md.c-side
>> bound is being sent as a separate patch.
>> 
>> badblocks_check() returns 0 rather than -EINVAL on the wrap case,
>> matching its existing "0: no known bad blocks in the range"
>> return convention instead of introducing a new error path callers
>> don't expect.
>> 
>> Suggested-by: Coly Li <colyli@fygo.io>
>> Fixes: aa511ff8218b ("badblocks: switch to the improved badblock handling code")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Ramesh Adhikari <adhikari.resume@gmail.com>
> 
> The patch looks good to me, thanks.
> 
> Reviewed-by: Coly Li <colyli@fygo.io>
> 
> BTW, the shift overflow checking in md raid code is accepted by
> md maintainer, so you may submit these 2 patches with my Reviewed-by
> to Jens. That's enough.

Hi Ramesh,

If these patches are not merged yet, I will submit them with my other bcache patches to Jens
later. It might take some time, but won’t be forgot.

Thanks.

Coly Li

Re: [PATCH v7 0/2] badblocks: fix rounding bug and validate input range

[Ramesh Adhikari]

Hi Jens, Coly,

Gentle ping on this series. Patch 1/2 has Coly's Reviewed-by, and
both patches address:

- an RCU stall / kernel hang reachable via the nvdimm ioctl path
  (badblocks_check() looping with a non-advancing cursor)
- a 32-bit (arm/i386) build break from 64-bit division helpers,
  reported by the kernel test robot

Both are tagged Fixes: and Cc: stable, so happy to resend or rebase
if anything's needed on my end  just let me know.

Thanks,
Ramesh

On Tue, Jul 21, 2026 at 10:11 PM Ramesh Adhikari
<adhikari.resume@gmail.com> wrote:
>
> Two related fixes to block/badblocks.c:
>
> Patch 1/2 fixes the actual reported bug: round_up()/round_down() don't
> modify their argument in place, they return the rounded value, and the
> callers in _badblocks_set()/_badblocks_clear()/badblocks_check() were
> discarding that return value. Depending on caller alignment this could
> leave sectors unrounded or, in the reported case, stall the CPU with a
> non-advancing cursor when reached via the nvdimm ioctl path. It also
> fixes a 32-bit build breakage reported by kernel test robot, since the
> old rounddown()/roundup() macros pull in 64-bit division helpers not
> linked on 32-bit builds. Unchanged since v6.
>
> Patch 2/2 hardens the same three functions against the input-range and
> shift edge cases the round_up/round_down fix exposed: s + sectors
> overflow, and rounding itself wrapping past ULLONG_MAX. Revised in v7
> per Coly's review of v6 — see the changelog in that patch for details.
> The remaining open item, bounding bb->shift where it's populated from
> the on-disk MD superblock in drivers/md/md.c, is being sent separately
> since it's a different file and a different root cause (missing input
> validation, not a rounding bug).
>
> Ramesh Adhikari (2):
>   badblocks: fix in-place round_up/round_down usage bug
>   badblocks: validate sector range and shift before rounding
>
>  block/badblocks.c         | 40 ++++++++++++++++++++++++++++++++-------
>  include/linux/badblocks.h |  6 +++++-
>  2 files changed, 38 insertions(+), 8 deletions(-)
>
> --
> 2.43.0
>

Re: [PATCH v7 0/2] badblocks: fix rounding bug and validate input range

[Coly Li]

Hi Ramesh,

I will review the 2nd patch soon.

Thanks.

Coly Li

> 2026年8月4日 13:41，Ramesh Adhikari <adhikari.resume@gmail.com> 写道：
> 
> Hi Jens, Coly,
> 
> Gentle ping on this series. Patch 1/2 has Coly's Reviewed-by, and
> both patches address:
> 
> - an RCU stall / kernel hang reachable via the nvdimm ioctl path
>  (badblocks_check() looping with a non-advancing cursor)
> - a 32-bit (arm/i386) build break from 64-bit division helpers,
>  reported by the kernel test robot
> 
> Both are tagged Fixes: and Cc: stable, so happy to resend or rebase
> if anything's needed on my end  just let me know.
> 
> Thanks,
> Ramesh
> 
> On Tue, Jul 21, 2026 at 10:11 PM Ramesh Adhikari
> <adhikari.resume@gmail.com> wrote:
>> 
>> Two related fixes to block/badblocks.c:
>> 
>> Patch 1/2 fixes the actual reported bug: round_up()/round_down() don't
>> modify their argument in place, they return the rounded value, and the
>> callers in _badblocks_set()/_badblocks_clear()/badblocks_check() were
>> discarding that return value. Depending on caller alignment this could
>> leave sectors unrounded or, in the reported case, stall the CPU with a
>> non-advancing cursor when reached via the nvdimm ioctl path. It also
>> fixes a 32-bit build breakage reported by kernel test robot, since the
>> old rounddown()/roundup() macros pull in 64-bit division helpers not
>> linked on 32-bit builds. Unchanged since v6.
>> 
>> Patch 2/2 hardens the same three functions against the input-range and
>> shift edge cases the round_up/round_down fix exposed: s + sectors
>> overflow, and rounding itself wrapping past ULLONG_MAX. Revised in v7
>> per Coly's review of v6 — see the changelog in that patch for details.
>> The remaining open item, bounding bb->shift where it's populated from
>> the on-disk MD superblock in drivers/md/md.c, is being sent separately
>> since it's a different file and a different root cause (missing input
>> validation, not a rounding bug).
>> 
>> Ramesh Adhikari (2):
>>  badblocks: fix in-place round_up/round_down usage bug
>>  badblocks: validate sector range and shift before rounding
>> 
>> block/badblocks.c         | 40 ++++++++++++++++++++++++++++++++-------
>> include/linux/badblocks.h |  6 +++++-
>> 2 files changed, 38 insertions(+), 8 deletions(-)
>> 
>> --
>> 2.43.0
>>