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: <tencent_9DE23D9537AC96BA36C24B3C740776336709@qq.com>
Date: Fri, 19 Dec 2025 15:00:02 +0800
From: Edward Adam Davis <eadavis@...com>
To: heming.zhao@...e.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, 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()")
> > 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.