linux-kernel - Re: [PATCH] afs: Fix memory leak in afs_put

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: <1306174.1591014689@warthog.procyon.org.uk>
Date:   Mon, 01 Jun 2020 13:31:29 +0100
From:   David Howells <dhowells@...hat.com>
To:     Zhihao Cheng <chengzhihao1@...wei.com>
Cc:     dhowells@...hat.com, linux-afs@...ts.infradead.org,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
        yi.zhang@...wei.com
Subject: Re: [PATCH] afs: Fix memory leak in afs_put_sysnames()

Zhihao Cheng <chengzhihao1@...wei.com> wrote:

> sysnames should be freed after refcnt being decreased to zero in
> afs_put_sysnames().

Good catch.

> Besides, it would be better set net->sysnames to 'NULL' after net->sysnames
> being released if afs_put_sysnames() aims on an afs_sysnames object.

Why?  We don't normally clear pointers when cleaning up a struct - and of the
two places this is relevant, in one we fail to set up a namespace and in the
other we're tearing down a namespace.  In neither case should the pointer be
accessed again.

> @@ -894,7 +894,7 @@ static struct dentry *afs_lookup_atsys(struct inode *dir, struct dentry *dentry,
>  	 */
>  	ret = NULL;
>  out_s:
> -	afs_put_sysnames(subs);
> +	afs_put_sysnames_and_null(net);

This is definitely wrong.  We obtained a ref 23 lines above and dropped the
lock:

	read_lock(&net->sysnames_lock);
	subs = net->sysnames;
	refcount_inc(&subs->usage);
	read_unlock(&net->sysnames_lock);

We are dropping *that* ref, not the one in the struct.

Just adding the missing kfree() call into afs_put_sysnames() should suffice,
thanks.

David