Re: [PATCH v2 1/6] qnx6: validate di_filelevels in qnx6_iget()

[Matthias Goergens]

Hi Hui,

I hit the bugs fixed by 1/6 and 3/6 while fuzzing qnx6, so I tested
your series rather than send my own fixes.  On mainline 40288c9206c1
with v2 1-6 applied:

- KASAN/UBSAN kernel under qemu: the Inode.levels = 6 fuzz image and
  two Longfile.levels = 6 images, one per active-superblock branch, no
  longer trigger the double-brelse warning.  A root inode with
  di_filelevels = 255 is rejected in qnx6_iget() without the UBSAN
  shift reports.

- Userspace fs/qnx6 build under ASan/UBSan: LeakSanitizer no longer
  reports the buffer_head leaks from qnx6_block_map() (2/6) or the
  mmi_fs error path (4/6); bad sb1 magic under SB_SILENT is rejected
  (5/6), and sb_blocksize = 0 no longer divides by zero (6/6).

- Six valid images with the same trees in both superblocks produced
  the same names, sizes and MD5 sums before and after the series.  They
  cover 512-byte and 4K blocks, zero to two indirect levels, either
  active superblock, and normal and MMI layouts.

Feel free to add:

Tested-by: Matthias Goergens <matthias.goergens@xxxxxxxxx>
Reviewed-by: Matthias Goergens <matthias.goergens@xxxxxxxxx>

Two pre-existing problems turned up; neither needs to hold up the
series:

1. qnx6_block_map() shifts a 32-bit block index by ptrbits * depth:
   35 bits for 512-byte blocks at depth 5 and 40 for 4K blocks at
   depth 4.  Both levels are valid, but UBSAN still flags them.  At a
   bit offset of at least 32, the tree-index component is zero; the
   mapper continues with the remaining indices.  Guarding both shifts
   against the index width avoids the undefined shifts without
   rejecting either level.  A test-only u64 cast removes those two
   reports, but the images force high levels onto shallow trees and do
   not test genuine level-4 or level-5 trees.

2. When superblock #2 is newer, qnx6_fill_super() selects it in
   sbi->sb and sbi->sb_buf and releases bh1, but still uses sb1 for the
   Inode and Longfile level checks and root nodes.  Thus it reads #1's
   inode and longfilename trees through a released buffer while
   sbi->sb points to #2.  On an image whose superblocks point at
   different inode trees, old_file appears instead of new_file.
   Setting sb1 = sb2 fixes all four later uses in my tests.

I can send both as follow-ups on top of your series, or you can fold
the shift fix into 1/6.  I'm also happy to share the images.

Thanks,
Matthias