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;
Date:   Mon, 15 Aug 2022 19:23:51 +0100
From:   Al Viro <viro@zeniv.linux.org.uk>
To:     Jiacheng Xu <578001344xu@gmail.com>
Cc:     linux-kernel@vger.kernel.org, gregkh@linuxfoundation.org,
        konishi.ryusuke@gmail.com, linux-nilfs@vger.kernel.org,
        security@kernel.org
Subject: Re: KASAN: use-after-free in nilfs_mdt_destroy
Message-ID: <YvqPN2T3Q8pfnCoV@ZenIV>
References: <CAO4S-mficMz1mQW06EuCF+o11+mRDiCpufqVfoHkcRbQbs8kVw@mail.gmail.com>
 <YvqKJppIL4lVCn9+@ZenIV>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YvqKJppIL4lVCn9+@ZenIV>
Sender: Al Viro <viro@ftp.linux.org.uk>
Precedence: bulk

On Mon, Aug 15, 2022 at 07:02:14PM +0100, Al Viro wrote:
> On Mon, Aug 15, 2022 at 10:03:21PM +0800, Jiacheng Xu wrote:
> 
> > Patch:
> > Fix this bug by moving the assignment of inode->i_private before
> > security_inode_alloc.
> > An ad-hoc patch is proposed:
> > https://patchwork.kernel.org/project/linux-fsdevel/patch/20211011030956.2459172-1-mudongliangabcd@gmail.com/
> 
> ... and that looks like utter bollocks.  Why does security_inode_alloc()
> look at ->i_private?  Which LSM is involved?

OK, I see...  The role of security_inode_alloc() here is that it's
a possible cause of failure.  And yes, dealing with that class of
bugs here might be better.

However, *IF* we go that way, why not move that thing to the
very end?  Just prior to
        this_cpu_inc(nr_inodes);

Sure, nilfs2 steps only into uninitialized ->i_private.
Who's to say that we won't run into somebody deciding that e.g.
inode->i_data.private_data is worth a look?

Just move the call of security_inode_alloc() down to immediately
before the nr_inode increment, with explanation along the lines
of

"In case of security_inode_alloc() failure the inode is passed to
->destroy_inode(); doing security_inode_alloc() last makes for
easier life for ->destroy_inode() instances - they can rely upon
the rest of inode_init_always() having been done.  It's not
just a theoretical problem - nilfs2 has actually stepped into that
[reference to report]"