From 249e441622a5820597cb96a5329f43dafe0c8675 Mon Sep 17 00:00:00 2001 From: Sebastien Buisson Date: Mon, 8 Apr 2024 11:06:50 +0200 Subject: [PATCH] LU-17714 gss: cleanup user keyring usage User keys are linked to the user keyring. But we should not keep an extra reference on the user keyring for every user key being created. This leads to too many references on this keyring, and prevents proper destroy in case the system wants to clean it up (because the user logged off for instance). And when unlinking a user key, we need to take care of the user namespace, in order to fetch the real user keyring, and not the one associated with the mapped uid in the user namespace. Finally we must handle the case where the user key is explicitly revoked via 'keyctl revoke' on the command line, by carrying out the same cleanup as when 'lfs flushctx' is called. This properly drops references on the key, and frees the security context associated with the key. Lustre-change: https://review.whamcloud.com/54692 Lustre-commit: afe0e091d1b82391a929df74717b9665a6f0ab75 Test-Parameters: kerberos=true testlist=sanity-krb5 Fixes: eef24d8a97 ("LU-17173 gss: user keys go to user keyring") Signed-off-by: Sebastien Buisson Change-Id: Ic168b68f8652689aa4402eaa4fcdbd852743d320 Reviewed-on: https://review.whamcloud.com/c/ex/lustre-release/+/55170 Tested-by: jenkins Tested-by: Maloo Reviewed-by: Andreas Dilger --- lustre/ptlrpc/gss/gss_keyring.c | 48 ++++++++++++++++++++++++++++++----------- 1 file changed, 35 insertions(+), 13 deletions(-) diff --git a/lustre/ptlrpc/gss/gss_keyring.c b/lustre/ptlrpc/gss/gss_keyring.c index f922e18..1b114b4 100644 --- a/lustre/ptlrpc/gss/gss_keyring.c +++ b/lustre/ptlrpc/gss/gss_keyring.c @@ -670,6 +670,11 @@ static inline struct key *get_user_keyring(const struct cred *cred) { return _user_key(KEY_SPEC_USER_KEYRING); } + +static inline struct key *get_session_keyring(const struct cred *cred) +{ + return _user_key(KEY_SPEC_SESSION_KEYRING); +} #else static inline struct key *get_user_session_keyring(const struct cred *cred) { @@ -680,6 +685,11 @@ static inline struct key *get_user_keyring(const struct cred *cred) { return key_get(cred->user->uid_keyring); } + +static inline struct key *get_session_keyring(const struct cred *cred) +{ + return key_get(cred->session_keyring); +} #endif /* @@ -689,22 +699,26 @@ static inline struct key *get_user_keyring(const struct cred *cred) static void request_key_unlink(struct key *key) { struct cred *cred = (struct cred *)current_cred(), *new_cred = NULL; + kuid_t kuid_orig = current_cred()->user->uid; #ifdef HAVE_USER_UID_KEYRING struct key *root_uid_keyring = NULL; #endif const struct cred *old_cred = NULL; - kuid_t uid = current_uid(); struct key *ring = NULL; + uid_t uid, key_uid; int res; + uid = from_kuid(current_user_ns(), kuid_orig); + key_uid = from_kuid(&init_user_ns, key->uid); /* unlink key with user's creds if it's a user key */ - if (!uid_eq(key->uid, current_uid())) { + if (key_uid != uid) { new_cred = prepare_creds(); if (new_cred == NULL) goto search; new_cred->uid = key->uid; new_cred->user->uid = key->uid; + new_cred->user_ns = &init_user_ns; #ifdef HAVE_USER_UID_KEYRING root_uid_keyring = current_cred()->user->uid_keyring; new_cred->user->uid_keyring = NULL; @@ -714,7 +728,7 @@ static void request_key_unlink(struct key *key) } /* User keys are linked to the user keyring. So get it now. */ - if (from_kuid(&init_user_ns, key->uid)) { + if (key_uid) { /* Getting a key(ring) normally increases its refcount by 1. * But if we overrode creds above, calling get_user_keyring() * will add one more ref, because of the user switch. @@ -778,12 +792,6 @@ do_unlink: key->serial, key, ring->serial, res); /* matches key_get()/get_user_keyring() above */ key_put(ring); - /* If this is a user key, it added an extra ref on the user - * keyring at link/instantiate stage. This ref needs to be - * removed now that the key has been unlinked. - */ - if (from_kuid(&init_user_ns, key->uid)) - key_put(ring); } else { CDEBUG(D_SEC, "Missing keyring, key %08x (%p) could not be unlinked, ignored\n", @@ -793,7 +801,7 @@ do_unlink: if (old_cred) { revert_creds(old_cred); put_cred(new_cred); - current_cred()->user->uid = uid; + current_cred()->user->uid = kuid_orig; #ifdef HAVE_USER_UID_KEYRING /* We are switching creds back, so need to drop ref on keyring * for kernel implementation based on user keyring pinned from @@ -1501,7 +1509,7 @@ int gss_kt_instantiate(struct key *key, const void *data, size_t datalen) */ uid = from_kuid(&init_user_ns, current_uid()); if (uid == 0) - keyring = current_cred()->session_keyring; + keyring = get_session_keyring(current_cred()); else keyring = get_user_keyring(current_cred()); @@ -1511,13 +1519,15 @@ int gss_kt_instantiate(struct key *key, const void *data, size_t datalen) if (unlikely(rc)) { CERROR("failed to link key %08x to keyring %08x: %d\n", key->serial, keyring->serial, rc); - RETURN(rc); + GOTO(out, rc); } CDEBUG(D_SEC, "key %08x (%p) linked to keyring %08x and instantiated, ctx %p\n", key->serial, key, keyring->serial, key_get_payload(key, 0)); - RETURN(0); +out: + key_put(keyring); + RETURN(rc); } /* @@ -1698,6 +1708,17 @@ void gss_kt_describe(const struct key *key, struct seq_file *s) seq_puts(s, key->description); } +static void gss_kt_revoke(struct key *key) +{ + CDEBUG(D_SEC, "revoking key %08x (%p) ref %d\n", + key->serial, key, ll_read_key_usage(key)); + kill_key_locked(key); + key_revoke_locked(key); + key_put(key); + CDEBUG(D_SEC, "key %08x (%p) revoked ref %d\n", + key->serial, key, ll_read_key_usage(key)); +} + static struct key_type gss_key_type = { .name = "lgssc", @@ -1711,6 +1732,7 @@ static struct key_type gss_key_type = #endif .destroy = gss_kt_destroy, .describe = gss_kt_describe, + .revoke = gss_kt_revoke, }; /**************************************** -- 1.8.3.1