Message ID | 20210113232634.23242-2-James.Bottomley@HansenPartnership.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | add sysfs exports for TPM 2 PCR registers | expand |
On Wed, Jan 13, 2021 at 03:26:33PM -0800, 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> > > --- > > 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 > v5: algorithm spelling fix WARN->dev_err > --- > drivers/char/tpm/tpm-sysfs.c | 179 +++++++++++++++++++++++++++++++++++ > include/linux/tpm.h | 9 +- > 2 files changed, 187 insertions(+), 1 deletion(-) > > diff --git a/drivers/char/tpm/tpm-sysfs.c b/drivers/char/tpm/tpm-sysfs.c > index e2ff0b273a0f..63f03cfb8e6a 100644 > --- a/drivers/char/tpm/tpm-sysfs.c > +++ b/drivers/char/tpm/tpm-sysfs.c > @@ -337,11 +337,190 @@ 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"); Please use sysfs_emit() and sysfs_emit_at() for new sysfs files. > +/* 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 \ Can you use __ATTR_RO()? "open" coding the sysfs mode is frowned apon these days. thanks, greg k-h
On Thu, 2021-01-14 at 08:59 +0100, Greg KH wrote: > On Wed, Jan 13, 2021 at 03:26:33PM -0800, 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> > > > > --- > > > > 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 > > v5: algorithm spelling fix WARN->dev_err > > --- > > drivers/char/tpm/tpm-sysfs.c | 179 > > +++++++++++++++++++++++++++++++++++ > > include/linux/tpm.h | 9 +- > > 2 files changed, 187 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/char/tpm/tpm-sysfs.c b/drivers/char/tpm/tpm- > > sysfs.c > > index e2ff0b273a0f..63f03cfb8e6a 100644 > > --- a/drivers/char/tpm/tpm-sysfs.c > > +++ b/drivers/char/tpm/tpm-sysfs.c > > @@ -337,11 +337,190 @@ 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"); > > Please use sysfs_emit() and sysfs_emit_at() for new sysfs files. Hey these interfaces were added after this patch began life. But looking at sysfs_emit_at() I've got to say "aah ... don't you guys ever read rusty's guide to interfaces?" an interface which takes in an absolute page position but returns a relative offset to the position it took in is asking for people to get it wrong. You should always be consistent about uses for inputs and outputs. Basically the only way you can ever use sysfs_emit_at in a show routine is as offset += sysfs_emit_at(buf, offset, ...); because you always need to track the absolute offset. It looks like we already have a couple of bugs in the kernel introduced by this confusion ... return sysfs_emit() vs return sysfs_emit_at() being the most tricky ... > > +/* 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 > > \ > > Can you use __ATTR_RO()? "open" coding the sysfs mode is frowned > apon these days. No because the .show function is the same for every attribute even though the name is different. Somewhere way back at the beginning of this there was a thread about trying to use the ATTR macros, but the problem is there are multiple hash banks that each want files called "1" "2" and so on ... we just can't structure the show functions to be one per the entire attribute set without either including the hash in the name, which we don't want because it's in the directory, or creating clashes in the .show file. James
On Wed, Jan 13, 2021 at 03:26:33PM -0800, 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> > > --- Tested-by: Jarkko Sakkinen <jarkko@kernel.org> Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org> /Jarkko
On Thu, Jan 14, 2021 at 04:21:08PM -0800, James Bottomley wrote: > On Thu, 2021-01-14 at 08:59 +0100, Greg KH wrote: > > On Wed, Jan 13, 2021 at 03:26:33PM -0800, 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> > > > > > > --- > > > > > > 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 > > > v5: algorithm spelling fix WARN->dev_err > > > --- > > > drivers/char/tpm/tpm-sysfs.c | 179 > > > +++++++++++++++++++++++++++++++++++ > > > include/linux/tpm.h | 9 +- > > > 2 files changed, 187 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/char/tpm/tpm-sysfs.c b/drivers/char/tpm/tpm- > > > sysfs.c > > > index e2ff0b273a0f..63f03cfb8e6a 100644 > > > --- a/drivers/char/tpm/tpm-sysfs.c > > > +++ b/drivers/char/tpm/tpm-sysfs.c > > > @@ -337,11 +337,190 @@ 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"); > > > > Please use sysfs_emit() and sysfs_emit_at() for new sysfs files. > > Hey these interfaces were added after this patch began life. But > looking at sysfs_emit_at() I've got to say "aah ... don't you guys ever > read rusty's guide to interfaces?" an interface which takes in an > absolute page position but returns a relative offset to the position it > took in is asking for people to get it wrong. You should always be > consistent about uses for inputs and outputs. Basically the only way > you can ever use sysfs_emit_at in a show routine is as > > offset += sysfs_emit_at(buf, offset, ...); > > because you always need to track the absolute offset. > > It looks like we already have a couple of bugs in the kernel introduced > by this confusion ... return sysfs_emit() vs return sysfs_emit_at() > being the most tricky ... How is using sysfs_emit() different from using snprintf() for the caller, ignoring the added safety measures? I'm new to this API. > > > +/* 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 > > > \ > > > > Can you use __ATTR_RO()? "open" coding the sysfs mode is frowned > > apon these days. > > No because the .show function is the same for every attribute even > though the name is different. Somewhere way back at the beginning of > this there was a thread about trying to use the ATTR macros, but the > problem is there are multiple hash banks that each want files called > "1" "2" and so on ... we just can't structure the show functions to be > one per the entire attribute set without either including the hash in > the name, which we don't want because it's in the directory, or > creating clashes in the .show file. One could introduce __ATTR_RO_SHOW(), like there is __ATTR_RO_MODE(). Not sure if this worth of trouble. > James /Jarkko
On Thu, Jan 14, 2021 at 04:21:08PM -0800, James Bottomley wrote: > On Thu, 2021-01-14 at 08:59 +0100, Greg KH wrote: > > On Wed, Jan 13, 2021 at 03:26:33PM -0800, 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> > > > > > > --- > > > > > > 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 > > > v5: algorithm spelling fix WARN->dev_err > > > --- > > > drivers/char/tpm/tpm-sysfs.c | 179 > > > +++++++++++++++++++++++++++++++++++ > > > include/linux/tpm.h | 9 +- > > > 2 files changed, 187 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/char/tpm/tpm-sysfs.c b/drivers/char/tpm/tpm- > > > sysfs.c > > > index e2ff0b273a0f..63f03cfb8e6a 100644 > > > --- a/drivers/char/tpm/tpm-sysfs.c > > > +++ b/drivers/char/tpm/tpm-sysfs.c > > > @@ -337,11 +337,190 @@ 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"); > > > > Please use sysfs_emit() and sysfs_emit_at() for new sysfs files. > > Hey these interfaces were added after this patch began life. But > looking at sysfs_emit_at() I've got to say "aah ... don't you guys ever > read rusty's guide to interfaces?" an interface which takes in an > absolute page position but returns a relative offset to the position it > took in is asking for people to get it wrong. You should always be > consistent about uses for inputs and outputs. Basically the only way > you can ever use sysfs_emit_at in a show routine is as > > offset += sysfs_emit_at(buf, offset, ...); > > because you always need to track the absolute offset. Well, you shouldn't be doing anything other than a "normal" single value write, so the _at() function should be rare. > It looks like we already have a couple of bugs in the kernel introduced > by this confusion ... return sysfs_emit() vs return sysfs_emit_at() > being the most tricky ... Hm, Joe, you did the conversion to these functions (and wrote the api), care to review this? > > > +/* 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 > > > \ > > > > Can you use __ATTR_RO()? "open" coding the sysfs mode is frowned > > apon these days. > > No because the .show function is the same for every attribute even > though the name is different. Somewhere way back at the beginning of > this there was a thread about trying to use the ATTR macros, but the > problem is there are multiple hash banks that each want files called > "1" "2" and so on ... we just can't structure the show functions to be > one per the entire attribute set without either including the hash in > the name, which we don't want because it's in the directory, or > creating clashes in the .show file. Ah, missed that you were using the same show() function. Ok, makes sense, but it feels odd to have your own attribute type for something as "simple" as a tiny driver like this. But you are maintaining it, not me :) thanks, greg k-h
On Fri, 2021-01-15 at 08:55 +0200, Jarkko Sakkinen wrote: > On Thu, Jan 14, 2021 at 04:21:08PM -0800, James Bottomley wrote: > > On Thu, 2021-01-14 at 08:59 +0100, Greg KH wrote: [...] > > > Please use sysfs_emit() and sysfs_emit_at() for new sysfs files. > > > > Hey these interfaces were added after this patch began life. But > > looking at sysfs_emit_at() I've got to say "aah ... don't you guys > > ever read rusty's guide to interfaces?" an interface which takes in > > an absolute page position but returns a relative offset to the > > position it took in is asking for people to get it wrong. You > > should always be consistent about uses for inputs and > > outputs. Basically the only way you can ever use sysfs_emit_at in > > a show routine is as > > > > offset += sysfs_emit_at(buf, offset, ...); > > > > because you always need to track the absolute offset. > > > > It looks like we already have a couple of bugs in the kernel > > introduced by this confusion ... return sysfs_emit() vs return > > sysfs_emit_at() being the most tricky ... > > How is using sysfs_emit() different from using snprintf() for the > caller, ignoring the added safety measures? I'm new to this API. Using the sprintX variants you maintain a cursor pointer, so they all look like char *cursor = buf; ... cursor += sprintX(cursor, "...", ... ... return cursor - buf; So the input is a relative cursor and the output is the additional offset. I'm not claiming it's the best interface but it is hard to get wrong, just that if we're going to force a new interface we should make it much better. with sysfs_emit_at you use an offset "cursor" but it's hard to know without reading the function how to do it because the return is relative rather than absolute. To have an interface it would be hard to misuse, I think the best way would be to take a pointer to the offset and adjust it after use, so sysfs_emit_at(buf, &offset, ...); That way it returns void so you can't use it in place of return sysfs_emit() And you don't have to worry about whether the return is absolute or relative because it adjusts the pointer for you. The whole point about Rusty and interfaces is that if you are going to invent new interfaces you should make them easy to get right and hard to misuse. A function you can't figure out how to use until you read the source is about 2/10 on the rusty scale: https://ozlabs.org/~rusty/index.cgi/tech/2008-03-30.html James
On Fri, 2021-01-15 at 14:54 +0100, Greg KH wrote: > On Thu, Jan 14, 2021 at 04:21:08PM -0800, James Bottomley wrote: [...] > > It looks like we already have a couple of bugs in the kernel > > introduced by this confusion ... return sysfs_emit() vs return > > sysfs_emit_at() being the most tricky ... > > Hm, Joe, you did the conversion to these functions (and wrote the > api), care to review this? A cursory glance tells me that summary_show in drivers/infiniband/hw/usnic/usnic_ib_sysfs.c has a problem, I think the last = should be += James
On Fri, 2021-01-15 at 09:26 -0800, James Bottomley wrote: > On Fri, 2021-01-15 at 14:54 +0100, Greg KH wrote: > > On Thu, Jan 14, 2021 at 04:21:08PM -0800, James Bottomley wrote: > [...] > > > It looks like we already have a couple of bugs in the kernel > > > introduced by this confusion ... return sysfs_emit() vs return > > > sysfs_emit_at() being the most tricky ... > > > > Hm, Joe, you did the conversion to these functions (and wrote the > > api), care to review this? > > A cursory glance tells me that summary_show in > drivers/infiniband/hw/usnic/usnic_ib_sysfs.c has a problem, I think > the last = should be += The use in drivers/base/node.c:node_read_meminfo() is highly questionable. While currently not emitting wrong code, it depends on len being 0 when passed in to sysfs_emit_at(). That argues it should either be using sysfs_emit() or it should have a len += just in case something gets prepended that makes len non zero. James
On Fri, 2021-01-15 at 14:54 +0100, Greg KH wrote: > On Thu, Jan 14, 2021 at 04:21:08PM -0800, James Bottomley wrote: [] > > It looks like we already have a couple of bugs in the kernel introduced > > by this confusion ... return sysfs_emit() vs return sysfs_emit_at() > > being the most tricky ... > > Hm, Joe, you did the conversion to these functions (and wrote the api), > care to review this? Can you point me at the original submission? I don't see it on lore/linux-api
On Fri, 2021-01-15 at 10:07 -0800, James Bottomley wrote: > On Fri, 2021-01-15 at 09:26 -0800, James Bottomley wrote: > > On Fri, 2021-01-15 at 14:54 +0100, Greg KH wrote: > > > On Thu, Jan 14, 2021 at 04:21:08PM -0800, James Bottomley wrote: > > [...] > > > > It looks like we already have a couple of bugs in the kernel > > > > introduced by this confusion ... return sysfs_emit() vs return > > > > sysfs_emit_at() being the most tricky ... > > > > > > Hm, Joe, you did the conversion to these functions (and wrote the > > > api), care to review this? > > > > A cursory glance tells me that summary_show in > > drivers/infiniband/hw/usnic/usnic_ib_sysfs.c has a problem, I think > > the last = should be += No, it's correct and overwriting what would otherwise be a trailing space. > The use in drivers/base/node.c:node_read_meminfo() is highly > questionable. While currently not emitting wrong code, it depends on > len being 0 when passed in to sysfs_emit_at(). That argues it should > either be using sysfs_emit() or it should have a len += just in case > something gets prepended that makes len non zero. <shrug>, it's currently correct and would be OK to change to += as len is initialized though I think it's extremely doubtful it would ever be changed. sysfs is ABI right so prepending would break things.
On Fri, 2021-01-15 at 12:48 -0800, Joe Perches wrote: > On Fri, 2021-01-15 at 10:07 -0800, James Bottomley wrote: > > On Fri, 2021-01-15 at 09:26 -0800, James Bottomley wrote: > > > On Fri, 2021-01-15 at 14:54 +0100, Greg KH wrote: > > > > On Thu, Jan 14, 2021 at 04:21:08PM -0800, James Bottomley > > > > wrote: > > > [...] > > > > > It looks like we already have a couple of bugs in the kernel > > > > > introduced by this confusion ... return sysfs_emit() vs > > > > > return > > > > > sysfs_emit_at() being the most tricky ... > > > > > > > > Hm, Joe, you did the conversion to these functions (and wrote > > > > the > > > > api), care to review this? > > > > > > A cursory glance tells me that summary_show in > > > drivers/infiniband/hw/usnic/usnic_ib_sysfs.c has a problem, I > > > think the last = should be += > > No, it's correct and overwriting what would otherwise be a trailing > space. The last two lines of summary_show() are len = sysfs_emit_at(buf, len, "\n"); return len; So that always returns 2: the length of "\n", rather than the length of everything you just put into buf, which is what sysfs attributes are supposed to return. James
On Fri, 2021-01-15 at 13:06 -0800, James Bottomley wrote: > On Fri, 2021-01-15 at 12:48 -0800, Joe Perches wrote: > > On Fri, 2021-01-15 at 10:07 -0800, James Bottomley wrote: > > > On Fri, 2021-01-15 at 09:26 -0800, James Bottomley wrote: > > > > On Fri, 2021-01-15 at 14:54 +0100, Greg KH wrote: > > > > > On Thu, Jan 14, 2021 at 04:21:08PM -0800, James Bottomley > > > > > wrote: > > > > [...] > > > > > > It looks like we already have a couple of bugs in the kernel > > > > > > introduced by this confusion ... return sysfs_emit() vs > > > > > > return > > > > > > sysfs_emit_at() being the most tricky ... > > > > > > > > > > Hm, Joe, you did the conversion to these functions (and wrote > > > > > the > > > > > api), care to review this? > > > > > > > > A cursory glance tells me that summary_show in > > > > drivers/infiniband/hw/usnic/usnic_ib_sysfs.c has a problem, I > > > > think the last = should be += > > > > No, it's correct and overwriting what would otherwise be a trailing > > space. > > The last two lines of summary_show() are > > len = sysfs_emit_at(buf, len, "\n"); > > return len; > > So that always returns 2, the length of "\n", 1 rather than 2, but you are otherwise correct. > rather than the length of > everything you just put into buf, which is what sysfs attributes are > supposed to return. Ah, right. My braino mistake. This should not use the sysfs_emit_at return value at all. Patch upcoming...
diff --git a/drivers/char/tpm/tpm-sysfs.c b/drivers/char/tpm/tpm-sysfs.c index e2ff0b273a0f..63f03cfb8e6a 100644 --- a/drivers/char/tpm/tpm-sysfs.c +++ b/drivers/char/tpm/tpm-sysfs.c @@ -337,11 +337,190 @@ 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 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. + */ + dev_err(&chip->dev, + "TPM with unsupported bank algorithm 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 df56d7fbe78f..c4ca52138e8b 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;