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,
