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: [email protected]
Closes: https://syzkaller.appspot.com/bug?extid=2f142b57f2af27974fda
Reported-by: [email protected]
Closes: https://syzkaller.appspot.com/bug?extid=5ad0824204c7bf9b67f2
Signed-off-by: Juntong Deng <[email protected]>
---
fs/minix/inode.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
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)
return false;
/*
--
2.39.2
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: [email protected]
> Closes: https://syzkaller.appspot.com/bug?extid=2f142b57f2af27974fda
> Reported-by: [email protected]
> Closes: https://syzkaller.appspot.com/bug?extid=5ad0824204c7bf9b67f2
> Signed-off-by: Juntong Deng <[email protected]>
> ---
...
> 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 <[email protected]>
SUSE Labs, CR