linux-kernel - Re: [PATCH] [v3] SUNRPC: fix some memleaks in gssx_dec_option

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]

Date: Tue, 2 Jan 2024 14:27:17 -0500
From: Chuck Lever <chuck.lever@...cle.com>
To: Zhipeng Lu <alexious@....edu.cn>
Cc: Trond Myklebust <trond.myklebust@...merspace.com>,
        Anna Schumaker <anna@...nel.org>, Jeff Layton <jlayton@...nel.org>,
        Neil Brown <neilb@...e.de>, Olga Kornievskaia <kolga@...app.com>,
        Dai Ngo <Dai.Ngo@...cle.com>, Tom Talpey <tom@...pey.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        "J. Bruce Fields" <bfields@...ldses.org>, Simo Sorce <simo@...hat.com>,
        linux-nfs@...r.kernel.org, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] [v3] SUNRPC: fix some memleaks in gssx_dec_option_array

On Tue, Jan 02, 2024 at 01:38:13PM +0800, Zhipeng Lu wrote:
> The creds and oa->data need to be freed in the error-handling paths after
> there allocation. So this patch add these deallocations in the
> corresponding paths.
> 
> Fixes: 1d658336b05f ("SUNRPC: Add RPC based upcall mechanism for RPCGSS auth")
> Signed-off-by: Zhipeng Lu <alexious@....edu.cn>
> ---
> Changelog:
> 
> v2: correct some syntactic problems.
> v3: delete unused label err.
> ---
>  net/sunrpc/auth_gss/gss_rpc_xdr.c | 27 +++++++++++++++++++--------
>  1 file changed, 19 insertions(+), 8 deletions(-)

I've applied these two patches for v6.9. They will appear in
nfsd-next once the v6.8 merge window closes.

Another comment below.


> diff --git a/net/sunrpc/auth_gss/gss_rpc_xdr.c b/net/sunrpc/auth_gss/gss_rpc_xdr.c
> index d79f12c2550a..cb32ab9a8395 100644
> --- a/net/sunrpc/auth_gss/gss_rpc_xdr.c
> +++ b/net/sunrpc/auth_gss/gss_rpc_xdr.c
> @@ -250,8 +250,8 @@ static int gssx_dec_option_array(struct xdr_stream *xdr,
>  
>  	creds = kzalloc(sizeof(struct svc_cred), GFP_KERNEL);
>  	if (!creds) {
> -		kfree(oa->data);
> -		return -ENOMEM;
> +		err = -ENOMEM;
> +		goto free_oa;
>  	}

I recognize that this patch does not introduce memory allocation
in this function. However, the general rule is that XDR decoding
does not perform memory allocation: that is supposed to be handled
by the next layer up.

But I don't see a good reason that memory allocation even has to be
done in here, and in fact both of these allocations are fixed in
size and number. Would it be nicer if these were made fixed fields
in struct gssx_option_array ?

Not an objection to this patch, could be a project for another day.


>  	oa->data[0].option.data = CREDS_VALUE;
> @@ -265,29 +265,40 @@ static int gssx_dec_option_array(struct xdr_stream *xdr,
>  
>  		/* option buffer */
>  		p = xdr_inline_decode(xdr, 4);
> -		if (unlikely(p == NULL))
> -			return -ENOSPC;
> +		if (unlikely(p == NULL)) {
> +			err = -ENOSPC;
> +			goto free_creds;
> +		}
>  
>  		length = be32_to_cpup(p);
>  		p = xdr_inline_decode(xdr, length);
> -		if (unlikely(p == NULL))
> -			return -ENOSPC;
> +		if (unlikely(p == NULL)) {
> +			err = -ENOSPC;
> +			goto free_creds;
> +		}
>  
>  		if (length == sizeof(CREDS_VALUE) &&
>  		    memcmp(p, CREDS_VALUE, sizeof(CREDS_VALUE)) == 0) {
>  			/* We have creds here. parse them */
>  			err = gssx_dec_linux_creds(xdr, creds);
>  			if (err)
> -				return err;
> +				goto free_creds;
>  			oa->data[0].value.len = 1; /* presence */
>  		} else {
>  			/* consume uninteresting buffer */
>  			err = gssx_dec_buffer(xdr, &dummy);
>  			if (err)
> -				return err;
> +				goto free_creds;
>  		}
>  	}
>  	return 0;
> +
> +free_creds:
> +	kfree(creds);
> +free_oa:
> +	kfree(oa->data);
> +	oa->data = NULL;
> +	return err;
>  }
>  
>  static int gssx_dec_status(struct xdr_stream *xdr,
> -- 
> 2.34.1
> 
> 

-- 
Chuck Lever