[PATCH] TPM: Fixup pubek sysfs file

[PATCH] TPM: Fixup pubek sysfs file

[Jason Gunthorpe]

tpm_show_pubek is utterly and completely wrong, all the offsets
into the binary blob are wrong, the output is garbage. Do it again.

Before:
Algorithm: 00 0C 00 00
Encscheme: 08 00
Sigscheme: 00 00
Parameters: 00 00 00 00 01 00 AF 6B 30 9B 0D B3
Modulus length: -1266846667
Modulus:
68 45 8D CA A5 EF A8 8A DD 0F D7 84 8E 8D 1F 40
22 92 09 CF 12 C8 9F 6E 55 57 6A 2C A8 0B 5E 45
C7 5E 3D 56 DA 64 E6 E1 F7 8C DD 41 92 28 2E 06
41 02 3E 11 7D B2 C5 46 38 E5 8C 60 D2 96 EE 0C
D6 3D F8 99 E3 02 3A 32 7A 02 C8 31 29 98 28 B9
1B EF 30 A1 A0 45 A0 C0 05 0E C5 96 95 FD 91 47
0A 35 E0 69 B0 8B 49 BD B9 F6 5D 25 21 25 79 1B
20 0D C3 C7 1F 87 5E 5F 41 4B DE 32 DF 55 F3 BD
7F CA D8 7D 3A B4 D5 0A EF CF 8E 72 20 52 15 FA
FB C6 C0 2E C2 AB C6 07 D0 9D 96 6B 2E 30 F7 54
C4 A5 CD 9B 13 54 A0 D1 71 66 91 97 06 12 B5 2D
B2 33 62 FB 56 62 64 A8 AA E9 F2 F4 03 C3 F4 49
2A 09 0D 7D 75 99 6C F0 47 1E 7D D5 A5 CA CE EF
45 B2 DA 88 93 B4 EE EB FB B0 A6 A4 19 C4 B8 0D
04 46 AE BD C5 2E 30 84 49 57 25 34 78 E6 ED C4
50 AF 3B F6 86 43 54 0A D9 DB 54 9C 06 B3 50 7F

After:
Algorithm: 00 00 00 01
Encscheme: 00 03
Sigscheme: 00 01
Parameters: 00 00 08 00 00 00 00 02 00 00 00 00
Modulus length: 256
Modulus:
AF 6B 30 9B 0D B3 B4 7D 74 35 68 45 8D CA A5 EF
A8 8A DD 0F D7 84 8E 8D 1F 40 22 92 09 CF 12 C8
9F 6E 55 57 6A 2C A8 0B 5E 45 C7 5E 3D 56 DA 64
E6 E1 F7 8C DD 41 92 28 2E 06 41 02 3E 11 7D B2
C5 46 38 E5 8C 60 D2 96 EE 0C D6 3D F8 99 E3 02
3A 32 7A 02 C8 31 29 98 28 B9 1B EF 30 A1 A0 45
A0 C0 05 0E C5 96 95 FD 91 47 0A 35 E0 69 B0 8B
49 BD B9 F6 5D 25 21 25 79 1B 20 0D C3 C7 1F 87
5E 5F 41 4B DE 32 DF 55 F3 BD 7F CA D8 7D 3A B4
D5 0A EF CF 8E 72 20 52 15 FA FB C6 C0 2E C2 AB
C6 07 D0 9D 96 6B 2E 30 F7 54 C4 A5 CD 9B 13 54
A0 D1 71 66 91 97 06 12 B5 2D B2 33 62 FB 56 62
64 A8 AA E9 F2 F4 03 C3 F4 49 2A 09 0D 7D 75 99
6C F0 47 1E 7D D5 A5 CA CE EF 45 B2 DA 88 93 B4
EE EB FB B0 A6 A4 19 C4 B8 0D 04 46 AE BD C5 2E
30 84 49 57 25 34 78 E6 ED C4 50 AF 3B F6 86 43

I've checked this decoded output in two different ways.

Tested on a winbond WPCT200

Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
---
 drivers/char/tpm/tpm.c |   77 +++++++++++++++++++++++++++++++++---------------
 1 files changed, 53 insertions(+), 24 deletions(-)

Andrew: More testing found this too. In truth this file is probably
fairly useless since it won't output anything once the TPM is owned,
but if it is going to be there it may as well work right.

diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c
index 5d5b324..196bc48 100644
--- a/drivers/char/tpm/tpm.c
+++ b/drivers/char/tpm/tpm.c
@@ -460,6 +460,11 @@ static ssize_t transmit_cmd(struct tpm_chip *chip, struct tpm_cmd_t *cmd,
 		dev_dbg(chip->dev, "A TPM error (%d) occurred %s\n", err, desc);
 		return err;
 	}
+	if (len != be32_to_cpu(cmd->header.out.length)) {
+		dev_dbg(chip->dev, "TPM returned the wrong length %x %x\n",
+			len, be32_to_cpu(cmd->header.out.length));
+		return -1;
+	}
 	return 0;
 }
 
