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

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

Date:   Thu, 6 Jun 2019 16:43:44 +0300
From:   Kirill Tkhai <ktkhai@...tuozzo.com>
To:     "J. Bruce Fields" <bfields@...ldses.org>
Cc:     syzbot <syzbot+83a43746cebef3508b49@...kaller.appspotmail.com>,
        akpm@...ux-foundation.org, bfields@...hat.com,
        chris@...isdown.name, daniel.m.jordan@...cle.com, guro@...com,
        hannes@...xchg.org, jlayton@...nel.org, laoar.shao@...il.com,
        linux-kernel@...r.kernel.org, linux-mm@...ck.org,
        linux-nfs@...r.kernel.org, mgorman@...hsingularity.net,
        mhocko@...e.com, sfr@...b.auug.org.au,
        syzkaller-bugs@...glegroups.com, yang.shi@...ux.alibaba.com
Subject: Re: KASAN: use-after-free Read in unregister_shrinker

On 06.06.2019 16:13, J. Bruce Fields wrote:
> On Thu, Jun 06, 2019 at 10:47:43AM +0300, Kirill Tkhai wrote:
>> This may be connected with that shrinker unregistering is forgotten on error path.
> 
> I was wondering about that too.  Seems like it would be hard to hit
> reproduceably though: one of the later allocations would have to fail,
> then later you'd have to create another namespace and this time have a
> later module's init fail.

Yes, it's had to bump into this in real life.

AFAIU, syzbot triggers such the problem by using fault-injections
on allocation places should_failslab()->should_fail(). It's possible
to configure a specific slab, so the allocations will fail with
requested probability.
 
> This is the patch I have, which also fixes a (probably less important)
> failure to free the slab cache.
> 
> --b.
> 
> commit 17c869b35dc9
> Author: J. Bruce Fields <bfields@...hat.com>
> Date:   Wed Jun 5 18:03:52 2019 -0400
> 
>     nfsd: fix cleanup of nfsd_reply_cache_init on failure
>     
>     Make sure everything is cleaned up on failure.
>     
>     Especially important for the shrinker, which will otherwise eventually
>     be freed while still referred to by global data structures.
>     
>     Signed-off-by: J. Bruce Fields <bfields@...hat.com>
> 
> diff --git a/fs/nfsd/nfscache.c b/fs/nfsd/nfscache.c
> index ea39497205f0..3dcac164e010 100644
> --- a/fs/nfsd/nfscache.c
> +++ b/fs/nfsd/nfscache.c
> @@ -157,12 +157,12 @@ int nfsd_reply_cache_init(struct nfsd_net *nn)
>  	nn->nfsd_reply_cache_shrinker.seeks = 1;
>  	status = register_shrinker(&nn->nfsd_reply_cache_shrinker);
>  	if (status)
> -		return status;
> +		goto out_nomem;
>  
>  	nn->drc_slab = kmem_cache_create("nfsd_drc",
>  				sizeof(struct svc_cacherep), 0, 0, NULL);
>  	if (!nn->drc_slab)
> -		goto out_nomem;
> +		goto out_shrinker;
>  
>  	nn->drc_hashtbl = kcalloc(hashsize,
>  				sizeof(*nn->drc_hashtbl), GFP_KERNEL);
> @@ -170,7 +170,7 @@ int nfsd_reply_cache_init(struct nfsd_net *nn)
>  		nn->drc_hashtbl = vzalloc(array_size(hashsize,
>  						 sizeof(*nn->drc_hashtbl)));
>  		if (!nn->drc_hashtbl)
> -			goto out_nomem;
> +			goto out_slab;
>  	}
>  
>  	for (i = 0; i < hashsize; i++) {
> @@ -180,6 +180,10 @@ int nfsd_reply_cache_init(struct nfsd_net *nn)
>  	nn->drc_hashsize = hashsize;
>  
>  	return 0;
> +out_slab:
> +	kmem_cache_destroy(nn->drc_slab);
> +out_shrinker:
> +	unregister_shrinker(&nn->nfsd_reply_cache_shrinker);
>  out_nomem:
>  	printk(KERN_ERR "nfsd: failed to allocate reply cache\n");
>  	return -ENOMEM;

Looks OK for me. Feel free to add my reviewed-by if you want.

Reviewed-by: Kirill Tkhai <ktkhai@...tuozzo.com>