Re: [PATCH] fs/minix: Improve validity checking of superblock

[Jan Kara]

On Thu 12-10-23 19:07:22, Juntong Deng wrote:
> According to the Minix source code, s_imap_blocks, s_zmap_blocks,
> s_ninodes, s_zones, s_firstdatazone, and s_log_zone_size should
> be checked for validity when reading superblocks.
> 
> The following is the content of minix/fs/mfs/super.c:332
> 
>   /* Make a few basic checks to see if super block looks reasonable. */
>   if (sp->s_imap_blocks < 1 || sp->s_zmap_blocks < 1
> 				|| sp->s_ninodes < 1 || sp->s_zones < 1
> 				|| sp->s_firstdatazone <= 4
> 				|| sp->s_firstdatazone >= sp->s_zones
> 				|| (unsigned) sp->s_log_zone_size > 4) {
> 	printf("not enough imap or zone map blocks, \n");
> 	printf("or not enough inodes, or not enough zones, \n"
> 		"or invalid first data zone, or zone size too large\n");
> 	return(EINVAL);
>   }
> 
> This patch improve the validity checking of superblock based on the
> Minix source code above.
> 
> Since the validity of s_log_zone_size is not currently checked,
> this can lead to errors when s_log_zone_size is subsequently used
> as a shift exponent.
> 
> The following are related bugs reported by Syzbot:
> 
> UBSAN: shift-out-of-bounds in fs/minix/bitmap.c:103:3
> shift exponent 34 is too large for 32-bit type 'unsigned int'
> 
> UBSAN: shift-out-of-bounds in fs/minix/inode.c:380:57
> shift exponent 65510 is too large for 64-bit type 'long unsigned int'
> 
> Reported-by: syzbot+2f142b57f2af27974fda@xxxxxxxxxxxxxxxxxxxxxxxxx
> Closes: https://syzkaller.appspot.com/bug?extid=2f142b57f2af27974fda
> Reported-by: syzbot+5ad0824204c7bf9b67f2@xxxxxxxxxxxxxxxxxxxxxxxxx
> Closes: https://syzkaller.appspot.com/bug?extid=5ad0824204c7bf9b67f2
> Signed-off-by: Juntong Deng <juntong.deng@xxxxxxxxxxx>
> ---
...
> diff --git a/fs/minix/inode.c b/fs/minix/inode.c
> index df575473c1cc..84c2c6e77d1d 100644
> --- a/fs/minix/inode.c
> +++ b/fs/minix/inode.c
> @@ -154,7 +154,11 @@ static bool minix_check_superblock(struct super_block *sb)
>  {
>  	struct minix_sb_info *sbi = minix_sb(sb);
>  
> -	if (sbi->s_imap_blocks == 0 || sbi->s_zmap_blocks == 0)
> +	if (sbi->s_imap_blocks < 1 || sbi->s_zmap_blocks < 1 ||
> +		sbi->s_ninodes < 1 || sbi->s_nzones < 1 ||
> +		sbi->s_firstdatazone <= 4 ||
> +		sbi->s_firstdatazone >= sbi->s_nzones ||
> +		sbi->s_log_zone_size > 4)

Nit: The indentation we use for long conditions is:
	if (sbi->s_imap_blocks < 1 || sbi->s_zmap_blocks < 1 ||
	    sbi->s_ninodes < 1 || sbi->s_nzones < 1 ||
	    sbi->s_firstdatazone <= 4 ||
	    sbi->s_firstdatazone >= sbi->s_nzones || sbi->s_log_zone_size > 4)

Also based on http://ohm.hgesser.de/sp-ss2012/Intro-MinixFS.pdf I think
s_firstdatazone is constrained even more (since there must be inode bitmap,
zone bitmap, and inode table before it).

Also s_log_zone_size is odd. Looking at how it is treated within the code
it only matters for reporting of free blocks (but zone bitmap is really
treated as having one bit per block) so I don't think we really support
anything else than 0 there but that would need confirmation by someone more
knowledgeable with the code.

								Honza
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR