[PATCH] KEYS: trusted: allow module init if TPM is inactive or deactivated

[PATCH] KEYS: trusted: allow module init if TPM is inactive or deactivated

[Roberto Sassu]

Commit c78719203fc6 ("KEYS: trusted: allow trusted.ko to initialize w/o a
TPM") allows the trusted module to be loaded even a TPM is not found to
avoid module dependency problems.

Unfortunately, this does not completely solve the issue, as there could be
a case where a TPM is found but is not functional (the TPM commands return
an error). Specifically, after the tpm_chip structure is returned by
tpm_default_chip() in init_trusted(), the execution terminates after
init_digests() returns -EFAULT (due to the fact that tpm_get_random()
returns a positive value, but less than TPM_MAX_DIGEST_SIZE).

This patch fixes the issue by ignoring the TPM_ERR_DEACTIVATED and
TPM_ERR_DISABLED errors.

Fixes: 240730437deb ("KEYS: trusted: explicitly use tpm_chip structure...")
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 drivers/char/tpm/tpm.h  | 2 --
 include/linux/tpm.h     | 3 +++
 security/keys/trusted.c | 6 +++++-
 3 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index e503ffc3aa39..a216ac396711 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -54,8 +54,6 @@ enum tpm_addr {
 
 #define TPM_WARN_RETRY          0x800
 #define TPM_WARN_DOING_SELFTEST 0x802
-#define TPM_ERR_DEACTIVATED     0x6
-#define TPM_ERR_DISABLED        0x7
 #define TPM_ERR_INVALID_POSTINIT 38
 
 #define TPM_HEADER_SIZE		10
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index 53c0ea9ec9df..efd3ccbb6aee 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -26,6 +26,9 @@
 #define TPM_DIGEST_SIZE 20	/* Max TPM v1.2 PCR size */
 #define TPM_MAX_DIGEST_SIZE SHA512_DIGEST_SIZE
 
+#define TPM_ERR_DEACTIVATED     0x6
+#define TPM_ERR_DISABLED        0x7
+
 struct tpm_chip;
 struct trusted_key_payload;
 struct trusted_key_options;
diff --git a/security/keys/trusted.c b/security/keys/trusted.c
index 9a94672e7adc..430d85090b3b 100644
--- a/security/keys/trusted.c
+++ b/security/keys/trusted.c
@@ -389,6 +389,10 @@ static int pcrlock(const int pcrnum)
 	if (!capable(CAP_SYS_ADMIN))
 		return -EPERM;
 
+	/* This can happen if the TPM is inactive. */
+	if (!digests)
+		return -EINVAL;
+
 	return tpm_pcr_extend(chip, pcrnum, digests) ? -EINVAL : 0;
 }
 
@@ -1233,7 +1237,7 @@ static int __init init_digests(void)
 	int i;
 
 	ret = tpm_get_random(chip, digest, TPM_MAX_DIGEST_SIZE);
-	if (ret < 0)
+	if (ret < 0 || ret == TPM_ERR_DEACTIVATED || ret == TPM_ERR_DISABLED)
 		return ret;
 	if (ret < TPM_MAX_DIGEST_SIZE)
 		return -EFAULT;
-- 
2.17.1

Re: [PATCH] KEYS: trusted: allow module init if TPM is inactive or deactivated

[Tyler Hicks]

On 2019-07-05 18:37:35, Roberto Sassu wrote:
> Commit c78719203fc6 ("KEYS: trusted: allow trusted.ko to initialize w/o a
> TPM") allows the trusted module to be loaded even a TPM is not found to
> avoid module dependency problems.
> 
> Unfortunately, this does not completely solve the issue, as there could be
> a case where a TPM is found but is not functional (the TPM commands return
> an error). Specifically, after the tpm_chip structure is returned by
> tpm_default_chip() in init_trusted(), the execution terminates after
> init_digests() returns -EFAULT (due to the fact that tpm_get_random()
> returns a positive value, but less than TPM_MAX_DIGEST_SIZE).
> 
> This patch fixes the issue by ignoring the TPM_ERR_DEACTIVATED and
> TPM_ERR_DISABLED errors.
> 
> Fixes: 240730437deb ("KEYS: trusted: explicitly use tpm_chip structure...")
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
>  drivers/char/tpm/tpm.h  | 2 --
>  include/linux/tpm.h     | 3 +++
>  security/keys/trusted.c | 6 +++++-
>  3 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index e503ffc3aa39..a216ac396711 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -54,8 +54,6 @@ enum tpm_addr {
>  
>  #define TPM_WARN_RETRY          0x800
>  #define TPM_WARN_DOING_SELFTEST 0x802
> -#define TPM_ERR_DEACTIVATED     0x6
> -#define TPM_ERR_DISABLED        0x7
>  #define TPM_ERR_INVALID_POSTINIT 38
>  
>  #define TPM_HEADER_SIZE		10
> diff --git a/include/linux/tpm.h b/include/linux/tpm.h
> index 53c0ea9ec9df..efd3ccbb6aee 100644
> --- a/include/linux/tpm.h
> +++ b/include/linux/tpm.h
> @@ -26,6 +26,9 @@
>  #define TPM_DIGEST_SIZE 20	/* Max TPM v1.2 PCR size */
>  #define TPM_MAX_DIGEST_SIZE SHA512_DIGEST_SIZE
>  
> +#define TPM_ERR_DEACTIVATED     0x6
> +#define TPM_ERR_DISABLED        0x7
> +
>  struct tpm_chip;
>  struct trusted_key_payload;
>  struct trusted_key_options;
> diff --git a/security/keys/trusted.c b/security/keys/trusted.c
> index 9a94672e7adc..430d85090b3b 100644
> --- a/security/keys/trusted.c
> +++ b/security/keys/trusted.c
> @@ -389,6 +389,10 @@ static int pcrlock(const int pcrnum)
>  	if (!capable(CAP_SYS_ADMIN))
>  		return -EPERM;
>  
> +	/* This can happen if the TPM is inactive. */
> +	if (!digests)
> +		return -EINVAL;
> +
>  	return tpm_pcr_extend(chip, pcrnum, digests) ? -EINVAL : 0;
>  }
>  
> @@ -1233,7 +1237,7 @@ static int __init init_digests(void)
>  	int i;
>  
>  	ret = tpm_get_random(chip, digest, TPM_MAX_DIGEST_SIZE);
> -	if (ret < 0)
> +	if (ret < 0 || ret == TPM_ERR_DEACTIVATED || ret == TPM_ERR_DISABLED)
>  		return ret;

As someone who hasn't looked at much of the TPM code, I would have
expected tpm_get_random() to return a positive value that only ever
indicates the number of random bytes saved to the buffer. From the
function documentation:

  Return: number of random bytes read or a negative error value.

Despite the function documentation and as your patch suggests, I can
see that it is possible for tpm_transmit_cmd() to return
a positive value that's also returned by tpm_get_random() even though it
may not have filled the buffer when the TPM is in an
inactive/deactivated state.

I think there are other callers which are not prepared for positive
return values that indicate a failure to fill the buffer with random
data. For instance, the way that tpm_hwrng_read() is calling
tpm_get_random() looks a little worrisome.

This patch would likely fix the bug reported against eCryptfs
(https://bugzilla.kernel.org/show_bug.cgi?id=203953) but I can't help to
think that callers of tpm_get_random() would benefit from a more
consolidated approach of handling TPM_ERR_* return values rather than
handling them at this single call site.

Tyler

>  	if (ret < TPM_MAX_DIGEST_SIZE)
>  		return -EFAULT;
> -- 
> 2.17.1
> 

Re: [PATCH] KEYS: trusted: allow module init if TPM is inactive or deactivated

[James Bottomley]

On Fri, 2019-07-05 at 18:37 +0200, Roberto Sassu wrote:
> Commit c78719203fc6 ("KEYS: trusted: allow trusted.ko to initialize
> w/o a
> TPM") allows the trusted module to be loaded even a TPM is not found
> to
> avoid module dependency problems.
> 
> Unfortunately, this does not completely solve the issue, as there
> could be
> a case where a TPM is found but is not functional (the TPM commands
> return
> an error). Specifically, after the tpm_chip structure is returned by
> tpm_default_chip() in init_trusted(), the execution terminates after
> init_digests() returns -EFAULT (due to the fact that tpm_get_random()
> returns a positive value, but less than TPM_MAX_DIGEST_SIZE).
> 
> This patch fixes the issue by ignoring the TPM_ERR_DEACTIVATED and
> TPM_ERR_DISABLED errors.
> 
> Fixes: 240730437deb ("KEYS: trusted: explicitly use tpm_chip
> structure...")
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
>  drivers/char/tpm/tpm.h  | 2 --
>  include/linux/tpm.h     | 3 +++
>  security/keys/trusted.c | 6 +++++-
>  3 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index e503ffc3aa39..a216ac396711 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -54,8 +54,6 @@ enum tpm_addr {
>  
>  #define TPM_WARN_RETRY          0x800
>  #define TPM_WARN_DOING_SELFTEST 0x802
> -#define TPM_ERR_DEACTIVATED     0x6
> -#define TPM_ERR_DISABLED        0x7
>  #define TPM_ERR_INVALID_POSTINIT 38
>  
>  #define TPM_HEADER_SIZE		10
> diff --git a/include/linux/tpm.h b/include/linux/tpm.h
> index 53c0ea9ec9df..efd3ccbb6aee 100644
> --- a/include/linux/tpm.h
> +++ b/include/linux/tpm.h
> @@ -26,6 +26,9 @@
>  #define TPM_DIGEST_SIZE 20	/* Max TPM v1.2 PCR size */
>  #define TPM_MAX_DIGEST_SIZE SHA512_DIGEST_SIZE
>  
> +#define TPM_ERR_DEACTIVATED     0x6
> +#define TPM_ERR_DISABLED        0x7
> +
>  struct tpm_chip;
>  struct trusted_key_payload;
>  struct trusted_key_options;
> diff --git a/security/keys/trusted.c b/security/keys/trusted.c
> index 9a94672e7adc..430d85090b3b 100644
> --- a/security/keys/trusted.c
> +++ b/security/keys/trusted.c
> @@ -389,6 +389,10 @@ static int pcrlock(const int pcrnum)
>  	if (!capable(CAP_SYS_ADMIN))
>  		return -EPERM;
>  
> +	/* This can happen if the TPM is inactive. */
> +	if (!digests)
> +		return -EINVAL;
> +
>  	return tpm_pcr_extend(chip, pcrnum, digests) ? -EINVAL : 0;
>  }
>  
> @@ -1233,7 +1237,7 @@ static int __init init_digests(void)
>  	int i;
>  
>  	ret = tpm_get_random(chip, digest, TPM_MAX_DIGEST_SIZE);

Not a criticism of your patch, but can we please stop doing this. 
Single random number sources are horrendously bad practice because it
gives an attacker a single target to subvert.  We should ensure the TPM
is plugged into the kernel RNG as a source and then take randomness
from the mixed pool so it's harder for an attacker because they have to
subvert all our sources to predict what came out.

James

Re: [PATCH] KEYS: trusted: allow module init if TPM is inactive or deactivated

[Jarkko Sakkinen]

On Mon, Jul 08, 2019 at 01:34:59PM -0700, James Bottomley wrote:
> Not a criticism of your patch, but can we please stop doing this. 
> Single random number sources are horrendously bad practice because it
> gives an attacker a single target to subvert.  We should ensure the TPM
> is plugged into the kernel RNG as a source and then take randomness
> from the mixed pool so it's harder for an attacker because they have to
> subvert all our sources to predict what came out.

It is and I agree.

/Jarkko

Re: [PATCH] KEYS: trusted: allow module init if TPM is inactive or deactivated

[Mimi Zohar]

On Tue, 2019-07-09 at 19:24 +0300, Jarkko Sakkinen wrote:
> On Mon, Jul 08, 2019 at 01:34:59PM -0700, James Bottomley wrote:
> > Not a criticism of your patch, but can we please stop doing this. 
> > Single random number sources are horrendously bad practice because it
> > gives an attacker a single target to subvert.  We should ensure the TPM
> > is plugged into the kernel RNG as a source and then take randomness
> > from the mixed pool so it's harder for an attacker because they have to
> > subvert all our sources to predict what came out.
> 
> It is and I agree.

I still haven't quite figured out why the digests need to be
initialized to anything other than 0.

Mimi

Re: [PATCH] KEYS: trusted: allow module init if TPM is inactive or deactivated

[Tyler Hicks]

On 2019-07-09 12:31:45, Mimi Zohar wrote:
> On Tue, 2019-07-09 at 19:24 +0300, Jarkko Sakkinen wrote:
> > On Mon, Jul 08, 2019 at 01:34:59PM -0700, James Bottomley wrote:
> > > Not a criticism of your patch, but can we please stop doing this. 
> > > Single random number sources are horrendously bad practice because it
> > > gives an attacker a single target to subvert.  We should ensure the TPM
> > > is plugged into the kernel RNG as a source and then take randomness
> > > from the mixed pool so it's harder for an attacker because they have to
> > > subvert all our sources to predict what came out.
> > 
> > It is and I agree.
> 
> I still haven't quite figured out why the digests need to be
> initialized to anything other than 0.

After looking into 0b6cf6b97b7ef1fa3c7fefab0cac897a1c4a3400, I have to
agree. I don't see the purpose of using tpm_get_random() in
init_digests().

Roberto, why can't we just initialize the digests with zeroes? It would
fix the bug for eCryptfs and NVDIMM and address the concern that James
had regarding the single random number source.

Tyler

Re: [PATCH] KEYS: trusted: allow module init if TPM is inactive or deactivated

[Jarkko Sakkinen]

On Fri, Jul 05, 2019 at 06:37:35PM +0200, Roberto Sassu wrote:
> Commit c78719203fc6 ("KEYS: trusted: allow trusted.ko to initialize w/o a
> TPM") allows the trusted module to be loaded even a TPM is not found to
> avoid module dependency problems.
> 
> Unfortunately, this does not completely solve the issue, as there could be
> a case where a TPM is found but is not functional (the TPM commands return
> an error). Specifically, after the tpm_chip structure is returned by
> tpm_default_chip() in init_trusted(), the execution terminates after
> init_digests() returns -EFAULT (due to the fact that tpm_get_random()
> returns a positive value, but less than TPM_MAX_DIGEST_SIZE).
> 
> This patch fixes the issue by ignoring the TPM_ERR_DEACTIVATED and
> TPM_ERR_DISABLED errors.

Why allow trusted module to initialize if TPM is not functional?

Also:

err = tpm_transmit_cmd(chip, &buf,
		       offsetof(struct tpm2_get_random_out,
				buffer),
		       "attempting get random");
if (err) {
	if (err > 0)
		err = -EIO;
	goto out;
}

/Jarkko

Re: [PATCH] KEYS: trusted: allow module init if TPM is inactive or deactivated

[Roberto Sassu]

On 7/11/2019 9:48 PM, Jarkko Sakkinen wrote:
> On Fri, Jul 05, 2019 at 06:37:35PM +0200, Roberto Sassu wrote:
>> Commit c78719203fc6 ("KEYS: trusted: allow trusted.ko to initialize w/o a
>> TPM") allows the trusted module to be loaded even a TPM is not found to
>> avoid module dependency problems.
>>
>> Unfortunately, this does not completely solve the issue, as there could be
>> a case where a TPM is found but is not functional (the TPM commands return
>> an error). Specifically, after the tpm_chip structure is returned by
>> tpm_default_chip() in init_trusted(), the execution terminates after
>> init_digests() returns -EFAULT (due to the fact that tpm_get_random()
>> returns a positive value, but less than TPM_MAX_DIGEST_SIZE).
>>
>> This patch fixes the issue by ignoring the TPM_ERR_DEACTIVATED and
>> TPM_ERR_DISABLED errors.
> 
> Why allow trusted module to initialize if TPM is not functional?

According to the bug report at https://bugs.archlinux.org/task/62678,
the trusted module is a dependency of the ecryptfs module. We should
load the trusted module even if the TPM is inactive or deactivated.

Given that commit 782779b60faa ("tpm: Actually fail on TPM errors during
"get random"") changes the return code of tpm_get_random(), the patch
should be modified to ignore the -EIO error. I will send a new version.

Roberto

-- 
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli

Re: [PATCH] KEYS: trusted: allow module init if TPM is inactive or deactivated

[Jarkko Sakkinen]

On Mon, Jul 15, 2019 at 06:44:28PM +0200, Roberto Sassu wrote:
> According to the bug report at https://bugs.archlinux.org/task/62678,
> the trusted module is a dependency of the ecryptfs module. We should
> load the trusted module even if the TPM is inactive or deactivated.
> 
> Given that commit 782779b60faa ("tpm: Actually fail on TPM errors during
> "get random"") changes the return code of tpm_get_random(), the patch
> should be modified to ignore the -EIO error. I will send a new version.

Do you have information where this dependency comes from?

/Jarkko

Re: [PATCH] KEYS: trusted: allow module init if TPM is inactive or deactivated

[Roberto Sassu]

On 8/1/2019 6:32 PM, Jarkko Sakkinen wrote:
> On Mon, Jul 15, 2019 at 06:44:28PM +0200, Roberto Sassu wrote:
>> According to the bug report at https://bugs.archlinux.org/task/62678,
>> the trusted module is a dependency of the ecryptfs module. We should
>> load the trusted module even if the TPM is inactive or deactivated.
>>
>> Given that commit 782779b60faa ("tpm: Actually fail on TPM errors during
>> "get random"") changes the return code of tpm_get_random(), the patch
>> should be modified to ignore the -EIO error. I will send a new version.
> 
> Do you have information where this dependency comes from?

ecryptfs retrieves the encryption key from encrypted keys (see
ecryptfs_get_encrypted_key()).

Roberto

-- 
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli

Re: [PATCH] KEYS: trusted: allow module init if TPM is inactive or deactivated

[Tyler Hicks]

On 2019-08-02 10:21:16, Roberto Sassu wrote:
> On 8/1/2019 6:32 PM, Jarkko Sakkinen wrote:
> > On Mon, Jul 15, 2019 at 06:44:28PM +0200, Roberto Sassu wrote:
> > > According to the bug report at https://bugs.archlinux.org/task/62678,
> > > the trusted module is a dependency of the ecryptfs module. We should
> > > load the trusted module even if the TPM is inactive or deactivated.
> > > 
> > > Given that commit 782779b60faa ("tpm: Actually fail on TPM errors during
> > > "get random"") changes the return code of tpm_get_random(), the patch
> > > should be modified to ignore the -EIO error. I will send a new version.
> > 
> > Do you have information where this dependency comes from?
> 
> ecryptfs retrieves the encryption key from encrypted keys (see
> ecryptfs_get_encrypted_key()).

That has been there for many years with any problems. It was added
in 2011:

 commit 1252cc3b232e582e887623dc5f70979418caaaa2
 Author: Roberto Sassu <roberto.sassu@polito.it>
 Date:   Mon Jun 27 13:45:45 2011 +0200

     eCryptfs: added support for the encrypted key type

What's recently changed the situation is this patch:

 commit 240730437deb213a58915830884e1a99045624dc
 Author: Roberto Sassu <roberto.sassu@huawei.com>
 Date:   Wed Feb 6 17:24:51 2019 +0100

     KEYS: trusted: explicitly use tpm_chip structure from tpm_default_chip()

Now eCryptfs has a hard dependency on a TPM chip that's working
as expected even if eCryptfs (or the rest of the system) isn't utilizing
the TPM. If the TPM behaves unexpectedly, you can't access your files.
We need to get this straightened out soon.

Tyler

> 
> Roberto
> 
> -- 
> HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
> Managing Director: Li Peng, Li Jian, Shi Yanli

Re: [PATCH] KEYS: trusted: allow module init if TPM is inactive or deactivated

[Jarkko Sakkinen]

On Fri, Aug 02, 2019 at 09:27:22AM -0500, Tyler Hicks wrote:
> On 2019-08-02 10:21:16, Roberto Sassu wrote:
> > On 8/1/2019 6:32 PM, Jarkko Sakkinen wrote:
> > > On Mon, Jul 15, 2019 at 06:44:28PM +0200, Roberto Sassu wrote:
> > > > According to the bug report at https://bugs.archlinux.org/task/62678,
> > > > the trusted module is a dependency of the ecryptfs module. We should
> > > > load the trusted module even if the TPM is inactive or deactivated.
> > > > 
> > > > Given that commit 782779b60faa ("tpm: Actually fail on TPM errors during
> > > > "get random"") changes the return code of tpm_get_random(), the patch
> > > > should be modified to ignore the -EIO error. I will send a new version.
> > > 
> > > Do you have information where this dependency comes from?
> > 
> > ecryptfs retrieves the encryption key from encrypted keys (see
> > ecryptfs_get_encrypted_key()).
> 
> That has been there for many years with any problems. It was added
> in 2011:
> 
>  commit 1252cc3b232e582e887623dc5f70979418caaaa2
>  Author: Roberto Sassu <roberto.sassu@polito.it>
>  Date:   Mon Jun 27 13:45:45 2011 +0200
> 
>      eCryptfs: added support for the encrypted key type
> 
> What's recently changed the situation is this patch:
> 
>  commit 240730437deb213a58915830884e1a99045624dc
>  Author: Roberto Sassu <roberto.sassu@huawei.com>
>  Date:   Wed Feb 6 17:24:51 2019 +0100
> 
>      KEYS: trusted: explicitly use tpm_chip structure from tpm_default_chip()
> 
> Now eCryptfs has a hard dependency on a TPM chip that's working
> as expected even if eCryptfs (or the rest of the system) isn't utilizing
> the TPM. If the TPM behaves unexpectedly, you can't access your files.
> We need to get this straightened out soon.

I agree with this conclusion that eCryptfs needs to be fixed, not
another workaround to trusted.ko.

/Jarkko

Re: [PATCH] KEYS: trusted: allow module init if TPM is inactive or deactivated

[Tyler Hicks]

On 2019-08-02 22:42:26, Jarkko Sakkinen wrote:
> On Fri, Aug 02, 2019 at 09:27:22AM -0500, Tyler Hicks wrote:
> > On 2019-08-02 10:21:16, Roberto Sassu wrote:
> > > On 8/1/2019 6:32 PM, Jarkko Sakkinen wrote:
> > > > On Mon, Jul 15, 2019 at 06:44:28PM +0200, Roberto Sassu wrote:
> > > > > According to the bug report at https://bugs.archlinux.org/task/62678,
> > > > > the trusted module is a dependency of the ecryptfs module. We should
> > > > > load the trusted module even if the TPM is inactive or deactivated.
> > > > > 
> > > > > Given that commit 782779b60faa ("tpm: Actually fail on TPM errors during
> > > > > "get random"") changes the return code of tpm_get_random(), the patch
> > > > > should be modified to ignore the -EIO error. I will send a new version.
> > > > 
> > > > Do you have information where this dependency comes from?
> > > 
> > > ecryptfs retrieves the encryption key from encrypted keys (see
> > > ecryptfs_get_encrypted_key()).
> > 
> > That has been there for many years with any problems. It was added
> > in 2011:
> > 
> >  commit 1252cc3b232e582e887623dc5f70979418caaaa2
> >  Author: Roberto Sassu <roberto.sassu@polito.it>
> >  Date:   Mon Jun 27 13:45:45 2011 +0200
> > 
> >      eCryptfs: added support for the encrypted key type
> > 
> > What's recently changed the situation is this patch:
> > 
> >  commit 240730437deb213a58915830884e1a99045624dc
> >  Author: Roberto Sassu <roberto.sassu@huawei.com>
> >  Date:   Wed Feb 6 17:24:51 2019 +0100
> > 
> >      KEYS: trusted: explicitly use tpm_chip structure from tpm_default_chip()
> > 
> > Now eCryptfs has a hard dependency on a TPM chip that's working
> > as expected even if eCryptfs (or the rest of the system) isn't utilizing
> > the TPM. If the TPM behaves unexpectedly, you can't access your files.
> > We need to get this straightened out soon.
> 
> I agree with this conclusion that eCryptfs needs to be fixed, not
> another workaround to trusted.ko.

That wasn't the conclusion that I came to. I prefer Robert's proposed
change to trusted.ko.

How do you propose that this be fixed in eCryptfs?

Removing encrypted_key support from eCryptfs is the only way that I can
see to fix the bug in eCryptfs. That support has been there since 2011.
I'm not sure of the number of users that would be broken by removing
encrypted_key support. I don't think the number is high but I can't say
that confidently.

Roberto, what was your use case when you added encrypted_key support to
eCryptfs back then? Are you aware of any users of eCryptfs +
encrypted_keys?

Jarkko, removing a long-standing feature is potentially more disruptive
to users than adding a workaround to trusted.ko which already requires a
similar workaround. I don't think that I agree with you on the proper
fix here.

Tyler

> 
> /Jarkko

Re: [PATCH] KEYS: trusted: allow module init if TPM is inactive or deactivated

[Tyler Hicks]

On 2019-08-02 15:23:43, Tyler Hicks wrote:
> On 2019-08-02 22:42:26, Jarkko Sakkinen wrote:
> > On Fri, Aug 02, 2019 at 09:27:22AM -0500, Tyler Hicks wrote:
> > > On 2019-08-02 10:21:16, Roberto Sassu wrote:
> > > > On 8/1/2019 6:32 PM, Jarkko Sakkinen wrote:
> > > > > On Mon, Jul 15, 2019 at 06:44:28PM +0200, Roberto Sassu wrote:
> > > > > > According to the bug report at https://bugs.archlinux.org/task/62678,
> > > > > > the trusted module is a dependency of the ecryptfs module. We should
> > > > > > load the trusted module even if the TPM is inactive or deactivated.
> > > > > > 
> > > > > > Given that commit 782779b60faa ("tpm: Actually fail on TPM errors during
> > > > > > "get random"") changes the return code of tpm_get_random(), the patch
> > > > > > should be modified to ignore the -EIO error. I will send a new version.
> > > > > 
> > > > > Do you have information where this dependency comes from?
> > > > 
> > > > ecryptfs retrieves the encryption key from encrypted keys (see
> > > > ecryptfs_get_encrypted_key()).
> > > 
> > > That has been there for many years with any problems. It was added
> > > in 2011:
> > > 
> > >  commit 1252cc3b232e582e887623dc5f70979418caaaa2
> > >  Author: Roberto Sassu <roberto.sassu@polito.it>
> > >  Date:   Mon Jun 27 13:45:45 2011 +0200
> > > 
> > >      eCryptfs: added support for the encrypted key type
> > > 
> > > What's recently changed the situation is this patch:
> > > 
> > >  commit 240730437deb213a58915830884e1a99045624dc
> > >  Author: Roberto Sassu <roberto.sassu@huawei.com>
> > >  Date:   Wed Feb 6 17:24:51 2019 +0100
> > > 
> > >      KEYS: trusted: explicitly use tpm_chip structure from tpm_default_chip()
> > > 
> > > Now eCryptfs has a hard dependency on a TPM chip that's working
> > > as expected even if eCryptfs (or the rest of the system) isn't utilizing
> > > the TPM. If the TPM behaves unexpectedly, you can't access your files.
> > > We need to get this straightened out soon.
> > 
> > I agree with this conclusion that eCryptfs needs to be fixed, not
> > another workaround to trusted.ko.
> 
> That wasn't the conclusion that I came to. I prefer Robert's proposed
> change to trusted.ko.
> 
> How do you propose that this be fixed in eCryptfs?
> 
> Removing encrypted_key support from eCryptfs is the only way that I can
> see to fix the bug in eCryptfs. That support has been there since 2011.
> I'm not sure of the number of users that would be broken by removing
> encrypted_key support. I don't think the number is high but I can't say
> that confidently.

AFAICT, this bug doesn't only affect eCryptfs. It also affects Intel
nvdimm support starting with:

 commit 4c6926a23b76ea23403976290cd45a7a143f6500
 Author: Dave Jiang <dave.jiang@intel.com>
 Date:   Thu Dec 6 12:40:01 2018 -0800

     acpi/nfit, libnvdimm: Add unlock of nvdimm support for Intel DIMMs

So without a workaround in trusted.ko, encrypted_key support will need
to be removed from eCryptfs and nvdimm to address this issue.

Tyler

> 
> Roberto, what was your use case when you added encrypted_key support to
> eCryptfs back then? Are you aware of any users of eCryptfs +
> encrypted_keys?
> 
> Jarkko, removing a long-standing feature is potentially more disruptive
> to users than adding a workaround to trusted.ko which already requires a
> similar workaround. I don't think that I agree with you on the proper
> fix here.
> 
> Tyler
> 
> > 
> > /Jarkko

Re: [PATCH] KEYS: trusted: allow module init if TPM is inactive or deactivated

[Jarkko Sakkinen]

On Fri, 2019-08-02 at 15:23 -0500, Tyler Hicks wrote:
> That wasn't the conclusion that I came to. I prefer Robert's proposed
> change to trusted.ko.
> 
> How do you propose that this be fixed in eCryptfs?
> 
> Removing encrypted_key support from eCryptfs is the only way that I can
> see to fix the bug in eCryptfs. That support has been there since 2011.
> I'm not sure of the number of users that would be broken by removing
> encrypted_key support. I don't think the number is high but I can't say
> that confidently.

Looking at the documentation [1] it is stated that

"Encrypted keys do not depend on a TPM, and are faster, as they use AES
for encryption/decryption."

Why would you need to remove support for encrypted keys? Isn't it a
regression in encrypted keys to hard depend on trusted keys given
what the documentation says?

> Roberto, what was your use case when you added encrypted_key support to
> eCryptfs back then? Are you aware of any users of eCryptfs +
> encrypted_keys?
> 
> Jarkko, removing a long-standing feature is potentially more disruptive
> to users than adding a workaround to trusted.ko which already requires a
> similar workaround. I don't think that I agree with you on the proper
> fix here.

There is nothing to disagree or agree. I just try to get the picture
since ecryptfs is relatively alien to me.

[1] https://www.kernel.org/doc/html/v4.13/security/keys/trusted-encrypted.html

/Jarkko

Re: [PATCH] KEYS: trusted: allow module init if TPM is inactive or deactivated

[Mimi Zohar]

On Sat, 2019-08-03 at 17:44 +0300, Jarkko Sakkinen wrote:
> On Fri, 2019-08-02 at 15:23 -0500, Tyler Hicks wrote:
> > That wasn't the conclusion that I came to. I prefer Robert's proposed
> > change to trusted.ko.
> > 
> > How do you propose that this be fixed in eCryptfs?
> > 
> > Removing encrypted_key support from eCryptfs is the only way that I can
> > see to fix the bug in eCryptfs. That support has been there since 2011.
> > I'm not sure of the number of users that would be broken by removing
> > encrypted_key support. I don't think the number is high but I can't say
> > that confidently.
> 
> Looking at the documentation [1] it is stated that
> 
> "Encrypted keys do not depend on a TPM, and are faster, as they use AES
> for encryption/decryption."
> 
> Why would you need to remove support for encrypted keys? Isn't it a
> regression in encrypted keys to hard depend on trusted keys given
> what the documentation says?

"Encrypted" key are symmetric keys, which are encrypted/decrypted
either by a "trusted" key or, for development purposes only, a "user"
key.

Mimi

Re: [PATCH] KEYS: trusted: allow module init if TPM is inactive or deactivated

[Roberto Sassu]

On 8/2/2019 10:23 PM, Tyler Hicks wrote:
> On 2019-08-02 22:42:26, Jarkko Sakkinen wrote:
>> On Fri, Aug 02, 2019 at 09:27:22AM -0500, Tyler Hicks wrote:
>>> On 2019-08-02 10:21:16, Roberto Sassu wrote:
>>>> On 8/1/2019 6:32 PM, Jarkko Sakkinen wrote:
>>>>> On Mon, Jul 15, 2019 at 06:44:28PM +0200, Roberto Sassu wrote:
>>>>>> According to the bug report at https://bugs.archlinux.org/task/62678,
>>>>>> the trusted module is a dependency of the ecryptfs module. We should
>>>>>> load the trusted module even if the TPM is inactive or deactivated.
>>>>>>
>>>>>> Given that commit 782779b60faa ("tpm: Actually fail on TPM errors during
>>>>>> "get random"") changes the return code of tpm_get_random(), the patch
>>>>>> should be modified to ignore the -EIO error. I will send a new version.
>>>>>
>>>>> Do you have information where this dependency comes from?
>>>>
>>>> ecryptfs retrieves the encryption key from encrypted keys (see
>>>> ecryptfs_get_encrypted_key()).
>>>
>>> That has been there for many years with any problems. It was added
>>> in 2011:
>>>
>>>   commit 1252cc3b232e582e887623dc5f70979418caaaa2
>>>   Author: Roberto Sassu <roberto.sassu@polito.it>
>>>   Date:   Mon Jun 27 13:45:45 2011 +0200
>>>
>>>       eCryptfs: added support for the encrypted key type
>>>
>>> What's recently changed the situation is this patch:
>>>
>>>   commit 240730437deb213a58915830884e1a99045624dc
>>>   Author: Roberto Sassu <roberto.sassu@huawei.com>
>>>   Date:   Wed Feb 6 17:24:51 2019 +0100
>>>
>>>       KEYS: trusted: explicitly use tpm_chip structure from tpm_default_chip()
>>>
>>> Now eCryptfs has a hard dependency on a TPM chip that's working
>>> as expected even if eCryptfs (or the rest of the system) isn't utilizing
>>> the TPM. If the TPM behaves unexpectedly, you can't access your files.
>>> We need to get this straightened out soon.
>>
>> I agree with this conclusion that eCryptfs needs to be fixed, not
>> another workaround to trusted.ko.
> 
> That wasn't the conclusion that I came to. I prefer Robert's proposed
> change to trusted.ko.
> 
> How do you propose that this be fixed in eCryptfs?
> 
> Removing encrypted_key support from eCryptfs is the only way that I can
> see to fix the bug in eCryptfs. That support has been there since 2011.
> I'm not sure of the number of users that would be broken by removing
> encrypted_key support. I don't think the number is high but I can't say
> that confidently.
> 
> Roberto, what was your use case when you added encrypted_key support to
> eCryptfs back then? Are you aware of any users of eCryptfs +
> encrypted_keys?

Well, my use case at that time (I was at the university) was to provide
a secure storage based on TPM. I'm not aware of users of this
functionality, but I believe that it is reasonable (depending on how
trusted keys are used, the secure storage is available only on a
specific platform and if the software configuration is good). This
secure storage would be even more effective if it is coupled with
Mandatory Access Control, to release sensitive data only to the
legitimate applications.


> Jarkko, removing a long-standing feature is potentially more disruptive
> to users than adding a workaround to trusted.ko which already requires a
> similar workaround. I don't think that I agree with you on the proper
> fix here.

I also don't think it is a good idea to remove this functionality.

Jarkko, we were discussing about this issue in another thread, and your
answer then (https://lkml.org/lkml/2019/3/21/396) was that it is a
priority to fix the regression.

The patch I proposed (https://lkml.org/lkml/2019/8/2/1282, I will apply
Tyler's comments) is not invasive and fixes the issue. If the digests
pointer is still NULL, pcrlock() returns an error.

Regarding Mimi's proposal to avoid the issue by extending the PCR with
zeros, I think it also achieve the goal. The purpose of pcrlock() is to
prevent unsealing of sealed data outside the kernel. If PCR values do
not change, sealed data can be unsealed and misused by a user space
process.

Extending the PCR with any value would be sufficient, as the extend
operation (the only way to update PCRs) was designed in a way that
finding the value that would be passed to the extend operation to obtain
again one of the previous PCR values is computationally infeasible.

Roberto

-- 
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli

Re: [PATCH] KEYS: trusted: allow module init if TPM is inactive or deactivated

[Mimi Zohar]

On Mon, 2019-08-05 at 16:50 +0200, Roberto Sassu wrote:
> Regarding Mimi's proposal to avoid the issue by extending the PCR with
> zeros, I think it also achieve the goal. 

Roberto, removing the following code from init_digests() would be the
equivalent to the prior code, without needing to make any other
changes.  Let's keep it simple.  Do you want to post the patch with
the change, or should I?

        ret = tpm_get_random(chip, digest, TPM_MAX_DIGEST_SIZE);
        if (ret < 0)
                return ret;
        if (ret < TPM_MAX_DIGEST_SIZE)
                return -EFAULT;

As I can't duplicate the problem, it would need to be tested by others
experiencing the problem.

thanks,

Mimi

Re: [PATCH] KEYS: trusted: allow module init if TPM is inactive or deactivated

[Roberto Sassu]

On 8/5/2019 5:54 PM, Mimi Zohar wrote:
> On Mon, 2019-08-05 at 16:50 +0200, Roberto Sassu wrote:
>> Regarding Mimi's proposal to avoid the issue by extending the PCR with
>> zeros, I think it also achieve the goal.
> 
> Roberto, removing the following code from init_digests() would be the
> equivalent to the prior code, without needing to make any other
> changes.  Let's keep it simple.  Do you want to post the patch with
> the change, or should I?

I will send the new patch.

Roberto

-- 
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli

Re: [PATCH] KEYS: trusted: allow module init if TPM is inactive or deactivated

[Tyler Hicks]

On 2019-08-05 11:54:19, Mimi Zohar wrote:
> On Mon, 2019-08-05 at 16:50 +0200, Roberto Sassu wrote:
> > Regarding Mimi's proposal to avoid the issue by extending the PCR with
> > zeros, I think it also achieve the goal. 
> 
> Roberto, removing the following code from init_digests() would be the
> equivalent to the prior code, without needing to make any other
> changes.  Let's keep it simple.  Do you want to post the patch with
> the change, or should I?
> 
>         ret = tpm_get_random(chip, digest, TPM_MAX_DIGEST_SIZE);
>         if (ret < 0)
>                 return ret;
>         if (ret < TPM_MAX_DIGEST_SIZE)
>                 return -EFAULT;
> 
> As I can't duplicate the problem, it would need to be tested by others
> experiencing the problem.

The bug reporter tested Roberto's last patch:

 https://bugzilla.kernel.org/show_bug.cgi?id=203953#c8

We should Cc the reporter on future patches or at least leave another
testing request in the bugzilla.

Tyler

> 
> thanks,
> 
> Mimi
> 

Re: [PATCH] KEYS: trusted: allow module init if TPM is inactive or deactivated

[Roberto Sassu]

On 8/5/2019 6:04 PM, Tyler Hicks wrote:
> On 2019-08-05 11:54:19, Mimi Zohar wrote:
>> On Mon, 2019-08-05 at 16:50 +0200, Roberto Sassu wrote:
>>> Regarding Mimi's proposal to avoid the issue by extending the PCR with
>>> zeros, I think it also achieve the goal.
>>
>> Roberto, removing the following code from init_digests() would be the
>> equivalent to the prior code, without needing to make any other
>> changes.  Let's keep it simple.  Do you want to post the patch with
>> the change, or should I?
>>
>>          ret = tpm_get_random(chip, digest, TPM_MAX_DIGEST_SIZE);
>>          if (ret < 0)
>>                  return ret;
>>          if (ret < TPM_MAX_DIGEST_SIZE)
>>                  return -EFAULT;
>>
>> As I can't duplicate the problem, it would need to be tested by others
>> experiencing the problem.
> 
> The bug reporter tested Roberto's last patch:
> 
>   https://bugzilla.kernel.org/show_bug.cgi?id=203953#c8
> 
> We should Cc the reporter on future patches or at least leave another
> testing request in the bugzilla.

I don't see the reporter's email. Please ask him to test the new patch.

Thanks

Roberto

-- 
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli

Re: [PATCH] KEYS: trusted: allow module init if TPM is inactive or deactivated

[Tyler Hicks]

On 2019-08-05 18:51:09, Roberto Sassu wrote:
> On 8/5/2019 6:04 PM, Tyler Hicks wrote:
> > On 2019-08-05 11:54:19, Mimi Zohar wrote:
> > > On Mon, 2019-08-05 at 16:50 +0200, Roberto Sassu wrote:
> > > > Regarding Mimi's proposal to avoid the issue by extending the PCR with
> > > > zeros, I think it also achieve the goal.
> > > 
> > > Roberto, removing the following code from init_digests() would be the
> > > equivalent to the prior code, without needing to make any other
> > > changes.  Let's keep it simple.  Do you want to post the patch with
> > > the change, or should I?
> > > 
> > >          ret = tpm_get_random(chip, digest, TPM_MAX_DIGEST_SIZE);
> > >          if (ret < 0)
> > >                  return ret;
> > >          if (ret < TPM_MAX_DIGEST_SIZE)
> > >                  return -EFAULT;
> > > 
> > > As I can't duplicate the problem, it would need to be tested by others
> > > experiencing the problem.
> > 
> > The bug reporter tested Roberto's last patch:
> > 
> >   https://bugzilla.kernel.org/show_bug.cgi?id=203953#c8
> > 
> > We should Cc the reporter on future patches or at least leave another
> > testing request in the bugzilla.
> 
> I don't see the reporter's email. Please ask him to test the new patch.

Done!

Tyler

> 
> Thanks
> 
> Roberto
> 
> -- 
> HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
> Managing Director: Li Peng, Li Jian, Shi Yanli

Re: [PATCH] KEYS: trusted: allow module init if TPM is inactive or deactivated

[Jarkko Sakkinen]

On Mon, Aug 05, 2019 at 04:50:35PM +0200, Roberto Sassu wrote:
> I also don't think it is a good idea to remove this functionality.
> 
> Jarkko, we were discussing about this issue in another thread, and your
> answer then (https://lkml.org/lkml/2019/3/21/396) was that it is a
> priority to fix the regression.

OK, I think I'm now in the same page. I guess vacation did the job.

/Jarkko

Re: [PATCH] KEYS: trusted: allow module init if TPM is inactive or deactivated

[Jarkko Sakkinen]

On Fri, Aug 02, 2019 at 10:21:16AM +0200, Roberto Sassu wrote:
> On 8/1/2019 6:32 PM, Jarkko Sakkinen wrote:
> > On Mon, Jul 15, 2019 at 06:44:28PM +0200, Roberto Sassu wrote:
> > > According to the bug report at https://bugs.archlinux.org/task/62678,
> > > the trusted module is a dependency of the ecryptfs module. We should
> > > load the trusted module even if the TPM is inactive or deactivated.
> > > 
> > > Given that commit 782779b60faa ("tpm: Actually fail on TPM errors during
> > > "get random"") changes the return code of tpm_get_random(), the patch
> > > should be modified to ignore the -EIO error. I will send a new version.
> > 
> > Do you have information where this dependency comes from?
> 
> ecryptfs retrieves the encryption key from encrypted keys (see
> ecryptfs_get_encrypted_key()).

So... what is preventing removing this requirement "in the source"?

/Jarkko