[PATCH v8 0/5] Enable root to update the blacklist keyring

[PATCH v8 0/5] Enable root to update the blacklist keyring

[Mickaël Salaün]

Hi,

This new patch series is a rebase on v5.14-rc1 .  David or Jarkko, if
it's still OK with you, could you please push this to linux-next?

I successfully tested this patch series with the 211 entries from
https://uefi.org/sites/default/files/resources/dbxupdate_x64.bin

The goal of these patches is to add a new configuration option to enable the
root user to load signed keys in the blacklist keyring.  This keyring is useful
to "untrust" certificates or files.  Enabling to safely update this keyring
without recompiling the kernel makes it more usable.

Previous patch series:
https://lore.kernel.org/lkml/20210312171232.2681989-1-mic@digikod.net/

Regards,

Mickaël Salaün (5):
  tools/certs: Add print-cert-tbs-hash.sh
  certs: Check that builtin blacklist hashes are valid
  certs: Make blacklist_vet_description() more strict
  certs: Factor out the blacklist hash creation
  certs: Allow root user to append signed hashes to the blacklist
    keyring

 MAINTAINERS                                   |   2 +
 certs/.gitignore                              |   1 +
 certs/Kconfig                                 |  17 +-
 certs/Makefile                                |  17 +-
 certs/blacklist.c                             | 218 ++++++++++++++----
 crypto/asymmetric_keys/x509_public_key.c      |   3 +-
 include/keys/system_keyring.h                 |  14 +-
 scripts/check-blacklist-hashes.awk            |  37 +++
 .../platform_certs/keyring_handler.c          |  26 +--
 tools/certs/print-cert-tbs-hash.sh            |  91 ++++++++
 10 files changed, 346 insertions(+), 80 deletions(-)
 create mode 100755 scripts/check-blacklist-hashes.awk
 create mode 100755 tools/certs/print-cert-tbs-hash.sh


base-commit: e73f0f0ee7541171d89f2e2491130c7771ba58d3
-- 
2.32.0

[PATCH v8 1/5] tools/certs: Add print-cert-tbs-hash.sh

[Mickaël Salaün]

From: Mickaël Salaün <mic@linux.microsoft.com>

Add a new helper print-cert-tbs-hash.sh to generate a TBSCertificate
hash from a given certificate.  This is useful to generate a blacklist
key description used to forbid loading a specific certificate in a
keyring, or to invalidate a certificate provided by a PKCS#7 file.

This kind of hash formatting is required to populate the file pointed
out by CONFIG_SYSTEM_BLACKLIST_HASH_LIST, but only the kernel code was
available to understand how to effectively create such hash.

Cc: David Howells <dhowells@redhat.com>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Eric Snowberg <eric.snowberg@oracle.com>
Signed-off-by: Mickaël Salaün <mic@linux.microsoft.com>
Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
Link: https://lore.kernel.org/r/20210712170313.884724-2-mic@digikod.net
---

Changes since v5:
* Add Reviewed-by Jarkko.

Changes since v3:
* Explain in the commit message that this kind of formating is not new
  but it wasn't documented.

Changes since v1:
* Fix typo.
* Use "if" block instead of "||" .
---
 MAINTAINERS                        |  1 +
 tools/certs/print-cert-tbs-hash.sh | 91 ++++++++++++++++++++++++++++++
 2 files changed, 92 insertions(+)
 create mode 100755 tools/certs/print-cert-tbs-hash.sh

diff --git a/MAINTAINERS b/MAINTAINERS
index a61f4f3b78a9..0b3be78d27ef 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4301,6 +4301,7 @@ F:	Documentation/admin-guide/module-signing.rst
 F:	certs/
 F:	scripts/extract-cert.c
 F:	scripts/sign-file.c
+F:	tools/certs/
 
 CFAG12864B LCD DRIVER
 M:	Miguel Ojeda <ojeda@kernel.org>
