linux-kernel - Re: KASAN: use-after-free Read in path

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 25 Mar 2019 12:21:54 +0100
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Al Viro <viro@...iv.linux.org.uk>
Cc:     Linus Torvalds <torvalds@...ux-foundation.org>,
        syzbot <syzbot+7a8ba368b47fdefca61e@...kaller.appspotmail.com>,
        Alexei Starovoitov <ast@...nel.org>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        Linux List Kernel Mailing <linux-kernel@...r.kernel.org>,
        syzkaller-bugs <syzkaller-bugs@...glegroups.com>
Subject: Re: KASAN: use-after-free Read in path_lookupat

On 03/25/2019 12:17 PM, Al Viro wrote:
> 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):

Ah yep, much better, agree. Want to cook proper patch for bpf, or want
me to take care of it?

Thanks,
Daniel

> 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 {
>