* Re: [PATCH] minix: validate s_imap_blocks and s_zmap_blocks in minix_check_superblock()
2026-09-19 22:26 [PATCH] minix: validate s_imap_blocks and s_zmap_blocks in minix_check_superblock() Hui Peng
@ 2026-09-22 5:11 ` Jeremy Bingham
0 siblings, 0 replies; 2+ messages in thread
From: Jeremy Bingham @ 2026-09-22 5:11 UTC (permalink / raw)
To: benquike; +Cc: brauner, jack, jlayton, linux-fsdevel, linux-kernel
On Sat, Sep 19, 2026 at 22:26:21 +0000, Hui Peng wrote:
>
> In minix_check_superblock() and minix_fill_super() (fs/minix/inode.c),
> verify that s_imap_blocks and s_zmap_blocks are large enough to cover
> s_ninodes + 1 and s_zones, and check the return value of
> sb_set_blocksize() to prevent out-of-bounds bitmap array reads in
> minix_count_free_inodes() and minix_new_inode().
This issue has already been taken care of with commit fb3e566cafc3,
which changed DIV_ROUND_UP to DIV_ROUND_UP_POW2 to prevent this very
overflow issue. The return value of sb_set_blocksize() isn't actually
checked in this patch, but that's OK because the code was already
checking those return values at both call sites.
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
For what it's worth, this is the very first commit to the Linux git
tree.
> Assisted-by: LLM
Did you review the commit message and patch generated by the LLM? Even
when you use LLM tools to assist with coding tasks, it's important to
make sure their output both makes sense and that it actually does what
it claims.
> Signed-off-by: Hui Peng <benquike@gmail.com>
> ---
> diff --git a/fs/minix/inode.c b/fs/minix/inode.c
> index daf83e4ff25c..b0857dfc1b46 100644
> --- a/fs/minix/inode.c
> +++ b/fs/minix/inode.c
> @@ -185,15 +185,15 @@ static bool minix_check_superblock(struct super_block *sb)
> return false;
> }
>
> - if (sbi->s_ninodes < 1 || sbi->s_firstdatazone <= 4 ||
> - sbi->s_firstdatazone >= sbi->s_nzones)
> + if (sbi->s_ninodes == 0 || sbi->s_ninodes == UINT_MAX ||
> + sbi->s_firstdatazone <= 4 || sbi->s_firstdatazone >= sbi->s_nzones)
> return false;
This is not the right way to check for an overflow here. s_ninodes is an
unsigned long, not an unsigned int, and I wouldn't want to rely on them
having the same overflow. Also, 'sbi->s_ninodes == UINT_MAX' would never
fire for V1/V2 filesystems, since the underlying types for those
versions are u16 and will never reach UINT_MAX, and it would only match
exactly one value for V3 filesystems (but see below).
> /* Apparently minix can create filesystems that allocate more blocks for
> * the bitmaps than needed. We simply ignore that, but verify it didn't
> * create one with not enough blocks and bail out if so.
> */
> - block = minix_blocks_needed(sbi->s_ninodes, sb->s_blocksize);
> + block = minix_blocks_needed((u64)sbi->s_ninodes + 1, sb->s_blocksize);
The u64 cast doesn't add anything. The operands are already unsigned
longs and '(u64)sbi->s_ninodes + 1' would, if sbi->s_ninodes were
UINT_MAX, equal 0x100000000. minix_blocks_needed takes unsigned ints for
its arguments, so that sum would end up being truncated to 0. The
'sbi->s_ninodes == UINT_MAX' guards against this, but the only reason
that guard needs to be there is because of 's_ninodes + 1'.
> if (sbi->s_imap_blocks < block) {
> printk("MINIX-fs: file system does not have enough "
> "imap blocks allocated. Refusing to mount.\n");
> @@ -201,7 +201,7 @@ static bool minix_check_superblock(struct super_block *sb)
> }
>
> block = minix_blocks_needed(
> - (sbi->s_nzones - sbi->s_firstdatazone + 1),
> + (u64)sbi->s_nzones - sbi->s_firstdatazone + 1,
As above, the cast adds nothing.
> sb->s_blocksize);
> if (sbi->s_zmap_blocks < block) {
> printk("MINIX-fs: file system does not have enough "
Note that the pre-image in your index line (daf83e4ff25c) is the current
fs/minix/inode.c in mainline, so this patch was made against a tree that
already contains fb3e566cafc3. In other words, the out-of-bounds reads
the commit message describes are already fixed there. Unfortunately,
this patch gets a NAK from me.
Thanks,
-j
^ permalink raw reply [flat|nested] 2+ messages in thread