Re: KASAN: use-after-free in nilfs_mdt_destroy

[Al Viro <viro-RmSDqhL/yNMiFSDQTTA3OLVCufUGDwFn@public.gmane.org>]

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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org/
> 
> ... 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]"