linux-integrity.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] KEYS: trusted: fix use as module when CONFIG_TCG_TPM=m
@ 2021-07-21 16:02 Ahmad Fatoum
  2021-07-21 20:17 ` Andreas Rammhold
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Ahmad Fatoum @ 2021-07-21 16:02 UTC (permalink / raw)
  To: Jarkko Sakkinen, James Morris, Serge E. Hallyn, James Bottomley,
	Mimi Zohar, Sumit Garg, David Howells, Herbert Xu,
	David S. Miller
  Cc: kernel, Andreas Rammhold, Ahmad Fatoum, David Gstir,
	Richard Weinberger, keyrings, linux-crypto, linux-kernel,
	linux-security-module, linux-integrity

Since commit 5d0682be3189 ("KEYS: trusted: Add generic trusted keys
framework"), trusted.ko built with CONFIG_TCG_TPM=CONFIG_TRUSTED_KEYS=m
will not register the TPM trusted key type at runtime.

This is because, after that rework, CONFIG_DEPENDENCY of the TPM
and TEE backends were checked with #ifdef, but that's only true
when they're built-in.

Fix this by introducing two new boolean Kconfig symbols:
TRUSTED_KEYS_TPM and TRUSTED_KEYS_TEE with the appropriate
dependencies and use them to check which backends are available.

This also has a positive effect on user experience:

 - It's now possible to use TEE trusted keys without CONFIG_TCG_TPM
 - It's now possible to enable CONFIG_TCG_TPM, but exclude TPM from
   available trust sources
 - TEE=m && TRUSTED_KEYS=y no longer leads to TEE support
   being silently dropped

Any code depending on the TPM trusted key backend or symbols exported
by it will now need to explicitly state that it

  depends on TRUSTED_KEYS && TRUSTED_KEYS_TPM

The latter to ensure the dependency is built and the former to ensure
it's reachable for module builds. This currently only affects
CONFIG_ASYMMETRIC_TPM_KEY_SUBTYPE, so it's fixed up here as well.

Reported-by: Andreas Rammhold <andreas@rammhold.de>
Fixes: 5d0682be3189 ("KEYS: trusted: Add generic trusted keys framework")
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---

(Implicit) v1 was as a preparatory patch for CAAM trusted keys[1] with the
goal of fixing the Kconfig inflexibility after the TEE trusted key rework.

Unbeknownst to me, it also fixes a regression, which was later
reported by Andreas[2] along with a patch.

I split out the fix from the CAAM series and adjusted the commit
message to explain the regression.

v1 -> v2:
  - Move rest of TPM-related selects from TRUSTED_KEYS to
    TRUSTED_KEYS_TPM (Sumit)
  - Remove left-over line in Makefile (Sumit)
  - added Fixes: tag
  - adjust commit message to reference the regression reported
    by Andreas
  - have ASYMMETRIC_TPM_KEY_SUBTYPE depend on TRUSTED_KEYS_TPM,
    because it references global symbols that are exported
    by the trusted key TPM backend.

[1]: https://lore.kernel.org/linux-integrity/f8285eb0135ba30c9d846cf9dd395d1f5f8b1efc.1624364386.git-series.a.fatoum@pengutronix.de/
[2]: https://lore.kernel.org/linux-integrity/20210719091335.vwfebcpkf4pag3wm@wrt/T/#t

To: Jarkko Sakkinen <jarkko@kernel.org>
To: James Morris <jmorris@namei.org>
To: "Serge E. Hallyn" <serge@hallyn.com>
To: James Bottomley <jejb@linux.ibm.com>
To: Mimi Zohar <zohar@linux.ibm.com>
To: Sumit Garg <sumit.garg@linaro.org>
To: David Howells <dhowells@redhat.com>
To: Herbert Xu <herbert@gondor.apana.org.au>
To: "David S. Miller" <davem@davemloft.net>
Cc: David Gstir <david@sigma-star.at>
Cc: Richard Weinberger <richard@nod.at>
Cc: keyrings@vger.kernel.org
Cc: linux-crypto@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: linux-security-module@vger.kernel.org
Cc: linux-integrity@vger.kernel.org
---
 crypto/asymmetric_keys/Kconfig            |  2 +-
 security/keys/Kconfig                     | 18 ++++++--------
 security/keys/trusted-keys/Kconfig        | 29 +++++++++++++++++++++++
 security/keys/trusted-keys/Makefile       |  8 +++----
 security/keys/trusted-keys/trusted_core.c |  4 ++--
 5 files changed, 43 insertions(+), 18 deletions(-)
 create mode 100644 security/keys/trusted-keys/Kconfig

diff --git a/crypto/asymmetric_keys/Kconfig b/crypto/asymmetric_keys/Kconfig
index 1f1f004dc757..8886eddbf881 100644
--- a/crypto/asymmetric_keys/Kconfig
+++ b/crypto/asymmetric_keys/Kconfig
@@ -25,7 +25,7 @@ config ASYMMETRIC_PUBLIC_KEY_SUBTYPE
 config ASYMMETRIC_TPM_KEY_SUBTYPE
 	tristate "Asymmetric TPM backed private key subtype"
 	depends on TCG_TPM
-	depends on TRUSTED_KEYS
+	depends on TRUSTED_KEYS && TRUSTED_KEYS_TPM
 	select CRYPTO_HMAC
 	select CRYPTO_SHA1
 	select CRYPTO_HASH_INFO
diff --git a/security/keys/Kconfig b/security/keys/Kconfig
index 64b81abd087e..9ec302962fe2 100644
--- a/security/keys/Kconfig
+++ b/security/keys/Kconfig
@@ -70,23 +70,19 @@ config BIG_KEYS
 
 config TRUSTED_KEYS
 	tristate "TRUSTED KEYS"
-	depends on KEYS && TCG_TPM
-	select CRYPTO
-	select CRYPTO_HMAC
-	select CRYPTO_SHA1
-	select CRYPTO_HASH_INFO
-	select ASN1_ENCODER
-	select OID_REGISTRY
-	select ASN1
+	depends on KEYS
 	help
 	  This option provides support for creating, sealing, and unsealing
 	  keys in the kernel. Trusted keys are random number symmetric keys,
-	  generated and RSA-sealed by the TPM. The TPM only unseals the keys,
-	  if the boot PCRs and other criteria match.  Userspace will only ever
-	  see encrypted blobs.
+	  generated and sealed by a trust source selected at kernel boot-time.
+	  Userspace will only ever see encrypted blobs.
 
 	  If you are unsure as to whether this is required, answer N.
 
+if TRUSTED_KEYS
+source "security/keys/trusted-keys/Kconfig"
+endif
+
 config ENCRYPTED_KEYS
 	tristate "ENCRYPTED KEYS"
 	depends on KEYS
diff --git a/security/keys/trusted-keys/Kconfig b/security/keys/trusted-keys/Kconfig
new file mode 100644
index 000000000000..c163cfeedff6
--- /dev/null
+++ b/security/keys/trusted-keys/Kconfig
@@ -0,0 +1,29 @@
+config TRUSTED_KEYS_TPM
+	bool "TPM-based trusted keys"
+	depends on TCG_TPM >= TRUSTED_KEYS
+	default y
+	select CRYPTO
+	select CRYPTO_HMAC
+	select CRYPTO_SHA1
+	select CRYPTO_HASH_INFO
+	select ASN1_ENCODER
+	select OID_REGISTRY
+	select ASN1
+	help
+	  Enable use of the Trusted Platform Module (TPM) as trusted key
+	  backend. Trusted keys are are random number symmetric keys,
+	  which will be generated and RSA-sealed by the TPM.
+	  The TPM only unseals the keys, if the boot PCRs and other
+	  criteria match.
+
+config TRUSTED_KEYS_TEE
+	bool "TEE-based trusted keys"
+	depends on TEE >= TRUSTED_KEYS
+	default y
+	help
+	  Enable use of the Trusted Execution Environment (TEE) as trusted
+	  key backend.
+
+if !TRUSTED_KEYS_TPM && !TRUSTED_KEYS_TEE
+comment "No trust source selected!"
+endif
diff --git a/security/keys/trusted-keys/Makefile b/security/keys/trusted-keys/Makefile
index feb8b6c3cc79..2e2371eae4d5 100644
--- a/security/keys/trusted-keys/Makefile
+++ b/security/keys/trusted-keys/Makefile
@@ -5,10 +5,10 @@
 
 obj-$(CONFIG_TRUSTED_KEYS) += trusted.o
 trusted-y += trusted_core.o
-trusted-y += trusted_tpm1.o
+trusted-$(CONFIG_TRUSTED_KEYS_TPM) += trusted_tpm1.o
 
 $(obj)/trusted_tpm2.o: $(obj)/tpm2key.asn1.h
-trusted-y += trusted_tpm2.o
-trusted-y += tpm2key.asn1.o
+trusted-$(CONFIG_TRUSTED_KEYS_TPM) += trusted_tpm2.o
+trusted-$(CONFIG_TRUSTED_KEYS_TPM) += tpm2key.asn1.o
 
-trusted-$(CONFIG_TEE) += trusted_tee.o
+trusted-$(CONFIG_TRUSTED_KEYS_TEE) += trusted_tee.o
diff --git a/security/keys/trusted-keys/trusted_core.c b/security/keys/trusted-keys/trusted_core.c
index d5c891d8d353..8cab69e5d0da 100644
--- a/security/keys/trusted-keys/trusted_core.c
+++ b/security/keys/trusted-keys/trusted_core.c
@@ -27,10 +27,10 @@ module_param_named(source, trusted_key_source, charp, 0);
 MODULE_PARM_DESC(source, "Select trusted keys source (tpm or tee)");
 
 static const struct trusted_key_source trusted_key_sources[] = {
-#if defined(CONFIG_TCG_TPM)
+#if defined(CONFIG_TRUSTED_KEYS_TPM)
 	{ "tpm", &trusted_key_tpm_ops },
 #endif
-#if defined(CONFIG_TEE)
+#if defined(CONFIG_TRUSTED_KEYS_TEE)
 	{ "tee", &trusted_key_tee_ops },
 #endif
 };
-- 
2.30.2


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] KEYS: trusted: fix use as module when CONFIG_TCG_TPM=m
  2021-07-21 16:02 [PATCH v2] KEYS: trusted: fix use as module when CONFIG_TCG_TPM=m Ahmad Fatoum
@ 2021-07-21 20:17 ` Andreas Rammhold
  2021-07-22  4:46 ` Sumit Garg
  2021-07-27  3:04 ` Jarkko Sakkinen
  2 siblings, 0 replies; 9+ messages in thread
