From f2705c4ec5598ca244bbb08673a1cfefd7342812 Mon Sep 17 00:00:00 2001 From: Bruno Faccini Date: Tue, 13 Feb 2024 12:14:40 +0100 Subject: [PATCH] LU-17535 gss: fix lsvcgssd crash in krb lib This patch fixes some logic around the need to call gss_delete_sec_context() or not vs kerberos implementations. snd->ctx address instead of value should be passed to serialize_context_for_kernel()/serialize_krb5_ctx() to allow each implementation to clear it with GSS_C_NO_CONTEXT if it has been destroyed internally, and cases where not can also be handled in handle_krb() now. Test-Parameters: trivial Test-Parameters: kerberos=true testlist=sanity-krb5 Test-Parameters: testgroup=review-dne-selinux-ssk-part-2 Signed-off-by: Bruno Faccini Change-Id: I752712168a2c0f0a5a7a496b851d4cddbb7e4236 Reviewed-on: https://review.whamcloud.com/c/fs/lustre-release/+/54023 Reviewed-by: Sebastien Buisson Reviewed-by: Aurelien Degremont Reviewed-by: Oleg Drokin Tested-by: jenkins Tested-by: Maloo --- lustre/utils/gss/context.c | 2 +- lustre/utils/gss/context.h | 6 +++--- lustre/utils/gss/context_heimdal.c | 12 ++++++------ lustre/utils/gss/context_lucid.c | 4 ++-- lustre/utils/gss/context_mit.c | 4 ++-- lustre/utils/gss/context_spkm3.c | 6 +++--- lustre/utils/gss/lgss_keyring.c | 2 +- lustre/utils/gss/svcgssd_proc.c | 10 +++++++--- 8 files changed, 25 insertions(+), 21 deletions(-) diff --git a/lustre/utils/gss/context.c b/lustre/utils/gss/context.c index f45caca..e9a067c 100644 --- a/lustre/utils/gss/context.c +++ b/lustre/utils/gss/context.c @@ -44,7 +44,7 @@ #include "context.h" int -serialize_context_for_kernel(gss_ctx_id_t ctx, +serialize_context_for_kernel(gss_ctx_id_t *ctx, gss_buffer_desc *buf, gss_OID mech) { diff --git a/lustre/utils/gss/context.h b/lustre/utils/gss/context.h index d19c0fe..5cc7195 100644 --- a/lustre/utils/gss/context.h +++ b/lustre/utils/gss/context.h @@ -78,9 +78,9 @@ extern krb5_error_code krb5_derive_key(const void *enc, const krb5_data *in_constant); #endif -int serialize_context_for_kernel(gss_ctx_id_t ctx, gss_buffer_desc *buf, +int serialize_context_for_kernel(gss_ctx_id_t *ctx, gss_buffer_desc *buf, gss_OID mech); -int serialize_spkm3_ctx(gss_ctx_id_t ctx, gss_buffer_desc *buf); -int serialize_krb5_ctx(gss_ctx_id_t ctx, gss_buffer_desc *buf); +int serialize_spkm3_ctx(gss_ctx_id_t *ctx, gss_buffer_desc *buf); +int serialize_krb5_ctx(gss_ctx_id_t *ctx, gss_buffer_desc *buf); #endif /* _CONTEXT_H_ */ diff --git a/lustre/utils/gss/context_heimdal.c b/lustre/utils/gss/context_heimdal.c index 25faa71..201ebf8 100644 --- a/lustre/utils/gss/context_heimdal.c +++ b/lustre/utils/gss/context_heimdal.c @@ -203,7 +203,7 @@ int write_heimdal_seq_key(char **p, char *end, gss_ctx_id_t ctx) */ int -serialize_krb5_ctx(gss_ctx_id_t ctx, gss_buffer_desc *buf) +serialize_krb5_ctx(gss_ctx_id_t *ctx, gss_buffer_desc *buf) { char *p, *end; @@ -219,7 +219,7 @@ serialize_krb5_ctx(gss_ctx_id_t ctx, gss_buffer_desc *buf) /* initiate: 1 => initiating 0 => accepting */ - if (ctx->more_flags & LOCAL) { + if ((*ctx)->more_flags & LOCAL) { if (WRITE_BYTES(&p, end, constant_one)) goto out_err; } else { @@ -242,19 +242,19 @@ serialize_krb5_ctx(gss_ctx_id_t ctx, gss_buffer_desc *buf) if (WRITE_BYTES(&p, end, algorithm)) goto out_err; /* endtime */ - if (WRITE_BYTES(&p, end, ctx->lifetime)) goto out_err; + if (WRITE_BYTES(&p, end, (*ctx)->lifetime)) goto out_err; /* seq_send */ - if (WRITE_BYTES(&p, end, ctx->auth_context->local_seqnumber)) + if (WRITE_BYTES(&p, end, (*ctx)->auth_context->local_seqnumber)) goto out_err; /* mech_used */ if (write_buffer(&p, end, (gss_buffer_desc*)&krb5oid)) goto out_err; /* enc: derive the encryption key and copy it into buffer */ - if (write_heimdal_enc_key(&p, end, ctx)) goto out_err; + if (write_heimdal_enc_key(&p, end, *ctx)) goto out_err; /* seq: get the sequence number key and copy it into buffer */ - if (write_heimdal_seq_key(&p, end, ctx)) goto out_err; + if (write_heimdal_seq_key(&p, end, *ctx)) goto out_err; buf->length = p - (char *)buf->value; printerr(2, "serialize_krb5_ctx: returning buffer " diff --git a/lustre/utils/gss/context_lucid.c b/lustre/utils/gss/context_lucid.c index ed33a4f..a49ba61 100644 --- a/lustre/utils/gss/context_lucid.c +++ b/lustre/utils/gss/context_lucid.c @@ -573,7 +573,7 @@ out_err: return -1; } int -serialize_krb5_ctx(gss_ctx_id_t ctx, gss_buffer_desc *buf) +serialize_krb5_ctx(gss_ctx_id_t *ctx, gss_buffer_desc *buf) { OM_uint32 maj_stat, min_stat; void *return_ctx = 0; @@ -582,7 +582,7 @@ serialize_krb5_ctx(gss_ctx_id_t ctx, gss_buffer_desc *buf) int retcode = 0; printerr(3, "lucid version!\n"); - maj_stat = gss_krb5_export_lucid_sec_context(&min_stat, &ctx, + maj_stat = gss_krb5_export_lucid_sec_context(&min_stat, ctx, 1, &return_ctx); if (maj_stat != GSS_S_COMPLETE) { pgsserr("gss_krb5_export_lucid_sec_context", diff --git a/lustre/utils/gss/context_mit.c b/lustre/utils/gss/context_mit.c index f6d0d24..b639689 100644 --- a/lustre/utils/gss/context_mit.c +++ b/lustre/utils/gss/context_mit.c @@ -275,9 +275,9 @@ typedef struct gss_union_ctx_id_t { } gss_union_ctx_id_desc, *gss_union_ctx_id_t; int -serialize_krb5_ctx(gss_ctx_id_t ctx, gss_buffer_desc *buf) +serialize_krb5_ctx(gss_ctx_id_t *ctx, gss_buffer_desc *buf) { - krb5_gss_ctx_id_t kctx = ((gss_union_ctx_id_t)ctx)->internal_ctx_id; + krb5_gss_ctx_id_t kctx = ((gss_union_ctx_id_t)*ctx)->internal_ctx_id; char *p, *end; static int constant_zero = 0; static int constant_one = 1; diff --git a/lustre/utils/gss/context_spkm3.c b/lustre/utils/gss/context_spkm3.c index 7a77bef..2ef1c1f 100644 --- a/lustre/utils/gss/context_spkm3.c +++ b/lustre/utils/gss/context_spkm3.c @@ -137,7 +137,7 @@ out_err: * and only export those fields to the kernel. */ int -serialize_spkm3_ctx(gss_ctx_id_t ctx, gss_buffer_desc *buf) +serialize_spkm3_ctx(gss_ctx_id_t *ctx, gss_buffer_desc *buf) { OM_uint32 vers, ret, maj_stat, min_stat; void *ret_ctx = 0; @@ -146,7 +146,7 @@ serialize_spkm3_ctx(gss_ctx_id_t ctx, gss_buffer_desc *buf) printerr(1, "serialize_spkm3_ctx called\n"); printerr(2, "DEBUG: serialize_spkm3_ctx: lucid version!\n"); - maj_stat = gss_export_lucid_sec_context(&min_stat, &ctx, 1, &ret_ctx); + maj_stat = gss_export_lucid_sec_context(&min_stat, ctx, 1, &ret_ctx); if (maj_stat != GSS_S_COMPLETE) goto out_err; @@ -160,7 +160,7 @@ serialize_spkm3_ctx(gss_ctx_id_t ctx, gss_buffer_desc *buf) } ret = prepare_spkm3_ctx_buffer(lctx, buf); - maj_stat = gss_free_lucid_sec_context(&min_stat, ctx, ret_ctx); + maj_stat = gss_free_lucid_sec_context(&min_stat, *ctx, ret_ctx); if (maj_stat != GSS_S_COMPLETE) printerr(0, "WARN: failed to free lucid sec context\n"); diff --git a/lustre/utils/gss/lgss_keyring.c b/lustre/utils/gss/lgss_keyring.c index aef75fc..4df569c 100644 --- a/lustre/utils/gss/lgss_keyring.c +++ b/lustre/utils/gss/lgss_keyring.c @@ -748,7 +748,7 @@ retry_nego: goto out; } - rc = serialize_context_for_kernel(lnd.lnd_ctx, &lnd.lnd_ctx_token, + rc = serialize_context_for_kernel(&lnd.lnd_ctx, &lnd.lnd_ctx_token, lnd.lnd_mech); if (rc) { logmsg(LL_ERR, "key %08x: failed to export context\n", keyid); diff --git a/lustre/utils/gss/svcgssd_proc.c b/lustre/utils/gss/svcgssd_proc.c index 456e19d..3e6f001 100644 --- a/lustre/utils/gss/svcgssd_proc.c +++ b/lustre/utils/gss/svcgssd_proc.c @@ -868,15 +868,19 @@ static int handle_krb(struct svc_nego_data *snd) /* kernel needs ctx to calculate verifier on null response, so * must give it context before doing null call: */ - if (serialize_context_for_kernel(snd->ctx, &snd->ctx_token, mech)) { + if (serialize_context_for_kernel(&snd->ctx, &snd->ctx_token, mech)) { printerr(LL_ERR, "ERROR: %s: serialize_context_for_kernel failed\n", __func__); snd->maj_stat = GSS_S_FAILURE; goto out_err; } - /* We no longer need the gss context */ - gss_delete_sec_context(&ignore_min_stat, &snd->ctx, &ignore_out_tok); + + /* heimdal/MIT implementations do not delete context at all */ + if (snd->ctx != GSS_C_NO_CONTEXT) + gss_delete_sec_context(&ignore_min_stat, &snd->ctx, + &ignore_out_tok); + do_svc_downcall(&snd->out_handle, &cred, mech, &snd->ctx_token); /* We no longer need the context token */ if (snd->ctx_token.value) { -- 1.8.3.1