Message-ID: <CAHc6FU7b-BaBXrMR3UqbZGF3a_y=20TKkCNde1GvqbmN2-h1xw@mail.gmail.com>
Date:   Tue, 28 Mar 2023 01:10:31 +0200
From:   Andreas Gruenbacher <agruenba@...hat.com>
To:     Ivan Orlov <ivan.orlov0322@...il.com>
Cc:     rpeterso@...hat.com, cluster-devel@...hat.com,
        linux-kernel@...r.kernel.org, skhan@...uxfoundation.org,
        himadrispandya@...il.com,
        linux-kernel-mentees@...ts.linuxfoundation.org,
        syzbot+45d4691b1ed3c48eba05@...kaller.appspotmail.com
Subject: Re: [PATCH] gfs2 FS: Fix UBSAN array-index-out-of-bounds in __gfs2_iomap_get

Hello Ivan,

On Wed, Mar 15, 2023 at 10:06 AM Ivan Orlov <ivan.orlov0322@...il.com> wrote:
> Syzkaller reported the following issue:
>
> UBSAN: array-index-out-of-bounds in fs/gfs2/bmap.c:901:46
> index 11 is out of range for type 'u64 [11]'
> CPU: 0 PID: 5067 Comm: syz-executor164 Not tainted 6.1.0-syzkaller-13031-g77856d911a8c #0
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 10/26/2022
> Call Trace:
>  <TASK>
>  __dump_stack lib/dump_stack.c:88 [inline]
>  dump_stack_lvl+0x1b1/0x290 lib/dump_stack.c:106
>  ubsan_epilogue lib/ubsan.c:151 [inline]
>  __ubsan_handle_out_of_bounds+0xe0/0x110 lib/ubsan.c:282
>  __gfs2_iomap_get+0x4a4/0x16e0 fs/gfs2/bmap.c:901
>  gfs2_iomap_get fs/gfs2/bmap.c:1399 [inline]
>  gfs2_block_map+0x28f/0x7f0 fs/gfs2/bmap.c:1214
>  gfs2_write_alloc_required+0x441/0x6e0 fs/gfs2/bmap.c:2322
>  gfs2_jdesc_check+0x1b9/0x290 fs/gfs2/super.c:114
>  init_journal+0x5a4/0x22c0 fs/gfs2/ops_fstype.c:804
>  init_inodes+0xdc/0x340 fs/gfs2/ops_fstype.c:889
>  gfs2_fill_super+0x1bb2/0x2700 fs/gfs2/ops_fstype.c:1247
>  get_tree_bdev+0x400/0x620 fs/super.c:1282
>  gfs2_get_tree+0x50/0x210 fs/gfs2/ops_fstype.c:1330
>  vfs_get_tree+0x88/0x270 fs/super.c:1489
>  do_new_mount+0x289/0xad0 fs/namespace.c:3145
>  do_mount fs/namespace.c:3488 [inline]
>  __do_sys_mount fs/namespace.c:3697 [inline]
>  __se_sys_mount+0x2d3/0x3c0 fs/namespace.c:3674
>  do_syscall_x64 arch/x86/entry/common.c:50 [inline]
>  do_syscall_64+0x3d/0xb0 arch/x86/entry/common.c:80
>  entry_SYSCALL_64_after_hwframe+0x63/0xcd
> RIP: 0033:0x7f2c63567aca
> Code: 83 c4 08 5b 5d c3 66 2e 0f 1f 84 00 00 00 00 00 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 49 89 ca b8 a5 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 c0 ff ff ff f7 d8 64 89 01 48
> RSP: 002b:00007ffd0e3a28d8 EFLAGS: 00000282 ORIG_RAX: 00000000000000a5
> RAX: ffffffffffffffda RBX: 0000000000000003 RCX: 00007f2c63567aca
> RDX: 0000000020037f40 RSI: 0000000020037f80 RDI: 00007ffd0e3a28e0
> RBP: 00007ffd0e3a28e0 R08: 00007ffd0e3a2920 R09: 0000000000043350
> R10: 0000000002000011 R11: 0000000000000282 R12: 0000000000000004
> R13: 00005555567192c0 R14: 00007ffd0e3a2920 R15: 0000000000000000
>  </TASK>
>
> This issue is caused by the 'while' loop in the '__gfs2_iomap_get' function,
> which increments 'height' var until it matches the condition. If height is
> larger or equal to 'sdp->sd_heightsize' array size (which is GFS2_MAX_META_HEIGHT
> + 1), the array-index-out-of-bounds issue occurs. Therefore we need to add extra
> condition to the while loop, which will prevent this issue.
>
> Additionally, if 'height' var after the while ending is equal to GFS2_MAX_META_HEIGHT,
> the 'find_metapath' call right after the loop will break (because it assumes that
> 'height' parameter will not be larger than the size of metapath's mp_list array length,
> which is GFS2_MAX_META_HEIGHT). So, we need to check the 'height' after the loop ending,
> and if its value is too big we need to break the execution of the function, and return
> a proper error if it is too big.