From: Andreas Rammhold @ 2021-07-21 20:17 UTC (permalink / raw)
  To: Ahmad Fatoum
  Cc: Jarkko Sakkinen, James Morris, Serge E. Hallyn, James Bottomley,
	Mimi Zohar, Sumit Garg, David Howells, Herbert Xu,
	David S. Miller, kernel, Andreas Rammhold, David Gstir,
	Richard Weinberger, keyrings, linux-crypto, linux-kernel,
	linux-security-module, linux-integrity

On 18:02 21.07.21, Ahmad Fatoum wrote:
> Since commit 5d0682be3189 ("KEYS: trusted: Add generic trusted keys
> framework"), trusted.ko built with CONFIG_TCG_TPM=CONFIG_TRUSTED_KEYS=m
> will not register the TPM trusted key type at runtime.
> 
> This is because, after that rework, CONFIG_DEPENDENCY of the TPM
> and TEE backends were checked with #ifdef, but that's only true
> when they're built-in.
> 
> Fix this by introducing two new boolean Kconfig symbols:
> TRUSTED_KEYS_TPM and TRUSTED_KEYS_TEE with the appropriate
> dependencies and use them to check which backends are available.
> 
> This also has a positive effect on user experience:
> 
>  - It's now possible to use TEE trusted keys without CONFIG_TCG_TPM
>  - It's now possible to enable CONFIG_TCG_TPM, but exclude TPM from
>    available trust sources
>  - TEE=m && TRUSTED_KEYS=y no longer leads to TEE support
>    being silently dropped
> 
> Any code depending on the TPM trusted key backend or symbols exported
> by it will now need to explicitly state that it
> 
>   depends on TRUSTED_KEYS && TRUSTED_KEYS_TPM
> 
> The latter to ensure the dependency is built and the former to ensure
> it's reachable for module builds. This currently only affects
> CONFIG_ASYMMETRIC_TPM_KEY_SUBTYPE, so it's fixed up here as well.
> 
> Reported-by: Andreas Rammhold <andreas@rammhold.de>
> Fixes: 5d0682be3189 ("KEYS: trusted: Add generic trusted keys framework")
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
> 
> (Implicit) v1 was as a preparatory patch for CAAM trusted keys[1] with the
> goal of fixing the Kconfig inflexibility after the TEE trusted key rework.
> 
> Unbeknownst to me, it also fixes a regression, which was later
> reported by Andreas[2] along with a patch.
> 
> I split out the fix from the CAAM series and adjusted the commit
> message to explain the regression.
> 
> v1 -> v2:
>   - Move rest of TPM-related selects from TRUSTED_KEYS to
>     TRUSTED_KEYS_TPM (Sumit)
>   - Remove left-over line in Makefile (Sumit)
>   - added Fixes: tag
>   - adjust commit message to reference the regression reported
>     by Andreas
>   - have ASYMMETRIC_TPM_KEY_SUBTYPE depend on TRUSTED_KEYS_TPM,
>     because it references global symbols that are exported
>     by the trusted key TPM backend.
> 
> [1]: https://lore.kernel.org/linux-integrity/f8285eb0135ba30c9d846cf9dd395d1f5f8b1efc.1624364386.git-series.a.fatoum@pengutronix.de/
> [2]: https://lore.kernel.org/linux-integrity/20210719091335.vwfebcpkf4pag3wm@wrt/T/#t
> 
> To: Jarkko Sakkinen <jarkko@kernel.org>
> To: James Morris <jmorris@namei.org>
> To: "Serge E. Hallyn" <serge@hallyn.com>
> To: James Bottomley <jejb@linux.ibm.com>
> To: Mimi Zohar <zohar@linux.ibm.com>
> To: Sumit Garg <sumit.garg@linaro.org>
> To: David Howells <dhowells@redhat.com>
> To: Herbert Xu <herbert@gondor.apana.org.au>
> To: "David S. Miller" <davem@davemloft.net>
> Cc: David Gstir <david@sigma-star.at>
> Cc: Richard Weinberger <richard@nod.at>
> Cc: keyrings@vger.kernel.org
> Cc: linux-crypto@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-security-module@vger.kernel.org
> Cc: linux-integrity@vger.kernel.org
> ---
>  crypto/asymmetric_keys/Kconfig            |  2 +-
>  security/keys/Kconfig                     | 18 ++++++--------
>  security/keys/trusted-keys/Kconfig        | 29 +++++++++++++++++++++++
>  security/keys/trusted-keys/Makefile       |  8 +++----
>  security/keys/trusted-keys/trusted_core.c |  4 ++--
>  5 files changed, 43 insertions(+), 18 deletions(-)
>  create mode 100644 security/keys/trusted-keys/Kconfig
> 
> diff --git a/crypto/asymmetric_keys/Kconfig b/crypto/asymmetric_keys/Kconfig
> index 1f1f004dc757..8886eddbf881 100644
> --- a/crypto/asymmetric_keys/Kconfig
> +++ b/crypto/asymmetric_keys/Kconfig
> @@ -25,7 +25,7 @@ config ASYMMETRIC_PUBLIC_KEY_SUBTYPE
>  config ASYMMETRIC_TPM_KEY_SUBTYPE
>  	tristate "Asymmetric TPM backed private key subtype"
>  	depends on TCG_TPM
> -	depends on TRUSTED_KEYS
> +	depends on TRUSTED_KEYS && TRUSTED_KEYS_TPM
>  	select CRYPTO_HMAC
>  	select CRYPTO_SHA1
>  	select CRYPTO_HASH_INFO
> diff --git a/security/keys/Kconfig b/security/keys/Kconfig
> index 64b81abd087e..9ec302962fe2 100644
> --- a/security/keys/Kconfig
> +++ b/security/keys/Kconfig
> @@ -70,23 +70,19 @@ config BIG_KEYS
>  
>  config TRUSTED_KEYS
>  	tristate "TRUSTED KEYS"
> -	depends on KEYS && TCG_TPM
> -	select CRYPTO
> -	select CRYPTO_HMAC
> -	select CRYPTO_SHA1
> -	select CRYPTO_HASH_INFO
> -	select ASN1_ENCODER
> -	select OID_REGISTRY
> -	select ASN1
> +	depends on KEYS
>  	help
>  	  This option provides support for creating, sealing, and unsealing
>  	  keys in the kernel. Trusted keys are random number symmetric keys,
> -	  generated and RSA-sealed by the TPM. The TPM only unseals the keys,
> -	  if the boot PCRs and other criteria match.  Userspace will only ever
> -	  see encrypted blobs.
> +	  generated and sealed by a trust source selected at kernel boot-time.
> +	  Userspace will only ever see encrypted blobs.
>  
>  	  If you are unsure as to whether this is required, answer N.
>  
> +if TRUSTED_KEYS
> +source "security/keys/trusted-keys/Kconfig"
> +endif
> +
>  config ENCRYPTED_KEYS
>  	tristate "ENCRYPTED KEYS"
>  	depends on KEYS
> diff --git a/security/keys/trusted-keys/Kconfig b/security/keys/trusted-keys/Kconfig
> new file mode 100644
> index 000000000000..c163cfeedff6
> --- /dev/null
> +++ b/security/keys/trusted-keys/Kconfig
> @@ -0,0 +1,29 @@
> +config TRUSTED_KEYS_TPM
> +	bool "TPM-based trusted keys"
> +	depends on TCG_TPM >= TRUSTED_KEYS
> +	default y
> +	select CRYPTO
> +	select CRYPTO_HMAC
> +	select CRYPTO_SHA1
> +	select CRYPTO_HASH_INFO
> +	select ASN1_ENCODER
> +	select OID_REGISTRY
> +	select ASN1
> +	help
> +	  Enable use of the Trusted Platform Module (TPM) as trusted key
> +	  backend. Trusted keys are are random number symmetric keys,
> +	  which will be generated and RSA-sealed by the TPM.
> +	  The TPM only unseals the keys, if the boot PCRs and other
> +	  criteria match.
> +
> +config TRUSTED_KEYS_TEE
> +	bool "TEE-based trusted keys"
> +	depends on TEE >= TRUSTED_KEYS
> +	default y
> +	help
> +	  Enable use of the Trusted Execution Environment (TEE) as trusted
> +	  key backend.
> +
> +if !TRUSTED_KEYS_TPM && !TRUSTED_KEYS_TEE
> +comment "No trust source selected!"
> +endif
> diff --git a/security/keys/trusted-keys/Makefile b/security/keys/trusted-keys/Makefile
> index feb8b6c3cc79..2e2371eae4d5 100644
> --- a/security/keys/trusted-keys/Makefile
> +++ b/security/keys/trusted-keys/Makefile
> @@ -5,10 +5,10 @@
>  
>  obj-$(CONFIG_TRUSTED_KEYS) += trusted.o
>  trusted-y += trusted_core.o
> -trusted-y += trusted_tpm1.o
> +trusted-$(CONFIG_TRUSTED_KEYS_TPM) += trusted_tpm1.o
>  
>  $(obj)/trusted_tpm2.o: $(obj)/tpm2key.asn1.h
> -trusted-y += trusted_tpm2.o
> -trusted-y += tpm2key.asn1.o
> +trusted-$(CONFIG_TRUSTED_KEYS_TPM) += trusted_tpm2.o
> +trusted-$(CONFIG_TRUSTED_KEYS_TPM) += tpm2key.asn1.o
>  
> -trusted-$(CONFIG_TEE) += trusted_tee.o
> +trusted-$(CONFIG_TRUSTED_KEYS_TEE) += trusted_tee.o
> diff --git a/security/keys/trusted-keys/trusted_core.c b/security/keys/trusted-keys/trusted_core.c
> index d5c891d8d353..8cab69e5d0da 100644
> --- a/security/keys/trusted-keys/trusted_core.c
> +++ b/security/keys/trusted-keys/trusted_core.c
> @@ -27,10 +27,10 @@ module_param_named(source, trusted_key_source, charp, 0);
>  MODULE_PARM_DESC(source, "Select trusted keys source (tpm or tee)");
>  
>  static const struct trusted_key_source trusted_key_sources[] = {
> -#if defined(CONFIG_TCG_TPM)
> +#if defined(CONFIG_TRUSTED_KEYS_TPM)
>  	{ "tpm", &trusted_key_tpm_ops },
>  #endif
> -#if defined(CONFIG_TEE)
> +#if defined(CONFIG_TRUSTED_KEYS_TEE)
>  	{ "tee", &trusted_key_tee_ops },
>  #endif
>  };
> -- 
> 2.30.2
> 

Works like a charm here. The key type was registered even thought I
built the kernel with CONFIG_TRUSTED_KEYS=m.

I've not actually tested if that key type works (as I've not progressed
that far in my toy project just yet).

