sbkeysync: Refactor signature database data structures

Rather than having three sets of (firmware, filesystem) key databases,
refactor into two sets of (kdk, db, dbx) databases. This allows us to
add the PK later.

Signed-off-by: Jeremy Kerr <jeremy.kerr@canonical.com>

1 files changed, 137 insertions, 167 deletions

diff --git a/src/sbkeysync.c b/src/sbkeysync.c
index 22852a4..3dfb1d8 100644
--- a/src/sbkeysync.c
+++ b/src/sbkeysync.c
@@ -109,8 +109,13 @@ struct cert_type {
 
 struct key_database {
 	const struct key_database_type	*type;
-	struct list_head		firmware_keys;
-	struct list_head		filesystem_keys;
+	struct list_head		keys;
+};
+
+struct keyset {
+	struct key_database	kek;
+	struct key_database	db;
+	struct key_database	dbx;
 };
 
 struct fs_keystore_entry {
@@ -129,9 +134,8 @@ struct fs_keystore {
 
 struct sync_context {
 	const char		*efivars_dir;
-	struct key_database	*kek;
-	struct key_database	*db;
-	struct key_database	*dbx;
+	struct keyset		*filesystem_keys;
+	struct keyset		*firmware_keys;
 	struct fs_keystore	*fs_keystore;
 	const char		**keystore_dirs;
 	unsigned int		n_keystore_dirs;
@@ -140,6 +144,7 @@ struct sync_context {
 	bool			dry_run;
 };
 
+
 #define GUID_STRLEN (8 + 1 + 4 + 1 + 4 + 1 + 4 + 1 + 12 + 1)
 static void guid_to_str(const EFI_GUID *guid, char *str)
 {
@@ -295,42 +300,63 @@ static int sigdb_iterate(void *db_data, size_t len,
 	return rc;
 }
 
-static int sigdb_add_key(EFI_SIGNATURE_DATA *sigdata, int len,
+struct keydb_add_ctx {
+	struct fs_keystore_entry *ke;
+	struct key_database *kdb;
+	struct keyset *keyset;
+};
+
+static int keydb_add_key(EFI_SIGNATURE_DATA *sigdata, int len,
 		const EFI_GUID *type, void *arg)
 {
-	struct key_database *kdb = arg;
+	struct keydb_add_ctx *add_ctx = arg;
 	struct key *key;
 	int rc;
 
-	key = talloc(kdb, struct key);
+	key = talloc(add_ctx->keyset, struct key);
 
 	rc = key_parse(key, type, sigdata->SignatureData,
 			len - sizeof(*sigdata));
 
-	if (rc)
+	if (rc) {
 		talloc_free(key);
-	else
-		list_add(&kdb->firmware_keys, &key->list);
+		return 0;
+	}
+	key->keystore_entry = add_ctx->ke;
+	key->type = *type;
+
+	/* add a reference to the keystore entry: we don't want it to be
+	 * deallocated if the keystore is deallocated before the
+	 * struct key. */
+	if (key->keystore_entry)
+		talloc_reference(key, key->keystore_entry);
+
+	list_add(&add_ctx->kdb->keys, &key->list);
 
 	return 0;
 }
 
-static int read_firmware_key_database(struct key_database *kdb,
-		const char *dir)
+static int read_firmware_keydb(struct sync_context *ctx,
+		struct key_database *kdb)
 {
+	struct keydb_add_ctx add_ctx;
 	char guid_str[GUID_STRLEN];
 	char *filename;
 	uint8_t *buf;
 	int rc = -1;
 	size_t len;
 
+	add_ctx.keyset = ctx->firmware_keys;
+	add_ctx.kdb = kdb;
+	add_ctx.ke = NULL;
+
 	guid_to_str(&kdb->type->guid, guid_str);
 
-	filename = talloc_asprintf(kdb, "%s/%s-%s", dir,
-					kdb->type->name, guid_str);
+	filename = talloc_asprintf(ctx->firmware_keys, "%s/%s-%s",
+			ctx->efivars_dir, kdb->type->name, guid_str);
 
 	buf = NULL;
-	rc = fileio_read_file_noerror(kdb, filename, &buf, &len);
+	rc = fileio_read_file_noerror(ctx->firmware_keys, filename, &buf, &len);
 	if (rc)
 		goto out;
 
@@ -342,7 +368,7 @@ static int read_firmware_key_database(struct key_database *kdb,
 	len -= sizeof(uint32_t);
 
 	rc = 0;
-	sigdb_iterate(buf, len, sigdb_add_key, kdb);
+	sigdb_iterate(buf, len, keydb_add_key, &add_ctx);
 
 out:
 	if (rc)
@@ -352,40 +378,6 @@ out:
 	return rc;
 }
 
-struct keystore_add_ctx {
-	struct fs_keystore *keystore;
-	struct fs_keystore_entry *ke;
-	struct key_database *kdb;
-};
-
-static int keystore_add_key(EFI_SIGNATURE_DATA *sigdata, int len,
-		const EFI_GUID *type, void *arg)
-{
-	struct keystore_add_ctx *add_ctx = arg;
-	struct key *key;
-	int rc;
-
-	key = talloc(add_ctx->kdb, struct key);
-	key->keystore_entry = add_ctx->ke;
-	key->type = *type;
-
-	rc = key_parse(key, type, sigdata->SignatureData,
-			len - sizeof(*sigdata));
-
-	if (rc) {
-		talloc_free(key);
-		return 0;
-	}
-
-	/* add a reference to the data: we don't want it to be
-	 * deallocated if the keystore is deallocated before the
-	 * struct key. */
-	talloc_reference(key, add_ctx->ke->data);
-	list_add(&add_ctx->kdb->filesystem_keys, &key->list);
-
-	return 0;
-}
-
 static void __attribute__((format(printf, 2, 3))) print_keystore_key_error(
 		struct fs_keystore_entry *ke, const char *fmt, ...)
 {
@@ -402,19 +394,19 @@ static void __attribute__((format(printf, 2, 3))) print_keystore_key_error(
 	va_end(ap);
 }
 
-static int read_keystore_key_database(struct key_database *kdb,
-		struct fs_keystore *keystore)
+static int read_filesystem_keydb(struct sync_context *ctx,
+		struct key_database *kdb)
 {
 	EFI_GUID cert_type_pkcs7 = EFI_CERT_TYPE_PKCS7_GUID;
 	EFI_VARIABLE_AUTHENTICATION_2 *auth;
-	struct keystore_add_ctx add_ctx;
+	struct keydb_add_ctx add_ctx;
 	struct fs_keystore_entry *ke;
 	int rc;
 
-	add_ctx.keystore = keystore;
+	add_ctx.keyset = ctx->filesystem_keys;
 	add_ctx.kdb = kdb;
 
-	list_for_each(&keystore->keys, ke, keystore_list) {
+	list_for_each(&ctx->fs_keystore->keys, ke, keystore_list) {
 		unsigned int len;
 		void *buf;
 
@@ -462,7 +454,7 @@ static int read_keystore_key_database(struct key_database *kdb,
 			auth->AuthInfo.Hdr.dwLength;
 
 		add_ctx.ke = ke;
-		rc = sigdb_iterate(buf, len, keystore_add_key, &add_ctx);
+		rc = sigdb_iterate(buf, len, keydb_add_key, &add_ctx);
 		if (rc) {
 			print_keystore_key_error(ke, "error parsing "
 					"EFI_SIGNATURE_LIST");
@@ -474,51 +466,41 @@ static int read_keystore_key_database(struct key_database *kdb,
 	return 0;
 }
 
-static int read_key_databases(struct sync_context *ctx)
+static int read_keysets(struct sync_context *ctx)
 {
-	struct key_database *kdbs[] = {
-		ctx->kek,
-		ctx->db,
-		ctx->dbx,
-	};
-	unsigned int i;
+	read_firmware_keydb(ctx, &ctx->firmware_keys->kek);
+	read_firmware_keydb(ctx, &ctx->firmware_keys->db);
+	read_firmware_keydb(ctx, &ctx->firmware_keys->dbx);
 
-	for (i = 0; i < ARRAY_SIZE(kdbs); i++) {
-		struct key_database *kdb = kdbs[i];
-		read_firmware_key_database(kdb, ctx->efivars_dir);
-		read_keystore_key_database(kdb, ctx->fs_keystore);
-	}
+	read_filesystem_keydb(ctx, &ctx->filesystem_keys->kek);
+	read_filesystem_keydb(ctx, &ctx->filesystem_keys->db);
+	read_filesystem_keydb(ctx, &ctx->filesystem_keys->dbx);
 
 	return 0;
 }
 
-static void print_key_database(struct key_database *kdb)
+static void print_keyset(struct keyset *keyset, const char *name)
 {
+	struct key_database *kdbs[] =
+		{ &keyset->kek, &keyset->db, &keyset->dbx };
 	struct key *key;
+	unsigned int i;
 
-	printf("  %s (firmware)\n", kdb->type->name);
-
-	list_for_each(&kdb->firmware_keys, key, list)
-		printf("    %s\n", key->description);
-
-	printf("  %s (filesystem)\n", kdb->type->name);
+	printf("%s keys:\n", name);
 
-	list_for_each(&kdb->filesystem_keys, key, list) {
-		printf("    %s\n", key->description);
-		printf("     from %s/%s\n",
-				key->keystore_entry->root,
-				key->keystore_entry->name);
+	for (i = 0; i < ARRAY_SIZE(kdbs); i++) {
+		printf("  %s:\n", kdbs[i]->type->name);
+
+		list_for_each(&kdbs[i]->keys, key, list) {
+			printf("    %s\n", key->description);
+			if (key->keystore_entry)
+				printf("     from %s/%s\n",
+						key->keystore_entry->root,
+						key->keystore_entry->name);
+		}
 	}
 }
 
-static void print_key_databases(struct sync_context *ctx)
-{
-	printf("EFI key databases:\n");
-	print_key_database(ctx->kek);
-	print_key_database(ctx->db);
-	print_key_database(ctx->dbx);
-}
-
 static int check_efivars_mount(const char *mountpoint)
 {
 	struct statfs statbuf;
@@ -534,13 +516,6 @@ static int check_efivars_mount(const char *mountpoint)
 	return 0;
 }
 
-/* for each root directory, top-level first:
- *  for each db/dbx/KEK:
- *   for each file:
- *     if file exists in keystore, skip
- *     add file to keystore
- */
-
 static int keystore_entry_read(struct fs_keystore_entry *ke)
 {
 	const char *path;
@@ -570,10 +545,11 @@ static bool keystore_contains_file(struct fs_keystore *keystore,
 
 static int update_keystore(struct fs_keystore *keystore, const char *root)
 {
+	struct fs_keystore_entry *ke;
 	unsigned int i;
 
 	for (i = 0; i < ARRAY_SIZE(keydb_types); i++) {
-		const char *dirname, *filename;
+		const char *filename, *dirname;
 		struct dirent *dirent;
 		DIR *dir;
 
@@ -585,7 +561,6 @@ static int update_keystore(struct fs_keystore *keystore, const char *root)
 			continue;
 
 		for (dirent = readdir(dir); dirent; dirent = readdir(dir)) {
-			struct fs_keystore_entry *ke;
 
 			if (dirent->d_name[0] == '.')
 				continue;
@@ -612,6 +587,7 @@ static int update_keystore(struct fs_keystore *keystore, const char *root)
 		closedir(dir);
 		talloc_free(dirname);
 	}
+
 	return 0;
 }
 
@@ -650,67 +626,55 @@ static int key_cmp(struct key *a, struct key *b)
 	return memcmp(a->id, b->id, a->id_len);
 }
 
-static int find_new_keys_in_kdb(struct sync_context *ctx,
-		struct key_database *kdb)
+/**
+ * Finds the set-difference of the filesystem and firmware keys, and
+ * populates ctx->new_keys with the keystore_entries that should be
+ * inserted into firmware
+ */
+static int find_new_keys(struct sync_context *ctx)
 {
-	struct fs_keystore_entry *ke;
-	struct key *fs_key, *fw_key;
-	bool found;
+	struct {
+		struct key_database *fs_kdb, *fw_kdb;
+	} kdbs[] = {
+		{ &ctx->filesystem_keys->kek, &ctx->firmware_keys->kek },
+		{ &ctx->filesystem_keys->db,  &ctx->firmware_keys->db },
+		{ &ctx->filesystem_keys->dbx, &ctx->firmware_keys->dbx },
+	};
+	unsigned int i;
 	int n = 0;
 
-	list_for_each(&kdb->filesystem_keys, fs_key, list) {
-		found = false;
-		list_for_each(&kdb->firmware_keys, fw_key, list) {
-			if (!key_cmp(fs_key, fw_key)) {
-				found = true;
-				break;
+	for (i = 0; i < ARRAY_SIZE(kdbs); i++ ) {
+		struct fs_keystore_entry *ke;
+		struct key *fs_key, *fw_key;
+		bool found;
+
+		list_for_each(&kdbs[i].fs_kdb->keys, fs_key, list) {
+			found = false;
+			list_for_each(&kdbs[i].fw_kdb->keys, fw_key, list) {
+				if (!key_cmp(fs_key, fw_key)) {
+					found = true;
+					break;
+				}
 			}
-		}
-		if (found)
-			continue;
+			if (found)
+				continue;
 
-		/* add the keystore entry if it's not already present */
-		found = false;
-		list_for_each(&ctx->new_keys, ke, new_list) {
-			if (fs_key->keystore_entry == ke) {
-				found = true;
-				break;
+			/* add the keystore entry if it's not already present */
+			found = false;
+			list_for_each(&ctx->new_keys, ke, new_list) {
+				if (fs_key->keystore_entry == ke) {
+					found = true;
+					break;
+				}
 			}
-		}
-
-		if (found)
-			continue;
 
-		list_add(&ctx->new_keys, &fs_key->keystore_entry->new_list);
-		n++;
-	}
-
-	return n;
-}
-
-/* Find the keys that are present in the filesystem, but not the firmware.
- * Returns:
- *  0   if there are no new keys to add
- *  >0  if there are keys to add
- *  -1  on error
- */
-static int find_new_keys(struct sync_context *ctx)
-{
-	struct key_database *kdbs[] = {
-		ctx->kek,
-		ctx->db,
-		ctx->dbx,
-	};
-	unsigned int n, i;
-	int rc;
-
-	n = 0;
+			if (found)
+				continue;
 
-	for (i = 0; i < ARRAY_SIZE(kdbs); i++) {
-		rc = find_new_keys_in_kdb(ctx, kdbs[i]);
-		if (rc < 0)
-			return rc;
-		n += rc;
+			list_add(&ctx->new_keys,
+					&fs_key->keystore_entry->new_list);
+			n++;
+		}
 	}
 
 	return n;
@@ -800,19 +764,22 @@ static int insert_new_keys(struct sync_context *ctx)
 	return rc;
 }
 
-static void init_key_database(struct sync_context *ctx,
-		struct key_database **kdb_p,
-		const struct key_database_type *type)
+static struct keyset *init_keyset(struct sync_context *ctx)
 {
-	struct key_database *kdb;
+	struct keyset *keyset;
+
+	keyset = talloc(ctx, struct keyset);
 
-	kdb = talloc(ctx, struct key_database);
+	list_head_init(&keyset->kek.keys);
+	keyset->kek.type = &keydb_types[0];
 
-	list_head_init(&kdb->firmware_keys);
-	list_head_init(&kdb->filesystem_keys);
-	kdb->type = type;
+	list_head_init(&keyset->db.keys);
+	keyset->db.type = &keydb_types[1];
 
-	*kdb_p = kdb;
+	list_head_init(&keyset->dbx.keys);
+	keyset->dbx.type = &keydb_types[2];
+
+	return keyset;
 }
 
 static struct option options[] = {
@@ -904,14 +871,13 @@ int main(int argc, char **argv)
 		return EXIT_FAILURE;
 	}
 
-	init_key_database(ctx, &ctx->kek, &keydb_types[0]);
-	init_key_database(ctx, &ctx->db, &keydb_types[1]);
-	init_key_database(ctx, &ctx->dbx, &keydb_types[2]);
-
 	ERR_load_crypto_strings();
 	OpenSSL_add_all_digests();
 	OpenSSL_add_all_ciphers();
 
+	ctx->filesystem_keys = init_keyset(ctx);
+	ctx->firmware_keys = init_keyset(ctx);
+
 	if (!ctx->efivars_dir) {
 		ctx->efivars_dir = EFIVARS_MOUNTPOINT;
 		if (check_efivars_mount(ctx->efivars_dir)) {
@@ -933,9 +899,11 @@ int main(int argc, char **argv)
 	if (ctx->verbose)
 		print_keystore(ctx->fs_keystore);
 
-	read_key_databases(ctx);
-	if (ctx->verbose)
-		print_key_databases(ctx);
+	read_keysets(ctx);
+	if (ctx->verbose) {
+		print_keyset(ctx->firmware_keys, "firmware");
+		print_keyset(ctx->filesystem_keys, "filesystem");
+	}
 
 	find_new_keys(ctx);
 
@@ -945,5 +913,7 @@ int main(int argc, char **argv)
 	if (!ctx->dry_run)
 		insert_new_keys(ctx);
 
+	talloc_free(ctx);
+
 	return EXIT_SUCCESS;
 }