* [PATCH] gfs2 FS: Fix UBSAN array-index-out-of-bounds in __gfs2_iomap_get
@ 2023-03-15 9:06 ` Ivan Orlov
0 siblings, 0 replies; 9+ messages in thread
From: Ivan Orlov @ 2023-03-15 9:06 UTC (permalink / raw)
To: rpeterso, agruenba
Cc: Ivan Orlov, linux-kernel, syzbot+45d4691b1ed3c48eba05,
cluster-devel, linux-kernel-mentees
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.
Tested via syzbot.
Reported-by: syzbot+45d4691b1ed3c48eba05@syzkaller.appspotmail.com
Link: https://syzkaller.appspot.com/bug?id=42296ea544c870f4dde3b25efa4cc1b89515d38e
Signed-off-by: Ivan Orlov <ivan.orlov0322@gmail.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
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH] gfs2 FS: Fix UBSAN array-index-out-of-bounds in __gfs2_iomap_get
@ 2023-03-15 9:06 ` Ivan Orlov
0 siblings, 0 replies; 9+ messages in thread
From: Ivan Orlov @ 2023-03-15 9:06 UTC (permalink / raw)
To: rpeterso, agruenba
Cc: Ivan Orlov, cluster-devel, linux-kernel, skhan, himadrispandya,
linux-kernel-mentees, syzbot+45d4691b1ed3c48eba05
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.
Tested via syzbot.
Reported-by: syzbot+45d4691b1ed3c48eba05@syzkaller.appspotmail.com
Link: https://syzkaller.appspot.com/bug?id=42296ea544c870f4dde3b25efa4cc1b89515d38e
Signed-off-by: Ivan Orlov <ivan.orlov0322@gmail.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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [Cluster-devel] [PATCH] gfs2 FS: Fix UBSAN array-index-out-of-bounds in __gfs2_iomap_get
@ 2023-03-15 9:06 ` Ivan Orlov
0 siblings, 0 replies; 9+ messages in thread
From: Ivan Orlov @ 2023-03-15 9:06 UTC (permalink / raw)
To: cluster-devel.redhat.com
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.
Tested via syzbot.
Reported-by: syzbot+45d4691b1ed3c48eba05 at syzkaller.appspotmail.com
Link: https://syzkaller.appspot.com/bug?id=42296ea544c870f4dde3b25efa4cc1b89515d38e
Signed-off-by: Ivan Orlov <ivan.orlov0322@gmail.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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] gfs2 FS: Fix UBSAN array-index-out-of-bounds in __gfs2_iomap_get
2023-03-15 9:06 ` Ivan Orlov
(?)
@ 2023-03-15 9:12 ` Ivan Orlov
-1 siblings, 0 replies; 9+ messages in thread
From: Ivan Orlov @ 2023-03-15 9:12 UTC (permalink / raw)
To: rpeterso, agruenba
Cc: cluster-devel, linux-kernel, skhan, himadrispandya,
linux-kernel-mentees, syzbot+45d4691b1ed3c48eba05
On 3/15/23 13:06, Ivan Orlov 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.
>
> Tested via syzbot.
>
> Reported-by: syzbot+45d4691b1ed3c48eba05@syzkaller.appspotmail.com
> Link: https://syzkaller.appspot.com/bug?id=42296ea544c870f4dde3b25efa4cc1b89515d38e
> Signed-off-by: Ivan Orlov <ivan.orlov0322@gmail.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;
Upd. I made a typo in the second paragraph of patch description: the
'find_metapath' call will break if height is larger than
GFS2_MAX_META_HEIGHT, not equal to. So this I check in corresponding
condition right after the while loop.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] gfs2 FS: Fix UBSAN array-index-out-of-bounds in __gfs2_iomap_get
@ 2023-03-15 9:12 ` Ivan Orlov
0 siblings, 0 replies; 9+ messages in thread
From: Ivan Orlov @ 2023-03-15 9:12 UTC (permalink / raw)
To: rpeterso, agruenba
Cc: linux-kernel, syzbot+45d4691b1ed3c48eba05, cluster-devel,
linux-kernel-mentees
On 3/15/23 13:06, Ivan Orlov 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.
>
> Tested via syzbot.
>
> Reported-by: syzbot+45d4691b1ed3c48eba05@syzkaller.appspotmail.com
> Link: https://syzkaller.appspot.com/bug?id=42296ea544c870f4dde3b25efa4cc1b89515d38e
> Signed-off-by: Ivan Orlov <ivan.orlov0322@gmail.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;
Upd. I made a typo in the second paragraph of patch description: the
'find_metapath' call will break if height is larger than
GFS2_MAX_META_HEIGHT, not equal to. So this I check in corresponding
condition right after the while loop.
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply [flat|nested] 9+ messages in thread
* [Cluster-devel] [PATCH] gfs2 FS: Fix UBSAN array-index-out-of-bounds in __gfs2_iomap_get
@ 2023-03-15 9:12 ` Ivan Orlov
0 siblings, 0 replies; 9+ messages in thread
From: Ivan Orlov @ 2023-03-15 9:12 UTC (permalink / raw)
To: cluster-devel.redhat.com
On 3/15/23 13:06, Ivan Orlov 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.
>
> Tested via syzbot.
>
> Reported-by: syzbot+45d4691b1ed3c48eba05 at syzkaller.appspotmail.com
> Link: https://syzkaller.appspot.com/bug?id=42296ea544c870f4dde3b25efa4cc1b89515d38e
> Signed-off-by: Ivan Orlov <ivan.orlov0322@gmail.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;
Upd. I made a typo in the second paragraph of patch description: the
'find_metapath' call will break if height is larger than
GFS2_MAX_META_HEIGHT, not equal to. So this I check in corresponding
condition right after the while loop.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] gfs2 FS: Fix UBSAN array-index-out-of-bounds in __gfs2_iomap_get
2023-03-15 9:06 ` Ivan Orlov
(?)
@ 2023-03-27 23:10 ` Andreas Gruenbacher
-1 siblings, 0 replies; 9+ messages in thread
From: Andreas Gruenbacher @ 2023-03-27 23:10 UTC (permalink / raw)
To: Ivan Orlov
Cc: linux-kernel, syzbot+45d4691b1ed3c48eba05, cluster-devel,
rpeterso, linux-kernel-mentees
Hello Ivan,
On Wed, Mar 15, 2023 at 10:06 AM Ivan Orlov <ivan.orlov0322@gmail.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@syzkaller.appspotmail.com
> Link: https://syzkaller.appspot.com/bug?id=42296ea544c870f4dde3b25efa4cc1b89515d38e
> Signed-off-by: Ivan Orlov <ivan.orlov0322@gmail.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
>
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] gfs2 FS: Fix UBSAN array-index-out-of-bounds in __gfs2_iomap_get
@ 2023-03-27 23:10 ` Andreas Gruenbacher
0 siblings, 0 replies; 9+ messages in thread
From: Andreas Gruenbacher @ 2023-03-27 23:10 UTC (permalink / raw)
To: Ivan Orlov
Cc: rpeterso, cluster-devel, linux-kernel, skhan, himadrispandya,
linux-kernel-mentees, syzbot+45d4691b1ed3c48eba05
Hello Ivan,
On Wed, Mar 15, 2023 at 10:06 AM Ivan Orlov <ivan.orlov0322@gmail.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@syzkaller.appspotmail.com
> Link: https://syzkaller.appspot.com/bug?id=42296ea544c870f4dde3b25efa4cc1b89515d38e
> Signed-off-by: Ivan Orlov <ivan.orlov0322@gmail.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
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* [Cluster-devel] [PATCH] gfs2 FS: Fix UBSAN array-index-out-of-bounds in __gfs2_iomap_get
@ 2023-03-27 23:10 ` Andreas Gruenbacher
0 siblings, 0 replies; 9+ messages in thread
From: Andreas Gruenbacher @ 2023-03-27 23:10 UTC (permalink / raw)
To: cluster-devel.redhat.com
Hello Ivan,
On Wed, Mar 15, 2023 at 10:06?AM Ivan Orlov <ivan.orlov0322@gmail.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 at syzkaller.appspotmail.com
> Link: https://syzkaller.appspot.com/bug?id=42296ea544c870f4dde3b25efa4cc1b89515d38e
> Signed-off-by: Ivan Orlov <ivan.orlov0322@gmail.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
>
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2023-03-27 23:11 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-03-15 9:06 [PATCH] gfs2 FS: Fix UBSAN array-index-out-of-bounds in __gfs2_iomap_get Ivan Orlov
2023-03-15 9:06 ` [Cluster-devel] " Ivan Orlov
2023-03-15 9:06 ` Ivan Orlov
2023-03-15 9:12 ` Ivan Orlov
2023-03-15 9:12 ` [Cluster-devel] " Ivan Orlov
2023-03-15 9:12 ` Ivan Orlov
2023-03-27 23:10 ` Andreas Gruenbacher
2023-03-27 23:10 ` [Cluster-devel] " Andreas Gruenbacher
2023-03-27 23:10 ` Andreas Gruenbacher
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.