Feel free to add my Test-By.

Tested-By: Andreas Rammhold <andreas@rammhold.de>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] KEYS: trusted: fix use as module when CONFIG_TCG_TPM=m
  2021-07-21 16:02 [PATCH v2] KEYS: trusted: fix use as module when CONFIG_TCG_TPM=m Ahmad Fatoum
  2021-07-21 20:17 ` Andreas Rammhold
@ 2021-07-22  4:46 ` Sumit Garg
  2021-07-27  3:04 ` Jarkko Sakkinen
  2 siblings, 0 replies; 9+ messages in thread
From: Sumit Garg @ 2021-07-22  4:46 UTC (permalink / raw)
  To: Ahmad Fatoum
  Cc: Jarkko Sakkinen, James Morris, Serge E. Hallyn, James Bottomley,
	Mimi Zohar, David Howells, Herbert Xu, David S. Miller, kernel,
	Andreas Rammhold, David Gstir, Richard Weinberger,
	open list:ASYMMETRIC KEYS,
	open list:HARDWARE RANDOM NUMBER GENERATOR CORE,
	Linux Kernel Mailing List, open list:SECURITY SUBSYSTEM,
	linux-integrity

On Wed, 21 Jul 2021 at 21:34, Ahmad Fatoum <a.fatoum@pengutronix.de> wrote:
>
> Since commit 5d0682be3189 ("KEYS: trusted: Add generic trusted keys
> framework"), trusted.ko built with CONFIG_TCG_TPM=CONFIG_TRUSTED_KEYS=m
> will not register the TPM trusted key type at runtime.
>
> This is because, after that rework, CONFIG_DEPENDENCY of the TPM
> and TEE backends were checked with #ifdef, but that's only true
> when they're built-in.
>
> Fix this by introducing two new boolean Kconfig symbols:
> TRUSTED_KEYS_TPM and TRUSTED_KEYS_TEE with the appropriate
> dependencies and use them to check which backends are available.
>
> This also has a positive effect on user experience:
>
>  - It's now possible to use TEE trusted keys without CONFIG_TCG_TPM
>  - It's now possible to enable CONFIG_TCG_TPM, but exclude TPM from
>    available trust sources
>  - TEE=m && TRUSTED_KEYS=y no longer leads to TEE support
>    being silently dropped
>
> Any code depending on the TPM trusted key backend or symbols exported
> by it will now need to explicitly state that it
>
>   depends on TRUSTED_KEYS && TRUSTED_KEYS_TPM
>
> The latter to ensure the dependency is built and the former to ensure
> it's reachable for module builds. This currently only affects
> CONFIG_ASYMMETRIC_TPM_KEY_SUBTYPE, so it's fixed up here as well.
>
> Reported-by: Andreas Rammhold <andreas@rammhold.de>
> Fixes: 5d0682be3189 ("KEYS: trusted: Add generic trusted keys framework")
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
>
> (Implicit) v1 was as a preparatory patch for CAAM trusted keys[1] with the
> goal of fixing the Kconfig inflexibility after the TEE trusted key rework.
>
> Unbeknownst to me, it also fixes a regression, which was later
> reported by Andreas[2] along with a patch.
>
> I split out the fix from the CAAM series and adjusted the commit
> message to explain the regression.
>
> v1 -> v2:
>   - Move rest of TPM-related selects from TRUSTED_KEYS to
>     TRUSTED_KEYS_TPM (Sumit)
>   - Remove left-over line in Makefile (Sumit)
>   - added Fixes: tag
>   - adjust commit message to reference the regression reported
>     by Andreas
>   - have ASYMMETRIC_TPM_KEY_SUBTYPE depend on TRUSTED_KEYS_TPM,
>     because it references global symbols that are exported
>     by the trusted key TPM backend.
>
> [1]: https://lore.kernel.org/linux-integrity/f8285eb0135ba30c9d846cf9dd395d1f5f8b1efc.1624364386.git-series.a.fatoum@pengutronix.de/
> [2]: https://lore.kernel.org/linux-integrity/20210719091335.vwfebcpkf4pag3wm@wrt/T/#t
>
> To: Jarkko Sakkinen <jarkko@kernel.org>
> To: James Morris <jmorris@namei.org>
> To: "Serge E. Hallyn" <serge@hallyn.com>
> To: James Bottomley <jejb@linux.ibm.com>
> To: Mimi Zohar <zohar@linux.ibm.com>
> To: Sumit Garg <sumit.garg@linaro.org>
> To: David Howells <dhowells@redhat.com>
> To: Herbert Xu <herbert@gondor.apana.org.au>
> To: "David S. Miller" <davem@davemloft.net>
> Cc: David Gstir <david@sigma-star.at>
> Cc: Richard Weinberger <richard@nod.at>
> Cc: keyrings@vger.kernel.org
> Cc: linux-crypto@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-security-module@vger.kernel.org
> Cc: linux-integrity@vger.kernel.org
> ---
>  crypto/asymmetric_keys/Kconfig            |  2 +-
>  security/keys/Kconfig                     | 18 ++++++--------
>  security/keys/trusted-keys/Kconfig        | 29 +++++++++++++++++++++++
>  security/keys/trusted-keys/Makefile       |  8 +++----
>  security/keys/trusted-keys/trusted_core.c |  4 ++--
>  5 files changed, 43 insertions(+), 18 deletions(-)
>  create mode 100644 security/keys/trusted-keys/Kconfig
>

Looks good to me apart from the minor comment below. With that fixed:

Reviewed-by: Sumit Garg <sumit.garg@linaro.org>

> diff --git a/crypto/asymmetric_keys/Kconfig b/crypto/asymmetric_keys/Kconfig
> index 1f1f004dc757..8886eddbf881 100644
> --- a/crypto/asymmetric_keys/Kconfig
> +++ b/crypto/asymmetric_keys/Kconfig
> @@ -25,7 +25,7 @@ config ASYMMETRIC_PUBLIC_KEY_SUBTYPE
>  config ASYMMETRIC_TPM_KEY_SUBTYPE
>         tristate "Asymmetric TPM backed private key subtype"
>         depends on TCG_TPM
> -       depends on TRUSTED_KEYS
> +       depends on TRUSTED_KEYS && TRUSTED_KEYS_TPM
>         select CRYPTO_HMAC
>         select CRYPTO_SHA1
>         select CRYPTO_HASH_INFO
> diff --git a/security/keys/Kconfig b/security/keys/Kconfig
> index 64b81abd087e..9ec302962fe2 100644
> --- a/security/keys/Kconfig
> +++ b/security/keys/Kconfig
> @@ -70,23 +70,19 @@ config BIG_KEYS
>
>  config TRUSTED_KEYS
>         tristate "TRUSTED KEYS"
> -       depends on KEYS && TCG_TPM
> -       select CRYPTO
> -       select CRYPTO_HMAC
> -       select CRYPTO_SHA1
> -       select CRYPTO_HASH_INFO
> -       select ASN1_ENCODER
> -       select OID_REGISTRY
> -       select ASN1
> +       depends on KEYS
>         help
>           This option provides support for creating, sealing, and unsealing
>           keys in the kernel. Trusted keys are random number symmetric keys,
> -         generated and RSA-sealed by the TPM. The TPM only unseals the keys,
> -         if the boot PCRs and other criteria match.  Userspace will only ever
> -         see encrypted blobs.
> +         generated and sealed by a trust source selected at kernel boot-time.
> +         Userspace will only ever see encrypted blobs.
>
>           If you are unsure as to whether this is required, answer N.
>
> +if TRUSTED_KEYS
> +source "security/keys/trusted-keys/Kconfig"
> +endif
> +
>  config ENCRYPTED_KEYS
>         tristate "ENCRYPTED KEYS"
>         depends on KEYS
> diff --git a/security/keys/trusted-keys/Kconfig b/security/keys/trusted-keys/Kconfig
> new file mode 100644
> index 000000000000..c163cfeedff6
> --- /dev/null
> +++ b/security/keys/trusted-keys/Kconfig
> @@ -0,0 +1,29 @@
> +config TRUSTED_KEYS_TPM
> +       bool "TPM-based trusted keys"
> +       depends on TCG_TPM >= TRUSTED_KEYS
> +       default y
> +       select CRYPTO
> +       select CRYPTO_HMAC
> +       select CRYPTO_SHA1
> +       select CRYPTO_HASH_INFO
> +       select ASN1_ENCODER
> +       select OID_REGISTRY
> +       select ASN1
> +       help
> +         Enable use of the Trusted Platform Module (TPM) as trusted key
> +         backend. Trusted keys are are random number symmetric keys,

s/are are/are/

-Sumit

> +         which will be generated and RSA-sealed by the TPM.
> +         The TPM only unseals the keys, if the boot PCRs and other
> +         criteria match.
> +
> +config TRUSTED_KEYS_TEE
> +       bool "TEE-based trusted keys"
> +       depends on TEE >= TRUSTED_KEYS
> +       default y
> +       help
> +         Enable use of the Trusted Execution Environment (TEE) as trusted
> +         key backend.
> +
> +if !TRUSTED_KEYS_TPM && !TRUSTED_KEYS_TEE
> +comment "No trust source selected!"
> +endif
> diff --git a/security/keys/trusted-keys/Makefile b/security/keys/trusted-keys/Makefile
> index feb8b6c3cc79..2e2371eae4d5 100644
> --- a/security/keys/trusted-keys/Makefile
> +++ b/security/keys/trusted-keys/Makefile
> @@ -5,10 +5,10 @@
>
>  obj-$(CONFIG_TRUSTED_KEYS) += trusted.o
>  trusted-y += trusted_core.o
> -trusted-y += trusted_tpm1.o
> +trusted-$(CONFIG_TRUSTED_KEYS_TPM) += trusted_tpm1.o
>
>  $(obj)/trusted_tpm2.o: $(obj)/tpm2key.asn1.h
> -trusted-y += trusted_tpm2.o
> -trusted-y += tpm2key.asn1.o
> +trusted-$(CONFIG_TRUSTED_KEYS_TPM) += trusted_tpm2.o
> +trusted-$(CONFIG_TRUSTED_KEYS_TPM) += tpm2key.asn1.o
>
> -trusted-$(CONFIG_TEE) += trusted_tee.o
> +trusted-$(CONFIG_TRUSTED_KEYS_TEE) += trusted_tee.o
> diff --git a/security/keys/trusted-keys/trusted_core.c b/security/keys/trusted-keys/trusted_core.c
> index d5c891d8d353..8cab69e5d0da 100644
> --- a/security/keys/trusted-keys/trusted_core.c
> +++ b/security/keys/trusted-keys/trusted_core.c
> @@ -27,10 +27,10 @@ module_param_named(source, trusted_key_source, charp, 0);
>  MODULE_PARM_DESC(source, "Select trusted keys source (tpm or tee)");
>
>  static const struct trusted_key_source trusted_key_sources[] = {
> -#if defined(CONFIG_TCG_TPM)
> +#if defined(CONFIG_TRUSTED_KEYS_TPM)
>         { "tpm", &trusted_key_tpm_ops },
>  #endif
> -#if defined(CONFIG_TEE)
> +#if defined(CONFIG_TRUSTED_KEYS_TEE)
>         { "tee", &trusted_key_tee_ops },
>  #endif
>  };
> --
> 2.30.2
>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] KEYS: trusted: fix use as module when CONFIG_TCG_TPM=m
  2021-07-21 16:02 [PATCH v2] KEYS: trusted: fix use as module when CONFIG_TCG_TPM=m Ahmad Fatoum
  2021-07-21 20:17 ` Andreas Rammhold
  2021-07-22  4:46 ` Sumit Garg
@ 2021-07-27  3:04 ` Jarkko Sakkinen
  2021-07-27  4:24   ` Ahmad Fatoum
  2 siblings, 1 reply; 9+ messages in thread
From: Jarkko Sakkinen @ 2021-07-27  3:04 UTC (permalink / raw)
  To: Ahmad Fatoum
  Cc: James Morris, Serge E. Hallyn, James Bottomley, Mimi Zohar,
	Sumit Garg, David Howells, Herbert Xu, David S. Miller, kernel,
	Andreas Rammhold, David Gstir, Richard Weinberger, keyrings,
	linux-crypto, linux-kernel, linux-security-module,
	linux-integrity

On Wed, Jul 21, 2021 at 06:02:59PM +0200, Ahmad Fatoum wrote:
> Since commit 5d0682be3189 ("KEYS: trusted: Add generic trusted keys
> framework"), trusted.ko built with CONFIG_TCG_TPM=CONFIG_TRUSTED_KEYS=m
> will not register the TPM trusted key type at runtime.
> 
> This is because, after that rework, CONFIG_DEPENDENCY of the TPM
> and TEE backends were checked with #ifdef, but that's only true
> when they're built-in.
> 
> Fix this by introducing two new boolean Kconfig symbols:
> TRUSTED_KEYS_TPM and TRUSTED_KEYS_TEE with the appropriate
> dependencies and use them to check which backends are available.
> 
> This also has a positive effect on user experience:
> 
>  - It's now possible to use TEE trusted keys without CONFIG_TCG_TPM
>  - It's now possible to enable CONFIG_TCG_TPM, but exclude TPM from
>    available trust sources
>  - TEE=m && TRUSTED_KEYS=y no longer leads to TEE support
>    being silently dropped
> 
> Any code depending on the TPM trusted key backend or symbols exported
> by it will now need to explicitly state that it
> 
>   depends on TRUSTED_KEYS && TRUSTED_KEYS_TPM
> 
> The latter to ensure the dependency is built and the former to ensure
> it's reachable for module builds. This currently only affects
> CONFIG_ASYMMETRIC_TPM_KEY_SUBTYPE, so it's fixed up here as well.
> 
> Reported-by: Andreas Rammhold <andreas@rammhold.de>
> Fixes: 5d0682be3189 ("KEYS: trusted: Add generic trusted keys framework")
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

Is it absolutely need to do all this *just* to fix the bug?

For a pure bug fix the most essential thing is to be able the backport
it to stable kernels.

I don't really care at all about extra niceties ("it's now possible
stuff).

This looks like a bug fix and improvements bundle into a single patch.

/Jarkko

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] KEYS: trusted: fix use as module when CONFIG_TCG_TPM=m
  2021-07-27  3:04 ` Jarkko Sakkinen
