Re: [PATCH v2] minix: avoid overflow in bitmap block count calculation

[Jan Kara]

On Thu 18-06-26 10:39:22, Michael Bommarito wrote:
> minix_check_superblock() uses minix_blocks_needed() to verify that the
> on-disk imap and zmap block counts are large enough for the advertised
> inode and zone counts.
> 
> The helper currently performs DIV_ROUND_UP() in unsigned int arithmetic.
> A Minix v3 image can set s_ninodes or s_zones near UINT_MAX so the
> addition inside DIV_ROUND_UP() wraps to zero. That makes a zero imap/zmap
> block count look valid, after which minix_fill_super() can dereference
> s_imap[0] or s_zmap[0] even though no bitmap buffers were allocated.
> 
> Impact: mounting a crafted Minix v3 image whose s_ninodes or s_zones is
> near UINT_MAX makes minix_check_superblock() accept a zero bitmap-block
> count and minix_fill_super() dereference s_imap[0]/s_zmap[0], panicking
> the kernel.
> 
> The divisor is the bitmap capacity in bits, blocksize * 8, which is
> always a power of two: minix_fill_super() obtains the block size through
> sb_set_blocksize(), and blk_validate_block_size() rejects any size that
> is not a power of two. Use DIV_ROUND_UP_POW2(), which divides before
> adding the round-up term and so cannot overflow for a power-of-two
> divisor.
> 
> Fixes: 8c97a6ddc956 ("minix: Add required sanity checking to minix_check_superblock()")
> Assisted-by: Claude:claude-opus-4-8
> Signed-off-by: Michael Bommarito <michael.bommarito@xxxxxxxxx>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@xxxxxxx>

								Honza


> ---
> Changes in v2:
> - Use DIV_ROUND_UP_POW2() instead of widening the helper to 64-bit
>   arithmetic (DIV_ROUND_UP_ULL, v1). The divisor blocksize * 8 is always
>   a power of two, so this is the more obvious fix and keeps the helper's
>   original types, per Jan Kara's review.
> 
> Link to v1: https://lore.kernel.org/all/20260617215740.1116778-1-michael.bommarito@xxxxxxxxx/
> 
> Testing: integer overflow (no sanitizer); the oracle is the downstream
> NULL/ZERO_SIZE_PTR dereference crash.
> 
> Reproduction (UML mount, same crafted image, before/after): a Minix v3
> image is crafted with s_ninodes near UINT_MAX so DIV_ROUND_UP(s_ninodes,
> blocksize * 8) wraps to zero, then mounted from /dev/ubda by a one-line
> init.
>   stock:   RIP: minix_fill_super+0x3f6/0x5c3 ; Kernel panic - not syncing:
>            Kernel mode fault at addr 0x10 (ZERO_SIZE_PTR deref of
>            s_imap[0]); UML exit code 134.
>   patched: "MINIX-fs: file system does not have enough imap blocks
>            allocated. Refusing to mount." / "bad superblock"; mount
>            fails cleanly, no crash.
> 
> Conditions: CONFIG_MINIX_FS=y and an attacker-supplied Minix image is
> mounted (CAP_SYS_ADMIN in the mounting namespace, or a removable-media
> automount path). v3 only: s_ninodes/s_zones are 32-bit; the v1/v2 16-bit
> fields cannot reach the wrap.
> 
> Mitigations: do not mount untrusted Minix images; most distributions do
> not enable CONFIG_MINIX_FS. No sysctl toggle.
> 
> Harness (init script, crafted image, KUnit) available on request.
> 
>  fs/minix/minix.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/minix/minix.h b/fs/minix/minix.h
> index f2025c9b58252..9e52d4302f0d2 100644
> --- a/fs/minix/minix.h
> +++ b/fs/minix/minix.h
> @@ -97,7 +97,7 @@ static inline struct minix_inode_info *minix_i(struct inode *inode)
>  
>  static inline unsigned minix_blocks_needed(unsigned bits, unsigned blocksize)
>  {
> -	return DIV_ROUND_UP(bits, blocksize * 8);
> +	return DIV_ROUND_UP_POW2(bits, blocksize * 8);
>  }
>  
>  #if defined(CONFIG_MINIX_FS_NATIVE_ENDIAN) && \
> -- 
> 2.53.0
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR