[PATCH] TPM: Fixup pcrs sysfs file

[PATCH] TPM: Fixup pcrs sysfs file

[Jason Gunthorpe]

I'm testing the tpm_tis low level driver with a winbond WPCT200:
$ cat caps
Manufacturer: 0x57454300
TCG version: 1.2
Firmware version: 2.16

and noted that tpm_pcr_read for the pcrs sysfile file does not function.
tpm_tis_recv returned with an error because the expected reply size was
set to 14 (the request size) and the chip returned 30 bytes.

The TCG spec says the reply size is supposed to be 30 bytes.

First, the BUILD_BUG_ON was surely never correct, testing a run time
value in big endian with that macro is just wrong. I belive the intended
test was to ensure that the cmd buffer has enough space to store the
reply.

Second, the length input to transmit_cmd is the size of the reply, not
of the request. It should be 30 for READ_PCR.

With this change my chip reports all 23 pcrs.

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

diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c
index a6b52d6..8ba0187 100644
--- a/drivers/char/tpm/tpm.c
+++ b/drivers/char/tpm/tpm.c
@@ -696,8 +696,8 @@ int __tpm_pcr_read(struct tpm_chip *chip, int pcr_idx, u8 *res_buf)
 
 	cmd.header.in = pcrread_header;
 	cmd.params.pcrread_in.pcr_idx = cpu_to_be32(pcr_idx);