@ 2021-07-27  4:24   ` Ahmad Fatoum
  2021-07-28 21:52     ` Jarkko Sakkinen
  0 siblings, 1 reply; 9+ messages in thread
From: Ahmad Fatoum @ 2021-07-27  4:24 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: James Morris, Serge E. Hallyn, James Bottomley, Mimi Zohar,
	Sumit Garg, David Howells, Herbert Xu, David S. Miller, kernel,
	Andreas Rammhold, David Gstir, Richard Weinberger, keyrings,
	linux-crypto, linux-kernel, linux-security-module,
	linux-integrity

On 27.07.21 05:04, Jarkko Sakkinen wrote:
>> Reported-by: Andreas Rammhold <andreas@rammhold.de>
>> Fixes: 5d0682be3189 ("KEYS: trusted: Add generic trusted keys framework")
>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> 
> Is it absolutely need to do all this *just* to fix the bug?
> 
> For a pure bug fix the most essential thing is to be able the backport
> it to stable kernels.

Not much happened in-between, so a backport should be trivial.
I can provide these if needed.

> I don't really care at all about extra niceties ("it's now possible
> stuff).
> 
> This looks like a bug fix and improvements bundle into a single patch.

You can replace the #ifdefs with #if IS_REACHABLE in trusted_core.c to fix the
intermediate breakage and then throw that away again to fix the remaining
dependency of trusted keys on TCG_TPM.

