linux-kernel - Re: [PATCH] nilfs2: Fix nilfs_sufile_mark_dirty() not set segment usage as dirty

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]

Date:   Sat, 19 Nov 2022 13:11:05 +0900
From:   Ryusuke Konishi <konishi.ryusuke@...il.com>
To:     Andrew Morton <akpm@...ux-foundation.org>
Cc:     Chen Zhongjin <chenzhongjin@...wei.com>,
        linux-nilfs@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] nilfs2: Fix nilfs_sufile_mark_dirty() not set segment
 usage as dirty

Hi Andrew,

On Sat, Nov 19, 2022 at 7:11 AM Andrew Morton wrote:
>
> On Fri, 18 Nov 2022 14:33:04 +0800 Chen Zhongjin wrote:
>
> > In nilfs_sufile_mark_dirty(), the buffer and inode are set dirty, but
> > nilfs_segment_usage is not set dirty, which makes it can be found by
> > nilfs_sufile_alloc() because it checks nilfs_segment_usage_clean(su).
> >
> > This will cause the problem reported by syzkaller:
> > https://syzkaller.appspot.com/bug?id=c7c4748e11ffcc367cef04f76e02e931833cbd24
> >
> > It's because the case starts with segbuf1.segnum = 3, nextnum = 4, and
> > nilfs_sufile_alloc() not called to allocate a new segment.
> >
> > The first time nilfs_segctor_extend_segments() allocated segment
> > segbuf2.segnum = segbuf1.nextnum = 4, then nilfs_sufile_alloc() found
> > nextnextnum = 4 segment because its su is not set dirty.
> > So segbuf2.nextnum = 4, which causes next segbuf3.segnum = 4.
> >
> > sb_getblk() will get same bh for segbuf2 and segbuf3, and this bh is
> > added to both buffer lists of two segbuf.
> > It makes the list head of second list linked to the first one. When
> > iterating the first one, it will access and deref the head of second,
> > which causes NULL pointer dereference.

Acked-by: Ryusuke Konishi <konishi.ryusuke@...il.com>
Tested-by: Ryusuke Konishi <konishi.ryusuke@...il.com>

This is a simple and side-effect-free nice patch for this problem,
even though the functionality of nilfs_sufile_mark_dirty() has changed
from what it is supposed to do.

Making the segment usage dirty was the responsibility of another
function, nilfs_sufile_alloc().
However, it turns out that the current implementation is insufficient
for corrupted disk images like those created by syzbot. This patch
complements that.

If you don't mind, please add the following tag as well:

Reported-by: Liu Shixin <liushixin2@...wei.com>

He initially tried to fix this issue [1]:

[1] https://lkml.kernel.org/r/20221108022928.497746-1-liushixin2@huawei.com

> >
> > Fixes: 9ff05123e3bf ("nilfs2: segment constructor")
>
> Merged in 2009!

This is not wrong.  Sorry for the long term bug.
Because mkfs.nilfs2 and the nilfs2 module don't create broken metadata
that would require this patch, so we never encountered this bug until
syzbot reported it.

>
> > --- a/fs/nilfs2/sufile.c
> > +++ b/fs/nilfs2/sufile.c
> > @@ -495,12 +495,18 @@ void nilfs_sufile_do_free(struct inode *sufile, __u64 segnum,
> >  int nilfs_sufile_mark_dirty(struct inode *sufile, __u64 segnum)
> >  {
> >       struct buffer_head *bh;
> > +     void *kaddr;
> > +     struct nilfs_segment_usage *su;
> >       int ret;
> >
> >       ret = nilfs_sufile_get_segment_usage_block(sufile, segnum, 0, &bh);
> >       if (!ret) {
> >               mark_buffer_dirty(bh);
> >               nilfs_mdt_mark_dirty(sufile);
> > +             kaddr = kmap_atomic(bh->b_page);
> > +             su = nilfs_sufile_block_get_segment_usage(sufile, segnum, bh, kaddr);
> > +             nilfs_segment_usage_set_dirty(su);
> > +             kunmap_atomic(kaddr);
> >               brelse(bh);
> >       }
> >       return ret;
>
> Do we feel that this fix should be backported into -stable kernels?

Yes, it should be. Please do so.

Thanks,
Ryusuke Konishi