linux-kernel - Re: [PATCH] GFS2: Fix mem leak in gfs2_get

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: <1335179575.2708.28.camel@menhir>
Date:	Mon, 23 Apr 2012 12:12:55 +0100
From:	Steven Whitehouse <swhiteho@...hat.com>
To:	Jesper Juhl <jj@...osbits.net>
Cc:	linux-kernel@...r.kernel.org, cluster-devel@...hat.com
Subject: Re: [PATCH] GFS2: Fix mem leak in gfs2_get_acl()

Hi,

On Sat, 2012-04-21 at 23:00 +0200, Jesper Juhl wrote:
> If gfs2_xattr_acl_get() returns 0 - which, as far as I can tell, it
> may do independently of having allocated memory for its third argument
> ('data' in this case) - then we may leak the memory allocated to data.
> 
I'm not so sure... in gfs2_xattr_acl_get() we have:

        error = gfs2_ea_find(ip, GFS2_EATYPE_SYS, name, &el);
        if (error)
                return error;
        if (!el.el_ea)
                goto out;
        if (!GFS2_EA_DATA_LEN(el.el_ea))   <---- zero length means return without allocating
                goto out;

        len = GFS2_EA_DATA_LEN(el.el_ea);
        data = kmalloc(len, GFP_NOFS);
etc.

So it looks to me as if we will never have allocated any
data unless the length is greater than zero, unless I've
missed something?

Steve.

> This patch initializes 'data' to NULL so that it will be safe to call
> kfree() on it even if we do not allocate anything and also makes sure
> that we kfree(data) in the 'len == 0' case.
> 
> Signed-off-by: Jesper Juhl <jj@...osbits.net>
> ---
>  fs/gfs2/acl.c |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
>  Note: I have tested that this change compiles. It has seen no other 
>        testing than that.
> 
> diff --git a/fs/gfs2/acl.c b/fs/gfs2/acl.c
> index 230eb0f..d254d98 100644
> --- a/fs/gfs2/acl.c
> +++ b/fs/gfs2/acl.c
> @@ -43,7 +43,7 @@ struct posix_acl *gfs2_get_acl(struct inode *inode, int type)
>  	struct gfs2_inode *ip = GFS2_I(inode);
>  	struct posix_acl *acl;
>  	const char *name;
> -	char *data;
> +	char *data = NULL;
>  	int len;
>  
>  	if (!ip->i_eattr)
> @@ -60,8 +60,10 @@ struct posix_acl *gfs2_get_acl(struct inode *inode, int type)
>  	len = gfs2_xattr_acl_get(ip, name, &data);
>  	if (len < 0)
>  		return ERR_PTR(len);
> -	if (len == 0)
> +	if (len == 0) {
> +		kfree(data);
>  		return NULL;
> +	}
>  
>  	acl = posix_acl_from_xattr(data, len);
>  	kfree(data);
> -- 
> 1.7.10
> 
> 


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/