| Message ID | 20170329102452.32212-5-roberto.sassu@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Mar 29, 2017 at 12:24:52PM +0200, Roberto Sassu wrote: > Allow TPM users to provide a digest for each PCR bank, > for the extend operation. > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> Not used for anything. Thus NAK. /Jarkko > --- > drivers/char/tpm/tpm-interface.c | 31 +++++++++++++++++++++++++++++++ > drivers/char/tpm/tpm.h | 6 ------ > include/linux/tpm.h | 14 ++++++++++++++ > 3 files changed, 45 insertions(+), 6 deletions(-) > > diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c > index 44e7c99..99789b2 100644 > --- a/drivers/char/tpm/tpm-interface.c > +++ b/drivers/char/tpm/tpm-interface.c > @@ -876,6 +876,37 @@ int tpm_pcr_extend(u32 chip_num, int pcr_idx, const u8 *hash) > EXPORT_SYMBOL_GPL(tpm_pcr_extend); > > /** > + * tpm_pcr_extend_digests - extend pcr banks values with provided digests values > + * @chip_num: tpm idx # or ANY > + * @pcr_idx: pcr idx to extend > + * @count: size of array > + * @digests: array of tpm2_digest structures > + * > + * The TPM driver should be built-in, but for whatever reason it > + * isn't, protect against the chip disappearing, by incrementing > + * the module usage count. > + */ > +int tpm_pcr_extend_digests(u32 chip_num, int pcr_idx, u32 count, > + struct tpm2_digest *digests) > +{ > + struct tpm_chip *chip; > + int rc = -ENODEV; > + > + chip = tpm_chip_find_get(chip_num); > + if (chip == NULL) > + return rc; > + > + if (!(chip->flags & TPM_CHIP_FLAG_TPM2)) > + goto out; > + > + rc = tpm2_pcr_extend(chip, pcr_idx, count, digests); > +out: > + tpm_put_ops(chip); > + return rc; > +} > +EXPORT_SYMBOL_GPL(tpm_pcr_extend_digests); > + > +/** > * tpm_pcr_algorithms - get TPM IDs of active PCR banks algorithms > * @chip_num: tpm idx # or ANY > * @algorithms: array of TPM IDs > diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h > index f15279b..e130b6d 100644 > --- a/drivers/char/tpm/tpm.h > +++ b/drivers/char/tpm/tpm.h > @@ -34,7 +34,6 @@ > #include <linux/acpi.h> > #include <linux/cdev.h> > #include <linux/highmem.h> > -#include <crypto/hash_info.h> > > enum tpm_const { > TPM_MINOR = 224, /* officially assigned */ > @@ -373,11 +372,6 @@ struct tpm_cmd_t { > tpm_cmd_params params; > } __packed; > > -struct tpm2_digest { > - u16 alg_id; > - u8 digest[SHA512_DIGEST_SIZE]; > -} __packed; > - > /* A string buffer type for constructing TPM commands. This is based on the > * ideas of string buffer code in security/keys/trusted.h but is heap based > * in order to keep the stack usage minimal. > diff --git a/include/linux/tpm.h b/include/linux/tpm.h > index 6552e43..3e38112 100644 > --- a/include/linux/tpm.h > +++ b/include/linux/tpm.h > @@ -22,6 +22,8 @@ > #ifndef __LINUX_TPM_H__ > #define __LINUX_TPM_H__ > > +#include <crypto/hash_info.h> > + > #define TPM_DIGEST_SIZE 20 /* Max TPM v1.2 PCR size */ > #define TPM_ACTIVE_BANKS_MAX 7 /* Max num of active banks for TPM 2.0 */ > > @@ -34,6 +36,11 @@ struct tpm_chip; > struct trusted_key_payload; > struct trusted_key_options; > > +struct tpm2_digest { > + u16 alg_id; > + u8 digest[SHA512_DIGEST_SIZE]; > +} __packed; > + > enum TPM_OPS_FLAGS { > TPM_OPS_AUTO_STARTUP = BIT(0), > }; > @@ -70,6 +77,8 @@ extern enum tpm2_algorithms tpm2_pcr_algo_from_crypto(enum hash_algo crypto_id); > extern int tpm_is_tpm2(u32 chip_num); > extern int tpm_pcr_read(u32 chip_num, int pcr_idx, u8 *res_buf); > extern int tpm_pcr_extend(u32 chip_num, int pcr_idx, const u8 *hash); > +extern int tpm_pcr_extend_digests(u32 chip_num, int pcr_idx, u32 count, > + struct tpm2_digest *digests); > extern int tpm_pcr_algorithms(u32 chip_num, u32 count, > enum tpm2_algorithms *algorithms); > extern int tpm_send(u32 chip_num, void *cmd, size_t buflen); > @@ -100,6 +109,11 @@ static inline int tpm_pcr_read(u32 chip_num, int pcr_idx, u8 *res_buf) { > static inline int tpm_pcr_extend(u32 chip_num, int pcr_idx, const u8 *hash) { > return -ENODEV; > } > +static inline int tpm_pcr_extend_digests(u32 chip_num, int pcr_idx, u32 count, > + struct tpm2_digest *digests) > +{ > + return -ENODEV; > +} > static inline int tpm_pcr_algorithms(u32 chip_num, u32 count, > enum tpm2_algorithms *algorithms) > { > -- > 2.9.3 > > > ------------------------------------------------------------------------------ > Check out the vibrant tech community on one of the world's most > engaging tech sites, Slashdot.org! http://sdm.link/slashdot > _______________________________________________ > tpmdd-devel mailing list > tpmdd-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/tpmdd-devel ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
On 4/5/2017 2:14 PM, Jarkko Sakkinen wrote: > On Wed, Mar 29, 2017 at 12:24:52PM +0200, Roberto Sassu wrote: >> Allow TPM users to provide a digest for each PCR bank, >> for the extend operation. >> >> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> > > Not used for anything. Thus NAK. As I mentioned earlier, it is used in this patch: https://sourceforge.net/p/linux-ima/mailman/message/35757195/ tpm_pcr_algorithms() and tpm2_pcr_algo_to_crypto() are used in this patch: https://sourceforge.net/p/linux-ima/mailman/message/35757194/ tpm2_pcr_algo_from_crypto() is used here: https://sourceforge.net/p/tpmdd/mailman/message/35756304/ Roberto ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
On 4/5/2017 3:50 PM, Roberto Sassu wrote: > As I mentioned earlier, it is used in this patch: > > https://sourceforge.net/p/linux-ima/mailman/message/35757195/ I have a question. As you can see in the IMA patch, I'm calling tpm_is_tpm2() to determine if I should invoke tpm_pcr_extend(), for TPM 1.2, or tpm_pcr_extend_digests(), for TPM 2.0. Should the new function work with TPM 1.2? If a tpm2_digest structure with a SHA1 digest is provided, I could call tpm_pcr_extend() instead of returning an error. Thanks Roberto ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
On Wed, Apr 05, 2017 at 03:50:08PM +0200, Roberto Sassu wrote: > On 4/5/2017 2:14 PM, Jarkko Sakkinen wrote: > > On Wed, Mar 29, 2017 at 12:24:52PM +0200, Roberto Sassu wrote: > > > Allow TPM users to provide a digest for each PCR bank, > > > for the extend operation. > > > > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> > > > > Not used for anything. Thus NAK. > > As I mentioned earlier, it is used in this patch: > > https://sourceforge.net/p/linux-ima/mailman/message/35757195/ > > > tpm_pcr_algorithms() and tpm2_pcr_algo_to_crypto() are used > in this patch: > > https://sourceforge.net/p/linux-ima/mailman/message/35757194/ > > > tpm2_pcr_algo_from_crypto() is used here: > > https://sourceforge.net/p/tpmdd/mailman/message/35756304/ > > Roberto Please describe these in the next version so that we can with reasonable effort evaluate these. /Jarkko ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
On 4/5/2017 4:36 PM, Roberto Sassu wrote: > I have a question. As you can see in the IMA patch, I'm calling > tpm_is_tpm2() to determine if I should invoke tpm_pcr_extend(), > for TPM 1.2, or tpm_pcr_extend_digests(), for TPM 2.0. > > Should the new function work with TPM 1.2? If a tpm2_digest > structure with a SHA1 digest is provided, I could call > tpm_pcr_extend() instead of returning an error. Hi Jarkko would you have any objection if the new functions work regardless of the TPM version? Thanks Roberto ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
On Fri, Apr 07, 2017 at 11:50:49AM +0200, Roberto Sassu wrote: > On 4/5/2017 4:36 PM, Roberto Sassu wrote: > > I have a question. As you can see in the IMA patch, I'm calling > > tpm_is_tpm2() to determine if I should invoke tpm_pcr_extend(), > > for TPM 1.2, or tpm_pcr_extend_digests(), for TPM 2.0. > > > > Should the new function work with TPM 1.2? If a tpm2_digest > > structure with a SHA1 digest is provided, I could call > > tpm_pcr_extend() instead of returning an error. > > Hi Jarkko > > would you have any objection if the new functions work > regardless of the TPM version? > > Thanks > > Roberto Yes, you should not add multiple functions that do the same thing essentially. Please rework tpm_pcr_extend instead. And while you are doing it, please also rework it to use tpm_buf for everything. /Jarkko ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
On Fri, Apr 07, 2017 at 10:31:56PM +0300, Jarkko Sakkinen wrote: > On Fri, Apr 07, 2017 at 11:50:49AM +0200, Roberto Sassu wrote: > > On 4/5/2017 4:36 PM, Roberto Sassu wrote: > > > I have a question. As you can see in the IMA patch, I'm calling > > > tpm_is_tpm2() to determine if I should invoke tpm_pcr_extend(), > > > for TPM 1.2, or tpm_pcr_extend_digests(), for TPM 2.0. > > > > > > Should the new function work with TPM 1.2? If a tpm2_digest > > > structure with a SHA1 digest is provided, I could call > > > tpm_pcr_extend() instead of returning an error. > > > > Hi Jarkko > > > > would you have any objection if the new functions work > > regardless of the TPM version? > > > > Thanks > > > > Roberto > > Yes, you should not add multiple functions that do the same thing > essentially. Please rework tpm_pcr_extend instead. > > And while you are doing it, please also rework it to use tpm_buf > for everything. > > /Jarkko Some prework is required before you implement your new things. 1. tpm1_pcr_extend() to tpm-interface.c that is called by tpm_pcr_extend() and make it use tpm_buf. (1 commit) 2. There's a race condition bug in the way Nayna has implemented the digest list extension. It takes and releases tpm_mutex multiple times. This bug needs to be fixed before any other changes are justified (1 commit). Please add the Fixes line to the commit message. For (2) you should probably rename the existing tpm2_pcr_extend() as tpm2_pcr_extend_bank() and change it as a static function. That functio should take tpm_transmit flags as the last parameter. Then implement tpm2_pcr_extend() that does the same thing as is done now inside tpm_pcr_extend(). Call tpm2_pcr_extend_bank() inside that function with TPM_TRANSMIT_UNLOCKED. You should make this as its own patch set without any of the new additions. Only after these fixes are landed I'm ready to review any new extensions to tpm_pcr_extend(). PS I *purposely* have not read any of the IMA links that you have sent to me. You should be able to explain these changes without requiring such action. /Jarkko ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
On Fri, Apr 07, 2017 at 11:10:37PM +0300, Jarkko Sakkinen wrote: > On Fri, Apr 07, 2017 at 10:31:56PM +0300, Jarkko Sakkinen wrote: > > On Fri, Apr 07, 2017 at 11:50:49AM +0200, Roberto Sassu wrote: > > > On 4/5/2017 4:36 PM, Roberto Sassu wrote: > > > > I have a question. As you can see in the IMA patch, I'm calling > > > > tpm_is_tpm2() to determine if I should invoke tpm_pcr_extend(), > > > > for TPM 1.2, or tpm_pcr_extend_digests(), for TPM 2.0. > > > > > > > > Should the new function work with TPM 1.2? If a tpm2_digest > > > > structure with a SHA1 digest is provided, I could call > > > > tpm_pcr_extend() instead of returning an error. > > > > > > Hi Jarkko > > > > > > would you have any objection if the new functions work > > > regardless of the TPM version? > > > > > > Thanks > > > > > > Roberto > > > > Yes, you should not add multiple functions that do the same thing > > essentially. Please rework tpm_pcr_extend instead. > > > > And while you are doing it, please also rework it to use tpm_buf > > for everything. > > > > /Jarkko > > Some prework is required before you implement your new things. > > 1. tpm1_pcr_extend() to tpm-interface.c that is called by > tpm_pcr_extend() and make it use tpm_buf. (1 commit) > > 2. There's a race condition bug in the way Nayna has implemented the > digest list extension. It takes and releases tpm_mutex multiple times. > This bug needs to be fixed before any other changes are justified > (1 commit). Please add the Fixes line to the commit message. > > For (2) you should probably rename the existing tpm2_pcr_extend() as > tpm2_pcr_extend_bank() and change it as a static function. That > functio should take tpm_transmit flags as the last parameter. Then > implement tpm2_pcr_extend() that does the same thing as is done now > inside tpm_pcr_extend(). Call tpm2_pcr_extend_bank() inside that > function with TPM_TRANSMIT_UNLOCKED. > > You should make this as its own patch set without any of the new > additions. Only after these fixes are landed I'm ready to review > any new extensions to tpm_pcr_extend(). > > PS I *purposely* have not read any of the IMA links that you have sent > to me. You should be able to explain these changes without requiring > such action. > > /Jarkko And there was one big problem in your first patches you did not have the RFC tag and still you did not include the kernel mailing list. I won't apply or give reviewed-by to any patches that do not linux-kernel and for non-trivial changes by defacto also include linux-security-module. /Jarkko ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
On 4/7/2017 9:31 PM, Jarkko Sakkinen wrote: >> would you have any objection if the new functions work >> regardless of the TPM version? > > Yes, you should not add multiple functions that do the same thing > essentially. Please rework tpm_pcr_extend instead. This means that callers of tpm_pcr_extend() (pcrlock() in security/keys/trusted.c) should be modified too, as the parameters will change. Also, tpm2_algorithms and tpm2_digest, the new arguments of tpm_pcr_extend(), should be renamed to tpm_*, since that function will be used regardless of the TPM version. Another problem is how to handle the general case when not all digests for PCR banks are provided. tpm_pcr_extend() pads the provided SHA1 digest to extend remaining banks. If multiple digests can be passed to this function, the digest to be used to extend remaining banks would depend on the input passed by the caller. The general rule could be that the first digest is used in all cases. To avoid confusion, I wanted to introduce a new function for providing multiple digests. If the caller does not provide a digest for each bank, the function returns an error. > And while you are doing it, please also rework it to use tpm_buf > for everything. tpm_buf_init() should be modified, to be used for TPM 1.2 commands. tag and ordinal should be written to the buffer in little endian. Roberto ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
On 4/7/2017 10:10 PM, Jarkko Sakkinen wrote: > 2. There's a race condition bug in the way Nayna has implemented the > digest list extension. It takes and releases tpm_mutex multiple times. > This bug needs to be fixed before any other changes are justified > (1 commit). Please add the Fixes line to the commit message. Isn't tpm_transmit_cmd() called only once? Roberto ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
On Mon, Apr 10, 2017 at 01:46:40PM +0200, Roberto Sassu wrote: > On 4/7/2017 9:31 PM, Jarkko Sakkinen wrote: > > > would you have any objection if the new functions work > > > regardless of the TPM version? > > > > Yes, you should not add multiple functions that do the same thing > > essentially. Please rework tpm_pcr_extend instead. > > This means that callers of tpm_pcr_extend() (pcrlock() > in security/keys/trusted.c) should be modified too, > as the parameters will change. Yes. You need to do that. > Also, tpm2_algorithms and tpm2_digest, the new arguments of > tpm_pcr_extend(), should be renamed to tpm_*, since > that function will be used regardless of the TPM version. Please do not rename enum tpm2_algorithm as those are the actual TPM 2.0 algorithm identifiers. > tpm_buf_init() should be modified, to be used for TPM 1.2 commands. > tag and ordinal should be written to the buffer in little endian. This is not true. They are in big-endian byte order. > Roberto /Jarkko ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
On Mon, Apr 10, 2017 at 01:51:13PM +0200, Roberto Sassu wrote: > On 4/7/2017 10:10 PM, Jarkko Sakkinen wrote: > > 2. There's a race condition bug in the way Nayna has implemented the > > digest list extension. It takes and releases tpm_mutex multiple times. > > This bug needs to be fixed before any other changes are justified > > (1 commit). Please add the Fixes line to the commit message. > > Isn't tpm_transmit_cmd() called only once? > > Roberto You are correct (sorry Nayna) :-) /Jarkko ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c index 44e7c99..99789b2 100644 --- a/drivers/char/tpm/tpm-interface.c +++ b/drivers/char/tpm/tpm-interface.c @@ -876,6 +876,37 @@ int tpm_pcr_extend(u32 chip_num, int pcr_idx, const u8 *hash) EXPORT_SYMBOL_GPL(tpm_pcr_extend); /** + * tpm_pcr_extend_digests - extend pcr banks values with provided digests values + * @chip_num: tpm idx # or ANY + * @pcr_idx: pcr idx to extend + * @count: size of array + * @digests: array of tpm2_digest structures + * + * The TPM driver should be built-in, but for whatever reason it + * isn't, protect against the chip disappearing, by incrementing + * the module usage count. + */ +int tpm_pcr_extend_digests(u32 chip_num, int pcr_idx, u32 count, + struct tpm2_digest *digests) +{ + struct tpm_chip *chip; + int rc = -ENODEV; + + chip = tpm_chip_find_get(chip_num); + if (chip == NULL) + return rc; + + if (!(chip->flags & TPM_CHIP_FLAG_TPM2)) + goto out; + + rc = tpm2_pcr_extend(chip, pcr_idx, count, digests); +out: + tpm_put_ops(chip); + return rc; +} +EXPORT_SYMBOL_GPL(tpm_pcr_extend_digests); + +/** * tpm_pcr_algorithms - get TPM IDs of active PCR banks algorithms * @chip_num: tpm idx # or ANY * @algorithms: array of TPM IDs diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h index f15279b..e130b6d 100644 --- a/drivers/char/tpm/tpm.h +++ b/drivers/char/tpm/tpm.h @@ -34,7 +34,6 @@ #include <linux/acpi.h> #include <linux/cdev.h> #include <linux/highmem.h> -#include <crypto/hash_info.h> enum tpm_const { TPM_MINOR = 224, /* officially assigned */ @@ -373,11 +372,6 @@ struct tpm_cmd_t { tpm_cmd_params params; } __packed; -struct tpm2_digest { - u16 alg_id; - u8 digest[SHA512_DIGEST_SIZE]; -} __packed; - /* A string buffer type for constructing TPM commands. This is based on the * ideas of string buffer code in security/keys/trusted.h but is heap based * in order to keep the stack usage minimal. diff --git a/include/linux/tpm.h b/include/linux/tpm.h index 6552e43..3e38112 100644 --- a/include/linux/tpm.h +++ b/include/linux/tpm.h @@ -22,6 +22,8 @@ #ifndef __LINUX_TPM_H__ #define __LINUX_TPM_H__ +#include <crypto/hash_info.h> + #define TPM_DIGEST_SIZE 20 /* Max TPM v1.2 PCR size */ #define TPM_ACTIVE_BANKS_MAX 7 /* Max num of active banks for TPM 2.0 */ @@ -34,6 +36,11 @@ struct tpm_chip; struct trusted_key_payload; struct trusted_key_options; +struct tpm2_digest { + u16 alg_id; + u8 digest[SHA512_DIGEST_SIZE]; +} __packed; + enum TPM_OPS_FLAGS { TPM_OPS_AUTO_STARTUP = BIT(0), }; @@ -70,6 +77,8 @@ extern enum tpm2_algorithms tpm2_pcr_algo_from_crypto(enum hash_algo crypto_id); extern int tpm_is_tpm2(u32 chip_num); extern int tpm_pcr_read(u32 chip_num, int pcr_idx, u8 *res_buf); extern int tpm_pcr_extend(u32 chip_num, int pcr_idx, const u8 *hash); +extern int tpm_pcr_extend_digests(u32 chip_num, int pcr_idx, u32 count, + struct tpm2_digest *digests); extern int tpm_pcr_algorithms(u32 chip_num, u32 count, enum tpm2_algorithms *algorithms); extern int tpm_send(u32 chip_num, void *cmd, size_t buflen); @@ -100,6 +109,11 @@ static inline int tpm_pcr_read(u32 chip_num, int pcr_idx, u8 *res_buf) { static inline int tpm_pcr_extend(u32 chip_num, int pcr_idx, const u8 *hash) { return -ENODEV; } +static inline int tpm_pcr_extend_digests(u32 chip_num, int pcr_idx, u32 count, + struct tpm2_digest *digests) +{ + return -ENODEV; +} static inline int tpm_pcr_algorithms(u32 chip_num, u32 count, enum tpm2_algorithms *algorithms) {
Allow TPM users to provide a digest for each PCR bank, for the extend operation. Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> --- drivers/char/tpm/tpm-interface.c | 31 +++++++++++++++++++++++++++++++ drivers/char/tpm/tpm.h | 6 ------ include/linux/tpm.h | 14 ++++++++++++++ 3 files changed, 45 insertions(+), 6 deletions(-)