[Cluster-devel] [PATCH] gfs2: Check sb_bsize_shift after reading superblock

[Cluster-devel] [PATCH] gfs2: Check sb_bsize_shift after reading superblock

[Andrew Price]

Fuzzers like to scribble over sb_bsize_shift but in reality it's very
unlikely that this field would be corrupted on its own. Nevertheless it
should be checked to avoid the possibility of messy mount errors due to
bad calculations. It's always a fixed value based on the block size so
we can just check that it's the expected value.

Tested with:

    mkfs.gfs2 -O -p lock_nolock /dev/vdb
    for i in 0 -1 64 65 32 33; do
        gfs2_edit -p sb field sb_bsize_shift $i /dev/vdb
        mount /dev/vdb /mnt/test && umount /mnt/test
    done

Before this patch we get a withdraw after

[   76.413681] gfs2: fsid=loop0.0: fatal: invalid metadata block
[   76.413681]   bh = 19 (type: exp=5, found=4)
[   76.413681]   function = gfs2_meta_buffer, file = fs/gfs2/meta_io.c, line = 492

and with UBSAN configured we also get complaints like

[   76.373395] UBSAN: shift-out-of-bounds in fs/gfs2/ops_fstype.c:295:19
[   76.373815] shift exponent 4294967287 is too large for 64-bit type 'long unsigned int'

After the patch, these complaints don't appear, mount fails immediately
and we get an explanation in dmesg.

Reported-by: syzbot+dcf33a7aae997956fe06 at syzkaller.appspotmail.com
Signed-off-by: Andrew Price <anprice@redhat.com>
---
 fs/gfs2/ops_fstype.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/fs/gfs2/ops_fstype.c b/fs/gfs2/ops_fstype.c
index 549879929c84..692e27f8f563 100644
--- a/fs/gfs2/ops_fstype.c
+++ b/fs/gfs2/ops_fstype.c
@@ -178,7 +178,10 @@ static int gfs2_check_sb(struct gfs2_sbd *sdp, int silent)
 		pr_warn("Invalid block size\n");
 		return -EINVAL;
 	}
-
+	if (sb->sb_bsize_shift != ffs(sb->sb_bsize) - 1) {
+		pr_warn("Invalid block size shift\n");
+		return -EINVAL;
+	}
 	return 0;
 }
 
-- 
2.37.1

[Cluster-devel] [PATCH] gfs2: Check sb_bsize_shift after reading superblock

[Andrew Price]

I've pushed this patch to my git repo:

   git fetch https://gitlab.com/andyprice/linux.git bsize_shift_check
   git show FETCH_HEAD

Andy

On 17/08/2022 13:22, Andrew Price wrote:
> Fuzzers like to scribble over sb_bsize_shift but in reality it's very
> unlikely that this field would be corrupted on its own. Nevertheless it
> should be checked to avoid the possibility of messy mount errors due to
> bad calculations. It's always a fixed value based on the block size so
> we can just check that it's the expected value.
> 
> Tested with:
> 
>      mkfs.gfs2 -O -p lock_nolock /dev/vdb
>      for i in 0 -1 64 65 32 33; do
>          gfs2_edit -p sb field sb_bsize_shift $i /dev/vdb
>          mount /dev/vdb /mnt/test && umount /mnt/test
>      done
> 
> Before this patch we get a withdraw after
> 
> [   76.413681] gfs2: fsid=loop0.0: fatal: invalid metadata block
> [   76.413681]   bh = 19 (type: exp=5, found=4)
> [   76.413681]   function = gfs2_meta_buffer, file = fs/gfs2/meta_io.c, line = 492
> 
> and with UBSAN configured we also get complaints like
> 
> [   76.373395] UBSAN: shift-out-of-bounds in fs/gfs2/ops_fstype.c:295:19
> [   76.373815] shift exponent 4294967287 is too large for 64-bit type 'long unsigned int'
> 
> After the patch, these complaints don't appear, mount fails immediately
> and we get an explanation in dmesg.
> 
> Reported-by: syzbot+dcf33a7aae997956fe06 at syzkaller.appspotmail.com
> Signed-off-by: Andrew Price <anprice@redhat.com>
> ---
>   fs/gfs2/ops_fstype.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/gfs2/ops_fstype.c b/fs/gfs2/ops_fstype.c
> index 549879929c84..692e27f8f563 100644
> --- a/fs/gfs2/ops_fstype.c
> +++ b/fs/gfs2/ops_fstype.c
> @@ -178,7 +178,10 @@ static int gfs2_check_sb(struct gfs2_sbd *sdp, int silent)
>   		pr_warn("Invalid block size\n");
>   		return -EINVAL;
>   	}
> -
> +	if (sb->sb_bsize_shift != ffs(sb->sb_bsize) - 1) {
> +		pr_warn("Invalid block size shift\n");
> +		return -EINVAL;
> +	}
>   	return 0;
>   }
>   

[Cluster-devel] [PATCH] gfs2: Check sb_bsize_shift after reading superblock

[Andreas Gruenbacher]

On Wed, Aug 17, 2022 at 3:17 PM Andrew Price <anprice@redhat.com> wrote:
> Fuzzers like to scribble over sb_bsize_shift but in reality it's very
> unlikely that this field would be corrupted on its own. Nevertheless it
> should be checked to avoid the possibility of messy mount errors due to
> bad calculations. It's always a fixed value based on the block size so
> we can just check that it's the expected value.
>
> Tested with:
>
>     mkfs.gfs2 -O -p lock_nolock /dev/vdb
>     for i in 0 -1 64 65 32 33; do
>         gfs2_edit -p sb field sb_bsize_shift $i /dev/vdb
>         mount /dev/vdb /mnt/test && umount /mnt/test
>     done
>
> Before this patch we get a withdraw after
>
> [   76.413681] gfs2: fsid=loop0.0: fatal: invalid metadata block
> [   76.413681]   bh = 19 (type: exp=5, found=4)
> [   76.413681]   function = gfs2_meta_buffer, file = fs/gfs2/meta_io.c, line = 492
>
> and with UBSAN configured we also get complaints like
>
> [   76.373395] UBSAN: shift-out-of-bounds in fs/gfs2/ops_fstype.c:295:19
> [   76.373815] shift exponent 4294967287 is too large for 64-bit type 'long unsigned int'
>
> After the patch, these complaints don't appear, mount fails immediately
> and we get an explanation in dmesg.
>
> Reported-by: syzbot+dcf33a7aae997956fe06 at syzkaller.appspotmail.com
> Signed-off-by: Andrew Price <anprice@redhat.com>
> ---
>  fs/gfs2/ops_fstype.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/fs/gfs2/ops_fstype.c b/fs/gfs2/ops_fstype.c
> index 549879929c84..692e27f8f563 100644
> --- a/fs/gfs2/ops_fstype.c
> +++ b/fs/gfs2/ops_fstype.c
> @@ -178,7 +178,10 @@ static int gfs2_check_sb(struct gfs2_sbd *sdp, int silent)
>                 pr_warn("Invalid block size\n");
>                 return -EINVAL;
>         }
> -
> +       if (sb->sb_bsize_shift != ffs(sb->sb_bsize) - 1) {
> +               pr_warn("Invalid block size shift\n");
> +               return -EINVAL;
> +       }
>         return 0;
>  }
>
> --
> 2.37.1

Added now; sorry for overlooking this.

Thanks,
Andreas