diff --git a/tools/certs/print-cert-tbs-hash.sh b/tools/certs/print-cert-tbs-hash.sh
new file mode 100755
index 000000000000..c93df5387ec9
--- /dev/null
+++ b/tools/certs/print-cert-tbs-hash.sh
@@ -0,0 +1,91 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+#
+# Copyright © 2020, Microsoft Corporation. All rights reserved.
+#
+# Author: Mickaël Salaün <mic@linux.microsoft.com>
+#
+# Compute and print the To Be Signed (TBS) hash of a certificate.  This is used
+# as description of keys in the blacklist keyring to identify certificates.
+# This output should be redirected, without newline, in a file (hash0.txt) and
+# signed to create a PKCS#7 file (hash0.p7s).  Both of these files can then be
+# loaded in the kernel with.
+#
+# Exemple on a workstation:
+# ./print-cert-tbs-hash.sh certificate-to-invalidate.pem > hash0.txt
+# openssl smime -sign -in hash0.txt -inkey builtin-private-key.pem \
+#               -signer builtin-certificate.pem -certfile certificate-chain.pem \
+#               -noattr -binary -outform DER -out hash0.p7s
+#
+# Exemple on a managed system:
+# keyctl padd blacklist "$(< hash0.txt)" %:.blacklist < hash0.p7s
+
+set -u -e -o pipefail
+
+CERT="${1:-}"
+BASENAME="$(basename -- "${BASH_SOURCE[0]}")"
+
+if [ $# -ne 1 ] || [ ! -f "${CERT}" ]; then
+	echo "usage: ${BASENAME} <certificate>" >&2
+	exit 1
+fi
+
+# Checks that it is indeed a certificate (PEM or DER encoded) and exclude the
+# optional PEM text header.
+if ! PEM="$(openssl x509 -inform DER -in "${CERT}" 2>/dev/null || openssl x509 -in "${CERT}")"; then
+	echo "ERROR: Failed to parse certificate" >&2
+	exit 1
+fi
+
+# TBSCertificate starts at the second entry.
+# Cf. https://tools.ietf.org/html/rfc3280#section-4.1
+#
+# Exemple of first lines printed by openssl asn1parse:
+#    0:d=0  hl=4 l= 763 cons: SEQUENCE
+#    4:d=1  hl=4 l= 483 cons: SEQUENCE
+#    8:d=2  hl=2 l=   3 cons: cont [ 0 ]
+#   10:d=3  hl=2 l=   1 prim: INTEGER           :02
+#   13:d=2  hl=2 l=  20 prim: INTEGER           :3CEB2CB8818D968AC00EEFE195F0DF9665328B7B
+#   35:d=2  hl=2 l=  13 cons: SEQUENCE
+#   37:d=3  hl=2 l=   9 prim: OBJECT            :sha256WithRSAEncryption
+RANGE_AND_DIGEST_RE='
+2s/^\s*\([0-9]\+\):d=\s*[0-9]\+\s\+hl=\s*[0-9]\+\s\+l=\s*\([0-9]\+\)\s\+cons:\s*SEQUENCE\s*$/\1 \2/p;
+7s/^\s*[0-9]\+:d=\s*[0-9]\+\s\+hl=\s*[0-9]\+\s\+l=\s*[0-9]\+\s\+prim:\s*OBJECT\s*:\(.*\)$/\1/p;
+'
+
+RANGE_AND_DIGEST=($(echo "${PEM}" | \
+	openssl asn1parse -in - | \
+	sed -n -e "${RANGE_AND_DIGEST_RE}"))
+
+if [ "${#RANGE_AND_DIGEST[@]}" != 3 ]; then
+	echo "ERROR: Failed to parse TBSCertificate." >&2
+	exit 1
+fi
+
+OFFSET="${RANGE_AND_DIGEST[0]}"
+END="$(( OFFSET + RANGE_AND_DIGEST[1] ))"
+DIGEST="${RANGE_AND_DIGEST[2]}"
+
+# The signature hash algorithm is used by Linux to blacklist certificates.
+# Cf. crypto/asymmetric_keys/x509_cert_parser.c:x509_note_pkey_algo()
+DIGEST_MATCH=""
+while read -r DIGEST_ITEM; do
+	if [ -z "${DIGEST_ITEM}" ]; then
+		break
+	fi
+	if echo "${DIGEST}" | grep -qiF "${DIGEST_ITEM}"; then
+		DIGEST_MATCH="${DIGEST_ITEM}"
+		break
+	fi
+done < <(openssl list -digest-commands | tr ' ' '\n' | sort -ur)
+
+if [ -z "${DIGEST_MATCH}" ]; then
+	echo "ERROR: Unknown digest algorithm: ${DIGEST}" >&2
+	exit 1
+fi
+
+echo "${PEM}" | \
+	openssl x509 -in - -outform DER | \
+	dd "bs=1" "skip=${OFFSET}" "count=${END}" "status=none" | \
+	openssl dgst "-${DIGEST_MATCH}" - | \
+	awk '{printf "tbs:" $2}'
-- 
2.32.0

[PATCH v8 2/5] certs: Check that builtin blacklist hashes are valid

[Mickaël Salaün]

From: Mickaël Salaün <mic@linux.microsoft.com>

Add and use a check-blacklist-hashes.awk script to make sure that the
builtin blacklist hashes set with CONFIG_SYSTEM_BLACKLIST_HASH_LIST will
effectively be taken into account as blacklisted hashes.  This is useful
to debug invalid hash formats, and it make sure that previous hashes
which could have been loaded in the kernel, but silently ignored, are
now noticed and deal with by the user at kernel build time.

This also prevent stricter blacklist key description checking (provided
by following commits) to failed for builtin hashes.

Update CONFIG_SYSTEM_BLACKLIST_HASH_LIST help to explain the content of
a hash string and how to generate certificate ones.

Cc: David Howells <dhowells@redhat.com>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Eric Snowberg <eric.snowberg@oracle.com>
Cc: Jarkko Sakkinen <jarkko@kernel.org>
Signed-off-by: Mickaël Salaün <mic@linux.microsoft.com>
Link: https://lore.kernel.org/r/20210712170313.884724-3-mic@digikod.net
---

Changes since v7:
* Rebase and fix trivial .gitignore conflict.

Changes since v5:
* Rebase on keys-next and fix conflict as previously done by David
  Howells.
* Enable to use a file path relative to the kernel source directory.
  This align with the handling of CONFIG_SYSTEM_TRUSTED_KEYS,
  CONFIG_MODULE_SIG_KEY and CONFIG_SYSTEM_REVOCATION_KEYS.

Changes since v3:
* Improve commit description.
* Update CONFIG_SYSTEM_BLACKLIST_HASH_LIST help.
* Remove Acked-by Jarkko Sakkinen because of the above changes.

Changes since v2:
* Add Jarkko's Acked-by.

Changes since v1:
* Prefix script path with $(scrtree)/ (suggested by David Howells).
* Fix hexadecimal number check.
---
 MAINTAINERS                        |  1 +
 certs/.gitignore                   |  1 +
 certs/Kconfig                      |  7 ++++--
 certs/Makefile                     | 17 +++++++++++++-
 scripts/check-blacklist-hashes.awk | 37 ++++++++++++++++++++++++++++++
 5 files changed, 60 insertions(+), 3 deletions(-)
 create mode 100755 scripts/check-blacklist-hashes.awk

diff --git a/MAINTAINERS b/MAINTAINERS
index 0b3be78d27ef..04b8fb5dcdac 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4299,6 +4299,7 @@ L:	keyrings@vger.kernel.org
 S:	Maintained
 F:	Documentation/admin-guide/module-signing.rst
 F:	certs/
+F:	scripts/check-blacklist-hashes.awk
 F:	scripts/extract-cert.c
 F:	scripts/sign-file.c
 F:	tools/certs/
diff --git a/certs/.gitignore b/certs/.gitignore
index 8c3763f80be3..01de9442e4e2 100644
--- a/certs/.gitignore
+++ b/certs/.gitignore
@@ -1,3 +1,4 @@
 # SPDX-License-Identifier: GPL-2.0-only
+/blacklist_hashes_checked
 /x509_certificate_list
 /x509_revocation_list
diff --git a/certs/Kconfig b/certs/Kconfig
index f4e61116f94e..0fbe184ceca5 100644
--- a/certs/Kconfig
+++ b/certs/Kconfig
@@ -80,8 +80,11 @@ config SYSTEM_BLACKLIST_HASH_LIST
 	help
 	  If set, this option should be the filename of a list of hashes in the
 	  form "<hash>", "<hash>", ... .  This will be included into a C
-	  wrapper to incorporate the list into the kernel.  Each <hash> should
-	  be a string of hex digits.
+	  wrapper to incorporate the list into the kernel.  Each <hash> must be a
+	  string starting with a prefix ("tbs" or "bin"), then a colon (":"), and
+	  finally an even number of hexadecimal lowercase characters (up to 128).
+	  Certificate hashes can be generated with
+	  tools/certs/print-cert-tbs-hash.sh .
 
 config SYSTEM_REVOCATION_LIST
 	bool "Provide system-wide ring of revocation certificates"
diff --git a/certs/Makefile b/certs/Makefile
index 359239a0ee9e..e43c1a8032de 100644
--- a/certs/Makefile
+++ b/certs/Makefile
@@ -7,7 +7,22 @@ obj-$(CONFIG_SYSTEM_TRUSTED_KEYRING) += system_keyring.o system_certificates.o c
 obj-$(CONFIG_SYSTEM_BLACKLIST_KEYRING) += blacklist.o common.o
 obj-$(CONFIG_SYSTEM_REVOCATION_LIST) += revocation_certificates.o
 ifneq ($(CONFIG_SYSTEM_BLACKLIST_HASH_LIST),"")
+
+quiet_cmd_check_blacklist_hashes = CHECK   $(patsubst "%",%,$(2))
+      cmd_check_blacklist_hashes = $(AWK) -f $(srctree)/scripts/check-blacklist-hashes.awk $(2); touch $@
+
+$(eval $(call config_filename,SYSTEM_BLACKLIST_HASH_LIST))
+
+$(obj)/blacklist_hashes.o: $(obj)/blacklist_hashes_checked
+
+CFLAGS_blacklist_hashes.o += -I$(srctree)
+
+targets += blacklist_hashes_checked
+$(obj)/blacklist_hashes_checked: $(SYSTEM_BLACKLIST_HASH_LIST_SRCPREFIX)$(SYSTEM_BLACKLIST_HASH_LIST_FILENAME) scripts/check-blacklist-hashes.awk FORCE
+	$(call if_changed,check_blacklist_hashes,$(SYSTEM_BLACKLIST_HASH_LIST_SRCPREFIX)$(CONFIG_SYSTEM_BLACKLIST_HASH_LIST))
+
 obj-$(CONFIG_SYSTEM_BLACKLIST_KEYRING) += blacklist_hashes.o
+
 else
 obj-$(CONFIG_SYSTEM_BLACKLIST_KEYRING) += blacklist_nohashes.o
 endif
@@ -30,7 +45,7 @@ $(obj)/x509_certificate_list: scripts/extract-cert $(SYSTEM_TRUSTED_KEYS_SRCPREF
 	$(call if_changed,extract_certs,$(SYSTEM_TRUSTED_KEYS_SRCPREFIX)$(CONFIG_SYSTEM_TRUSTED_KEYS))
 endif # CONFIG_SYSTEM_TRUSTED_KEYRING
 
-clean-files := x509_certificate_list .x509.list x509_revocation_list
+clean-files := x509_certificate_list .x509.list x509_revocation_list blacklist_hashes_checked
 
 ifeq ($(CONFIG_MODULE_SIG),y)
 	SIGN_KEY = y
diff --git a/scripts/check-blacklist-hashes.awk b/scripts/check-blacklist-hashes.awk
new file mode 100755
index 000000000000..107c1d3204d4
--- /dev/null
+++ b/scripts/check-blacklist-hashes.awk
@@ -0,0 +1,37 @@
+#!/usr/bin/awk -f
+# SPDX-License-Identifier: GPL-2.0
+#
+# Copyright © 2020, Microsoft Corporation. All rights reserved.
+#
+# Author: Mickaël Salaün <mic@linux.microsoft.com>
+#
+# Check that a CONFIG_SYSTEM_BLACKLIST_HASH_LIST file contains a valid array of
+# hash strings.  Such string must start with a prefix ("tbs" or "bin"), then a
+# colon (":"), and finally an even number of hexadecimal lowercase characters
+# (up to 128).
+
+BEGIN {
+	RS = ","
+}
+{
+	if (!match($0, "^[ \t\n\r]*\"([^\"]*)\"[ \t\n\r]*$", part1)) {
+		print "Not a string (item " NR "):", $0;
+		exit 1;
+	}
+	if (!match(part1[1], "^(tbs|bin):(.*)$", part2)) {
+		print "Unknown prefix (item " NR "):", part1[1];
+		exit 1;
+	}
+	if (!match(part2[2], "^([0-9a-f]+)$", part3)) {
+		print "Not a lowercase hexadecimal string (item " NR "):", part2[2];
+		exit 1;
+	}
+	if (length(part3[1]) > 128) {
+		print "Hash string too long (item " NR "):", part3[1];
+		exit 1;
+	}
+	if (length(part3[1]) % 2 == 1) {
+		print "Not an even number of hexadecimal characters (item " NR "):", part3[1];
+		exit 1;
+	}
+}
-- 
2.32.0

[PATCH v8 3/5] certs: Make blacklist_vet_description() more strict

[Mickaël Salaün]

From: Mickaël Salaün <mic@linux.microsoft.com>

Before exposing this new key type to user space, make sure that only
meaningful blacklisted hashes are accepted.  This is also checked for
builtin blacklisted hashes, but a following commit make sure that the
user will notice (at built time) and will fix the configuration if it
already included errors.

Check that a blacklist key description starts with a valid prefix and
then a valid hexadecimal string.

Cc: David Howells <dhowells@redhat.com>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Eric Snowberg <eric.snowberg@oracle.com>
Signed-off-by: Mickaël Salaün <mic@linux.microsoft.com>
Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
Link: https://lore.kernel.org/r/20210712170313.884724-4-mic@digikod.net
---

Changes since v5:
* Add Reviewed-by Jarkko.

Changes since v2:
* Fix typo in blacklist_vet_description() comment, spotted by Tyler
  Hicks.
* Add Jarkko's Acked-by.

Changes since v1:
* Return ENOPKG (instead of EINVAL) when a hash is greater than the
  maximum currently known hash (suggested by David Howells).
---
 certs/blacklist.c | 46 ++++++++++++++++++++++++++++++++++++----------
 1 file changed, 36 insertions(+), 10 deletions(-)

diff --git a/certs/blacklist.c b/certs/blacklist.c
index c9a435b15af4..97a35cf9a62c 100644
--- a/certs/blacklist.c
+++ b/certs/blacklist.c
@@ -19,6 +19,16 @@
 #include "blacklist.h"
 #include "common.h"
 
+/*
+ * According to crypto/asymmetric_keys/x509_cert_parser.c:x509_note_pkey_algo(),
+ * the size of the currently longest supported hash algorithm is 512 bits,
+ * which translates into 128 hex characters.
+ */
+#define MAX_HASH_LEN	128
+
+static const char tbs_prefix[] = "tbs";
+static const char bin_prefix[] = "bin";
+
 static struct key *blacklist_keyring;
 
 #ifdef CONFIG_SYSTEM_REVOCATION_LIST
@@ -32,24 +42,40 @@ extern __initconst const unsigned long revocation_certificate_list_size;
  */
 static int blacklist_vet_description(const char *desc)
 {
-	int n = 0;
-
-	if (*desc == ':')
-		return -EINVAL;
-	for (; *desc; desc++)
-		if (*desc == ':')
-			goto found_colon;
+	int i, prefix_len, tbs_step = 0, bin_step = 0;
+
+	/* The following algorithm only works if prefix lengths match. */
+	BUILD_BUG_ON(sizeof(tbs_prefix) != sizeof(bin_prefix));
+	prefix_len = sizeof(tbs_prefix) - 1;
+	for (i = 0; *desc; desc++, i++) {
+		if (*desc == ':') {
+			if (tbs_step == prefix_len)
+				goto found_colon;
+			if (bin_step == prefix_len)
+				goto found_colon;
+			return -EINVAL;
+		}
+		if (i >= prefix_len)
+			return -EINVAL;
+		if (*desc == tbs_prefix[i])
+			tbs_step++;
+		if (*desc == bin_prefix[i])
+			bin_step++;
+	}
 	return -EINVAL;
 
 found_colon:
 	desc++;
-	for (; *desc; desc++) {
+	for (i = 0; *desc && i < MAX_HASH_LEN; desc++, i++) {
 		if (!isxdigit(*desc) || isupper(*desc))
 			return -EINVAL;
-		n++;
 	}
+	if (*desc)
+		/* The hash is greater than MAX_HASH_LEN. */
+		return -ENOPKG;
 
-	if (n == 0 || n & 1)
+	/* Checks for an even number of hexadecimal characters. */
+	if (i == 0 || i & 1)
 		return -EINVAL;
 	return 0;
 }
-- 
2.32.0

[PATCH v8 4/5] certs: Factor out the blacklist hash creation

[Mickaël Salaün]

From: Mickaël Salaün <mic@linux.microsoft.com>

Factor out the blacklist hash creation with the get_raw_hash() helper.
This also centralize the "tbs" and "bin" prefixes and make them private,
which help to manage them consistently.

Cc: David Howells <dhowells@redhat.com>
Cc: David S. Miller <davem@davemloft.net>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Eric Snowberg <eric.snowberg@oracle.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: Jarkko Sakkinen <jarkko@kernel.org>
Signed-off-by: Mickaël Salaün <mic@linux.microsoft.com>
Link: https://lore.kernel.org/r/20210712170313.884724-5-mic@digikod.net
---

Changes since v6:
* Rebase on keys-cve-2020-26541-v3: commit ebd9c2ae369a ("integrity:
  Load mokx variables into the blacklist keyring").

Changes since v5:
* Rebase on keys-next and fix conflict as previously done by David
  Howells.
* Fix missing part to effectively handle UEFI DBX blacklisting.
* Remove Jarkko's Acked-by because of the above changes.

Changes since v2:
* Add Jarkko's Acked-by.
---
 certs/blacklist.c                             | 76 ++++++++++++++-----
 crypto/asymmetric_keys/x509_public_key.c      |  3 +-
 include/keys/system_keyring.h                 | 14 +++-
 .../platform_certs/keyring_handler.c          | 26 +------
 4 files changed, 73 insertions(+), 46 deletions(-)

diff --git a/certs/blacklist.c b/certs/blacklist.c
index 97a35cf9a62c..b254c87ceb3a 100644
--- a/certs/blacklist.c
+++ b/certs/blacklist.c
@@ -109,11 +109,43 @@ static struct key_type key_type_blacklist = {
 	.describe		= blacklist_describe,
 };
 
+static char *get_raw_hash(const u8 *hash, size_t hash_len,
+		enum blacklist_hash_type hash_type)
+{
+	size_t type_len;
+	const char *type_prefix;
+	char *buffer, *p;
+
+	switch (hash_type) {
+	case BLACKLIST_HASH_X509_TBS:
+		type_len = sizeof(tbs_prefix) - 1;
+		type_prefix = tbs_prefix;
+		break;
+	case BLACKLIST_HASH_BINARY:
+		type_len = sizeof(bin_prefix) - 1;
+		type_prefix = bin_prefix;
+		break;
+	default:
+		WARN_ON_ONCE(1);
+		return ERR_PTR(-EINVAL);
+	}
+	buffer = kmalloc(type_len + 1 + hash_len * 2 + 1, GFP_KERNEL);
+	if (!buffer)
+		return ERR_PTR(-ENOMEM);
+	p = memcpy(buffer, type_prefix, type_len);
+	p += type_len;
+	*p++ = ':';
+	bin2hex(p, hash, hash_len);
+	p += hash_len * 2;
+	*p = '\0';
+	return buffer;
+}
+
 /**
- * mark_hash_blacklisted - Add a hash to the system blacklist
+ * mark_raw_hash_blacklisted - Add a hash to the system blacklist
  * @hash: The hash as a hex string with a type prefix (eg. "tbs:23aa429783")
  */
-int mark_hash_blacklisted(const char *hash)
+static int mark_raw_hash_blacklisted(const char *hash)
 {
 	key_ref_t key;
 
@@ -133,29 +165,36 @@ int mark_hash_blacklisted(const char *hash)
 	return 0;
 }
 
+int mark_hash_blacklisted(const u8 *hash, size_t hash_len,
+		enum blacklist_hash_type hash_type)
+{
+	const char *buffer;
+	int err;
+
+	buffer = get_raw_hash(hash, hash_len, hash_type);
+	if (IS_ERR(buffer))
+		return PTR_ERR(buffer);
+	err = mark_raw_hash_blacklisted(buffer);
+	kfree(buffer);
+	return err;
+}
+
 /**
  * is_hash_blacklisted - Determine if a hash is blacklisted
  * @hash: The hash to be checked as a binary blob
  * @hash_len: The length of the binary hash
- * @type: Type of hash
+ * @hash_type: Type of hash
  */
-int is_hash_blacklisted(const u8 *hash, size_t hash_len, const char *type)
+int is_hash_blacklisted(const u8 *hash, size_t hash_len,
+		enum blacklist_hash_type hash_type)
 {
 	key_ref_t kref;
-	size_t type_len = strlen(type);
-	char *buffer, *p;
+	const char *buffer;
 	int ret = 0;
 
-	buffer = kmalloc(type_len + 1 + hash_len * 2 + 1, GFP_KERNEL);
-	if (!buffer)
-		return -ENOMEM;
-	p = memcpy(buffer, type, type_len);
-	p += type_len;
-	*p++ = ':';
-	bin2hex(p, hash, hash_len);
-	p += hash_len * 2;
-	*p = 0;
-
+	buffer = get_raw_hash(hash, hash_len, hash_type);
+	if (IS_ERR(buffer))
+		return PTR_ERR(buffer);
 	kref = keyring_search(make_key_ref(blacklist_keyring, true),
 			      &key_type_blacklist, buffer, false);
 	if (!IS_ERR(kref)) {
@@ -170,7 +209,8 @@ EXPORT_SYMBOL_GPL(is_hash_blacklisted);
 
 int is_binary_blacklisted(const u8 *hash, size_t hash_len)
 {
-	if (is_hash_blacklisted(hash, hash_len, "bin") == -EKEYREJECTED)
+	if (is_hash_blacklisted(hash, hash_len, BLACKLIST_HASH_BINARY) ==
+			-EKEYREJECTED)
 		return -EPERM;
 
 	return 0;
@@ -243,7 +283,7 @@ static int __init blacklist_init(void)
 		panic("Can't allocate system blacklist keyring\n");
 
 	for (bl = blacklist_hashes; *bl; bl++)
-		if (mark_hash_blacklisted(*bl) < 0)
+		if (mark_raw_hash_blacklisted(*bl) < 0)
 			pr_err("- blacklisting failed\n");
 	return 0;
 }
diff --git a/crypto/asymmetric_keys/x509_public_key.c b/crypto/asymmetric_keys/x509_public_key.c
index 3d45161b271a..4b3d5166643b 100644
--- a/crypto/asymmetric_keys/x509_public_key.c
+++ b/crypto/asymmetric_keys/x509_public_key.c
@@ -81,7 +81,8 @@ int x509_get_sig_params(struct x509_certificate *cert)
 	if (ret < 0)
 		goto error_2;
 
-	ret = is_hash_blacklisted(sig->digest, sig->digest_size, "tbs");
+	ret = is_hash_blacklisted(sig->digest, sig->digest_size,
+				  BLACKLIST_HASH_X509_TBS);
 	if (ret == -EKEYREJECTED) {
 		pr_err("Cert %*phN is blacklisted\n",
 		       sig->digest_size, sig->digest);
diff --git a/include/keys/system_keyring.h b/include/keys/system_keyring.h
index 6acd3cf13a18..1efb52bc494f 100644
--- a/include/keys/system_keyring.h
+++ b/include/keys/system_keyring.h
@@ -10,6 +10,13 @@
 
 #include <linux/key.h>
 
+enum blacklist_hash_type {
+	/* TBSCertificate hash */
+	BLACKLIST_HASH_X509_TBS = 1,
+	/* Raw data hash */
+	BLACKLIST_HASH_BINARY = 2,
+};
+
 #ifdef CONFIG_SYSTEM_TRUSTED_KEYRING
 
 extern int restrict_link_by_builtin_trusted(struct key *keyring,
@@ -40,13 +47,14 @@ extern int restrict_link_by_builtin_and_secondary_trusted(
 
 extern struct pkcs7_message *pkcs7;
 #ifdef CONFIG_SYSTEM_BLACKLIST_KEYRING
-extern int mark_hash_blacklisted(const char *hash);
+extern int mark_hash_blacklisted(const u8 *hash, size_t hash_len,
+			       enum blacklist_hash_type hash_type);
 extern int is_hash_blacklisted(const u8 *hash, size_t hash_len,
-			       const char *type);
+			       enum blacklist_hash_type hash_type);
 extern int is_binary_blacklisted(const u8 *hash, size_t hash_len);
 #else
 static inline int is_hash_blacklisted(const u8 *hash, size_t hash_len,
-				      const char *type)
+				      enum blacklist_hash_type hash_type)
 {
 	return 0;
 }
diff --git a/security/integrity/platform_certs/keyring_handler.c b/security/integrity/platform_certs/keyring_handler.c
index 5604bd57c990..9e4f156b356e 100644
--- a/security/integrity/platform_certs/keyring_handler.c
+++ b/security/integrity/platform_certs/keyring_handler.c
@@ -15,35 +15,13 @@ static efi_guid_t efi_cert_x509_sha256_guid __initdata =
 	EFI_CERT_X509_SHA256_GUID;
 static efi_guid_t efi_cert_sha256_guid __initdata = EFI_CERT_SHA256_GUID;
 
-/*
- * Blacklist a hash.
- */
-static __init void uefi_blacklist_hash(const char *source, const void *data,
-				       size_t len, const char *type,
-				       size_t type_len)
-{
-	char *hash, *p;
-
-	hash = kmalloc(type_len + len * 2 + 1, GFP_KERNEL);
-	if (!hash)
-		return;
-	p = memcpy(hash, type, type_len);
-	p += type_len;
-	bin2hex(p, data, len);
-	p += len * 2;
-	*p = 0;
-
-	mark_hash_blacklisted(hash);
-	kfree(hash);
-}
-
 /*
  * Blacklist an X509 TBS hash.
  */
 static __init void uefi_blacklist_x509_tbs(const char *source,
 					   const void *data, size_t len)
 {
-	uefi_blacklist_hash(source, data, len, "tbs:", 4);
+	mark_hash_blacklisted(data, len, BLACKLIST_HASH_X509_TBS);
 }
 
 /*
@@ -52,7 +30,7 @@ static __init void uefi_blacklist_x509_tbs(const char *source,
 static __init void uefi_blacklist_binary(const char *source,
 					 const void *data, size_t len)
 {
-	uefi_blacklist_hash(source, data, len, "bin:", 4);
+	mark_hash_blacklisted(data, len, BLACKLIST_HASH_BINARY);
 }
 
 /*
-- 
2.32.0

[PATCH v8 5/5] certs: Allow root user to append signed hashes to the blacklist keyring

[Mickaël Salaün]

From: Mickaël Salaün <mic@linux.microsoft.com>

Add a kernel option SYSTEM_BLACKLIST_AUTH_UPDATE to enable the root user
to dynamically add new keys to the blacklist keyring.  This enables to
invalidate new certificates, either from being loaded in a keyring, or
from being trusted in a PKCS#7 certificate chain.  This also enables to
add new file hashes to be denied by the integrity infrastructure.

Being able to untrust a certificate which could have normaly been
trusted is a sensitive operation.  This is why adding new hashes to the
blacklist keyring is only allowed when these hashes are signed and
vouched by the builtin trusted keyring.  A blacklist hash is stored as a
key description.  The PKCS#7 signature of this description must be
provided as the key payload.

Marking a certificate as untrusted should be enforced while the system
is running.  It is then forbiden to remove such blacklist keys.

Update blacklist keyring, blacklist key and revoked certificate access rights:
* allows the root user to search for a specific blacklisted hash, which
  make sense because the descriptions are already viewable;
* forbids key update (blacklist and asymmetric ones);
* restricts kernel rights on the blacklist keyring to align with the
  root user rights.

See help in tools/certs/print-cert-tbs-hash.sh .

Cc: David Howells <dhowells@redhat.com>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Eric Snowberg <eric.snowberg@oracle.com>
Cc: Jarkko Sakkinen <jarkko@kernel.org>
Signed-off-by: Mickaël Salaün <mic@linux.microsoft.com>
Link: https://lore.kernel.org/r/20210712170313.884724-6-mic@digikod.net
---

Changes since v6:
* Rebase on keys-cve-2020-26541-v3: commit ebd9c2ae369a ("integrity:
  Load mokx variables into the blacklist keyring").

Changes since v5:
* Rebase on keys-next, fix Kconfig conflict, and update the asymmetric
  key rights added to the blacklist keyring by the new
  add_key_to_revocation_list(): align with blacklist key rights by
  removing KEY_POS_WRITE as a safeguard, and add
  KEY_ALLOC_BYPASS_RESTRICTION to not be subject to
  restrict_link_for_blacklist() that only allows blacklist key types to
  be added to the keyring.
* Change the return code for restrict_link_for_blacklist() from -EPERM
  to -EOPNOTSUPP to align with asymmetric key keyrings.

Changes since v3:
* Update commit message for print-cert-tbs-hash.sh .

Changes since v2:
* Add comment for blacklist_key_instantiate().
---
 certs/Kconfig     | 10 +++++
 certs/blacklist.c | 96 ++++++++++++++++++++++++++++++++++++-----------
 2 files changed, 85 insertions(+), 21 deletions(-)

diff --git a/certs/Kconfig b/certs/Kconfig
index 0fbe184ceca5..e0e524b7eff9 100644
--- a/certs/Kconfig
+++ b/certs/Kconfig
@@ -103,4 +103,14 @@ config SYSTEM_REVOCATION_KEYS
 	  containing X.509 certificates to be included in the default blacklist
 	  keyring.
 
+config SYSTEM_BLACKLIST_AUTH_UPDATE
+	bool "Allow root to add signed blacklist keys"
+	depends on SYSTEM_BLACKLIST_KEYRING
+	depends on SYSTEM_DATA_VERIFICATION
+	help
+	  If set, provide the ability to load new blacklist keys at run time if
+	  they are signed and vouched by a certificate from the builtin trusted
+	  keyring.  The PKCS#7 signature of the description is set in the key
+	  payload.  Blacklist keys cannot be removed.
+
 endmenu
diff --git a/certs/blacklist.c b/certs/blacklist.c
index b254c87ceb3a..486ce0dd8e9c 100644
--- a/certs/blacklist.c
+++ b/certs/blacklist.c
@@ -15,6 +15,7 @@
 #include <linux/err.h>
 #include <linux/seq_file.h>
 #include <linux/uidgid.h>
+#include <linux/verification.h>
 #include <keys/system_keyring.h>
 #include "blacklist.h"
 #include "common.h"
@@ -26,6 +27,9 @@
  */
 #define MAX_HASH_LEN	128
 
+#define BLACKLIST_KEY_PERM (KEY_POS_SEARCH | KEY_POS_VIEW | \
+			    KEY_USR_SEARCH | KEY_USR_VIEW)
+
 static const char tbs_prefix[] = "tbs";
 static const char bin_prefix[] = "bin";
 
@@ -80,19 +84,51 @@ static int blacklist_vet_description(const char *desc)
 	return 0;
 }
 
-/*
- * The hash to be blacklisted is expected to be in the description.  There will
- * be no payload.
- */
-static int blacklist_preparse(struct key_preparsed_payload *prep)
+static int blacklist_key_instantiate(struct key *key,
+		struct key_preparsed_payload *prep)
 {
-	if (prep->datalen > 0)
-		return -EINVAL;
-	return 0;
+#ifdef CONFIG_SYSTEM_BLACKLIST_AUTH_UPDATE
+	int err;
+#endif
+
+	/* Sets safe default permissions for keys loaded by user space. */
+	key->perm = BLACKLIST_KEY_PERM;
+
+	/*
+	 * Skips the authentication step for builtin hashes, they are not
+	 * signed but still trusted.
+	 */
+	if (key->flags & (1 << KEY_FLAG_BUILTIN))
+		goto out;
+
+#ifdef CONFIG_SYSTEM_BLACKLIST_AUTH_UPDATE
+	/*
+	 * Verifies the description's PKCS#7 signature against the builtin
+	 * trusted keyring.
+	 */
+	err = verify_pkcs7_signature(key->description,
+			strlen(key->description), prep->data, prep->datalen,
+			NULL, VERIFYING_UNSPECIFIED_SIGNATURE, NULL, NULL);
+	if (err)
+		return err;
+#else
+	/*
+	 * It should not be possible to come here because the keyring doesn't
+	 * have KEY_USR_WRITE and the only other way to call this function is
+	 * for builtin hashes.
+	 */
+	WARN_ON_ONCE(1);
+	return -EPERM;
+#endif
+
+out:
+	return generic_key_instantiate(key, prep);
 }
 
-static void blacklist_free_preparse(struct key_preparsed_payload *prep)
+static int blacklist_key_update(struct key *key,
+		struct key_preparsed_payload *prep)
 {
+	return -EPERM;
 }
 
 static void blacklist_describe(const struct key *key, struct seq_file *m)
@@ -103,9 +139,8 @@ static void blacklist_describe(const struct key *key, struct seq_file *m)
 static struct key_type key_type_blacklist = {
 	.name			= "blacklist",
 	.vet_description	= blacklist_vet_description,
-	.preparse		= blacklist_preparse,
-	.free_preparse		= blacklist_free_preparse,
-	.instantiate		= generic_key_instantiate,
+	.instantiate		= blacklist_key_instantiate,
+	.update			= blacklist_key_update,
 	.describe		= blacklist_describe,
 };
 
@@ -154,8 +189,7 @@ static int mark_raw_hash_blacklisted(const char *hash)
 				   hash,
 				   NULL,
 				   0,
-				   ((KEY_POS_ALL & ~KEY_POS_SETATTR) |
-				    KEY_USR_VIEW),
+				   BLACKLIST_KEY_PERM,
 				   KEY_ALLOC_NOT_IN_QUOTA |
 				   KEY_ALLOC_BUILT_IN);
 	if (IS_ERR(key)) {
@@ -232,8 +266,10 @@ int add_key_to_revocation_list(const char *data, size_t size)
 				   NULL,
 				   data,
 				   size,
-				   ((KEY_POS_ALL & ~KEY_POS_SETATTR) | KEY_USR_VIEW),
-				   KEY_ALLOC_NOT_IN_QUOTA | KEY_ALLOC_BUILT_IN);
+				   KEY_POS_VIEW | KEY_POS_READ | KEY_POS_SEARCH
+				   | KEY_USR_VIEW,
+				   KEY_ALLOC_NOT_IN_QUOTA | KEY_ALLOC_BUILT_IN
+				   | KEY_ALLOC_BYPASS_RESTRICTION);
 
 	if (IS_ERR(key)) {
 		pr_err("Problem with revocation key (%ld)\n", PTR_ERR(key));
@@ -260,25 +296,43 @@ int is_key_on_revocation_list(struct pkcs7_message *pkcs7)
 }
 #endif
 
+static int restrict_link_for_blacklist(struct key *dest_keyring,
+		const struct key_type *type, const union key_payload *payload,
+		struct key *restrict_key)
+{
+	if (type == &key_type_blacklist)
+		return 0;
+	return -EOPNOTSUPP;
+}
+
 /*
  * Initialise the blacklist
  */
 static int __init blacklist_init(void)
 {
 	const char *const *bl;
+	struct key_restriction *restriction;
 
 	if (register_key_type(&key_type_blacklist) < 0)
 		panic("Can't allocate system blacklist key type\n");
 
+	restriction = kzalloc(sizeof(*restriction), GFP_KERNEL);
+	if (!restriction)
+		panic("Can't allocate blacklist keyring restriction\n");
+	restriction->check = restrict_link_for_blacklist;
+
 	blacklist_keyring =
 		keyring_alloc(".blacklist",
 			      GLOBAL_ROOT_UID, GLOBAL_ROOT_GID, current_cred(),
-			      (KEY_POS_ALL & ~KEY_POS_SETATTR) |
-			      KEY_USR_VIEW | KEY_USR_READ |
-			      KEY_USR_SEARCH,
-			      KEY_ALLOC_NOT_IN_QUOTA |
+			      KEY_POS_VIEW | KEY_POS_READ | KEY_POS_SEARCH |
+			      KEY_POS_WRITE |
+			      KEY_USR_VIEW | KEY_USR_READ | KEY_USR_SEARCH
+#ifdef CONFIG_SYSTEM_BLACKLIST_AUTH_UPDATE
+			      | KEY_USR_WRITE
+#endif
+			      , KEY_ALLOC_NOT_IN_QUOTA |
 			      KEY_ALLOC_SET_KEEP,
-			      NULL, NULL);
+			      restriction, NULL);
 	if (IS_ERR(blacklist_keyring))
 		panic("Can't allocate system blacklist keyring\n");
 
-- 
2.32.0

Re: [PATCH v8 0/5] Enable root to update the blacklist keyring

[Mickaël Salaün]

Hi Jarkko,

Since everyone seems OK with this and had plenty of time to complain, 
could you please take this patch series in your tree? It still applies 
on v5.16-rc5 and it is really important to us. Please let me know if you 
need something more.

Regards,
  Mickaël


On 12/07/2021 19:03, Mickaël Salaün wrote:
> Hi,
> 
> This new patch series is a rebase on v5.14-rc1 .  David or Jarkko, if
> it's still OK with you, could you please push this to linux-next?
> 
> I successfully tested this patch series with the 211 entries from
> https://uefi.org/sites/default/files/resources/dbxupdate_x64.bin
> 
> The goal of these patches is to add a new configuration option to enable the
> root user to load signed keys in the blacklist keyring.  This keyring is useful
> to "untrust" certificates or files.  Enabling to safely update this keyring
> without recompiling the kernel makes it more usable.
> 
> Previous patch series:
> https://lore.kernel.org/lkml/20210312171232.2681989-1-mic@digikod.net/
> 
> Regards,
> 
> Mickaël Salaün (5):
>    tools/certs: Add print-cert-tbs-hash.sh
>    certs: Check that builtin blacklist hashes are valid
>    certs: Make blacklist_vet_description() more strict
>    certs: Factor out the blacklist hash creation
>    certs: Allow root user to append signed hashes to the blacklist
>      keyring
> 
>   MAINTAINERS                                   |   2 +
>   certs/.gitignore                              |   1 +
>   certs/Kconfig                                 |  17 +-
>   certs/Makefile                                |  17 +-
>   certs/blacklist.c                             | 218 ++++++++++++++----
>   crypto/asymmetric_keys/x509_public_key.c      |   3 +-
>   include/keys/system_keyring.h                 |  14 +-
>   scripts/check-blacklist-hashes.awk            |  37 +++
>   .../platform_certs/keyring_handler.c          |  26 +--
>   tools/certs/print-cert-tbs-hash.sh            |  91 ++++++++
>   10 files changed, 346 insertions(+), 80 deletions(-)
>   create mode 100755 scripts/check-blacklist-hashes.awk
>   create mode 100755 tools/certs/print-cert-tbs-hash.sh
> 
> 
> base-commit: e73f0f0ee7541171d89f2e2491130c7771ba58d3
> 

Re: [PATCH v8 0/5] Enable root to update the blacklist keyring

[Jarkko Sakkinen]

On Mon, Dec 13, 2021 at 04:30:29PM +0100, Mickaël Salaün wrote:
> Hi Jarkko,
> 
> Since everyone seems OK with this and had plenty of time to complain, could
> you please take this patch series in your tree? It still applies on
> v5.16-rc5 and it is really important to us. Please let me know if you need
> something more.
> 
> Regards,
>  Mickaël

I'm off-work up until end of the year, i.e. I will address only important
bug fixes and v5.16 up until that.

If any of the patches is yet missing my ack, feel free to

Acked-by: Jarkko Sakkinen <jarkko@kernel.org>

/Jarkko

Re: [PATCH v8 0/5] Enable root to update the blacklist keyring

[Mickaël Salaün]

On 21/12/2021 09:50, Jarkko Sakkinen wrote:
> On Mon, Dec 13, 2021 at 04:30:29PM +0100, Mickaël Salaün wrote:
>> Hi Jarkko,
>>
>> Since everyone seems OK with this and had plenty of time to complain, could
>> you please take this patch series in your tree? It still applies on
>> v5.16-rc5 and it is really important to us. Please let me know if you need
>> something more.
>>
>> Regards,
>>   Mickaël
> 
> I'm off-work up until end of the year, i.e. I will address only important
> bug fixes and v5.16 up until that.
> 
> If any of the patches is yet missing my ack, feel free to
> 
> Acked-by: Jarkko Sakkinen <jarkko@kernel.org>

Thanks Jarkko. Can you please take it into your tree?

Regards,
  Mickaël

Re: [PATCH v8 0/5] Enable root to update the blacklist keyring

[Jarkko Sakkinen]

On Tue, Jan 04, 2022 at 04:56:36PM +0100, Mickaël Salaün wrote:
> 
> On 21/12/2021 09:50, Jarkko Sakkinen wrote:
> > On Mon, Dec 13, 2021 at 04:30:29PM +0100, Mickaël Salaün wrote:
> > > Hi Jarkko,
> > > 
> > > Since everyone seems OK with this and had plenty of time to complain, could
> > > you please take this patch series in your tree? It still applies on
> > > v5.16-rc5 and it is really important to us. Please let me know if you need
> > > something more.
> > > 
> > > Regards,
> > >   Mickaël
> > 
> > I'm off-work up until end of the year, i.e. I will address only important
> > bug fixes and v5.16 up until that.
> > 
> > If any of the patches is yet missing my ack, feel free to
> > 
> > Acked-by: Jarkko Sakkinen <jarkko@kernel.org>
> 
> Thanks Jarkko. Can you please take it into your tree?

I can yes, as I need to anyway do a revised PR for v5.17, as one commit
in my first trial had a truncated fixes tag.

/Jarkko

Re: [PATCH v8 0/5] Enable root to update the blacklist keyring

[Jarkko Sakkinen]

On Thu, Jan 06, 2022 at 09:12:22PM +0200, Jarkko Sakkinen wrote:
> On Tue, Jan 04, 2022 at 04:56:36PM +0100, Mickaël Salaün wrote:
> > 
> > On 21/12/2021 09:50, Jarkko Sakkinen wrote:
> > > On Mon, Dec 13, 2021 at 04:30:29PM +0100, Mickaël Salaün wrote:
> > > > Hi Jarkko,
> > > > 
> > > > Since everyone seems OK with this and had plenty of time to complain, could
> > > > you please take this patch series in your tree? It still applies on
> > > > v5.16-rc5 and it is really important to us. Please let me know if you need
> > > > something more.
> > > > 
> > > > Regards,
> > > >   Mickaël
> > > 
> > > I'm off-work up until end of the year, i.e. I will address only important
> > > bug fixes and v5.16 up until that.
> > > 
> > > If any of the patches is yet missing my ack, feel free to
> > > 
> > > Acked-by: Jarkko Sakkinen <jarkko@kernel.org>
> > 
> > Thanks Jarkko. Can you please take it into your tree?
> 
> I can yes, as I need to anyway do a revised PR for v5.17, as one commit
> in my first trial had a truncated fixes tag.

Please check:

git://git.kernel.org/pub/scm/linux/kernel/git/jarkko/linux-tpmdd.git

/Jarkko

Re: [PATCH v8 0/5] Enable root to update the blacklist keyring

[Mickaël Salaün]

On 06/01/2022 20:16, Jarkko Sakkinen wrote:
> On Thu, Jan 06, 2022 at 09:12:22PM +0200, Jarkko Sakkinen wrote:
>> On Tue, Jan 04, 2022 at 04:56:36PM +0100, Mickaël Salaün wrote:
>>>
>>> On 21/12/2021 09:50, Jarkko Sakkinen wrote:
>>>> On Mon, Dec 13, 2021 at 04:30:29PM +0100, Mickaël Salaün wrote:
>>>>> Hi Jarkko,
>>>>>
>>>>> Since everyone seems OK with this and had plenty of time to complain, could
>>>>> you please take this patch series in your tree? It still applies on
>>>>> v5.16-rc5 and it is really important to us. Please let me know if you need
>>>>> something more.
>>>>>
>>>>> Regards,
>>>>>    Mickaël
>>>>
>>>> I'm off-work up until end of the year, i.e. I will address only important
>>>> bug fixes and v5.16 up until that.
>>>>
>>>> If any of the patches is yet missing my ack, feel free to
>>>>
>>>> Acked-by: Jarkko Sakkinen <jarkko@kernel.org>
>>>
>>> Thanks Jarkko. Can you please take it into your tree?
>>
>> I can yes, as I need to anyway do a revised PR for v5.17, as one commit
>> in my first trial had a truncated fixes tag.
> 
> Please check:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/jarkko/linux-tpmdd.git
> 
> /Jarkko

Great, thanks!

Re: [PATCH v8 0/5] Enable root to update the blacklist keyring

[Mickaël Salaün]

On 07/01/2022 13:14, Mickaël Salaün wrote:
> 
> On 06/01/2022 20:16, Jarkko Sakkinen wrote:
>> On Thu, Jan 06, 2022 at 09:12:22PM +0200, Jarkko Sakkinen wrote:
>>> On Tue, Jan 04, 2022 at 04:56:36PM +0100, Mickaël Salaün wrote:
>>>>
>>>> On 21/12/2021 09:50, Jarkko Sakkinen wrote:
>>>>> On Mon, Dec 13, 2021 at 04:30:29PM +0100, Mickaël Salaün wrote:
>>>>>> Hi Jarkko,
>>>>>>
>>>>>> Since everyone seems OK with this and had plenty of time to 
>>>>>> complain, could
>>>>>> you please take this patch series in your tree? It still applies on
>>>>>> v5.16-rc5 and it is really important to us. Please let me know if 
>>>>>> you need
>>>>>> something more.
>>>>>>
>>>>>> Regards,
>>>>>>    Mickaël
>>>>>
>>>>> I'm off-work up until end of the year, i.e. I will address only 
>>>>> important
>>>>> bug fixes and v5.16 up until that.
>>>>>
>>>>> If any of the patches is yet missing my ack, feel free to
>>>>>
>>>>> Acked-by: Jarkko Sakkinen <jarkko@kernel.org>
>>>>
>>>> Thanks Jarkko. Can you please take it into your tree?
>>>
>>> I can yes, as I need to anyway do a revised PR for v5.17, as one commit
>>> in my first trial had a truncated fixes tag.
>>
>> Please check:
>>
>> git://git.kernel.org/pub/scm/linux/kernel/git/jarkko/linux-tpmdd.git
>>
>> /Jarkko
> 
> Great, thanks!

Hi Jarkko,

I noticed your commits 
https://git.kernel.org/pub/scm/linux/kernel/git/jarkko/linux-tpmdd.git/commit/?id=3ec9c3a0531ac868422be3b12fc17310ed8c07dc 
are no more referenced in your tree. Is there an issue?

Regards,
  Mickaël

Re: [PATCH v8 0/5] Enable root to update the blacklist keyring

[Jarkko Sakkinen]

On Mon, Jan 31, 2022 at 12:33:51PM +0100, Mickaël Salaün wrote:
> 
> On 07/01/2022 13:14, Mickaël Salaün wrote:
> > 
> > On 06/01/2022 20:16, Jarkko Sakkinen wrote:
> > > On Thu, Jan 06, 2022 at 09:12:22PM +0200, Jarkko Sakkinen wrote:
> > > > On Tue, Jan 04, 2022 at 04:56:36PM +0100, Mickaël Salaün wrote:
> > > > > 
> > > > > On 21/12/2021 09:50, Jarkko Sakkinen wrote:
> > > > > > On Mon, Dec 13, 2021 at 04:30:29PM +0100, Mickaël Salaün wrote:
> > > > > > > Hi Jarkko,
> > > > > > > 
> > > > > > > Since everyone seems OK with this and had plenty of
> > > > > > > time to complain, could
> > > > > > > you please take this patch series in your tree? It still applies on
> > > > > > > v5.16-rc5 and it is really important to us. Please
> > > > > > > let me know if you need
> > > > > > > something more.
> > > > > > > 
> > > > > > > Regards,
> > > > > > >    Mickaël
> > > > > > 
> > > > > > I'm off-work up until end of the year, i.e. I will
> > > > > > address only important
> > > > > > bug fixes and v5.16 up until that.
> > > > > > 
> > > > > > If any of the patches is yet missing my ack, feel free to
> > > > > > 
> > > > > > Acked-by: Jarkko Sakkinen <jarkko@kernel.org>
> > > > > 
> > > > > Thanks Jarkko. Can you please take it into your tree?
> > > > 
> > > > I can yes, as I need to anyway do a revised PR for v5.17, as one commit
> > > > in my first trial had a truncated fixes tag.
> > > 
> > > Please check:
> > > 
> > > git://git.kernel.org/pub/scm/linux/kernel/git/jarkko/linux-tpmdd.git
> > > 
> > > /Jarkko
> > 
> > Great, thanks!
> 
> Hi Jarkko,
> 
> I noticed your commits https://git.kernel.org/pub/scm/linux/kernel/git/jarkko/linux-tpmdd.git/commit/?id=3ec9c3a0531ac868422be3b12fc17310ed8c07dc
> are no more referenced in your tree. Is there an issue?

This must be some sort of mistake I've made. I'll re-apply the patches.
Sorry about this.

BR, Jarkko

Re: [PATCH v8 0/5] Enable root to update the blacklist keyring

[Jarkko Sakkinen]

On Thu, Feb 17, 2022 at 08:58:57PM +0100, Jarkko Sakkinen wrote:
> On Mon, Jan 31, 2022 at 12:33:51PM +0100, Mickaël Salaün wrote:
> > 
> > On 07/01/2022 13:14, Mickaël Salaün wrote:
> > > 
> > > On 06/01/2022 20:16, Jarkko Sakkinen wrote:
> > > > On Thu, Jan 06, 2022 at 09:12:22PM +0200, Jarkko Sakkinen wrote:
> > > > > On Tue, Jan 04, 2022 at 04:56:36PM +0100, Mickaël Salaün wrote:
> > > > > > 
> > > > > > On 21/12/2021 09:50, Jarkko Sakkinen wrote:
> > > > > > > On Mon, Dec 13, 2021 at 04:30:29PM +0100, Mickaël Salaün wrote:
> > > > > > > > Hi Jarkko,
> > > > > > > > 
> > > > > > > > Since everyone seems OK with this and had plenty of
> > > > > > > > time to complain, could
> > > > > > > > you please take this patch series in your tree? It still applies on
> > > > > > > > v5.16-rc5 and it is really important to us. Please
> > > > > > > > let me know if you need
> > > > > > > > something more.
> > > > > > > > 
> > > > > > > > Regards,
> > > > > > > >    Mickaël
> > > > > > > 
> > > > > > > I'm off-work up until end of the year, i.e. I will
> > > > > > > address only important
> > > > > > > bug fixes and v5.16 up until that.
> > > > > > > 
> > > > > > > If any of the patches is yet missing my ack, feel free to
> > > > > > > 
> > > > > > > Acked-by: Jarkko Sakkinen <jarkko@kernel.org>
> > > > > > 
> > > > > > Thanks Jarkko. Can you please take it into your tree?
> > > > > 
> > > > > I can yes, as I need to anyway do a revised PR for v5.17, as one commit
> > > > > in my first trial had a truncated fixes tag.
> > > > 
> > > > Please check:
> > > > 
> > > > git://git.kernel.org/pub/scm/linux/kernel/git/jarkko/linux-tpmdd.git
> > > > 
> > > > /Jarkko
> > > 
> > > Great, thanks!
> > 
> > Hi Jarkko,
> > 
> > I noticed your commits https://git.kernel.org/pub/scm/linux/kernel/git/jarkko/linux-tpmdd.git/commit/?id=3ec9c3a0531ac868422be3b12fc17310ed8c07dc
> > are no more referenced in your tree. Is there an issue?
> 
> This must be some sort of mistake I've made. I'll re-apply the patches.
> Sorry about this.

OK now the patches are in and will be included to the next PR. I fixed
merge conflicts caused by 5cca36069d4c ("certs: refactor file cleaning")
in "certs: Check that builtin blacklist hashes are valid" so please
sanity check that it is good.

BR, Jarkko

Re: [PATCH v8 5/5] certs: Allow root user to append signed hashes to the blacklist keyring

[Jarkko Sakkinen]

On Mon, Jul 12, 2021 at 07:03:13PM +0200, Mickaël Salaün wrote:
> From: Mickaël Salaün <mic@linux.microsoft.com>
> 
> Add a kernel option SYSTEM_BLACKLIST_AUTH_UPDATE to enable the root user
> to dynamically add new keys to the blacklist keyring.  This enables to
> invalidate new certificates, either from being loaded in a keyring, or
> from being trusted in a PKCS#7 certificate chain.  This also enables to
> add new file hashes to be denied by the integrity infrastructure.
> 
> Being able to untrust a certificate which could have normaly been
> trusted is a sensitive operation.  This is why adding new hashes to the
> blacklist keyring is only allowed when these hashes are signed and
> vouched by the builtin trusted keyring.  A blacklist hash is stored as a
> key description.  The PKCS#7 signature of this description must be
> provided as the key payload.
> 
> Marking a certificate as untrusted should be enforced while the system
> is running.  It is then forbiden to remove such blacklist keys.
> 
> Update blacklist keyring, blacklist key and revoked certificate access rights:
> * allows the root user to search for a specific blacklisted hash, which
>   make sense because the descriptions are already viewable;
> * forbids key update (blacklist and asymmetric ones);
> * restricts kernel rights on the blacklist keyring to align with the
>   root user rights.
> 
> See help in tools/certs/print-cert-tbs-hash.sh .
> 
> Cc: David Howells <dhowells@redhat.com>
> Cc: David Woodhouse <dwmw2@infradead.org>
> Cc: Eric Snowberg <eric.snowberg@oracle.com>
> Cc: Jarkko Sakkinen <jarkko@kernel.org>
> Signed-off-by: Mickaël Salaün <mic@linux.microsoft.com>
> Link: https://lore.kernel.org/r/20210712170313.884724-6-mic@digikod.net
> ---
> 
> Changes since v6:
> * Rebase on keys-cve-2020-26541-v3: commit ebd9c2ae369a ("integrity:
>   Load mokx variables into the blacklist keyring").
> 
> Changes since v5:
> * Rebase on keys-next, fix Kconfig conflict, and update the asymmetric
>   key rights added to the blacklist keyring by the new
>   add_key_to_revocation_list(): align with blacklist key rights by
>   removing KEY_POS_WRITE as a safeguard, and add
>   KEY_ALLOC_BYPASS_RESTRICTION to not be subject to
>   restrict_link_for_blacklist() that only allows blacklist key types to
>   be added to the keyring.
> * Change the return code for restrict_link_for_blacklist() from -EPERM
>   to -EOPNOTSUPP to align with asymmetric key keyrings.
> 
> Changes since v3:
> * Update commit message for print-cert-tbs-hash.sh .
> 
> Changes since v2:
> * Add comment for blacklist_key_instantiate().
> ---
>  certs/Kconfig     | 10 +++++
>  certs/blacklist.c | 96 ++++++++++++++++++++++++++++++++++++-----------
>  2 files changed, 85 insertions(+), 21 deletions(-)
> 
> diff --git a/certs/Kconfig b/certs/Kconfig
> index 0fbe184ceca5..e0e524b7eff9 100644
> --- a/certs/Kconfig
> +++ b/certs/Kconfig
> @@ -103,4 +103,14 @@ config SYSTEM_REVOCATION_KEYS
>  	  containing X.509 certificates to be included in the default blacklist
>  	  keyring.
>  
> +config SYSTEM_BLACKLIST_AUTH_UPDATE
> +	bool "Allow root to add signed blacklist keys"
> +	depends on SYSTEM_BLACKLIST_KEYRING
> +	depends on SYSTEM_DATA_VERIFICATION
> +	help
> +	  If set, provide the ability to load new blacklist keys at run time if
> +	  they are signed and vouched by a certificate from the builtin trusted
> +	  keyring.  The PKCS#7 signature of the description is set in the key
> +	  payload.  Blacklist keys cannot be removed.
> +
>  endmenu
> diff --git a/certs/blacklist.c b/certs/blacklist.c
> index b254c87ceb3a..486ce0dd8e9c 100644
> --- a/certs/blacklist.c
> +++ b/certs/blacklist.c
> @@ -15,6 +15,7 @@
>  #include <linux/err.h>
>  #include <linux/seq_file.h>
>  #include <linux/uidgid.h>
> +#include <linux/verification.h>
>  #include <keys/system_keyring.h>
>  #include "blacklist.h"
>  #include "common.h"
> @@ -26,6 +27,9 @@
>   */
>  #define MAX_HASH_LEN	128
>  
> +#define BLACKLIST_KEY_PERM (KEY_POS_SEARCH | KEY_POS_VIEW | \
> +			    KEY_USR_SEARCH | KEY_USR_VIEW)
> +
>  static const char tbs_prefix[] = "tbs";
>  static const char bin_prefix[] = "bin";
>  
> @@ -80,19 +84,51 @@ static int blacklist_vet_description(const char *desc)
>  	return 0;
>  }
>  
> -/*
> - * The hash to be blacklisted is expected to be in the description.  There will
> - * be no payload.
> - */
> -static int blacklist_preparse(struct key_preparsed_payload *prep)
> +static int blacklist_key_instantiate(struct key *key,
> +		struct key_preparsed_payload *prep)
>  {
> -	if (prep->datalen > 0)
> -		return -EINVAL;
> -	return 0;
> +#ifdef CONFIG_SYSTEM_BLACKLIST_AUTH_UPDATE
> +	int err;
> +#endif
> +
> +	/* Sets safe default permissions for keys loaded by user space. */
> +	key->perm = BLACKLIST_KEY_PERM;
> +
> +	/*
> +	 * Skips the authentication step for builtin hashes, they are not
> +	 * signed but still trusted.
> +	 */
> +	if (key->flags & (1 << KEY_FLAG_BUILTIN))
> +		goto out;
> +
> +#ifdef CONFIG_SYSTEM_BLACKLIST_AUTH_UPDATE
> +	/*
> +	 * Verifies the description's PKCS#7 signature against the builtin
> +	 * trusted keyring.
> +	 */
> +	err = verify_pkcs7_signature(key->description,
> +			strlen(key->description), prep->data, prep->datalen,
> +			NULL, VERIFYING_UNSPECIFIED_SIGNATURE, NULL, NULL);
> +	if (err)
> +		return err;
> +#else
> +	/*
> +	 * It should not be possible to come here because the keyring doesn't
> +	 * have KEY_USR_WRITE and the only other way to call this function is
> +	 * for builtin hashes.
> +	 */
> +	WARN_ON_ONCE(1);
> +	return -EPERM;
> +#endif
> +
> +out:
> +	return generic_key_instantiate(key, prep);
>  }
>  
> -static void blacklist_free_preparse(struct key_preparsed_payload *prep)
> +static int blacklist_key_update(struct key *key,
> +		struct key_preparsed_payload *prep)
>  {
> +	return -EPERM;
>  }
>  
>  static void blacklist_describe(const struct key *key, struct seq_file *m)
> @@ -103,9 +139,8 @@ static void blacklist_describe(const struct key *key, struct seq_file *m)
>  static struct key_type key_type_blacklist = {
>  	.name			= "blacklist",
>  	.vet_description	= blacklist_vet_description,
> -	.preparse		= blacklist_preparse,
> -	.free_preparse		= blacklist_free_preparse,
> -	.instantiate		= generic_key_instantiate,
> +	.instantiate		= blacklist_key_instantiate,
> +	.update			= blacklist_key_update,
>  	.describe		= blacklist_describe,
>  };
>  
> @@ -154,8 +189,7 @@ static int mark_raw_hash_blacklisted(const char *hash)
>  				   hash,
>  				   NULL,
>  				   0,
> -				   ((KEY_POS_ALL & ~KEY_POS_SETATTR) |
> -				    KEY_USR_VIEW),
> +				   BLACKLIST_KEY_PERM,
>  				   KEY_ALLOC_NOT_IN_QUOTA |
>  				   KEY_ALLOC_BUILT_IN);
>  	if (IS_ERR(key)) {
> @@ -232,8 +266,10 @@ int add_key_to_revocation_list(const char *data, size_t size)
>  				   NULL,
>  				   data,
>  				   size,
> -				   ((KEY_POS_ALL & ~KEY_POS_SETATTR) | KEY_USR_VIEW),
> -				   KEY_ALLOC_NOT_IN_QUOTA | KEY_ALLOC_BUILT_IN);
> +				   KEY_POS_VIEW | KEY_POS_READ | KEY_POS_SEARCH
> +				   | KEY_USR_VIEW,
> +				   KEY_ALLOC_NOT_IN_QUOTA | KEY_ALLOC_BUILT_IN
> +				   | KEY_ALLOC_BYPASS_RESTRICTION);
>  
>  	if (IS_ERR(key)) {
>  		pr_err("Problem with revocation key (%ld)\n", PTR_ERR(key));
> @@ -260,25 +296,43 @@ int is_key_on_revocation_list(struct pkcs7_message *pkcs7)
>  }
>  #endif
>  
> +static int restrict_link_for_blacklist(struct key *dest_keyring,
> +		const struct key_type *type, const union key_payload *payload,
> +		struct key *restrict_key)
> +{
> +	if (type == &key_type_blacklist)
> +		return 0;
> +	return -EOPNOTSUPP;
> +}
> +
>  /*
>   * Initialise the blacklist
>   */
>  static int __init blacklist_init(void)
>  {
>  	const char *const *bl;
> +	struct key_restriction *restriction;
>  
>  	if (register_key_type(&key_type_blacklist) < 0)
>  		panic("Can't allocate system blacklist key type\n");
>  
> +	restriction = kzalloc(sizeof(*restriction), GFP_KERNEL);
> +	if (!restriction)
> +		panic("Can't allocate blacklist keyring restriction\n");


This prevents me from taking this to my pull request. In moderns standards,
no new BUG_ON(), panic() etc. should never added to the kernel.

I missed this in my review.

This should rather be e.g.

        restriction = kzalloc(sizeof(*restriction), GFP_KERNEL);
	if (!restriction) {
		pr_err("Can't allocate blacklist keyring restriction\n");
                return 0;
        }

Unfortunately I need to drop this patch set, because adding new panic()
is simply a no-go.

BR, Jarkko

Re: [PATCH v8 5/5] certs: Allow root user to append signed hashes to the blacklist keyring

[Mickaël Salaün]

On 08/03/2022 12:53, Jarkko Sakkinen wrote:
> On Mon, Jul 12, 2021 at 07:03:13PM +0200, Mickaël Salaün wrote:
>> From: Mickaël Salaün <mic@linux.microsoft.com>
>>
>> Add a kernel option SYSTEM_BLACKLIST_AUTH_UPDATE to enable the root user
>> to dynamically add new keys to the blacklist keyring.  This enables to
>> invalidate new certificates, either from being loaded in a keyring, or
>> from being trusted in a PKCS#7 certificate chain.  This also enables to
>> add new file hashes to be denied by the integrity infrastructure.
>>
>> Being able to untrust a certificate which could have normaly been
>> trusted is a sensitive operation.  This is why adding new hashes to the
>> blacklist keyring is only allowed when these hashes are signed and
>> vouched by the builtin trusted keyring.  A blacklist hash is stored as a
>> key description.  The PKCS#7 signature of this description must be
>> provided as the key payload.
>>
>> Marking a certificate as untrusted should be enforced while the system
>> is running.  It is then forbiden to remove such blacklist keys.
>>
>> Update blacklist keyring, blacklist key and revoked certificate access rights:
>> * allows the root user to search for a specific blacklisted hash, which
>>    make sense because the descriptions are already viewable;
>> * forbids key update (blacklist and asymmetric ones);
>> * restricts kernel rights on the blacklist keyring to align with the
>>    root user rights.
>>
>> See help in tools/certs/print-cert-tbs-hash.sh .
>>
>> Cc: David Howells <dhowells@redhat.com>
>> Cc: David Woodhouse <dwmw2@infradead.org>
>> Cc: Eric Snowberg <eric.snowberg@oracle.com>
>> Cc: Jarkko Sakkinen <jarkko@kernel.org>
>> Signed-off-by: Mickaël Salaün <mic@linux.microsoft.com>
>> Link: https://lore.kernel.org/r/20210712170313.884724-6-mic@digikod.net
>> ---
>>
>> Changes since v6:
>> * Rebase on keys-cve-2020-26541-v3: commit ebd9c2ae369a ("integrity:
>>    Load mokx variables into the blacklist keyring").
>>
>> Changes since v5:
>> * Rebase on keys-next, fix Kconfig conflict, and update the asymmetric
>>    key rights added to the blacklist keyring by the new
>>    add_key_to_revocation_list(): align with blacklist key rights by
>>    removing KEY_POS_WRITE as a safeguard, and add
>>    KEY_ALLOC_BYPASS_RESTRICTION to not be subject to
>>    restrict_link_for_blacklist() that only allows blacklist key types to
>>    be added to the keyring.
>> * Change the return code for restrict_link_for_blacklist() from -EPERM
>>    to -EOPNOTSUPP to align with asymmetric key keyrings.
>>
>> Changes since v3:
>> * Update commit message for print-cert-tbs-hash.sh .
>>
>> Changes since v2:
>> * Add comment for blacklist_key_instantiate().
>> ---
>>   certs/Kconfig     | 10 +++++
>>   certs/blacklist.c | 96 ++++++++++++++++++++++++++++++++++++-----------
>>   2 files changed, 85 insertions(+), 21 deletions(-)
>>
>> diff --git a/certs/Kconfig b/certs/Kconfig
>> index 0fbe184ceca5..e0e524b7eff9 100644
>> --- a/certs/Kconfig
>> +++ b/certs/Kconfig
>> @@ -103,4 +103,14 @@ config SYSTEM_REVOCATION_KEYS
>>   	  containing X.509 certificates to be included in the default blacklist
>>   	  keyring.
>>   
>> +config SYSTEM_BLACKLIST_AUTH_UPDATE
>> +	bool "Allow root to add signed blacklist keys"
>> +	depends on SYSTEM_BLACKLIST_KEYRING
>> +	depends on SYSTEM_DATA_VERIFICATION
>> +	help
>> +	  If set, provide the ability to load new blacklist keys at run time if
>> +	  they are signed and vouched by a certificate from the builtin trusted
>> +	  keyring.  The PKCS#7 signature of the description is set in the key
>> +	  payload.  Blacklist keys cannot be removed.
>> +
>>   endmenu
>> diff --git a/certs/blacklist.c b/certs/blacklist.c
>> index b254c87ceb3a..486ce0dd8e9c 100644
>> --- a/certs/blacklist.c
>> +++ b/certs/blacklist.c
>> @@ -15,6 +15,7 @@
>>   #include <linux/err.h>
>>   #include <linux/seq_file.h>
>>   #include <linux/uidgid.h>
>> +#include <linux/verification.h>
>>   #include <keys/system_keyring.h>
>>   #include "blacklist.h"
>>   #include "common.h"
>> @@ -26,6 +27,9 @@
>>    */
>>   #define MAX_HASH_LEN	128
>>   
>> +#define BLACKLIST_KEY_PERM (KEY_POS_SEARCH | KEY_POS_VIEW | \
>> +			    KEY_USR_SEARCH | KEY_USR_VIEW)
>> +
>>   static const char tbs_prefix[] = "tbs";
>>   static const char bin_prefix[] = "bin";
>>   
>> @@ -80,19 +84,51 @@ static int blacklist_vet_description(const char *desc)
>>   	return 0;
>>   }
>>   
>> -/*
>> - * The hash to be blacklisted is expected to be in the description.  There will
>> - * be no payload.
>> - */
>> -static int blacklist_preparse(struct key_preparsed_payload *prep)
>> +static int blacklist_key_instantiate(struct key *key,
>> +		struct key_preparsed_payload *prep)
>>   {
>> -	if (prep->datalen > 0)
>> -		return -EINVAL;
>> -	return 0;
>> +#ifdef CONFIG_SYSTEM_BLACKLIST_AUTH_UPDATE
>> +	int err;
>> +#endif
>> +
>> +	/* Sets safe default permissions for keys loaded by user space. */
>> +	key->perm = BLACKLIST_KEY_PERM;
>> +
>> +	/*
>> +	 * Skips the authentication step for builtin hashes, they are not
>> +	 * signed but still trusted.
>> +	 */
>> +	if (key->flags & (1 << KEY_FLAG_BUILTIN))
>> +		goto out;
>> +
>> +#ifdef CONFIG_SYSTEM_BLACKLIST_AUTH_UPDATE
>> +	/*
>> +	 * Verifies the description's PKCS#7 signature against the builtin
>> +	 * trusted keyring.
>> +	 */
>> +	err = verify_pkcs7_signature(key->description,
>> +			strlen(key->description), prep->data, prep->datalen,
>> +			NULL, VERIFYING_UNSPECIFIED_SIGNATURE, NULL, NULL);
>> +	if (err)
>> +		return err;
>> +#else
>> +	/*
>> +	 * It should not be possible to come here because the keyring doesn't
>> +	 * have KEY_USR_WRITE and the only other way to call this function is
>> +	 * for builtin hashes.
>> +	 */
>> +	WARN_ON_ONCE(1);
>> +	return -EPERM;
>> +#endif
>> +
>> +out:
>> +	return generic_key_instantiate(key, prep);
>>   }
>>   
>> -static void blacklist_free_preparse(struct key_preparsed_payload *prep)
>> +static int blacklist_key_update(struct key *key,
>> +		struct key_preparsed_payload *prep)
>>   {
>> +	return -EPERM;
>>   }
>>   
>>   static void blacklist_describe(const struct key *key, struct seq_file *m)
>> @@ -103,9 +139,8 @@ static void blacklist_describe(const struct key *key, struct seq_file *m)
>>   static struct key_type key_type_blacklist = {
>>   	.name			= "blacklist",
>>   	.vet_description	= blacklist_vet_description,
>> -	.preparse		= blacklist_preparse,
>> -	.free_preparse		= blacklist_free_preparse,
>> -	.instantiate		= generic_key_instantiate,
>> +	.instantiate		= blacklist_key_instantiate,
>> +	.update			= blacklist_key_update,
>>   	.describe		= blacklist_describe,
>>   };
>>   
>> @@ -154,8 +189,7 @@ static int mark_raw_hash_blacklisted(const char *hash)
>>   				   hash,
>>   				   NULL,
>>   				   0,
>> -				   ((KEY_POS_ALL & ~KEY_POS_SETATTR) |
>> -				    KEY_USR_VIEW),
>> +				   BLACKLIST_KEY_PERM,
>>   				   KEY_ALLOC_NOT_IN_QUOTA |
>>   				   KEY_ALLOC_BUILT_IN);
>>   	if (IS_ERR(key)) {
>> @@ -232,8 +266,10 @@ int add_key_to_revocation_list(const char *data, size_t size)
>>   				   NULL,
>>   				   data,
>>   				   size,
>> -				   ((KEY_POS_ALL & ~KEY_POS_SETATTR) | KEY_USR_VIEW),
>> -				   KEY_ALLOC_NOT_IN_QUOTA | KEY_ALLOC_BUILT_IN);
>> +				   KEY_POS_VIEW | KEY_POS_READ | KEY_POS_SEARCH
>> +				   | KEY_USR_VIEW,
>> +				   KEY_ALLOC_NOT_IN_QUOTA | KEY_ALLOC_BUILT_IN
>> +				   | KEY_ALLOC_BYPASS_RESTRICTION);
>>   
>>   	if (IS_ERR(key)) {
>>   		pr_err("Problem with revocation key (%ld)\n", PTR_ERR(key));
>> @@ -260,25 +296,43 @@ int is_key_on_revocation_list(struct pkcs7_message *pkcs7)
>>   }
>>   #endif
>>   
>> +static int restrict_link_for_blacklist(struct key *dest_keyring,
>> +		const struct key_type *type, const union key_payload *payload,
>> +		struct key *restrict_key)
>> +{
>> +	if (type == &key_type_blacklist)
>> +		return 0;
>> +	return -EOPNOTSUPP;
>> +}
>> +
>>   /*
>>    * Initialise the blacklist
>>    */
>>   static int __init blacklist_init(void)
>>   {
>>   	const char *const *bl;
>> +	struct key_restriction *restriction;
>>   
>>   	if (register_key_type(&key_type_blacklist) < 0)
>>   		panic("Can't allocate system blacklist key type\n");
>>   
>> +	restriction = kzalloc(sizeof(*restriction), GFP_KERNEL);
>> +	if (!restriction)
>> +		panic("Can't allocate blacklist keyring restriction\n");
> 
> 
> This prevents me from taking this to my pull request. In moderns standards,
> no new BUG_ON(), panic() etc. should never added to the kernel.
> 
> I missed this in my review.
> 
> This should rather be e.g.
> 
>          restriction = kzalloc(sizeof(*restriction), GFP_KERNEL);
> 	if (!restriction) {
> 		pr_err("Can't allocate blacklist keyring restriction\n");
>                  return 0;
>          }
> 
> Unfortunately I need to drop this patch set, because adding new panic()
> is simply a no-go.

I agree that panic() is not great in general, but I followed the other 
part of the code (just above) that do the same. This part of the kernel 
should failed if critical memory allocation failed at boot time (only). 
It doesn't impact the kernel once it is running. I don't think that just 
ignoring this error with return 0 is fine, after all it's a critical 
error right?

Calling panic() seems OK here. Is there a better way to stop the kernel 
for such critical error? If the kernel cannot allocate memory at this 
time, it would be useless to try continuing booting.

Re: [PATCH v8 5/5] certs: Allow root user to append signed hashes to the blacklist keyring

[Jarkko Sakkinen]

On Tue, Mar 08, 2022 at 01:18:28PM +0100, Mickaël Salaün wrote:
> 
> On 08/03/2022 12:53, Jarkko Sakkinen wrote:
> > On Mon, Jul 12, 2021 at 07:03:13PM +0200, Mickaël Salaün wrote:
> > > From: Mickaël Salaün <mic@linux.microsoft.com>
> > > 
> > > Add a kernel option SYSTEM_BLACKLIST_AUTH_UPDATE to enable the root user
> > > to dynamically add new keys to the blacklist keyring.  This enables to
> > > invalidate new certificates, either from being loaded in a keyring, or
> > > from being trusted in a PKCS#7 certificate chain.  This also enables to
> > > add new file hashes to be denied by the integrity infrastructure.
> > > 
> > > Being able to untrust a certificate which could have normaly been
> > > trusted is a sensitive operation.  This is why adding new hashes to the
> > > blacklist keyring is only allowed when these hashes are signed and
> > > vouched by the builtin trusted keyring.  A blacklist hash is stored as a
> > > key description.  The PKCS#7 signature of this description must be
> > > provided as the key payload.
> > > 
> > > Marking a certificate as untrusted should be enforced while the system
> > > is running.  It is then forbiden to remove such blacklist keys.
> > > 
> > > Update blacklist keyring, blacklist key and revoked certificate access rights:
> > > * allows the root user to search for a specific blacklisted hash, which
> > >    make sense because the descriptions are already viewable;
> > > * forbids key update (blacklist and asymmetric ones);
> > > * restricts kernel rights on the blacklist keyring to align with the
> > >    root user rights.
> > > 
> > > See help in tools/certs/print-cert-tbs-hash.sh .
> > > 
> > > Cc: David Howells <dhowells@redhat.com>
> > > Cc: David Woodhouse <dwmw2@infradead.org>
> > > Cc: Eric Snowberg <eric.snowberg@oracle.com>
> > > Cc: Jarkko Sakkinen <jarkko@kernel.org>
> > > Signed-off-by: Mickaël Salaün <mic@linux.microsoft.com>
> > > Link: https://lore.kernel.org/r/20210712170313.884724-6-mic@digikod.net
> > > ---
> > > 
> > > Changes since v6:
> > > * Rebase on keys-cve-2020-26541-v3: commit ebd9c2ae369a ("integrity:
> > >    Load mokx variables into the blacklist keyring").
> > > 
> > > Changes since v5:
> > > * Rebase on keys-next, fix Kconfig conflict, and update the asymmetric
> > >    key rights added to the blacklist keyring by the new
> > >    add_key_to_revocation_list(): align with blacklist key rights by
> > >    removing KEY_POS_WRITE as a safeguard, and add
> > >    KEY_ALLOC_BYPASS_RESTRICTION to not be subject to
> > >    restrict_link_for_blacklist() that only allows blacklist key types to
> > >    be added to the keyring.
> > > * Change the return code for restrict_link_for_blacklist() from -EPERM
> > >    to -EOPNOTSUPP to align with asymmetric key keyrings.
> > > 
> > > Changes since v3:
> > > * Update commit message for print-cert-tbs-hash.sh .
> > > 
> > > Changes since v2:
> > > * Add comment for blacklist_key_instantiate().
> > > ---
> > >   certs/Kconfig     | 10 +++++
> > >   certs/blacklist.c | 96 ++++++++++++++++++++++++++++++++++++-----------
> > >   2 files changed, 85 insertions(+), 21 deletions(-)
> > > 
> > > diff --git a/certs/Kconfig b/certs/Kconfig
> > > index 0fbe184ceca5..e0e524b7eff9 100644
> > > --- a/certs/Kconfig
> > > +++ b/certs/Kconfig
> > > @@ -103,4 +103,14 @@ config SYSTEM_REVOCATION_KEYS
> > >   	  containing X.509 certificates to be included in the default blacklist
> > >   	  keyring.
> > > +config SYSTEM_BLACKLIST_AUTH_UPDATE
> > > +	bool "Allow root to add signed blacklist keys"
> > > +	depends on SYSTEM_BLACKLIST_KEYRING
> > > +	depends on SYSTEM_DATA_VERIFICATION
> > > +	help
> > > +	  If set, provide the ability to load new blacklist keys at run time if
> > > +	  they are signed and vouched by a certificate from the builtin trusted
> > > +	  keyring.  The PKCS#7 signature of the description is set in the key
> > > +	  payload.  Blacklist keys cannot be removed.
> > > +
> > >   endmenu
> > > diff --git a/certs/blacklist.c b/certs/blacklist.c
> > > index b254c87ceb3a..486ce0dd8e9c 100644
> > > --- a/certs/blacklist.c
> > > +++ b/certs/blacklist.c
> > > @@ -15,6 +15,7 @@
> > >   #include <linux/err.h>
> > >   #include <linux/seq_file.h>
> > >   #include <linux/uidgid.h>
> > > +#include <linux/verification.h>
> > >   #include <keys/system_keyring.h>
> > >   #include "blacklist.h"
> > >   #include "common.h"
> > > @@ -26,6 +27,9 @@
> > >    */
> > >   #define MAX_HASH_LEN	128
> > > +#define BLACKLIST_KEY_PERM (KEY_POS_SEARCH | KEY_POS_VIEW | \
> > > +			    KEY_USR_SEARCH | KEY_USR_VIEW)
> > > +
> > >   static const char tbs_prefix[] = "tbs";
> > >   static const char bin_prefix[] = "bin";
> > > @@ -80,19 +84,51 @@ static int blacklist_vet_description(const char *desc)
> > >   	return 0;
> > >   }
> > > -/*
> > > - * The hash to be blacklisted is expected to be in the description.  There will
> > > - * be no payload.
> > > - */
> > > -static int blacklist_preparse(struct key_preparsed_payload *prep)
> > > +static int blacklist_key_instantiate(struct key *key,
> > > +		struct key_preparsed_payload *prep)
> > >   {
> > > -	if (prep->datalen > 0)
> > > -		return -EINVAL;
> > > -	return 0;
> > > +#ifdef CONFIG_SYSTEM_BLACKLIST_AUTH_UPDATE
> > > +	int err;
> > > +#endif
> > > +
> > > +	/* Sets safe default permissions for keys loaded by user space. */
> > > +	key->perm = BLACKLIST_KEY_PERM;
> > > +
> > > +	/*
> > > +	 * Skips the authentication step for builtin hashes, they are not
> > > +	 * signed but still trusted.
> > > +	 */
> > > +	if (key->flags & (1 << KEY_FLAG_BUILTIN))
> > > +		goto out;
> > > +
> > > +#ifdef CONFIG_SYSTEM_BLACKLIST_AUTH_UPDATE
> > > +	/*
> > > +	 * Verifies the description's PKCS#7 signature against the builtin
> > > +	 * trusted keyring.
> > > +	 */
> > > +	err = verify_pkcs7_signature(key->description,
> > > +			strlen(key->description), prep->data, prep->datalen,
> > > +			NULL, VERIFYING_UNSPECIFIED_SIGNATURE, NULL, NULL);
> > > +	if (err)
> > > +		return err;
> > > +#else
> > > +	/*
> > > +	 * It should not be possible to come here because the keyring doesn't
> > > +	 * have KEY_USR_WRITE and the only other way to call this function is
> > > +	 * for builtin hashes.
> > > +	 */
> > > +	WARN_ON_ONCE(1);
> > > +	return -EPERM;
> > > +#endif
> > > +
> > > +out:
> > > +	return generic_key_instantiate(key, prep);
> > >   }
> > > -static void blacklist_free_preparse(struct key_preparsed_payload *prep)
> > > +static int blacklist_key_update(struct key *key,
> > > +		struct key_preparsed_payload *prep)
> > >   {
> > > +	return -EPERM;
> > >   }
> > >   static void blacklist_describe(const struct key *key, struct seq_file *m)
> > > @@ -103,9 +139,8 @@ static void blacklist_describe(const struct key *key, struct seq_file *m)
> > >   static struct key_type key_type_blacklist = {
> > >   	.name			= "blacklist",
> > >   	.vet_description	= blacklist_vet_description,
> > > -	.preparse		= blacklist_preparse,
> > > -	.free_preparse		= blacklist_free_preparse,
> > > -	.instantiate		= generic_key_instantiate,
> > > +	.instantiate		= blacklist_key_instantiate,
> > > +	.update			= blacklist_key_update,
> > >   	.describe		= blacklist_describe,
> > >   };
> > > @@ -154,8 +189,7 @@ static int mark_raw_hash_blacklisted(const char *hash)
> > >   				   hash,
> > >   				   NULL,
> > >   				   0,
> > > -				   ((KEY_POS_ALL & ~KEY_POS_SETATTR) |
> > > -				    KEY_USR_VIEW),
> > > +				   BLACKLIST_KEY_PERM,
> > >   				   KEY_ALLOC_NOT_IN_QUOTA |
> > >   				   KEY_ALLOC_BUILT_IN);
> > >   	if (IS_ERR(key)) {
> > > @@ -232,8 +266,10 @@ int add_key_to_revocation_list(const char *data, size_t size)
> > >   				   NULL,
> > >   				   data,
> > >   				   size,
> > > -				   ((KEY_POS_ALL & ~KEY_POS_SETATTR) | KEY_USR_VIEW),
> > > -				   KEY_ALLOC_NOT_IN_QUOTA | KEY_ALLOC_BUILT_IN);
> > > +				   KEY_POS_VIEW | KEY_POS_READ | KEY_POS_SEARCH
> > > +				   | KEY_USR_VIEW,
> > > +				   KEY_ALLOC_NOT_IN_QUOTA | KEY_ALLOC_BUILT_IN
> > > +				   | KEY_ALLOC_BYPASS_RESTRICTION);
> > >   	if (IS_ERR(key)) {
> > >   		pr_err("Problem with revocation key (%ld)\n", PTR_ERR(key));
> > > @@ -260,25 +296,43 @@ int is_key_on_revocation_list(struct pkcs7_message *pkcs7)
> > >   }
> > >   #endif
> > > +static int restrict_link_for_blacklist(struct key *dest_keyring,
> > > +		const struct key_type *type, const union key_payload *payload,
> > > +		struct key *restrict_key)
> > > +{
> > > +	if (type == &key_type_blacklist)
> > > +		return 0;
> > > +	return -EOPNOTSUPP;
> > > +}
> > > +
> > >   /*
> > >    * Initialise the blacklist
> > >    */
> > >   static int __init blacklist_init(void)
> > >   {
> > >   	const char *const *bl;
> > > +	struct key_restriction *restriction;
> > >   	if (register_key_type(&key_type_blacklist) < 0)
> > >   		panic("Can't allocate system blacklist key type\n");
> > > +	restriction = kzalloc(sizeof(*restriction), GFP_KERNEL);
> > > +	if (!restriction)
> > > +		panic("Can't allocate blacklist keyring restriction\n");
> > 
> > 
> > This prevents me from taking this to my pull request. In moderns standards,
> > no new BUG_ON(), panic() etc. should never added to the kernel.
> > 
> > I missed this in my review.
> > 
> > This should rather be e.g.
> > 
> >          restriction = kzalloc(sizeof(*restriction), GFP_KERNEL);
> > 	if (!restriction) {
> > 		pr_err("Can't allocate blacklist keyring restriction\n");
> >                  return 0;
> >          }
> > 
> > Unfortunately I need to drop this patch set, because adding new panic()
> > is simply a no-go.
> 
> I agree that panic() is not great in general, but I followed the other part
> of the code (just above) that do the same. This part of the kernel should
> failed if critical memory allocation failed at boot time (only). It doesn't
> impact the kernel once it is running. I don't think that just ignoring this
> error with return 0 is fine, after all it's a critical error right?

It's not good reason enough to crash the whole kernel, even if it is a
critical error (e.g. run-time foresincs). Even WARN() is not recommended
these days [*].

For the existing panic()-statements: I'm happy to review patches that
render them out.

Not sure tho, if this fails should it be then "everything blacklisted".
Just one thing to consider.

> Calling panic() seems OK here. Is there a better way to stop the kernel for
> such critical error? If the kernel cannot allocate memory at this time, it
> would be useless to try continuing booting.

[*] https://lore.kernel.org/linux-sgx/YA0tvOGp%2FshchVhu@kroah.com/

BR, Jarkko

Re: [PATCH v8 5/5] certs: Allow root user to append signed hashes to the blacklist keyring

[Mickaël Salaün]

On 08/03/2022 14:19, Jarkko Sakkinen wrote:
> On Tue, Mar 08, 2022 at 01:18:28PM +0100, Mickaël Salaün wrote:
>>
>> On 08/03/2022 12:53, Jarkko Sakkinen wrote:
>>> On Mon, Jul 12, 2021 at 07:03:13PM +0200, Mickaël Salaün wrote:
>>>> From: Mickaël Salaün <mic@linux.microsoft.com>
>>>>
>>>> Add a kernel option SYSTEM_BLACKLIST_AUTH_UPDATE to enable the root user
>>>> to dynamically add new keys to the blacklist keyring.  This enables to
>>>> invalidate new certificates, either from being loaded in a keyring, or
>>>> from being trusted in a PKCS#7 certificate chain.  This also enables to
>>>> add new file hashes to be denied by the integrity infrastructure.
>>>>
>>>> Being able to untrust a certificate which could have normaly been
>>>> trusted is a sensitive operation.  This is why adding new hashes to the
>>>> blacklist keyring is only allowed when these hashes are signed and
>>>> vouched by the builtin trusted keyring.  A blacklist hash is stored as a
>>>> key description.  The PKCS#7 signature of this description must be
>>>> provided as the key payload.
>>>>
>>>> Marking a certificate as untrusted should be enforced while the system
>>>> is running.  It is then forbiden to remove such blacklist keys.
>>>>
>>>> Update blacklist keyring, blacklist key and revoked certificate access rights:
>>>> * allows the root user to search for a specific blacklisted hash, which
>>>>     make sense because the descriptions are already viewable;
>>>> * forbids key update (blacklist and asymmetric ones);
>>>> * restricts kernel rights on the blacklist keyring to align with the
>>>>     root user rights.
>>>>
>>>> See help in tools/certs/print-cert-tbs-hash.sh .
>>>>
>>>> Cc: David Howells <dhowells@redhat.com>
>>>> Cc: David Woodhouse <dwmw2@infradead.org>
>>>> Cc: Eric Snowberg <eric.snowberg@oracle.com>
>>>> Cc: Jarkko Sakkinen <jarkko@kernel.org>
>>>> Signed-off-by: Mickaël Salaün <mic@linux.microsoft.com>
>>>> Link: https://lore.kernel.org/r/20210712170313.884724-6-mic@digikod.net
>>>> ---
>>>>
>>>> Changes since v6:
>>>> * Rebase on keys-cve-2020-26541-v3: commit ebd9c2ae369a ("integrity:
>>>>     Load mokx variables into the blacklist keyring").
>>>>
>>>> Changes since v5:
>>>> * Rebase on keys-next, fix Kconfig conflict, and update the asymmetric
>>>>     key rights added to the blacklist keyring by the new
>>>>     add_key_to_revocation_list(): align with blacklist key rights by
>>>>     removing KEY_POS_WRITE as a safeguard, and add
>>>>     KEY_ALLOC_BYPASS_RESTRICTION to not be subject to
>>>>     restrict_link_for_blacklist() that only allows blacklist key types to
>>>>     be added to the keyring.
>>>> * Change the return code for restrict_link_for_blacklist() from -EPERM
>>>>     to -EOPNOTSUPP to align with asymmetric key keyrings.
>>>>
>>>> Changes since v3:
>>>> * Update commit message for print-cert-tbs-hash.sh .
>>>>
>>>> Changes since v2:
>>>> * Add comment for blacklist_key_instantiate().
>>>> ---
>>>>    certs/Kconfig     | 10 +++++
>>>>    certs/blacklist.c | 96 ++++++++++++++++++++++++++++++++++++-----------
>>>>    2 files changed, 85 insertions(+), 21 deletions(-)
>>>>
>>>> diff --git a/certs/Kconfig b/certs/Kconfig
>>>> index 0fbe184ceca5..e0e524b7eff9 100644
>>>> --- a/certs/Kconfig
>>>> +++ b/certs/Kconfig
>>>> @@ -103,4 +103,14 @@ config SYSTEM_REVOCATION_KEYS
>>>>    	  containing X.509 certificates to be included in the default blacklist
>>>>    	  keyring.
>>>> +config SYSTEM_BLACKLIST_AUTH_UPDATE
>>>> +	bool "Allow root to add signed blacklist keys"
>>>> +	depends on SYSTEM_BLACKLIST_KEYRING
>>>> +	depends on SYSTEM_DATA_VERIFICATION
>>>> +	help
>>>> +	  If set, provide the ability to load new blacklist keys at run time if
>>>> +	  they are signed and vouched by a certificate from the builtin trusted
>>>> +	  keyring.  The PKCS#7 signature of the description is set in the key
>>>> +	  payload.  Blacklist keys cannot be removed.
>>>> +
>>>>    endmenu
>>>> diff --git a/certs/blacklist.c b/certs/blacklist.c
>>>> index b254c87ceb3a..486ce0dd8e9c 100644
>>>> --- a/certs/blacklist.c
>>>> +++ b/certs/blacklist.c
>>>> @@ -15,6 +15,7 @@
>>>>    #include <linux/err.h>
>>>>    #include <linux/seq_file.h>
>>>>    #include <linux/uidgid.h>
>>>> +#include <linux/verification.h>
>>>>    #include <keys/system_keyring.h>
>>>>    #include "blacklist.h"
>>>>    #include "common.h"
>>>> @@ -26,6 +27,9 @@
>>>>     */
>>>>    #define MAX_HASH_LEN	128
>>>> +#define BLACKLIST_KEY_PERM (KEY_POS_SEARCH | KEY_POS_VIEW | \
>>>> +			    KEY_USR_SEARCH | KEY_USR_VIEW)
>>>> +
>>>>    static const char tbs_prefix[] = "tbs";
>>>>    static const char bin_prefix[] = "bin";
>>>> @@ -80,19 +84,51 @@ static int blacklist_vet_description(const char *desc)
>>>>    	return 0;
>>>>    }
>>>> -/*
>>>> - * The hash to be blacklisted is expected to be in the description.  There will
>>>> - * be no payload.
>>>> - */
>>>> -static int blacklist_preparse(struct key_preparsed_payload *prep)
>>>> +static int blacklist_key_instantiate(struct key *key,
>>>> +		struct key_preparsed_payload *prep)
>>>>    {
>>>> -	if (prep->datalen > 0)
>>>> -		return -EINVAL;
>>>> -	return 0;
>>>> +#ifdef CONFIG_SYSTEM_BLACKLIST_AUTH_UPDATE
>>>> +	int err;
>>>> +#endif
>>>> +
>>>> +	/* Sets safe default permissions for keys loaded by user space. */
>>>> +	key->perm = BLACKLIST_KEY_PERM;
>>>> +
>>>> +	/*
>>>> +	 * Skips the authentication step for builtin hashes, they are not
>>>> +	 * signed but still trusted.
>>>> +	 */
>>>> +	if (key->flags & (1 << KEY_FLAG_BUILTIN))
>>>> +		goto out;
>>>> +
>>>> +#ifdef CONFIG_SYSTEM_BLACKLIST_AUTH_UPDATE
>>>> +	/*
>>>> +	 * Verifies the description's PKCS#7 signature against the builtin
>>>> +	 * trusted keyring.
>>>> +	 */
>>>> +	err = verify_pkcs7_signature(key->description,
>>>> +			strlen(key->description), prep->data, prep->datalen,
>>>> +			NULL, VERIFYING_UNSPECIFIED_SIGNATURE, NULL, NULL);
>>>> +	if (err)
>>>> +		return err;
>>>> +#else
>>>> +	/*
>>>> +	 * It should not be possible to come here because the keyring doesn't
>>>> +	 * have KEY_USR_WRITE and the only other way to call this function is
>>>> +	 * for builtin hashes.
>>>> +	 */
>>>> +	WARN_ON_ONCE(1);
>>>> +	return -EPERM;
>>>> +#endif
>>>> +
>>>> +out:
>>>> +	return generic_key_instantiate(key, prep);
>>>>    }
>>>> -static void blacklist_free_preparse(struct key_preparsed_payload *prep)
>>>> +static int blacklist_key_update(struct key *key,
>>>> +		struct key_preparsed_payload *prep)
>>>>    {
>>>> +	return -EPERM;
>>>>    }
>>>>    static void blacklist_describe(const struct key *key, struct seq_file *m)
>>>> @@ -103,9 +139,8 @@ static void blacklist_describe(const struct key *key, struct seq_file *m)
>>>>    static struct key_type key_type_blacklist = {
>>>>    	.name			= "blacklist",
>>>>    	.vet_description	= blacklist_vet_description,
>>>> -	.preparse		= blacklist_preparse,
>>>> -	.free_preparse		= blacklist_free_preparse,
>>>> -	.instantiate		= generic_key_instantiate,
>>>> +	.instantiate		= blacklist_key_instantiate,
>>>> +	.update			= blacklist_key_update,
>>>>    	.describe		= blacklist_describe,
>>>>    };
>>>> @@ -154,8 +189,7 @@ static int mark_raw_hash_blacklisted(const char *hash)
>>>>    				   hash,
>>>>    				   NULL,
>>>>    				   0,
>>>> -				   ((KEY_POS_ALL & ~KEY_POS_SETATTR) |
>>>> -				    KEY_USR_VIEW),
>>>> +				   BLACKLIST_KEY_PERM,
>>>>    				   KEY_ALLOC_NOT_IN_QUOTA |
>>>>    				   KEY_ALLOC_BUILT_IN);
>>>>    	if (IS_ERR(key)) {
>>>> @@ -232,8 +266,10 @@ int add_key_to_revocation_list(const char *data, size_t size)
>>>>    				   NULL,
>>>>    				   data,
>>>>    				   size,
>>>> -				   ((KEY_POS_ALL & ~KEY_POS_SETATTR) | KEY_USR_VIEW),
>>>> -				   KEY_ALLOC_NOT_IN_QUOTA | KEY_ALLOC_BUILT_IN);
>>>> +				   KEY_POS_VIEW | KEY_POS_READ | KEY_POS_SEARCH
>>>> +				   | KEY_USR_VIEW,
>>>> +				   KEY_ALLOC_NOT_IN_QUOTA | KEY_ALLOC_BUILT_IN
>>>> +				   | KEY_ALLOC_BYPASS_RESTRICTION);
>>>>    	if (IS_ERR(key)) {
>>>>    		pr_err("Problem with revocation key (%ld)\n", PTR_ERR(key));
>>>> @@ -260,25 +296,43 @@ int is_key_on_revocation_list(struct pkcs7_message *pkcs7)
>>>>    }
>>>>    #endif
>>>> +static int restrict_link_for_blacklist(struct key *dest_keyring,
>>>> +		const struct key_type *type, const union key_payload *payload,
>>>> +		struct key *restrict_key)
>>>> +{
>>>> +	if (type == &key_type_blacklist)
>>>> +		return 0;
>>>> +	return -EOPNOTSUPP;
>>>> +}
>>>> +
>>>>    /*
>>>>     * Initialise the blacklist
>>>>     */
>>>>    static int __init blacklist_init(void)
>>>>    {
>>>>    	const char *const *bl;
>>>> +	struct key_restriction *restriction;
>>>>    	if (register_key_type(&key_type_blacklist) < 0)
>>>>    		panic("Can't allocate system blacklist key type\n");
>>>> +	restriction = kzalloc(sizeof(*restriction), GFP_KERNEL);
>>>> +	if (!restriction)
>>>> +		panic("Can't allocate blacklist keyring restriction\n");
>>>
>>>
>>> This prevents me from taking this to my pull request. In moderns standards,
>>> no new BUG_ON(), panic() etc. should never added to the kernel.
>>>
>>> I missed this in my review.
>>>
>>> This should rather be e.g.
>>>
>>>           restriction = kzalloc(sizeof(*restriction), GFP_KERNEL);
>>> 	if (!restriction) {
>>> 		pr_err("Can't allocate blacklist keyring restriction\n");
>>>                   return 0;
>>>           }
>>>
>>> Unfortunately I need to drop this patch set, because adding new panic()
>>> is simply a no-go.
>>
>> I agree that panic() is not great in general, but I followed the other part
>> of the code (just above) that do the same. This part of the kernel should
>> failed if critical memory allocation failed at boot time (only). It doesn't
>> impact the kernel once it is running. I don't think that just ignoring this
>> error with return 0 is fine, after all it's a critical error right?
> 
> It's not good reason enough to crash the whole kernel, even if it is a
> critical error (e.g. run-time foresincs). Even WARN() is not recommended
> these days [*].

I think that what Greg said in this email is that WARN*() should only be 
used for cases that should never happen, it is definitely not 
deprecated, but WARN_ON_ONCE() may be a better idea though. WARN*() 
helps detect such thought-to-be-impossible cases, that can happen e.g. 
with code refactoring.

A lot of initialization/boot code (e.g. without user space nor external 
interactions, mostly __init functions) do panic if there is unexpected 
and unrecoverable errors like failed memory allocations. I think 
handling such errors otherwise would be more complex for no benefit. 
Moreover, delegating such error handling to user space could create new 
(silent) issues.

> 
> For the existing panic()-statements: I'm happy to review patches that
> render them out. >
> Not sure tho, if this fails should it be then "everything blacklisted".
> Just one thing to consider.

Well, if it fail it will be "nothing will work afterwards". Do you have 
a working and useful scenario for this kind of error?

> 
>> Calling panic() seems OK here. Is there a better way to stop the kernel for
>> such critical error? If the kernel cannot allocate memory at this time, it
>> would be useless to try continuing booting.
> 
> [*] https://lore.kernel.org/linux-sgx/YA0tvOGp%2FshchVhu@kroah.com/

Re: [PATCH v8 5/5] certs: Allow root user to append signed hashes to the blacklist keyring

[Jarkko Sakkinen]

On Tue, Mar 08, 2022 at 05:02:23PM +0100, Mickaël Salaün wrote:
> 
> On 08/03/2022 14:19, Jarkko Sakkinen wrote:
> > On Tue, Mar 08, 2022 at 01:18:28PM +0100, Mickaël Salaün wrote:
> > > 
> > > On 08/03/2022 12:53, Jarkko Sakkinen wrote:
> > > > On Mon, Jul 12, 2021 at 07:03:13PM +0200, Mickaël Salaün wrote:
> > > > > From: Mickaël Salaün <mic@linux.microsoft.com>
> > > > > 
> > > > > Add a kernel option SYSTEM_BLACKLIST_AUTH_UPDATE to enable the root user
> > > > > to dynamically add new keys to the blacklist keyring.  This enables to
> > > > > invalidate new certificates, either from being loaded in a keyring, or
> > > > > from being trusted in a PKCS#7 certificate chain.  This also enables to
> > > > > add new file hashes to be denied by the integrity infrastructure.
> > > > > 
> > > > > Being able to untrust a certificate which could have normaly been
> > > > > trusted is a sensitive operation.  This is why adding new hashes to the
> > > > > blacklist keyring is only allowed when these hashes are signed and
> > > > > vouched by the builtin trusted keyring.  A blacklist hash is stored as a
> > > > > key description.  The PKCS#7 signature of this description must be
> > > > > provided as the key payload.
> > > > > 
> > > > > Marking a certificate as untrusted should be enforced while the system
> > > > > is running.  It is then forbiden to remove such blacklist keys.
> > > > > 
> > > > > Update blacklist keyring, blacklist key and revoked certificate access rights:
> > > > > * allows the root user to search for a specific blacklisted hash, which
> > > > >     make sense because the descriptions are already viewable;
> > > > > * forbids key update (blacklist and asymmetric ones);
> > > > > * restricts kernel rights on the blacklist keyring to align with the
> > > > >     root user rights.
> > > > > 
> > > > > See help in tools/certs/print-cert-tbs-hash.sh .
> > > > > 
> > > > > Cc: David Howells <dhowells@redhat.com>
> > > > > Cc: David Woodhouse <dwmw2@infradead.org>
> > > > > Cc: Eric Snowberg <eric.snowberg@oracle.com>
> > > > > Cc: Jarkko Sakkinen <jarkko@kernel.org>
> > > > > Signed-off-by: Mickaël Salaün <mic@linux.microsoft.com>
> > > > > Link: https://lore.kernel.org/r/20210712170313.884724-6-mic@digikod.net
> > > > > ---
> > > > > 
> > > > > Changes since v6:
> > > > > * Rebase on keys-cve-2020-26541-v3: commit ebd9c2ae369a ("integrity:
> > > > >     Load mokx variables into the blacklist keyring").
> > > > > 
> > > > > Changes since v5:
> > > > > * Rebase on keys-next, fix Kconfig conflict, and update the asymmetric
> > > > >     key rights added to the blacklist keyring by the new
> > > > >     add_key_to_revocation_list(): align with blacklist key rights by
> > > > >     removing KEY_POS_WRITE as a safeguard, and add
> > > > >     KEY_ALLOC_BYPASS_RESTRICTION to not be subject to
> > > > >     restrict_link_for_blacklist() that only allows blacklist key types to
> > > > >     be added to the keyring.
> > > > > * Change the return code for restrict_link_for_blacklist() from -EPERM
> > > > >     to -EOPNOTSUPP to align with asymmetric key keyrings.
> > > > > 
> > > > > Changes since v3:
> > > > > * Update commit message for print-cert-tbs-hash.sh .
> > > > > 
> > > > > Changes since v2:
> > > > > * Add comment for blacklist_key_instantiate().
> > > > > ---
> > > > >    certs/Kconfig     | 10 +++++
> > > > >    certs/blacklist.c | 96 ++++++++++++++++++++++++++++++++++++-----------
> > > > >    2 files changed, 85 insertions(+), 21 deletions(-)
> > > > > 
> > > > > diff --git a/certs/Kconfig b/certs/Kconfig
> > > > > index 0fbe184ceca5..e0e524b7eff9 100644
> > > > > --- a/certs/Kconfig
> > > > > +++ b/certs/Kconfig
> > > > > @@ -103,4 +103,14 @@ config SYSTEM_REVOCATION_KEYS
> > > > >    	  containing X.509 certificates to be included in the default blacklist
> > > > >    	  keyring.
> > > > > +config SYSTEM_BLACKLIST_AUTH_UPDATE
> > > > > +	bool "Allow root to add signed blacklist keys"
> > > > > +	depends on SYSTEM_BLACKLIST_KEYRING
> > > > > +	depends on SYSTEM_DATA_VERIFICATION
> > > > > +	help
> > > > > +	  If set, provide the ability to load new blacklist keys at run time if
> > > > > +	  they are signed and vouched by a certificate from the builtin trusted
> > > > > +	  keyring.  The PKCS#7 signature of the description is set in the key
> > > > > +	  payload.  Blacklist keys cannot be removed.
> > > > > +
> > > > >    endmenu
> > > > > diff --git a/certs/blacklist.c b/certs/blacklist.c
> > > > > index b254c87ceb3a..486ce0dd8e9c 100644
> > > > > --- a/certs/blacklist.c
> > > > > +++ b/certs/blacklist.c
> > > > > @@ -15,6 +15,7 @@
> > > > >    #include <linux/err.h>
> > > > >    #include <linux/seq_file.h>
> > > > >    #include <linux/uidgid.h>
> > > > > +#include <linux/verification.h>
> > > > >    #include <keys/system_keyring.h>
> > > > >    #include "blacklist.h"
> > > > >    #include "common.h"
> > > > > @@ -26,6 +27,9 @@
> > > > >     */
> > > > >    #define MAX_HASH_LEN	128
> > > > > +#define BLACKLIST_KEY_PERM (KEY_POS_SEARCH | KEY_POS_VIEW | \
> > > > > +			    KEY_USR_SEARCH | KEY_USR_VIEW)
> > > > > +
> > > > >    static const char tbs_prefix[] = "tbs";
> > > > >    static const char bin_prefix[] = "bin";
> > > > > @@ -80,19 +84,51 @@ static int blacklist_vet_description(const char *desc)
> > > > >    	return 0;
> > > > >    }
> > > > > -/*
> > > > > - * The hash to be blacklisted is expected to be in the description.  There will
> > > > > - * be no payload.
> > > > > - */
> > > > > -static int blacklist_preparse(struct key_preparsed_payload *prep)
> > > > > +static int blacklist_key_instantiate(struct key *key,
> > > > > +		struct key_preparsed_payload *prep)
> > > > >    {
> > > > > -	if (prep->datalen > 0)
> > > > > -		return -EINVAL;
> > > > > -	return 0;
> > > > > +#ifdef CONFIG_SYSTEM_BLACKLIST_AUTH_UPDATE
> > > > > +	int err;
> > > > > +#endif
> > > > > +
> > > > > +	/* Sets safe default permissions for keys loaded by user space. */
> > > > > +	key->perm = BLACKLIST_KEY_PERM;
> > > > > +
> > > > > +	/*
> > > > > +	 * Skips the authentication step for builtin hashes, they are not
> > > > > +	 * signed but still trusted.
> > > > > +	 */
> > > > > +	if (key->flags & (1 << KEY_FLAG_BUILTIN))
> > > > > +		goto out;
> > > > > +
> > > > > +#ifdef CONFIG_SYSTEM_BLACKLIST_AUTH_UPDATE
> > > > > +	/*
> > > > > +	 * Verifies the description's PKCS#7 signature against the builtin
> > > > > +	 * trusted keyring.
> > > > > +	 */
> > > > > +	err = verify_pkcs7_signature(key->description,
> > > > > +			strlen(key->description), prep->data, prep->datalen,
> > > > > +			NULL, VERIFYING_UNSPECIFIED_SIGNATURE, NULL, NULL);
> > > > > +	if (err)
> > > > > +		return err;
> > > > > +#else
> > > > > +	/*
> > > > > +	 * It should not be possible to come here because the keyring doesn't
> > > > > +	 * have KEY_USR_WRITE and the only other way to call this function is
> > > > > +	 * for builtin hashes.
> > > > > +	 */
> > > > > +	WARN_ON_ONCE(1);
> > > > > +	return -EPERM;
> > > > > +#endif
> > > > > +
> > > > > +out:
> > > > > +	return generic_key_instantiate(key, prep);
> > > > >    }
> > > > > -static void blacklist_free_preparse(struct key_preparsed_payload *prep)
> > > > > +static int blacklist_key_update(struct key *key,
> > > > > +		struct key_preparsed_payload *prep)
> > > > >    {
> > > > > +	return -EPERM;
> > > > >    }
> > > > >    static void blacklist_describe(const struct key *key, struct seq_file *m)
> > > > > @@ -103,9 +139,8 @@ static void blacklist_describe(const struct key *key, struct seq_file *m)
> > > > >    static struct key_type key_type_blacklist = {
> > > > >    	.name			= "blacklist",
> > > > >    	.vet_description	= blacklist_vet_description,
> > > > > -	.preparse		= blacklist_preparse,
> > > > > -	.free_preparse		= blacklist_free_preparse,
> > > > > -	.instantiate		= generic_key_instantiate,
> > > > > +	.instantiate		= blacklist_key_instantiate,
> > > > > +	.update			= blacklist_key_update,
> > > > >    	.describe		= blacklist_describe,
> > > > >    };
> > > > > @@ -154,8 +189,7 @@ static int mark_raw_hash_blacklisted(const char *hash)
> > > > >    				   hash,
> > > > >    				   NULL,
> > > > >    				   0,
> > > > > -				   ((KEY_POS_ALL & ~KEY_POS_SETATTR) |
> > > > > -				    KEY_USR_VIEW),
> > > > > +				   BLACKLIST_KEY_PERM,
> > > > >    				   KEY_ALLOC_NOT_IN_QUOTA |
> > > > >    				   KEY_ALLOC_BUILT_IN);
> > > > >    	if (IS_ERR(key)) {
> > > > > @@ -232,8 +266,10 @@ int add_key_to_revocation_list(const char *data, size_t size)
> > > > >    				   NULL,
> > > > >    				   data,
> > > > >    				   size,
> > > > > -				   ((KEY_POS_ALL & ~KEY_POS_SETATTR) | KEY_USR_VIEW),
> > > > > -				   KEY_ALLOC_NOT_IN_QUOTA | KEY_ALLOC_BUILT_IN);
> > > > > +				   KEY_POS_VIEW | KEY_POS_READ | KEY_POS_SEARCH
> > > > > +				   | KEY_USR_VIEW,
> > > > > +				   KEY_ALLOC_NOT_IN_QUOTA | KEY_ALLOC_BUILT_IN
> > > > > +				   | KEY_ALLOC_BYPASS_RESTRICTION);
> > > > >    	if (IS_ERR(key)) {
> > > > >    		pr_err("Problem with revocation key (%ld)\n", PTR_ERR(key));
> > > > > @@ -260,25 +296,43 @@ int is_key_on_revocation_list(struct pkcs7_message *pkcs7)
> > > > >    }
> > > > >    #endif
> > > > > +static int restrict_link_for_blacklist(struct key *dest_keyring,
> > > > > +		const struct key_type *type, const union key_payload *payload,
> > > > > +		struct key *restrict_key)
> > > > > +{
> > > > > +	if (type == &key_type_blacklist)
> > > > > +		return 0;
> > > > > +	return -EOPNOTSUPP;
> > > > > +}
> > > > > +
> > > > >    /*
> > > > >     * Initialise the blacklist
> > > > >     */
> > > > >    static int __init blacklist_init(void)
> > > > >    {
> > > > >    	const char *const *bl;
> > > > > +	struct key_restriction *restriction;
> > > > >    	if (register_key_type(&key_type_blacklist) < 0)
> > > > >    		panic("Can't allocate system blacklist key type\n");
> > > > > +	restriction = kzalloc(sizeof(*restriction), GFP_KERNEL);
> > > > > +	if (!restriction)
> > > > > +		panic("Can't allocate blacklist keyring restriction\n");
> > > > 
> > > > 
> > > > This prevents me from taking this to my pull request. In moderns standards,
> > > > no new BUG_ON(), panic() etc. should never added to the kernel.
> > > > 
> > > > I missed this in my review.
> > > > 
> > > > This should rather be e.g.
> > > > 
> > > >           restriction = kzalloc(sizeof(*restriction), GFP_KERNEL);
> > > > 	if (!restriction) {
> > > > 		pr_err("Can't allocate blacklist keyring restriction\n");
> > > >                   return 0;
> > > >           }
> > > > 
> > > > Unfortunately I need to drop this patch set, because adding new panic()
> > > > is simply a no-go.
> > > 
> > > I agree that panic() is not great in general, but I followed the other part
> > > of the code (just above) that do the same. This part of the kernel should
> > > failed if critical memory allocation failed at boot time (only). It doesn't
> > > impact the kernel once it is running. I don't think that just ignoring this
> > > error with return 0 is fine, after all it's a critical error right?
> > 
> > It's not good reason enough to crash the whole kernel, even if it is a
> > critical error (e.g. run-time foresincs). Even WARN() is not recommended
> > these days [*].
> 
> I think that what Greg said in this email is that WARN*() should only be
> used for cases that should never happen, it is definitely not deprecated,
> but WARN_ON_ONCE() may be a better idea though. WARN*() helps detect such
> thought-to-be-impossible cases, that can happen e.g. with code refactoring.
> 
> A lot of initialization/boot code (e.g. without user space nor external
> interactions, mostly __init functions) do panic if there is unexpected and
> unrecoverable errors like failed memory allocations. I think handling such
> errors otherwise would be more complex for no benefit. Moreover, delegating
> such error handling to user space could create new (silent) issues.

To crash the whole kernel, you should be able to clearly explain why it
makes sense in the situation.

> > 
> > For the existing panic()-statements: I'm happy to review patches that
> > render them out. >
> > Not sure tho, if this fails should it be then "everything blacklisted".
> > Just one thing to consider.
> 
> Well, if it fail it will be "nothing will work afterwards". Do you have a
> working and useful scenario for this kind of error?

So you have zero chances to get a shell without blacklist just to do
kernel forensics?

BR, Jarkko

Re: [PATCH v8 5/5] certs: Allow root user to append signed hashes to the blacklist keyring

[Mickaël Salaün]

On 09/03/2022 17:01, Jarkko Sakkinen wrote:
> On Tue, Mar 08, 2022 at 05:02:23PM +0100, Mickaël Salaün wrote:
>>
>> On 08/03/2022 14:19, Jarkko Sakkinen wrote:
>>> On Tue, Mar 08, 2022 at 01:18:28PM +0100, Mickaël Salaün wrote:
>>>>
>>>> On 08/03/2022 12:53, Jarkko Sakkinen wrote:
>>>>> On Mon, Jul 12, 2021 at 07:03:13PM +0200, Mickaël Salaün wrote:
>>>>>> From: Mickaël Salaün <mic@linux.microsoft.com>
>>>>>>
>>>>>> Add a kernel option SYSTEM_BLACKLIST_AUTH_UPDATE to enable the root user
>>>>>> to dynamically add new keys to the blacklist keyring.  This enables to
>>>>>> invalidate new certificates, either from being loaded in a keyring, or
>>>>>> from being trusted in a PKCS#7 certificate chain.  This also enables to
>>>>>> add new file hashes to be denied by the integrity infrastructure.
>>>>>>
>>>>>> Being able to untrust a certificate which could have normaly been
>>>>>> trusted is a sensitive operation.  This is why adding new hashes to the
>>>>>> blacklist keyring is only allowed when these hashes are signed and
>>>>>> vouched by the builtin trusted keyring.  A blacklist hash is stored as a
>>>>>> key description.  The PKCS#7 signature of this description must be
>>>>>> provided as the key payload.
>>>>>>
>>>>>> Marking a certificate as untrusted should be enforced while the system
>>>>>> is running.  It is then forbiden to remove such blacklist keys.
>>>>>>
>>>>>> Update blacklist keyring, blacklist key and revoked certificate access rights:
>>>>>> * allows the root user to search for a specific blacklisted hash, which
>>>>>>      make sense because the descriptions are already viewable;
>>>>>> * forbids key update (blacklist and asymmetric ones);
>>>>>> * restricts kernel rights on the blacklist keyring to align with the
>>>>>>      root user rights.
>>>>>>
>>>>>> See help in tools/certs/print-cert-tbs-hash.sh .
>>>>>>
>>>>>> Cc: David Howells <dhowells@redhat.com>
>>>>>> Cc: David Woodhouse <dwmw2@infradead.org>
>>>>>> Cc: Eric Snowberg <eric.snowberg@oracle.com>
>>>>>> Cc: Jarkko Sakkinen <jarkko@kernel.org>
>>>>>> Signed-off-by: Mickaël Salaün <mic@linux.microsoft.com>
>>>>>> Link: https://lore.kernel.org/r/20210712170313.884724-6-mic@digikod.net
>>>>>> ---
>>>>>>
>>>>>> Changes since v6:
>>>>>> * Rebase on keys-cve-2020-26541-v3: commit ebd9c2ae369a ("integrity:
>>>>>>      Load mokx variables into the blacklist keyring").
>>>>>>
>>>>>> Changes since v5:
>>>>>> * Rebase on keys-next, fix Kconfig conflict, and update the asymmetric
>>>>>>      key rights added to the blacklist keyring by the new
>>>>>>      add_key_to_revocation_list(): align with blacklist key rights by
>>>>>>      removing KEY_POS_WRITE as a safeguard, and add
>>>>>>      KEY_ALLOC_BYPASS_RESTRICTION to not be subject to
>>>>>>      restrict_link_for_blacklist() that only allows blacklist key types to
>>>>>>      be added to the keyring.
>>>>>> * Change the return code for restrict_link_for_blacklist() from -EPERM
>>>>>>      to -EOPNOTSUPP to align with asymmetric key keyrings.
>>>>>>
>>>>>> Changes since v3:
>>>>>> * Update commit message for print-cert-tbs-hash.sh .
>>>>>>
>>>>>> Changes since v2:
>>>>>> * Add comment for blacklist_key_instantiate().
>>>>>> ---
>>>>>>     certs/Kconfig     | 10 +++++
>>>>>>     certs/blacklist.c | 96 ++++++++++++++++++++++++++++++++++++-----------
>>>>>>     2 files changed, 85 insertions(+), 21 deletions(-)
>>>>>>
>>>>>> diff --git a/certs/Kconfig b/certs/Kconfig
>>>>>> index 0fbe184ceca5..e0e524b7eff9 100644
>>>>>> --- a/certs/Kconfig
>>>>>> +++ b/certs/Kconfig
>>>>>> @@ -103,4 +103,14 @@ config SYSTEM_REVOCATION_KEYS
>>>>>>     	  containing X.509 certificates to be included in the default blacklist
>>>>>>     	  keyring.
>>>>>> +config SYSTEM_BLACKLIST_AUTH_UPDATE
>>>>>> +	bool "Allow root to add signed blacklist keys"
>>>>>> +	depends on SYSTEM_BLACKLIST_KEYRING
>>>>>> +	depends on SYSTEM_DATA_VERIFICATION
>>>>>> +	help
>>>>>> +	  If set, provide the ability to load new blacklist keys at run time if
>>>>>> +	  they are signed and vouched by a certificate from the builtin trusted
>>>>>> +	  keyring.  The PKCS#7 signature of the description is set in the key
>>>>>> +	  payload.  Blacklist keys cannot be removed.
>>>>>> +
>>>>>>     endmenu
>>>>>> diff --git a/certs/blacklist.c b/certs/blacklist.c
>>>>>> index b254c87ceb3a..486ce0dd8e9c 100644
>>>>>> --- a/certs/blacklist.c
>>>>>> +++ b/certs/blacklist.c
>>>>>> @@ -15,6 +15,7 @@
>>>>>>     #include <linux/err.h>
>>>>>>     #include <linux/seq_file.h>
>>>>>>     #include <linux/uidgid.h>
>>>>>> +#include <linux/verification.h>
>>>>>>     #include <keys/system_keyring.h>
>>>>>>     #include "blacklist.h"
>>>>>>     #include "common.h"
>>>>>> @@ -26,6 +27,9 @@
>>>>>>      */
>>>>>>     #define MAX_HASH_LEN	128
>>>>>> +#define BLACKLIST_KEY_PERM (KEY_POS_SEARCH | KEY_POS_VIEW | \
>>>>>> +			    KEY_USR_SEARCH | KEY_USR_VIEW)
>>>>>> +
>>>>>>     static const char tbs_prefix[] = "tbs";
>>>>>>     static const char bin_prefix[] = "bin";
>>>>>> @@ -80,19 +84,51 @@ static int blacklist_vet_description(const char *desc)
>>>>>>     	return 0;
>>>>>>     }
>>>>>> -/*
>>>>>> - * The hash to be blacklisted is expected to be in the description.  There will
>>>>>> - * be no payload.
>>>>>> - */
>>>>>> -static int blacklist_preparse(struct key_preparsed_payload *prep)
>>>>>> +static int blacklist_key_instantiate(struct key *key,
>>>>>> +		struct key_preparsed_payload *prep)
>>>>>>     {
>>>>>> -	if (prep->datalen > 0)
>>>>>> -		return -EINVAL;
>>>>>> -	return 0;
>>>>>> +#ifdef CONFIG_SYSTEM_BLACKLIST_AUTH_UPDATE
>>>>>> +	int err;
>>>>>> +#endif
>>>>>> +
>>>>>> +	/* Sets safe default permissions for keys loaded by user space. */
>>>>>> +	key->perm = BLACKLIST_KEY_PERM;
>>>>>> +
>>>>>> +	/*
>>>>>> +	 * Skips the authentication step for builtin hashes, they are not
>>>>>> +	 * signed but still trusted.
>>>>>> +	 */
>>>>>> +	if (key->flags & (1 << KEY_FLAG_BUILTIN))
>>>>>> +		goto out;
>>>>>> +
>>>>>> +#ifdef CONFIG_SYSTEM_BLACKLIST_AUTH_UPDATE
>>>>>> +	/*
>>>>>> +	 * Verifies the description's PKCS#7 signature against the builtin
>>>>>> +	 * trusted keyring.
>>>>>> +	 */
>>>>>> +	err = verify_pkcs7_signature(key->description,
>>>>>> +			strlen(key->description), prep->data, prep->datalen,
>>>>>> +			NULL, VERIFYING_UNSPECIFIED_SIGNATURE, NULL, NULL);
>>>>>> +	if (err)
>>>>>> +		return err;
>>>>>> +#else
>>>>>> +	/*
>>>>>> +	 * It should not be possible to come here because the keyring doesn't
>>>>>> +	 * have KEY_USR_WRITE and the only other way to call this function is
>>>>>> +	 * for builtin hashes.
>>>>>> +	 */
>>>>>> +	WARN_ON_ONCE(1);
>>>>>> +	return -EPERM;
>>>>>> +#endif
>>>>>> +
>>>>>> +out:
>>>>>> +	return generic_key_instantiate(key, prep);
>>>>>>     }
>>>>>> -static void blacklist_free_preparse(struct key_preparsed_payload *prep)
>>>>>> +static int blacklist_key_update(struct key *key,
>>>>>> +		struct key_preparsed_payload *prep)
>>>>>>     {
>>>>>> +	return -EPERM;
>>>>>>     }
>>>>>>     static void blacklist_describe(const struct key *key, struct seq_file *m)
>>>>>> @@ -103,9 +139,8 @@ static void blacklist_describe(const struct key *key, struct seq_file *m)
>>>>>>     static struct key_type key_type_blacklist = {
>>>>>>     	.name			= "blacklist",
>>>>>>     	.vet_description	= blacklist_vet_description,
>>>>>> -	.preparse		= blacklist_preparse,
>>>>>> -	.free_preparse		= blacklist_free_preparse,
>>>>>> -	.instantiate		= generic_key_instantiate,
>>>>>> +	.instantiate		= blacklist_key_instantiate,
>>>>>> +	.update			= blacklist_key_update,
>>>>>>     	.describe		= blacklist_describe,
>>>>>>     };
>>>>>> @@ -154,8 +189,7 @@ static int mark_raw_hash_blacklisted(const char *hash)
>>>>>>     				   hash,
>>>>>>     				   NULL,
>>>>>>     				   0,
>>>>>> -				   ((KEY_POS_ALL & ~KEY_POS_SETATTR) |
>>>>>> -				    KEY_USR_VIEW),
>>>>>> +				   BLACKLIST_KEY_PERM,
>>>>>>     				   KEY_ALLOC_NOT_IN_QUOTA |
>>>>>>     				   KEY_ALLOC_BUILT_IN);
>>>>>>     	if (IS_ERR(key)) {
>>>>>> @@ -232,8 +266,10 @@ int add_key_to_revocation_list(const char *data, size_t size)
>>>>>>     				   NULL,
>>>>>>     				   data,
>>>>>>     				   size,
>>>>>> -				   ((KEY_POS_ALL & ~KEY_POS_SETATTR) | KEY_USR_VIEW),
>>>>>> -				   KEY_ALLOC_NOT_IN_QUOTA | KEY_ALLOC_BUILT_IN);
>>>>>> +				   KEY_POS_VIEW | KEY_POS_READ | KEY_POS_SEARCH
>>>>>> +				   | KEY_USR_VIEW,
>>>>>> +				   KEY_ALLOC_NOT_IN_QUOTA | KEY_ALLOC_BUILT_IN
>>>>>> +				   | KEY_ALLOC_BYPASS_RESTRICTION);
>>>>>>     	if (IS_ERR(key)) {
>>>>>>     		pr_err("Problem with revocation key (%ld)\n", PTR_ERR(key));
>>>>>> @@ -260,25 +296,43 @@ int is_key_on_revocation_list(struct pkcs7_message *pkcs7)
>>>>>>     }
>>>>>>     #endif
>>>>>> +static int restrict_link_for_blacklist(struct key *dest_keyring,
>>>>>> +		const struct key_type *type, const union key_payload *payload,
>>>>>> +		struct key *restrict_key)
>>>>>> +{
>>>>>> +	if (type == &key_type_blacklist)
>>>>>> +		return 0;
>>>>>> +	return -EOPNOTSUPP;
>>>>>> +}
>>>>>> +
>>>>>>     /*
>>>>>>      * Initialise the blacklist
>>>>>>      */
>>>>>>     static int __init blacklist_init(void)
>>>>>>     {
>>>>>>     	const char *const *bl;
>>>>>> +	struct key_restriction *restriction;
>>>>>>     	if (register_key_type(&key_type_blacklist) < 0)
>>>>>>     		panic("Can't allocate system blacklist key type\n");
>>>>>> +	restriction = kzalloc(sizeof(*restriction), GFP_KERNEL);
>>>>>> +	if (!restriction)
>>>>>> +		panic("Can't allocate blacklist keyring restriction\n");
>>>>>
>>>>>
>>>>> This prevents me from taking this to my pull request. In moderns standards,
>>>>> no new BUG_ON(), panic() etc. should never added to the kernel.
>>>>>
>>>>> I missed this in my review.
>>>>>
>>>>> This should rather be e.g.
>>>>>
>>>>>            restriction = kzalloc(sizeof(*restriction), GFP_KERNEL);
>>>>> 	if (!restriction) {
>>>>> 		pr_err("Can't allocate blacklist keyring restriction\n");
>>>>>                    return 0;
>>>>>            }
>>>>>
>>>>> Unfortunately I need to drop this patch set, because adding new panic()
>>>>> is simply a no-go.
>>>>
>>>> I agree that panic() is not great in general, but I followed the other part
>>>> of the code (just above) that do the same. This part of the kernel should
>>>> failed if critical memory allocation failed at boot time (only). It doesn't
>>>> impact the kernel once it is running. I don't think that just ignoring this
>>>> error with return 0 is fine, after all it's a critical error right?
>>>
>>> It's not good reason enough to crash the whole kernel, even if it is a
>>> critical error (e.g. run-time foresincs). Even WARN() is not recommended
>>> these days [*].
>>
>> I think that what Greg said in this email is that WARN*() should only be
>> used for cases that should never happen, it is definitely not deprecated,
>> but WARN_ON_ONCE() may be a better idea though. WARN*() helps detect such
>> thought-to-be-impossible cases, that can happen e.g. with code refactoring.
>>
>> A lot of initialization/boot code (e.g. without user space nor external
>> interactions, mostly __init functions) do panic if there is unexpected and
>> unrecoverable errors like failed memory allocations. I think handling such
>> errors otherwise would be more complex for no benefit. Moreover, delegating
>> such error handling to user space could create new (silent) issues.
> 
> To crash the whole kernel, you should be able to clearly explain why it
> makes sense in the situation.

If there is no enough memory to allocate 24 bytes (struct 
key_restriction) during early boot, I really doubt the kernel can do much.

> 
>>>
>>> For the existing panic()-statements: I'm happy to review patches that
>>> render them out. >
>>> Not sure tho, if this fails should it be then "everything blacklisted".
>>> Just one thing to consider.
>>
>> Well, if it fail it will be "nothing will work afterwards". Do you have a
>> working and useful scenario for this kind of error?
> 
> So you have zero chances to get a shell without blacklist just to do
> kernel forensics?

Right, I don't think the kernel can launch any process (nor continue 
early boot) if it cannot allocate 24 bytes.