If you prefer that, Andreas perhaps could respin his series with
s/IS_ENABLED/IS_REACHABLE/?

Cheers,
Ahmad


> 
> /Jarkko
> 


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] KEYS: trusted: fix use as module when CONFIG_TCG_TPM=m
  2021-07-27  4:24   ` Ahmad Fatoum
@ 2021-07-28 21:52     ` Jarkko Sakkinen
  2021-07-28 22:29       ` Ahmad Fatoum
  0 siblings, 1 reply; 9+ messages in thread
From: Jarkko Sakkinen @ 2021-07-28 21:52 UTC (permalink / raw)
  To: Ahmad Fatoum
  Cc: James Morris, Serge E. Hallyn, James Bottomley, Mimi Zohar,
	Sumit Garg, David Howells, Herbert Xu, David S. Miller, kernel,
	Andreas Rammhold, David Gstir, Richard Weinberger, keyrings,
	linux-crypto, linux-kernel, linux-security-module,
	linux-integrity

On Tue, Jul 27, 2021 at 06:24:49AM +0200, Ahmad Fatoum wrote:
> On 27.07.21 05:04, Jarkko Sakkinen wrote:
> >> Reported-by: Andreas Rammhold <andreas@rammhold.de>
> >> Fixes: 5d0682be3189 ("KEYS: trusted: Add generic trusted keys framework")
> >> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> > 
> > Is it absolutely need to do all this *just* to fix the bug?
> > 
> > For a pure bug fix the most essential thing is to be able the backport
> > it to stable kernels.
> 
> Not much happened in-between, so a backport should be trivial.
> I can provide these if needed.

"not much" is not good enough. It should be "not anything".

/Jarkko

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] KEYS: trusted: fix use as module when CONFIG_TCG_TPM=m
  2021-07-28 21:52     ` Jarkko Sakkinen