@@ -811,43 +816,67 @@ ssize_t tpm_show_pubek(struct device *dev, struct device_attribute *attr,
 {
 	u8 *data;
 	struct tpm_cmd_t tpm_cmd;
-	ssize_t err;
-	int i, rc;
+	ssize_t len;
+	int i, rc, key_len;
 	char *str = buf;
 
 	struct tpm_chip *chip = dev_get_drvdata(dev);
 
 	tpm_cmd.header.in = tpm_readpubek_header;
-	err = transmit_cmd(chip, &tpm_cmd, READ_PUBEK_RESULT_SIZE,
-			"attempting to read the PUBEK");
-	if (err)
+	if (transmit_cmd(chip, &tpm_cmd, READ_PUBEK_RESULT_SIZE,
+			 "attempting to read the PUBEK"))
+		goto out;
+
+	len = be32_to_cpu(tpm_cmd.header.out.length);
+	if (len <= 0)
 		goto out;
 
-	/* 
-	   ignore header 10 bytes
-	   algorithm 32 bits (1 == RSA )
-	   encscheme 16 bits
-	   sigscheme 16 bits
-	   parameters (RSA 12->bytes: keybit, #primes, expbit)  
-	   keylenbytes 32 bits
-	   256 byte modulus
-	   ignore checksum 20 bytes
+	/* Format (in bytes):
+	     10 RPC Header (tpm_cmd.header)
+	     xx TPM_PUBKEY pubEK
+		xx TPM_KEY_PARMS
+		  4 algorithmID
+		  2 encScheme
+		  2 sigcScheme
+		  4 parmSize
+		  xx parms (typically 12 bytes for a RSA key)
+		xx TPM_STORE_PUBKEY
+		  4 keyLength
+		  xx key (typically 256 for a RSA key)
+	     20 TPM_DIGEST checksum
 	 */
 	data = tpm_cmd.params.readpubek_out_buffer;
+	len -= 10;
+	if (len < 24)
+		goto out;
 	str +=
 	    sprintf(str,
 		    "Algorithm: %02X %02X %02X %02X\nEncscheme: %02X %02X\n"
 		    "Sigscheme: %02X %02X\nParameters: %02X %02X %02X %02X"
-		    " %02X %02X %02X %02X %02X %02X %02X %02X\n"
-		    "Modulus length: %d\nModulus: \n",
-		    data[10], data[11], data[12], data[13], data[14],
-		    data[15], data[16], data[17], data[22], data[23],
-		    data[24], data[25], data[26], data[27], data[28],
-		    data[29], data[30], data[31], data[32], data[33],
-		    be32_to_cpu(*((__be32 *) (data + 34))));
-
-	for (i = 0; i < 256; i++) {
-		str += sprintf(str, "%02X ", data[i + 38]);
+		    " %02X %02X %02X %02X %02X %02X %02X %02X\n",
+		    data[0], data[1], data[2], data[3],
+		    data[4], data[5],
+		    data[6], data[7],
+		    data[12], data[13], data[14], data[15], data[16], data[17],
+		    data[18], data[19], data[20], data[21], data[22], data[23]);
+	/* Skip TPM_KEY_PARMS.parms and TPM_STORE_PBUKEY.keyLength */
+	i = 4 + 2 + 2 + 4 + be32_to_cpu(*((__be32 *) (data + 8))) + 4;
+	if (i > len)
+		goto out;
+	data += i;
+	len -= i;
+	key_len = be32_to_cpu(*((__be32 *) (data) - 1));
+
+	/* Note: The value printed here is the TPM_STORE_PUBKEY. For RSA keys
+	   this is a modulus, but it can vary for other key types. */
+	str += sprintf(str, "Modulus length: %d\nModulus: \n", key_len);
+
+	/* len is bounded, so key_len is bounded, so sprintf is bounded, so
+	   str doesn't overflow. Well, maybe. */
+	if (key_len > len)
+		goto out;
+	for (i = 0; i < key_len; i++) {
+		str += sprintf(str, "%02X ", data[i]);
 		if ((i + 1) % 16 == 0)
 			str += sprintf(str, "\n");
 	}
-- 
1.5.4.2

Re: [PATCH] TPM: Fixup pubek sysfs file

[Roland Dreier]

 > Algorithm: 00 00 00 01
 > Encscheme: 00 03
 > Sigscheme: 00 01
 > Parameters: 00 00 08 00 00 00 00 02 00 00 00 00
 > Modulus length: 256

No argument that this fix is an improvement, but what happened to the
"one value per file" rule for sysfs??

 - R.

Re: [PATCH] TPM: Fixup pubek sysfs file

[Andrew Morton]

On Thu, 03 Sep 2009 22:02:24 -0700
Roland Dreier <rdreier@cisco.com> wrote:

> 
>  > Algorithm: 00 00 00 01
>  > Encscheme: 00 03
>  > Sigscheme: 00 01
>  > Parameters: 00 00 08 00 00 00 00 02 00 00 00 00
>  > Modulus length: 256
> 
> No argument that this fix is an improvement, but what happened to the
> "one value per file" rule for sysfs??

We goofed.  This one snuck through.

It's one of the reasons why I recommend that people cite examples of
the sysfs/procfs output in changelogs.  These things are part of the
kernel ABI and once they go in we cannot change them so we have to get
them right first time.

Re: [PATCH] TPM: Fixup pubek sysfs file

[Jason Gunthorpe]

On Fri, Sep 04, 2009 at 02:03:25PM -0700, Andrew Morton wrote:
> On Thu, 03 Sep 2009 22:02:24 -0700
> Roland Dreier <rdreier@cisco.com> wrote:
> 
> > 
> >  > Algorithm: 00 00 00 01
> >  > Encscheme: 00 03
> >  > Sigscheme: 00 01
> >  > Parameters: 00 00 08 00 00 00 00 02 00 00 00 00
> >  > Modulus length: 256
> > 
> > No argument that this fix is an improvement, but what happened to the
> > "one value per file" rule for sysfs??
> 
> We goofed.  This one snuck through.
> 
> It's one of the reasons why I recommend that people cite examples of
> the sysfs/procfs output in changelogs.  These things are part of the
> kernel ABI and once they go in we cannot change them so we have to get
> them right first time.

Several of the sysfs files in the TPM driver are goofed:

/sys/devices/platform/tpm_tis.0 # cat caps
Manufacturer: 0x57454300
TCG version: 1.2
Firmware version: 2.16

/sys/devices/platform/tpm_tis.0 # cat pcrs 
PCR-00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
PCR-01: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
PCR-02: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
PCR-03: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
PCR-04: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
PCR-05: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
PCR-06: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
PCR-07: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
PCR-08: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
PCR-09: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
PCR-10: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
PCR-11: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
PCR-12: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
PCR-13: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
PCR-14: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
PCR-15: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
PCR-16: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
PCR-17: FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF 
PCR-18: FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF 
PCR-19: FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF 
PCR-20: FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF 
PCR-21: FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF 
PCR-22: FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF 
PCR-23: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 

/sys/devices/platform/tpm_tis.0 # cat pubek
Algorithm: 00 00 00 01
Encscheme: 00 03
Sigscheme: 00 01
Parameters: 00 00 08 00 00 00 00 02 00 00 00 00
Modulus length: 256
Modulus: 
AF 6B 30 9B 0D B3 B4 7D 74 35 68 45 8D CA A5 EF 
A8 8A DD 0F D7 84 8E 8D 1F 40 22 92 09 CF 12 C8 
9F 6E 55 57 6A 2C A8 0B 5E 45 C7 5E 3D 56 DA 64 
E6 E1 F7 8C DD 41 92 28 2E 06 41 02 3E 11 7D B2 
C5 46 38 E5 8C 60 D2 96 EE 0C D6 3D F8 99 E3 02 
3A 32 7A 02 C8 31 29 98 28 B9 1B EF 30 A1 A0 45 
A0 C0 05 0E C5 96 95 FD 91 47 0A 35 E0 69 B0 8B 
49 BD B9 F6 5D 25 21 25 79 1B 20 0D C3 C7 1F 87 
5E 5F 41 4B DE 32 DF 55 F3 BD 7F CA D8 7D 3A B4 
D5 0A EF CF 8E 72 20 52 15 FA FB C6 C0 2E C2 AB 
C6 07 D0 9D 96 6B 2E 30 F7 54 C4 A5 CD 9B 13 54 
A0 D1 71 66 91 97 06 12 B5 2D B2 33 62 FB 56 62 
64 A8 AA E9 F2 F4 03 C3 F4 49 2A 09 0D 7D 75 99 
6C F0 47 1E 7D D5 A5 CA CE EF 45 B2 DA 88 93 B4 
EE EB FB B0 A6 A4 19 C4 B8 0D 04 46 AE BD C5 2E 
30 84 49 57 25 34 78 E6 ED C4 50 AF 3B F6 86 43 

At least this has been like this since before the git history, so it
isn't recent..

Can these be scheduled for removal or some such process? I haven't
seen anything actually using the files, the only one that is really
useful is caps...

They probably should have been in debugfs, had it existed at the time.

Thanks,
Jason

Re: [PATCH] TPM: Fixup pubek sysfs file

[Greg KH]

On Fri, Sep 04, 2009 at 02:03:25PM -0700, Andrew Morton wrote:
> On Thu, 03 Sep 2009 22:02:24 -0700
> Roland Dreier <rdreier@cisco.com> wrote:
> 
> > 
> >  > Algorithm: 00 00 00 01
> >  > Encscheme: 00 03
> >  > Sigscheme: 00 01
> >  > Parameters: 00 00 08 00 00 00 00 02 00 00 00 00
> >  > Modulus length: 256
> > 
> > No argument that this fix is an improvement, but what happened to the
> > "one value per file" rule for sysfs??
> 
> We goofed.  This one snuck through.
> 
> It's one of the reasons why I recommend that people cite examples of
> the sysfs/procfs output in changelogs.  These things are part of the
> kernel ABI and once they go in we cannot change them so we have to get
> them right first time.

Also, I try to ask for a Documentation/ABI/ update for every new sysfs
file that is added.  That will show this kind of thing quite easily.

thanks,

greg k-h

Re: [PATCH] TPM: Fixup pubek sysfs file

[Greg KH]

On Fri, Sep 04, 2009 at 03:23:16PM -0600, Jason Gunthorpe wrote:
> On Fri, Sep 04, 2009 at 02:03:25PM -0700, Andrew Morton wrote:
> > On Thu, 03 Sep 2009 22:02:24 -0700
> > Roland Dreier <rdreier@cisco.com> wrote:
> > 
> > > 
> > >  > Algorithm: 00 00 00 01
> > >  > Encscheme: 00 03
> > >  > Sigscheme: 00 01
> > >  > Parameters: 00 00 08 00 00 00 00 02 00 00 00 00
> > >  > Modulus length: 256
> > > 
> > > No argument that this fix is an improvement, but what happened to the
> > > "one value per file" rule for sysfs??
> > 
> > We goofed.  This one snuck through.
> > 
> > It's one of the reasons why I recommend that people cite examples of
> > the sysfs/procfs output in changelogs.  These things are part of the
> > kernel ABI and once they go in we cannot change them so we have to get
> > them right first time.
> 
> Several of the sysfs files in the TPM driver are goofed:
> 
> /sys/devices/platform/tpm_tis.0 # cat caps
> Manufacturer: 0x57454300
> TCG version: 1.2
> Firmware version: 2.16
> 
> /sys/devices/platform/tpm_tis.0 # cat pcrs 
> PCR-00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
> PCR-01: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
> PCR-02: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
> PCR-03: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
> PCR-04: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
> PCR-05: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
> PCR-06: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
> PCR-07: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
> PCR-08: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
> PCR-09: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
> PCR-10: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
> PCR-11: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
> PCR-12: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
> PCR-13: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
> PCR-14: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
> PCR-15: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
> PCR-16: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
> PCR-17: FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF 
> PCR-18: FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF 
> PCR-19: FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF 
> PCR-20: FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF 
> PCR-21: FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF 
> PCR-22: FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF 
> PCR-23: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
> 
> /sys/devices/platform/tpm_tis.0 # cat pubek
> Algorithm: 00 00 00 01
> Encscheme: 00 03
> Sigscheme: 00 01
> Parameters: 00 00 08 00 00 00 00 02 00 00 00 00
> Modulus length: 256
> Modulus: 
> AF 6B 30 9B 0D B3 B4 7D 74 35 68 45 8D CA A5 EF 
> A8 8A DD 0F D7 84 8E 8D 1F 40 22 92 09 CF 12 C8 
> 9F 6E 55 57 6A 2C A8 0B 5E 45 C7 5E 3D 56 DA 64 
> E6 E1 F7 8C DD 41 92 28 2E 06 41 02 3E 11 7D B2 
> C5 46 38 E5 8C 60 D2 96 EE 0C D6 3D F8 99 E3 02 
> 3A 32 7A 02 C8 31 29 98 28 B9 1B EF 30 A1 A0 45 
> A0 C0 05 0E C5 96 95 FD 91 47 0A 35 E0 69 B0 8B 
> 49 BD B9 F6 5D 25 21 25 79 1B 20 0D C3 C7 1F 87 
> 5E 5F 41 4B DE 32 DF 55 F3 BD 7F CA D8 7D 3A B4 
> D5 0A EF CF 8E 72 20 52 15 FA FB C6 C0 2E C2 AB 
> C6 07 D0 9D 96 6B 2E 30 F7 54 C4 A5 CD 9B 13 54 
> A0 D1 71 66 91 97 06 12 B5 2D B2 33 62 FB 56 62 
> 64 A8 AA E9 F2 F4 03 C3 F4 49 2A 09 0D 7D 75 99 
> 6C F0 47 1E 7D D5 A5 CA CE EF 45 B2 DA 88 93 B4 
> EE EB FB B0 A6 A4 19 C4 B8 0D 04 46 AE BD C5 2E 
> 30 84 49 57 25 34 78 E6 ED C4 50 AF 3B F6 86 43 
> 
> At least this has been like this since before the git history, so it
> isn't recent..
> 
> Can these be scheduled for removal or some such process? I haven't
> seen anything actually using the files, the only one that is really
> useful is caps...
> 
> They probably should have been in debugfs, had it existed at the time.

Feel free to move them to debugfs, it looks like that is what they are
for.

thanks,

greg k-h

Re: [tpmdd-devel] [PATCH] TPM: Fixup pubek sysfs file

[Jonathan M. McCune]

Hello everybody,

I have decided to chime in as a frequent (and I'm going to claim
"experienced") user of the linux kernel TPM driver.

To the best of my knowledge there is very little code in the wild that
depends on the current structure of these sysfs entries.  Probably the
most used set of libraries and programs -- "TrouSerS" and "TPM Tools"
(trousers.sourceforge.net) -- interact directly with the TPM via
/dev/tpm*.  I'm fairly confident that "tboot" (tboot.sourceforge.net) 
and "jTPM Tools" (trustedjava.sourceforge.net) don't use the sysfs 
entries either.

Thus, fixing the one-item-per-file issues seems reasonable to me. For 
example, changing /sys/devices/platform/tpm_tis/pcrs from a single
multi-entry file to a directory containing files named 0-15 or 0-23 that
each then contain only the relevant 20-byte value seems quite
reasonable. (Today's TPMs contain either 16 or 24 PCRs but future ones
could contain many more.)

I believe the pubek entry has gone broken for so long because once a TPM
has been "owned" (and "taking ownership" is almost certainly the first
thing one does when using the TPM for something), accessing the pubek
needs to be authorized and so the sysfs entry shouldn't return anything
anyways ("Authorization required."?). But it should definitely be fixed
or removed.

Likewise "caps" could be made into a directory containing files for the
relevant data (Manufacturer, TCG version, Firmware version, ...).

What I _disagree_ with is that these values should be moved debugfs.
They are characteristics of a particular hardware device's current state
and I believe fit the definition of what belongs in sysfs reasonably
well.  Their current implementation simply needs help.

The changes I've mentioned are larger and don't seem appropriate for 
2.6.31, but perhaps a plan to have them implemented for an upcoming 
version is reasonable?  Is anybody willing to help put together such a 
patch?  Discuss whether it's reasonable / acceptable to do so?

Thanks,
-Jon


Greg KH wrote:
> On Fri, Sep 04, 2009 at 03:23:16PM -0600, Jason Gunthorpe wrote:
>> On Fri, Sep 04, 2009 at 02:03:25PM -0700, Andrew Morton wrote:
>>> On Thu, 03 Sep 2009 22:02:24 -0700
>>> Roland Dreier <rdreier@cisco.com> wrote:
>>>
>>>>  > Algorithm: 00 00 00 01
>>>>  > Encscheme: 00 03
>>>>  > Sigscheme: 00 01
>>>>  > Parameters: 00 00 08 00 00 00 00 02 00 00 00 00
>>>>  > Modulus length: 256
>>>>
>>>> No argument that this fix is an improvement, but what happened to the
>>>> "one value per file" rule for sysfs??
>>> We goofed.  This one snuck through.
>>>
>>> It's one of the reasons why I recommend that people cite examples of
>>> the sysfs/procfs output in changelogs.  These things are part of the
>>> kernel ABI and once they go in we cannot change them so we have to get
>>> them right first time.
>> Several of the sysfs files in the TPM driver are goofed:
>>
>> /sys/devices/platform/tpm_tis.0 # cat caps
>> Manufacturer: 0x57454300
>> TCG version: 1.2
>> Firmware version: 2.16
>>
>> /sys/devices/platform/tpm_tis.0 # cat pcrs 
>> PCR-00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
>> PCR-01: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
>> PCR-02: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
>> PCR-03: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
>> PCR-04: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
>> PCR-05: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
>> PCR-06: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
>> PCR-07: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
>> PCR-08: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
>> PCR-09: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
>> PCR-10: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
>> PCR-11: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
>> PCR-12: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
>> PCR-13: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
>> PCR-14: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
>> PCR-15: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
>> PCR-16: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
>> PCR-17: FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF 
>> PCR-18: FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF 
>> PCR-19: FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF 
>> PCR-20: FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF 
>> PCR-21: FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF 
>> PCR-22: FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF 
>> PCR-23: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
>>
>> /sys/devices/platform/tpm_tis.0 # cat pubek
>> Algorithm: 00 00 00 01
>> Encscheme: 00 03
>> Sigscheme: 00 01
>> Parameters: 00 00 08 00 00 00 00 02 00 00 00 00
>> Modulus length: 256
>> Modulus: 
>> AF 6B 30 9B 0D B3 B4 7D 74 35 68 45 8D CA A5 EF 
>> A8 8A DD 0F D7 84 8E 8D 1F 40 22 92 09 CF 12 C8 
>> 9F 6E 55 57 6A 2C A8 0B 5E 45 C7 5E 3D 56 DA 64 
>> E6 E1 F7 8C DD 41 92 28 2E 06 41 02 3E 11 7D B2 
>> C5 46 38 E5 8C 60 D2 96 EE 0C D6 3D F8 99 E3 02 
>> 3A 32 7A 02 C8 31 29 98 28 B9 1B EF 30 A1 A0 45 
>> A0 C0 05 0E C5 96 95 FD 91 47 0A 35 E0 69 B0 8B 
>> 49 BD B9 F6 5D 25 21 25 79 1B 20 0D C3 C7 1F 87 
>> 5E 5F 41 4B DE 32 DF 55 F3 BD 7F CA D8 7D 3A B4 
>> D5 0A EF CF 8E 72 20 52 15 FA FB C6 C0 2E C2 AB 
>> C6 07 D0 9D 96 6B 2E 30 F7 54 C4 A5 CD 9B 13 54 
>> A0 D1 71 66 91 97 06 12 B5 2D B2 33 62 FB 56 62 
>> 64 A8 AA E9 F2 F4 03 C3 F4 49 2A 09 0D 7D 75 99 
>> 6C F0 47 1E 7D D5 A5 CA CE EF 45 B2 DA 88 93 B4 
>> EE EB FB B0 A6 A4 19 C4 B8 0D 04 46 AE BD C5 2E 
>> 30 84 49 57 25 34 78 E6 ED C4 50 AF 3B F6 86 43 
>>
>> At least this has been like this since before the git history, so it
>> isn't recent..
>>
>> Can these be scheduled for removal or some such process? I haven't
>> seen anything actually using the files, the only one that is really
>> useful is caps...
>>
>> They probably should have been in debugfs, had it existed at the time.
> 
> Feel free to move them to debugfs, it looks like that is what they are
> for.
> 
> thanks,
> 
> greg k-h
> 
> ------------------------------------------------------------------------------
> Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day 
> trial. Simplify your report design, integration and deployment - and focus on 
> what you do best, core application coding. Discover what's new with 
> Crystal Reports now.  http://p.sf.net/sfu/bobj-july
> _______________________________________________
> tpmdd-devel mailing list
> tpmdd-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/tpmdd-devel
> 

Re: [tpmdd-devel] [PATCH] TPM: Fixup pubek sysfs file

[Rajiv Andrade]

Ok, when reading the other queued patches I saw this discussion and
meant to write about this too.

On Mon, 2009-09-14 at 14:01 -0400, Jonathan M. McCune wrote:
> Hello everybody,
> 
> I have decided to chime in as a frequent (and I'm going to claim
> "experienced") user of the linux kernel TPM driver.
> 
> To the best of my knowledge there is very little code in the wild that
> depends on the current structure of these sysfs entries.  Probably the
> most used set of libraries and programs -- "TrouSerS" and "TPM Tools"
> (trousers.sourceforge.net) -- interact directly with the TPM via
> /dev/tpm*.  I'm fairly confident that "tboot" (tboot.sourceforge.net) 
> and "jTPM Tools" (trustedjava.sourceforge.net) don't use the sysfs 
> entries either.

At least for TrouSerS I can say it doesn't use sysfs at all. tpm-tools
runs on top of TrouSerS, so the same apply. And for jTPM, it can use the
jTSS wrapper, that wraps TrouSerS function calls, so no sysfs here too,
and IAIK jTSS, that's is intented to be cross-platform, so no sysfs here
too I assume. If I'm not wrong tboot doesn't run in user space. So, good
points for us.

> 
> Thus, fixing the one-item-per-file issues seems reasonable to me. For 
> example, changing /sys/devices/platform/tpm_tis/pcrs from a single
> multi-entry file to a directory containing files named 0-15 or 0-23 that
> each then contain only the relevant 20-byte value seems quite
> reasonable. (Today's TPMs contain either 16 or 24 PCRs but future ones
> could contain many more.)
> 

The number of PCRs of a particular TPM can be retrieved sending it the
TPM_GetCapability command, so it's feasible to build this directory
structure dynamically (chip independent).


> The changes I've mentioned are larger and don't seem appropriate for 
> 2.6.31, but perhaps a plan to have them implemented for an upcoming 
> version is reasonable?  Is anybody willing to help put together such a 
> patch?  Discuss whether it's reasonable / acceptable to do so?
> 

Count on me. ;-)

Do you all agree? I'll start writing something/patches and will post
here for further discussions.

Rajiv

> Thanks,
> -Jon
> 
> 
> Greg KH wrote:
> > On Fri, Sep 04, 2009 at 03:23:16PM -0600, Jason Gunthorpe wrote:
> >> On Fri, Sep 04, 2009 at 02:03:25PM -0700, Andrew Morton wrote:
> >>> On Thu, 03 Sep 2009 22:02:24 -0700
> >>> Roland Dreier <rdreier@cisco.com> wrote:
> >>>
> >>>>  > Algorithm: 00 00 00 01
> >>>>  > Encscheme: 00 03
> >>>>  > Sigscheme: 00 01
> >>>>  > Parameters: 00 00 08 00 00 00 00 02 00 00 00 00
> >>>>  > Modulus length: 256
> >>>>
> >>>> No argument that this fix is an improvement, but what happened to the
> >>>> "one value per file" rule for sysfs??
> >>> We goofed.  This one snuck through.
> >>>
> >>> It's one of the reasons why I recommend that people cite examples of
> >>> the sysfs/procfs output in changelogs.  These things are part of the
> >>> kernel ABI and once they go in we cannot change them so we have to get
> >>> them right first time.
> >> Several of the sysfs files in the TPM driver are goofed:
> >>
> >> /sys/devices/platform/tpm_tis.0 # cat caps
> >> Manufacturer: 0x57454300
> >> TCG version: 1.2
> >> Firmware version: 2.16
> >>
> >> /sys/devices/platform/tpm_tis.0 # cat pcrs 
> >> PCR-00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
> >> PCR-01: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
> >> PCR-02: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
> >> PCR-03: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
> >> PCR-04: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
> >> PCR-05: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
> >> PCR-06: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
> >> PCR-07: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
> >> PCR-08: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
> >> PCR-09: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
> >> PCR-10: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
> >> PCR-11: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
> >> PCR-12: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
> >> PCR-13: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
> >> PCR-14: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
> >> PCR-15: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
> >> PCR-16: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
> >> PCR-17: FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF 
> >> PCR-18: FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF 
> >> PCR-19: FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF 
> >> PCR-20: FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF 
> >> PCR-21: FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF 
> >> PCR-22: FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF FF 
> >> PCR-23: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
> >>
> >> /sys/devices/platform/tpm_tis.0 # cat pubek
> >> Algorithm: 00 00 00 01
> >> Encscheme: 00 03
> >> Sigscheme: 00 01
> >> Parameters: 00 00 08 00 00 00 00 02 00 00 00 00
> >> Modulus length: 256
> >> Modulus: 
> >> AF 6B 30 9B 0D B3 B4 7D 74 35 68 45 8D CA A5 EF 
> >> A8 8A DD 0F D7 84 8E 8D 1F 40 22 92 09 CF 12 C8 
> >> 9F 6E 55 57 6A 2C A8 0B 5E 45 C7 5E 3D 56 DA 64 
> >> E6 E1 F7 8C DD 41 92 28 2E 06 41 02 3E 11 7D B2 
> >> C5 46 38 E5 8C 60 D2 96 EE 0C D6 3D F8 99 E3 02 
> >> 3A 32 7A 02 C8 31 29 98 28 B9 1B EF 30 A1 A0 45 
> >> A0 C0 05 0E C5 96 95 FD 91 47 0A 35 E0 69 B0 8B 
> >> 49 BD B9 F6 5D 25 21 25 79 1B 20 0D C3 C7 1F 87 
> >> 5E 5F 41 4B DE 32 DF 55 F3 BD 7F CA D8 7D 3A B4 
> >> D5 0A EF CF 8E 72 20 52 15 FA FB C6 C0 2E C2 AB 
> >> C6 07 D0 9D 96 6B 2E 30 F7 54 C4 A5 CD 9B 13 54 
> >> A0 D1 71 66 91 97 06 12 B5 2D B2 33 62 FB 56 62 
> >> 64 A8 AA E9 F2 F4 03 C3 F4 49 2A 09 0D 7D 75 99 
> >> 6C F0 47 1E 7D D5 A5 CA CE EF 45 B2 DA 88 93 B4 
> >> EE EB FB B0 A6 A4 19 C4 B8 0D 04 46 AE BD C5 2E 
> >> 30 84 49 57 25 34 78 E6 ED C4 50 AF 3B F6 86 43 
> >>
> >> At least this has been like this since before the git history, so it
> >> isn't recent..
> >>
> >> Can these be scheduled for removal or some such process? I haven't
> >> seen anything actually using the files, the only one that is really
> >> useful is caps...
> >>
> >> They probably should have been in debugfs, had it existed at the time.
> > 
> > Feel free to move them to debugfs, it looks like that is what they are
> > for.
> > 
> > thanks,
> > 
> > greg k-h
> > 
> > ------------------------------------------------------------------------------
> > Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day 
> > trial. Simplify your report design, integration and deployment - and focus on 
> > what you do best, core application coding. Discover what's new with 
> > Crystal Reports now.  http://p.sf.net/sfu/bobj-july
> > _______________________________________________
> > tpmdd-devel mailing list
> > tpmdd-devel@lists.sourceforge.net
> > https://lists.sourceforge.net/lists/listinfo/tpmdd-devel
> > 
> 

Re: [tpmdd-devel] [PATCH] TPM: Fixup pubek sysfs file

[Jason Gunthorpe]

On Mon, Sep 14, 2009 at 03:34:39PM -0300, Rajiv Andrade wrote:

> > Thus, fixing the one-item-per-file issues seems reasonable to me. For 
> > example, changing /sys/devices/platform/tpm_tis/pcrs from a single
> > multi-entry file to a directory containing files named 0-15 or 0-23 that
> > each then contain only the relevant 20-byte value seems quite
> > reasonable. (Today's TPMs contain either 16 or 24 PCRs but future ones
> > could contain many more.)

> The number of PCRs of a particular TPM can be retrieved sending it the
> TPM_GetCapability command, so it's feasible to build this directory
> structure dynamically (chip independent).

I think as we've seen, keeping the sysfs stuff working has a real
cost. So that should be balanced with an actual need. Further, talking
to the TPM and handling all the parsing properly is a huge complex
pain, it would be better to have this code gone completely.

Can anyone think of a reason why the PCRs should be accessed from
sysfs? I can't. They are only really useful in connection with other
complex TPM operations. Userspace can fully access them using the
relevant TPM calls.

The pubek should definately go, it is useless in 99% of cases.

The caps file should be split up into 'manufacturer' 'tcg_version'
(please include the spec rev too!!) and maybe 'firmware_version'. This
would at least be useful to udev.

Regards,
Jason

Re: [tpmdd-devel] [PATCH] TPM: Fixup pubek sysfs file

[Jonathan M. McCune]

Inline...

Jason Gunthorpe wrote:
> On Mon, Sep 14, 2009 at 03:34:39PM -0300, Rajiv Andrade wrote:
> 
>>> Thus, fixing the one-item-per-file issues seems reasonable to me. For 
>>> example, changing /sys/devices/platform/tpm_tis/pcrs from a single
>>> multi-entry file to a directory containing files named 0-15 or 0-23 that
>>> each then contain only the relevant 20-byte value seems quite
>>> reasonable. (Today's TPMs contain either 16 or 24 PCRs but future ones
>>> could contain many more.)
> 
>> The number of PCRs of a particular TPM can be retrieved sending it the
>> TPM_GetCapability command, so it's feasible to build this directory
>> structure dynamically (chip independent).
> 
> I think as we've seen, keeping the sysfs stuff working has a real
> cost. So that should be balanced with an actual need. Further, talking
> to the TPM and handling all the parsing properly is a huge complex
> pain, it would be better to have this code gone completely.

The "talking to the TPM" code is indeed somewhat complex, but see my 
comments below regarding IMA (*).

> Can anyone think of a reason why the PCRs should be accessed from
> sysfs? I can't. They are only really useful in connection with other
> complex TPM operations. Userspace can fully access them using the
> relevant TPM calls.

Fair enough, though they are useful in combination with IMA and as a 
sanity check that the driver is working correctly.

> The pubek should definately go, it is useless in 99% of cases.

Seconded.

> The caps file should be split up into 'manufacturer' 'tcg_version'
> (please include the spec rev too!!) and maybe 'firmware_version'. This
> would at least be useful to udev.

Seems reasonable.

* IBM's "Integrity Measurement Architecture (IMA)" is part of the Linux 
kernel as of v2.6.30 (linux-ima.sourceforge.net).  See 
security/integrity/ima/*.  This mechanism can make use of the TPM.  I've 
copied Reiner Sailer, one of the developers of IMA.

IMA can leverage the ability to read and extend PCRs, and its security 
properties derive from that functionality being a part of the kernel. 
PCR reading is currently part of the TPM driver in drivers/char/tpm/tpm.c.

Thus, to the question, "Can we remove the ability of the kernel to send 
TPM commands such as GetCapability and PCRRead?" I believe the answer is 
no. Or at least, not without causing a lot of work for the IMA system. 
Although it may be worth discussing whether this functionality should be 
moved from the driver into IMA.

To the question, "Can we remove the sysfs entries relating to PCRs and 
pubek," I believe the answer is yes, if the resulting complexity 
reduction actually turns out to be significant.

Regards,
-Jon

Re: [tpmdd-devel] [PATCH] TPM: Fixup pubek sysfs file

[Jason Gunthorpe]

On Mon, Sep 14, 2009 at 03:23:09PM -0400, Jonathan M. McCune wrote:

>> Can anyone think of a reason why the PCRs should be accessed from
>> sysfs? I can't. They are only really useful in connection with other
>> complex TPM operations. Userspace can fully access them using the
>> relevant TPM calls.
>
> Fair enough, though they are useful in combination with IMA and as a  
> sanity check that the driver is working correctly.
[..]
> IMA can leverage the ability to read and extend PCRs, and its security  
> properties derive from that functionality being a part of the kernel.  
> PCR reading is currently part of the TPM driver in 
> drivers/char/tpm/tpm.c.

Sanzzy

> Thus, to the question, "Can we remove the ability of the kernel to send  
> TPM commands such as GetCapability and PCRRead?" I believe the answer is  
> no. Or at least, not without causing a lot of work for the IMA system.  
> Although it may be worth discussing whether this functionality should be  
> moved from the driver into IMA.

Fair enough, fortunately TPM_PCRRead and TPM_Extend are two of the
simplest commands. As has been done special casing the fixed size
messages is easy and robust.

The pcrs file really isn't that offsensive to me, the PCR registers
can reasonably thought of as a single memory space. If it wasn't for
the PCR-%u prefix I think it would be fine for sysfs??

Ie:

BD ED 7B B2 EB AE BE 31 4E CC 57 82 81 64 FC 8A 09 FA 19 25 
05 37 25 B5 24 59 14 A5 94 1A 42 97 39 E6 07 40 27 3A 7D 48 
3A 3F 78 0F 11 A4 B4 99 69 FC AA 80 CD 6E 39 57 C3 3B 22 75 

not:

PCR-00: BD ED 7B B2 EB AE BE 31 4E CC 57 82 81 64 FC 8A 09 FA 19 25 
PCR-01: 05 37 25 B5 24 59 14 A5 94 1A 42 97 39 E6 07 40 27 3A 7D 48 
PCR-02: 3A 3F 78 0F 11 A4 B4 99 69 FC AA 80 CD 6E 39 57 C3 3B 22 75 

Greg?

Jason

Re: [tpmdd-devel] [PATCH] TPM: Fixup pubek sysfs file

[Greg KH]

On Mon, Sep 14, 2009 at 01:46:26PM -0600, Jason Gunthorpe wrote:
> On Mon, Sep 14, 2009 at 03:23:09PM -0400, Jonathan M. McCune wrote:
> 
> >> Can anyone think of a reason why the PCRs should be accessed from
> >> sysfs? I can't. They are only really useful in connection with other
> >> complex TPM operations. Userspace can fully access them using the
> >> relevant TPM calls.
> >
> > Fair enough, though they are useful in combination with IMA and as a  
> > sanity check that the driver is working correctly.
> [..]
> > IMA can leverage the ability to read and extend PCRs, and its security  
> > properties derive from that functionality being a part of the kernel.  
> > PCR reading is currently part of the TPM driver in 
> > drivers/char/tpm/tpm.c.
> 
> Sanzzy
> 
> > Thus, to the question, "Can we remove the ability of the kernel to send  
> > TPM commands such as GetCapability and PCRRead?" I believe the answer is  
> > no. Or at least, not without causing a lot of work for the IMA system.  
> > Although it may be worth discussing whether this functionality should be  
> > moved from the driver into IMA.
> 
> Fair enough, fortunately TPM_PCRRead and TPM_Extend are two of the
> simplest commands. As has been done special casing the fixed size
> messages is easy and robust.
> 
> The pcrs file really isn't that offsensive to me, the PCR registers
> can reasonably thought of as a single memory space. If it wasn't for
> the PCR-%u prefix I think it would be fine for sysfs??
> 
> Ie:
> 
> BD ED 7B B2 EB AE BE 31 4E CC 57 82 81 64 FC 8A 09 FA 19 25 
> 05 37 25 B5 24 59 14 A5 94 1A 42 97 39 E6 07 40 27 3A 7D 48 
> 3A 3F 78 0F 11 A4 B4 99 69 FC AA 80 CD 6E 39 57 C3 3B 22 75 
> 
> not:
> 
> PCR-00: BD ED 7B B2 EB AE BE 31 4E CC 57 82 81 64 FC 8A 09 FA 19 25 
> PCR-01: 05 37 25 B5 24 59 14 A5 94 1A 42 97 39 E6 07 40 27 3A 7D 48 
> PCR-02: 3A 3F 78 0F 11 A4 B4 99 69 FC AA 80 CD 6E 39 57 C3 3B 22 75 
> 
> Greg?

That should just be a binary sysfs file, as it shows something that is
represented in hardware, not anything that is modified by the kernel,
right?

thanks,

greg k-h

Re: [PATCH] TPM: Fixup pubek sysfs file

[Rajiv Andrade]

I was about to suggest including some #defines here but the comment
section explaining the format already documents the offsets.

Thanks

Reviewed-by: Rajiv Andrade <srajiv@linux.vnet.ibm.com>

On Thu, 2009-09-03 at 22:52 -0600, Jason Gunthorpe wrote:
> tpm_show_pubek is utterly and completely wrong, all the offsets
> into the binary blob are wrong, the output is garbage. Do it again.
> 
> Before:
> Algorithm: 00 0C 00 00
> Encscheme: 08 00
> Sigscheme: 00 00
> Parameters: 00 00 00 00 01 00 AF 6B 30 9B 0D B3
> Modulus length: -1266846667
> Modulus:
> 68 45 8D CA A5 EF A8 8A DD 0F D7 84 8E 8D 1F 40
> 22 92 09 CF 12 C8 9F 6E 55 57 6A 2C A8 0B 5E 45
> C7 5E 3D 56 DA 64 E6 E1 F7 8C DD 41 92 28 2E 06
> 41 02 3E 11 7D B2 C5 46 38 E5 8C 60 D2 96 EE 0C
> D6 3D F8 99 E3 02 3A 32 7A 02 C8 31 29 98 28 B9
> 1B EF 30 A1 A0 45 A0 C0 05 0E C5 96 95 FD 91 47
> 0A 35 E0 69 B0 8B 49 BD B9 F6 5D 25 21 25 79 1B
> 20 0D C3 C7 1F 87 5E 5F 41 4B DE 32 DF 55 F3 BD
> 7F CA D8 7D 3A B4 D5 0A EF CF 8E 72 20 52 15 FA
> FB C6 C0 2E C2 AB C6 07 D0 9D 96 6B 2E 30 F7 54
> C4 A5 CD 9B 13 54 A0 D1 71 66 91 97 06 12 B5 2D
> B2 33 62 FB 56 62 64 A8 AA E9 F2 F4 03 C3 F4 49
> 2A 09 0D 7D 75 99 6C F0 47 1E 7D D5 A5 CA CE EF
> 45 B2 DA 88 93 B4 EE EB FB B0 A6 A4 19 C4 B8 0D
> 04 46 AE BD C5 2E 30 84 49 57 25 34 78 E6 ED C4
> 50 AF 3B F6 86 43 54 0A D9 DB 54 9C 06 B3 50 7F
> 
> After:
> Algorithm: 00 00 00 01
> Encscheme: 00 03
> Sigscheme: 00 01
> Parameters: 00 00 08 00 00 00 00 02 00 00 00 00
> Modulus length: 256
> Modulus:
> AF 6B 30 9B 0D B3 B4 7D 74 35 68 45 8D CA A5 EF
> A8 8A DD 0F D7 84 8E 8D 1F 40 22 92 09 CF 12 C8
> 9F 6E 55 57 6A 2C A8 0B 5E 45 C7 5E 3D 56 DA 64
> E6 E1 F7 8C DD 41 92 28 2E 06 41 02 3E 11 7D B2
> C5 46 38 E5 8C 60 D2 96 EE 0C D6 3D F8 99 E3 02
> 3A 32 7A 02 C8 31 29 98 28 B9 1B EF 30 A1 A0 45
> A0 C0 05 0E C5 96 95 FD 91 47 0A 35 E0 69 B0 8B
> 49 BD B9 F6 5D 25 21 25 79 1B 20 0D C3 C7 1F 87
> 5E 5F 41 4B DE 32 DF 55 F3 BD 7F CA D8 7D 3A B4
> D5 0A EF CF 8E 72 20 52 15 FA FB C6 C0 2E C2 AB
> C6 07 D0 9D 96 6B 2E 30 F7 54 C4 A5 CD 9B 13 54
> A0 D1 71 66 91 97 06 12 B5 2D B2 33 62 FB 56 62
> 64 A8 AA E9 F2 F4 03 C3 F4 49 2A 09 0D 7D 75 99
> 6C F0 47 1E 7D D5 A5 CA CE EF 45 B2 DA 88 93 B4
> EE EB FB B0 A6 A4 19 C4 B8 0D 04 46 AE BD C5 2E
> 30 84 49 57 25 34 78 E6 ED C4 50 AF 3B F6 86 43
> 
> I've checked this decoded output in two different ways.
> 
> Tested on a winbond WPCT200
> 
> Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> ---
>  drivers/char/tpm/tpm.c |   77 +++++++++++++++++++++++++++++++++---------------
>  1 files changed, 53 insertions(+), 24 deletions(-)
> 
> Andrew: More testing found this too. In truth this file is probably
> fairly useless since it won't output anything once the TPM is owned,
> but if it is going to be there it may as well work right.
> 
> diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c
> index 5d5b324..196bc48 100644
> --- a/drivers/char/tpm/tpm.c
> +++ b/drivers/char/tpm/tpm.c
> @@ -460,6 +460,11 @@ static ssize_t transmit_cmd(struct tpm_chip *chip, struct tpm_cmd_t *cmd,
>  		dev_dbg(chip->dev, "A TPM error (%d) occurred %s\n", err, desc);
>  		return err;
>  	}
> +	if (len != be32_to_cpu(cmd->header.out.length)) {
> +		dev_dbg(chip->dev, "TPM returned the wrong length %x %x\n",
> +			len, be32_to_cpu(cmd->header.out.length));
> +		return -1;
> +	}
>  	return 0;
>  }
> 
> @@ -811,43 +816,67 @@ ssize_t tpm_show_pubek(struct device *dev, struct device_attribute *attr,
>  {
>  	u8 *data;
>  	struct tpm_cmd_t tpm_cmd;
> -	ssize_t err;
> -	int i, rc;
> +	ssize_t len;
> +	int i, rc, key_len;
>  	char *str = buf;
> 
>  	struct tpm_chip *chip = dev_get_drvdata(dev);
> 
>  	tpm_cmd.header.in = tpm_readpubek_header;
> -	err = transmit_cmd(chip, &tpm_cmd, READ_PUBEK_RESULT_SIZE,
> -			"attempting to read the PUBEK");
> -	if (err)
> +	if (transmit_cmd(chip, &tpm_cmd, READ_PUBEK_RESULT_SIZE,
> +			 "attempting to read the PUBEK"))
> +		goto out;
> +
> +	len = be32_to_cpu(tpm_cmd.header.out.length);
> +	if (len <= 0)
>  		goto out;
> 
> -	/* 
> -	   ignore header 10 bytes
> -	   algorithm 32 bits (1 == RSA )
> -	   encscheme 16 bits
> -	   sigscheme 16 bits
> -	   parameters (RSA 12->bytes: keybit, #primes, expbit)  
> -	   keylenbytes 32 bits
> -	   256 byte modulus
> -	   ignore checksum 20 bytes
> +	/* Format (in bytes):
> +	     10 RPC Header (tpm_cmd.header)
> +	     xx TPM_PUBKEY pubEK
> +		xx TPM_KEY_PARMS
> +		  4 algorithmID
> +		  2 encScheme
> +		  2 sigcScheme
> +		  4 parmSize
> +		  xx parms (typically 12 bytes for a RSA key)
> +		xx TPM_STORE_PUBKEY
> +		  4 keyLength
> +		  xx key (typically 256 for a RSA key)
> +	     20 TPM_DIGEST checksum
>  	 */
>  	data = tpm_cmd.params.readpubek_out_buffer;
> +	len -= 10;
> +	if (len < 24)
> +		goto out;
>  	str +=
>  	    sprintf(str,
>  		    "Algorithm: %02X %02X %02X %02X\nEncscheme: %02X %02X\n"
>  		    "Sigscheme: %02X %02X\nParameters: %02X %02X %02X %02X"
> -		    " %02X %02X %02X %02X %02X %02X %02X %02X\n"
> -		    "Modulus length: %d\nModulus: \n",
> -		    data[10], data[11], data[12], data[13], data[14],
> -		    data[15], data[16], data[17], data[22], data[23],
> -		    data[24], data[25], data[26], data[27], data[28],
> -		    data[29], data[30], data[31], data[32], data[33],
> -		    be32_to_cpu(*((__be32 *) (data + 34))));
> -
> -	for (i = 0; i < 256; i++) {
> -		str += sprintf(str, "%02X ", data[i + 38]);
> +		    " %02X %02X %02X %02X %02X %02X %02X %02X\n",
> +		    data[0], data[1], data[2], data[3],
> +		    data[4], data[5],
> +		    data[6], data[7],
> +		    data[12], data[13], data[14], data[15], data[16], data[17],
> +		    data[18], data[19], data[20], data[21], data[22], data[23]);
> +	/* Skip TPM_KEY_PARMS.parms and TPM_STORE_PBUKEY.keyLength */
> +	i = 4 + 2 + 2 + 4 + be32_to_cpu(*((__be32 *) (data + 8))) + 4;
> +	if (i > len)
> +		goto out;
> +	data += i;
> +	len -= i;
> +	key_len = be32_to_cpu(*((__be32 *) (data) - 1));
> +
> +	/* Note: The value printed here is the TPM_STORE_PUBKEY. For RSA keys
> +	   this is a modulus, but it can vary for other key types. */
> +	str += sprintf(str, "Modulus length: %d\nModulus: \n", key_len);
> +
> +	/* len is bounded, so key_len is bounded, so sprintf is bounded, so
> +	   str doesn't overflow. Well, maybe. */
> +	if (key_len > len)
> +		goto out;
> +	for (i = 0; i < key_len; i++) {
> +		str += sprintf(str, "%02X ", data[i]);
>  		if ((i + 1) % 16 == 0)
>  			str += sprintf(str, "\n");
>  	}

Re: [tpmdd-devel] [PATCH] TPM: Fixup pubek sysfs file

[Mimi Zohar]

On Mon, 2009-09-14 at 13:46 -0600, Jason Gunthorpe wrote: 
> On Mon, Sep 14, 2009 at 03:23:09PM -0400, Jonathan M. McCune wrote:
> 
> >> Can anyone think of a reason why the PCRs should be accessed from
> >> sysfs? I can't. They are only really useful in connection with other
> >> complex TPM operations. Userspace can fully access them using the
> >> relevant TPM calls.

> > Fair enough, though they are useful in combination with IMA and as a  
> > sanity check that the driver is working correctly.

The IMA ltp testsuite currently uses the sysfs PCR values 0-7 to
validate the boot aggregate, the first measurement in the the IMA
measurement list. 

> [..]
> > IMA can leverage the ability to read and extend PCRs, and its security  
> > properties derive from that functionality being a part of the kernel.  
> > PCR reading is currently part of the TPM driver in 
> > drivers/char/tpm/tpm.c.
> 
> Sanzzy
> 
> > Thus, to the question, "Can we remove the ability of the kernel to send  
> > TPM commands such as GetCapability and PCRRead?" I believe the answer is  
> > no. Or at least, not without causing a lot of work for the IMA system.

Definitely! On boot, IMA reads pcr 0-7 to calculate the boot aggregate.
IMA then extends the PCR, normally PCR-10, with the boot aggregate and
stores it as the first measurement in the IMA measurement list. Each
subsequent measurement, based on policy, extends the PCR and is appended
to the IMA measurement list. 
  
> > Although it may be worth discussing whether this functionality should be  
> > moved from the driver into IMA.

Are you really suggesting moving the tpm_pcr_read()/extend() to IMA?
Why?

> Fair enough, fortunately TPM_PCRRead and TPM_Extend are two of the
> simplest commands. As has been done special casing the fixed size
> messages is easy and robust.

Mimi

Re: [tpmdd-devel] [PATCH] TPM: Fixup pubek sysfs file

[Hal Finney]

One file-based aspect of the TPM driver interface is the securityfs
file system, typically mounted at /sys/kernel/security/tpm0. This
includes the IMA and/or BIOS event measurements. Trousers does read
these files optionally, however it has to be configured in tcsd.conf.
But the file formats are shared between the kernel driver and the
Trousers parsing code. This functionality is necessary for Trousers to
report pre-boot and post-boot measurement events to user code, which
in turn is necessary for meaningful Quote operations in some contexts.

Hal Finney

On Mon, Sep 14, 2009 at 11:01 AM, Jonathan M. McCune <jonmccune@cmu.edu> wrote:
> Hello everybody,
>
> I have decided to chime in as a frequent (and I'm going to claim
> "experienced") user of the linux kernel TPM driver.
>
> To the best of my knowledge there is very little code in the wild that
> depends on the current structure of these sysfs entries.  Probably the
> most used set of libraries and programs -- "TrouSerS" and "TPM Tools"
> (trousers.sourceforge.net) -- interact directly with the TPM via
> /dev/tpm*.  I'm fairly confident that "tboot" (tboot.sourceforge.net)
> and "jTPM Tools" (trustedjava.sourceforge.net) don't use the sysfs
> entries either.
>
> Thus, fixing the one-item-per-file issues seems reasonable to me. For
> example, changing /sys/devices/platform/tpm_tis/pcrs from a single
> multi-entry file to a directory containing files named 0-15 or 0-23 that
> each then contain only the relevant 20-byte value seems quite
> reasonable. (Today's TPMs contain either 16 or 24 PCRs but future ones
> could contain many more.)
>
> I believe the pubek entry has gone broken for so long because once a TPM
> has been "owned" (and "taking ownership" is almost certainly the first
> thing one does when using the TPM for something), accessing the pubek
> needs to be authorized and so the sysfs entry shouldn't return anything
> anyways ("Authorization required."?). But it should definitely be fixed
> or removed.
>
> Likewise "caps" could be made into a directory containing files for the
> relevant data (Manufacturer, TCG version, Firmware version, ...).
>
> What I _disagree_ with is that these values should be moved debugfs.
> They are characteristics of a particular hardware device's current state
> and I believe fit the definition of what belongs in sysfs reasonably
> well.  Their current implementation simply needs help.
>
> The changes I've mentioned are larger and don't seem appropriate for
> 2.6.31, but perhaps a plan to have them implemented for an upcoming
> version is reasonable?  Is anybody willing to help put together such a
> patch?  Discuss whether it's reasonable / acceptable to do so?
>
> Thanks,
> -Jon

Re: [tpmdd-devel] [PATCH] TPM: Fixup pubek sysfs file

[Jonathan M. McCune]

>   
>>> Although it may be worth discussing whether this functionality should be  
>>> moved from the driver into IMA.
> 
> Are you really suggesting moving the tpm_pcr_read()/extend() to IMA?
> Why?
> 

I mentioned this since I'm not aware of anything else in the kernel that 
uses this functionality.  But this may also be a case of "it ain't 
broke, so don't fix it."

Cheers,
-Jon