File 0010-gssd-Fix-locking-for-machine-principal-lis... of Package nfs-utils.23507

Overview Repositories Revisions Requests Users Attributes Meta

File 0010-gssd-Fix-locking-for-machine-principal-list.patch of Package nfs-utils.23507

From 82e75b12c29c3d2d774d9c8a0ac79b5c19252307 Mon Sep 17 00:00:00 2001
From: Doug Nazar <nazard@nazar.ca>
Date: Tue, 14 Jul 2020 13:13:33 -0400
Subject: [PATCH] gssd: Fix locking for machine principal list

Add missing locking for some scans of the global list. There was
also no prevention of ple->ccname being changed concurrently so
use the same lock to protect that. Reference counting was also added
to ensure that the ple is not freed out from under us in the few
places we now drop the lock while doing work.

Signed-off-by: Doug Nazar <nazard@nazar.ca>
Signed-off-by: Steve Dickson <steved@redhat.com>
---
 utils/gssd/gssd.h      |    1 
 utils/gssd/gssd_proc.c |    2 
 utils/gssd/krb5_util.c |  286 +++++++++++++++++++++++++++++++------------------
 utils/gssd/krb5_util.h |   14 --
 4 files changed, 183 insertions(+), 120 deletions(-)

--- a/utils/gssd/gssd.h
+++ b/utils/gssd/gssd.h
@@ -62,7 +62,6 @@ extern int			root_uses_machine_creds;
 extern unsigned int 		context_timeout;
 extern unsigned int rpc_timeout;
 extern char			*preferred_realm;
-extern pthread_mutex_t ple_lock;
 extern pthread_cond_t pcond;
 extern pthread_mutex_t pmutex;
 extern int thread_started;
