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

[James Bottomley <James.Bottomley@HansenPartnership.com>]

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