-	BUILD_BUG_ON(cmd.header.in.length > READ_PCR_RESULT_SIZE);
-	rc = transmit_cmd(chip, &cmd, cmd.header.in.length,
+	BUILD_BUG_ON(sizeof(cmd) < READ_PCR_RESULT_SIZE);
+	rc = transmit_cmd(chip, &cmd, READ_PCR_RESULT_SIZE,
 			  "attempting to read a pcr value");
 
 	if (rc == 0)
-- 
1.5.4.2

Re: [PATCH] TPM: Fixup pcrs sysfs file

[Andrew Morton]

On Tue, 1 Sep 2009 21:16:13 -0600
Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote:

> I'm testing the tpm_tis low level driver with a winbond WPCT200:
> $ cat caps
> Manufacturer: 0x57454300
> TCG version: 1.2
> Firmware version: 2.16
> 
> and noted that tpm_pcr_read for the pcrs sysfile file does not function.
> tpm_tis_recv returned with an error because the expected reply size was
> set to 14 (the request size) and the chip returned 30 bytes.
> 
> The TCG spec says the reply size is supposed to be 30 bytes.
> 
> First, the BUILD_BUG_ON was surely never correct, testing a run time
> value in big endian with that macro is just wrong. I belive the intended
> test was to ensure that the cmd buffer has enough space to store the
> reply.
> 
> Second, the length input to transmit_cmd is the size of the reply, not
> of the request. It should be 30 for READ_PCR.
> 
> With this change my chip reports all 23 pcrs.
> 
> Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> ---
>  drivers/char/tpm/tpm.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c
> index a6b52d6..8ba0187 100644
> --- a/drivers/char/tpm/tpm.c
> +++ b/drivers/char/tpm/tpm.c
> @@ -696,8 +696,8 @@ int __tpm_pcr_read(struct tpm_chip *chip, int pcr_idx, u8 *res_buf)
>  
>  	cmd.header.in = pcrread_header;
>  	cmd.params.pcrread_in.pcr_idx = cpu_to_be32(pcr_idx);
> -	BUILD_BUG_ON(cmd.header.in.length > READ_PCR_RESULT_SIZE);
> -	rc = transmit_cmd(chip, &cmd, cmd.header.in.length,
> +	BUILD_BUG_ON(sizeof(cmd) < READ_PCR_RESULT_SIZE);
> +	rc = transmit_cmd(chip, &cmd, READ_PCR_RESULT_SIZE,
>  			  "attempting to read a pcr value");
>  
>  	if (rc == 0)

That sounds like a fairly serious bug, and this looks like a 2.6.31
patch.

Jan's build_bug_on-fix-it-and-a-couple-of-bogus-uses-of-it.patch (in
-mm) simply removes the bogus BUILD_BUG_ON(). I think we might as well
do that within the context of your patch.

So I end up with the below, which I propose for 2.6.31:



From: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>

I'm testing the tpm_tis low level driver with a winbond WPCT200:

$ cat caps
Manufacturer: 0x57454300
TCG version: 1.2
Firmware version: 2.16

and noted that tpm_pcr_read for the pcrs sysfile file does not function. 
tpm_tis_recv returned with an error because the expected reply size was
set to 14 (the request size) and the chip returned 30 bytes.

The TCG spec says the reply size is supposed to be 30 bytes.

The length input to transmit_cmd is the size of the reply, not of the
request.  It should be 30 for READ_PCR.

With this change my chip reports all 23 pcrs.

Also, the BUILD_BUG_ON() is just wrong - it's testing a value which isn't
a compile-time constant.  Simply remove that assertion.

Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
Cc: Debora Velarde <debora@linux.vnet.ibm.com>
Cc: Rajiv Andrade <srajiv@linux.vnet.ibm.com>
Cc: Marcel Selhorst <m.selhorst@sirrix.com>
Cc: James Morris <jmorris@namei.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 drivers/char/tpm/tpm.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff -puN drivers/char/tpm/tpm.c~tpm-fixup-pcrs-sysfs-file drivers/char/tpm/tpm.c
--- a/drivers/char/tpm/tpm.c~tpm-fixup-pcrs-sysfs-file
+++ a/drivers/char/tpm/tpm.c
@@ -696,8 +696,7 @@ int __tpm_pcr_read(struct tpm_chip *chip
 
 	cmd.header.in = pcrread_header;
 	cmd.params.pcrread_in.pcr_idx = cpu_to_be32(pcr_idx);
-	BUILD_BUG_ON(cmd.header.in.length > READ_PCR_RESULT_SIZE);
-	rc = transmit_cmd(chip, &cmd, cmd.header.in.length,
+	rc = transmit_cmd(chip, &cmd, READ_PCR_RESULT_SIZE,,
 			  "attempting to read a pcr value");
 
 	if (rc == 0)
_

Re: [PATCH] TPM: Fixup pcrs sysfs file

[Jason Gunthorpe]

On Thu, Sep 03, 2009 at 04:52:19PM -0700, Andrew Morton wrote:

> > diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c
> > index a6b52d6..8ba0187 100644
> > +++ b/drivers/char/tpm/tpm.c
> > @@ -696,8 +696,8 @@ int __tpm_pcr_read(struct tpm_chip *chip, int pcr_idx, u8 *res_buf)
> >  
> >  	cmd.header.in = pcrread_header;
> >  	cmd.params.pcrread_in.pcr_idx = cpu_to_be32(pcr_idx);
> > -	BUILD_BUG_ON(cmd.header.in.length > READ_PCR_RESULT_SIZE);
> > -	rc = transmit_cmd(chip, &cmd, cmd.header.in.length,
> > +	BUILD_BUG_ON(sizeof(cmd) < READ_PCR_RESULT_SIZE);
> > +	rc = transmit_cmd(chip, &cmd, READ_PCR_RESULT_SIZE,
> >  			  "attempting to read a pcr value");
> >  
> >  	if (rc == 0)
> 
> That sounds like a fairly serious bug, and this looks like a 2.6.31
> patch.

To be fair, I'm not sure the pcrs sysfile provides anything terribly
usefull.. None of the sysfs files in this driver seem to follow the
standard one-value-one-file convention either. But, if it is going to
be included it may as well work properly...

> Jan's build_bug_on-fix-it-and-a-couple-of-bogus-uses-of-it.patch (in
> -mm) simply removes the bogus BUILD_BUG_ON(). I think we might as well
> do that within the context of your patch.

> So I end up with the below, which I propose for 2.6.31:

OK. That is fair. The tpm_cmd_params union contains a tpm_pcrread_out
which should 'by design' ensure there is enough space.

Jan's removal of the 2nd BUILD_BUG_ON is also good.

But I notice tpm_pcr_extend also has a mis-use of the transmit_cmd
idiom. This one functions ok because the in/out RPC message size
happen to be the same. But lets fix it too?

Thanks,
Jason

>From 25da64a0927088c766745763728c6bcd973d0f4e Mon Sep 17 00:00:00 2001
From: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
Date: Tue, 1 Sep 2009 21:08:55 -0600
Subject: [PATCH] TPM: Fixup pcrs sysfs file

I'm testing the tpm_tis low level driver with a winbond WPCT200:
$ cat caps
Manufacturer: 0x57454300
TCG version: 1.2
Firmware version: 2.16

and noted that tpm_pcr_read for the pcrs sysfile file does not function.
tpm_tis_recv returned with an error because the expected reply size was
set to 14 (the request size) and the chip returned 30 bytes.

The TCG spec says the reply size for READ_PCR is supposed to be 30 bytes.

The length input to transmit_cmd is the size of the reply, not of the
request.

With this change my chip reports all 23 pcrs.

Also fix tpm_pcr_extend to match the idiom of the rest of the code to
prevent future confusion.

Finally, the BUILD_BUG_ON() is just wrong - it's testing a value which
isn't a compile-time constant. Simply remove that assertion, the
buffer is large enough by design.

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

diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c
index a6b52d6..5d5b324 100644
--- a/drivers/char/tpm/tpm.c
+++ b/drivers/char/tpm/tpm.c
@@ -696,8 +696,7 @@ int __tpm_pcr_read(struct tpm_chip *chip, int pcr_idx, u8 *res_buf)
 
 	cmd.header.in = pcrread_header;
 	cmd.params.pcrread_in.pcr_idx = cpu_to_be32(pcr_idx);
-	BUILD_BUG_ON(cmd.header.in.length > READ_PCR_RESULT_SIZE);
-	rc = transmit_cmd(chip, &cmd, cmd.header.in.length,
+	rc = transmit_cmd(chip, &cmd, READ_PCR_RESULT_SIZE,
 			  "attempting to read a pcr value");
 
 	if (rc == 0)
@@ -742,7 +741,7 @@ EXPORT_SYMBOL_GPL(tpm_pcr_read);
  * the module usage count.
  */
 #define TPM_ORD_PCR_EXTEND cpu_to_be32(20)
-#define EXTEND_PCR_SIZE 34
+#define EXTEND_PCR_RESULT_SIZE 34
 static struct tpm_input_header pcrextend_header = {
 	.tag = TPM_TAG_RQU_COMMAND,
 	.length = cpu_to_be32(34),
@@ -760,10 +759,9 @@ int tpm_pcr_extend(u32 chip_num, int pcr_idx, const u8 *hash)
 		return -ENODEV;
 
 	cmd.header.in = pcrextend_header;
-	BUILD_BUG_ON(be32_to_cpu(cmd.header.in.length) > EXTEND_PCR_SIZE);
 	cmd.params.pcrextend_in.pcr_idx = cpu_to_be32(pcr_idx);
 	memcpy(cmd.params.pcrextend_in.hash, hash, TPM_DIGEST_SIZE);
-	rc = transmit_cmd(chip, &cmd, cmd.header.in.length,
+	rc = transmit_cmd(chip, &cmd, EXTEND_PCR_RESULT_SIZE,
 			  "attempting extend a PCR value");
 
 	module_put(chip->dev->driver->owner);
-- 
1.5.4.2

Re: [PATCH] TPM: Fixup pcrs sysfs file

[James Morris]

On Thu, 3 Sep 2009, Jason Gunthorpe wrote:

> > 
> > That sounds like a fairly serious bug, and this looks like a 2.6.31
> > patch.

Any comments from the maintainers on this patch?


> To be fair, I'm not sure the pcrs sysfile provides anything terribly
> usefull.. None of the sysfs files in this driver seem to follow the
> standard one-value-one-file convention either. But, if it is going to
> be included it may as well work properly...
> 
> > Jan's build_bug_on-fix-it-and-a-couple-of-bogus-uses-of-it.patch (in
> > -mm) simply removes the bogus BUILD_BUG_ON(). I think we might as well
> > do that within the context of your patch.
> 
> > So I end up with the below, which I propose for 2.6.31:
> 
> OK. That is fair. The tpm_cmd_params union contains a tpm_pcrread_out
> which should 'by design' ensure there is enough space.
> 
> Jan's removal of the 2nd BUILD_BUG_ON is also good.
> 
> But I notice tpm_pcr_extend also has a mis-use of the transmit_cmd
> idiom. This one functions ok because the in/out RPC message size
> happen to be the same. But lets fix it too?
> 
> Thanks,
> Jason
> 
> >From 25da64a0927088c766745763728c6bcd973d0f4e Mon Sep 17 00:00:00 2001
> From: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> Date: Tue, 1 Sep 2009 21:08:55 -0600
> Subject: [PATCH] TPM: Fixup pcrs sysfs file
> 
> I'm testing the tpm_tis low level driver with a winbond WPCT200:
> $ cat caps
> Manufacturer: 0x57454300
> TCG version: 1.2
> Firmware version: 2.16
> 
> and noted that tpm_pcr_read for the pcrs sysfile file does not function.
> tpm_tis_recv returned with an error because the expected reply size was
> set to 14 (the request size) and the chip returned 30 bytes.
> 
> The TCG spec says the reply size for READ_PCR is supposed to be 30 bytes.
> 
> The length input to transmit_cmd is the size of the reply, not of the
> request.
> 
> With this change my chip reports all 23 pcrs.
> 
> Also fix tpm_pcr_extend to match the idiom of the rest of the code to
> prevent future confusion.
> 
> Finally, the BUILD_BUG_ON() is just wrong - it's testing a value which
> isn't a compile-time constant. Simply remove that assertion, the
> buffer is large enough by design.
> 
> Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> ---
>  drivers/char/tpm/tpm.c |    8 +++-----
>  1 files changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c
> index a6b52d6..5d5b324 100644
> --- a/drivers/char/tpm/tpm.c
> +++ b/drivers/char/tpm/tpm.c
> @@ -696,8 +696,7 @@ int __tpm_pcr_read(struct tpm_chip *chip, int pcr_idx, u8 *res_buf)
>  
>  	cmd.header.in = pcrread_header;
>  	cmd.params.pcrread_in.pcr_idx = cpu_to_be32(pcr_idx);
> -	BUILD_BUG_ON(cmd.header.in.length > READ_PCR_RESULT_SIZE);
> -	rc = transmit_cmd(chip, &cmd, cmd.header.in.length,
> +	rc = transmit_cmd(chip, &cmd, READ_PCR_RESULT_SIZE,
>  			  "attempting to read a pcr value");
>  
>  	if (rc == 0)
> @@ -742,7 +741,7 @@ EXPORT_SYMBOL_GPL(tpm_pcr_read);
>   * the module usage count.
>   */
>  #define TPM_ORD_PCR_EXTEND cpu_to_be32(20)
> -#define EXTEND_PCR_SIZE 34
> +#define EXTEND_PCR_RESULT_SIZE 34
>  static struct tpm_input_header pcrextend_header = {
>  	.tag = TPM_TAG_RQU_COMMAND,
>  	.length = cpu_to_be32(34),
> @@ -760,10 +759,9 @@ int tpm_pcr_extend(u32 chip_num, int pcr_idx, const u8 *hash)
>  		return -ENODEV;
>  
>  	cmd.header.in = pcrextend_header;
> -	BUILD_BUG_ON(be32_to_cpu(cmd.header.in.length) > EXTEND_PCR_SIZE);
>  	cmd.params.pcrextend_in.pcr_idx = cpu_to_be32(pcr_idx);
>  	memcpy(cmd.params.pcrextend_in.hash, hash, TPM_DIGEST_SIZE);
> -	rc = transmit_cmd(chip, &cmd, cmd.header.in.length,
> +	rc = transmit_cmd(chip, &cmd, EXTEND_PCR_RESULT_SIZE,
>  			  "attempting extend a PCR value");
>  
>  	module_put(chip->dev->driver->owner);
> -- 
> 1.5.4.2
> 

-- 
James Morris
<jmorris@namei.org>

Re: [PATCH] TPM: Fixup pcrs sysfs file

[Jason Gunthorpe]

On Mon, Sep 14, 2009 at 12:36:39PM +1000, James Morris wrote:
> On Thu, 3 Sep 2009, Jason Gunthorpe wrote:

> > > That sounds like a fairly serious bug, and this looks like a 2.6.31
> > > patch.
> 
> Any comments from the maintainers on this patch?

FWIW, I didn't mention in the patch emails, but all the patches I sent
fix regressions that have been introduced in the past couple years by
clean up patches that never tested the code paths they alter.

What becomes of patches that end up in the mm tree? Do they still
route through you for mainline inclusion?

Thanks for looking at this.

Jason

Re: [PATCH] TPM: Fixup pcrs sysfs file

[Andrew Morton]

On Mon, 14 Sep 2009 00:25:20 -0600 Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote:

> On Mon, Sep 14, 2009 at 12:36:39PM +1000, James Morris wrote:
> > On Thu, 3 Sep 2009, Jason Gunthorpe wrote:
> 
> > > > That sounds like a fairly serious bug, and this looks like a 2.6.31
> > > > patch.
> > 
> > Any comments from the maintainers on this patch?
> 
> FWIW, I didn't mention in the patch emails, but all the patches I sent
> fix regressions that have been introduced in the past couple years by
> clean up patches that never tested the code paths they alter.
> 
> What becomes of patches that end up in the mm tree? Do they still
> route through you for mainline inclusion?

I'll be sending tpm patches to James henceforth.  I presently have

tpm-fixup-pcrs-sysfs-file.patch
tpm-fixup-pcrs-sysfs-file-update.patch
tpm-fix-up-pubek-sysfs-file.patch

Re: [PATCH] TPM: Fixup pcrs sysfs file

[Rajiv Andrade]

On Mon, 2009-09-14 at 00:25 -0600, Jason Gunthorpe wrote:
> On Mon, Sep 14, 2009 at 12:36:39PM +1000, James Morris wrote:
> > On Thu, 3 Sep 2009, Jason Gunthorpe wrote:
> 
> > > > That sounds like a fairly serious bug, and this looks like a 2.6.31
> > > > patch.
> > 
> > Any comments from the maintainers on this patch?
> 

Sorry for the delay, just reviewed the patches and Jason is right,
Jason's and Andrew's final work (last patch) is great.

> FWIW, I didn't mention in the patch emails, but all the patches I sent
> fix regressions that have been introduced in the past couple years by
> clean up patches that never tested the code paths they alter.
> 

Hold on, I tested these functions before when submitting them and they
worked fine, and, I couldn't figure it out why yet (only guesses), they
still work for me here, probably due the mishandling of the be32
formatted value when calling transmit_cmd().


Thanks,
Rajiv

Re: [PATCH] TPM: Fixup pcrs sysfs file

[Rajiv Andrade]

On Mon, 2009-09-14 at 14:45 -0300, Rajiv Andrade wrote:
> On Mon, 2009-09-14 at 00:25 -0600, Jason Gunthorpe wrote:
> > On Mon, Sep 14, 2009 at 12:36:39PM +1000, James Morris wrote:
> > > On Thu, 3 Sep 2009, Jason Gunthorpe wrote:
> > 
> > > > > That sounds like a fairly serious bug, and this looks like a 2.6.31
> > > > > patch.
> > > 
> > > Any comments from the maintainers on this patch?
> > 
> 
> Sorry for the delay, just reviewed the patches and Jason is right,
> Jason's and Andrew's final work (last patch) is great.
> 
> > FWIW, I didn't mention in the patch emails, but all the patches I sent
> > fix regressions that have been introduced in the past couple years by
> > clean up patches that never tested the code paths they alter.
> > 
> 
> Hold on, I tested these functions before when submitting them and they
> worked fine, and, I couldn't figure it out why yet (only guesses), they
> still work for me here, probably due the mishandling of the be32
> formatted value when calling transmit_cmd().

Regardless the testing and what's happening here in my machine, I agree
with you both that these fixes should go to 2.6.31.

Rajiv

Re: [PATCH] TPM: Fixup pcrs sysfs file

[Jason Gunthorpe]

On Mon, Sep 14, 2009 at 02:45:28PM -0300, Rajiv Andrade wrote:

> Sorry for the delay, just reviewed the patches and Jason is right,
> Jason's and Andrew's final work (last patch) is great.

Thnx
 
> > FWIW, I didn't mention in the patch emails, but all the patches I sent
> > fix regressions that have been introduced in the past couple years by
> > clean up patches that never tested the code paths they alter.

> Hold on, I tested these functions before when submitting them and they
> worked fine, and, I couldn't figure it out why yet (only guesses), they
> still work for me here, probably due the mishandling of the be32
> formatted value when calling transmit_cmd().

Ahh, yes, you are right, the pcr thing wouldn't blow up on a LE
system, I'm testing on BE. Didn't notice that subtlety. Sorry

Also please look at the last patch Andrew has queued up:
http://patchwork.kernel.org/patch/45512/

Regards,
Jason