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

["Coly Li" <colyli@fygo.io>]

> 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