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@xxxxxxxxxxxxxxxxxxxxx>
> > > Reviewed-by: Jerry Snitselaar <jsnitsel@xxxxxxxxxx>
> > > Tested-by: Thiago Jung Bauermann <bauerman@xxxxxxxxxxxxx>
> > >
> > > ---
> > >
> > > 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
