diff mbox series

[v5,1/2] tpm: add sysfs exports for all banks of PCR registers

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

Commit Message

James Bottomley Jan. 13, 2021, 11:26 p.m. UTC
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(-)

Comments

Greg KH Jan. 14, 2021, 7:59 a.m. UTC | #1
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
James Bottomley Jan. 15, 2021, 12:21 a.m. UTC | #2
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
Jarkko Sakkinen Jan. 15, 2021, 6:48 a.m. UTC | #3
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
Jarkko Sakkinen Jan. 15, 2021, 6:55 a.m. UTC | #4
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
Greg KH Jan. 15, 2021, 1:54 p.m. UTC | #5
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
James Bottomley Jan. 15, 2021, 5:10 p.m. UTC | #6
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
James Bottomley Jan. 15, 2021, 5:26 p.m. UTC | #7
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
James Bottomley Jan. 15, 2021, 6:07 p.m. UTC | #8
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
Joe Perches Jan. 15, 2021, 8:32 p.m. UTC | #9
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
Joe Perches Jan. 15, 2021, 8:48 p.m. UTC | #10
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.
James Bottomley Jan. 15, 2021, 9:06 p.m. UTC | #11
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
Joe Perches Jan. 15, 2021, 9:14 p.m. UTC | #12
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 mbox series

Patch

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;