Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 25 Mar 2019 11:17:57 +0000
From:   Al Viro <viro@zeniv.linux.org.uk>
To:     Daniel Borkmann <daniel@iogearbox.net>
Cc:     Linus Torvalds <torvalds@linux-foundation.org>,
        syzbot <syzbot+7a8ba368b47fdefca61e@syzkaller.appspotmail.com>,
        Alexei Starovoitov <ast@kernel.org>,
        linux-fsdevel <linux-fsdevel@vger.kernel.org>,
        Linux List Kernel Mailing <linux-kernel@vger.kernel.org>,
        syzkaller-bugs <syzkaller-bugs@googlegroups.com>
Subject: Re: KASAN: use-after-free Read in path_lookupat
Message-ID: <20190325111756.GN2217@ZenIV.linux.org.uk>
References: <0000000000006946d2057bbd0eef@google.com>
 <CAHk-=wiijJMTw=nTj_ED+YWMCrvHzh3ezfVYRxvzcW6+trgyPA@mail.gmail.com>
 <20190325045744.GK2217@ZenIV.linux.org.uk>
 <717eaff9-1f49-28f3-bbb6-c8f14ebbe03b@iogearbox.net>
 <20190325111123.GM2217@ZenIV.linux.org.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20190325111123.GM2217@ZenIV.linux.org.uk>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, Mar 25, 2019 at 11:11:23AM +0000, Al Viro wrote:
> On Mon, Mar 25, 2019 at 10:15:40AM +0100, Daniel Borkmann wrote:
> > > So we have 5 broken cases, all with the same kind of fix: move freeing
> > > into the RCU-delayed part of ->destroy_inode(); for debugfs and bpf
> > > that requires adding ->alloc_inode()/->destroy_inode(), rather than
> > > relying upon the defaults from fs/inode.c
> > 
> > I believe I copied that logic from one of the other fs'es back then, sigh.
> > Thanks for the analysis and hints for fixing. Would the below (only compile
> > tested for now) look reasonable to you? I believe there is no other way
> > around than to add our own inode cache, but so be it. The freeing of the
> > i_link is then RCU-deferred in bpf_destroy_inode_deferred():
> 
> It looks like it would suffice, but it's way too much boilerplate for my
> taste ;-/
> 
> Most of your headache here comes from messing with slab setup and the
> only reason for that is being unable to free stuff into inode_cachep.
> So grep for inode_cachep would be a good idea, and it digs up the
> following sucker:
> void free_inode_nonrcu(struct inode *inode)  
> {
>         kmem_cache_free(inode_cachep, inode);
> }
> EXPORT_SYMBOL(free_inode_nonrcu);
> 
> IOW, you need nothing on ->alloc_inode() side and for ->destroy_inode()
> just do call_rcu() of a callback, that would kfree(inode->link) if
> it was a symlink, then call free_inode_nonrcu(inode).
> 
> Considerably smaller patch that way...

AFAICS, it boils down to just this (also only compile-tested):

diff --git a/kernel/bpf/inode.c b/kernel/bpf/inode.c
index 2ada5e21dfa6..9437f88b6acf 100644
--- a/kernel/bpf/inode.c
+++ b/kernel/bpf/inode.c
@@ -80,6 +80,20 @@ static const struct inode_operations bpf_dir_iops;
 static const struct inode_operations bpf_prog_iops = { };
 static const struct inode_operations bpf_map_iops  = { };
 
+static void bpf_destroy_inode_deferred(struct rcu_head *head)
+{
+	struct inode *inode = container_of(head, struct inode, i_rcu);
+
+	if (S_ISLNK(inode->i_mode))
+		kfree(inode->i_link);
+	free_inode_nonrcu(inode);
+}
+
+static void bpf_destroy_inode(struct inode *inode)
+{
+	call_rcu(&inode->i_rcu, bpf_destroy_inode_deferred);
+}
+
 static struct inode *bpf_get_inode(struct super_block *sb,
 				   const struct inode *dir,
 				   umode_t mode)
@@ -561,8 +575,6 @@ static void bpf_evict_inode(struct inode *inode)
 	truncate_inode_pages_final(&inode->i_data);
 	clear_inode(inode);
 
-	if (S_ISLNK(inode->i_mode))
-		kfree(inode->i_link);
 	if (!bpf_inode_type(inode, &type))
 		bpf_any_put(inode->i_private, type);
 }
@@ -584,6 +596,7 @@ static const struct super_operations bpf_super_ops = {
 	.drop_inode	= generic_delete_inode,
 	.show_options	= bpf_show_options,
 	.evict_inode	= bpf_evict_inode,
+	.destroy_inode	= bpf_destroy_inode,
 };
 
 enum {