2024-01-02 05:44:54

by Zhipeng Lu

Subject: [PATCH] [v3] SUNRPC: fix some memleaks in gssx_dec_option_array

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 <[email protected]>
---
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(-)

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;
}

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

2024-01-02 19:27:44

by Chuck Lever III

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 <[email protected]>
> ---
> 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