--- a/utils/gssd/gssd_proc.c
+++ b/utils/gssd/gssd_proc.c
@@ -533,7 +533,7 @@ krb5_use_machine_creds(struct clnt_info
 		uid, tgtname);
 
 	do {
-		gssd_refresh_krb5_machine_credential(clp->servername, NULL,
+		gssd_refresh_krb5_machine_credential(clp->servername,
 						service);
 	/*
 	 * Get a list of credential cache names and try each
--- a/utils/gssd/krb5_util.c
+++ b/utils/gssd/krb5_util.c
@@ -126,9 +126,28 @@
 #include "gss_util.h"
 #include "krb5_util.h"
 
+/*
+ * List of principals from our keytab that we
+ * will try to use to obtain credentials
+ * (known as a principal list entry (ple))
+ */
+struct gssd_k5_kt_princ {
+	struct gssd_k5_kt_princ *next;
+	// Only protect against deletion, not modification
+	int refcount;
+	// Only set during creation in new_ple()
+	krb5_principal princ;
+	char *realm;
+	// Modified during usage by gssd_get_single_krb5_cred()
+	char *ccname;
+	krb5_timestamp endtime;
+};
+
+
 /* Global list of principals/cache file names for machine credentials */
-struct gssd_k5_kt_princ *gssd_k5_kt_princ_list = NULL;
-pthread_mutex_t ple_lock = PTHREAD_MUTEX_INITIALIZER;
+static struct gssd_k5_kt_princ *gssd_k5_kt_princ_list = NULL;
+/* This mutex protects list modification & ple->ccname */
+static pthread_mutex_t ple_lock = PTHREAD_MUTEX_INITIALIZER;
 
 #ifdef HAVE_SET_ALLOWABLE_ENCTYPES
 int limit_to_legacy_enctypes = 0;
@@ -146,6 +165,18 @@ static int gssd_get_single_krb5_cred(krb
 static int query_krb5_ccache(const char* cred_cache, char **ret_princname,
 		char **ret_realm);
 
+static void release_ple(krb5_context context, struct gssd_k5_kt_princ *ple)
+{
+	if (--ple->refcount)
+		return;
+
+	printerr(3, "freeing cached principal (ccname=%s, realm=%s)\n", ple->ccname, ple->realm);
+	krb5_free_principal(context, ple->princ);
+	free(ple->ccname);
+	free(ple->realm);
+	free(ple);
+}
+
 /*
  * Called from the scandir function to weed out potential krb5
  * credentials cache files
@@ -356,12 +387,15 @@ gssd_get_single_krb5_cred(krb5_context c
 	 * 300 because clock skew must be within 300sec for kerberos
 	 */
 	now += 300;
+	pthread_mutex_lock(&ple_lock);
 	if (ple->ccname && ple->endtime > now && !nocache) {
 		printerr(3, "INFO: Credentials in CC '%s' are good until %d\n",
 			 ple->ccname, ple->endtime);
 		code = 0;
+		pthread_mutex_unlock(&ple_lock);
 		goto out;
 	}
+	pthread_mutex_unlock(&ple_lock);
 
 	if ((code = krb5_kt_get_name(context, kt, kt_name, BUFSIZ))) {
 		printerr(0, "ERROR: Unable to get keytab name in "
@@ -414,6 +448,7 @@ gssd_get_single_krb5_cred(krb5_context c
 	 * Initialize cache file which we're going to be using
 	 */
 
+	pthread_mutex_lock(&ple_lock);
 	if (use_memcache)
 	    cache_type = "MEMORY";
 	else
@@ -423,15 +458,18 @@ gssd_get_single_krb5_cred(krb5_context c
 		ccachesearch[0], GSSD_DEFAULT_CRED_PREFIX,
 		GSSD_DEFAULT_MACHINE_CRED_SUFFIX, ple->realm);
 	ple->endtime = my_creds.times.endtime;
-	if (ple->ccname != NULL)
+	if (ple->ccname == NULL || strcmp(ple->ccname, cc_name) != 0) {
 		free(ple->ccname);
-	ple->ccname = strdup(cc_name);
-	if (ple->ccname == NULL) {
-		printerr(0, "ERROR: no storage to duplicate credentials "
-			    "cache name '%s'\n", cc_name);
-		code = ENOMEM;
-		goto out;
+		ple->ccname = strdup(cc_name);
+		if (ple->ccname == NULL) {
+			printerr(0, "ERROR: no storage to duplicate credentials "
+				    "cache name '%s'\n", cc_name);
+			code = ENOMEM;
+			pthread_mutex_unlock(&ple_lock);
+			goto out;
+		}
 	}
+	pthread_mutex_unlock(&ple_lock);
 	if ((code = krb5_cc_resolve(context, cc_name, &ccache))) {
 		k5err = gssd_k5_err_msg(context, code);
 		printerr(0, "ERROR: %s while opening credential cache '%s'\n",
@@ -469,6 +507,7 @@ gssd_get_single_krb5_cred(krb5_context c
 
 /*
  * Given a principal, find a matching ple structure
+ * Called with mutex held
  */
 static struct gssd_k5_kt_princ *
 find_ple_by_princ(krb5_context context, krb5_principal princ)
@@ -485,6 +524,7 @@ find_ple_by_princ(krb5_context context,
 
 /*
  * Create, initialize, and add a new ple structure to the global list
+ * Called with mutex held
  */
 static struct gssd_k5_kt_princ *
 new_ple(krb5_context context, krb5_principal princ)
@@ -536,6 +576,7 @@ new_ple(krb5_context context, krb5_princ
 			p->next = ple;
 	}
 
+	ple->refcount = 1;
 	return ple;
 outerr:
 	if (ple) {
@@ -554,13 +595,14 @@ get_ple_by_princ(krb5_context context, k
 {
 	struct gssd_k5_kt_princ *ple;
 
-	/* Need to serialize list if we ever become multi-threaded! */
-
 	pthread_mutex_lock(&ple_lock);
 	ple = find_ple_by_princ(context, princ);
 	if (ple == NULL) {
 		ple = new_ple(context, princ);
 	}
+	if (ple != NULL) {
+		ple->refcount++;
+	}
 	pthread_mutex_unlock(&ple_lock);
 
 	return ple;
@@ -723,6 +765,8 @@ gssd_search_krb5_keytab(krb5_context con
 				retval = ENOMEM;
 				k5_free_kt_entry(context, kte);
 			} else {
+				release_ple(context, ple);
+				ple = NULL;
 				retval = 0;
 				*found = 1;
 			}
@@ -1112,6 +1156,91 @@ gssd_setup_krb5_user_gss_ccache(uid_t ui
 }
 
 /*
+ * Obtain (or refresh if necessary) Kerberos machine credentials
+ * If a ple is passed in, it's reference will be released
+ */
+int
+gssd_refresh_krb5_machine_credential_internal(char *hostname,
+				     struct gssd_k5_kt_princ *ple,
+				      char *service)
+{
+	krb5_error_code code = 0;
+	krb5_context context;
+	krb5_keytab kt = NULL;;
+	int retval = 0;
+	char *k5err = NULL;
+	const char *svcnames[] = { "$", "root", "nfs", "host", NULL };
+
+	printerr(2, "%s: hostname=%s ple=%p service=%s\n",
+		 __func__, hostname, ple, service);
+	/*
+	 * If a specific service name was specified, use it.
+	 * Otherwise, use the default list.
+	 */
+	if (service != NULL && strcmp(service, "*") != 0) {
+		svcnames[0] = service;
+		svcnames[1] = NULL;
+	}
+	if (hostname == NULL && ple == NULL)
+		return EINVAL;
+
+	code = krb5_init_context(&context);
+	if (code) {
+		k5err = gssd_k5_err_msg(NULL, code);
+		printerr(0, "ERROR: %s: %s while initializing krb5 context\n",
+			 __func__, k5err);
+		retval = code;
+		goto out;
+	}
+
+	if ((code = krb5_kt_resolve(context, keytabfile, &kt))) {
+		k5err = gssd_k5_err_msg(context, code);
+		printerr(0, "ERROR: %s: %s while resolving keytab '%s'\n",
+			 __func__, k5err, keytabfile);
+		goto out_free_context;
+	}
+
+	if (ple == NULL) {
+		krb5_keytab_entry kte;
+
+		code = find_keytab_entry(context, kt, hostname, &kte, svcnames);
+		if (code) {
+			printerr(0, "ERROR: %s: no usable keytab entry found "
+				 "in keytab %s for connection with host %s\n",
+				 __FUNCTION__, keytabfile, hostname);
+			retval = code;
+			goto out_free_kt;
+		}
+
+		ple = get_ple_by_princ(context, kte.principal);
+		k5_free_kt_entry(context, &kte);
+		if (ple == NULL) {
+			char *pname;
+			if ((krb5_unparse_name(context, kte.principal, &pname))) {
+				pname = NULL;
+			}
+			printerr(0, "ERROR: %s: Could not locate or create "
+				 "ple struct for principal %s for connection "
+				 "with host %s\n",
+				 __FUNCTION__, pname ? pname : "<unparsable>",
+				 hostname);
+			if (pname) k5_free_unparsed_name(context, pname);
+			goto out_free_kt;
+		}
+	}
+	retval = gssd_get_single_krb5_cred(context, kt, ple, 0);
+out_free_kt:
+	krb5_kt_close(context, kt);
+out_free_context:
+	if (ple)
+		release_ple(context, ple);
+	krb5_free_context(context);
+out:
+	free(k5err);
+	return retval;
+}
+
+/*
  * Return an array of pointers to names of credential cache files
  * which can be used to try to create gss contexts with a server.
  *
@@ -1138,36 +1267,55 @@ gssd_get_krb5_machine_cred_list(char ***
 		goto out;
 	}
 
-	/* Need to serialize list if we ever become multi-threaded! */
-
+	pthread_mutex_lock(&ple_lock);
 	for (ple = gssd_k5_kt_princ_list; ple; ple = ple->next) {
-		if (ple->ccname) {
-			/* Make sure cred is up-to-date before returning it */
-			retval = gssd_refresh_krb5_machine_credential(NULL, ple,
-				NULL);
-			if (retval)
-				continue;
-			if (i + 1 > listsize) {
-				listsize += listinc;
-				l = (char **)
-					realloc(l, listsize * sizeof(char *));
-				if (l == NULL) {
-					retval = ENOMEM;
-					goto out;
-				}
-			}
-			if ((l[i++] = strdup(ple->ccname)) == NULL) {
+		if (!ple->ccname)
+			continue;
+
+		/* Take advantage of the fact we only remove the ple
+		 * from the list during shutdown. If it's modified
+		 * concurrently at worst we'll just miss a new entry
+		 * before the current ple
+		 *
+		 * gssd_refresh_krb5_machine_credential_internal() will
+		 * release the ple refcount
+		 */
+		ple->refcount++;
+		pthread_mutex_unlock(&ple_lock);
+		/* Make sure cred is up-to-date before returning it */
+		retval = gssd_refresh_krb5_machine_credential_internal(NULL, ple,
+								       NULL);
+		pthread_mutex_lock(&ple_lock);
+		if (gssd_k5_kt_princ_list == NULL) {
+			/* Looks like we did shutdown... abort */
+			l[i] = NULL;
+			gssd_free_krb5_machine_cred_list(l);
+			retval = ENOMEM;
+			goto out_lock;
+		}
+		if (retval)
+			continue;
+		if (i + 1 > listsize) {
+			listsize += listinc;
+			l = (char **)
+				realloc(l, listsize * sizeof(char *));
+			if (l == NULL) {
 				retval = ENOMEM;
-				goto out;
+				goto out_lock;
 			}
 		}
+		if ((l[i++] = strdup(ple->ccname)) == NULL) {
+			retval = ENOMEM;
+			goto out_lock;
+		}
 	}
 	if (i > 0) {
 		l[i] = NULL;
 		*list = l;
 		retval = 0;
-		goto out;
 	}
+out_lock:
+	pthread_mutex_unlock(&ple_lock);
   out:
 	return retval;
 }
@@ -1233,80 +1381,10 @@ gssd_destroy_krb5_machine_creds(void)
  * Obtain (or refresh if necessary) Kerberos machine credentials
  */
 int
-gssd_refresh_krb5_machine_credential(char *hostname,
-				     struct gssd_k5_kt_princ *ple, 
-					 char *service)
+gssd_refresh_krb5_machine_credential(char *hostname, char *service)
 {
-	krb5_error_code code = 0;
-	krb5_context context;
-	krb5_keytab kt = NULL;;
-	int retval = 0;
-	char *k5err = NULL;
-	const char *svcnames[] = { "$", "root", "nfs", "host", NULL };
-
-	/*
-	 * If a specific service name was specified, use it.
-	 * Otherwise, use the default list.
-	 */
-	if (service != NULL && strcmp(service, "*") != 0) {
-		svcnames[0] = service;
-		svcnames[1] = NULL;
-	}
-	if (hostname == NULL && ple == NULL)
-		return EINVAL;
-
-	code = krb5_init_context(&context);
-	if (code) {
-		k5err = gssd_k5_err_msg(NULL, code);
-		printerr(0, "ERROR: %s: %s while initializing krb5 context\n",
-			 __func__, k5err);
-		retval = code;
-		goto out;
-	}
-
-	if ((code = krb5_kt_resolve(context, keytabfile, &kt))) {
-		k5err = gssd_k5_err_msg(context, code);
-		printerr(0, "ERROR: %s: %s while resolving keytab '%s'\n",
-			 __func__, k5err, keytabfile);
-		goto out_free_context;
-	}
-
-	if (ple == NULL) {
-		krb5_keytab_entry kte;
-
-		code = find_keytab_entry(context, kt, hostname, &kte, svcnames);
-		if (code) {
-			printerr(0, "ERROR: %s: no usable keytab entry found "
-				 "in keytab %s for connection with host %s\n",
-				 __FUNCTION__, keytabfile, hostname);
-			retval = code;
-			goto out_free_kt;
-		}
-
-		ple = get_ple_by_princ(context, kte.principal);
-		k5_free_kt_entry(context, &kte);
-		if (ple == NULL) {
-			char *pname;
-			if ((krb5_unparse_name(context, kte.principal, &pname))) {
-				pname = NULL;
-			}
-			printerr(0, "ERROR: %s: Could not locate or create "
-				 "ple struct for principal %s for connection "
-				 "with host %s\n",
-				 __FUNCTION__, pname ? pname : "<unparsable>",
-				 hostname);
-			if (pname) k5_free_unparsed_name(context, pname);
-			goto out_free_kt;
-		}
-	}
-	retval = gssd_get_single_krb5_cred(context, kt, ple, 0);
-out_free_kt:
-	krb5_kt_close(context, kt);
-out_free_context:
-	krb5_free_context(context);
-out:
-	free(k5err);
-	return retval;
+    return gssd_refresh_krb5_machine_credential_internal(hostname, NULL,
+							 service);
 }
 
 /*
--- a/utils/gssd/krb5_util.h
+++ b/utils/gssd/krb5_util.h
@@ -9,19 +9,6 @@
 #include "gss_oids.h"
 #endif
 
-/*
- * List of principals from our keytab that we
- * will try to use to obtain credentials
- * (known as a principal list entry (ple))
- */
-struct gssd_k5_kt_princ {
-	struct gssd_k5_kt_princ *next;
-	krb5_principal princ;
-	char *ccname;
-	char *realm;
-	krb5_timestamp endtime;
-};
-
 
 int gssd_setup_krb5_user_gss_ccache(uid_t uid, char *servername,
 				     char *dirname);
@@ -29,7 +16,6 @@ int  gssd_get_krb5_machine_cred_list(cha
 void gssd_free_krb5_machine_cred_list(char **list);
 void gssd_destroy_krb5_machine_creds(void);
 int  gssd_refresh_krb5_machine_credential(char *hostname,
-					  struct gssd_k5_kt_princ *ple, 
 					  char *service);
 char *gssd_k5_err_msg(krb5_context context, krb5_error_code code);
 void gssd_k5_get_default_realm(char **def_realm);

Places

File 0010-gssd-Fix-locking-for-machine-principal-list.patch of Package nfs-utils.23507

Places