* [PATCH v4 0/1] add sysfs exports for TPM 2 PCR registers @ 2020-08-17 21:35 James Bottomley 2020-08-17 21:35 ` [PATCH v4 1/1] tpm: add sysfs exports for all banks of " James Bottomley 0 siblings, 1 reply; 54+ messages in thread From: James Bottomley @ 2020-08-17 21:35 UTC (permalink / raw) To: linux-integrity; +Cc: Mimi Zohar, Jarkko Sakkinen export TPM 2 PCRs via sysfs. We also agreed we should conform to sysfs rules of one value per file, which rules out the "pcrs" file format of TPM 1.2 which has every PCR value in the same file. I added these files using device groups, so one group per bank hash of the TPM. Using an emulator which supports a variety of hashes, you can see the structure of the group files: root@testdeb:~# ls -F /sys/class/tpm/tpm0/ dev pcr-sha1/ pcr-sha384/ power/ tpm_version_major device@ pcr-sha256/ pcr-sha512/ subsystem@ uevent As a future enhancement, we could use the group is_visible function to remove files corresponding to PCRs which don't exist. The reason this isn't present is that so far I've never seen a TPM with a missing PCR. James --- James Bottomley (1): tpm: add sysfs exports for all banks of PCR registers drivers/char/tpm/tpm-sysfs.c | 178 +++++++++++++++++++++++++++++++++++ include/linux/tpm.h | 9 +- 2 files changed, 186 insertions(+), 1 deletion(-) -- 2.26.2 ^ permalink raw reply [flat|nested] 54+ messages in thread
* [PATCH v4 1/1] tpm: add sysfs exports for all banks of PCR registers 2020-08-17 21:35 [PATCH v4 0/1] add sysfs exports for TPM 2 PCR registers James Bottomley @ 2020-08-17 21:35 ` James Bottomley 2020-08-18 16:12 ` Jarkko Sakkinen ` (2 more replies) 0 siblings, 3 replies; 54+ messages in thread From: James Bottomley @ 2020-08-17 21:35 UTC (permalink / raw) To: linux-integrity; +Cc: Mimi Zohar, Jarkko Sakkinen Create sysfs per hash groups with 24 PCR files in them one group, named pcr-<hash>, for each agile hash of the TPM. The files are plugged in to a PCR read function which is TPM version agnostic, so this works also for TPM 1.2 but the hash is only sha1 in that case. Note: the macros used to create the hashes emit spurious checkpatch warnings. Do not try to "fix" them as checkpatch recommends, otherwise they'll break. Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com> Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com> Tested-by: Thiago Jung Bauermann <bauerman@linux.ibm.com> --- v2: fix TPM 1.2 legacy links failure v3: fix warn on and add note to tpm_algorithms v4: reword commit and add tested-by --- drivers/char/tpm/tpm-sysfs.c | 178 +++++++++++++++++++++++++++++++++++ include/linux/tpm.h | 9 +- 2 files changed, 186 insertions(+), 1 deletion(-) diff --git a/drivers/char/tpm/tpm-sysfs.c b/drivers/char/tpm/tpm-sysfs.c index d52bf4df0bca..81a02200b207 100644 --- a/drivers/char/tpm/tpm-sysfs.c +++ b/drivers/char/tpm/tpm-sysfs.c @@ -348,11 +348,189 @@ static const struct attribute_group tpm2_dev_group = { .attrs = tpm2_dev_attrs, }; +struct tpm_pcr_attr { + int alg_id; + int pcr; + struct device_attribute attr; +}; + +#define to_tpm_pcr_attr(a) container_of(a, struct tpm_pcr_attr, attr) + +static ssize_t pcr_value_show(struct device *dev, + struct device_attribute *attr, + char *buf) +{ + struct tpm_pcr_attr *ha = to_tpm_pcr_attr(attr); + struct tpm_chip *chip = to_tpm_chip(dev); + struct tpm_digest digest; + int i; + int digest_size = 0; + int rc; + char *str = buf; + + for (i = 0; i < chip->nr_allocated_banks; i++) + if (ha->alg_id == chip->allocated_banks[i].alg_id) + digest_size = chip->allocated_banks[i].digest_size; + /* should never happen */ + if (!digest_size) + return -EINVAL; + + digest.alg_id = ha->alg_id; + rc = tpm_pcr_read(chip, ha->pcr, &digest); + if (rc) + return rc; + for (i = 0; i < digest_size; i++) + str += sprintf(str, "%02X", digest.digest[i]); + str += sprintf(str, "\n"); + + return str - buf; +} + +/* + * The following set of defines represents all the magic to build + * the per hash attribute groups for displaying each bank of PCRs. + * The only slight problem with this approach is that every PCR is + * hard coded to be present, so you don't know if an PCR is missing + * until a cat of the file returns -EINVAL + * + * Also note you must ignore checkpatch warnings in this macro + * code. This is deep macro magic that checkpatch.pl doesn't + * understand. + */ + +/* Note, this must match TPM2_PLATFORM_PCR which is fixed at 24. */ +#define _TPM_HELPER(_alg, _hash, F) \ + F(_alg, _hash, 0) \ + F(_alg, _hash, 1) \ + F(_alg, _hash, 2) \ + F(_alg, _hash, 3) \ + F(_alg, _hash, 4) \ + F(_alg, _hash, 5) \ + F(_alg, _hash, 6) \ + F(_alg, _hash, 7) \ + F(_alg, _hash, 8) \ + F(_alg, _hash, 9) \ + F(_alg, _hash, 10) \ + F(_alg, _hash, 11) \ + F(_alg, _hash, 12) \ + F(_alg, _hash, 13) \ + F(_alg, _hash, 14) \ + F(_alg, _hash, 15) \ + F(_alg, _hash, 16) \ + F(_alg, _hash, 17) \ + F(_alg, _hash, 18) \ + F(_alg, _hash, 19) \ + F(_alg, _hash, 20) \ + F(_alg, _hash, 21) \ + F(_alg, _hash, 22) \ + F(_alg, _hash, 23) + +/* ignore checkpatch warning about trailing ; in macro. */ +#define PCR_ATTR(_alg, _hash, _pcr) \ + static struct tpm_pcr_attr dev_attr_pcr_##_hash##_##_pcr = { \ + .alg_id = _alg, \ + .pcr = _pcr, \ + .attr = { \ + .attr = { \ + .name = __stringify(_pcr), \ + .mode = 0444 \ + }, \ + .show = pcr_value_show \ + } \ + }; + +#define PCR_ATTRS(_alg, _hash) \ + _TPM_HELPER(_alg, _hash, PCR_ATTR) + +/* ignore checkpatch warning about trailing , in macro. */ +#define PCR_ATTR_VAL(_alg, _hash, _pcr) \ + &dev_attr_pcr_##_hash##_##_pcr.attr.attr, + +#define PCR_ATTR_GROUP_ARRAY(_alg, _hash) \ + static struct attribute *pcr_group_attrs_##_hash[] = { \ + _TPM_HELPER(_alg, _hash, PCR_ATTR_VAL) \ + NULL \ + } + +#define PCR_ATTR_GROUP(_alg, _hash) \ + static struct attribute_group pcr_group_##_hash = { \ + .name = "pcr-" __stringify(_hash), \ + .attrs = pcr_group_attrs_##_hash \ + } + +#define PCR_ATTR_BUILD(_alg, _hash) \ + PCR_ATTRS(_alg, _hash) \ + PCR_ATTR_GROUP_ARRAY(_alg, _hash); \ + PCR_ATTR_GROUP(_alg, _hash) +/* + * End of macro structure to build an attribute group containing 24 + * PCR value files for each supported hash algorithm + */ + +/* + * The next set of macros implements the cleverness for each hash to + * build a static attribute group called pcr_group_<hash> which can be + * added to chip->groups[]. + * + * The first argument is the TPM algorithm id and the second is the + * hash used as both the suffix and the group name. Note: the group + * name is a directory in the top level tpm class with the name + * pcr-<hash>, so it must not clash with any other names already + * in the sysfs directory. + */ +PCR_ATTR_BUILD(TPM_ALG_SHA1, sha1); +PCR_ATTR_BUILD(TPM_ALG_SHA256, sha256); +PCR_ATTR_BUILD(TPM_ALG_SHA384, sha384); +PCR_ATTR_BUILD(TPM_ALG_SHA512, sha512); +PCR_ATTR_BUILD(TPM_ALG_SM3_256, sm3); + + void tpm_sysfs_add_device(struct tpm_chip *chip) { + int i; + WARN_ON(chip->groups_cnt != 0); + if (chip->flags & TPM_CHIP_FLAG_TPM2) chip->groups[chip->groups_cnt++] = &tpm2_dev_group; else chip->groups[chip->groups_cnt++] = &tpm1_dev_group; + + /* add one group for each bank hash */ + for (i = 0; i < chip->nr_allocated_banks; i++) { + switch (chip->allocated_banks[i].alg_id) { + case TPM_ALG_SHA1: + chip->groups[chip->groups_cnt++] = &pcr_group_sha1; + break; + case TPM_ALG_SHA256: + chip->groups[chip->groups_cnt++] = &pcr_group_sha256; + break; + case TPM_ALG_SHA384: + chip->groups[chip->groups_cnt++] = &pcr_group_sha384; + break; + case TPM_ALG_SHA512: + chip->groups[chip->groups_cnt++] = &pcr_group_sha512; + break; + case TPM_ALG_SM3_256: + chip->groups[chip->groups_cnt++] = &pcr_group_sm3; + break; + default: + /* + * If this warning triggers, send a patch to + * add both a PCR_ATTR_BUILD() macro above for + * the missing algorithm as well as an + * additional case in this switch statement. + */ + WARN(1, "TPM with unsupported bank algorthm 0x%04x", + chip->allocated_banks[i].alg_id); + break; + } + } + + /* + * This will only trigger if someone has added an additional + * hash to the tpm_algorithms enum without incrementing + * TPM_MAX_HASHES. + */ + WARN_ON(chip->groups_cnt > TPM_MAX_HASHES + 1); } diff --git a/include/linux/tpm.h b/include/linux/tpm.h index 3b5d455501c5..cc0b94dcf21e 100644 --- a/include/linux/tpm.h +++ b/include/linux/tpm.h @@ -31,6 +31,7 @@ struct tpm_chip; struct trusted_key_payload; struct trusted_key_options; +/* if you add a new hash to this, increment TPM_MAX_HASHES below */ enum tpm_algorithms { TPM_ALG_ERROR = 0x0000, TPM_ALG_SHA1 = 0x0004, @@ -42,6 +43,12 @@ enum tpm_algorithms { TPM_ALG_SM3_256 = 0x0012, }; +/* + * maximum number of hashing algorithms a TPM can have. This is + * basically a count of every hash in tpm_algorithms above + */ +#define TPM_MAX_HASHES 5 + struct tpm_digest { u16 alg_id; u8 digest[TPM_MAX_DIGEST_SIZE]; @@ -146,7 +153,7 @@ struct tpm_chip { struct dentry *bios_dir[TPM_NUM_EVENT_LOG_FILES]; - const struct attribute_group *groups[3]; + const struct attribute_group *groups[3 + TPM_MAX_HASHES]; unsigned int groups_cnt; u32 nr_allocated_banks; -- 2.26.2 ^ permalink raw reply related [flat|nested] 54+ messages in thread
* Re: [PATCH v4 1/1] tpm: add sysfs exports for all banks of PCR registers 2020-08-17 21:35 ` [PATCH v4 1/1] tpm: add sysfs exports for all banks of " James Bottomley @ 2020-08-18 16:12 ` Jarkko Sakkinen 2020-08-18 16:19 ` Jarkko Sakkinen 2020-09-14 17:41 ` Jarkko Sakkinen 2020-10-08 11:45 ` Petr Vorel 2 siblings, 1 reply; 54+ messages in thread From: Jarkko Sakkinen @ 2020-08-18 16:12 UTC (permalink / raw) To: James Bottomley; +Cc: linux-integrity, Mimi Zohar On Mon, Aug 17, 2020 at 02:35:06PM -0700, James Bottomley wrote: > Create sysfs per hash groups with 24 PCR files in them one group, > named pcr-<hash>, for each agile hash of the TPM. The files are > plugged in to a PCR read function which is TPM version agnostic, so > this works also for TPM 1.2 but the hash is only sha1 in that case. > > Note: the macros used to create the hashes emit spurious checkpatch > warnings. Do not try to "fix" them as checkpatch recommends, otherwise > they'll break. > > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com> > Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com> > Tested-by: Thiago Jung Bauermann <bauerman@linux.ibm.com> I have hard time understanding why this is required. You can grab the information through /dev/tpm0 just fine. /Jarkko ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v4 1/1] tpm: add sysfs exports for all banks of PCR registers 2020-08-18 16:12 ` Jarkko Sakkinen @ 2020-08-18 16:19 ` Jarkko Sakkinen 2020-08-18 16:26 ` Jarkko Sakkinen 2020-08-18 16:44 ` James Bottomley 0 siblings, 2 replies; 54+ messages in thread From: Jarkko Sakkinen @ 2020-08-18 16:19 UTC (permalink / raw) To: James Bottomley; +Cc: linux-integrity, Mimi Zohar On Tue, Aug 18, 2020 at 07:12:09PM +0300, Jarkko Sakkinen wrote: > On Mon, Aug 17, 2020 at 02:35:06PM -0700, James Bottomley wrote: > > Create sysfs per hash groups with 24 PCR files in them one group, > > named pcr-<hash>, for each agile hash of the TPM. The files are > > plugged in to a PCR read function which is TPM version agnostic, so > > this works also for TPM 1.2 but the hash is only sha1 in that case. > > > > Note: the macros used to create the hashes emit spurious checkpatch > > warnings. Do not try to "fix" them as checkpatch recommends, otherwise > > they'll break. > > > > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com> > > Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com> > > Tested-by: Thiago Jung Bauermann <bauerman@linux.ibm.com> > > I have hard time understanding why this is required. > > You can grab the information through /dev/tpm0 just fine. I just think it is principally wrong to add sysfs files if they don't have any measurable value other than perhaps some convenience. It is trival to write only a libc dependent program that outputs PCRs. I think this is essentially an user space problem that is getting sorted out with kernel code. /Jarkko ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v4 1/1] tpm: add sysfs exports for all banks of PCR registers 2020-08-18 16:19 ` Jarkko Sakkinen @ 2020-08-18 16:26 ` Jarkko Sakkinen 2020-08-18 16:46 ` Jason Gunthorpe 2020-08-18 16:44 ` James Bottomley 1 sibling, 1 reply; 54+ messages in thread From: Jarkko Sakkinen @ 2020-08-18 16:26 UTC (permalink / raw) To: James Bottomley, Jason Gunthorpe; +Cc: linux-integrity, Mimi Zohar On Tue, Aug 18, 2020 at 07:19:57PM +0300, Jarkko Sakkinen wrote: > On Tue, Aug 18, 2020 at 07:12:09PM +0300, Jarkko Sakkinen wrote: > > On Mon, Aug 17, 2020 at 02:35:06PM -0700, James Bottomley wrote: > > > Create sysfs per hash groups with 24 PCR files in them one group, > > > named pcr-<hash>, for each agile hash of the TPM. The files are > > > plugged in to a PCR read function which is TPM version agnostic, so > > > this works also for TPM 1.2 but the hash is only sha1 in that case. > > > > > > Note: the macros used to create the hashes emit spurious checkpatch > > > warnings. Do not try to "fix" them as checkpatch recommends, otherwise > > > they'll break. > > > > > > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com> > > > Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com> > > > Tested-by: Thiago Jung Bauermann <bauerman@linux.ibm.com> > > > > I have hard time understanding why this is required. > > > > You can grab the information through /dev/tpm0 just fine. > > I just think it is principally wrong to add sysfs files if they don't > have any measurable value other than perhaps some convenience. > > It is trival to write only a libc dependent program that outputs PCRs. > > I think this is essentially an user space problem that is getting sorted > out with kernel code. Jason, what do you make of this? I recall that it was you who I discussed with about this topic when TPM 2.0 support was first upstreamed. /Jarkko ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v4 1/1] tpm: add sysfs exports for all banks of PCR registers 2020-08-18 16:26 ` Jarkko Sakkinen @ 2020-08-18 16:46 ` Jason Gunthorpe 2020-08-18 18:26 ` Mimi Zohar 0 siblings, 1 reply; 54+ messages in thread From: Jason Gunthorpe @ 2020-08-18 16:46 UTC (permalink / raw) To: Jarkko Sakkinen; +Cc: James Bottomley, linux-integrity, Mimi Zohar On Tue, Aug 18, 2020 at 07:26:30PM +0300, Jarkko Sakkinen wrote: > On Tue, Aug 18, 2020 at 07:19:57PM +0300, Jarkko Sakkinen wrote: > > On Tue, Aug 18, 2020 at 07:12:09PM +0300, Jarkko Sakkinen wrote: > > > On Mon, Aug 17, 2020 at 02:35:06PM -0700, James Bottomley wrote: > > > > Create sysfs per hash groups with 24 PCR files in them one group, > > > > named pcr-<hash>, for each agile hash of the TPM. The files are > > > > plugged in to a PCR read function which is TPM version agnostic, so > > > > this works also for TPM 1.2 but the hash is only sha1 in that case. > > > > > > > > Note: the macros used to create the hashes emit spurious checkpatch > > > > warnings. Do not try to "fix" them as checkpatch recommends, otherwise > > > > they'll break. > > > > > > > > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com> > > > > Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com> > > > > Tested-by: Thiago Jung Bauermann <bauerman@linux.ibm.com> > > > > > > I have hard time understanding why this is required. > > > > > > You can grab the information through /dev/tpm0 just fine. > > > > I just think it is principally wrong to add sysfs files if they don't > > have any measurable value other than perhaps some convenience. > > > > It is trival to write only a libc dependent program that outputs PCRs. > > > > I think this is essentially an user space problem that is getting sorted > > out with kernel code. > > Jason, what do you make of this? I recall that it was you who I > discussed with about this topic when TPM 2.0 support was first > upstreamed. TPM 2.0 broke all the userspace so it made sense to get rid of the non-conforming sysfs files from TPM v1.x time as part of the userspace API. That was the main reason to not continue forward with PCR in userspace. As far as doing it properly as this patch does.. I agree with you that sysfs files should have some reason to be added, espcially if it causes quite big code cost as this does. eg to drive a udev rule decision. Why is PCRs so special it needs to be in sysfs? What is userspace going to do with this information? Jason ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v4 1/1] tpm: add sysfs exports for all banks of PCR registers 2020-08-18 16:46 ` Jason Gunthorpe @ 2020-08-18 18:26 ` Mimi Zohar 2020-08-18 18:36 ` Jason Gunthorpe 2020-08-19 22:01 ` Jarkko Sakkinen 0 siblings, 2 replies; 54+ messages in thread From: Mimi Zohar @ 2020-08-18 18:26 UTC (permalink / raw) To: Jason Gunthorpe, Jarkko Sakkinen; +Cc: James Bottomley, linux-integrity On Tue, 2020-08-18 at 13:46 -0300, Jason Gunthorpe wrote: > On Tue, Aug 18, 2020 at 07:26:30PM +0300, Jarkko Sakkinen wrote: > > On Tue, Aug 18, 2020 at 07:19:57PM +0300, Jarkko Sakkinen wrote: > > > On Tue, Aug 18, 2020 at 07:12:09PM +0300, Jarkko Sakkinen wrote: > > > > On Mon, Aug 17, 2020 at 02:35:06PM -0700, James Bottomley wrote: > > > > > Create sysfs per hash groups with 24 PCR files in them one group, > > > > > named pcr-<hash>, for each agile hash of the TPM. The files are > > > > > plugged in to a PCR read function which is TPM version agnostic, so > > > > > this works also for TPM 1.2 but the hash is only sha1 in that case. > > > > > > > > > > Note: the macros used to create the hashes emit spurious checkpatch > > > > > warnings. Do not try to "fix" them as checkpatch recommends, otherwise > > > > > they'll break. > > > > > > > > > > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com> > > > > > Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com> > > > > > Tested-by: Thiago Jung Bauermann <bauerman@linux.ibm.com> > > > > > > > > I have hard time understanding why this is required. > > > > > > > > You can grab the information through /dev/tpm0 just fine. > > > > > > I just think it is principally wrong to add sysfs files if they don't > > > have any measurable value other than perhaps some convenience. > > > > > > It is trival to write only a libc dependent program that outputs PCRs. > > > > > > I think this is essentially an user space problem that is getting sorted > > > out with kernel code. > > > > Jason, what do you make of this? I recall that it was you who I > > discussed with about this topic when TPM 2.0 support was first > > upstreamed. > > TPM 2.0 broke all the userspace so it made sense to get rid of the > non-conforming sysfs files from TPM v1.x time as part of the userspace > API. That was the main reason to not continue forward with PCR in > userspace. > > As far as doing it properly as this patch does.. I agree with you that > sysfs files should have some reason to be added, espcially if it > causes quite big code cost as this does. eg to drive a udev rule > decision. > > Why is PCRs so special it needs to be in sysfs? What is userspace > going to do with this information? The original IMA LTP "boot_aggregate" regression test is dependent on the exported TPM event log and PCRs. Similar support is needed for TPM 2.0. There isn't just a single userspace application for reading PCRs. As soon as we add support for one userspace application, support for the other applications is requested. So instead of a having a simple regression test with a single method of reading PCRs, we're now required to support multiple userspace applications. I'm definitely in favor of exporting the PCRs, just as the TPM 2.0 event log and version are now exported. Mimi ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v4 1/1] tpm: add sysfs exports for all banks of PCR registers 2020-08-18 18:26 ` Mimi Zohar @ 2020-08-18 18:36 ` Jason Gunthorpe 2020-08-18 18:55 ` Mimi Zohar ` (2 more replies) 2020-08-19 22:01 ` Jarkko Sakkinen 1 sibling, 3 replies; 54+ messages in thread From: Jason Gunthorpe @ 2020-08-18 18:36 UTC (permalink / raw) To: Mimi Zohar; +Cc: Jarkko Sakkinen, James Bottomley, linux-integrity On Tue, Aug 18, 2020 at 02:26:04PM -0400, Mimi Zohar wrote: > On Tue, 2020-08-18 at 13:46 -0300, Jason Gunthorpe wrote: > > On Tue, Aug 18, 2020 at 07:26:30PM +0300, Jarkko Sakkinen wrote: > > > On Tue, Aug 18, 2020 at 07:19:57PM +0300, Jarkko Sakkinen wrote: > > > > On Tue, Aug 18, 2020 at 07:12:09PM +0300, Jarkko Sakkinen wrote: > > > > > On Mon, Aug 17, 2020 at 02:35:06PM -0700, James Bottomley wrote: > > > > > > Create sysfs per hash groups with 24 PCR files in them one group, > > > > > > named pcr-<hash>, for each agile hash of the TPM. The files are > > > > > > plugged in to a PCR read function which is TPM version agnostic, so > > > > > > this works also for TPM 1.2 but the hash is only sha1 in that case. > > > > > > > > > > > > Note: the macros used to create the hashes emit spurious checkpatch > > > > > > warnings. Do not try to "fix" them as checkpatch recommends, otherwise > > > > > > they'll break. > > > > > > > > > > > > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com> > > > > > > Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com> > > > > > > Tested-by: Thiago Jung Bauermann <bauerman@linux.ibm.com> > > > > > > > > > > I have hard time understanding why this is required. > > > > > > > > > > You can grab the information through /dev/tpm0 just fine. > > > > > > > > I just think it is principally wrong to add sysfs files if they don't > > > > have any measurable value other than perhaps some convenience. > > > > > > > > It is trival to write only a libc dependent program that outputs PCRs. > > > > > > > > I think this is essentially an user space problem that is getting sorted > > > > out with kernel code. > > > > > > Jason, what do you make of this? I recall that it was you who I > > > discussed with about this topic when TPM 2.0 support was first > > > upstreamed. > > > > TPM 2.0 broke all the userspace so it made sense to get rid of the > > non-conforming sysfs files from TPM v1.x time as part of the userspace > > API. That was the main reason to not continue forward with PCR in > > userspace. > > > > As far as doing it properly as this patch does.. I agree with you that > > sysfs files should have some reason to be added, espcially if it > > causes quite big code cost as this does. eg to drive a udev rule > > decision. > > > > Why is PCRs so special it needs to be in sysfs? What is userspace > > going to do with this information? > > The original IMA LTP "boot_aggregate" regression test is dependent on > the exported TPM event log and PCRs. Similar support is needed for TPM > 2.0. There isn't just a single userspace application for reading > PCRs. As soon as we add support for one userspace > application, support for the other applications is requested. So > instead of a having a simple regression test with a single method of > reading PCRs, we're now required to support multiple userspace > applications. But this test already has a C program as part of the boot aggregate test, why is it such a problem to use a C program to also read the PCRs? As Jarkko says it is not so hard Jason ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v4 1/1] tpm: add sysfs exports for all banks of PCR registers 2020-08-18 18:36 ` Jason Gunthorpe @ 2020-08-18 18:55 ` Mimi Zohar 2020-08-19 12:02 ` Jason Gunthorpe 2020-08-19 22:14 ` Jarkko Sakkinen 2020-08-18 19:03 ` James Bottomley 2020-08-19 22:13 ` Jarkko Sakkinen 2 siblings, 2 replies; 54+ messages in thread From: Mimi Zohar @ 2020-08-18 18:55 UTC (permalink / raw) To: Jason Gunthorpe; +Cc: Jarkko Sakkinen, James Bottomley, linux-integrity On Tue, 2020-08-18 at 15:36 -0300, Jason Gunthorpe wrote: > On Tue, Aug 18, 2020 at 02:26:04PM -0400, Mimi Zohar wrote: > > On Tue, 2020-08-18 at 13:46 -0300, Jason Gunthorpe wrote: > > > On Tue, Aug 18, 2020 at 07:26:30PM +0300, Jarkko Sakkinen wrote: > > > > On Tue, Aug 18, 2020 at 07:19:57PM +0300, Jarkko Sakkinen wrote: > > > > > On Tue, Aug 18, 2020 at 07:12:09PM +0300, Jarkko Sakkinen wrote: > > > > > > On Mon, Aug 17, 2020 at 02:35:06PM -0700, James Bottomley wrote: > > > > > > > Create sysfs per hash groups with 24 PCR files in them one group, > > > > > > > named pcr-<hash>, for each agile hash of the TPM. The files are > > > > > > > plugged in to a PCR read function which is TPM version agnostic, so > > > > > > > this works also for TPM 1.2 but the hash is only sha1 in that case. > > > > > > > > > > > > > > Note: the macros used to create the hashes emit spurious checkpatch > > > > > > > warnings. Do not try to "fix" them as checkpatch recommends, otherwise > > > > > > > they'll break. > > > > > > > > > > > > > > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com> > > > > > > > Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com> > > > > > > > Tested-by: Thiago Jung Bauermann <bauerman@linux.ibm.com> > > > > > > > > > > > > I have hard time understanding why this is required. > > > > > > > > > > > > You can grab the information through /dev/tpm0 just fine. > > > > > > > > > > I just think it is principally wrong to add sysfs files if they don't > > > > > have any measurable value other than perhaps some convenience. > > > > > > > > > > It is trival to write only a libc dependent program that outputs PCRs. > > > > > > > > > > I think this is essentially an user space problem that is getting sorted > > > > > out with kernel code. > > > > > > > > Jason, what do you make of this? I recall that it was you who I > > > > discussed with about this topic when TPM 2.0 support was first > > > > upstreamed. > > > > > > TPM 2.0 broke all the userspace so it made sense to get rid of the > > > non-conforming sysfs files from TPM v1.x time as part of the userspace > > > API. That was the main reason to not continue forward with PCR in > > > userspace. > > > > > > As far as doing it properly as this patch does.. I agree with you that > > > sysfs files should have some reason to be added, espcially if it > > > causes quite big code cost as this does. eg to drive a udev rule > > > decision. > > > > > > Why is PCRs so special it needs to be in sysfs? What is userspace > > > going to do with this information? > > > > The original IMA LTP "boot_aggregate" regression test is dependent on > > the exported TPM event log and PCRs. Similar support is needed for TPM > > 2.0. There isn't just a single userspace application for reading > > PCRs. As soon as we add support for one userspace > > application, support for the other applications is requested. So > > instead of a having a simple regression test with a single method of > > reading PCRs, we're now required to support multiple userspace > > applications. > > But this test already has a C program as part of the boot aggregate > test, why is it such a problem to use a C program to also read the > PCRs? > > As Jarkko says it is not so hard The problem is that there isn't just one single userspace library or application for reading PCRs. So now not only is there the kernel "boot_aggregate" regression testing, but regression testing of the tool itself to support multiple methods of reading the PCRs. Mimi ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v4 1/1] tpm: add sysfs exports for all banks of PCR registers 2020-08-18 18:55 ` Mimi Zohar @ 2020-08-19 12:02 ` Jason Gunthorpe 2020-08-19 13:27 ` Mimi Zohar 2020-08-19 15:17 ` James Bottomley 2020-08-19 22:14 ` Jarkko Sakkinen 1 sibling, 2 replies; 54+ messages in thread From: Jason Gunthorpe @ 2020-08-19 12:02 UTC (permalink / raw) To: Mimi Zohar; +Cc: Jarkko Sakkinen, James Bottomley, linux-integrity On Tue, Aug 18, 2020 at 02:55:50PM -0400, Mimi Zohar wrote: > The problem is that there isn't just one single userspace library or > application for reading PCRs. So now not only is there the kernel > "boot_aggregate" regression testing, but regression testing of the tool > itself to support multiple methods of reading the PCRs. I was thinking just open code open("/dev/tpm") write(read_pcrs_cmd) read(read_pcrs_cmd) It isn't particularly hard to retrive the PCRs, don't really need to depend on a library. Jason ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v4 1/1] tpm: add sysfs exports for all banks of PCR registers 2020-08-19 12:02 ` Jason Gunthorpe @ 2020-08-19 13:27 ` Mimi Zohar 2020-08-19 14:09 ` Jason Gunthorpe 2020-08-19 22:15 ` Jarkko Sakkinen 2020-08-19 15:17 ` James Bottomley 1 sibling, 2 replies; 54+ messages in thread From: Mimi Zohar @ 2020-08-19 13:27 UTC (permalink / raw) To: Jason Gunthorpe; +Cc: Jarkko Sakkinen, James Bottomley, linux-integrity On Wed, 2020-08-19 at 09:02 -0300, Jason Gunthorpe wrote: > On Tue, Aug 18, 2020 at 02:55:50PM -0400, Mimi Zohar wrote: > > > The problem is that there isn't just one single userspace library or > > application for reading PCRs. So now not only is there the kernel > > "boot_aggregate" regression testing, but regression testing of the tool > > itself to support multiple methods of reading the PCRs. > > I was thinking just open code > open("/dev/tpm") > write(read_pcrs_cmd) > read(read_pcrs_cmd) > > It isn't particularly hard to retrive the PCRs, don't really need to > depend on a library. Ok, do you want to contribute it to ima-evm-utils? While you're at it, do you also have code to parse the TPM 2.0 event log that you could contribute? Seriously, we shouldn't be (re-)writing code to do this. Mimi ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v4 1/1] tpm: add sysfs exports for all banks of PCR registers 2020-08-19 13:27 ` Mimi Zohar @ 2020-08-19 14:09 ` Jason Gunthorpe 2020-08-19 14:53 ` Mimi Zohar 2020-08-19 14:56 ` Serge E. Hallyn 2020-08-19 22:15 ` Jarkko Sakkinen 1 sibling, 2 replies; 54+ messages in thread From: Jason Gunthorpe @ 2020-08-19 14:09 UTC (permalink / raw) To: Mimi Zohar; +Cc: Jarkko Sakkinen, James Bottomley, linux-integrity On Wed, Aug 19, 2020 at 09:27:33AM -0400, Mimi Zohar wrote: > On Wed, 2020-08-19 at 09:02 -0300, Jason Gunthorpe wrote: > > On Tue, Aug 18, 2020 at 02:55:50PM -0400, Mimi Zohar wrote: > > > > > The problem is that there isn't just one single userspace library or > > > application for reading PCRs. So now not only is there the kernel > > > "boot_aggregate" regression testing, but regression testing of the tool > > > itself to support multiple methods of reading the PCRs. > > > > I was thinking just open code > > open("/dev/tpm") > > write(read_pcrs_cmd) > > read(read_pcrs_cmd) > > > > It isn't particularly hard to retrive the PCRs, don't really need to > > depend on a library. > > Ok, do you want to contribute it to ima-evm-utils? While you're at it, > do you also have code to parse the TPM 2.0 event log that you could > contribute? > > Seriously, we shouldn't be (re-)writing code to do this. The kernel should not be used a dumping ground to work around a dysfunctional userspace either. :( You've basicaly said you can't rely on a sane userspace library because *reasons* so we need to dump stuff in the kernel instead. It is not a good justification to add new uAPI. James seems to have the same basic conclusion too, unfortunately. Jason ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v4 1/1] tpm: add sysfs exports for all banks of PCR registers 2020-08-19 14:09 ` Jason Gunthorpe @ 2020-08-19 14:53 ` Mimi Zohar 2020-08-19 14:55 ` Mimi Zohar 2020-08-19 22:16 ` Jarkko Sakkinen 2020-08-19 14:56 ` Serge E. Hallyn 1 sibling, 2 replies; 54+ messages in thread From: Mimi Zohar @ 2020-08-19 14:53 UTC (permalink / raw) To: Jason Gunthorpe; +Cc: Jarkko Sakkinen, James Bottomley, linux-integrity On Wed, 2020-08-19 at 11:09 -0300, Jason Gunthorpe wrote: > On Wed, Aug 19, 2020 at 09:27:33AM -0400, Mimi Zohar wrote: > > On Wed, 2020-08-19 at 09:02 -0300, Jason Gunthorpe wrote: > > > On Tue, Aug 18, 2020 at 02:55:50PM -0400, Mimi Zohar wrote: > > > > > > > The problem is that there isn't just one single userspace library or > > > > application for reading PCRs. So now not only is there the kernel > > > > "boot_aggregate" regression testing, but regression testing of the tool > > > > itself to support multiple methods of reading the PCRs. > > > > > > I was thinking just open code > > > open("/dev/tpm") > > > write(read_pcrs_cmd) > > > read(read_pcrs_cmd) > > > > > > It isn't particularly hard to retrive the PCRs, don't really need to > > > depend on a library. > > > > Ok, do you want to contribute it to ima-evm-utils? While you're at it, > > do you also have code to parse the TPM 2.0 event log that you could > > contribute? > > > > Seriously, we shouldn't be (re-)writing code to do this. > > The kernel should not be used a dumping ground to work around a > dysfunctional userspace either. :( > > You've basicaly said you can't rely on a sane userspace library > because *reasons* so we need to dump stuff in the kernel instead. > > It is not a good justification to add new uAPI. > > James seems to have the same basic conclusion too, unfortunately. "dysfunctional" is dropping existing TPM 1.2 sysfs support, which was done without consideration about existing applications/tools (e.g. ima- evm-utils, ltp) and without community input. It's not only James that is advocating for exporting the TPM PCRs, but Jerry Snitselaar, who reviewed this patch and exported the TPM version, and Nayna Jain, who exported the TPM 2.0 event log. I'm pretty sure there are a number of other people who would agree. Mimi ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v4 1/1] tpm: add sysfs exports for all banks of PCR registers 2020-08-19 14:53 ` Mimi Zohar @ 2020-08-19 14:55 ` Mimi Zohar 2020-08-19 22:16 ` Jarkko Sakkinen 1 sibling, 0 replies; 54+ messages in thread From: Mimi Zohar @ 2020-08-19 14:55 UTC (permalink / raw) To: Jason Gunthorpe; +Cc: Jarkko Sakkinen, James Bottomley, linux-integrity On Wed, 2020-08-19 at 10:53 -0400, Mimi Zohar wrote: > On Wed, 2020-08-19 at 11:09 -0300, Jason Gunthorpe wrote: > > On Wed, Aug 19, 2020 at 09:27:33AM -0400, Mimi Zohar wrote: > > > On Wed, 2020-08-19 at 09:02 -0300, Jason Gunthorpe wrote: > > > > On Tue, Aug 18, 2020 at 02:55:50PM -0400, Mimi Zohar wrote: > > > > > > > > > The problem is that there isn't just one single userspace library or > > > > > application for reading PCRs. So now not only is there the kernel > > > > > "boot_aggregate" regression testing, but regression testing of the tool > > > > > itself to support multiple methods of reading the PCRs. > > > > > > > > I was thinking just open code > > > > open("/dev/tpm") > > > > write(read_pcrs_cmd) > > > > read(read_pcrs_cmd) > > > > > > > > It isn't particularly hard to retrive the PCRs, don't really need to > > > > depend on a library. > > > > > > Ok, do you want to contribute it to ima-evm-utils? While you're at it, > > > do you also have code to parse the TPM 2.0 event log that you could > > > contribute? > > > > > > Seriously, we shouldn't be (re-)writing code to do this. > > > > The kernel should not be used a dumping ground to work around a > > dysfunctional userspace either. :( > > > > You've basicaly said you can't rely on a sane userspace library > > because *reasons* so we need to dump stuff in the kernel instead. > > > > It is not a good justification to add new uAPI. > > > > James seems to have the same basic conclusion too, unfortunately. > > "dysfunctional" is dropping existing TPM 1.2 sysfs support, which was ^not supporting existing TPM 1.2 sysfs for TPM 2.0 > done without consideration about existing applications/tools (e.g. ima- > evm-utils, ltp) and without community input. It's not only James that > is advocating for exporting the TPM PCRs, but Jerry Snitselaar, who > reviewed this patch and exported the TPM version, and Nayna Jain, who > exported the TPM 2.0 event log. I'm pretty sure there are a number of > other people who would agree. > > Mimi > ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v4 1/1] tpm: add sysfs exports for all banks of PCR registers 2020-08-19 14:53 ` Mimi Zohar 2020-08-19 14:55 ` Mimi Zohar @ 2020-08-19 22:16 ` Jarkko Sakkinen 2020-08-19 22:48 ` Jerry Snitselaar 1 sibling, 1 reply; 54+ messages in thread From: Jarkko Sakkinen @ 2020-08-19 22:16 UTC (permalink / raw) To: Mimi Zohar; +Cc: Jason Gunthorpe, James Bottomley, linux-integrity On Wed, Aug 19, 2020 at 10:53:38AM -0400, Mimi Zohar wrote: > On Wed, 2020-08-19 at 11:09 -0300, Jason Gunthorpe wrote: > > On Wed, Aug 19, 2020 at 09:27:33AM -0400, Mimi Zohar wrote: > > > On Wed, 2020-08-19 at 09:02 -0300, Jason Gunthorpe wrote: > > > > On Tue, Aug 18, 2020 at 02:55:50PM -0400, Mimi Zohar wrote: > > > > > > > > > The problem is that there isn't just one single userspace library or > > > > > application for reading PCRs. So now not only is there the kernel > > > > > "boot_aggregate" regression testing, but regression testing of the tool > > > > > itself to support multiple methods of reading the PCRs. > > > > > > > > I was thinking just open code > > > > open("/dev/tpm") > > > > write(read_pcrs_cmd) > > > > read(read_pcrs_cmd) > > > > > > > > It isn't particularly hard to retrive the PCRs, don't really need to > > > > depend on a library. > > > > > > Ok, do you want to contribute it to ima-evm-utils? While you're at it, > > > do you also have code to parse the TPM 2.0 event log that you could > > > contribute? > > > > > > Seriously, we shouldn't be (re-)writing code to do this. > > > > The kernel should not be used a dumping ground to work around a > > dysfunctional userspace either. :( > > > > You've basicaly said you can't rely on a sane userspace library > > because *reasons* so we need to dump stuff in the kernel instead. > > > > It is not a good justification to add new uAPI. > > > > James seems to have the same basic conclusion too, unfortunately. > > "dysfunctional" is dropping existing TPM 1.2 sysfs support, which was > done without consideration about existing applications/tools (e.g. ima- > evm-utils, ltp) and without community input. It's not only James that > is advocating for exporting the TPM PCRs, but Jerry Snitselaar, who > reviewed this patch and exported the TPM version, and Nayna Jain, who > exported the TPM 2.0 event log. I'm pretty sure there are a number of > other people who would agree. > > Mimi This is not true. TPM 1.2 sysfs was not dropped. Not adding something does not mean technically dropping something. /Jarkko ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v4 1/1] tpm: add sysfs exports for all banks of PCR registers 2020-08-19 22:16 ` Jarkko Sakkinen @ 2020-08-19 22:48 ` Jerry Snitselaar 2020-08-19 23:26 ` Jason Gunthorpe 2020-08-20 15:46 ` Jarkko Sakkinen 0 siblings, 2 replies; 54+ messages in thread From: Jerry Snitselaar @ 2020-08-19 22:48 UTC (permalink / raw) To: Jarkko Sakkinen Cc: Mimi Zohar, Jason Gunthorpe, James Bottomley, linux-integrity Jarkko Sakkinen @ 2020-08-19 15:16 MST: > On Wed, Aug 19, 2020 at 10:53:38AM -0400, Mimi Zohar wrote: >> On Wed, 2020-08-19 at 11:09 -0300, Jason Gunthorpe wrote: >> > On Wed, Aug 19, 2020 at 09:27:33AM -0400, Mimi Zohar wrote: >> > > On Wed, 2020-08-19 at 09:02 -0300, Jason Gunthorpe wrote: >> > > > On Tue, Aug 18, 2020 at 02:55:50PM -0400, Mimi Zohar wrote: >> > > > >> > > > > The problem is that there isn't just one single userspace library or >> > > > > application for reading PCRs. So now not only is there the kernel >> > > > > "boot_aggregate" regression testing, but regression testing of the tool >> > > > > itself to support multiple methods of reading the PCRs. >> > > > >> > > > I was thinking just open code >> > > > open("/dev/tpm") >> > > > write(read_pcrs_cmd) >> > > > read(read_pcrs_cmd) >> > > > >> > > > It isn't particularly hard to retrive the PCRs, don't really need to >> > > > depend on a library. >> > > >> > > Ok, do you want to contribute it to ima-evm-utils? While you're at it, >> > > do you also have code to parse the TPM 2.0 event log that you could >> > > contribute? >> > > >> > > Seriously, we shouldn't be (re-)writing code to do this. >> > >> > The kernel should not be used a dumping ground to work around a >> > dysfunctional userspace either. :( >> > >> > You've basicaly said you can't rely on a sane userspace library >> > because *reasons* so we need to dump stuff in the kernel instead. >> > >> > It is not a good justification to add new uAPI. >> > >> > James seems to have the same basic conclusion too, unfortunately. >> >> "dysfunctional" is dropping existing TPM 1.2 sysfs support, which was >> done without consideration about existing applications/tools (e.g. ima- >> evm-utils, ltp) and without community input. It's not only James that >> is advocating for exporting the TPM PCRs, but Jerry Snitselaar, who >> reviewed this patch and exported the TPM version, and Nayna Jain, who >> exported the TPM 2.0 event log. I'm pretty sure there are a number of >> other people who would agree. >> >> Mimi > > This is not true. TPM 1.2 sysfs was not dropped. > > Not adding something does not mean technically dropping something. > > /Jarkko When reviewing it I honestly didn't give much(any?) thought to whether it should be there. My thought was it adhered to the 1 value per file rule unlike the 1.2 pcrs file and that was about it. IIRC when 2.0 was added there was the issue of things like the 1.2 pcrs not conforming to standards, possible issues of races, and a question of what exactly should be exported. 1.2 has a number of files for doing things that I think were also handled by ppi and userspace. I guess the question is where does the line get drawn. My exporting the major version of the tpm probably could've been handled instead with a pr_info spitting it out for people to grab out of dmesg. Jerry ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v4 1/1] tpm: add sysfs exports for all banks of PCR registers 2020-08-19 22:48 ` Jerry Snitselaar @ 2020-08-19 23:26 ` Jason Gunthorpe 2020-08-20 15:46 ` Jarkko Sakkinen 1 sibling, 0 replies; 54+ messages in thread From: Jason Gunthorpe @ 2020-08-19 23:26 UTC (permalink / raw) To: Jerry Snitselaar Cc: Jarkko Sakkinen, Mimi Zohar, James Bottomley, linux-integrity On Wed, Aug 19, 2020 at 03:48:11PM -0700, Jerry Snitselaar wrote: > I guess the question is where does the line get drawn. My exporting the > major version of the tpm probably could've been handled instead with a > pr_info spitting it out for people to grab out of dmesg. FWIW, I did feel the tpm_version could be detected from the /dev/tpm as well. However the small size of kernel code, the general early boot argument, udev (eg only start the TPM2 daemons on a TPM2 system), and something like how INQUIRY is used by reporting tools, made it seem OK. Jason ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v4 1/1] tpm: add sysfs exports for all banks of PCR registers 2020-08-19 22:48 ` Jerry Snitselaar 2020-08-19 23:26 ` Jason Gunthorpe @ 2020-08-20 15:46 ` Jarkko Sakkinen 1 sibling, 0 replies; 54+ messages in thread From: Jarkko Sakkinen @ 2020-08-20 15:46 UTC (permalink / raw) To: Jerry Snitselaar Cc: Mimi Zohar, Jason Gunthorpe, James Bottomley, linux-integrity On Wed, Aug 19, 2020 at 03:48:11PM -0700, Jerry Snitselaar wrote: > > Jarkko Sakkinen @ 2020-08-19 15:16 MST: > > > On Wed, Aug 19, 2020 at 10:53:38AM -0400, Mimi Zohar wrote: > >> On Wed, 2020-08-19 at 11:09 -0300, Jason Gunthorpe wrote: > >> > On Wed, Aug 19, 2020 at 09:27:33AM -0400, Mimi Zohar wrote: > >> > > On Wed, 2020-08-19 at 09:02 -0300, Jason Gunthorpe wrote: > >> > > > On Tue, Aug 18, 2020 at 02:55:50PM -0400, Mimi Zohar wrote: > >> > > > > >> > > > > The problem is that there isn't just one single userspace library or > >> > > > > application for reading PCRs. So now not only is there the kernel > >> > > > > "boot_aggregate" regression testing, but regression testing of the tool > >> > > > > itself to support multiple methods of reading the PCRs. > >> > > > > >> > > > I was thinking just open code > >> > > > open("/dev/tpm") > >> > > > write(read_pcrs_cmd) > >> > > > read(read_pcrs_cmd) > >> > > > > >> > > > It isn't particularly hard to retrive the PCRs, don't really need to > >> > > > depend on a library. > >> > > > >> > > Ok, do you want to contribute it to ima-evm-utils? While you're at it, > >> > > do you also have code to parse the TPM 2.0 event log that you could > >> > > contribute? > >> > > > >> > > Seriously, we shouldn't be (re-)writing code to do this. > >> > > >> > The kernel should not be used a dumping ground to work around a > >> > dysfunctional userspace either. :( > >> > > >> > You've basicaly said you can't rely on a sane userspace library > >> > because *reasons* so we need to dump stuff in the kernel instead. > >> > > >> > It is not a good justification to add new uAPI. > >> > > >> > James seems to have the same basic conclusion too, unfortunately. > >> > >> "dysfunctional" is dropping existing TPM 1.2 sysfs support, which was > >> done without consideration about existing applications/tools (e.g. ima- > >> evm-utils, ltp) and without community input. It's not only James that > >> is advocating for exporting the TPM PCRs, but Jerry Snitselaar, who > >> reviewed this patch and exported the TPM version, and Nayna Jain, who > >> exported the TPM 2.0 event log. I'm pretty sure there are a number of > >> other people who would agree. > >> > >> Mimi > > > > This is not true. TPM 1.2 sysfs was not dropped. > > > > Not adding something does not mean technically dropping something. > > > > /Jarkko > > When reviewing it I honestly didn't give much(any?) thought to whether > it should be there. My thought was it adhered to the 1 value per file > rule unlike the 1.2 pcrs file and that was about it. > > IIRC when 2.0 was added there was the issue of things like the 1.2 pcrs > not conforming to standards, possible issues of races, and a question of > what exactly should be exported. 1.2 has a number of files for doing > things that I think were also handled by ppi and userspace. > > I guess the question is where does the line get drawn. My exporting the > major version of the tpm probably could've been handled instead with a > pr_info spitting it out for people to grab out of dmesg. > > > Jerry TPM protocol version is a different case tha dumping all the PCRs as ASCII. It fairly unintrusive feature for the kernel, kernel has this knowledge stored already and it is constant for a boot cycle. /Jarkko ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v4 1/1] tpm: add sysfs exports for all banks of PCR registers 2020-08-19 14:09 ` Jason Gunthorpe 2020-08-19 14:53 ` Mimi Zohar @ 2020-08-19 14:56 ` Serge E. Hallyn 1 sibling, 0 replies; 54+ messages in thread From: Serge E. Hallyn @ 2020-08-19 14:56 UTC (permalink / raw) To: Jason Gunthorpe Cc: Mimi Zohar, Jarkko Sakkinen, James Bottomley, linux-integrity On Wed, Aug 19, 2020 at 11:09:43AM -0300, Jason Gunthorpe wrote: > On Wed, Aug 19, 2020 at 09:27:33AM -0400, Mimi Zohar wrote: > > On Wed, 2020-08-19 at 09:02 -0300, Jason Gunthorpe wrote: > > > On Tue, Aug 18, 2020 at 02:55:50PM -0400, Mimi Zohar wrote: > > > > > > > The problem is that there isn't just one single userspace library or > > > > application for reading PCRs. So now not only is there the kernel > > > > "boot_aggregate" regression testing, but regression testing of the tool > > > > itself to support multiple methods of reading the PCRs. > > > > > > I was thinking just open code > > > open("/dev/tpm") > > > write(read_pcrs_cmd) > > > read(read_pcrs_cmd) > > > > > > It isn't particularly hard to retrive the PCRs, don't really need to > > > depend on a library. > > > > Ok, do you want to contribute it to ima-evm-utils? While you're at it, > > do you also have code to parse the TPM 2.0 event log that you could > > contribute? > > > > Seriously, we shouldn't be (re-)writing code to do this. > > The kernel should not be used a dumping ground to work around a > dysfunctional userspace either. :( > > You've basicaly said you can't rely on a sane userspace library > because *reasons* so we need to dump stuff in the kernel instead. > > It is not a good justification to add new uAPI. > > James seems to have the same basic conclusion too, unfortunately. It seems to me the point is that we often want to do these things in very early and/or stripped-down userspaces. I'm definately also in favor of having sysfs provide these. ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v4 1/1] tpm: add sysfs exports for all banks of PCR registers 2020-08-19 13:27 ` Mimi Zohar 2020-08-19 14:09 ` Jason Gunthorpe @ 2020-08-19 22:15 ` Jarkko Sakkinen 1 sibling, 0 replies; 54+ messages in thread From: Jarkko Sakkinen @ 2020-08-19 22:15 UTC (permalink / raw) To: Mimi Zohar; +Cc: Jason Gunthorpe, James Bottomley, linux-integrity On Wed, Aug 19, 2020 at 09:27:33AM -0400, Mimi Zohar wrote: > On Wed, 2020-08-19 at 09:02 -0300, Jason Gunthorpe wrote: > > On Tue, Aug 18, 2020 at 02:55:50PM -0400, Mimi Zohar wrote: > > > > > The problem is that there isn't just one single userspace library or > > > application for reading PCRs. So now not only is there the kernel > > > "boot_aggregate" regression testing, but regression testing of the tool > > > itself to support multiple methods of reading the PCRs. > > > > I was thinking just open code > > open("/dev/tpm") > > write(read_pcrs_cmd) > > read(read_pcrs_cmd) > > > > It isn't particularly hard to retrive the PCRs, don't really need to > > depend on a library. > > Ok, do you want to contribute it to ima-evm-utils? While you're at it, > do you also have code to parse the TPM 2.0 event log that you could > contribute? > > Seriously, we shouldn't be (re-)writing code to do this. > > Mimi > I'm puzzled why go the trouble of submitting this to LKML and not use that time to do the whatever changes ima-evm-utils requires? /Jarkko ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v4 1/1] tpm: add sysfs exports for all banks of PCR registers 2020-08-19 12:02 ` Jason Gunthorpe 2020-08-19 13:27 ` Mimi Zohar @ 2020-08-19 15:17 ` James Bottomley 2020-08-19 16:18 ` Jason Gunthorpe 1 sibling, 1 reply; 54+ messages in thread From: James Bottomley @ 2020-08-19 15:17 UTC (permalink / raw) To: Jason Gunthorpe, Mimi Zohar; +Cc: Jarkko Sakkinen, linux-integrity On Wed, 2020-08-19 at 09:02 -0300, Jason Gunthorpe wrote: > On Tue, Aug 18, 2020 at 02:55:50PM -0400, Mimi Zohar wrote: > > > The problem is that there isn't just one single userspace library > > or application for reading PCRs. So now not only is there the > > kernel "boot_aggregate" regression testing, but regression testing > > of the tool itself to support multiple methods of reading the PCRs. > > I was thinking just open code > open("/dev/tpm") > write(read_pcrs_cmd) That's rather an over simplification. The command takes a TPML_PCR_SELECTION structure which is, in turn, a packed array of TPMS_PCR_SELECTION which is a hash type and big endian packed bitmap. > read(read_pcrs_cmd) And the return is a TPML_PCR_SELECTION, in case the system couldn't provide something you asked for followed by TPML_DIGEST structure which is a counted array of TPM2B packed digests. This is a marshal/unmarshal nightmare for the uninitiated. It is *not* simple or even straightforward. > It isn't particularly hard to retrive the PCRs, don't really need to > depend on a library. Well, having a PhD in Theoretical Physics, I find quantum field theory remarkably easy to understand. My friends tell me this is just me and they'd rather I not talk about it at parties ... James ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v4 1/1] tpm: add sysfs exports for all banks of PCR registers 2020-08-19 15:17 ` James Bottomley @ 2020-08-19 16:18 ` Jason Gunthorpe 2020-08-19 16:57 ` Mimi Zohar 0 siblings, 1 reply; 54+ messages in thread From: Jason Gunthorpe @ 2020-08-19 16:18 UTC (permalink / raw) To: James Bottomley; +Cc: Mimi Zohar, Jarkko Sakkinen, linux-integrity On Wed, Aug 19, 2020 at 08:17:11AM -0700, James Bottomley wrote: > On Wed, 2020-08-19 at 09:02 -0300, Jason Gunthorpe wrote: > > On Tue, Aug 18, 2020 at 02:55:50PM -0400, Mimi Zohar wrote: > > > > > The problem is that there isn't just one single userspace library > > > or application for reading PCRs. So now not only is there the > > > kernel "boot_aggregate" regression testing, but regression testing > > > of the tool itself to support multiple methods of reading the PCRs. > > > > I was thinking just open code > > open("/dev/tpm") > > write(read_pcrs_cmd) > > That's rather an over simplification. The command takes a > TPML_PCR_SELECTION structure which is, in turn, a packed array of > TPMS_PCR_SELECTION which is a hash type and big endian packed bitmap. Which is a fixed value, so at least the write is not a complete over simplification, and this is a LTP where it is reasonable to directly talk to the kernel. Remember, the only reason we got here is because apparently there is no sane single userspace library the LTP could use. Which seems crazy at this point.. Your other reasons are more compelling, but they also sound like a tool will be needed to do the other parts - and if we need a tool we can't the tool read the PCRS progmatically too? Especially if the tool has to parse log, call hashing functions and unpack TPM keys already. And to Mimi's other jab: > "dysfunctional" is dropping existing TPM 1.2 sysfs support, which was > done without consideration about existing applications/tools (e.g. ima- > evm-utils, ltp) and without community input. Yes - it was dropped because TPM 2 was a *complete ABI break* for everything. The kernel was reset to a uABI that matches current uABI standards starting TPM 2. The whole userspace needed to be redone anyhow, and certainly nobody objected at the time. At least my expecation was that a sensible userspace for TPM (for administrator user) would be built, like we see in other subsystems eg 'ip' for netdev. Jason ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v4 1/1] tpm: add sysfs exports for all banks of PCR registers 2020-08-19 16:18 ` Jason Gunthorpe @ 2020-08-19 16:57 ` Mimi Zohar 2020-08-19 17:17 ` Jason Gunthorpe 0 siblings, 1 reply; 54+ messages in thread From: Mimi Zohar @ 2020-08-19 16:57 UTC (permalink / raw) To: Jason Gunthorpe, James Bottomley; +Cc: Jarkko Sakkinen, linux-integrity On Wed, 2020-08-19 at 13:18 -0300, Jason Gunthorpe wrote: > Yes - it was dropped because TPM 2 was a *complete ABI break* for > everything. The kernel was reset to a uABI that matches current > uABI standards starting TPM 2. > > The whole userspace needed to be redone anyhow, and certainly nobody > objected at the time. > > At least my expecation was that a sensible userspace for TPM (for > administrator user) would be built, like we see in other subsystems eg > 'ip' for netdev. "Because TPM 2 was a complete ABI break for everything" could be reason for upstreaming a minimal subset of functionality initially, which could be expanded over time. I don't recall a discussion about limting features in the future. Mimi ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v4 1/1] tpm: add sysfs exports for all banks of PCR registers 2020-08-19 16:57 ` Mimi Zohar @ 2020-08-19 17:17 ` Jason Gunthorpe 2020-08-19 20:09 ` James Bottomley 0 siblings, 1 reply; 54+ messages in thread From: Jason Gunthorpe @ 2020-08-19 17:17 UTC (permalink / raw) To: Mimi Zohar; +Cc: James Bottomley, Jarkko Sakkinen, linux-integrity On Wed, Aug 19, 2020 at 12:57:42PM -0400, Mimi Zohar wrote: > On Wed, 2020-08-19 at 13:18 -0300, Jason Gunthorpe wrote: > > Yes - it was dropped because TPM 2 was a *complete ABI break* for > > everything. The kernel was reset to a uABI that matches current > > uABI standards starting TPM 2. > > > > The whole userspace needed to be redone anyhow, and certainly nobody > > objected at the time. > > > > At least my expecation was that a sensible userspace for TPM (for > > administrator user) would be built, like we see in other subsystems eg > > 'ip' for netdev. > > "Because TPM 2 was a complete ABI break for everything" could be reason > for upstreaming a minimal subset of functionality initially, which > could be expanded over time. I don't recall a discussion about limting > features in the future. All new uAPI additions need to pass the usual uAPI hurdles. As James outlined, justify why the kernel must present a duplicated uAPI between sysfs and /dev/tpm. There have been good reasons in the past, eg SCSI inquiry. But there are also bad reasons like "our userpsace is dysfunctional and can't make a library or tool". Jason ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v4 1/1] tpm: add sysfs exports for all banks of PCR registers 2020-08-19 17:17 ` Jason Gunthorpe @ 2020-08-19 20:09 ` James Bottomley 2020-08-19 23:21 ` Jason Gunthorpe 0 siblings, 1 reply; 54+ messages in thread From: James Bottomley @ 2020-08-19 20:09 UTC (permalink / raw) To: Jason Gunthorpe, Mimi Zohar; +Cc: Jarkko Sakkinen, linux-integrity On Wed, 2020-08-19 at 14:17 -0300, Jason Gunthorpe wrote: > On Wed, Aug 19, 2020 at 12:57:42PM -0400, Mimi Zohar wrote: > > On Wed, 2020-08-19 at 13:18 -0300, Jason Gunthorpe wrote: > > > Yes - it was dropped because TPM 2 was a *complete ABI break* for > > > everything. The kernel was reset to a uABI that matches current > > > uABI standards starting TPM 2. > > > > > > The whole userspace needed to be redone anyhow, and certainly > > > nobody objected at the time. > > > > > > At least my expecation was that a sensible userspace for TPM (for > > > administrator user) would be built, like we see in other > > > subsystems eg 'ip' for netdev. > > > > "Because TPM 2 was a complete ABI break for everything" could be > > reason for upstreaming a minimal subset of functionality initially, > > which could be expanded over time. I don't recall a discussion > > about limting features in the future. > > All new uAPI additions need to pass the usual uAPI hurdles. > > As James outlined, justify why the kernel must present a duplicated > uAPI between sysfs and /dev/tpm. > > There have been good reasons in the past, eg SCSI inquiry. First, can we please agree /dev/tpm does not substitute as a "duplicate API". I can now clarify the objection into "it's a binary marshalled interface and Linus doesn't think we should force users to use them": https://lore.kernel.org/linux-api/CAHk-=wh5YifP7hzKSbwJj94+DZ2czjrZsczy6GBimiogZws=rg@mail.gmail.com/ So the question boils down to whether we're providing a simple interface that allows preboot and other use cases or should we require the complexity of a TSS installation to get the necessary tools. Perhaps we should also simply copy linux-api and accept the judgment of the experts on whether we should expose PCRs via sysfs. > But there are also bad reasons like "our userpsace is dysfunctional > and can't make a library or tool". The reason we provide a kernel interface instead of a library or tool is that libraries and tools tend to be domain specific and the information needs to be provided across domains. So: both the current TPM 2.0 TSSs are written in C. This means they can just about be plugged into python but not easily into Go because of its abhorrence of ffis. Providing the PCRs from sysfs allows Go attestation easy access that the TSS tools don't because of the language domain problem. James ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v4 1/1] tpm: add sysfs exports for all banks of PCR registers 2020-08-19 20:09 ` James Bottomley @ 2020-08-19 23:21 ` Jason Gunthorpe 2020-08-20 16:14 ` James Bottomley 0 siblings, 1 reply; 54+ messages in thread From: Jason Gunthorpe @ 2020-08-19 23:21 UTC (permalink / raw) To: James Bottomley; +Cc: Mimi Zohar, Jarkko Sakkinen, linux-integrity On Wed, Aug 19, 2020 at 01:09:16PM -0700, James Bottomley wrote: > On Wed, 2020-08-19 at 14:17 -0300, Jason Gunthorpe wrote: > > On Wed, Aug 19, 2020 at 12:57:42PM -0400, Mimi Zohar wrote: > > > On Wed, 2020-08-19 at 13:18 -0300, Jason Gunthorpe wrote: > > > > Yes - it was dropped because TPM 2 was a *complete ABI break* for > > > > everything. The kernel was reset to a uABI that matches current > > > > uABI standards starting TPM 2. > > > > > > > > The whole userspace needed to be redone anyhow, and certainly > > > > nobody objected at the time. > > > > > > > > At least my expecation was that a sensible userspace for TPM (for > > > > administrator user) would be built, like we see in other > > > > subsystems eg 'ip' for netdev. > > > > > > "Because TPM 2 was a complete ABI break for everything" could be > > > reason for upstreaming a minimal subset of functionality initially, > > > which could be expanded over time. I don't recall a discussion > > > about limting features in the future. > > > > All new uAPI additions need to pass the usual uAPI hurdles. > > > > As James outlined, justify why the kernel must present a duplicated > > uAPI between sysfs and /dev/tpm. > > > > There have been good reasons in the past, eg SCSI inquiry. > > First, can we please agree /dev/tpm does not substitute as a "duplicate > API". Er? Huh? How so? > I can now clarify the objection into "it's a binary marshalled > interface and Linus doesn't think we should force users to use them": > > https://lore.kernel.org/linux-api/CAHk-=wh5YifP7hzKSbwJj94+DZ2czjrZsczy6GBimiogZws=rg@mail.gmail.com/ I'm not sure which part of that you want to quote? "It's great for well-specified wire protocols." which is describing /dev/tpm - it has a multivendor standards body. Bit puzzled about the rest of this message? Do you think Linus belives netlink should have been implemented as ASCII? JSON parser in the kernel maybe? Confusing. > Perhaps we should also simply copy linux-api and accept the judgment of > the experts on whether we should expose PCRs via sysfs. Well, AFAIK, for a long time now the mantra has been "if it can be done in userspace then it should not be in the kernel" .. I would really like to see a better reason for this - one that doesn't boil down to it being 'too hard' to write a bit of code in userspace. eg we can't do it because we can't access /dev/tpm for permissions or something. > The reason we provide a kernel interface instead of a library or tool > is that libraries and tools tend to be domain specific and the > information needs to be provided across domains. So: both the current > TPM 2.0 TSSs are written in C. This means they can just about be > plugged into python but not easily into Go because of its abhorrence of > ffis. Providing the PCRs from sysfs allows Go attestation easy access > that the TSS tools don't because of the language domain problem. I went to try to make a python implementation.. After about 10mins I came up with this approximate thing: select = struct.pack(">BBB", 1, 0, 0) # PCR 1 pcrread_in = struct.pack(">IHB", 1, TPM2_ALG_SHA1, len(select)) + select msg = struct.pack(">HII", TPM2_ST_NO_SESSIONS, 10 + len(pcrread_in), TPM2_CC_PCR_READ) + pcrread_in with open("/dev/tpm","wb") as tpm: tpm.write(msg) resp = tpm.read(msg) tag, length, return_code = struct.unpack(">HII",res[:10]) if not return_code: raise Error() return res[10+20:] # digest Which is hopefully quite close to being something working - at least it looks fairly close to what the kernel implementation does. Fortunately no Phd was required! I think Go would be about similar, right? Jason ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v4 1/1] tpm: add sysfs exports for all banks of PCR registers 2020-08-19 23:21 ` Jason Gunthorpe @ 2020-08-20 16:14 ` James Bottomley 2020-08-20 16:55 ` Serge E. Hallyn ` (2 more replies) 0 siblings, 3 replies; 54+ messages in thread From: James Bottomley @ 2020-08-20 16:14 UTC (permalink / raw) To: Jason Gunthorpe; +Cc: Mimi Zohar, Jarkko Sakkinen, linux-integrity On Wed, 2020-08-19 at 20:21 -0300, Jason Gunthorpe wrote: > On Wed, Aug 19, 2020 at 01:09:16PM -0700, James Bottomley wrote: > > On Wed, 2020-08-19 at 14:17 -0300, Jason Gunthorpe wrote: > > > On Wed, Aug 19, 2020 at 12:57:42PM -0400, Mimi Zohar wrote: > > > > On Wed, 2020-08-19 at 13:18 -0300, Jason Gunthorpe wrote: > > > > > Yes - it was dropped because TPM 2 was a *complete ABI break* > > > > > for everything. The kernel was reset to a uABI that matches > > > > > current uABI standards starting TPM 2. > > > > > > > > > > The whole userspace needed to be redone anyhow, and certainly > > > > > nobody objected at the time. > > > > > > > > > > At least my expecation was that a sensible userspace for TPM > > > > > (for administrator user) would be built, like we see in other > > > > > subsystems eg 'ip' for netdev. > > > > > > > > "Because TPM 2 was a complete ABI break for everything" could > > > > be reason for upstreaming a minimal subset of functionality > > > > initially, which could be expanded over time. I don't recall a > > > > discussion about limting features in the future. > > > > > > All new uAPI additions need to pass the usual uAPI hurdles. > > > > > > As James outlined, justify why the kernel must present a > > > duplicated uAPI between sysfs and /dev/tpm. > > > > > > There have been good reasons in the past, eg SCSI inquiry. > > > > First, can we please agree /dev/tpm does not substitute as a > > "duplicate API". > > Er? Huh? How so? Because like the SCSI command interface it's a binary marshalled protocol we want to abstract for users. We can still argue whether the kernel or a toolkit should do the abstraction but it's not one we want to dump on users and say "this is it, what do you mean you don't like it?" > > I can now clarify the objection into "it's a binary marshalled > > interface and Linus doesn't think we should force users to use > > them": > > > > https://lore.kernel.org/linux-api/CAHk-=wh5YifP7hzKSbwJj94+DZ2czjrZ > > sczy6GBimiogZws=rg@mail.gmail.com/ > > I'm not sure which part of that you want to quote? > > "It's great for well-specified wire protocols." which is describing > /dev/tpm - it has a multivendor standards body. Actually this bit "I think marshalling binary data is actively evil and wrong. It's great for well-specified wire protocols. It's great for internal communication in user space. It's *NOT* great for a kernel system call interface." /dev/tpm is a user to kernel interface. > Bit puzzled about the rest of this message? Do you think Linus > belives netlink should have been implemented as ASCII? JSON parser in > the kernel maybe? Confusing. Heh, well I remember ASN.1 parser over my dead body and now we have one ... > > Perhaps we should also simply copy linux-api and accept the > > judgment of the experts on whether we should expose PCRs via sysfs. > > Well, AFAIK, for a long time now the mantra has been "if it can be > done in userspace then it should not be in the kernel" .. Really, no, we've never had that. A filesystem can be done in userspace but there's no move to throw them all out of the kernel. We bring stuff into the kernel when it's more efficient and useful and makes the presentation layer easier. The question isn't could the user do it at any cost, the question is if we do it can we maintain it easily and does it make life easier for the user (and would they actually use the interface). > I would really like to see a better reason for this - one that > doesn't boil down to it being 'too hard' to write a bit of code in > userspace. I haven't said that. I've said there are three (and got corrected to 4) reasons to have easy access to PCR hex values: 1. early boot measurement checks 2. log verification 3. key sealing 4. log stability checking > eg we can't do it because we can't access /dev/tpm for permissions or > something. I already said that: we can't it's root.root 0600 currently. All the TSSs seem to change at least /dev/tpmrm to tpm.tpm 0660 but we can't do that in the kernel because there's no fixed tpm uid/gid. > > The reason we provide a kernel interface instead of a library or > > tool is that libraries and tools tend to be domain specific and the > > information needs to be provided across domains. So: both the > > current TPM 2.0 TSSs are written in C. This means they can just > > about be plugged into python but not easily into Go because of its > > abhorrence of ffis. Providing the PCRs from sysfs allows Go > > attestation easy access that the TSS tools don't because of the > > language domain problem. > > I went to try to make a python implementation.. After about 10mins I > came up with this approximate thing: > > select = struct.pack(">BBB", 1, 0, 0) # PCR 1 > pcrread_in = struct.pack(">IHB", 1, TPM2_ALG_SHA1, len(select)) + > select > msg = struct.pack(">HII", TPM2_ST_NO_SESSIONS, 10 + len(pcrread_in), > TPM2_CC_PCR_READ) + pcrread_in > > with open("/dev/tpm","wb") as tpm: > tpm.write(msg) > resp = tpm.read(msg) > > tag, length, return_code = struct.unpack(">HII",res[:10]) > if not return_code: > raise Error() > > return res[10+20:] # digest > > Which is hopefully quite close to being something working - at least > it looks fairly close to what the kernel implementation does. > > Fortunately no Phd was required! I think Go would be about similar, > right? I could do the same with perl, but not bash. In the same way I could construct an anomalous SO(3) higgs model as a party trick. the point is that when you ask users would they rather do the above or cat /sys/class/tpm/tpm0/pcr-sha1/1 they'll universally opt for the latter because it's way simpler. Now perhaps if the mechanism that services this in the kernel were thousands of lines long and unmaintainable you'd think twice, but it's not, it's under 200 lines. So the maintainability bar to us providing this is low and the user convenience quite high ... that's what makes it look like a good interface. James ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v4 1/1] tpm: add sysfs exports for all banks of PCR registers 2020-08-20 16:14 ` James Bottomley @ 2020-08-20 16:55 ` Serge E. Hallyn 2020-08-21 17:41 ` Jarkko Sakkinen 2020-08-21 19:38 ` Jason Gunthorpe 2 siblings, 0 replies; 54+ messages in thread From: Serge E. Hallyn @ 2020-08-20 16:55 UTC (permalink / raw) To: James Bottomley Cc: Jason Gunthorpe, Mimi Zohar, Jarkko Sakkinen, linux-integrity On Thu, Aug 20, 2020 at 09:14:44AM -0700, James Bottomley wrote: > On Wed, 2020-08-19 at 20:21 -0300, Jason Gunthorpe wrote: > > On Wed, Aug 19, 2020 at 01:09:16PM -0700, James Bottomley wrote: > > I went to try to make a python implementation.. After about 10mins I > > came up with this approximate thing: > > > > select = struct.pack(">BBB", 1, 0, 0) # PCR 1 > > pcrread_in = struct.pack(">IHB", 1, TPM2_ALG_SHA1, len(select)) + > > select > > msg = struct.pack(">HII", TPM2_ST_NO_SESSIONS, 10 + len(pcrread_in), > > TPM2_CC_PCR_READ) + pcrread_in > > > > with open("/dev/tpm","wb") as tpm: > > tpm.write(msg) > > resp = tpm.read(msg) > > > > tag, length, return_code = struct.unpack(">HII",res[:10]) > > if not return_code: > > raise Error() > > > > return res[10+20:] # digest > > > > Which is hopefully quite close to being something working - at least > > it looks fairly close to what the kernel implementation does. > > > > Fortunately no Phd was required! I think Go would be about similar, > > right? > > I could do the same with perl, but not bash. In the same way I could > construct an anomalous SO(3) higgs model as a party trick. > > the point is that when you ask users would they rather do the above or > cat /sys/class/tpm/tpm0/pcr-sha1/1 they'll universally opt for the > latter because it's way simpler. > > Now perhaps if the mechanism that services this in the kernel were > thousands of lines long and unmaintainable you'd think twice, but it's > not, it's under 200 lines. So the maintainability bar to us providing > this is low and the user convenience quite high ... that's what makes > it look like a good interface. > > James I'd also point out that this is the fundamental thing you do with the pcrs. There is no other way that some library would want to do it, and everything builds on it. We're exporting the core functionality as a simpler file read/write. I know that after taking filesystem interfaces to an extreme, over the past 20 years we've turned back a bit, but in this case it seems the right way to do it. ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v4 1/1] tpm: add sysfs exports for all banks of PCR registers 2020-08-20 16:14 ` James Bottomley 2020-08-20 16:55 ` Serge E. Hallyn @ 2020-08-21 17:41 ` Jarkko Sakkinen 2020-08-21 19:38 ` Jason Gunthorpe 2 siblings, 0 replies; 54+ messages in thread From: Jarkko Sakkinen @ 2020-08-21 17:41 UTC (permalink / raw) To: James Bottomley; +Cc: Jason Gunthorpe, Mimi Zohar, linux-integrity On Thu, Aug 20, 2020 at 09:14:44AM -0700, James Bottomley wrote: > On Wed, 2020-08-19 at 20:21 -0300, Jason Gunthorpe wrote: > > On Wed, Aug 19, 2020 at 01:09:16PM -0700, James Bottomley wrote: > > > On Wed, 2020-08-19 at 14:17 -0300, Jason Gunthorpe wrote: > > > > On Wed, Aug 19, 2020 at 12:57:42PM -0400, Mimi Zohar wrote: > > > > > On Wed, 2020-08-19 at 13:18 -0300, Jason Gunthorpe wrote: > > > > > > Yes - it was dropped because TPM 2 was a *complete ABI break* > > > > > > for everything. The kernel was reset to a uABI that matches > > > > > > current uABI standards starting TPM 2. > > > > > > > > > > > > The whole userspace needed to be redone anyhow, and certainly > > > > > > nobody objected at the time. > > > > > > > > > > > > At least my expecation was that a sensible userspace for TPM > > > > > > (for administrator user) would be built, like we see in other > > > > > > subsystems eg 'ip' for netdev. > > > > > > > > > > "Because TPM 2 was a complete ABI break for everything" could > > > > > be reason for upstreaming a minimal subset of functionality > > > > > initially, which could be expanded over time. I don't recall a > > > > > discussion about limting features in the future. > > > > > > > > All new uAPI additions need to pass the usual uAPI hurdles. > > > > > > > > As James outlined, justify why the kernel must present a > > > > duplicated uAPI between sysfs and /dev/tpm. > > > > > > > > There have been good reasons in the past, eg SCSI inquiry. > > > > > > First, can we please agree /dev/tpm does not substitute as a > > > "duplicate API". > > > > Er? Huh? How so? > > Because like the SCSI command interface it's a binary marshalled > protocol we want to abstract for users. We can still argue whether the > kernel or a toolkit should do the abstraction but it's not one we want > to dump on users and say "this is it, what do you mean you don't like > it?" No we don't. On the contrary, we expose the protocol through /dev/tpm0. /Jarkko ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v4 1/1] tpm: add sysfs exports for all banks of PCR registers 2020-08-20 16:14 ` James Bottomley 2020-08-20 16:55 ` Serge E. Hallyn 2020-08-21 17:41 ` Jarkko Sakkinen @ 2020-08-21 19:38 ` Jason Gunthorpe 2020-08-24 19:44 ` Jarkko Sakkinen 2 siblings, 1 reply; 54+ messages in thread From: Jason Gunthorpe @ 2020-08-21 19:38 UTC (permalink / raw) To: James Bottomley; +Cc: Mimi Zohar, Jarkko Sakkinen, linux-integrity On Thu, Aug 20, 2020 at 09:14:44AM -0700, James Bottomley wrote: > > eg we can't do it because we can't access /dev/tpm for permissions or > > something. > > I already said that: we can't it's root.root 0600 currently. All the > TSSs seem to change at least /dev/tpmrm to tpm.tpm 0660 but we can't do > that in the kernel because there's no fixed tpm uid/gid. Permissions is a pretty good reason to add a sysfs file. Jason ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v4 1/1] tpm: add sysfs exports for all banks of PCR registers 2020-08-21 19:38 ` Jason Gunthorpe @ 2020-08-24 19:44 ` Jarkko Sakkinen 2020-08-24 20:20 ` James Bottomley 2020-08-24 21:57 ` Jason Gunthorpe 0 siblings, 2 replies; 54+ messages in thread From: Jarkko Sakkinen @ 2020-08-24 19:44 UTC (permalink / raw) To: Jason Gunthorpe; +Cc: James Bottomley, Mimi Zohar, linux-integrity On Fri, Aug 21, 2020 at 04:38:47PM -0300, Jason Gunthorpe wrote: > On Thu, Aug 20, 2020 at 09:14:44AM -0700, James Bottomley wrote: > > > > eg we can't do it because we can't access /dev/tpm for permissions or > > > something. > > > > I already said that: we can't it's root.root 0600 currently. All the > > TSSs seem to change at least /dev/tpmrm to tpm.tpm 0660 but we can't do > > that in the kernel because there's no fixed tpm uid/gid. > > Permissions is a pretty good reason to add a sysfs file. > > Jason I'm not sure why suid/sgid utility to read pcrs would be worse. /Jarkko ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v4 1/1] tpm: add sysfs exports for all banks of PCR registers 2020-08-24 19:44 ` Jarkko Sakkinen @ 2020-08-24 20:20 ` James Bottomley 2020-08-25 15:27 ` Jarkko Sakkinen 2020-08-24 21:57 ` Jason Gunthorpe 1 sibling, 1 reply; 54+ messages in thread From: James Bottomley @ 2020-08-24 20:20 UTC (permalink / raw) To: Jarkko Sakkinen, Jason Gunthorpe; +Cc: Mimi Zohar, linux-integrity On Mon, 2020-08-24 at 22:44 +0300, Jarkko Sakkinen wrote: > On Fri, Aug 21, 2020 at 04:38:47PM -0300, Jason Gunthorpe wrote: > > On Thu, Aug 20, 2020 at 09:14:44AM -0700, James Bottomley wrote: > > > > > > eg we can't do it because we can't access /dev/tpm for > > > > permissions or > > > > something. > > > > > > I already said that: we can't it's root.root 0600 currently. All > > > the TSSs seem to change at least /dev/tpmrm to tpm.tpm 0660 but > > > we can't do that in the kernel because there's no fixed tpm > > > uid/gid. > > > > Permissions is a pretty good reason to add a sysfs file. > > > > Jason > > I'm not sure why suid/sgid utility to read pcrs would be worse. We don't do root running or suid/sgid binaries any more because they're exceptional security risks. That's why both TSSs for TPM 2.0 change the device ownership. For Trousers and TPM 1.2 we used to run the daemon as root until we started getting CVEs about it. James ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v4 1/1] tpm: add sysfs exports for all banks of PCR registers 2020-08-24 20:20 ` James Bottomley @ 2020-08-25 15:27 ` Jarkko Sakkinen 2020-08-25 15:33 ` James Bottomley 0 siblings, 1 reply; 54+ messages in thread From: Jarkko Sakkinen @ 2020-08-25 15:27 UTC (permalink / raw) To: James Bottomley; +Cc: Jason Gunthorpe, Mimi Zohar, linux-integrity On Mon, Aug 24, 2020 at 01:20:46PM -0700, James Bottomley wrote: > On Mon, 2020-08-24 at 22:44 +0300, Jarkko Sakkinen wrote: > > On Fri, Aug 21, 2020 at 04:38:47PM -0300, Jason Gunthorpe wrote: > > > On Thu, Aug 20, 2020 at 09:14:44AM -0700, James Bottomley wrote: > > > > > > > > eg we can't do it because we can't access /dev/tpm for > > > > > permissions or > > > > > something. > > > > > > > > I already said that: we can't it's root.root 0600 currently. All > > > > the TSSs seem to change at least /dev/tpmrm to tpm.tpm 0660 but > > > > we can't do that in the kernel because there's no fixed tpm > > > > uid/gid. > > > > > > Permissions is a pretty good reason to add a sysfs file. > > > > > > Jason > > > > I'm not sure why suid/sgid utility to read pcrs would be worse. > > We don't do root running or suid/sgid binaries any more because they're > exceptional security risks. That's why both TSSs for TPM 2.0 change > the device ownership. For Trousers and TPM 1.2 we used to run the > daemon as root until we started getting CVEs about it. > > James OK, then a binary blob for pcrs would be sufficient. /Jarkko ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v4 1/1] tpm: add sysfs exports for all banks of PCR registers 2020-08-25 15:27 ` Jarkko Sakkinen @ 2020-08-25 15:33 ` James Bottomley 2020-08-26 13:15 ` Jarkko Sakkinen 0 siblings, 1 reply; 54+ messages in thread From: James Bottomley @ 2020-08-25 15:33 UTC (permalink / raw) To: Jarkko Sakkinen; +Cc: Jason Gunthorpe, Mimi Zohar, linux-integrity On Tue, 2020-08-25 at 18:27 +0300, Jarkko Sakkinen wrote: > On Mon, Aug 24, 2020 at 01:20:46PM -0700, James Bottomley wrote: > > On Mon, 2020-08-24 at 22:44 +0300, Jarkko Sakkinen wrote: > > > On Fri, Aug 21, 2020 at 04:38:47PM -0300, Jason Gunthorpe wrote: > > > > On Thu, Aug 20, 2020 at 09:14:44AM -0700, James Bottomley > > > > wrote: > > > > > > > > > > eg we can't do it because we can't access /dev/tpm for > > > > > > permissions or something. > > > > > > > > > > I already said that: we can't it's root.root 0600 > > > > > currently. All the TSSs seem to change at least /dev/tpmrm > > > > > to tpm.tpm 0660 but we can't do that in the kernel because > > > > > there's no fixed tpm uid/gid. > > > > > > > > Permissions is a pretty good reason to add a sysfs file. > > > > > > > > Jason > > > > > > I'm not sure why suid/sgid utility to read pcrs would be worse. > > > > We don't do root running or suid/sgid binaries any more because > > they're exceptional security risks. That's why both TSSs for TPM > > 2.0 change the device ownership. For Trousers and TPM 1.2 we used > > to run the daemon as root until we started getting CVEs about it. > > > > James > > OK, then a binary blob for pcrs would be sufficient. From a sysfs perspective we only do one value per file and we don't export binary if a valid and useful ascii representation exists. On both of those kernel principles, the current proposal is canonical. James ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v4 1/1] tpm: add sysfs exports for all banks of PCR registers 2020-08-25 15:33 ` James Bottomley @ 2020-08-26 13:15 ` Jarkko Sakkinen 2020-08-26 13:19 ` Jarkko Sakkinen 0 siblings, 1 reply; 54+ messages in thread From: Jarkko Sakkinen @ 2020-08-26 13:15 UTC (permalink / raw) To: James Bottomley; +Cc: Jason Gunthorpe, Mimi Zohar, linux-integrity On Tue, Aug 25, 2020 at 08:33:41AM -0700, James Bottomley wrote: > On Tue, 2020-08-25 at 18:27 +0300, Jarkko Sakkinen wrote: > > On Mon, Aug 24, 2020 at 01:20:46PM -0700, James Bottomley wrote: > > > On Mon, 2020-08-24 at 22:44 +0300, Jarkko Sakkinen wrote: > > > > On Fri, Aug 21, 2020 at 04:38:47PM -0300, Jason Gunthorpe wrote: > > > > > On Thu, Aug 20, 2020 at 09:14:44AM -0700, James Bottomley > > > > > wrote: > > > > > > > > > > > > eg we can't do it because we can't access /dev/tpm for > > > > > > > permissions or something. > > > > > > > > > > > > I already said that: we can't it's root.root 0600 > > > > > > currently. All the TSSs seem to change at least /dev/tpmrm > > > > > > to tpm.tpm 0660 but we can't do that in the kernel because > > > > > > there's no fixed tpm uid/gid. > > > > > > > > > > Permissions is a pretty good reason to add a sysfs file. > > > > > > > > > > Jason > > > > > > > > I'm not sure why suid/sgid utility to read pcrs would be worse. > > > > > > We don't do root running or suid/sgid binaries any more because > > > they're exceptional security risks. That's why both TSSs for TPM > > > 2.0 change the device ownership. For Trousers and TPM 1.2 we used > > > to run the daemon as root until we started getting CVEs about it. > > > > > > James > > > > OK, then a binary blob for pcrs would be sufficient. > > From a sysfs perspective we only do one value per file and we don't > export binary if a valid and useful ascii representation exists. On > both of those kernel principles, the current proposal is canonical. > > James The event log is also exported as a binary. This patch set pollutes the sysfs and adds too much overhead for maintaining. Every single algorithm will needs its own file and needs to be patched to the kernel. A single 'pcrs' blob could with contents as <alg id, data> pairs would remain static. If you speak about principles, please add a reference and/or CC your patch set also to sysfs maintainers. All I care if what is pragmatically the best choice. /Jarkko ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v4 1/1] tpm: add sysfs exports for all banks of PCR registers 2020-08-26 13:15 ` Jarkko Sakkinen @ 2020-08-26 13:19 ` Jarkko Sakkinen 0 siblings, 0 replies; 54+ messages in thread From: Jarkko Sakkinen @ 2020-08-26 13:19 UTC (permalink / raw) To: James Bottomley; +Cc: Jason Gunthorpe, Mimi Zohar, linux-integrity On Wed, Aug 26, 2020 at 04:15:25PM +0300, Jarkko Sakkinen wrote: > On Tue, Aug 25, 2020 at 08:33:41AM -0700, James Bottomley wrote: > > On Tue, 2020-08-25 at 18:27 +0300, Jarkko Sakkinen wrote: > > > On Mon, Aug 24, 2020 at 01:20:46PM -0700, James Bottomley wrote: > > > > On Mon, 2020-08-24 at 22:44 +0300, Jarkko Sakkinen wrote: > > > > > On Fri, Aug 21, 2020 at 04:38:47PM -0300, Jason Gunthorpe wrote: > > > > > > On Thu, Aug 20, 2020 at 09:14:44AM -0700, James Bottomley > > > > > > wrote: > > > > > > > > > > > > > > eg we can't do it because we can't access /dev/tpm for > > > > > > > > permissions or something. > > > > > > > > > > > > > > I already said that: we can't it's root.root 0600 > > > > > > > currently. All the TSSs seem to change at least /dev/tpmrm > > > > > > > to tpm.tpm 0660 but we can't do that in the kernel because > > > > > > > there's no fixed tpm uid/gid. > > > > > > > > > > > > Permissions is a pretty good reason to add a sysfs file. > > > > > > > > > > > > Jason > > > > > > > > > > I'm not sure why suid/sgid utility to read pcrs would be worse. > > > > > > > > We don't do root running or suid/sgid binaries any more because > > > > they're exceptional security risks. That's why both TSSs for TPM > > > > 2.0 change the device ownership. For Trousers and TPM 1.2 we used > > > > to run the daemon as root until we started getting CVEs about it. > > > > > > > > James > > > > > > OK, then a binary blob for pcrs would be sufficient. > > > > From a sysfs perspective we only do one value per file and we don't > > export binary if a valid and useful ascii representation exists. On > > both of those kernel principles, the current proposal is canonical. > > > > James > > The event log is also exported as a binary. This patch set pollutes the > sysfs and adds too much overhead for maintaining. Every single algorithm > will needs its own file and needs to be patched to the kernel. > > A single 'pcrs' blob could with contents as <alg id, data> pairs would > remain static. > > If you speak about principles, please add a reference and/or CC your > patch set also to sysfs maintainers. All I care if what is pragmatically > the best choice. A correction: event log is exported through securityfs I think pcrs should be exported also there instead of sysfs. Does not feel very sound to have these files in different locations. > /Jarkko /Jarkko ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v4 1/1] tpm: add sysfs exports for all banks of PCR registers 2020-08-24 19:44 ` Jarkko Sakkinen 2020-08-24 20:20 ` James Bottomley @ 2020-08-24 21:57 ` Jason Gunthorpe 1 sibling, 0 replies; 54+ messages in thread From: Jason Gunthorpe @ 2020-08-24 21:57 UTC (permalink / raw) To: Jarkko Sakkinen; +Cc: James Bottomley, Mimi Zohar, linux-integrity On Mon, Aug 24, 2020 at 10:44:57PM +0300, Jarkko Sakkinen wrote: > On Fri, Aug 21, 2020 at 04:38:47PM -0300, Jason Gunthorpe wrote: > > On Thu, Aug 20, 2020 at 09:14:44AM -0700, James Bottomley wrote: > > > > > > eg we can't do it because we can't access /dev/tpm for permissions or > > > > something. > > > > > > I already said that: we can't it's root.root 0600 currently. All the > > > TSSs seem to change at least /dev/tpmrm to tpm.tpm 0660 but we can't do > > > that in the kernel because there's no fixed tpm uid/gid. > > > > Permissions is a pretty good reason to add a sysfs file. > > > > Jason > > I'm not sure why suid/sgid utility to read pcrs would be worse. suid would be more complex than this patch - most things ever made suid seem to have security bugs :\ Jason ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v4 1/1] tpm: add sysfs exports for all banks of PCR registers 2020-08-18 18:55 ` Mimi Zohar 2020-08-19 12:02 ` Jason Gunthorpe @ 2020-08-19 22:14 ` Jarkko Sakkinen 1 sibling, 0 replies; 54+ messages in thread From: Jarkko Sakkinen @ 2020-08-19 22:14 UTC (permalink / raw) To: Mimi Zohar; +Cc: Jason Gunthorpe, James Bottomley, linux-integrity On Tue, Aug 18, 2020 at 02:55:50PM -0400, Mimi Zohar wrote: > On Tue, 2020-08-18 at 15:36 -0300, Jason Gunthorpe wrote: > > On Tue, Aug 18, 2020 at 02:26:04PM -0400, Mimi Zohar wrote: > > > On Tue, 2020-08-18 at 13:46 -0300, Jason Gunthorpe wrote: > > > > On Tue, Aug 18, 2020 at 07:26:30PM +0300, Jarkko Sakkinen wrote: > > > > > On Tue, Aug 18, 2020 at 07:19:57PM +0300, Jarkko Sakkinen wrote: > > > > > > On Tue, Aug 18, 2020 at 07:12:09PM +0300, Jarkko Sakkinen wrote: > > > > > > > On Mon, Aug 17, 2020 at 02:35:06PM -0700, James Bottomley wrote: > > > > > > > > Create sysfs per hash groups with 24 PCR files in them one group, > > > > > > > > named pcr-<hash>, for each agile hash of the TPM. The files are > > > > > > > > plugged in to a PCR read function which is TPM version agnostic, so > > > > > > > > this works also for TPM 1.2 but the hash is only sha1 in that case. > > > > > > > > > > > > > > > > Note: the macros used to create the hashes emit spurious checkpatch > > > > > > > > warnings. Do not try to "fix" them as checkpatch recommends, otherwise > > > > > > > > they'll break. > > > > > > > > > > > > > > > > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com> > > > > > > > > Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com> > > > > > > > > Tested-by: Thiago Jung Bauermann <bauerman@linux.ibm.com> > > > > > > > > > > > > > > I have hard time understanding why this is required. > > > > > > > > > > > > > > You can grab the information through /dev/tpm0 just fine. > > > > > > > > > > > > I just think it is principally wrong to add sysfs files if they don't > > > > > > have any measurable value other than perhaps some convenience. > > > > > > > > > > > > It is trival to write only a libc dependent program that outputs PCRs. > > > > > > > > > > > > I think this is essentially an user space problem that is getting sorted > > > > > > out with kernel code. > > > > > > > > > > Jason, what do you make of this? I recall that it was you who I > > > > > discussed with about this topic when TPM 2.0 support was first > > > > > upstreamed. > > > > > > > > TPM 2.0 broke all the userspace so it made sense to get rid of the > > > > non-conforming sysfs files from TPM v1.x time as part of the userspace > > > > API. That was the main reason to not continue forward with PCR in > > > > userspace. > > > > > > > > As far as doing it properly as this patch does.. I agree with you that > > > > sysfs files should have some reason to be added, espcially if it > > > > causes quite big code cost as this does. eg to drive a udev rule > > > > decision. > > > > > > > > Why is PCRs so special it needs to be in sysfs? What is userspace > > > > going to do with this information? > > > > > > The original IMA LTP "boot_aggregate" regression test is dependent on > > > the exported TPM event log and PCRs. Similar support is needed for TPM > > > 2.0. There isn't just a single userspace application for reading > > > PCRs. As soon as we add support for one userspace > > > application, support for the other applications is requested. So > > > instead of a having a simple regression test with a single method of > > > reading PCRs, we're now required to support multiple userspace > > > applications. > > > > But this test already has a C program as part of the boot aggregate > > test, why is it such a problem to use a C program to also read the > > PCRs? > > > > As Jarkko says it is not so hard > > The problem is that there isn't just one single userspace library or > application for reading PCRs. So now not only is there the kernel > "boot_aggregate" regression testing, but regression testing of the tool > itself to support multiple methods of reading the PCRs. > > Mimi It's not hard to write a program to read PCRs with just libc. Takes about one afternoon. /Jarkko ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v4 1/1] tpm: add sysfs exports for all banks of PCR registers 2020-08-18 18:36 ` Jason Gunthorpe 2020-08-18 18:55 ` Mimi Zohar @ 2020-08-18 19:03 ` James Bottomley 2020-08-19 22:13 ` Jarkko Sakkinen 2 siblings, 0 replies; 54+ messages in thread From: James Bottomley @ 2020-08-18 19:03 UTC (permalink / raw) To: Jason Gunthorpe, Mimi Zohar; +Cc: Jarkko Sakkinen, linux-integrity On Tue, 2020-08-18 at 15:36 -0300, Jason Gunthorpe wrote: > On Tue, Aug 18, 2020 at 02:26:04PM -0400, Mimi Zohar wrote: > > On Tue, 2020-08-18 at 13:46 -0300, Jason Gunthorpe wrote: > > > On Tue, Aug 18, 2020 at 07:26:30PM +0300, Jarkko Sakkinen wrote: > > > > On Tue, Aug 18, 2020 at 07:19:57PM +0300, Jarkko Sakkinen > > > > wrote: > > > > > On Tue, Aug 18, 2020 at 07:12:09PM +0300, Jarkko Sakkinen > > > > > wrote: > > > > > > On Mon, Aug 17, 2020 at 02:35:06PM -0700, James Bottomley > > > > > > wrote: > > > > > > > Create sysfs per hash groups with 24 PCR files in them > > > > > > > one group, named pcr-<hash>, for each agile hash of the > > > > > > > TPM. The files are plugged in to a PCR read function > > > > > > > which is TPM version agnostic, so this works also for TPM > > > > > > > 1.2 but the hash is only sha1 in that case. > > > > > > > > > > > > > > Note: the macros used to create the hashes emit spurious > > > > > > > checkpatch warnings. Do not try to "fix" them as > > > > > > > checkpatch recommends, otherwise they'll break. > > > > > > > > > > > > > > Signed-off-by: James Bottomley <James.Bottomley@HansenPar > > > > > > > tnership.com> > > > > > > > Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com> > > > > > > > Tested-by: Thiago Jung Bauermann <bauerman@linux.ibm.com> > > > > > > > > > > > > I have hard time understanding why this is required. > > > > > > > > > > > > You can grab the information through /dev/tpm0 just fine. > > > > > > > > > > I just think it is principally wrong to add sysfs files if > > > > > they don't have any measurable value other than perhaps some > > > > > convenience. > > > > > > > > > > It is trival to write only a libc dependent program that > > > > > outputs PCRs. > > > > > > > > > > I think this is essentially an user space problem that is > > > > > getting sorted out with kernel code. > > > > > > > > Jason, what do you make of this? I recall that it was you who I > > > > discussed with about this topic when TPM 2.0 support was first > > > > upstreamed. > > > > > > TPM 2.0 broke all the userspace so it made sense to get rid of > > > the non-conforming sysfs files from TPM v1.x time as part of the > > > userspace API. That was the main reason to not continue forward > > > with PCR in userspace. > > > > > > As far as doing it properly as this patch does.. I agree with you > > > that sysfs files should have some reason to be added, espcially > > > if it causes quite big code cost as this does. eg to drive a udev > > > rule decision. > > > > > > Why is PCRs so special it needs to be in sysfs? What is userspace > > > going to do with this information? > > > > The original IMA LTP "boot_aggregate" regression test is dependent > > on the exported TPM event log and PCRs. Similar support is needed > > for TPM 2.0. There isn't just a single userspace application for > > reading PCRs. As soon as we add support for one userspace > > application, support for the other applications is requested. So > > instead of a having a simple regression test with a single method > > of reading PCRs, we're now required to support multiple userspace > > applications. > > But this test already has a C program as part of the boot aggregate > test, why is it such a problem to use a C program to also read the > PCRs? > > As Jarkko says it is not so hard It's not so hard for system programmer who owns the root account, like the people who hang out on this list. For your average user, a root only device you're expected to send and receive binary packets of big endian data over is a textbook definition of an unusable interface. I can potentially buy arguments that ordinary users can get this information from TSS based toolkits (although both the Intel and the IBM TSS seem to go out of their way to make it hard to extract). Or even that the average user has no use for the information (although I've given three examples). However, it's not realistic to expect that the average user start speaking binary packet protocols from shell scripts. James ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v4 1/1] tpm: add sysfs exports for all banks of PCR registers 2020-08-18 18:36 ` Jason Gunthorpe 2020-08-18 18:55 ` Mimi Zohar 2020-08-18 19:03 ` James Bottomley @ 2020-08-19 22:13 ` Jarkko Sakkinen 2 siblings, 0 replies; 54+ messages in thread From: Jarkko Sakkinen @ 2020-08-19 22:13 UTC (permalink / raw) To: Jason Gunthorpe; +Cc: Mimi Zohar, James Bottomley, linux-integrity On Tue, Aug 18, 2020 at 03:36:03PM -0300, Jason Gunthorpe wrote: > On Tue, Aug 18, 2020 at 02:26:04PM -0400, Mimi Zohar wrote: > > On Tue, 2020-08-18 at 13:46 -0300, Jason Gunthorpe wrote: > > > On Tue, Aug 18, 2020 at 07:26:30PM +0300, Jarkko Sakkinen wrote: > > > > On Tue, Aug 18, 2020 at 07:19:57PM +0300, Jarkko Sakkinen wrote: > > > > > On Tue, Aug 18, 2020 at 07:12:09PM +0300, Jarkko Sakkinen wrote: > > > > > > On Mon, Aug 17, 2020 at 02:35:06PM -0700, James Bottomley wrote: > > > > > > > Create sysfs per hash groups with 24 PCR files in them one group, > > > > > > > named pcr-<hash>, for each agile hash of the TPM. The files are > > > > > > > plugged in to a PCR read function which is TPM version agnostic, so > > > > > > > this works also for TPM 1.2 but the hash is only sha1 in that case. > > > > > > > > > > > > > > Note: the macros used to create the hashes emit spurious checkpatch > > > > > > > warnings. Do not try to "fix" them as checkpatch recommends, otherwise > > > > > > > they'll break. > > > > > > > > > > > > > > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com> > > > > > > > Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com> > > > > > > > Tested-by: Thiago Jung Bauermann <bauerman@linux.ibm.com> > > > > > > > > > > > > I have hard time understanding why this is required. > > > > > > > > > > > > You can grab the information through /dev/tpm0 just fine. > > > > > > > > > > I just think it is principally wrong to add sysfs files if they don't > > > > > have any measurable value other than perhaps some convenience. > > > > > > > > > > It is trival to write only a libc dependent program that outputs PCRs. > > > > > > > > > > I think this is essentially an user space problem that is getting sorted > > > > > out with kernel code. > > > > > > > > Jason, what do you make of this? I recall that it was you who I > > > > discussed with about this topic when TPM 2.0 support was first > > > > upstreamed. > > > > > > TPM 2.0 broke all the userspace so it made sense to get rid of the > > > non-conforming sysfs files from TPM v1.x time as part of the userspace > > > API. That was the main reason to not continue forward with PCR in > > > userspace. > > > > > > As far as doing it properly as this patch does.. I agree with you that > > > sysfs files should have some reason to be added, espcially if it > > > causes quite big code cost as this does. eg to drive a udev rule > > > decision. > > > > > > Why is PCRs so special it needs to be in sysfs? What is userspace > > > going to do with this information? > > > > The original IMA LTP "boot_aggregate" regression test is dependent on > > the exported TPM event log and PCRs. Similar support is needed for TPM > > 2.0. There isn't just a single userspace application for reading > > PCRs. As soon as we add support for one userspace > > application, support for the other applications is requested. So > > instead of a having a simple regression test with a single method of > > reading PCRs, we're now required to support multiple userspace > > applications. > > But this test already has a C program as part of the boot aggregate > test, why is it such a problem to use a C program to also read the > PCRs? > > As Jarkko says it is not so hard > > Jason "A PCR blob" idea would have the (questionable, I'm not sure if it is useful) benefit of atomic snapshot but it requires a more complex implementation to be efficient. E.g. you would have to read PCRs once in a boot and then intervene PCR manipulating operations, in order to not cause too much run-time stress. Convenience does not cut the deal here. /Jarkko ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v4 1/1] tpm: add sysfs exports for all banks of PCR registers 2020-08-18 18:26 ` Mimi Zohar 2020-08-18 18:36 ` Jason Gunthorpe @ 2020-08-19 22:01 ` Jarkko Sakkinen 1 sibling, 0 replies; 54+ messages in thread From: Jarkko Sakkinen @ 2020-08-19 22:01 UTC (permalink / raw) To: Mimi Zohar; +Cc: Jason Gunthorpe, James Bottomley, linux-integrity On Tue, Aug 18, 2020 at 02:26:04PM -0400, Mimi Zohar wrote: > On Tue, 2020-08-18 at 13:46 -0300, Jason Gunthorpe wrote: > > On Tue, Aug 18, 2020 at 07:26:30PM +0300, Jarkko Sakkinen wrote: > > > On Tue, Aug 18, 2020 at 07:19:57PM +0300, Jarkko Sakkinen wrote: > > > > On Tue, Aug 18, 2020 at 07:12:09PM +0300, Jarkko Sakkinen wrote: > > > > > On Mon, Aug 17, 2020 at 02:35:06PM -0700, James Bottomley wrote: > > > > > > Create sysfs per hash groups with 24 PCR files in them one group, > > > > > > named pcr-<hash>, for each agile hash of the TPM. The files are > > > > > > plugged in to a PCR read function which is TPM version agnostic, so > > > > > > this works also for TPM 1.2 but the hash is only sha1 in that case. > > > > > > > > > > > > Note: the macros used to create the hashes emit spurious checkpatch > > > > > > warnings. Do not try to "fix" them as checkpatch recommends, otherwise > > > > > > they'll break. > > > > > > > > > > > > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com> > > > > > > Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com> > > > > > > Tested-by: Thiago Jung Bauermann <bauerman@linux.ibm.com> > > > > > > > > > > I have hard time understanding why this is required. > > > > > > > > > > You can grab the information through /dev/tpm0 just fine. > > > > > > > > I just think it is principally wrong to add sysfs files if they don't > > > > have any measurable value other than perhaps some convenience. > > > > > > > > It is trival to write only a libc dependent program that outputs PCRs. > > > > > > > > I think this is essentially an user space problem that is getting sorted > > > > out with kernel code. > > > > > > Jason, what do you make of this? I recall that it was you who I > > > discussed with about this topic when TPM 2.0 support was first > > > upstreamed. > > > > TPM 2.0 broke all the userspace so it made sense to get rid of the > > non-conforming sysfs files from TPM v1.x time as part of the userspace > > API. That was the main reason to not continue forward with PCR in > > userspace. > > > > As far as doing it properly as this patch does.. I agree with you that > > sysfs files should have some reason to be added, espcially if it > > causes quite big code cost as this does. eg to drive a udev rule > > decision. > > > > Why is PCRs so special it needs to be in sysfs? What is userspace > > going to do with this information? > > The original IMA LTP "boot_aggregate" regression test is dependent on > the exported TPM event log and PCRs. Similar support is needed for TPM > 2.0. There isn't just a single userspace application for reading > PCRs. As soon as we add support for one userspace > application, support for the other applications is requested. So > instead of a having a simple regression test with a single method of > reading PCRs, we're now required to support multiple userspace > applications. > > I'm definitely in favor of exporting the PCRs, just as the TPM 2.0 > event log and version are now exported. > > Mimi They are very different use cases from this. In both kernel has informatino that the user space does not possess. This is not the case with the PCR values. /Jarkko ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v4 1/1] tpm: add sysfs exports for all banks of PCR registers 2020-08-18 16:19 ` Jarkko Sakkinen 2020-08-18 16:26 ` Jarkko Sakkinen @ 2020-08-18 16:44 ` James Bottomley 2020-08-18 17:17 ` Jason Gunthorpe 2020-08-19 21:33 ` Jarkko Sakkinen 1 sibling, 2 replies; 54+ messages in thread From: James Bottomley @ 2020-08-18 16:44 UTC (permalink / raw) To: Jarkko Sakkinen; +Cc: linux-integrity, Mimi Zohar On Tue, 2020-08-18 at 19:19 +0300, Jarkko Sakkinen wrote: > On Tue, Aug 18, 2020 at 07:12:09PM +0300, Jarkko Sakkinen wrote: > > On Mon, Aug 17, 2020 at 02:35:06PM -0700, James Bottomley wrote: > > > Create sysfs per hash groups with 24 PCR files in them one group, > > > named pcr-<hash>, for each agile hash of the TPM. The files are > > > plugged in to a PCR read function which is TPM version agnostic, > > > so this works also for TPM 1.2 but the hash is only sha1 in that > > > case. > > > > > > Note: the macros used to create the hashes emit spurious > > > checkpatch warnings. Do not try to "fix" them as checkpatch > > > recommends, otherwise they'll break. > > > > > > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership > > > .com> > > > Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com> > > > Tested-by: Thiago Jung Bauermann <bauerman@linux.ibm.com> > > > > I have hard time understanding why this is required. > > > > You can grab the information through /dev/tpm0 just fine. > > I just think it is principally wrong to add sysfs files if they don't > have any measurable value other than perhaps some convenience. That's pretty much the whole point of sysfs (and procfs): to add convenient extraction of information even if it could potentially be obtained by other sources. For instance, the whole reason we add a lot of the broken out inquiry data in SCSI via sysfs is precisely so users don't have to go prodding devices with direct SCSI commands, which are pretty much analagous to TPM device commands. The question you should be asking isn't whether the information *could* be obtained by other means, but whether providing it in this form facilitates current operations and whether the interface would have users. > It is trival to write only a libc dependent program that outputs > PCRs. Not for someone who understands only scripting. There are two problems: firstly the actual construction of TPM commands is somewhat complex and pretty much impossible even for shells like bash to construct and secondly without a TSS installed, the tpm and tpmrm devices are root owned and 0600 permissioned, so a non-root user simply can't use them. > I think this is essentially an user space problem that is getting > sorted out with kernel code. So you'd argue that a kernel shouldn't provide a filesystem because it's simply a seekable key/value retrieval system provided over storage devices (and there are plenty of databases that do it for you on raw devices from userspace)? A big point of a Kernel is to provide a load of convenience interfaces to users even if users could, in theory, do it all themselves. A filesystem is actually a classic example because the directory structure and file API make data organization and retrieval relatively easy for the average user, whereas just presenting them with a SCSI command interface and telling them to use it would be an instant blocker for most of them. The question, again, is not whether a user *could* do this another way but whether the interface provided makes a task (or set of tasks) easier, whether the API provided is easy to use and finally, whether the interface will actually attract any users. James ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v4 1/1] tpm: add sysfs exports for all banks of PCR registers 2020-08-18 16:44 ` James Bottomley @ 2020-08-18 17:17 ` Jason Gunthorpe 2020-08-18 18:49 ` James Bottomley 2020-08-19 21:33 ` Jarkko Sakkinen 1 sibling, 1 reply; 54+ messages in thread From: Jason Gunthorpe @ 2020-08-18 17:17 UTC (permalink / raw) To: James Bottomley; +Cc: Jarkko Sakkinen, linux-integrity, Mimi Zohar On Tue, Aug 18, 2020 at 09:44:30AM -0700, James Bottomley wrote: > The question you should be asking isn't whether the information *could* > be obtained by other means, but whether providing it in this form > facilitates current operations and whether the interface would have > users. Sure. What are the use cases that need PCRs but no other TPM operations? The cover letter didn't say. As PCR is really only useful in the context of the local TPM I'm not thinking of anything.. Jason ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v4 1/1] tpm: add sysfs exports for all banks of PCR registers 2020-08-18 17:17 ` Jason Gunthorpe @ 2020-08-18 18:49 ` James Bottomley 2020-08-19 21:53 ` Jarkko Sakkinen 0 siblings, 1 reply; 54+ messages in thread From: James Bottomley @ 2020-08-18 18:49 UTC (permalink / raw) To: Jason Gunthorpe; +Cc: Jarkko Sakkinen, linux-integrity, Mimi Zohar On Tue, 2020-08-18 at 14:17 -0300, Jason Gunthorpe wrote: > On Tue, Aug 18, 2020 at 09:44:30AM -0700, James Bottomley wrote: > > > The question you should be asking isn't whether the information > > *could* > > be obtained by other means, but whether providing it in this form > > facilitates current operations and whether the interface would have > > users. > > Sure. What are the use cases that need PCRs but no other TPM > operations? > > The cover letter didn't say. As PCR is really only useful in the > context of the local TPM I'm not thinking of anything.. The three use cases I picked up at the Boot and Security MC were: 1. local log verification: a script can run through the IMA ascii log and construct the PCR 10 hash which can then be verified 2. Knowing what the PCR values actually are for sealed keys. With the current trusted key infrastructure you have to calculate and supply the hash yourself. With the new proposed infrastructure, the hash would be calculated by the seal operation, but you're still going to need the actual PCR values to debug unseal failures. 3. As a stability check for log shipping: you read the PCR take the log then read the PCR: if the two reads are the same the PCR backing the log is stable for quoting. James ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v4 1/1] tpm: add sysfs exports for all banks of PCR registers 2020-08-18 18:49 ` James Bottomley @ 2020-08-19 21:53 ` Jarkko Sakkinen 2020-08-19 22:46 ` James Bottomley 0 siblings, 1 reply; 54+ messages in thread From: Jarkko Sakkinen @ 2020-08-19 21:53 UTC (permalink / raw) To: James Bottomley; +Cc: Jason Gunthorpe, linux-integrity, Mimi Zohar > On Tue, 2020-08-18 at 14:17 -0300, Jason Gunthorpe wrote: > > On Tue, Aug 18, 2020 at 09:44:30AM -0700, James Bottomley wrote: > > > > > The question you should be asking isn't whether the information > > > *could* > > > be obtained by other means, but whether providing it in this form > > > facilitates current operations and whether the interface would have > > > users. > > > > Sure. What are the use cases that need PCRs but no other TPM > > operations? > > > > The cover letter didn't say. As PCR is really only useful in the > > context of the local TPM I'm not thinking of anything.. > > The three use cases I picked up at the Boot and Security MC were: > > 1. local log verification: a script can run through the IMA ascii log > and construct the PCR 10 hash which can then be verified > 2. Knowing what the PCR values actually are for sealed keys. With the > current trusted key infrastructure you have to calculate and supply > the hash yourself. With the new proposed infrastructure, the hash > would be calculated by the seal operation, but you're still going to > need the actual PCR values to debug unseal failures. > 3. As a stability check for log shipping: you read the PCR take the log > then read the PCR: if the two reads are the same the PCR backing the > log is stable for quoting. > > James The proposed sysfs attributes are racy in the sense that PCRs could change in-between reading different hashes. A blob containing all the hashes would make more sense as it does not have this issue. If this is for scripts to further process, it is also more efficient than printable ASCII text. /Jarkko ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v4 1/1] tpm: add sysfs exports for all banks of PCR registers 2020-08-19 21:53 ` Jarkko Sakkinen @ 2020-08-19 22:46 ` James Bottomley 2020-08-20 15:22 ` Jarkko Sakkinen 0 siblings, 1 reply; 54+ messages in thread From: James Bottomley @ 2020-08-19 22:46 UTC (permalink / raw) To: Jarkko Sakkinen; +Cc: Jason Gunthorpe, linux-integrity, Mimi Zohar On Thu, 2020-08-20 at 00:53 +0300, Jarkko Sakkinen wrote: > > On Tue, 2020-08-18 at 14:17 -0300, Jason Gunthorpe wrote: > > > On Tue, Aug 18, 2020 at 09:44:30AM -0700, James Bottomley wrote: > > > > > > > The question you should be asking isn't whether the information > > > > *could* be obtained by other means, but whether providing it in > > > > this form facilitates current operations and whether the > > > > interface would have users. > > > > > > Sure. What are the use cases that need PCRs but no other TPM > > > operations? > > > > > > The cover letter didn't say. As PCR is really only useful in the > > > context of the local TPM I'm not thinking of anything.. > > > > The three use cases I picked up at the Boot and Security MC were: > > > > 1. local log verification: a script can run through the IMA > > ascii log > > and construct the PCR 10 hash which can then be verified > > 2. Knowing what the PCR values actually are for sealed > > keys. With the > > current trusted key infrastructure you have to calculate and > > supply > > the hash yourself. With the new proposed infrastructure, the > > hash > > would be calculated by the seal operation, but you're still > > going to > > need the actual PCR values to debug unseal failures. > > 3. As a stability check for log shipping: you read the PCR take > > the log > > then read the PCR: if the two reads are the same the PCR > > backing the > > log is stable for quoting. > > > > James > > The proposed sysfs attributes are racy in the sense that PCRs could > change in-between reading different hashes. That's not really a problem, is it? For use case 2. we expect them to be stable otherwise you're doing the wrong thing sealing to them. For the IMA PCR you use the stability protocol in 3. > A blob containing all the hashes would make more sense as it does not > have this issue. It doesn't really buy anything though. If you're verifying the log you always have the problem that the PCR and the log are at different points, so you follow the protocol in 3. or read PCR then log and unwind the log until it matches or you've gone too far. > If this is for scripts to further process, it is also more efficient > than printable ASCII text. I'm not fundamentally opposed to binary attributes, but realistically if I want the hash of PCRs 1 4 and 6 it's not fundamentally different to me whether I do cat /sys/class/tpm/tpm0/pcr-sha256/1 /sys/class/tpm/tpm0/pcr-sha256/4 /sys/class/tpm/tpm0/pcr-sha256/6|sha256sum or cat /sys/class/tpm/tpm0/pcr-sha256/1 /sys/class/tpm/tpm0/pcr-sha256/4 /sys/class/tpm/tpm0/pcr-sha256/6|xxd -r -p|sha256sum The point being the tool to convert the hex output back to binary already exists and is well known ... and binary attributes have nasty console properties if you accidentally cat them directly. James ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v4 1/1] tpm: add sysfs exports for all banks of PCR registers 2020-08-19 22:46 ` James Bottomley @ 2020-08-20 15:22 ` Jarkko Sakkinen 0 siblings, 0 replies; 54+ messages in thread From: Jarkko Sakkinen @ 2020-08-20 15:22 UTC (permalink / raw) To: James Bottomley; +Cc: Jason Gunthorpe, linux-integrity, Mimi Zohar On Wed, Aug 19, 2020 at 03:46:15PM -0700, James Bottomley wrote: > On Thu, 2020-08-20 at 00:53 +0300, Jarkko Sakkinen wrote: > > > On Tue, 2020-08-18 at 14:17 -0300, Jason Gunthorpe wrote: > > > > On Tue, Aug 18, 2020 at 09:44:30AM -0700, James Bottomley wrote: > > > > > > > > > The question you should be asking isn't whether the information > > > > > *could* be obtained by other means, but whether providing it in > > > > > this form facilitates current operations and whether the > > > > > interface would have users. > > > > > > > > Sure. What are the use cases that need PCRs but no other TPM > > > > operations? > > > > > > > > The cover letter didn't say. As PCR is really only useful in the > > > > context of the local TPM I'm not thinking of anything.. > > > > > > The three use cases I picked up at the Boot and Security MC were: > > > > > > 1. local log verification: a script can run through the IMA > > > ascii log > > > and construct the PCR 10 hash which can then be verified > > > 2. Knowing what the PCR values actually are for sealed > > > keys. With the > > > current trusted key infrastructure you have to calculate and > > > supply > > > the hash yourself. With the new proposed infrastructure, the > > > hash > > > would be calculated by the seal operation, but you're still > > > going to > > > need the actual PCR values to debug unseal failures. > > > 3. As a stability check for log shipping: you read the PCR take > > > the log > > > then read the PCR: if the two reads are the same the PCR > > > backing the > > > log is stable for quoting. > > > > > > James > > > > The proposed sysfs attributes are racy in the sense that PCRs could > > change in-between reading different hashes. > > That's not really a problem, is it? For use case 2. we expect them to > be stable otherwise you're doing the wrong thing sealing to them. For > the IMA PCR you use the stability protocol in 3. > > > A blob containing all the hashes would make more sense as it does not > > have this issue. > > It doesn't really buy anything though. If you're verifying the log you > always have the problem that the PCR and the log are at different > points, so you follow the protocol in 3. or read PCR then log and > unwind the log until it matches or you've gone too far. > > > If this is for scripts to further process, it is also more efficient > > than printable ASCII text. > > I'm not fundamentally opposed to binary attributes, but realistically > if I want the hash of PCRs 1 4 and 6 it's not fundamentally different > to me whether I do > > cat /sys/class/tpm/tpm0/pcr-sha256/1 /sys/class/tpm/tpm0/pcr-sha256/4 /sys/class/tpm/tpm0/pcr-sha256/6|sha256sum > > or > > cat /sys/class/tpm/tpm0/pcr-sha256/1 /sys/class/tpm/tpm0/pcr-sha256/4 /sys/class/tpm/tpm0/pcr-sha256/6|xxd -r -p|sha256sum > > The point being the tool to convert the hex output back to binary > already exists and is well known ... and binary attributes have nasty > console properties if you accidentally cat them directly. > > James This does not look like a kind of framework of things that we want to maintain. Especially given that it is easy to get all the data through /dev/tpm0 easily. It is an enormous addition to uapi with a questionable value. /Jarkko ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v4 1/1] tpm: add sysfs exports for all banks of PCR registers 2020-08-18 16:44 ` James Bottomley 2020-08-18 17:17 ` Jason Gunthorpe @ 2020-08-19 21:33 ` Jarkko Sakkinen 1 sibling, 0 replies; 54+ messages in thread From: Jarkko Sakkinen @ 2020-08-19 21:33 UTC (permalink / raw) To: James Bottomley; +Cc: linux-integrity, Mimi Zohar On Tue, Aug 18, 2020 at 09:44:30AM -0700, James Bottomley wrote: > On Tue, 2020-08-18 at 19:19 +0300, Jarkko Sakkinen wrote: > > On Tue, Aug 18, 2020 at 07:12:09PM +0300, Jarkko Sakkinen wrote: > > > On Mon, Aug 17, 2020 at 02:35:06PM -0700, James Bottomley wrote: > > > > Create sysfs per hash groups with 24 PCR files in them one group, > > > > named pcr-<hash>, for each agile hash of the TPM. The files are > > > > plugged in to a PCR read function which is TPM version agnostic, > > > > so this works also for TPM 1.2 but the hash is only sha1 in that > > > > case. > > > > > > > > Note: the macros used to create the hashes emit spurious > > > > checkpatch warnings. Do not try to "fix" them as checkpatch > > > > recommends, otherwise they'll break. > > > > > > > > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership > > > > .com> > > > > Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com> > > > > Tested-by: Thiago Jung Bauermann <bauerman@linux.ibm.com> > > > > > > I have hard time understanding why this is required. > > > > > > You can grab the information through /dev/tpm0 just fine. > > > > I just think it is principally wrong to add sysfs files if they don't > > have any measurable value other than perhaps some convenience. > > That's pretty much the whole point of sysfs (and procfs): to add > convenient extraction of information even if it could potentially be > obtained by other sources. For instance, the whole reason we add a lot > of the broken out inquiry data in SCSI via sysfs is precisely so users > don't have to go prodding devices with direct SCSI commands, which are > pretty much analagous to TPM device commands. > > The question you should be asking isn't whether the information *could* > be obtained by other means, but whether providing it in this form > facilitates current operations and whether the interface would have > users. Usually users use some appropriate applications to do their work, not talk directly to the kernel. Grabbing PCRs is a trivial program to write and I don't get the logic. My email program is useful for me but I definitely do not want it to be part of the Linux kernel. One great reason for that is that it would involve a tedious process to update it later on. /Jarkko ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v4 1/1] tpm: add sysfs exports for all banks of PCR registers 2020-08-17 21:35 ` [PATCH v4 1/1] tpm: add sysfs exports for all banks of " James Bottomley 2020-08-18 16:12 ` Jarkko Sakkinen @ 2020-09-14 17:41 ` Jarkko Sakkinen 2020-09-14 19:19 ` James Bottomley 2020-10-08 11:45 ` Petr Vorel 2 siblings, 1 reply; 54+ messages in thread From: Jarkko Sakkinen @ 2020-09-14 17:41 UTC (permalink / raw) To: James Bottomley; +Cc: linux-integrity, Mimi Zohar On Mon, Aug 17, 2020 at 02:35:06PM -0700, James Bottomley wrote: > Create sysfs per hash groups with 24 PCR files in them one group, > named pcr-<hash>, for each agile hash of the TPM. The files are > plugged in to a PCR read function which is TPM version agnostic, so > this works also for TPM 1.2 but the hash is only sha1 in that case. > > Note: the macros used to create the hashes emit spurious checkpatch > warnings. Do not try to "fix" them as checkpatch recommends, otherwise > they'll break. "PCR access is required because IMA tools should be able to run without any sort of TSS dependencies." AFAIK, this is the only reason to merge this and it is missing from the description. Perhaps you could either include that sentence, or alternatively write something along the lines? > > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com> > Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com> > Tested-by: Thiago Jung Bauermann <bauerman@linux.ibm.com> Please also cc this at least to Greg and Jason Gunthorpe next time. > --- > > v2: fix TPM 1.2 legacy links failure > v3: fix warn on and add note to tpm_algorithms > v4: reword commit and add tested-by > --- > drivers/char/tpm/tpm-sysfs.c | 178 +++++++++++++++++++++++++++++++++++ > include/linux/tpm.h | 9 +- > 2 files changed, 186 insertions(+), 1 deletion(-) > > diff --git a/drivers/char/tpm/tpm-sysfs.c b/drivers/char/tpm/tpm-sysfs.c > index d52bf4df0bca..81a02200b207 100644 > --- a/drivers/char/tpm/tpm-sysfs.c > +++ b/drivers/char/tpm/tpm-sysfs.c > @@ -348,11 +348,189 @@ static const struct attribute_group tpm2_dev_group = { > .attrs = tpm2_dev_attrs, > }; > > +struct tpm_pcr_attr { > + int alg_id; > + int pcr; > + struct device_attribute attr; > +}; > + > +#define to_tpm_pcr_attr(a) container_of(a, struct tpm_pcr_attr, attr) > + > +static ssize_t pcr_value_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct tpm_pcr_attr *ha = to_tpm_pcr_attr(attr); > + struct tpm_chip *chip = to_tpm_chip(dev); > + struct tpm_digest digest; > + int i; > + int digest_size = 0; > + int rc; > + char *str = buf; > + > + for (i = 0; i < chip->nr_allocated_banks; i++) > + if (ha->alg_id == chip->allocated_banks[i].alg_id) > + digest_size = chip->allocated_banks[i].digest_size; > + /* should never happen */ > + if (!digest_size) > + return -EINVAL; > + > + digest.alg_id = ha->alg_id; > + rc = tpm_pcr_read(chip, ha->pcr, &digest); > + if (rc) > + return rc; > + for (i = 0; i < digest_size; i++) > + str += sprintf(str, "%02X", digest.digest[i]); > + str += sprintf(str, "\n"); > + > + return str - buf; > +} > + > +/* > + * The following set of defines represents all the magic to build > + * the per hash attribute groups for displaying each bank of PCRs. > + * The only slight problem with this approach is that every PCR is > + * hard coded to be present, so you don't know if an PCR is missing > + * until a cat of the file returns -EINVAL > + * > + * Also note you must ignore checkpatch warnings in this macro > + * code. This is deep macro magic that checkpatch.pl doesn't > + * understand. > + */ > + > +/* Note, this must match TPM2_PLATFORM_PCR which is fixed at 24. */ > +#define _TPM_HELPER(_alg, _hash, F) \ > + F(_alg, _hash, 0) \ > + F(_alg, _hash, 1) \ > + F(_alg, _hash, 2) \ > + F(_alg, _hash, 3) \ > + F(_alg, _hash, 4) \ > + F(_alg, _hash, 5) \ > + F(_alg, _hash, 6) \ > + F(_alg, _hash, 7) \ > + F(_alg, _hash, 8) \ > + F(_alg, _hash, 9) \ > + F(_alg, _hash, 10) \ > + F(_alg, _hash, 11) \ > + F(_alg, _hash, 12) \ > + F(_alg, _hash, 13) \ > + F(_alg, _hash, 14) \ > + F(_alg, _hash, 15) \ > + F(_alg, _hash, 16) \ > + F(_alg, _hash, 17) \ > + F(_alg, _hash, 18) \ > + F(_alg, _hash, 19) \ > + F(_alg, _hash, 20) \ > + F(_alg, _hash, 21) \ > + F(_alg, _hash, 22) \ > + F(_alg, _hash, 23) > + > +/* ignore checkpatch warning about trailing ; in macro. */ > +#define PCR_ATTR(_alg, _hash, _pcr) \ > + static struct tpm_pcr_attr dev_attr_pcr_##_hash##_##_pcr = { \ > + .alg_id = _alg, \ > + .pcr = _pcr, \ > + .attr = { \ > + .attr = { \ > + .name = __stringify(_pcr), \ > + .mode = 0444 \ > + }, \ > + .show = pcr_value_show \ > + } \ > + }; > + > +#define PCR_ATTRS(_alg, _hash) \ > + _TPM_HELPER(_alg, _hash, PCR_ATTR) > + > +/* ignore checkpatch warning about trailing , in macro. */ > +#define PCR_ATTR_VAL(_alg, _hash, _pcr) \ > + &dev_attr_pcr_##_hash##_##_pcr.attr.attr, > + > +#define PCR_ATTR_GROUP_ARRAY(_alg, _hash) \ > + static struct attribute *pcr_group_attrs_##_hash[] = { \ > + _TPM_HELPER(_alg, _hash, PCR_ATTR_VAL) \ > + NULL \ > + } > + > +#define PCR_ATTR_GROUP(_alg, _hash) \ > + static struct attribute_group pcr_group_##_hash = { \ > + .name = "pcr-" __stringify(_hash), \ > + .attrs = pcr_group_attrs_##_hash \ > + } > + > +#define PCR_ATTR_BUILD(_alg, _hash) \ > + PCR_ATTRS(_alg, _hash) \ > + PCR_ATTR_GROUP_ARRAY(_alg, _hash); \ > + PCR_ATTR_GROUP(_alg, _hash) > +/* > + * End of macro structure to build an attribute group containing 24 > + * PCR value files for each supported hash algorithm > + */ > + > +/* > + * The next set of macros implements the cleverness for each hash to > + * build a static attribute group called pcr_group_<hash> which can be > + * added to chip->groups[]. > + * > + * The first argument is the TPM algorithm id and the second is the > + * hash used as both the suffix and the group name. Note: the group > + * name is a directory in the top level tpm class with the name > + * pcr-<hash>, so it must not clash with any other names already > + * in the sysfs directory. > + */ > +PCR_ATTR_BUILD(TPM_ALG_SHA1, sha1); > +PCR_ATTR_BUILD(TPM_ALG_SHA256, sha256); > +PCR_ATTR_BUILD(TPM_ALG_SHA384, sha384); > +PCR_ATTR_BUILD(TPM_ALG_SHA512, sha512); > +PCR_ATTR_BUILD(TPM_ALG_SM3_256, sm3); > + > + > void tpm_sysfs_add_device(struct tpm_chip *chip) > { > + int i; > + > WARN_ON(chip->groups_cnt != 0); > + > if (chip->flags & TPM_CHIP_FLAG_TPM2) > chip->groups[chip->groups_cnt++] = &tpm2_dev_group; > else > chip->groups[chip->groups_cnt++] = &tpm1_dev_group; > + > + /* add one group for each bank hash */ > + for (i = 0; i < chip->nr_allocated_banks; i++) { > + switch (chip->allocated_banks[i].alg_id) { > + case TPM_ALG_SHA1: > + chip->groups[chip->groups_cnt++] = &pcr_group_sha1; > + break; > + case TPM_ALG_SHA256: > + chip->groups[chip->groups_cnt++] = &pcr_group_sha256; > + break; > + case TPM_ALG_SHA384: > + chip->groups[chip->groups_cnt++] = &pcr_group_sha384; > + break; > + case TPM_ALG_SHA512: > + chip->groups[chip->groups_cnt++] = &pcr_group_sha512; > + break; > + case TPM_ALG_SM3_256: > + chip->groups[chip->groups_cnt++] = &pcr_group_sm3; > + break; > + default: > + /* > + * If this warning triggers, send a patch to > + * add both a PCR_ATTR_BUILD() macro above for > + * the missing algorithm as well as an > + * additional case in this switch statement. > + */ > + WARN(1, "TPM with unsupported bank algorthm 0x%04x", > + chip->allocated_banks[i].alg_id); > + break; > + } > + } > + > + /* > + * This will only trigger if someone has added an additional > + * hash to the tpm_algorithms enum without incrementing > + * TPM_MAX_HASHES. > + */ > + WARN_ON(chip->groups_cnt > TPM_MAX_HASHES + 1); > } > diff --git a/include/linux/tpm.h b/include/linux/tpm.h > index 3b5d455501c5..cc0b94dcf21e 100644 > --- a/include/linux/tpm.h > +++ b/include/linux/tpm.h > @@ -31,6 +31,7 @@ struct tpm_chip; > struct trusted_key_payload; > struct trusted_key_options; > > +/* if you add a new hash to this, increment TPM_MAX_HASHES below */ > enum tpm_algorithms { > TPM_ALG_ERROR = 0x0000, > TPM_ALG_SHA1 = 0x0004, > @@ -42,6 +43,12 @@ enum tpm_algorithms { > TPM_ALG_SM3_256 = 0x0012, > }; > > +/* > + * maximum number of hashing algorithms a TPM can have. This is > + * basically a count of every hash in tpm_algorithms above > + */ > +#define TPM_MAX_HASHES 5 This is an unnecessarly hard to maintain. A better construction would be this. enum tpm_alg_misc { TPM_ALG_ERROR = 0x0000, TPM_ALG_KEYEDHASH = 0x0008, TPM_ALG_NULL = 0x0010, } enum tpm_alg_hash { TPM_ALG_SHA1 = 0x0004, TPM_ALG_SHA256 = 0x000B, TPM_ALG_SHA384 = 0x000C, TPM_ALG_SHA512 = 0x000D, TPM_ALG_SM3_256 = 0x0012, TPM_ALG_HASH_MAX, }; > + > struct tpm_digest { > u16 alg_id; > u8 digest[TPM_MAX_DIGEST_SIZE]; > @@ -146,7 +153,7 @@ struct tpm_chip { > > struct dentry *bios_dir[TPM_NUM_EVENT_LOG_FILES]; > > - const struct attribute_group *groups[3]; > + const struct attribute_group *groups[3 + TPM_MAX_HASHES]; > unsigned int groups_cnt; > > u32 nr_allocated_banks; /Jarkko ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v4 1/1] tpm: add sysfs exports for all banks of PCR registers 2020-09-14 17:41 ` Jarkko Sakkinen @ 2020-09-14 19:19 ` James Bottomley 2020-09-15 11:22 ` Jarkko Sakkinen 0 siblings, 1 reply; 54+ messages in thread From: James Bottomley @ 2020-09-14 19:19 UTC (permalink / raw) To: Jarkko Sakkinen; +Cc: linux-integrity, Mimi Zohar On Mon, 2020-09-14 at 20:41 +0300, Jarkko Sakkinen wrote: > On Mon, Aug 17, 2020 at 02:35:06PM -0700, James Bottomley wrote: > > Create sysfs per hash groups with 24 PCR files in them one group, > > named pcr-<hash>, for each agile hash of the TPM. The files are > > plugged in to a PCR read function which is TPM version agnostic, so > > this works also for TPM 1.2 but the hash is only sha1 in that case. > > > > Note: the macros used to create the hashes emit spurious checkpatch > > warnings. Do not try to "fix" them as checkpatch recommends, > > otherwise > > they'll break. > > "PCR access is required because IMA tools should be able to run > without any sort of TSS dependencies." > > AFAIK, this is the only reason to merge this and it is missing from > the description. Perhaps you could either include that sentence, or > alternatively write something along the lines? Sure, I'll add all of them: it's IMA tools, early boot and key locking to PCR policy. > > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.c > > om> > > Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com> > > Tested-by: Thiago Jung Bauermann <bauerman@linux.ibm.com> > > Please also cc this at least to Greg and Jason Gunthorpe next time. OK [...] > > > enum tpm_alg_misc { > TPM_ALG_ERROR = 0x0000, > TPM_ALG_KEYEDHASH = 0x0008, > TPM_ALG_NULL = 0x0010, > } > > enum tpm_alg_hash { > TPM_ALG_SHA1 = 0x0004, > TPM_ALG_SHA256 = 0x000B, > TPM_ALG_SHA384 = 0x000C, > TPM_ALG_SHA512 = 0x000D, > TPM_ALG_SM3_256 = 0x0012, > TPM_ALG_HASH_MAX, > }; I can separate them if you insist, but the latter construction won't work. TPM_ALG_HASH_MAX will get set to the previous value plus one. You can see this with the test programme: --- #include <stdio.h> #include <stdlib.h> #include <unistd.h> enum tpm_alg_hash { TPM_ALG_SHA1 = 0x0004, TPM_ALG_SHA256 = 0x000B, TPM_ALG_SHA384 = 0x000C, TPM_ALG_SHA512 = 0x000D, TPM_ALG_SM3_256 = 0x0012, TPM_ALG_HASH_MAX, }; int main() { printf("TPM_ALG_HASH_MAX = %d\n", TPM_ALG_HASH_MAX); } --- Which gives jejb@jarvis> ./a.out TPM_ALG_HASH_MAX = 19 Which is clearly the wrong value (it's 0x12 + 1). That being so, is there any reason to separate up the algorithms enum? James ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v4 1/1] tpm: add sysfs exports for all banks of PCR registers 2020-09-14 19:19 ` James Bottomley @ 2020-09-15 11:22 ` Jarkko Sakkinen 0 siblings, 0 replies; 54+ messages in thread From: Jarkko Sakkinen @ 2020-09-15 11:22 UTC (permalink / raw) To: James Bottomley; +Cc: linux-integrity, Mimi Zohar On Mon, Sep 14, 2020 at 12:19:28PM -0700, James Bottomley wrote: > On Mon, 2020-09-14 at 20:41 +0300, Jarkko Sakkinen wrote: > > On Mon, Aug 17, 2020 at 02:35:06PM -0700, James Bottomley wrote: > > > Create sysfs per hash groups with 24 PCR files in them one group, > > > named pcr-<hash>, for each agile hash of the TPM. The files are > > > plugged in to a PCR read function which is TPM version agnostic, so > > > this works also for TPM 1.2 but the hash is only sha1 in that case. > > > > > > Note: the macros used to create the hashes emit spurious checkpatch > > > warnings. Do not try to "fix" them as checkpatch recommends, > > > otherwise > > > they'll break. > > > > "PCR access is required because IMA tools should be able to run > > without any sort of TSS dependencies." > > > > AFAIK, this is the only reason to merge this and it is missing from > > the description. Perhaps you could either include that sentence, or > > alternatively write something along the lines? > > Sure, I'll add all of them: it's IMA tools, early boot and key locking > to PCR policy. Great! > > > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.c > > > om> > > > Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com> > > > Tested-by: Thiago Jung Bauermann <bauerman@linux.ibm.com> > > > > Please also cc this at least to Greg and Jason Gunthorpe next time. > > OK > > [...] > > > > > > enum tpm_alg_misc { > > TPM_ALG_ERROR = 0x0000, > > TPM_ALG_KEYEDHASH = 0x0008, > > TPM_ALG_NULL = 0x0010, > > } > > > > enum tpm_alg_hash { > > TPM_ALG_SHA1 = 0x0004, > > TPM_ALG_SHA256 = 0x000B, > > TPM_ALG_SHA384 = 0x000C, > > TPM_ALG_SHA512 = 0x000D, > > TPM_ALG_SM3_256 = 0x0012, > > TPM_ALG_HASH_MAX, > > }; > > I can separate them if you insist, but the latter construction won't > work. TPM_ALG_HASH_MAX will get set to the previous value plus one. > > You can see this with the test programme: > > --- > #include <stdio.h> > #include <stdlib.h> > #include <unistd.h> > > enum tpm_alg_hash { > TPM_ALG_SHA1 = 0x0004, > TPM_ALG_SHA256 = 0x000B, > TPM_ALG_SHA384 = 0x000C, > TPM_ALG_SHA512 = 0x000D, > TPM_ALG_SM3_256 = 0x0012, > TPM_ALG_HASH_MAX, > }; > > int main() > { > printf("TPM_ALG_HASH_MAX = %d\n", TPM_ALG_HASH_MAX); > } > --- > > Which gives > > jejb@jarvis> ./a.out > TPM_ALG_HASH_MAX = 19 > > Which is clearly the wrong value (it's 0x12 + 1). > > That being so, is there any reason to separate up the algorithms enum? > > James No, my bad. /Jarkko ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v4 1/1] tpm: add sysfs exports for all banks of PCR registers 2020-08-17 21:35 ` [PATCH v4 1/1] tpm: add sysfs exports for all banks of " James Bottomley 2020-08-18 16:12 ` Jarkko Sakkinen 2020-09-14 17:41 ` Jarkko Sakkinen @ 2020-10-08 11:45 ` Petr Vorel 2020-10-08 14:29 ` James Bottomley 2020-10-09 16:12 ` Jarkko Sakkinen 2 siblings, 2 replies; 54+ messages in thread From: Petr Vorel @ 2020-10-08 11:45 UTC (permalink / raw) To: James Bottomley, Jarkko Sakkinen Cc: linux-integrity, Mimi Zohar, Greg Kroah-Hartman, Jason Gunthorpe Hi, > Create sysfs per hash groups with 24 PCR files in them one group, > named pcr-<hash>, for each agile hash of the TPM. The files are > plugged in to a PCR read function which is TPM version agnostic, so > this works also for TPM 1.2 but the hash is only sha1 in that case. > Note: the macros used to create the hashes emit spurious checkpatch > warnings. Do not try to "fix" them as checkpatch recommends, otherwise > they'll break. > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com> > Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com> > Tested-by: Thiago Jung Bauermann <bauerman@linux.ibm.com> Reviewed-by: Petr Vorel <pvorel@suse.cz> Tested-by: Petr Vorel <pvorel@suse.cz> On both TPM 1.2 and TPM 2.0. /sys/class/tpm/tpm0/pcr-sha*/* is exporting well on both. James, thanks for implementing nice API! BTW are all of these just TPM 1.2 specific? /sys/class/tpm/tpm0/device/enabled /sys/class/tpm/tpm0/device/pcrs /sys/kernel/security/tpm0/binary_bios_measurements Kind regards, Petr ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v4 1/1] tpm: add sysfs exports for all banks of PCR registers 2020-10-08 11:45 ` Petr Vorel @ 2020-10-08 14:29 ` James Bottomley 2020-10-09 16:12 ` Jarkko Sakkinen 1 sibling, 0 replies; 54+ messages in thread From: James Bottomley @ 2020-10-08 14:29 UTC (permalink / raw) To: Petr Vorel, Jarkko Sakkinen Cc: linux-integrity, Mimi Zohar, Greg Kroah-Hartman, Jason Gunthorpe On Thu, 2020-10-08 at 13:45 +0200, Petr Vorel wrote: > Hi, > > > Create sysfs per hash groups with 24 PCR files in them one group, > > named pcr-<hash>, for each agile hash of the TPM. The files are > > plugged in to a PCR read function which is TPM version agnostic, so > > this works also for TPM 1.2 but the hash is only sha1 in that case. > > Note: the macros used to create the hashes emit spurious checkpatch > > warnings. Do not try to "fix" them as checkpatch recommends, > > otherwise > > they'll break. > > Signed-off-by: James Bottomley < > > James.Bottomley@HansenPartnership.com> > > Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com> > > Tested-by: Thiago Jung Bauermann <bauerman@linux.ibm.com> > > Reviewed-by: Petr Vorel <pvorel@suse.cz> > Tested-by: Petr Vorel <pvorel@suse.cz> > On both TPM 1.2 and TPM 2.0. > /sys/class/tpm/tpm0/pcr-sha*/* is exporting well on both. > James, thanks for implementing nice API! You're welcome > BTW are all of these just TPM 1.2 specific? > /sys/class/tpm/tpm0/device/enabled > /sys/class/tpm/tpm0/device/pcrs These two are > /sys/kernel/security/tpm0/binary_bios_measurements But this one isn't ... although the format is different from 1.2 to 2.0 and the 2.0 version didn't appear until around 4.16. James ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v4 1/1] tpm: add sysfs exports for all banks of PCR registers 2020-10-08 11:45 ` Petr Vorel 2020-10-08 14:29 ` James Bottomley @ 2020-10-09 16:12 ` Jarkko Sakkinen 1 sibling, 0 replies; 54+ messages in thread From: Jarkko Sakkinen @ 2020-10-09 16:12 UTC (permalink / raw) To: Petr Vorel Cc: James Bottomley, linux-integrity, Mimi Zohar, Greg Kroah-Hartman, Jason Gunthorpe On Thu, Oct 08, 2020 at 01:45:18PM +0200, Petr Vorel wrote: > Hi, > > > Create sysfs per hash groups with 24 PCR files in them one group, > > named pcr-<hash>, for each agile hash of the TPM. The files are > > plugged in to a PCR read function which is TPM version agnostic, so > > this works also for TPM 1.2 but the hash is only sha1 in that case. > > > Note: the macros used to create the hashes emit spurious checkpatch > > warnings. Do not try to "fix" them as checkpatch recommends, otherwise > > they'll break. > > > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com> > > Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com> > > Tested-by: Thiago Jung Bauermann <bauerman@linux.ibm.com> > > Reviewed-by: Petr Vorel <pvorel@suse.cz> > Tested-by: Petr Vorel <pvorel@suse.cz> > On both TPM 1.2 and TPM 2.0. > /sys/class/tpm/tpm0/pcr-sha*/* is exporting well on both. > James, thanks for implementing nice API! OK, great thank you. Unfortunately too late for v5.10 but without doubt going to v5.11. Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > BTW are all of these just TPM 1.2 specific? > /sys/class/tpm/tpm0/device/enabled > /sys/class/tpm/tpm0/device/pcrs > /sys/kernel/security/tpm0/binary_bios_measurements Yes. > Kind regards, > Petr Thanks a lot for your feedback, easier to judge given that you actually consume this :-) /Jarkko ^ permalink raw reply [flat|nested] 54+ messages in thread
end of thread, other threads:[~2020-10-09 16:12 UTC | newest] Thread overview: 54+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-08-17 21:35 [PATCH v4 0/1] add sysfs exports for TPM 2 PCR registers James Bottomley 2020-08-17 21:35 ` [PATCH v4 1/1] tpm: add sysfs exports for all banks of " James Bottomley 2020-08-18 16:12 ` Jarkko Sakkinen 2020-08-18 16:19 ` Jarkko Sakkinen 2020-08-18 16:26 ` Jarkko Sakkinen 2020-08-18 16:46 ` Jason Gunthorpe 2020-08-18 18:26 ` Mimi Zohar 2020-08-18 18:36 ` Jason Gunthorpe 2020-08-18 18:55 ` Mimi Zohar 2020-08-19 12:02 ` Jason Gunthorpe 2020-08-19 13:27 ` Mimi Zohar 2020-08-19 14:09 ` Jason Gunthorpe 2020-08-19 14:53 ` Mimi Zohar 2020-08-19 14:55 ` Mimi Zohar 2020-08-19 22:16 ` Jarkko Sakkinen 2020-08-19 22:48 ` Jerry Snitselaar 2020-08-19 23:26 ` Jason Gunthorpe 2020-08-20 15:46 ` Jarkko Sakkinen 2020-08-19 14:56 ` Serge E. Hallyn 2020-08-19 22:15 ` Jarkko Sakkinen 2020-08-19 15:17 ` James Bottomley 2020-08-19 16:18 ` Jason Gunthorpe 2020-08-19 16:57 ` Mimi Zohar 2020-08-19 17:17 ` Jason Gunthorpe 2020-08-19 20:09 ` James Bottomley 2020-08-19 23:21 ` Jason Gunthorpe 2020-08-20 16:14 ` James Bottomley 2020-08-20 16:55 ` Serge E. Hallyn 2020-08-21 17:41 ` Jarkko Sakkinen 2020-08-21 19:38 ` Jason Gunthorpe 2020-08-24 19:44 ` Jarkko Sakkinen 2020-08-24 20:20 ` James Bottomley 2020-08-25 15:27 ` Jarkko Sakkinen 2020-08-25 15:33 ` James Bottomley 2020-08-26 13:15 ` Jarkko Sakkinen 2020-08-26 13:19 ` Jarkko Sakkinen 2020-08-24 21:57 ` Jason Gunthorpe 2020-08-19 22:14 ` Jarkko Sakkinen 2020-08-18 19:03 ` James Bottomley 2020-08-19 22:13 ` Jarkko Sakkinen 2020-08-19 22:01 ` Jarkko Sakkinen 2020-08-18 16:44 ` James Bottomley 2020-08-18 17:17 ` Jason Gunthorpe 2020-08-18 18:49 ` James Bottomley 2020-08-19 21:53 ` Jarkko Sakkinen 2020-08-19 22:46 ` James Bottomley 2020-08-20 15:22 ` Jarkko Sakkinen 2020-08-19 21:33 ` Jarkko Sakkinen 2020-09-14 17:41 ` Jarkko Sakkinen 2020-09-14 19:19 ` James Bottomley 2020-09-15 11:22 ` Jarkko Sakkinen 2020-10-08 11:45 ` Petr Vorel 2020-10-08 14:29 ` James Bottomley 2020-10-09 16:12 ` Jarkko Sakkinen
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.