Re: [PATCH v8 5/5] certs: Allow root user to append signed hashes to the blacklist keyring

[Jarkko Sakkinen]

On Wed, Mar 09, 2022 at 07:36:50PM +0100, Mickaël Salaün wrote:
> 
> On 09/03/2022 17:01, Jarkko Sakkinen wrote:
> > On Tue, Mar 08, 2022 at 05:02:23PM +0100, Mickaël Salaün wrote:
> > > 
> > > On 08/03/2022 14:19, Jarkko Sakkinen wrote:
> > > > On Tue, Mar 08, 2022 at 01:18:28PM +0100, Mickaël Salaün wrote:
> > > > > 
> > > > > On 08/03/2022 12:53, Jarkko Sakkinen wrote:
> > > > > > On Mon, Jul 12, 2021 at 07:03:13PM +0200, Mickaël Salaün wrote:
> > > > > > > From: Mickaël Salaün <mic@linux.microsoft.com>
> > > > > > > 
> > > > > > > Add a kernel option SYSTEM_BLACKLIST_AUTH_UPDATE to enable the root user
> > > > > > > to dynamically add new keys to the blacklist keyring.  This enables to
> > > > > > > invalidate new certificates, either from being loaded in a keyring, or
> > > > > > > from being trusted in a PKCS#7 certificate chain.  This also enables to
> > > > > > > add new file hashes to be denied by the integrity infrastructure.
> > > > > > > 
> > > > > > > Being able to untrust a certificate which could have normaly been
> > > > > > > trusted is a sensitive operation.  This is why adding new hashes to the
> > > > > > > blacklist keyring is only allowed when these hashes are signed and
> > > > > > > vouched by the builtin trusted keyring.  A blacklist hash is stored as a
> > > > > > > key description.  The PKCS#7 signature of this description must be
> > > > > > > provided as the key payload.
> > > > > > > 
> > > > > > > Marking a certificate as untrusted should be enforced while the system
> > > > > > > is running.  It is then forbiden to remove such blacklist keys.
> > > > > > > 
> > > > > > > Update blacklist keyring, blacklist key and revoked certificate access rights:
> > > > > > > * allows the root user to search for a specific blacklisted hash, which
> > > > > > >      make sense because the descriptions are already viewable;
> > > > > > > * forbids key update (blacklist and asymmetric ones);
> > > > > > > * restricts kernel rights on the blacklist keyring to align with the
> > > > > > >      root user rights.
> > > > > > > 
> > > > > > > See help in tools/certs/print-cert-tbs-hash.sh .
> > > > > > > 
> > > > > > > Cc: David Howells <dhowells@redhat.com>
> > > > > > > Cc: David Woodhouse <dwmw2@infradead.org>
> > > > > > > Cc: Eric Snowberg <eric.snowberg@oracle.com>
> > > > > > > Cc: Jarkko Sakkinen <jarkko@kernel.org>
> > > > > > > Signed-off-by: Mickaël Salaün <mic@linux.microsoft.com>
> > > > > > > Link: https://lore.kernel.org/r/20210712170313.884724-6-mic@digikod.net
> > > > > > > ---
> > > > > > > 
> > > > > > > Changes since v6:
> > > > > > > * Rebase on keys-cve-2020-26541-v3: commit ebd9c2ae369a ("integrity:
> > > > > > >      Load mokx variables into the blacklist keyring").
> > > > > > > 
> > > > > > > Changes since v5:
> > > > > > > * Rebase on keys-next, fix Kconfig conflict, and update the asymmetric
> > > > > > >      key rights added to the blacklist keyring by the new
> > > > > > >      add_key_to_revocation_list(): align with blacklist key rights by
> > > > > > >      removing KEY_POS_WRITE as a safeguard, and add
> > > > > > >      KEY_ALLOC_BYPASS_RESTRICTION to not be subject to
> > > > > > >      restrict_link_for_blacklist() that only allows blacklist key types to
> > > > > > >      be added to the keyring.
> > > > > > > * Change the return code for restrict_link_for_blacklist() from -EPERM
> > > > > > >      to -EOPNOTSUPP to align with asymmetric key keyrings.
> > > > > > > 
> > > > > > > Changes since v3:
> > > > > > > * Update commit message for print-cert-tbs-hash.sh .
> > > > > > > 
> > > > > > > Changes since v2:
> > > > > > > * Add comment for blacklist_key_instantiate().
> > > > > > > ---
> > > > > > >     certs/Kconfig     | 10 +++++
> > > > > > >     certs/blacklist.c | 96 ++++++++++++++++++++++++++++++++++++-----------
> > > > > > >     2 files changed, 85 insertions(+), 21 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/certs/Kconfig b/certs/Kconfig
> > > > > > > index 0fbe184ceca5..e0e524b7eff9 100644
> > > > > > > --- a/certs/Kconfig
> > > > > > > +++ b/certs/Kconfig
> > > > > > > @@ -103,4 +103,14 @@ config SYSTEM_REVOCATION_KEYS
> > > > > > >     	  containing X.509 certificates to be included in the default blacklist
> > > > > > >     	  keyring.
> > > > > > > +config SYSTEM_BLACKLIST_AUTH_UPDATE
> > > > > > > +	bool "Allow root to add signed blacklist keys"
> > > > > > > +	depends on SYSTEM_BLACKLIST_KEYRING
> > > > > > > +	depends on SYSTEM_DATA_VERIFICATION
> > > > > > > +	help
> > > > > > > +	  If set, provide the ability to load new blacklist keys at run time if
> > > > > > > +	  they are signed and vouched by a certificate from the builtin trusted
> > > > > > > +	  keyring.  The PKCS#7 signature of the description is set in the key
> > > > > > > +	  payload.  Blacklist keys cannot be removed.
> > > > > > > +
> > > > > > >     endmenu
> > > > > > > diff --git a/certs/blacklist.c b/certs/blacklist.c
> > > > > > > index b254c87ceb3a..486ce0dd8e9c 100644
> > > > > > > --- a/certs/blacklist.c
> > > > > > > +++ b/certs/blacklist.c
> > > > > > > @@ -15,6 +15,7 @@
> > > > > > >     #include <linux/err.h>
> > > > > > >     #include <linux/seq_file.h>
> > > > > > >     #include <linux/uidgid.h>
> > > > > > > +#include <linux/verification.h>
> > > > > > >     #include <keys/system_keyring.h>
> > > > > > >     #include "blacklist.h"
> > > > > > >     #include "common.h"
> > > > > > > @@ -26,6 +27,9 @@
> > > > > > >      */
> > > > > > >     #define MAX_HASH_LEN	128
> > > > > > > +#define BLACKLIST_KEY_PERM (KEY_POS_SEARCH | KEY_POS_VIEW | \
> > > > > > > +			    KEY_USR_SEARCH | KEY_USR_VIEW)
> > > > > > > +
> > > > > > >     static const char tbs_prefix[] = "tbs";
> > > > > > >     static const char bin_prefix[] = "bin";
> > > > > > > @@ -80,19 +84,51 @@ static int blacklist_vet_description(const char *desc)
> > > > > > >     	return 0;
> > > > > > >     }
> > > > > > > -/*
> > > > > > > - * The hash to be blacklisted is expected to be in the description.  There will
> > > > > > > - * be no payload.
> > > > > > > - */
> > > > > > > -static int blacklist_preparse(struct key_preparsed_payload *prep)
> > > > > > > +static int blacklist_key_instantiate(struct key *key,
> > > > > > > +		struct key_preparsed_payload *prep)
> > > > > > >     {
> > > > > > > -	if (prep->datalen > 0)
> > > > > > > -		return -EINVAL;
> > > > > > > -	return 0;
> > > > > > > +#ifdef CONFIG_SYSTEM_BLACKLIST_AUTH_UPDATE
> > > > > > > +	int err;
> > > > > > > +#endif
> > > > > > > +
> > > > > > > +	/* Sets safe default permissions for keys loaded by user space. */
> > > > > > > +	key->perm = BLACKLIST_KEY_PERM;
> > > > > > > +
> > > > > > > +	/*
> > > > > > > +	 * Skips the authentication step for builtin hashes, they are not
> > > > > > > +	 * signed but still trusted.
> > > > > > > +	 */
> > > > > > > +	if (key->flags & (1 << KEY_FLAG_BUILTIN))
> > > > > > > +		goto out;
> > > > > > > +
> > > > > > > +#ifdef CONFIG_SYSTEM_BLACKLIST_AUTH_UPDATE
> > > > > > > +	/*
> > > > > > > +	 * Verifies the description's PKCS#7 signature against the builtin
> > > > > > > +	 * trusted keyring.
> > > > > > > +	 */
> > > > > > > +	err = verify_pkcs7_signature(key->description,
> > > > > > > +			strlen(key->description), prep->data, prep->datalen,
> > > > > > > +			NULL, VERIFYING_UNSPECIFIED_SIGNATURE, NULL, NULL);
> > > > > > > +	if (err)
> > > > > > > +		return err;
> > > > > > > +#else
> > > > > > > +	/*
> > > > > > > +	 * It should not be possible to come here because the keyring doesn't
> > > > > > > +	 * have KEY_USR_WRITE and the only other way to call this function is
> > > > > > > +	 * for builtin hashes.
> > > > > > > +	 */
> > > > > > > +	WARN_ON_ONCE(1);
> > > > > > > +	return -EPERM;
> > > > > > > +#endif
> > > > > > > +
> > > > > > > +out:
> > > > > > > +	return generic_key_instantiate(key, prep);
> > > > > > >     }
> > > > > > > -static void blacklist_free_preparse(struct key_preparsed_payload *prep)
> > > > > > > +static int blacklist_key_update(struct key *key,
> > > > > > > +		struct key_preparsed_payload *prep)
> > > > > > >     {
> > > > > > > +	return -EPERM;
> > > > > > >     }
> > > > > > >     static void blacklist_describe(const struct key *key, struct seq_file *m)
> > > > > > > @@ -103,9 +139,8 @@ static void blacklist_describe(const struct key *key, struct seq_file *m)
> > > > > > >     static struct key_type key_type_blacklist = {
> > > > > > >     	.name			= "blacklist",
> > > > > > >     	.vet_description	= blacklist_vet_description,
> > > > > > > -	.preparse		= blacklist_preparse,
> > > > > > > -	.free_preparse		= blacklist_free_preparse,
> > > > > > > -	.instantiate		= generic_key_instantiate,
> > > > > > > +	.instantiate		= blacklist_key_instantiate,
> > > > > > > +	.update			= blacklist_key_update,
> > > > > > >     	.describe		= blacklist_describe,
> > > > > > >     };
> > > > > > > @@ -154,8 +189,7 @@ static int mark_raw_hash_blacklisted(const char *hash)
> > > > > > >     				   hash,
> > > > > > >     				   NULL,
> > > > > > >     				   0,
> > > > > > > -				   ((KEY_POS_ALL & ~KEY_POS_SETATTR) |
> > > > > > > -				    KEY_USR_VIEW),
> > > > > > > +				   BLACKLIST_KEY_PERM,
> > > > > > >     				   KEY_ALLOC_NOT_IN_QUOTA |
> > > > > > >     				   KEY_ALLOC_BUILT_IN);
> > > > > > >     	if (IS_ERR(key)) {
> > > > > > > @@ -232,8 +266,10 @@ int add_key_to_revocation_list(const char *data, size_t size)
> > > > > > >     				   NULL,
> > > > > > >     				   data,
> > > > > > >     				   size,
> > > > > > > -				   ((KEY_POS_ALL & ~KEY_POS_SETATTR) | KEY_USR_VIEW),
> > > > > > > -				   KEY_ALLOC_NOT_IN_QUOTA | KEY_ALLOC_BUILT_IN);
> > > > > > > +				   KEY_POS_VIEW | KEY_POS_READ | KEY_POS_SEARCH
> > > > > > > +				   | KEY_USR_VIEW,
> > > > > > > +				   KEY_ALLOC_NOT_IN_QUOTA | KEY_ALLOC_BUILT_IN
> > > > > > > +				   | KEY_ALLOC_BYPASS_RESTRICTION);
> > > > > > >     	if (IS_ERR(key)) {
> > > > > > >     		pr_err("Problem with revocation key (%ld)\n", PTR_ERR(key));
> > > > > > > @@ -260,25 +296,43 @@ int is_key_on_revocation_list(struct pkcs7_message *pkcs7)
> > > > > > >     }
> > > > > > >     #endif
> > > > > > > +static int restrict_link_for_blacklist(struct key *dest_keyring,
> > > > > > > +		const struct key_type *type, const union key_payload *payload,
> > > > > > > +		struct key *restrict_key)
> > > > > > > +{
> > > > > > > +	if (type == &key_type_blacklist)
> > > > > > > +		return 0;
> > > > > > > +	return -EOPNOTSUPP;
> > > > > > > +}
> > > > > > > +
> > > > > > >     /*
> > > > > > >      * Initialise the blacklist
> > > > > > >      */
> > > > > > >     static int __init blacklist_init(void)
> > > > > > >     {
> > > > > > >     	const char *const *bl;
> > > > > > > +	struct key_restriction *restriction;
> > > > > > >     	if (register_key_type(&key_type_blacklist) < 0)
> > > > > > >     		panic("Can't allocate system blacklist key type\n");
> > > > > > > +	restriction = kzalloc(sizeof(*restriction), GFP_KERNEL);
> > > > > > > +	if (!restriction)
> > > > > > > +		panic("Can't allocate blacklist keyring restriction\n");
> > > > > > 
> > > > > > 
> > > > > > This prevents me from taking this to my pull request. In moderns standards,
> > > > > > no new BUG_ON(), panic() etc. should never added to the kernel.
> > > > > > 
> > > > > > I missed this in my review.
> > > > > > 
> > > > > > This should rather be e.g.
> > > > > > 
> > > > > >            restriction = kzalloc(sizeof(*restriction), GFP_KERNEL);
> > > > > > 	if (!restriction) {
> > > > > > 		pr_err("Can't allocate blacklist keyring restriction\n");
> > > > > >                    return 0;
> > > > > >            }
> > > > > > 
> > > > > > Unfortunately I need to drop this patch set, because adding new panic()
> > > > > > is simply a no-go.
> > > > > 
> > > > > I agree that panic() is not great in general, but I followed the other part
> > > > > of the code (just above) that do the same. This part of the kernel should
> > > > > failed if critical memory allocation failed at boot time (only). It doesn't
> > > > > impact the kernel once it is running. I don't think that just ignoring this
> > > > > error with return 0 is fine, after all it's a critical error right?
> > > > 
> > > > It's not good reason enough to crash the whole kernel, even if it is a
> > > > critical error (e.g. run-time foresincs). Even WARN() is not recommended
> > > > these days [*].
> > > 
> > > I think that what Greg said in this email is that WARN*() should only be
> > > used for cases that should never happen, it is definitely not deprecated,
> > > but WARN_ON_ONCE() may be a better idea though. WARN*() helps detect such
> > > thought-to-be-impossible cases, that can happen e.g. with code refactoring.
> > > 
> > > A lot of initialization/boot code (e.g. without user space nor external
> > > interactions, mostly __init functions) do panic if there is unexpected and
> > > unrecoverable errors like failed memory allocations. I think handling such
> > > errors otherwise would be more complex for no benefit. Moreover, delegating
> > > such error handling to user space could create new (silent) issues.
> > 
> > To crash the whole kernel, you should be able to clearly explain why it
> > makes sense in the situation.
> 
> If there is no enough memory to allocate 24 bytes (struct key_restriction)
> during early boot, I really doubt the kernel can do much.
> 
> > 
> > > > 
> > > > For the existing panic()-statements: I'm happy to review patches that
> > > > render them out. >
> > > > Not sure tho, if this fails should it be then "everything blacklisted".
> > > > Just one thing to consider.
> > > 
> > > Well, if it fail it will be "nothing will work afterwards". Do you have a
> > > working and useful scenario for this kind of error?
> > 
> > So you have zero chances to get a shell without blacklist just to do
> > kernel forensics?
> 
> Right, I don't think the kernel can launch any process (nor continue early
> boot) if it cannot allocate 24 bytes.

