Re: [PATCH 4/4] iomap: fix out-of-bounds bitmap_set() with zero-length range

["Darrick J. Wong" <djwong@kernel.org>]

On Fri, May 15, 2026 at 09:50:08AM +0800, Zhang Yi wrote:
> On 5/15/2026 2:10 AM, Darrick J. Wong wrote:
> > On Thu, May 14, 2026 at 02:29:55PM +0800, Zhang Yi wrote:
> >> From: Zhang Yi <yi.zhang@huawei.com>
> >>
> >> ifs_set_range_dirty() and ifs_set_range_uptodate() compute last_blk
> >> as (off + len - 1) >> i_blkbits.  When off is 0 and len is 0, the
> >> unsigned subtraction underflows to SIZE_MAX, producing a huge
> >> last_blk and nr_blks value that causes bitmap_set() to write far
> >> beyond the ifs->state allocation.
> >>
> >> Regarding ifs_set_range_uptodate(), it is temporarily safe because len
> >> cannot be passed in as 0. However, for ifs_set_range_dirty() this is
> >> reachable from __iomap_write_end(): when copy_folio_from_iter_atomic()
> >> returns 0 (e.g. user buffer fault) and the folio is already uptodate,
> >> the guard at the top of __iomap_write_end() does not trigger because
> >> !folio_test_uptodate() is false, and iomap_set_range_dirty() is called
> >> with copied == 0.
> >>
> >> Add a !len guard to both functions before the computation, so that a
> >> zero-length range is a no-op.
> >>
> >> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
> >> ---
> >>  fs/iomap/buffered-io.c | 23 +++++++++++++++--------
> >>  1 file changed, 15 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> >> index 27ab33edbdee..6fe5f7e998fd 100644
> >> --- a/fs/iomap/buffered-io.c
> >> +++ b/fs/iomap/buffered-io.c
> >> @@ -67,11 +67,14 @@ static bool ifs_set_range_uptodate(struct folio *folio,
> >>  		struct iomap_folio_state *ifs, size_t off, size_t len)
> >>  {
> >>  	struct inode *inode = folio->mapping->host;
> >> -	unsigned int first_blk = off >> inode->i_blkbits;
> >> -	unsigned int last_blk = (off + len - 1) >> inode->i_blkbits;
> >> -	unsigned int nr_blks = last_blk - first_blk + 1;
> >> +	unsigned int first_blk, last_blk;
> >>  
> >> -	bitmap_set(ifs->state, first_blk, nr_blks);
> >> +	if (!len)
> >> +		return true;
> >> +
> >> +	first_blk = off >> inode->i_blkbits;
> >> +	last_blk = (off + len - 1) >> inode->i_blkbits;
> >> +	bitmap_set(ifs->state, first_blk, last_blk - first_blk + 1);
> >>  	return ifs_is_fully_uptodate(folio, ifs);
> >>  }
> >>  
> >> @@ -203,13 +206,17 @@ static void ifs_set_range_dirty(struct folio *folio,
> >>  {
> >>  	struct inode *inode = folio->mapping->host;
> >>  	unsigned int blks_per_folio = i_blocks_per_folio(inode, folio);
> >> -	unsigned int first_blk = (off >> inode->i_blkbits);
> >> -	unsigned int last_blk = (off + len - 1) >> inode->i_blkbits;
> >> -	unsigned int nr_blks = last_blk - first_blk + 1;
> >> +	unsigned int first_blk, last_blk;
> >>  	unsigned long flags;
> >>  
> >> +	if (!len)
> >> +		return;
> >> +
> >> +	first_blk = off >> inode->i_blkbits;
> >> +	last_blk = (off + len - 1) >> inode->i_blkbits;
> >>  	spin_lock_irqsave(&ifs->state_lock, flags);
> >> -	bitmap_set(ifs->state, first_blk + blks_per_folio, nr_blks);
> >> +	bitmap_set(ifs->state, first_blk + blks_per_folio,
> >> +		   last_blk - first_blk + 1);
> > 
> > I'm curious about the inconsistency in the computations between
> > ifs_clear_range_dirty and ifs_set_range_dirty now.  In the function that
> > clears dirty bits, off/len are rounded inwards:
> > 
> > 	unsigned int first_blk = round_up(off, i_blocksize(inode)) >>
> > 				 inode->i_blkbits;
> > 	unsigned int last_blk = (off + len) >> inode->i_blkbits;
> > 	unsigned long flags;
> > 
> > 	if (first_blk >= last_blk)
> > 		return;
> > 
> > 	spin_lock_irqsave(&ifs->state_lock, flags);
> > 	bitmap_clear(ifs->state, first_blk + blks_per_folio,
> > 		     last_blk - first_blk);
> > 
> > but here we're still rounding outwards:
> > 
> > 	if (!len)
> > 		return;
> > 
> > 	first_blk = off >> inode->i_blkbits;
> > 	last_blk = (off + len - 1) >> inode->i_blkbits;
> > 	spin_lock_irqsave(&ifs->state_lock, flags);
> > 	bitmap_set(ifs->state, first_blk + blks_per_folio,
> > 		   last_blk - first_blk + 1);
> > 
> > That doesn't quite sound right to me without an explanation in the code,
> > which currently lacks one.  I *think* the reason for the discrepancy is
> > that if we want to dirty part of an fsblock, we need to mark the whole
> > block dirty in the ifs so that all the blocks get written out; but when
> > we're clearing dirty bits, we want to leave an fsblock dirty if we only
> > wrote back part of that fsblock.  Does that sound right?
> > 
> > --D
> > 
> 
> Yes, that's right. The primary purpose of clearing the dirty bit is for
> invalidating a partial folio(e.g., when punching hole). Only when a full
> fsblock has been invalidated can its corresponding dirty bit be cleared.
> Write-back operations never seem to write back only part of a fsblock.
> 
> I can add comments for them in my next iteration.

Sounds good, thank you.

(Does this patch also need a fixes tag like Joanne asked?)

--D

> Thanks,
> Yi.
> 
> 
>