Re: [Cluster-devel] [syzbot] [cluster?] possible deadlock in freeze_super (2)
syzbot suspects this issue was fixed by commit: commit b66f723bb552ad59c2acb5d45ea45c890f84498b Author: Andreas Gruenbacher Date: Tue Jan 31 14:06:53 2023 + gfs2: Improve gfs2_make_fs_rw error handling bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=117e2e29c8 start commit: 4a7d37e824f5 Merge tag 'hardening-v6.3-rc1' of git://git.k.. git tree: upstream kernel config: https://syzkaller.appspot.com/x/.config?x=8b969c5af147d31c dashboard link: https://syzkaller.appspot.com/bug?extid=be899d4f10b2a9522dce syz repro: https://syzkaller.appspot.com/x/repro.syz?x=11484328c8 C reproducer: https://syzkaller.appspot.com/x/repro.c?x=127093a0c8 If the result looks correct, please mark the issue as fixed by replying with: #syz fix: gfs2: Improve gfs2_make_fs_rw error handling For information about bisection process see: https://goo.gl/tpsmEJ#bisection
[Cluster-devel] [PATCH v2 4.19/5.4/5.10 1/1] gfs2: Always check inode size of inline inodes
From: Andreas Gruenbacher commit 70376c7ff31221f1d21db5611d8209e677781d3a upstream. Check if the inode size of stuffed (inline) inodes is within the allowed range when reading inodes from disk (gfs2_dinode_in()). This prevents us from on-disk corruption. The two checks in stuffed_readpage() and gfs2_unstuffer_page() that just truncate inline data to the maximum allowed size don't actually make sense, and they can be removed now as well. Reported-by: syzbot+7bb81dfa9cda07d9c...@syzkaller.appspotmail.com Signed-off-by: Andreas Gruenbacher [pchel...@ispras.ru: adjust the inode variable inside gfs2_dinode_in with the format used before upstream commit 7db35ad8 ("gfs2: Cosmetic gfs2_dinode_{in,out} cleanup")] Signed-off-by: Fedor Pchelkin --- v2: add missed From: tag fs/gfs2/aops.c | 2 -- fs/gfs2/bmap.c | 3 --- fs/gfs2/glops.c | 3 +++ 3 files changed, 3 insertions(+), 5 deletions(-) diff --git a/fs/gfs2/aops.c b/fs/gfs2/aops.c index 530659554870..a0430da033b3 100644 --- a/fs/gfs2/aops.c +++ b/fs/gfs2/aops.c @@ -451,8 +451,6 @@ static int stuffed_readpage(struct gfs2_inode *ip, struct page *page) return error; kaddr = kmap_atomic(page); - if (dsize > gfs2_max_stuffed_size(ip)) - dsize = gfs2_max_stuffed_size(ip); memcpy(kaddr, dibh->b_data + sizeof(struct gfs2_dinode), dsize); memset(kaddr + dsize, 0, PAGE_SIZE - dsize); kunmap_atomic(kaddr); diff --git a/fs/gfs2/bmap.c b/fs/gfs2/bmap.c index b4fde3a8eeb4..eaee95d2ad14 100644 --- a/fs/gfs2/bmap.c +++ b/fs/gfs2/bmap.c @@ -69,9 +69,6 @@ static int gfs2_unstuffer_page(struct gfs2_inode *ip, struct buffer_head *dibh, void *kaddr = kmap(page); u64 dsize = i_size_read(inode); - if (dsize > gfs2_max_stuffed_size(ip)) - dsize = gfs2_max_stuffed_size(ip); - memcpy(kaddr, dibh->b_data + sizeof(struct gfs2_dinode), dsize); memset(kaddr + dsize, 0, PAGE_SIZE - dsize); kunmap(page); diff --git a/fs/gfs2/glops.c b/fs/gfs2/glops.c index bf539eab92c6..db28c240dae3 100644 --- a/fs/gfs2/glops.c +++ b/fs/gfs2/glops.c @@ -454,6 +454,9 @@ static int gfs2_dinode_in(struct gfs2_inode *ip, const void *buf) ip->i_depth = (u8)depth; ip->i_entries = be32_to_cpu(str->di_entries); + if (gfs2_is_stuffed(ip) && ip->i_inode.i_size > gfs2_max_stuffed_size(ip)) + goto corrupt; + if (S_ISREG(ip->i_inode.i_mode)) gfs2_set_aops(&ip->i_inode); -- 2.34.1
[Cluster-devel] [PATCH 4.19/5.4/5.10 1/1] gfs2: Always check inode size of inline inodes
commit 70376c7ff31221f1d21db5611d8209e677781d3a upstream. Check if the inode size of stuffed (inline) inodes is within the allowed range when reading inodes from disk (gfs2_dinode_in()). This prevents us from on-disk corruption. The two checks in stuffed_readpage() and gfs2_unstuffer_page() that just truncate inline data to the maximum allowed size don't actually make sense, and they can be removed now as well. Reported-by: syzbot+7bb81dfa9cda07d9c...@syzkaller.appspotmail.com Signed-off-by: Andreas Gruenbacher [pchel...@ispras.ru: adjust the inode variable inside gfs2_dinode_in with the format used before upstream commit 7db35ad8 ("gfs2: Cosmetic gfs2_dinode_{in,out} cleanup")] Signed-off-by: Fedor Pchelkin --- fs/gfs2/aops.c | 2 -- fs/gfs2/bmap.c | 3 --- fs/gfs2/glops.c | 3 +++ 3 files changed, 3 insertions(+), 5 deletions(-) diff --git a/fs/gfs2/aops.c b/fs/gfs2/aops.c index 530659554870..a0430da033b3 100644 --- a/fs/gfs2/aops.c +++ b/fs/gfs2/aops.c @@ -451,8 +451,6 @@ static int stuffed_readpage(struct gfs2_inode *ip, struct page *page) return error; kaddr = kmap_atomic(page); - if (dsize > gfs2_max_stuffed_size(ip)) - dsize = gfs2_max_stuffed_size(ip); memcpy(kaddr, dibh->b_data + sizeof(struct gfs2_dinode), dsize); memset(kaddr + dsize, 0, PAGE_SIZE - dsize); kunmap_atomic(kaddr); diff --git a/fs/gfs2/bmap.c b/fs/gfs2/bmap.c index b4fde3a8eeb4..eaee95d2ad14 100644 --- a/fs/gfs2/bmap.c +++ b/fs/gfs2/bmap.c @@ -69,9 +69,6 @@ static int gfs2_unstuffer_page(struct gfs2_inode *ip, struct buffer_head *dibh, void *kaddr = kmap(page); u64 dsize = i_size_read(inode); - if (dsize > gfs2_max_stuffed_size(ip)) - dsize = gfs2_max_stuffed_size(ip); - memcpy(kaddr, dibh->b_data + sizeof(struct gfs2_dinode), dsize); memset(kaddr + dsize, 0, PAGE_SIZE - dsize); kunmap(page); diff --git a/fs/gfs2/glops.c b/fs/gfs2/glops.c index bf539eab92c6..db28c240dae3 100644 --- a/fs/gfs2/glops.c +++ b/fs/gfs2/glops.c @@ -454,6 +454,9 @@ static int gfs2_dinode_in(struct gfs2_inode *ip, const void *buf) ip->i_depth = (u8)depth; ip->i_entries = be32_to_cpu(str->di_entries); + if (gfs2_is_stuffed(ip) && ip->i_inode.i_size > gfs2_max_stuffed_size(ip)) + goto corrupt; + if (S_ISREG(ip->i_inode.i_mode)) gfs2_set_aops(&ip->i_inode); -- 2.34.1
[Cluster-devel] [PATCH 4.19/5.4/5.10 0/1] gfs2: Always check inode size of inline inodes
Kernel bug in iomap_read_inline_data() fixed by the following patch is hit on older stables 4.19/5.4/5.10. The patch failed to be initially backported into stable branches older than 5.15 due to the upstream commit 7db35ad8 ("gfs2: Cosmetic gfs2_dinode_{in,out} cleanup"). Now it can be cleanly applied to the 4.19/5.4/5.10 stable branches.
Re: [Cluster-devel] [PATCH 0/3] gfs2_(un)link cleanups
On Tue, Mar 14, 2023 at 2:18 PM Andrew Price wrote: > Some trivial cleanups from my O_TMPFILE branch. That work isn't ready > yet but there was no reason not to send these patches. Applied, thanks. Andreas
[Cluster-devel] [PATCH] gfs2: Fix inode height consistency check
The maximum allowed height of an inode's metadata tree depends on the filesystem block size; it is lower for bigger-block filesystems. When reading in an inode, make sure that the height doesn't exceed the maximum allowed height. Arrays like sd_heightsize are sized to be big enough for any filesystem block size; they will often be slightly bigger than what's needed for a specific filesystem. Reported-by: syzbot+45d4691b1ed3c48eb...@syzkaller.appspotmail.com Signed-off-by: Andreas Gruenbacher --- fs/gfs2/glops.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/fs/gfs2/glops.c b/fs/gfs2/glops.c index 4d99cc77a29b..b65950e76be5 100644 --- a/fs/gfs2/glops.c +++ b/fs/gfs2/glops.c @@ -396,6 +396,7 @@ static int inode_go_demote_ok(const struct gfs2_glock *gl) static int gfs2_dinode_in(struct gfs2_inode *ip, const void *buf) { + struct gfs2_sbd *sdp = GFS2_SB(&ip->i_inode); const struct gfs2_dinode *str = buf; struct timespec64 atime; u16 height, depth; @@ -442,7 +443,7 @@ static int gfs2_dinode_in(struct gfs2_inode *ip, const void *buf) /* i_diskflags and i_eattr must be set before gfs2_set_inode_flags() */ gfs2_set_inode_flags(inode); height = be16_to_cpu(str->di_height); - if (unlikely(height > GFS2_MAX_META_HEIGHT)) + if (unlikely(height > sdp->sd_max_height)) goto corrupt; ip->i_height = (u8)height; -- 2.39.2
Re: [Cluster-devel] [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 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: > > __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:7ffd0e3a28d8 EFLAGS: 0282 ORIG_RAX: 00a5 > RAX: ffda RBX: 0003 RCX: 7f2c63567aca > RDX: 20037f40 RSI: 20037f80 RDI: 7ffd0e3a28e0 > RBP: 7ffd0e3a28e0 R08: 7ffd0e3a2920 R09: 00043350 > R10: 0211 R11: 0282 R12: 0004 > R13: 567192c0 R14: 7ffd0e3a2920 R15: > > > 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+45d4691b1ed3c48eb...@syzkaller.appspotmail.com > Link: > https://syzkaller.appspot.com/bug?id=42296ea544c870f4dde3b25efa4cc1b89515d38e > Signed-off-by: Ivan Orlov > --- > 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,