initcall is just wrong layer to choose to crash the kernel. It should be
either do_initcall_level() or do_one_initcall() that should care about
this (or not care). You can print error message and return -ENODEV;

BR, Jarkko

Re: [PATCH v8 5/5] certs: Allow root user to append signed hashes to the blacklist keyring

[Mickaël Salaün]

On 10/03/2022 00:11, Jarkko Sakkinen wrote:
> On Wed, Mar 09, 2022 at 07:36:50PM +0100, Mickaël Salaün wrote:
>>
>> On 09/03/2022 17:01, Jarkko Sakkinen wrote:
>>> On Tue, Mar 08, 2022 at 05:02:23PM +0100, Mickaël Salaün wrote:
>>>>
>>>> On 08/03/2022 14:19, Jarkko Sakkinen wrote:
>>>>> On Tue, Mar 08, 2022 at 01:18:28PM +0100, Mickaël Salaün wrote:
>>>>>>
>>>>>> On 08/03/2022 12:53, Jarkko Sakkinen wrote:
>>>>>>> On Mon, Jul 12, 2021 at 07:03:13PM +0200, Mickaël Salaün wrote:
>>>>>>>> From: Mickaël Salaün <mic@linux.microsoft.com>
>>>>>>>>
>>>>>>>> Add a kernel option SYSTEM_BLACKLIST_AUTH_UPDATE to enable the root user
>>>>>>>> to dynamically add new keys to the blacklist keyring.  This enables to
>>>>>>>> invalidate new certificates, either from being loaded in a keyring, or
>>>>>>>> from being trusted in a PKCS#7 certificate chain.  This also enables to
>>>>>>>> add new file hashes to be denied by the integrity infrastructure.
>>>>>>>>
>>>>>>>> Being able to untrust a certificate which could have normaly been
>>>>>>>> trusted is a sensitive operation.  This is why adding new hashes to the
>>>>>>>> blacklist keyring is only allowed when these hashes are signed and
>>>>>>>> vouched by the builtin trusted keyring.  A blacklist hash is stored as a
>>>>>>>> key description.  The PKCS#7 signature of this description must be
>>>>>>>> provided as the key payload.
>>>>>>>>
>>>>>>>> Marking a certificate as untrusted should be enforced while the system
>>>>>>>> is running.  It is then forbiden to remove such blacklist keys.
>>>>>>>>
>>>>>>>> Update blacklist keyring, blacklist key and revoked certificate access rights:
>>>>>>>> * allows the root user to search for a specific blacklisted hash, which
>>>>>>>>       make sense because the descriptions are already viewable;
>>>>>>>> * forbids key update (blacklist and asymmetric ones);
>>>>>>>> * restricts kernel rights on the blacklist keyring to align with the
>>>>>>>>       root user rights.
>>>>>>>>
>>>>>>>> See help in tools/certs/print-cert-tbs-hash.sh .
>>>>>>>>
>>>>>>>> Cc: David Howells <dhowells@redhat.com>
>>>>>>>> Cc: David Woodhouse <dwmw2@infradead.org>
>>>>>>>> Cc: Eric Snowberg <eric.snowberg@oracle.com>
>>>>>>>> Cc: Jarkko Sakkinen <jarkko@kernel.org>
>>>>>>>> Signed-off-by: Mickaël Salaün <mic@linux.microsoft.com>
>>>>>>>> Link: https://lore.kernel.org/r/20210712170313.884724-6-mic@digikod.net
>>>>>>>> ---
>>>>>>>>
>>>>>>>> Changes since v6:
>>>>>>>> * Rebase on keys-cve-2020-26541-v3: commit ebd9c2ae369a ("integrity:
>>>>>>>>       Load mokx variables into the blacklist keyring").
>>>>>>>>
>>>>>>>> Changes since v5:
>>>>>>>> * Rebase on keys-next, fix Kconfig conflict, and update the asymmetric
>>>>>>>>       key rights added to the blacklist keyring by the new
>>>>>>>>       add_key_to_revocation_list(): align with blacklist key rights by
>>>>>>>>       removing KEY_POS_WRITE as a safeguard, and add
>>>>>>>>       KEY_ALLOC_BYPASS_RESTRICTION to not be subject to
>>>>>>>>       restrict_link_for_blacklist() that only allows blacklist key types to
>>>>>>>>       be added to the keyring.
>>>>>>>> * Change the return code for restrict_link_for_blacklist() from -EPERM
>>>>>>>>       to -EOPNOTSUPP to align with asymmetric key keyrings.
>>>>>>>>
>>>>>>>> Changes since v3:
>>>>>>>> * Update commit message for print-cert-tbs-hash.sh .
>>>>>>>>
>>>>>>>> Changes since v2:
>>>>>>>> * Add comment for blacklist_key_instantiate().
>>>>>>>> ---
>>>>>>>>      certs/Kconfig     | 10 +++++
>>>>>>>>      certs/blacklist.c | 96 ++++++++++++++++++++++++++++++++++++-----------
>>>>>>>>      2 files changed, 85 insertions(+), 21 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/certs/Kconfig b/certs/Kconfig
>>>>>>>> index 0fbe184ceca5..e0e524b7eff9 100644
>>>>>>>> --- a/certs/Kconfig
>>>>>>>> +++ b/certs/Kconfig
>>>>>>>> @@ -103,4 +103,14 @@ config SYSTEM_REVOCATION_KEYS
>>>>>>>>      	  containing X.509 certificates to be included in the default blacklist
>>>>>>>>      	  keyring.
>>>>>>>> +config SYSTEM_BLACKLIST_AUTH_UPDATE
>>>>>>>> +	bool "Allow root to add signed blacklist keys"
>>>>>>>> +	depends on SYSTEM_BLACKLIST_KEYRING
>>>>>>>> +	depends on SYSTEM_DATA_VERIFICATION
>>>>>>>> +	help
>>>>>>>> +	  If set, provide the ability to load new blacklist keys at run time if
>>>>>>>> +	  they are signed and vouched by a certificate from the builtin trusted
>>>>>>>> +	  keyring.  The PKCS#7 signature of the description is set in the key
>>>>>>>> +	  payload.  Blacklist keys cannot be removed.
>>>>>>>> +
>>>>>>>>      endmenu
>>>>>>>> diff --git a/certs/blacklist.c b/certs/blacklist.c
>>>>>>>> index b254c87ceb3a..486ce0dd8e9c 100644
>>>>>>>> --- a/certs/blacklist.c
>>>>>>>> +++ b/certs/blacklist.c
>>>>>>>> @@ -15,6 +15,7 @@
>>>>>>>>      #include <linux/err.h>
>>>>>>>>      #include <linux/seq_file.h>
>>>>>>>>      #include <linux/uidgid.h>
>>>>>>>> +#include <linux/verification.h>
>>>>>>>>      #include <keys/system_keyring.h>
>>>>>>>>      #include "blacklist.h"
>>>>>>>>      #include "common.h"
>>>>>>>> @@ -26,6 +27,9 @@
>>>>>>>>       */
>>>>>>>>      #define MAX_HASH_LEN	128
>>>>>>>> +#define BLACKLIST_KEY_PERM (KEY_POS_SEARCH | KEY_POS_VIEW | \
>>>>>>>> +			    KEY_USR_SEARCH | KEY_USR_VIEW)
>>>>>>>> +
>>>>>>>>      static const char tbs_prefix[] = "tbs";
>>>>>>>>      static const char bin_prefix[] = "bin";
>>>>>>>> @@ -80,19 +84,51 @@ static int blacklist_vet_description(const char *desc)
>>>>>>>>      	return 0;
>>>>>>>>      }
>>>>>>>> -/*
>>>>>>>> - * The hash to be blacklisted is expected to be in the description.  There will
>>>>>>>> - * be no payload.
>>>>>>>> - */
>>>>>>>> -static int blacklist_preparse(struct key_preparsed_payload *prep)
>>>>>>>> +static int blacklist_key_instantiate(struct key *key,
>>>>>>>> +		struct key_preparsed_payload *prep)
>>>>>>>>      {
>>>>>>>> -	if (prep->datalen > 0)
>>>>>>>> -		return -EINVAL;
>>>>>>>> -	return 0;
>>>>>>>> +#ifdef CONFIG_SYSTEM_BLACKLIST_AUTH_UPDATE
>>>>>>>> +	int err;
>>>>>>>> +#endif
>>>>>>>> +
>>>>>>>> +	/* Sets safe default permissions for keys loaded by user space. */
>>>>>>>> +	key->perm = BLACKLIST_KEY_PERM;
>>>>>>>> +
>>>>>>>> +	/*
>>>>>>>> +	 * Skips the authentication step for builtin hashes, they are not
>>>>>>>> +	 * signed but still trusted.
>>>>>>>> +	 */
>>>>>>>> +	if (key->flags & (1 << KEY_FLAG_BUILTIN))
>>>>>>>> +		goto out;
>>>>>>>> +
>>>>>>>> +#ifdef CONFIG_SYSTEM_BLACKLIST_AUTH_UPDATE
>>>>>>>> +	/*
>>>>>>>> +	 * Verifies the description's PKCS#7 signature against the builtin
>>>>>>>> +	 * trusted keyring.
>>>>>>>> +	 */
>>>>>>>> +	err = verify_pkcs7_signature(key->description,
>>>>>>>> +			strlen(key->description), prep->data, prep->datalen,
>>>>>>>> +			NULL, VERIFYING_UNSPECIFIED_SIGNATURE, NULL, NULL);
>>>>>>>> +	if (err)
>>>>>>>> +		return err;
>>>>>>>> +#else
>>>>>>>> +	/*
>>>>>>>> +	 * It should not be possible to come here because the keyring doesn't
>>>>>>>> +	 * have KEY_USR_WRITE and the only other way to call this function is
>>>>>>>> +	 * for builtin hashes.
>>>>>>>> +	 */
>>>>>>>> +	WARN_ON_ONCE(1);
>>>>>>>> +	return -EPERM;
>>>>>>>> +#endif
>>>>>>>> +
>>>>>>>> +out:
>>>>>>>> +	return generic_key_instantiate(key, prep);
>>>>>>>>      }
>>>>>>>> -static void blacklist_free_preparse(struct key_preparsed_payload *prep)
>>>>>>>> +static int blacklist_key_update(struct key *key,
>>>>>>>> +		struct key_preparsed_payload *prep)
>>>>>>>>      {
>>>>>>>> +	return -EPERM;
>>>>>>>>      }
>>>>>>>>      static void blacklist_describe(const struct key *key, struct seq_file *m)
>>>>>>>> @@ -103,9 +139,8 @@ static void blacklist_describe(const struct key *key, struct seq_file *m)
>>>>>>>>      static struct key_type key_type_blacklist = {
>>>>>>>>      	.name			= "blacklist",
>>>>>>>>      	.vet_description	= blacklist_vet_description,
>>>>>>>> -	.preparse		= blacklist_preparse,
>>>>>>>> -	.free_preparse		= blacklist_free_preparse,
>>>>>>>> -	.instantiate		= generic_key_instantiate,
>>>>>>>> +	.instantiate		= blacklist_key_instantiate,
>>>>>>>> +	.update			= blacklist_key_update,
>>>>>>>>      	.describe		= blacklist_describe,
>>>>>>>>      };
>>>>>>>> @@ -154,8 +189,7 @@ static int mark_raw_hash_blacklisted(const char *hash)
>>>>>>>>      				   hash,
>>>>>>>>      				   NULL,
>>>>>>>>      				   0,
>>>>>>>> -				   ((KEY_POS_ALL & ~KEY_POS_SETATTR) |
>>>>>>>> -				    KEY_USR_VIEW),
>>>>>>>> +				   BLACKLIST_KEY_PERM,
>>>>>>>>      				   KEY_ALLOC_NOT_IN_QUOTA |
>>>>>>>>      				   KEY_ALLOC_BUILT_IN);
>>>>>>>>      	if (IS_ERR(key)) {
>>>>>>>> @@ -232,8 +266,10 @@ int add_key_to_revocation_list(const char *data, size_t size)
>>>>>>>>      				   NULL,
>>>>>>>>      				   data,
>>>>>>>>      				   size,
>>>>>>>> -				   ((KEY_POS_ALL & ~KEY_POS_SETATTR) | KEY_USR_VIEW),
>>>>>>>> -				   KEY_ALLOC_NOT_IN_QUOTA | KEY_ALLOC_BUILT_IN);
>>>>>>>> +				   KEY_POS_VIEW | KEY_POS_READ | KEY_POS_SEARCH
>>>>>>>> +				   | KEY_USR_VIEW,
>>>>>>>> +				   KEY_ALLOC_NOT_IN_QUOTA | KEY_ALLOC_BUILT_IN
>>>>>>>> +				   | KEY_ALLOC_BYPASS_RESTRICTION);
>>>>>>>>      	if (IS_ERR(key)) {
>>>>>>>>      		pr_err("Problem with revocation key (%ld)\n", PTR_ERR(key));
>>>>>>>> @@ -260,25 +296,43 @@ int is_key_on_revocation_list(struct pkcs7_message *pkcs7)
>>>>>>>>      }
>>>>>>>>      #endif
>>>>>>>> +static int restrict_link_for_blacklist(struct key *dest_keyring,
>>>>>>>> +		const struct key_type *type, const union key_payload *payload,
>>>>>>>> +		struct key *restrict_key)
>>>>>>>> +{
>>>>>>>> +	if (type == &key_type_blacklist)
>>>>>>>> +		return 0;
>>>>>>>> +	return -EOPNOTSUPP;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>>      /*
>>>>>>>>       * Initialise the blacklist
>>>>>>>>       */
>>>>>>>>      static int __init blacklist_init(void)
>>>>>>>>      {
>>>>>>>>      	const char *const *bl;
>>>>>>>> +	struct key_restriction *restriction;
>>>>>>>>      	if (register_key_type(&key_type_blacklist) < 0)
>>>>>>>>      		panic("Can't allocate system blacklist key type\n");
>>>>>>>> +	restriction = kzalloc(sizeof(*restriction), GFP_KERNEL);
>>>>>>>> +	if (!restriction)
>>>>>>>> +		panic("Can't allocate blacklist keyring restriction\n");
>>>>>>>
>>>>>>>
>>>>>>> This prevents me from taking this to my pull request. In moderns standards,
>>>>>>> no new BUG_ON(), panic() etc. should never added to the kernel.
>>>>>>>
>>>>>>> I missed this in my review.
>>>>>>>
>>>>>>> This should rather be e.g.
>>>>>>>
>>>>>>>             restriction = kzalloc(sizeof(*restriction), GFP_KERNEL);
>>>>>>> 	if (!restriction) {
>>>>>>> 		pr_err("Can't allocate blacklist keyring restriction\n");
>>>>>>>                     return 0;
>>>>>>>             }
>>>>>>>
>>>>>>> Unfortunately I need to drop this patch set, because adding new panic()
>>>>>>> is simply a no-go.
>>>>>>
>>>>>> I agree that panic() is not great in general, but I followed the other part
>>>>>> of the code (just above) that do the same. This part of the kernel should
>>>>>> failed if critical memory allocation failed at boot time (only). It doesn't
>>>>>> impact the kernel once it is running. I don't think that just ignoring this
>>>>>> error with return 0 is fine, after all it's a critical error right?
>>>>>
>>>>> It's not good reason enough to crash the whole kernel, even if it is a
>>>>> critical error (e.g. run-time foresincs). Even WARN() is not recommended
>>>>> these days [*].
>>>>
>>>> I think that what Greg said in this email is that WARN*() should only be
>>>> used for cases that should never happen, it is definitely not deprecated,
>>>> but WARN_ON_ONCE() may be a better idea though. WARN*() helps detect such
>>>> thought-to-be-impossible cases, that can happen e.g. with code refactoring.
>>>>
>>>> A lot of initialization/boot code (e.g. without user space nor external
>>>> interactions, mostly __init functions) do panic if there is unexpected and
>>>> unrecoverable errors like failed memory allocations. I think handling such
>>>> errors otherwise would be more complex for no benefit. Moreover, delegating
>>>> such error handling to user space could create new (silent) issues.
>>>
>>> To crash the whole kernel, you should be able to clearly explain why it
>>> makes sense in the situation.
>>
>> If there is no enough memory to allocate 24 bytes (struct key_restriction)
>> during early boot, I really doubt the kernel can do much.
>>
>>>
>>>>>
>>>>> For the existing panic()-statements: I'm happy to review patches that
>>>>> render them out. >
>>>>> Not sure tho, if this fails should it be then "everything blacklisted".
>>>>> Just one thing to consider.
>>>>
>>>> Well, if it fail it will be "nothing will work afterwards". Do you have a
>>>> working and useful scenario for this kind of error?
>>>
>>> So you have zero chances to get a shell without blacklist just to do
>>> kernel forensics?
>>
>> Right, I don't think the kernel can launch any process (nor continue early
>> boot) if it cannot allocate 24 bytes.
> 
> initcall is just wrong layer to choose to crash the kernel. It should be
> either do_initcall_level() or do_one_initcall() that should care about
> this (or not care). You can print error message and return -ENODEV;

Ok, I'll do that. Is it OK if I send you a patch fixing all panic calls 
from blacklist_init() and system_trusted_keyring_init() to apply after 
this series (with the panic call)?

Re: [PATCH v8 5/5] certs: Allow root user to append signed hashes to the blacklist keyring

[Jarkko Sakkinen]

On Fri, 2022-03-11 at 17:36 +0100, Mickaël Salaün wrote:
> 
> On 10/03/2022 00:11, Jarkko Sakkinen wrote:
> > On Wed, Mar 09, 2022 at 07:36:50PM +0100, Mickaël Salaün wrote:
> > > 
> > > On 09/03/2022 17:01, Jarkko Sakkinen wrote:
> > > > On Tue, Mar 08, 2022 at 05:02:23PM +0100, Mickaël Salaün wrote:
> > > > > 
> > > > > On 08/03/2022 14:19, Jarkko Sakkinen wrote:
> > > > > > On Tue, Mar 08, 2022 at 01:18:28PM +0100, Mickaël Salaün wrote:
> > > > > > > 
> > > > > > > On 08/03/2022 12:53, Jarkko Sakkinen wrote:
> > > > > > > > On Mon, Jul 12, 2021 at 07:03:13PM +0200, Mickaël Salaün wrote:
> > > > > > > > > From: Mickaël Salaün <mic@linux.microsoft.com>
> > > > > > > > > 
> > > > > > > > > Add a kernel option SYSTEM_BLACKLIST_AUTH_UPDATE to enable the root user
> > > > > > > > > to dynamically add new keys to the blacklist keyring.  This enables to
> > > > > > > > > invalidate new certificates, either from being loaded in a keyring, or
> > > > > > > > > from being trusted in a PKCS#7 certificate chain.  This also enables to
> > > > > > > > > add new file hashes to be denied by the integrity infrastructure.
> > > > > > > > > 
> > > > > > > > > Being able to untrust a certificate which could have normaly been
> > > > > > > > > trusted is a sensitive operation.  This is why adding new hashes to the
> > > > > > > > > blacklist keyring is only allowed when these hashes are signed and
> > > > > > > > > vouched by the builtin trusted keyring.  A blacklist hash is stored as a
> > > > > > > > > key description.  The PKCS#7 signature of this description must be
> > > > > > > > > provided as the key payload.
> > > > > > > > > 
> > > > > > > > > Marking a certificate as untrusted should be enforced while the system
> > > > > > > > > is running.  It is then forbiden to remove such blacklist keys.
> > > > > > > > > 
> > > > > > > > > Update blacklist keyring, blacklist key and revoked certificate access rights:
> > > > > > > > > * allows the root user to search for a specific blacklisted hash, which
> > > > > > > > >       make sense because the descriptions are already viewable;
> > > > > > > > > * forbids key update (blacklist and asymmetric ones);
> > > > > > > > > * restricts kernel rights on the blacklist keyring to align with the
> > > > > > > > >       root user rights.
> > > > > > > > > 
> > > > > > > > > See help in tools/certs/print-cert-tbs-hash.sh .
> > > > > > > > > 
> > > > > > > > > Cc: David Howells <dhowells@redhat.com>
> > > > > > > > > Cc: David Woodhouse <dwmw2@infradead.org>
> > > > > > > > > Cc: Eric Snowberg <eric.snowberg@oracle.com>
> > > > > > > > > Cc: Jarkko Sakkinen <jarkko@kernel.org>
> > > > > > > > > Signed-off-by: Mickaël Salaün <mic@linux.microsoft.com>
> > > > > > > > > Link: https://lore.kernel.org/r/20210712170313.884724-6-mic@digikod.net
> > > > > > > > > ---
> > > > > > > > > 
> > > > > > > > > Changes since v6:
> > > > > > > > > * Rebase on keys-cve-2020-26541-v3: commit ebd9c2ae369a ("integrity:
> > > > > > > > >       Load mokx variables into the blacklist keyring").
> > > > > > > > > 
> > > > > > > > > Changes since v5:
> > > > > > > > > * Rebase on keys-next, fix Kconfig conflict, and update the asymmetric
> > > > > > > > >       key rights added to the blacklist keyring by the new
> > > > > > > > >       add_key_to_revocation_list(): align with blacklist key rights by
> > > > > > > > >       removing KEY_POS_WRITE as a safeguard, and add
> > > > > > > > >       KEY_ALLOC_BYPASS_RESTRICTION to not be subject to
> > > > > > > > >       restrict_link_for_blacklist() that only allows blacklist key types to
> > > > > > > > >       be added to the keyring.
> > > > > > > > > * Change the return code for restrict_link_for_blacklist() from -EPERM
> > > > > > > > >       to -EOPNOTSUPP to align with asymmetric key keyrings.
> > > > > > > > > 
> > > > > > > > > Changes since v3:
> > > > > > > > > * Update commit message for print-cert-tbs-hash.sh .
> > > > > > > > > 
> > > > > > > > > Changes since v2:
> > > > > > > > > * Add comment for blacklist_key_instantiate().
> > > > > > > > > ---
> > > > > > > > >      certs/Kconfig     | 10 +++++
> > > > > > > > >      certs/blacklist.c | 96 ++++++++++++++++++++++++++++++++++++-----------
> > > > > > > > >      2 files changed, 85 insertions(+), 21 deletions(-)
> > > > > > > > > 
> > > > > > > > > diff --git a/certs/Kconfig b/certs/Kconfig
> > > > > > > > > index 0fbe184ceca5..e0e524b7eff9 100644
> > > > > > > > > --- a/certs/Kconfig
> > > > > > > > > +++ b/certs/Kconfig
> > > > > > > > > @@ -103,4 +103,14 @@ config SYSTEM_REVOCATION_KEYS
> > > > > > > > >           containing X.509 certificates to be included in the default blacklist
> > > > > > > > >           keyring.
> > > > > > > > > +config SYSTEM_BLACKLIST_AUTH_UPDATE
> > > > > > > > > +       bool "Allow root to add signed blacklist keys"
> > > > > > > > > +       depends on SYSTEM_BLACKLIST_KEYRING
> > > > > > > > > +       depends on SYSTEM_DATA_VERIFICATION
> > > > > > > > > +       help
> > > > > > > > > +         If set, provide the ability to load new blacklist keys at run time if
> > > > > > > > > +         they are signed and vouched by a certificate from the builtin trusted
> > > > > > > > > +         keyring.  The PKCS#7 signature of the description is set in the key
> > > > > > > > > +         payload.  Blacklist keys cannot be removed.
> > > > > > > > > +
> > > > > > > > >      endmenu
> > > > > > > > > diff --git a/certs/blacklist.c b/certs/blacklist.c
> > > > > > > > > index b254c87ceb3a..486ce0dd8e9c 100644
> > > > > > > > > --- a/certs/blacklist.c
> > > > > > > > > +++ b/certs/blacklist.c
> > > > > > > > > @@ -15,6 +15,7 @@
> > > > > > > > >      #include <linux/err.h>
> > > > > > > > >      #include <linux/seq_file.h>
> > > > > > > > >      #include <linux/uidgid.h>
> > > > > > > > > +#include <linux/verification.h>
> > > > > > > > >      #include <keys/system_keyring.h>
> > > > > > > > >      #include "blacklist.h"
> > > > > > > > >      #include "common.h"
> > > > > > > > > @@ -26,6 +27,9 @@
> > > > > > > > >       */
> > > > > > > > >      #define MAX_HASH_LEN       128
> > > > > > > > > +#define BLACKLIST_KEY_PERM (KEY_POS_SEARCH | KEY_POS_VIEW | \
> > > > > > > > > +                           KEY_USR_SEARCH | KEY_USR_VIEW)
> > > > > > > > > +
> > > > > > > > >      static const char tbs_prefix[] = "tbs";
> > > > > > > > >      static const char bin_prefix[] = "bin";
> > > > > > > > > @@ -80,19 +84,51 @@ static int blacklist_vet_description(const char *desc)
> > > > > > > > >         return 0;
> > > > > > > > >      }
> > > > > > > > > -/*
> > > > > > > > > - * The hash to be blacklisted is expected to be in the description.  There will
> > > > > > > > > - * be no payload.
> > > > > > > > > - */
> > > > > > > > > -static int blacklist_preparse(struct key_preparsed_payload *prep)
> > > > > > > > > +static int blacklist_key_instantiate(struct key *key,
> > > > > > > > > +               struct key_preparsed_payload *prep)
> > > > > > > > >      {
> > > > > > > > > -       if (prep->datalen > 0)
> > > > > > > > > -               return -EINVAL;
> > > > > > > > > -       return 0;
> > > > > > > > > +#ifdef CONFIG_SYSTEM_BLACKLIST_AUTH_UPDATE
> > > > > > > > > +       int err;
> > > > > > > > > +#endif
> > > > > > > > > +
> > > > > > > > > +       /* Sets safe default permissions for keys loaded by user space. */
> > > > > > > > > +       key->perm = BLACKLIST_KEY_PERM;
> > > > > > > > > +
> > > > > > > > > +       /*
> > > > > > > > > +        * Skips the authentication step for builtin hashes, they are not
> > > > > > > > > +        * signed but still trusted.
> > > > > > > > > +        */
> > > > > > > > > +       if (key->flags & (1 << KEY_FLAG_BUILTIN))
> > > > > > > > > +               goto out;
> > > > > > > > > +
> > > > > > > > > +#ifdef CONFIG_SYSTEM_BLACKLIST_AUTH_UPDATE
> > > > > > > > > +       /*
> > > > > > > > > +        * Verifies the description's PKCS#7 signature against the builtin
> > > > > > > > > +        * trusted keyring.
> > > > > > > > > +        */
> > > > > > > > > +       err = verify_pkcs7_signature(key->description,
> > > > > > > > > +                       strlen(key->description), prep->data, prep->datalen,
> > > > > > > > > +                       NULL, VERIFYING_UNSPECIFIED_SIGNATURE, NULL, NULL);
> > > > > > > > > +       if (err)
> > > > > > > > > +               return err;
> > > > > > > > > +#else
> > > > > > > > > +       /*
> > > > > > > > > +        * It should not be possible to come here because the keyring doesn't
> > > > > > > > > +        * have KEY_USR_WRITE and the only other way to call this function is
> > > > > > > > > +        * for builtin hashes.
> > > > > > > > > +        */
> > > > > > > > > +       WARN_ON_ONCE(1);
> > > > > > > > > +       return -EPERM;
> > > > > > > > > +#endif
> > > > > > > > > +
> > > > > > > > > +out:
> > > > > > > > > +       return generic_key_instantiate(key, prep);
> > > > > > > > >      }
> > > > > > > > > -static void blacklist_free_preparse(struct key_preparsed_payload *prep)
> > > > > > > > > +static int blacklist_key_update(struct key *key,
> > > > > > > > > +               struct key_preparsed_payload *prep)
> > > > > > > > >      {
> > > > > > > > > +       return -EPERM;
> > > > > > > > >      }
> > > > > > > > >      static void blacklist_describe(const struct key *key, struct seq_file *m)
> > > > > > > > > @@ -103,9 +139,8 @@ static void blacklist_describe(const struct key *key, struct seq_file *m)
> > > > > > > > >      static struct key_type key_type_blacklist = {
> > > > > > > > >         .name                   = "blacklist",
> > > > > > > > >         .vet_description        = blacklist_vet_description,
> > > > > > > > > -       .preparse               = blacklist_preparse,
> > > > > > > > > -       .free_preparse          = blacklist_free_preparse,
> > > > > > > > > -       .instantiate            = generic_key_instantiate,
> > > > > > > > > +       .instantiate            = blacklist_key_instantiate,
> > > > > > > > > +       .update                 = blacklist_key_update,
> > > > > > > > >         .describe               = blacklist_describe,
> > > > > > > > >      };
> > > > > > > > > @@ -154,8 +189,7 @@ static int mark_raw_hash_blacklisted(const char *hash)
> > > > > > > > >                                    hash,
> > > > > > > > >                                    NULL,
> > > > > > > > >                                    0,
> > > > > > > > > -                                  ((KEY_POS_ALL & ~KEY_POS_SETATTR) |
> > > > > > > > > -                                   KEY_USR_VIEW),
> > > > > > > > > +                                  BLACKLIST_KEY_PERM,
> > > > > > > > >                                    KEY_ALLOC_NOT_IN_QUOTA |
> > > > > > > > >                                    KEY_ALLOC_BUILT_IN);
> > > > > > > > >         if (IS_ERR(key)) {
> > > > > > > > > @@ -232,8 +266,10 @@ int add_key_to_revocation_list(const char *data, size_t size)
> > > > > > > > >                                    NULL,
> > > > > > > > >                                    data,
> > > > > > > > >                                    size,
> > > > > > > > > -                                  ((KEY_POS_ALL & ~KEY_POS_SETATTR) | KEY_USR_VIEW),
> > > > > > > > > -                                  KEY_ALLOC_NOT_IN_QUOTA | KEY_ALLOC_BUILT_IN);
> > > > > > > > > +                                  KEY_POS_VIEW | KEY_POS_READ | KEY_POS_SEARCH
> > > > > > > > > +                                  | KEY_USR_VIEW,
> > > > > > > > > +                                  KEY_ALLOC_NOT_IN_QUOTA | KEY_ALLOC_BUILT_IN
> > > > > > > > > +                                  | KEY_ALLOC_BYPASS_RESTRICTION);
> > > > > > > > >         if (IS_ERR(key)) {
> > > > > > > > >                 pr_err("Problem with revocation key (%ld)\n", PTR_ERR(key));
> > > > > > > > > @@ -260,25 +296,43 @@ int is_key_on_revocation_list(struct pkcs7_message *pkcs7)
> > > > > > > > >      }
> > > > > > > > >      #endif
> > > > > > > > > +static int restrict_link_for_blacklist(struct key *dest_keyring,
> > > > > > > > > +               const struct key_type *type, const union key_payload *payload,
> > > > > > > > > +               struct key *restrict_key)
> > > > > > > > > +{
> > > > > > > > > +       if (type == &key_type_blacklist)
> > > > > > > > > +               return 0;
> > > > > > > > > +       return -EOPNOTSUPP;
> > > > > > > > > +}
> > > > > > > > > +
> > > > > > > > >      /*
> > > > > > > > >       * Initialise the blacklist
> > > > > > > > >       */
> > > > > > > > >      static int __init blacklist_init(void)
> > > > > > > > >      {
> > > > > > > > >         const char *const *bl;
> > > > > > > > > +       struct key_restriction *restriction;
> > > > > > > > >         if (register_key_type(&key_type_blacklist) < 0)
> > > > > > > > >                 panic("Can't allocate system blacklist key type\n");
> > > > > > > > > +       restriction = kzalloc(sizeof(*restriction), GFP_KERNEL);
> > > > > > > > > +       if (!restriction)
> > > > > > > > > +               panic("Can't allocate blacklist keyring restriction\n");
> > > > > > > > 
> > > > > > > > 
> > > > > > > > This prevents me from taking this to my pull request. In moderns standards,
> > > > > > > > no new BUG_ON(), panic() etc. should never added to the kernel.
> > > > > > > > 
> > > > > > > > I missed this in my review.
> > > > > > > > 
> > > > > > > > This should rather be e.g.
> > > > > > > > 
> > > > > > > >             restriction = kzalloc(sizeof(*restriction), GFP_KERNEL);
> > > > > > > >         if (!restriction) {
> > > > > > > >                 pr_err("Can't allocate blacklist keyring restriction\n");
> > > > > > > >                     return 0;
> > > > > > > >             }
> > > > > > > > 
> > > > > > > > Unfortunately I need to drop this patch set, because adding new panic()
> > > > > > > > is simply a no-go.
> > > > > > > 
> > > > > > > I agree that panic() is not great in general, but I followed the other part
> > > > > > > of the code (just above) that do the same. This part of the kernel should
> > > > > > > failed if critical memory allocation failed at boot time (only). It doesn't
> > > > > > > impact the kernel once it is running. I don't think that just ignoring this
> > > > > > > error with return 0 is fine, after all it's a critical error right?
> > > > > > 
> > > > > > It's not good reason enough to crash the whole kernel, even if it is a
> > > > > > critical error (e.g. run-time foresincs). Even WARN() is not recommended
> > > > > > these days [*].
> > > > > 
> > > > > I think that what Greg said in this email is that WARN*() should only be
> > > > > used for cases that should never happen, it is definitely not deprecated,
> > > > > but WARN_ON_ONCE() may be a better idea though. WARN*() helps detect such
> > > > > thought-to-be-impossible cases, that can happen e.g. with code refactoring.
> > > > > 
> > > > > A lot of initialization/boot code (e.g. without user space nor external
> > > > > interactions, mostly __init functions) do panic if there is unexpected and
> > > > > unrecoverable errors like failed memory allocations. I think handling such
> > > > > errors otherwise would be more complex for no benefit. Moreover, delegating
> > > > > such error handling to user space could create new (silent) issues.
> > > > 
> > > > To crash the whole kernel, you should be able to clearly explain why it
> > > > makes sense in the situation.
> > > 
> > > If there is no enough memory to allocate 24 bytes (struct key_restriction)
> > > during early boot, I really doubt the kernel can do much.
> > > 
> > > > 
> > > > > > 
> > > > > > For the existing panic()-statements: I'm happy to review patches that
> > > > > > render them out. >
> > > > > > Not sure tho, if this fails should it be then "everything blacklisted".
> > > > > > Just one thing to consider.
> > > > > 
> > > > > Well, if it fail it will be "nothing will work afterwards". Do you have a
> > > > > working and useful scenario for this kind of error?
> > > > 
> > > > So you have zero chances to get a shell without blacklist just to do
> > > > kernel forensics?
> > > 
> > > Right, I don't think the kernel can launch any process (nor continue early
> > > boot) if it cannot allocate 24 bytes.
> > 
> > initcall is just wrong layer to choose to crash the kernel. It should be
> > either do_initcall_level() or do_one_initcall() that should care about
> > this (or not care). You can print error message and return -ENODEV;
> 
> Ok, I'll do that. Is it OK if I send you a patch fixing all panic calls 
> from blacklist_init() and system_trusted_keyring_init() to apply after 
> this series (with the panic call)?

