Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220815175114.23576-1-konishi.ryusuke@gmail.com> <YvqP/f2P2YgIIO9U@ZenIV>
In-Reply-To: <YvqP/f2P2YgIIO9U@ZenIV>
From:   Ryusuke Konishi <konishi.ryusuke@gmail.com>
Date:   Tue, 16 Aug 2022 05:34:12 +0900
Message-ID: <CAKFNMomyjXpsz-=BtG+G3q1J7CFUBMEfP13FfxwhWB==9qb++w@mail.gmail.com>
Subject: Re: [PATCH] nilfs2: fix use-after-free bug in nilfs_mdt_destroy()
To:     Al Viro <viro@zeniv.linux.org.uk>
Cc:     Andrew Morton <akpm@linux-foundation.org>,
        linux-nilfs <linux-nilfs@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Jiacheng Xu <stitch@zju.edu.cn>,
        Mudong Liang <mudongliangabcd@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, Aug 16, 2022 at 3:27 AM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Tue, Aug 16, 2022 at 02:51:14AM +0900, Ryusuke Konishi wrote:
> > In alloc_inode(), inode_init_always() could return -ENOMEM if
> > security_inode_alloc() fails.  If this happens for nilfs2,
> > nilfs_free_inode() is called without initializing inode->i_private and
> > nilfs_free_inode() wrongly calls nilfs_mdt_destroy(), which frees
> > uninitialized inode->i_private and can trigger a crash.
> >
> > Fix this bug by initializing inode->i_private in nilfs_alloc_inode().
> >
> > Link: https://lkml.kernel.org/r/CAFcO6XOcf1Jj2SeGt=jJV59wmhESeSKpfR0omdFRq+J9nD1vfQ@mail.gmail.com
> > Link: https://lkml.kernel.org/r/20211011030956.2459172-1-mudongliangabcd@gmail.com
> > Reported-by: butt3rflyh4ck <butterflyhuangxx@gmail.com>
> > Reported-by: Hao Sun <sunhao.th@gmail.com>
> > Reported-by: Jiacheng Xu <stitch@zju.edu.cn>
> > Reported-by: Mudong Liang <mudongliangabcd@gmail.com>
> > Signed-off-by: Ryusuke Konishi <konishi.ryusuke@gmail.com>
> > Cc: Al Viro <viro@zeniv.linux.org.uk>
> > ---
> >  fs/nilfs2/super.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/fs/nilfs2/super.c b/fs/nilfs2/super.c
> > index ba108f915391..aca5614f1b44 100644
> > --- a/fs/nilfs2/super.c
> > +++ b/fs/nilfs2/super.c
> > @@ -159,6 +159,7 @@ struct inode *nilfs_alloc_inode(struct super_block *sb)
> >       ii->i_cno = 0;
> >       ii->i_assoc_inode = NULL;
> >       ii->i_bmap = &ii->i_bmap_data;
> > +     ii->vfs_inode.i_private = NULL;
> >       return &ii->vfs_inode;
> >  }
>
> FWIW, I think it's better to deal with that in inode_init_always(), but
> not just moving ->i_private initialization up - we ought to move
> security_inode_alloc() to the very end.  No sense playing whack-a-mole
> with further possible bugs of that sort...

Yes, I agree it's better if security_inode_alloc() is moved to the end as
possible in the sense of avoiding similar issues.
But, would that vfs change be safe to backport to stable trees?

It looks like the error handling for security_inode_alloc()  is in the
middle of inode_init_always() for a very long time..

If you want to see the impact of the vfs change, I think it's one way
to apply this one in advance.  Or if you want to fix it in one step,
I think it's good too.  How do you feel about this ?

Thanks,
Ryusuke Konishi