| Message ID | 20241217104345.358461-1-dmantipov@yandex.ru (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | ocfs2: add extent record check in ocfs2_commit_truncate() | expand |
> On Dec 17, 2024, at 18:43, Dmitry Antipov <dmantipov@yandex.ru> wrote: > > Syzbot has reported the following BUG(): > > Kernel BUG at fs/ocfs2/alloc.c:686! > ... > RIP: 0010:ocfs2_new_path fs/ocfs2/alloc.c:686 [inline] > RIP: 0010:ocfs2_commit_truncate+0x2547/0x2550 fs/ocfs2/alloc.c:7234 > Call Trace: > <TASK> > ocfs2_truncate_for_delete fs/ocfs2/inode.c:623 [inline] > ocfs2_wipe_inode fs/ocfs2/inode.c:790 [inline] > ocfs2_delete_inode fs/ocfs2/inode.c:1079 [inline] > ocfs2_evict_inode+0x1c46/0x4630 fs/ocfs2/inode.c:1216 > evict+0x4e8/0x9a0 fs/inode.c:796 > d_delete_notify include/linux/fsnotify.h:332 [inline] > vfs_rmdir+0x3d7/0x510 fs/namei.c:4407 > do_rmdir+0x3b5/0x580 fs/namei.c:4453 > __do_sys_rmdir fs/namei.c:4472 [inline] > __se_sys_rmdir fs/namei.c:4470 [inline] > __x64_sys_rmdir+0x47/0x50 fs/namei.c:4470 > do_syscall_x64 arch/x86/entry/common.c:52 [inline] > do_syscall_64+0xf3/0x230 arch/x86/entry/common.c:83 > entry_SYSCALL_64_after_hwframe+0x77/0x7f > ... > </TASK> > > In 'ocfs2_commit_truncate()', add an extra extent tree depth check > to refuse further operations on (presumably) corrupted on-disk inode. The BUG_ON is in ocfs2_new_path(). The grace way to avoid further similar fuzz reports is that change the BUG_ON() to check and return ERR_PTR(-EIO) but it needs change of all call sites. For now, I think the fix is fine. > Reported-by: syzbot+c16daba279a1161acfb0@syzkaller.appspotmail.com > Closes: https://syzkaller.appspot.com/bug?extid=c16daba279a1161acfb0 > Fixes: 78f94673d7fa ("Ocfs2: Optimize ocfs2 truncate to use ocfs2_remove_btree_range() instead.") > Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru> > --- > fs/ocfs2/alloc.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/fs/ocfs2/alloc.c b/fs/ocfs2/alloc.c > index 395e23920632..040aa2694f53 100644 > --- a/fs/ocfs2/alloc.c > +++ b/fs/ocfs2/alloc.c > @@ -7219,12 +7219,20 @@ int ocfs2_commit_truncate(struct ocfs2_super *osb, > struct ocfs2_extent_rec *rec; > struct ocfs2_path *path = NULL; > struct ocfs2_dinode *di = (struct ocfs2_dinode *)di_bh->b_data; > + u16 tree_depth = le16_to_cpu(di->id2.i_list.l_tree_depth); No need of tree_depth. Just call If (unlikely(le16_to_cpu(di->id2.i_list.l_tree_depth) >= OCFS2_MAX_PATH_DEPTH)) { ... > struct ocfs2_extent_list *root_el = &(di->id2.i_list); > u64 refcount_loc = le64_to_cpu(di->i_refcount_loc); > struct ocfs2_extent_tree et; > struct ocfs2_cached_dealloc_ctxt dealloc; > struct ocfs2_refcount_tree *ref_tree = NULL; > > + if (unlikely(tree_depth >= OCFS2_MAX_PATH_DEPTH)) { > + ocfs2_error(inode->i_sb, > + "dinode %llu has invalid extent record depth %u\n", > + (u64)di_bh->b_blocknr, tree_depth); > + return -EINVAL; -EIO is better. — Su > + } > + > ocfs2_init_dinode_extent_tree(&et, INODE_CACHE(inode), di_bh); > ocfs2_init_dealloc_ctxt(&dealloc); > > -- > 2.47.1 > >
On Sun, Dec 29, 2024 at 10:05:17AM +0800, Glass Su wrote: > > +++ b/fs/ocfs2/alloc.c > > @@ -7219,12 +7219,20 @@ int ocfs2_commit_truncate(struct ocfs2_super *osb, > > struct ocfs2_extent_rec *rec; > > struct ocfs2_path *path = NULL; > > struct ocfs2_dinode *di = (struct ocfs2_dinode *)di_bh->b_data; > > + u16 tree_depth = le16_to_cpu(di->id2.i_list.l_tree_depth); > > No need of tree_depth. Just call > If (unlikely(le16_to_cpu(di->id2.i_list.l_tree_depth) >= OCFS2_MAX_PATH_DEPTH)) { That seems more complicated to me. I find the original easier to read. > > + if (unlikely(tree_depth >= OCFS2_MAX_PATH_DEPTH)) { > > + ocfs2_error(inode->i_sb, > > + "dinode %llu has invalid extent record depth %u\n", > > + (u64)di_bh->b_blocknr, tree_depth); > > + return -EINVAL; > > -EIO is better. -EFSCORRUPTED is surely the right errno here?
> On Dec 29, 2024, at 18:49, Matthew Wilcox <willy@infradead.org> wrote: > > On Sun, Dec 29, 2024 at 10:05:17AM +0800, Glass Su wrote: >>> +++ b/fs/ocfs2/alloc.c >>> @@ -7219,12 +7219,20 @@ int ocfs2_commit_truncate(struct ocfs2_super *osb, >>> struct ocfs2_extent_rec *rec; >>> struct ocfs2_path *path = NULL; >>> struct ocfs2_dinode *di = (struct ocfs2_dinode *)di_bh->b_data; >>> + u16 tree_depth = le16_to_cpu(di->id2.i_list.l_tree_depth); >> >> No need of tree_depth. Just call >> If (unlikely(le16_to_cpu(di->id2.i_list.l_tree_depth) >= OCFS2_MAX_PATH_DEPTH)) { > > That seems more complicated to me. I find the original easier to read. > >>> + if (unlikely(tree_depth >= OCFS2_MAX_PATH_DEPTH)) { >>> + ocfs2_error(inode->i_sb, >>> + "dinode %llu has invalid extent record depth %u\n", >>> + (u64)di_bh->b_blocknr, tree_depth); >>> + return -EINVAL; >> >> -EIO is better. > > -EFSCORRUPTED is surely the right errno here? Indeed. -EIO is an old school errno. -EFSCORRUPTED is more accurate. — Su
On 2024/12/30 10:05, Glass Su wrote: > > >> On Dec 29, 2024, at 18:49, Matthew Wilcox <willy@infradead.org> wrote: >> >> On Sun, Dec 29, 2024 at 10:05:17AM +0800, Glass Su wrote: >>>> +++ b/fs/ocfs2/alloc.c >>>> @@ -7219,12 +7219,20 @@ int ocfs2_commit_truncate(struct ocfs2_super *osb, >>>> struct ocfs2_extent_rec *rec; >>>> struct ocfs2_path *path = NULL; >>>> struct ocfs2_dinode *di = (struct ocfs2_dinode *)di_bh->b_data; >>>> + u16 tree_depth = le16_to_cpu(di->id2.i_list.l_tree_depth); >>> >>> No need of tree_depth. Just call >>> If (unlikely(le16_to_cpu(di->id2.i_list.l_tree_depth) >= OCFS2_MAX_PATH_DEPTH)) { >> >> That seems more complicated to me. I find the original easier to read. >> >>>> + if (unlikely(tree_depth >= OCFS2_MAX_PATH_DEPTH)) { >>>> + ocfs2_error(inode->i_sb, >>>> + "dinode %llu has invalid extent record depth %u\n", >>>> + (u64)di_bh->b_blocknr, tree_depth); >>>> + return -EINVAL; >>> >>> -EIO is better. >> >> -EFSCORRUPTED is surely the right errno here? > > Indeed. -EIO is an old school errno. -EFSCORRUPTED is more accurate. > Yes, EFSCORRUPTED is more proper. It seems that it is caused by a crafted ocfs2 image. And I think there will be more bugs for those similar crafted images. Thanks, Joseph
On 12/30/24 5:26 AM, Joseph Qi wrote:
> It seems that it is caused by a crafted ocfs2 image.
Sure, this is the syzkaller's default method to find bugs in filesystems code.
Dmitry
diff --git a/fs/ocfs2/alloc.c b/fs/ocfs2/alloc.c index 395e23920632..040aa2694f53 100644 --- a/fs/ocfs2/alloc.c +++ b/fs/ocfs2/alloc.c @@ -7219,12 +7219,20 @@ int ocfs2_commit_truncate(struct ocfs2_super *osb, struct ocfs2_extent_rec *rec; struct ocfs2_path *path = NULL; struct ocfs2_dinode *di = (struct ocfs2_dinode *)di_bh->b_data; + u16 tree_depth = le16_to_cpu(di->id2.i_list.l_tree_depth); struct ocfs2_extent_list *root_el = &(di->id2.i_list); u64 refcount_loc = le64_to_cpu(di->i_refcount_loc); struct ocfs2_extent_tree et; struct ocfs2_cached_dealloc_ctxt dealloc; struct ocfs2_refcount_tree *ref_tree = NULL; + if (unlikely(tree_depth >= OCFS2_MAX_PATH_DEPTH)) { + ocfs2_error(inode->i_sb, + "dinode %llu has invalid extent record depth %u\n", + (u64)di_bh->b_blocknr, tree_depth); + return -EINVAL; + } + ocfs2_init_dinode_extent_tree(&et, INODE_CACHE(inode), di_bh); ocfs2_init_dealloc_ctxt(&dealloc);
Syzbot has reported the following BUG(): Kernel BUG at fs/ocfs2/alloc.c:686! ... RIP: 0010:ocfs2_new_path fs/ocfs2/alloc.c:686 [inline] RIP: 0010:ocfs2_commit_truncate+0x2547/0x2550 fs/ocfs2/alloc.c:7234 Call Trace: <TASK> ocfs2_truncate_for_delete fs/ocfs2/inode.c:623 [inline] ocfs2_wipe_inode fs/ocfs2/inode.c:790 [inline] ocfs2_delete_inode fs/ocfs2/inode.c:1079 [inline] ocfs2_evict_inode+0x1c46/0x4630 fs/ocfs2/inode.c:1216 evict+0x4e8/0x9a0 fs/inode.c:796 d_delete_notify include/linux/fsnotify.h:332 [inline] vfs_rmdir+0x3d7/0x510 fs/namei.c:4407 do_rmdir+0x3b5/0x580 fs/namei.c:4453 __do_sys_rmdir fs/namei.c:4472 [inline] __se_sys_rmdir fs/namei.c:4470 [inline] __x64_sys_rmdir+0x47/0x50 fs/namei.c:4470 do_syscall_x64 arch/x86/entry/common.c:52 [inline] do_syscall_64+0xf3/0x230 arch/x86/entry/common.c:83 entry_SYSCALL_64_after_hwframe+0x77/0x7f ... </TASK> In 'ocfs2_commit_truncate()', add an extra extent tree depth check to refuse further operations on (presumably) corrupted on-disk inode. Reported-by: syzbot+c16daba279a1161acfb0@syzkaller.appspotmail.com Closes: https://syzkaller.appspot.com/bug?extid=c16daba279a1161acfb0 Fixes: 78f94673d7fa ("Ocfs2: Optimize ocfs2 truncate to use ocfs2_remove_btree_range() instead.") Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru> --- fs/ocfs2/alloc.c | 8 ++++++++ 1 file changed, 8 insertions(+)