@ 2021-07-28 22:29       ` Ahmad Fatoum
  2021-07-30  0:31         ` Jarkko Sakkinen
  0 siblings, 1 reply; 9+ messages in thread
From: Ahmad Fatoum @ 2021-07-28 22:29 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: James Morris, Serge E. Hallyn, James Bottomley, Mimi Zohar,
	Sumit Garg, David Howells, Herbert Xu, David S. Miller, kernel,
	Andreas Rammhold, David Gstir, Richard Weinberger, keyrings,
	linux-crypto, linux-kernel, linux-security-module,
	linux-integrity

On 28.07.21 23:52, Jarkko Sakkinen wrote:
> On Tue, Jul 27, 2021 at 06:24:49AM +0200, Ahmad Fatoum wrote:
>> On 27.07.21 05:04, Jarkko Sakkinen wrote:
>>>> Reported-by: Andreas Rammhold <andreas@rammhold.de>
>>>> Fixes: 5d0682be3189 ("KEYS: trusted: Add generic trusted keys framework")
>>>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>>>
>>> Is it absolutely need to do all this *just* to fix the bug?
>>>
>>> For a pure bug fix the most essential thing is to be able the backport
>>> it to stable kernels.
>>
>> Not much happened in-between, so a backport should be trivial.
>> I can provide these if needed.
> 
> "not much" is not good enough. It should be "not anything".