Yes, and I still have not sent my PR, i.e. can include this if you
can provide me patches soonish (include that to the same series).

In the end of the day it comes to this: it's not a void function.
ATM the framework above does not do much at all but it is IMHO
going beyond the abstraction, which we should not do.

BR, Jarkko

Re: [PATCH v8 5/5] certs: Allow root user to append signed hashes to the blacklist keyring

[David Howells]

Jarkko Sakkinen <jarkko@kernel.org> wrote:

> >  /*
> >   * Initialise the blacklist
> >   */
> >  static int __init blacklist_init(void)
> >  {
> >  	const char *const *bl;
> > +	struct key_restriction *restriction;
> >  
> >  	if (register_key_type(&key_type_blacklist) < 0)
> >  		panic("Can't allocate system blacklist key type\n");
> >  
> > +	restriction = kzalloc(sizeof(*restriction), GFP_KERNEL);
> > +	if (!restriction)
> > +		panic("Can't allocate blacklist keyring restriction\n");
> 
> 
> This prevents me from taking this to my pull request. In moderns standards,
> no new BUG_ON(), panic() etc. should never added to the kernel.

I would argue that in this case, though, it is reasonable.  This should only
be called during kernel initialisation and, as Mickaël points out, if you
can't allocate that small amount of memory, the kernel isn't going to boot
much further.

> I missed this in my review.
> 
> This should rather be e.g.
> 
>         restriction = kzalloc(sizeof(*restriction), GFP_KERNEL);
> 	if (!restriction) {
> 		pr_err("Can't allocate blacklist keyring restriction\n");
>                 return 0;
>         }

You can't just return 0.  That indicates success - but if by some miracle, the
kernel actually gets to a point where userspace can happen, it could mean that
we're missing the security restrictions of the blacklist.

Now, we could defer the panic to add_key_to_revocation_list(), but if you
can't set in place the required security restrictions, I think it's arguable
that the kernel either needs to panic or it needs to blacklist everything.

David