| Message ID | 20190201100641.26936-7-roberto.sassu@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tpm: retrieve digest size of unknown algorithms from TPM | expand |
On Fri, Feb 01, 2019 at 11:06:41AM +0100, Roberto Sassu wrote: > Currently, tpm_pcr_extend() accepts as an input only a SHA1 digest. > > This patch replaces the hash parameter of tpm_pcr_extend() with an array of > tpm_digest structures, so that the caller can provide a digest for each PCR > bank currently allocated in the TPM. > > tpm_pcr_extend() will not extend banks for which no digest was provided, > as it happened before this patch, but instead it requires that callers > provide the full set of digests. Since the number of digests will always be > chip->nr_allocated_banks, the count parameter has been removed. > > Due to the API change, ima_pcr_extend() and pcrlock() have been modified. > Since the number of allocated banks is not known in advance, the memory for > the digests must be dynamically allocated. To avoid performance degradation > and to avoid that a PCR extend is not done due to lack of memory, the array > of tpm_digest structures is allocated by the users of the TPM driver at > initialization time. > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> /Jarkko
On Fri, Feb 01, 2019 at 03:39:49PM +0200, Jarkko Sakkinen wrote: > On Fri, Feb 01, 2019 at 11:06:41AM +0100, Roberto Sassu wrote: > > Currently, tpm_pcr_extend() accepts as an input only a SHA1 digest. > > > > This patch replaces the hash parameter of tpm_pcr_extend() with an array of > > tpm_digest structures, so that the caller can provide a digest for each PCR > > bank currently allocated in the TPM. > > > > tpm_pcr_extend() will not extend banks for which no digest was provided, > > as it happened before this patch, but instead it requires that callers > > provide the full set of digests. Since the number of digests will always be > > chip->nr_allocated_banks, the count parameter has been removed. > > > > Due to the API change, ima_pcr_extend() and pcrlock() have been modified. > > Since the number of allocated banks is not known in advance, the memory for > > the digests must be dynamically allocated. To avoid performance degradation > > and to avoid that a PCR extend is not done due to lack of memory, the array > > of tpm_digest structures is allocated by the users of the TPM driver at > > initialization time. > > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> > > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> I tested that this does not break TPM. I'd need someone to check that this does not break IMA. After that, I'm ready to apply this series. /Jarkko
On Fri, 2019-02-01 at 15:41 +0200, Jarkko Sakkinen wrote: > On Fri, Feb 01, 2019 at 03:39:49PM +0200, Jarkko Sakkinen wrote: > > On Fri, Feb 01, 2019 at 11:06:41AM +0100, Roberto Sassu wrote: > > > Currently, tpm_pcr_extend() accepts as an input only a SHA1 digest. > > > > > > This patch replaces the hash parameter of tpm_pcr_extend() with an array of > > > tpm_digest structures, so that the caller can provide a digest for each PCR > > > bank currently allocated in the TPM. > > > > > > tpm_pcr_extend() will not extend banks for which no digest was provided, > > > as it happened before this patch, but instead it requires that callers > > > provide the full set of digests. Since the number of digests will always be > > > chip->nr_allocated_banks, the count parameter has been removed. > > > > > > Due to the API change, ima_pcr_extend() and pcrlock() have been modified. > > > Since the number of allocated banks is not known in advance, the memory for > > > the digests must be dynamically allocated. To avoid performance degradation > > > and to avoid that a PCR extend is not done due to lack of memory, the array > > > of tpm_digest structures is allocated by the users of the TPM driver at > > > initialization time. > > > > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> > > > > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > > Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > > I tested that this does not break TPM. I'd need someone to check that > this does not break IMA. After that, I'm ready to apply this series. Thanks! I just finished compiling, rebooting, and verifying the IMA boot-aggregate matches. While compiling, I saw some messages that "TPM_BUFSIZE is redefined" for tpm_i2c_infineon, tpm_i2c_nuvoton, and st33zp24. Didn't look to see if this is a result of this patch set or not. The IMA boot-aggregate matches the PCRs. Tested-by: Mimi Zohar <zohar@linux.ibm.com> (on x86 for TPM 1.2 & PTT TPM 2.0)
On Fri, Feb 01, 2019 at 09:33:09AM -0500, Mimi Zohar wrote: > Thanks! I just finished compiling, rebooting, and verifying the IMA > boot-aggregate matches. > > While compiling, I saw some messages that "TPM_BUFSIZE is redefined" > for tpm_i2c_infineon, tpm_i2c_nuvoton, and st33zp24. Didn't look to > see if this is a result of this patch set or not. > > The IMA boot-aggregate matches the PCRs. > > Tested-by: Mimi Zohar <zohar@linux.ibm.com> (on x86 for TPM 1.2 & PTT > TPM 2.0) Thank you. I found the reason for that warning. It is redefined, for reason unknown to me in drivers/char/tpm/st33zp24/st33zp24.h (commit bf38b8710892). I'll post a fix to that commit ASAP. After that commit has been applied I can apply Roberto's patches. /Jarkko
On Fri, Feb 01, 2019 at 07:33:30PM +0200, Jarkko Sakkinen wrote: > On Fri, Feb 01, 2019 at 09:33:09AM -0500, Mimi Zohar wrote: > > Thanks! I just finished compiling, rebooting, and verifying the IMA > > boot-aggregate matches. > > > > While compiling, I saw some messages that "TPM_BUFSIZE is redefined" > > for tpm_i2c_infineon, tpm_i2c_nuvoton, and st33zp24. Didn't look to > > see if this is a result of this patch set or not. > > > > The IMA boot-aggregate matches the PCRs. > > > > Tested-by: Mimi Zohar <zohar@linux.ibm.com> (on x86 for TPM 1.2 & PTT > > TPM 2.0) > > Thank you. > > I found the reason for that warning. It is redefined, for reason unknown > to me in drivers/char/tpm/st33zp24/st33zp24.h (commit bf38b8710892). > > I'll post a fix to that commit ASAP. After that commit has been applied > I can apply Roberto's patches. Just sent. No need to any testing for that one. Just need one reviewed-by or even acked-by, and after that I will apply all Roberto's patches. /Jarkko
Hi Roberto, Sorry for the delayed review. A few comments inline below, minor suggestions. > diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h > index cc12f3449a72..e6b2dcb0846a 100644 > --- a/security/integrity/ima/ima.h > +++ b/security/integrity/ima/ima.h > @@ -56,6 +56,7 @@ extern int ima_policy_flag; > extern int ima_hash_algo; > extern int ima_appraise; > extern struct tpm_chip *ima_tpm_chip; > +extern struct tpm_digest *digests; > > /* IMA event related data */ > struct ima_event_data { > diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c > index 6bb42a9c5e47..296a965b11ef 100644 > --- a/security/integrity/ima/ima_init.c > +++ b/security/integrity/ima/ima_init.c > @@ -27,6 +27,7 @@ > /* name for boot aggregate entry */ > static const char boot_aggregate_name[] = "boot_aggregate"; > struct tpm_chip *ima_tpm_chip; > +struct tpm_digest *digests; "digests" is used in the new ima_init_digests() and in ima_pcr_extend(). It's nice that the initialization routines are grouped together here in ima_init.c, but wouldn't it better to define "digests" in ima_queued.c, where it is currently being used? "digests" could then be defined as static. > > /* Add the boot aggregate to the IMA measurement list and extend > * the PCR register. > @@ -104,6 +105,24 @@ void __init ima_load_x509(void) > } > #endif > > +int __init ima_init_digests(void) > +{ > + int i; > + > + if (!ima_tpm_chip) > + return 0; > + > + digests = kcalloc(ima_tpm_chip->nr_allocated_banks, sizeof(*digests), > + GFP_NOFS); > + if (!digests) > + return -ENOMEM; > + > + for (i = 0; i < ima_tpm_chip->nr_allocated_banks; i++) > + digests[i].alg_id = ima_tpm_chip->allocated_banks[i].alg_id; > + > + return 0; > +} > + > int __init ima_init(void) > { > int rc; > @@ -125,6 +144,9 @@ int __init ima_init(void) > > ima_load_kexec_buffer(); > > + rc = ima_init_digests(); Ok. Getting the tpm chip is at the beginning of this function. Deferring allocating "digests" to here, avoids having to free memory on failure. ima_load_kexec_buffer() restores prior measurements, but doesn't extend the TPM. For anyone reading the code, a short comment above ima_load_kexec_buffer() would make sense. Mimi > + if (rc != 0) > + return rc; > rc = ima_add_boot_aggregate(); /* boot aggregate must be first entry */ > if (rc != 0) > return rc; > diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c > index 0e41dc1df1d4..719e88ca58f6 100644 > --- a/security/integrity/ima/ima_queue.c > +++ b/security/integrity/ima/ima_queue.c > @@ -140,11 +140,15 @@ unsigned long ima_get_binary_runtime_size(void) > static int ima_pcr_extend(const u8 *hash, int pcr) > { > int result = 0; > + int i; > > if (!ima_tpm_chip) > return result; > > - result = tpm_pcr_extend(ima_tpm_chip, pcr, hash); > + for (i = 0; i < ima_tpm_chip->nr_allocated_banks; i++) > + memcpy(digests[i].digest, hash, TPM_DIGEST_SIZE); > + > + result = tpm_pcr_extend(ima_tpm_chip, pcr, digests); > if (result != 0) > pr_err("Error Communicating to TPM chip, result: %d\n", result); > return result; >
On 2/1/2019 8:15 PM, Mimi Zohar wrote: > Hi Roberto, > > Sorry for the delayed review. A few comments inline below, minor > suggestions. > >> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h >> index cc12f3449a72..e6b2dcb0846a 100644 >> --- a/security/integrity/ima/ima.h >> +++ b/security/integrity/ima/ima.h >> @@ -56,6 +56,7 @@ extern int ima_policy_flag; >> extern int ima_hash_algo; >> extern int ima_appraise; >> extern struct tpm_chip *ima_tpm_chip; >> +extern struct tpm_digest *digests; >> >> /* IMA event related data */ >> struct ima_event_data { >> diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c >> index 6bb42a9c5e47..296a965b11ef 100644 >> --- a/security/integrity/ima/ima_init.c >> +++ b/security/integrity/ima/ima_init.c >> @@ -27,6 +27,7 @@ >> /* name for boot aggregate entry */ >> static const char boot_aggregate_name[] = "boot_aggregate"; >> struct tpm_chip *ima_tpm_chip; >> +struct tpm_digest *digests; > > "digests" is used in the new ima_init_digests() and in > ima_pcr_extend(). It's nice that the initialization routines are > grouped together here in ima_init.c, but wouldn't it better to define > "digests" in ima_queued.c, where it is currently being used? > "digests" could then be defined as static. 'digests' and ima_init_digests() can be moved to ima_queue.c, but I have to add the definition of ima_init_digests() to ima.h. Should I do it? >> /* Add the boot aggregate to the IMA measurement list and extend >> * the PCR register. >> @@ -104,6 +105,24 @@ void __init ima_load_x509(void) >> } >> #endif >> >> +int __init ima_init_digests(void) >> +{ >> + int i; >> + >> + if (!ima_tpm_chip) >> + return 0; >> + >> + digests = kcalloc(ima_tpm_chip->nr_allocated_banks, sizeof(*digests), >> + GFP_NOFS); >> + if (!digests) >> + return -ENOMEM; >> + >> + for (i = 0; i < ima_tpm_chip->nr_allocated_banks; i++) >> + digests[i].alg_id = ima_tpm_chip->allocated_banks[i].alg_id; >> + >> + return 0; >> +} >> + >> int __init ima_init(void) >> { >> int rc; >> @@ -125,6 +144,9 @@ int __init ima_init(void) >> >> ima_load_kexec_buffer(); >> >> + rc = ima_init_digests(); > > Ok. Getting the tpm chip is at the beginning of this function. > Deferring allocating "digests" to here, avoids having to free memory > on failure. > > ima_load_kexec_buffer() restores prior measurements, but doesn't > extend the TPM. For anyone reading the code, a short comment above > ima_load_kexec_buffer() would make sense. Ok. Should I resend the last patch again with the fixes you suggested? Thanks Roberto > Mimi > >> + if (rc != 0) >> + return rc; >> rc = ima_add_boot_aggregate(); /* boot aggregate must be first entry */ >> if (rc != 0) >> return rc; >> diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c >> index 0e41dc1df1d4..719e88ca58f6 100644 >> --- a/security/integrity/ima/ima_queue.c >> +++ b/security/integrity/ima/ima_queue.c >> @@ -140,11 +140,15 @@ unsigned long ima_get_binary_runtime_size(void) >> static int ima_pcr_extend(const u8 *hash, int pcr) >> { >> int result = 0; >> + int i; >> >> if (!ima_tpm_chip) >> return result; >> >> - result = tpm_pcr_extend(ima_tpm_chip, pcr, hash); >> + for (i = 0; i < ima_tpm_chip->nr_allocated_banks; i++) >> + memcpy(digests[i].digest, hash, TPM_DIGEST_SIZE); >> + >> + result = tpm_pcr_extend(ima_tpm_chip, pcr, digests); >> if (result != 0) >> pr_err("Error Communicating to TPM chip, result: %d\n", result); >> return result; >> >
On Mon, Feb 04, 2019 at 10:14:38AM +0100, Roberto Sassu wrote: > On 2/1/2019 8:15 PM, Mimi Zohar wrote: > > Hi Roberto, > > > > Sorry for the delayed review. A few comments inline below, minor > > suggestions. > > > > > diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h > > > index cc12f3449a72..e6b2dcb0846a 100644 > > > --- a/security/integrity/ima/ima.h > > > +++ b/security/integrity/ima/ima.h > > > @@ -56,6 +56,7 @@ extern int ima_policy_flag; > > > extern int ima_hash_algo; > > > extern int ima_appraise; > > > extern struct tpm_chip *ima_tpm_chip; > > > +extern struct tpm_digest *digests; > > > /* IMA event related data */ > > > struct ima_event_data { > > > diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c > > > index 6bb42a9c5e47..296a965b11ef 100644 > > > --- a/security/integrity/ima/ima_init.c > > > +++ b/security/integrity/ima/ima_init.c > > > @@ -27,6 +27,7 @@ > > > /* name for boot aggregate entry */ > > > static const char boot_aggregate_name[] = "boot_aggregate"; > > > struct tpm_chip *ima_tpm_chip; > > > +struct tpm_digest *digests; > > > > "digests" is used in the new ima_init_digests() and in > > ima_pcr_extend(). It's nice that the initialization routines are > > grouped together here in ima_init.c, but wouldn't it better to define > > "digests" in ima_queued.c, where it is currently being used? > > "digests" could then be defined as static. > > 'digests' and ima_init_digests() can be moved to ima_queue.c, but I have > to add the definition of ima_init_digests() to ima.h. Should I do it? > > > > > /* Add the boot aggregate to the IMA measurement list and extend > > > * the PCR register. > > > @@ -104,6 +105,24 @@ void __init ima_load_x509(void) > > > } > > > #endif > > > +int __init ima_init_digests(void) > > > +{ > > > + int i; > > > + > > > + if (!ima_tpm_chip) > > > + return 0; > > > + > > > + digests = kcalloc(ima_tpm_chip->nr_allocated_banks, sizeof(*digests), > > > + GFP_NOFS); > > > + if (!digests) > > > + return -ENOMEM; > > > + > > > + for (i = 0; i < ima_tpm_chip->nr_allocated_banks; i++) > > > + digests[i].alg_id = ima_tpm_chip->allocated_banks[i].alg_id; > > > + > > > + return 0; > > > +} > > > + > > > int __init ima_init(void) > > > { > > > int rc; > > > @@ -125,6 +144,9 @@ int __init ima_init(void) > > > ima_load_kexec_buffer(); > > > + rc = ima_init_digests(); > > > > Ok. Getting the tpm chip is at the beginning of this function. > > Deferring allocating "digests" to here, avoids having to free memory > > on failure. > > > > ima_load_kexec_buffer() restores prior measurements, but doesn't > > extend the TPM. For anyone reading the code, a short comment above > > ima_load_kexec_buffer() would make sense. > > Ok. Should I resend the last patch again with the fixes you suggested? Send the full patch set. For me it is easier then to apply the series rather than cherry-picking patches from random versions of the patch set. /Jarkko
On Mon, 2019-02-04 at 14:07 +0200, Jarkko Sakkinen wrote: > On Mon, Feb 04, 2019 at 10:14:38AM +0100, Roberto Sassu wrote: > > On 2/1/2019 8:15 PM, Mimi Zohar wrote: > > > Hi Roberto, > > > > > > Sorry for the delayed review. A few comments inline below, minor > > > suggestions. > > > > > > > diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h > > > > index cc12f3449a72..e6b2dcb0846a 100644 > > > > --- a/security/integrity/ima/ima.h > > > > +++ b/security/integrity/ima/ima.h > > > > @@ -56,6 +56,7 @@ extern int ima_policy_flag; > > > > extern int ima_hash_algo; > > > > extern int ima_appraise; > > > > extern struct tpm_chip *ima_tpm_chip; > > > > +extern struct tpm_digest *digests; > > > > /* IMA event related data */ > > > > struct ima_event_data { > > > > diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c > > > > index 6bb42a9c5e47..296a965b11ef 100644 > > > > --- a/security/integrity/ima/ima_init.c > > > > +++ b/security/integrity/ima/ima_init.c > > > > @@ -27,6 +27,7 @@ > > > > /* name for boot aggregate entry */ > > > > static const char boot_aggregate_name[] = "boot_aggregate"; > > > > struct tpm_chip *ima_tpm_chip; > > > > +struct tpm_digest *digests; > > > > > > "digests" is used in the new ima_init_digests() and in > > > ima_pcr_extend(). It's nice that the initialization routines are > > > grouped together here in ima_init.c, but wouldn't it better to define > > > "digests" in ima_queued.c, where it is currently being used? > > > "digests" could then be defined as static. > > > > 'digests' and ima_init_digests() can be moved to ima_queue.c, but I have > > to add the definition of ima_init_digests() to ima.h. Should I do it? Yes, I think it is preferable, as it's defined as an __init. > > > > > > > > /* Add the boot aggregate to the IMA measurement list and extend > > > > * the PCR register. > > > > @@ -104,6 +105,24 @@ void __init ima_load_x509(void) > > > > } > > > > #endif > > > > +int __init ima_init_digests(void) > > > > +{ > > > > + int i; > > > > + > > > > + if (!ima_tpm_chip) > > > > + return 0; > > > > + > > > > + digests = kcalloc(ima_tpm_chip->nr_allocated_banks, sizeof(*digests), > > > > + GFP_NOFS); > > > > + if (!digests) > > > > + return -ENOMEM; > > > > + > > > > + for (i = 0; i < ima_tpm_chip->nr_allocated_banks; i++) > > > > + digests[i].alg_id = ima_tpm_chip->allocated_banks[i].alg_id; > > > > + > > > > + return 0; > > > > +} > > > > + > > > > int __init ima_init(void) > > > > { > > > > int rc; > > > > @@ -125,6 +144,9 @@ int __init ima_init(void) > > > > ima_load_kexec_buffer(); > > > > + rc = ima_init_digests(); > > > > > > Ok. Getting the tpm chip is at the beginning of this function. > > > Deferring allocating "digests" to here, avoids having to free memory > > > on failure. > > > > > > ima_load_kexec_buffer() restores prior measurements, but doesn't > > > extend the TPM. For anyone reading the code, a short comment above > > > ima_load_kexec_buffer() would make sense. > > > > Ok. Should I resend the last patch again with the fixes you suggested? > > Send the full patch set. For me it is easier then to apply the series > rather than cherry-picking patches from random versions of the patch > set. Jarkko, thanks. I've been running with previous versions of this patchset, and now with this latest version. Mimi
On 2/4/2019 1:07 PM, Jarkko Sakkinen wrote: > On Mon, Feb 04, 2019 at 10:14:38AM +0100, Roberto Sassu wrote: >> On 2/1/2019 8:15 PM, Mimi Zohar wrote: >>> Hi Roberto, >>> >>> Sorry for the delayed review. A few comments inline below, minor >>> suggestions. >>> >>>> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h >>>> index cc12f3449a72..e6b2dcb0846a 100644 >>>> --- a/security/integrity/ima/ima.h >>>> +++ b/security/integrity/ima/ima.h >>>> @@ -56,6 +56,7 @@ extern int ima_policy_flag; >>>> extern int ima_hash_algo; >>>> extern int ima_appraise; >>>> extern struct tpm_chip *ima_tpm_chip; >>>> +extern struct tpm_digest *digests; >>>> /* IMA event related data */ >>>> struct ima_event_data { >>>> diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c >>>> index 6bb42a9c5e47..296a965b11ef 100644 >>>> --- a/security/integrity/ima/ima_init.c >>>> +++ b/security/integrity/ima/ima_init.c >>>> @@ -27,6 +27,7 @@ >>>> /* name for boot aggregate entry */ >>>> static const char boot_aggregate_name[] = "boot_aggregate"; >>>> struct tpm_chip *ima_tpm_chip; >>>> +struct tpm_digest *digests; >>> >>> "digests" is used in the new ima_init_digests() and in >>> ima_pcr_extend(). It's nice that the initialization routines are >>> grouped together here in ima_init.c, but wouldn't it better to define >>> "digests" in ima_queued.c, where it is currently being used? >>> "digests" could then be defined as static. >> >> 'digests' and ima_init_digests() can be moved to ima_queue.c, but I have >> to add the definition of ima_init_digests() to ima.h. Should I do it? >> >> >>>> /* Add the boot aggregate to the IMA measurement list and extend >>>> * the PCR register. >>>> @@ -104,6 +105,24 @@ void __init ima_load_x509(void) >>>> } >>>> #endif >>>> +int __init ima_init_digests(void) >>>> +{ >>>> + int i; >>>> + >>>> + if (!ima_tpm_chip) >>>> + return 0; >>>> + >>>> + digests = kcalloc(ima_tpm_chip->nr_allocated_banks, sizeof(*digests), >>>> + GFP_NOFS); >>>> + if (!digests) >>>> + return -ENOMEM; >>>> + >>>> + for (i = 0; i < ima_tpm_chip->nr_allocated_banks; i++) >>>> + digests[i].alg_id = ima_tpm_chip->allocated_banks[i].alg_id; >>>> + >>>> + return 0; >>>> +} >>>> + >>>> int __init ima_init(void) >>>> { >>>> int rc; >>>> @@ -125,6 +144,9 @@ int __init ima_init(void) >>>> ima_load_kexec_buffer(); >>>> + rc = ima_init_digests(); >>> >>> Ok. Getting the tpm chip is at the beginning of this function. >>> Deferring allocating "digests" to here, avoids having to free memory >>> on failure. >>> >>> ima_load_kexec_buffer() restores prior measurements, but doesn't >>> extend the TPM. For anyone reading the code, a short comment above >>> ima_load_kexec_buffer() would make sense. >> >> Ok. Should I resend the last patch again with the fixes you suggested? > > Send the full patch set. For me it is easier then to apply the series > rather than cherry-picking patches from random versions of the patch > set. I can include your fix in patch 4/6, if you prefer. Roberto > /Jarkko >
On Mon, Feb 04, 2019 at 02:21:59PM +0100, Roberto Sassu wrote:
> I can include your fix in patch 4/6, if you prefer.
It is not really a question of my preference because the changes in
4/6 are not part of the bug fix.
/Jarkko
On Tue, Feb 05, 2019 at 01:26:40AM +0200, Jarkko Sakkinen wrote: > On Mon, Feb 04, 2019 at 02:21:59PM +0100, Roberto Sassu wrote: > > I can include your fix in patch 4/6, if you prefer. > > It is not really a question of my preference because the changes in > 4/6 are not part of the bug fix. e.g. fixes tags and stable backports /Jarkko
On 2/1/2019 8:15 PM, Mimi Zohar wrote: > Hi Roberto, > > Sorry for the delayed review. A few comments inline below, minor > suggestions. > >> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h >> index cc12f3449a72..e6b2dcb0846a 100644 >> --- a/security/integrity/ima/ima.h >> +++ b/security/integrity/ima/ima.h >> @@ -56,6 +56,7 @@ extern int ima_policy_flag; >> extern int ima_hash_algo; >> extern int ima_appraise; >> extern struct tpm_chip *ima_tpm_chip; >> +extern struct tpm_digest *digests; >> >> /* IMA event related data */ >> struct ima_event_data { >> diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c >> index 6bb42a9c5e47..296a965b11ef 100644 >> --- a/security/integrity/ima/ima_init.c >> +++ b/security/integrity/ima/ima_init.c >> @@ -27,6 +27,7 @@ >> /* name for boot aggregate entry */ >> static const char boot_aggregate_name[] = "boot_aggregate"; >> struct tpm_chip *ima_tpm_chip; >> +struct tpm_digest *digests; > > "digests" is used in the new ima_init_digests() and in > ima_pcr_extend(). It's nice that the initialization routines are > grouped together here in ima_init.c, but wouldn't it better to define > "digests" in ima_queued.c, where it is currently being used? > "digests" could then be defined as static. > >> >> /* Add the boot aggregate to the IMA measurement list and extend >> * the PCR register. >> @@ -104,6 +105,24 @@ void __init ima_load_x509(void) >> } >> #endif >> >> +int __init ima_init_digests(void) >> +{ >> + int i; >> + >> + if (!ima_tpm_chip) >> + return 0; >> + >> + digests = kcalloc(ima_tpm_chip->nr_allocated_banks, sizeof(*digests), >> + GFP_NOFS); >> + if (!digests) >> + return -ENOMEM; >> + >> + for (i = 0; i < ima_tpm_chip->nr_allocated_banks; i++) >> + digests[i].alg_id = ima_tpm_chip->allocated_banks[i].alg_id; >> + >> + return 0; >> +} >> + >> int __init ima_init(void) >> { >> int rc; >> @@ -125,6 +144,9 @@ int __init ima_init(void) >> >> ima_load_kexec_buffer(); >> >> + rc = ima_init_digests(); > > Ok. Getting the tpm chip is at the beginning of this function. > Deferring allocating "digests" to here, avoids having to free memory > on failure. If I understood the code correctly, ima_init() does not undo the actions of previous functions. For example, if ima_init_template() fails, ima_shash_tfm is not freed. Probably, this can be improved later. Roberto > ima_load_kexec_buffer() restores prior measurements, but doesn't > extend the TPM. For anyone reading the code, a short comment above > ima_load_kexec_buffer() would make sense. > > Mimi > >> + if (rc != 0) >> + return rc; >> rc = ima_add_boot_aggregate(); /* boot aggregate must be first entry */ >> if (rc != 0) >> return rc; >> diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c >> index 0e41dc1df1d4..719e88ca58f6 100644 >> --- a/security/integrity/ima/ima_queue.c >> +++ b/security/integrity/ima/ima_queue.c >> @@ -140,11 +140,15 @@ unsigned long ima_get_binary_runtime_size(void) >> static int ima_pcr_extend(const u8 *hash, int pcr) >> { >> int result = 0; >> + int i; >> >> if (!ima_tpm_chip) >> return result; >> >> - result = tpm_pcr_extend(ima_tpm_chip, pcr, hash); >> + for (i = 0; i < ima_tpm_chip->nr_allocated_banks; i++) >> + memcpy(digests[i].digest, hash, TPM_DIGEST_SIZE); >> + >> + result = tpm_pcr_extend(ima_tpm_chip, pcr, digests); >> if (result != 0) >> pr_err("Error Communicating to TPM chip, result: %d\n", result); >> return result; >> >
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c index cde2f7cbe0cf..1cef63d6519e 100644 --- a/drivers/char/tpm/tpm-interface.c +++ b/drivers/char/tpm/tpm-interface.c @@ -478,42 +478,34 @@ EXPORT_SYMBOL_GPL(tpm_pcr_read); * tpm_pcr_extend - extend a PCR value in SHA1 bank. * @chip: a &struct tpm_chip instance, %NULL for the default chip * @pcr_idx: the PCR to be retrieved - * @hash: the hash value used to extend the PCR value + * @digests: array of tpm_digest structures used to extend PCRs * - * Note: with TPM 2.0 extends also those banks for which no digest was - * specified in order to prevent malicious use of those PCR banks. + * Note: callers must pass a digest for every allocated PCR bank, in the same + * order of the banks in chip->allocated_banks. * * Return: same as with tpm_transmit_cmd() */ -int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash) +int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, + struct tpm_digest *digests) { int rc; - struct tpm_digest *digest_list; int i; chip = tpm_find_get_ops(chip); if (!chip) return -ENODEV; - if (chip->flags & TPM_CHIP_FLAG_TPM2) { - digest_list = kcalloc(chip->nr_allocated_banks, - sizeof(*digest_list), GFP_KERNEL); - if (!digest_list) - return -ENOMEM; - - for (i = 0; i < chip->nr_allocated_banks; i++) { - digest_list[i].alg_id = chip->allocated_banks[i].alg_id; - memcpy(digest_list[i].digest, hash, TPM_DIGEST_SIZE); - } + for (i = 0; i < chip->nr_allocated_banks; i++) + if (digests[i].alg_id != chip->allocated_banks[i].alg_id) + return -EINVAL; - rc = tpm2_pcr_extend(chip, pcr_idx, chip->nr_allocated_banks, - digest_list); - kfree(digest_list); + if (chip->flags & TPM_CHIP_FLAG_TPM2) { + rc = tpm2_pcr_extend(chip, pcr_idx, digests); tpm_put_ops(chip); return rc; } - rc = tpm1_pcr_extend(chip, pcr_idx, hash, + rc = tpm1_pcr_extend(chip, pcr_idx, digests[0].digest, "attempting extend a PCR value"); tpm_put_ops(chip); return rc; diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h index 001a626ca4c3..43835c2f1b09 100644 --- a/drivers/char/tpm/tpm.h +++ b/drivers/char/tpm/tpm.h @@ -458,7 +458,7 @@ static inline u32 tpm2_rc_value(u32 rc) int tpm2_get_timeouts(struct tpm_chip *chip); int tpm2_pcr_read(struct tpm_chip *chip, u32 pcr_idx, struct tpm_digest *digest, u16 *digest_size_ptr); -int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count, +int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, struct tpm_digest *digests); int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t max); void tpm2_flush_context_cmd(struct tpm_chip *chip, u32 handle, diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c index 760893957f13..dc115096e280 100644 --- a/drivers/char/tpm/tpm2-cmd.c +++ b/drivers/char/tpm/tpm2-cmd.c @@ -247,12 +247,11 @@ struct tpm2_null_auth_area { * * @chip: TPM chip to use. * @pcr_idx: index of the PCR. - * @count: number of digests passed. * @digests: list of pcr banks and corresponding digest values to extend. * * Return: Same as with tpm_transmit_cmd. */ -int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count, +int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, struct tpm_digest *digests) { struct tpm_buf buf; @@ -260,9 +259,6 @@ int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count, int rc; int i; - if (count > chip->nr_allocated_banks) - return -EINVAL; - rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_PCR_EXTEND); if (rc) return rc; @@ -277,9 +273,9 @@ int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count, tpm_buf_append_u32(&buf, sizeof(struct tpm2_null_auth_area)); tpm_buf_append(&buf, (const unsigned char *)&auth_area, sizeof(auth_area)); - tpm_buf_append_u32(&buf, count); + tpm_buf_append_u32(&buf, chip->nr_allocated_banks); - for (i = 0; i < count; i++) { + for (i = 0; i < chip->nr_allocated_banks; i++) { tpm_buf_append_u16(&buf, digests[i].alg_id); tpm_buf_append(&buf, (const unsigned char *)&digests[i].digest, chip->allocated_banks[i].digest_size); diff --git a/include/linux/tpm.h b/include/linux/tpm.h index f9fea1e4075e..6051d479a44d 100644 --- a/include/linux/tpm.h +++ b/include/linux/tpm.h @@ -170,7 +170,8 @@ struct tpm_chip { extern int tpm_is_tpm2(struct tpm_chip *chip); extern int tpm_pcr_read(struct tpm_chip *chip, u32 pcr_idx, struct tpm_digest *digest); -extern int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash); +extern int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, + struct tpm_digest *digests); extern int tpm_send(struct tpm_chip *chip, void *cmd, size_t buflen); extern int tpm_get_random(struct tpm_chip *chip, u8 *data, size_t max); extern int tpm_seal_trusted(struct tpm_chip *chip, @@ -193,7 +194,7 @@ static inline int tpm_pcr_read(struct tpm_chip *chip, int pcr_idx, } static inline int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, - const u8 *hash) + struct tpm_digest *digests) { return -ENODEV; } diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h index cc12f3449a72..e6b2dcb0846a 100644 --- a/security/integrity/ima/ima.h +++ b/security/integrity/ima/ima.h @@ -56,6 +56,7 @@ extern int ima_policy_flag; extern int ima_hash_algo; extern int ima_appraise; extern struct tpm_chip *ima_tpm_chip; +extern struct tpm_digest *digests; /* IMA event related data */ struct ima_event_data { diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c index 6bb42a9c5e47..296a965b11ef 100644 --- a/security/integrity/ima/ima_init.c +++ b/security/integrity/ima/ima_init.c @@ -27,6 +27,7 @@ /* name for boot aggregate entry */ static const char boot_aggregate_name[] = "boot_aggregate"; struct tpm_chip *ima_tpm_chip; +struct tpm_digest *digests; /* Add the boot aggregate to the IMA measurement list and extend * the PCR register. @@ -104,6 +105,24 @@ void __init ima_load_x509(void) } #endif +int __init ima_init_digests(void) +{ + int i; + + if (!ima_tpm_chip) + return 0; + + digests = kcalloc(ima_tpm_chip->nr_allocated_banks, sizeof(*digests), + GFP_NOFS); + if (!digests) + return -ENOMEM; + + for (i = 0; i < ima_tpm_chip->nr_allocated_banks; i++) + digests[i].alg_id = ima_tpm_chip->allocated_banks[i].alg_id; + + return 0; +} + int __init ima_init(void) { int rc; @@ -125,6 +144,9 @@ int __init ima_init(void) ima_load_kexec_buffer(); + rc = ima_init_digests(); + if (rc != 0) + return rc; rc = ima_add_boot_aggregate(); /* boot aggregate must be first entry */ if (rc != 0) return rc; diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c index 0e41dc1df1d4..719e88ca58f6 100644 --- a/security/integrity/ima/ima_queue.c +++ b/security/integrity/ima/ima_queue.c @@ -140,11 +140,15 @@ unsigned long ima_get_binary_runtime_size(void) static int ima_pcr_extend(const u8 *hash, int pcr) { int result = 0; + int i; if (!ima_tpm_chip) return result; - result = tpm_pcr_extend(ima_tpm_chip, pcr, hash); + for (i = 0; i < ima_tpm_chip->nr_allocated_banks; i++) + memcpy(digests[i].digest, hash, TPM_DIGEST_SIZE); + + result = tpm_pcr_extend(ima_tpm_chip, pcr, digests); if (result != 0) pr_err("Error Communicating to TPM chip, result: %d\n", result); return result; diff --git a/security/keys/trusted.c b/security/keys/trusted.c index 5b852263eae1..bcc9c6ead7fd 100644 --- a/security/keys/trusted.c +++ b/security/keys/trusted.c @@ -35,6 +35,7 @@ static const char hmac_alg[] = "hmac(sha1)"; static const char hash_alg[] = "sha1"; static struct tpm_chip *chip; +static struct tpm_digest *digests; struct sdesc { struct shash_desc shash; @@ -380,15 +381,10 @@ EXPORT_SYMBOL_GPL(trusted_tpm_send); */ static int pcrlock(const int pcrnum) { - unsigned char hash[SHA1_DIGEST_SIZE]; - int ret; - if (!capable(CAP_SYS_ADMIN)) return -EPERM; - ret = tpm_get_random(chip, hash, SHA1_DIGEST_SIZE); - if (ret != SHA1_DIGEST_SIZE) - return ret; - return tpm_pcr_extend(chip, pcrnum, hash) ? -EINVAL : 0; + + return tpm_pcr_extend(chip, pcrnum, digests) ? -EINVAL : 0; } /* @@ -1222,6 +1218,29 @@ static int __init trusted_shash_alloc(void) return ret; } +static int __init init_digests(void) +{ + u8 digest[TPM_MAX_DIGEST_SIZE]; + int ret; + int i; + + ret = tpm_get_random(chip, digest, TPM_MAX_DIGEST_SIZE); + if (ret < 0) + return ret; + if (ret < TPM_MAX_DIGEST_SIZE) + return -EFAULT; + + digests = kcalloc(chip->nr_allocated_banks, sizeof(*digests), + GFP_KERNEL); + if (!digests) + return -ENOMEM; + + for (i = 0; i < chip->nr_allocated_banks; i++) + memcpy(digests[i].digest, digest, TPM_MAX_DIGEST_SIZE); + + return 0; +} + static int __init init_trusted(void) { int ret; @@ -1229,15 +1248,20 @@ static int __init init_trusted(void) chip = tpm_default_chip(); if (!chip) return -ENOENT; - ret = trusted_shash_alloc(); + ret = init_digests(); if (ret < 0) goto err_put; + ret = trusted_shash_alloc(); + if (ret < 0) + goto err_free; ret = register_key_type(&key_type_trusted); if (ret < 0) goto err_release; return 0; err_release: trusted_shash_release(); +err_free: + kfree(digests); err_put: put_device(&chip->dev); return ret; @@ -1246,6 +1270,7 @@ static int __init init_trusted(void) static void __exit cleanup_trusted(void) { put_device(&chip->dev); + kfree(digests); trusted_shash_release(); unregister_key_type(&key_type_trusted); }
Currently, tpm_pcr_extend() accepts as an input only a SHA1 digest. This patch replaces the hash parameter of tpm_pcr_extend() with an array of tpm_digest structures, so that the caller can provide a digest for each PCR bank currently allocated in the TPM. tpm_pcr_extend() will not extend banks for which no digest was provided, as it happened before this patch, but instead it requires that callers provide the full set of digests. Since the number of digests will always be chip->nr_allocated_banks, the count parameter has been removed. Due to the API change, ima_pcr_extend() and pcrlock() have been modified. Since the number of allocated banks is not known in advance, the memory for the digests must be dynamically allocated. To avoid performance degradation and to avoid that a PCR extend is not done due to lack of memory, the array of tpm_digest structures is allocated by the users of the TPM driver at initialization time. Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> --- drivers/char/tpm/tpm-interface.c | 30 ++++++++-------------- drivers/char/tpm/tpm.h | 2 +- drivers/char/tpm/tpm2-cmd.c | 10 +++----- include/linux/tpm.h | 5 ++-- security/integrity/ima/ima.h | 1 + security/integrity/ima/ima_init.c | 22 ++++++++++++++++ security/integrity/ima/ima_queue.c | 6 ++++- security/keys/trusted.c | 41 ++++++++++++++++++++++++------ 8 files changed, 79 insertions(+), 38 deletions(-)