"Not much" [code that could conflict was added in-between].

I just checked and it applies cleanly on v5.13. On the off chance
that this patch conflicts with another stable backport by the time
it's backported, I'll get a friendly automated email and send out
a rebased patch.

Cheers,
Ahmad


> 
> /Jarkko
> 


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] KEYS: trusted: fix use as module when CONFIG_TCG_TPM=m
  2021-07-28 22:29       ` Ahmad Fatoum
@ 2021-07-30  0:31         ` Jarkko Sakkinen
  2021-07-30  6:21           ` Ahmad Fatoum
  0 siblings, 1 reply; 9+ messages in thread
From: Jarkko Sakkinen @ 2021-07-30  0:31 UTC (permalink / raw)
  To: Ahmad Fatoum
  Cc: James Morris, Serge E. Hallyn, James Bottomley, Mimi Zohar,
	Sumit Garg, David Howells, Herbert Xu, David S. Miller, kernel,
	Andreas Rammhold, David Gstir, Richard Weinberger, keyrings,
	linux-crypto, linux-kernel, linux-security-module,
	linux-integrity

On Thu, Jul 29, 2021 at 12:29:38AM +0200, Ahmad Fatoum wrote:
> On 28.07.21 23:52, Jarkko Sakkinen wrote:
> > On Tue, Jul 27, 2021 at 06:24:49AM +0200, Ahmad Fatoum wrote:
> >> On 27.07.21 05:04, Jarkko Sakkinen wrote:
> >>>> Reported-by: Andreas Rammhold <andreas@rammhold.de>
> >>>> Fixes: 5d0682be3189 ("KEYS: trusted: Add generic trusted keys framework")
> >>>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> >>>
> >>> Is it absolutely need to do all this *just* to fix the bug?
> >>>
> >>> For a pure bug fix the most essential thing is to be able the backport
> >>> it to stable kernels.
> >>
> >> Not much happened in-between, so a backport should be trivial.
> >> I can provide these if needed.
> > 
> > "not much" is not good enough. It should be "not anything".
> 
> "Not much" [code that could conflict was added in-between].
> 
> I just checked and it applies cleanly on v5.13. On the off chance
> that this patch conflicts with another stable backport by the time
> it's backported, I'll get a friendly automated email and send out
> a rebased patch.

What you should do is to split this into patch that exactly
fixes the issue, and to one that adds the "niceties".

/Jarkko

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] KEYS: trusted: fix use as module when CONFIG_TCG_TPM=m
  2021-07-30  0:31         ` Jarkko Sakkinen
