linux-kernel - Re: KASAN: use-after-free in nilfs_mdt

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] [day] [month] [year] [list]

Message-ID: <YvqPN2T3Q8pfnCoV@ZenIV>
Date:   Mon, 15 Aug 2022 19:23:51 +0100
From:   Al Viro <viro@...iv.linux.org.uk>
To:     Jiacheng Xu <578001344xu@...il.com>
Cc:     linux-kernel@...r.kernel.org, gregkh@...uxfoundation.org,
        konishi.ryusuke@...il.com, linux-nilfs@...r.kernel.org,
        security@...nel.org
Subject: Re: KASAN: use-after-free in nilfs_mdt_destroy

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]"