| Message ID | 20190705163735.11539-1-roberto.sassu@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KEYS: trusted: allow module init if TPM is inactive or deactivated | expand |
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 >
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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 >
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
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
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
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;
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(-)