Re: xfs: XLOG_WAS_CONT_TRANS on a transaction with no buffered regions reads ri_buf[-1] - null deref Oops during mount log recovery

[Guikun Yang]

Hi,

One more from the same recovery path - this time in the inode item
handling, a different asymmetry from the two reported above.

## 6. ilf_len is used as the buffer read length without requiring
   whole filesystem blocks

xlog_recover_inode_commit_pass2() passes the logged ilf_len (a basic
block count) straight to xfs_buf_read()
(fs/xfs/xfs_inode_item_recover.c:351):

  error = xfs_buf_read(mp->m_ddev_targp, in_f->ilf_blkno, in_f->ilf_len,
                 0, &bp, &xfs_inode_buf_ops);

Nothing requires ilf_len to cover whole filesystem blocks. The
verifier, however, computes the number of inodes from the buffer
length rounded up (fs/xfs/libxfs/xfs_inode_buf.c):

  ni = XFS_BB_TO_FSB(mp, bp->b_length) * mp->m_sb.sb_inopblock;
  for (i = 0; i < ni; i++) {
          dip = xfs_buf_offset(bp, (i << mp->m_sb.sb_inodelog));
          unlinked_ino = be32_to_cpu(dip->di_next_unlinked);
          ...

With a 512-byte block size on the log and a 4k filesystem block,
ilf_len = 1 reads a single 512-byte buffer (kmalloc-512), the rounding
produces 1 FSB * 8 inodes, and the loop walks 4096 bytes - reading
past the slab from the second inode on:

  BUG: KASAN: slab-out-of-bounds in xfs_inode_buf_verify+...
  Read of size 2 ...
   xfs_inode_buf_verify -> xfs_buf_read -> xlog_recover_inode_commit_pass2
  Allocated by: kmalloc-512 (the buffer from xfs_buf_read)

The readahead path passes ilf_len through unbounded as well
(fs/xfs/xfs_inode_item_recover.c:36,41).

9232301894 ("xfs: bound inode fork length against the fork size during
log recovery") added bounds for the logged fork regions in this same
function, but the buffer read length itself is still unchecked - the
same log record carries both fields, one is now bounded and the other
is not.

## Reproducer

The dirty-log image from a normal crash is modified in five bytes:
ilf_len in the inode log item from 32 to 1, plus the four CRC bytes of
the log record. ilf_blkno and ilf_boffset keep their original values.
The unmodified image recovers and mounts cleanly; the modified one
hits the KASAN report above at mount(2), 3/3.

## Suggested fix

The verifier is the defence layer for every caller, so bounding the
walk by what the buffer actually holds seems most robust:

  ni = XFS_BB_TO_FSB(mp, bp->b_length) * mp->m_sb.sb_inopblock;
  becomes something like
  ni = XFS_BB_TO_FSB(mp, min(bp->b_length, XFS_FSB_TO_BB(mp, 1))) * ...

or equivalently computing ni from the buffer length divided by the
inode size instead of rounding up to a filesystem block. Rejecting
non-block-aligned ilf_len before xfs_buf_read() would also work but
leaves the verifier as-is for other callers.

The reproducer (log scanner, image builder, ready-made image, serial
log) is available on request.

Thanks,
Guikun Yang