On Fri, 2025-12-26 at 10:15 -0500, Chuck Lever wrote:
From: Chuck Lever chuck.lever@oracle.com
The gssx_dec_ctx(), gssx_dec_status(), and gssx_dec_name() functions allocate memory via gssx_dec_buffer(), which calls kmemdup(). When a subsequent decode operation fails, these functions return immediately without freeing previously allocated buffers, causing memory leaks.
The leak in gssx_dec_ctx() is particularly relevant because the caller (gssp_accept_sec_context_upcall) initializes several buffer length fields to non-zero values, resulting in memory allocation:
struct gssx_ctx rctxh = { .exported_context_token.len = GSSX_max_output_handle_sz, .mech.len = GSS_OID_MAX_LEN, .src_name.display_name.len = GSSX_max_princ_sz, .targ_name.display_name.len = GSSX_max_princ_sz };If, for example, gssx_dec_name() succeeds for src_name but fails for targ_name, the memory allocated for exported_context_token, mech, and src_name.display_name remains unreferenced and cannot be reclaimed.
Add error handling with goto-based cleanup to free any previously allocated buffers before returning an error.
Reported-by: Xingjing Deng micro6947@gmail.com Closes: https://lore.kernel.org/linux-nfs/CAK+ZN9qttsFDu6h1FoqGadXjMx1QXqPMoYQ=6O9RY... Fixes: 1d658336b05f ("SUNRPC: Add RPC based upcall mechanism for RPCGSS auth") Cc: stable@vger.kernel.org Signed-off-by: Chuck Lever chuck.lever@oracle.com
net/sunrpc/auth_gss/gss_rpc_xdr.c | 82 ++++++++++++++++++++++++------- 1 file changed, 64 insertions(+), 18 deletions(-)
diff --git a/net/sunrpc/auth_gss/gss_rpc_xdr.c b/net/sunrpc/auth_gss/gss_rpc_xdr.c index 7d2cdc2bd374..f320c0a8e604 100644 --- a/net/sunrpc/auth_gss/gss_rpc_xdr.c +++ b/net/sunrpc/auth_gss/gss_rpc_xdr.c @@ -320,29 +320,47 @@ static int gssx_dec_status(struct xdr_stream *xdr, /* status->minor_status */ p = xdr_inline_decode(xdr, 8);
- if (unlikely(p == NULL))
return -ENOSPC;
- if (unlikely(p == NULL)) {
err = -ENOSPC;goto out_free_mech;- } p = xdr_decode_hyper(p, &status->minor_status);
/* status->major_status_string */ err = gssx_dec_buffer(xdr, &status->major_status_string); if (err)
return err;
goto out_free_mech;/* status->minor_status_string */ err = gssx_dec_buffer(xdr, &status->minor_status_string); if (err)
return err;
goto out_free_major_status_string;/* status->server_ctx */ err = gssx_dec_buffer(xdr, &status->server_ctx); if (err)
return err;
goto out_free_minor_status_string;/* we assume we have no options for now, so simply consume them */ /* status->options */ err = dummy_dec_opt_array(xdr, &status->options);
- if (err)
goto out_free_server_ctx;
- return 0;
+out_free_server_ctx:
- kfree(status->server_ctx.data);
- status->server_ctx.data = NULL;
+out_free_minor_status_string:
- kfree(status->minor_status_string.data);
- status->minor_status_string.data = NULL;
+out_free_major_status_string:
- kfree(status->major_status_string.data);
- status->major_status_string.data = NULL;
+out_free_mech:
- kfree(status->mech.data);
- status->mech.data = NULL; return err;
} @@ -505,28 +523,35 @@ static int gssx_dec_name(struct xdr_stream *xdr, /* name->name_type */ err = gssx_dec_buffer(xdr, &dummy_netobj); if (err)
return err;
goto out_free_display_name;/* name->exported_name */ err = gssx_dec_buffer(xdr, &dummy_netobj); if (err)
return err;
goto out_free_display_name;/* name->exported_composite_name */ err = gssx_dec_buffer(xdr, &dummy_netobj); if (err)
return err;
goto out_free_display_name;/* we assume we have no attributes for now, so simply consume them */ /* name->name_attributes */ err = dummy_dec_nameattr_array(xdr, &dummy_name_attr_array); if (err)
return err;
goto out_free_display_name;/* we assume we have no options for now, so simply consume them */ /* name->extensions */ err = dummy_dec_opt_array(xdr, &dummy_option_array);
- if (err)
goto out_free_display_name;
- return 0;
+out_free_display_name:
- kfree(name->display_name.data);
- name->display_name.data = NULL; return err;
} @@ -649,32 +674,34 @@ static int gssx_dec_ctx(struct xdr_stream *xdr, /* ctx->state */ err = gssx_dec_buffer(xdr, &ctx->state); if (err)
return err;
goto out_free_exported_context_token;/* ctx->need_release */ err = gssx_dec_bool(xdr, &ctx->need_release); if (err)
return err;
goto out_free_state;/* ctx->mech */ err = gssx_dec_buffer(xdr, &ctx->mech); if (err)
return err;
goto out_free_state;/* ctx->src_name */ err = gssx_dec_name(xdr, &ctx->src_name); if (err)
return err;
goto out_free_mech;/* ctx->targ_name */ err = gssx_dec_name(xdr, &ctx->targ_name); if (err)
return err;
goto out_free_src_name;/* ctx->lifetime */ p = xdr_inline_decode(xdr, 8+8);
- if (unlikely(p == NULL))
return -ENOSPC;
- if (unlikely(p == NULL)) {
err = -ENOSPC;goto out_free_targ_name;- } p = xdr_decode_hyper(p, &ctx->lifetime);
/* ctx->ctx_flags */ @@ -683,17 +710,36 @@ static int gssx_dec_ctx(struct xdr_stream *xdr, /* ctx->locally_initiated */ err = gssx_dec_bool(xdr, &ctx->locally_initiated); if (err)
return err;
goto out_free_targ_name;/* ctx->open */ err = gssx_dec_bool(xdr, &ctx->open); if (err)
return err;
goto out_free_targ_name;/* we assume we have no options for now, so simply consume them */ /* ctx->options */ err = dummy_dec_opt_array(xdr, &ctx->options);
- if (err)
goto out_free_targ_name;
- return 0;
+out_free_targ_name:
- kfree(ctx->targ_name.display_name.data);
- ctx->targ_name.display_name.data = NULL;
+out_free_src_name:
- kfree(ctx->src_name.display_name.data);
- ctx->src_name.display_name.data = NULL;
+out_free_mech:
- kfree(ctx->mech.data);
- ctx->mech.data = NULL;
+out_free_state:
- kfree(ctx->state.data);
- ctx->state.data = NULL;
+out_free_exported_context_token:
- kfree(ctx->exported_context_token.data);
- ctx->exported_context_token.data = NULL; return err;
}
Reviewed-by: Jeff Layton jlayton@kernel.org