linux-kernel - Re: [PATCH v2] ocfs2: fix oob in __ocfs2_find

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <5hyc4g4g6uxix5jncqi7ukhmdzq6xoxvou5dkvpj2o5y6v7zoe@qgvucbpcvbqn>
Date: Fri, 19 Dec 2025 15:25:31 +0800
From: Heming Zhao <heming.zhao@...e.com>
To: Edward Adam Davis <eadavis@...com>
Cc: eadavis@...org, jlbec@...lplan.org, joseph.qi@...ux.alibaba.com, 
	linux-kernel@...r.kernel.org, mark@...heh.com, ocfs2-devel@...ts.linux.dev, 
	syzkaller-bugs@...glegroups.com
Subject: Re: [PATCH v2] ocfs2: fix oob in __ocfs2_find_path

On Fri, Dec 19, 2025 at 03:00:02PM +0800, Edward Adam Davis wrote:
> On Fri, 19 Dec 2025 14:31:58 +0800, Heming Zhao wrote:
> > > Patch 2f26f58df041 modifies the definition of the array l_recs, specifying
> > > the number of its members as l_count. In the path shown in [1], it will
> > > first read the inode block where the system file directory is located
> > > from the disk, and then read the l_count of each extension block from
> > > the disk according to the extension list of the directory. Then the
> > > value is 0 before l_count is read. If the array l_recs member is directly
> > > accessed, the oob in [1] will be triggered.
> > 
> > Please revise the commit log.
> > I noticed a mistake in my previous email. While __ocfs2_find_path() does issue
> > IOs via ocfs2_read_extent_block(), the claim that "the value is 0 before
> > l_count is read" cannot be true in this context. All data for struct
> > ocfs2_extent_list (el) is fully read from the disk before the OOB issue is
> > triggered.
> Upon closer inspection, yes, the root extent list has been read from
> the disk, and e_blkno is being retrieved to read subsequent extension
> blocks. However, an error occurred while reading e_blkno because the
> l_count value of the root extent list is 0.
> Therefore, the Fix tag is incorrect; it shouldn't be 2f26f58df041,
> but rather dcd0538ff4e8 ("ocfs2: sparse b-tree support").
> > 
> > > The loop terminates when l_count is 0, similar to when next_free is 0.
> > >
> > > [1]
> > > UBSAN: array-index-out-of-bounds in fs/ocfs2/alloc.c:1838:11
> > > index 0 is out of range for type 'struct ocfs2_extent_rec[] __counted_by(l_count)' (aka 'struct ocfs2_extent_rec[]')
> > > Call Trace:
> > >  __ocfs2_find_path+0x606/0xa40 fs/ocfs2/alloc.c:1838
> > >  ocfs2_find_leaf+0xab/0x1c0 fs/ocfs2/alloc.c:1946
> > >  ocfs2_get_clusters_nocache+0x172/0xc60 fs/ocfs2/extent_map.c:418
> > >  ocfs2_get_clusters+0x505/0xa70 fs/ocfs2/extent_map.c:631
> > >  ocfs2_extent_map_get_blocks+0x202/0x6a0 fs/ocfs2/extent_map.c:678
> > >  ocfs2_read_virt_blocks+0x286/0x930 fs/ocfs2/extent_map.c:1001
> > >  ocfs2_read_dir_block fs/ocfs2/dir.c:521 [inline]
> > >  ocfs2_find_entry_el fs/ocfs2/dir.c:728 [inline]
> > >  ocfs2_find_entry+0x3e4/0x2090 fs/ocfs2/dir.c:1120
> > >  ocfs2_find_files_on_disk+0xdf/0x310 fs/ocfs2/dir.c:2023
> > >  ocfs2_lookup_ino_from_name+0x52/0x100 fs/ocfs2/dir.c:2045
> > >  _ocfs2_get_system_file_inode fs/ocfs2/sysfile.c:136 [inline]
> > >  ocfs2_get_system_file_inode+0x326/0x770 fs/ocfs2/sysfile.c:112
> > >  ocfs2_init_global_system_inodes+0x319/0x660 fs/ocfs2/super.c:461
> > >  ocfs2_initialize_super fs/ocfs2/super.c:2196 [inline]
> > >  ocfs2_fill_super+0x4432/0x65b0 fs/ocfs2/super.c:993
> > >  get_tree_bdev_flags+0x40e/0x4d0 fs/super.c:1691
> > >  vfs_get_tree+0x92/0x2a0 fs/super.c:1751
> > >  fc_mount fs/namespace.c:1199 [inline]
> > >
> > > Fixes: 2f26f58df041 ("ocfs2: annotate flexible array members with __counted_by_le()")

This commit doesn't introduce any bugs, rather, it introduces stricter sanity
checks for struct usage. Please remove the 'Fixes' tag.

The code logic of this function is correct. This issue is triggered by a
syzbot-crafted image and is very unlikely to occur in the real world. I would
prefer not to add a 'Fixes:' tag, as doing so would increase the backporting
workload for Linux distribution maintainers.

> > > Reported-by: syzbot+151afab124dfbc5f15e6@...kaller.appspotmail.com
> > > Closes: https://syzkaller.appspot.com/bug?extid=151afab124dfbc5f15e6
> > > Signed-off-by: Edward Adam Davis <eadavis@...com>
> > > ---
> > > v1 -> v2: check l_count and update comments
> > >
> > >  fs/ocfs2/alloc.c | 10 ++++++----
> > >  1 file changed, 6 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/fs/ocfs2/alloc.c b/fs/ocfs2/alloc.c
> > > index 58bf58b68955..44bbc51276de 100644
> > > --- a/fs/ocfs2/alloc.c
> > > +++ b/fs/ocfs2/alloc.c
> > > @@ -1812,14 +1812,16 @@ static int __ocfs2_find_path(struct ocfs2_caching_info *ci,
> > >  			ret = -EROFS;
> > >  			goto out;
> > >  		}
> > > -		if (le16_to_cpu(el->l_next_free_rec) == 0) {
> > > +		if (le16_to_cpu(el->l_next_free_rec) == 0 ||
> > > +		    le16_to_cpu(el->l_count) == 0) {
> > 
> > please restore the code above to my original patch. Since the value is expected
> > to be zero, converting from le16 to cpu endian is a redundant operation and
> > waste of cpu cycles.
> My view remains unchanged, because l_count/l_next_free_rec originates
> from the disk, not from memory.
> 

What are the bits representation of zero? Is byte-swapping necessary for
different-endian architectures?" We just need to perform the conversion in the
error msg log. 

- Heming