@ 2021-07-30  6:21           ` Ahmad Fatoum
  0 siblings, 0 replies; 9+ messages in thread
From: Ahmad Fatoum @ 2021-07-30  6:21 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: James Morris, Serge E. Hallyn, James Bottomley, Mimi Zohar,
	Sumit Garg, David Howells, Herbert Xu, David S. Miller, kernel,
	Andreas Rammhold, David Gstir, Richard Weinberger, keyrings,
	linux-crypto, linux-kernel, linux-security-module,
	linux-integrity

On 30.07.21 02:31, Jarkko Sakkinen wrote:
> On Thu, Jul 29, 2021 at 12:29:38AM +0200, Ahmad Fatoum wrote:
>> On 28.07.21 23:52, Jarkko Sakkinen wrote:
>>> On Tue, Jul 27, 2021 at 06:24:49AM +0200, Ahmad Fatoum wrote:
>>>> On 27.07.21 05:04, Jarkko Sakkinen wrote:
>>>>>> Reported-by: Andreas Rammhold <andreas@rammhold.de>
>>>>>> Fixes: 5d0682be3189 ("KEYS: trusted: Add generic trusted keys framework")
>>>>>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>>>>>
>>>>> Is it absolutely need to do all this *just* to fix the bug?
>>>>>
>>>>> For a pure bug fix the most essential thing is to be able the backport
>>>>> it to stable kernels.
>>>>
>>>> Not much happened in-between, so a backport should be trivial.
>>>> I can provide these if needed.
>>>
>>> "not much" is not good enough. It should be "not anything".
>>
>> "Not much" [code that could conflict was added in-between].
>>
>> I just checked and it applies cleanly on v5.13. On the off chance
>> that this patch conflicts with another stable backport by the time
>> it's backported, I'll get a friendly automated email and send out
>> a rebased patch.
> 
> What you should do is to split this into patch that exactly
> fixes the issue, and to one that adds the "niceties".

I'd rather not send out patches I believe to be incomplete, sorry.
If you want to fix the regression's root cause of insufficient
Kconfig description, that here is what it takes.

You can take Andreas' regression fix for stable and replace it with my patch later.
I'll send out a new version removing references that it fixes a regression.

Cheers,
Ahmad

> 
> /Jarkko
> 


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2021-07-30  6:22 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-21 16:02 [PATCH v2] KEYS: trusted: fix use as module when CONFIG_TCG_TPM=m Ahmad Fatoum
2021-07-21 20:17 ` Andreas Rammhold
2021-07-22  4:46 ` Sumit Garg
2021-07-27  3:04 ` Jarkko Sakkinen
2021-07-27  4:24   ` Ahmad Fatoum
2021-07-28 21:52     ` Jarkko Sakkinen
2021-07-28 22:29       ` Ahmad Fatoum
2021-07-30  0:31         ` Jarkko Sakkinen
2021-07-30  6:21           ` Ahmad Fatoum

This is a public inbox, see mirroring instructions
on how to clone and mirror all data and code used for this inbox