Thanks for the patch, but the actual problem here is that we're
starting out with an invalid height; when starting with a valid
height, the loop will always terminate. Here's a proper fix:

https://listman.redhat.com/archives/cluster-devel/2023-March/023644.html

While looking into this, I had difficulties getting prepro.c to work.
The reason is that it always uses /dev/loop0 instead of creating its
own loop device. Here's a partial fix for that (but note that this
doesn't include the necessary cleanup code at the end):

--- a/repro.c
+++ b/repro.c
@@ -26,8 +26,6 @@
 #define __NR_memfd_create 319
 #endif

-static unsigned long long procid;
-
 //% This code is derived from puff.{c,h}, found in the zlib development. The
 //% original files come with the following copyright notice:

@@ -423,8 +421,15 @@ static long syz_mount_image(volatile long fsarg,
volatile long dir,
   char* source = NULL;
   char loopname[64];
   if (need_loop_device) {
+    int loopctlfd;
+    long devnr;
+
+    loopctlfd = open("/dev/loop-control", O_RDWR);
+    devnr = ioctl(loopctlfd, LOOP_CTL_GET_FREE);
+    close(loopctlfd);
+
     memset(loopname, 0, sizeof(loopname));
-    snprintf(loopname, sizeof(loopname), "/dev/loop%llu", procid);
+    snprintf(loopname, sizeof(loopname), "/dev/loop%lu", devnr);
     if (setup_loop_device(data, size, loopname, &loopfd) == -1)
       return -1;
     source = loopname;

Thanks,
Andreas

> Tested via syzbot.
>
> Reported-by: syzbot+45d4691b1ed3c48eba05@...kaller.appspotmail.com
> Link: https://syzkaller.appspot.com/bug?id=42296ea544c870f4dde3b25efa4cc1b89515d38e
> Signed-off-by: Ivan Orlov <ivan.orlov0322@...il.com>
> ---
>  fs/gfs2/bmap.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/fs/gfs2/bmap.c b/fs/gfs2/bmap.c
> index eedf6926c652..9b7358165f96 100644
> --- a/fs/gfs2/bmap.c
> +++ b/fs/gfs2/bmap.c
> @@ -895,8 +895,16 @@ static int __gfs2_iomap_get(struct inode *inode, loff_t pos, loff_t length,
>         iomap->length = len << inode->i_blkbits;
>
>         height = ip->i_height;
> -       while ((lblock + 1) * sdp->sd_sb.sb_bsize > sdp->sd_heightsize[height])
> +
> +       while (height <= GFS2_MAX_META_HEIGHT
> +               && (lblock + 1) * sdp->sd_sb.sb_bsize > sdp->sd_heightsize[height])
>                 height++;
> +
> +       if (height > GFS2_MAX_META_HEIGHT) {
> +               ret = -ERANGE;
> +               goto unlock;
> +       }
> +
>         find_metapath(sdp, lblock, mp, height);
>         if (height > ip->i_height || gfs2_is_stuffed(ip))
>                 goto do_alloc;
> --
> 2.34.1
>

Powered by blists - more mailing lists