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

[Jarkko Sakkinen <jarkko@kernel.org>]

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