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:

quoted

On Wed, Jan 13, 2021 at 03:26:33PM -0800, James Bottomley wrote:

quoted

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 <redacted>
Tested-by: Thiago Jung Bauermann <redacted>

---

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-

quoted

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?

quoted

quoted

+/* 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