Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1443844 > unrolled thread

[PATCH 2/2] tpm: fix byte-order for the value read by tpm2_get_tpm_pt

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 2/2] tpm: fix byte-order for the value read by tpm2_get_tpm_pt Andrey Pronin <apronin@chromium.org> - 2016-07-15 03:10 +0200
    Re: [PATCH 2/2] tpm: fix byte-order for the value read by  tpm2_get_tpm_pt Andrey Pronin <apronin@chromium.org> - 2016-07-15 05:20 +0200
      Re: [PATCH 2/2] tpm: fix byte-order for the value read by  tpm2_get_tpm_pt Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2016-07-15 05:40 +0200
        Re: [PATCH 2/2] tpm: fix byte-order for the value read by  tpm2_get_tpm_pt Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2016-07-15 21:40 +0200
          Re: [PATCH 2/2] tpm: fix byte-order for the value read by  tpm2_get_tpm_pt Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2016-07-18 21:10 +0200
        Re: [PATCH 2/2] tpm: fix byte-order for the value read by  tpm2_get_tpm_pt Andrey Pronin <apronin@chromium.org> - 2016-07-15 21:40 +0200
    Re: [PATCH 2/2] tpm: fix byte-order for the value read by  tpm2_get_tpm_pt Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2016-07-15 05:20 +0200
    Re: [PATCH 2/2] tpm: fix byte-order for the value read by  tpm2_get_tpm_pt Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2016-07-18 21:10 +0200
    Re: [PATCH 2/2] tpm: fix byte-order for the value read by  tpm2_get_tpm_pt Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2016-07-18 21:10 +0200
      Re: [PATCH 2/2] tpm: fix byte-order for the value read by  tpm2_get_tpm_pt Andrey Pronin <apronin@chromium.org> - 2016-07-18 21:20 +0200

#1443844 — [PATCH 2/2] tpm: fix byte-order for the value read by tpm2_get_tpm_pt

Change-Id: I7d71cd379b1a3b7659d20a1b6008216762596590
Signed-off-by: Andrey Pronin <apronin@chromium.org>
---
 drivers/char/tpm/tpm2-cmd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index a1673dc..a88b31e 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -703,7 +703,7 @@ ssize_t tpm2_get_tpm_pt(struct tpm_chip *chip, u32 property_id,  u32 *value,
 
 	rc = tpm_transmit_cmd(chip, &cmd, sizeof(cmd), desc);
 	if (!rc)
-		*value = cmd.params.get_tpm_pt_out.value;
+		*value = be32_to_cpu(cmd.params.get_tpm_pt_out.value);
 
 	return rc;
 }
-- 
2.6.6

[toc] | [next] | [standalone]

#1443908 — Re: [PATCH 2/2] tpm: fix byte-order for the value read by tpm2_get_tpm_pt

On Thu, Jul 14, 2016 at 09:10:46PM -0600, Jason Gunthorpe wrote:
> On Thu, Jul 14, 2016 at 06:07:18PM -0700, Andrey Pronin wrote:
> > Change-Id: I7d71cd379b1a3b7659d20a1b6008216762596590
> > Signed-off-by: Andrey Pronin <apronin@chromium.org>
> >  drivers/char/tpm/tpm2-cmd.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
> > index a1673dc..a88b31e 100644
> > +++ b/drivers/char/tpm/tpm2-cmd.c
> > @@ -703,7 +703,7 @@ ssize_t tpm2_get_tpm_pt(struct tpm_chip *chip, u32 property_id,  u32 *value,
> >  
> >  	rc = tpm_transmit_cmd(chip, &cmd, sizeof(cmd), desc);
> >  	if (!rc)
> > -		*value = cmd.params.get_tpm_pt_out.value;
> > +		*value = be32_to_cpu(cmd.params.get_tpm_pt_out.value);
> 
> Huh.
> 
> Jarkko: Are you running sparse on the tpm stuff? The annotations look
> right here, sparse should have complained on this? Andrey, did sparse
> complain here or is there something more serious wrong as well??
> 
> Reviewed-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> 
> Jason

No, this is not sparse complaining. I tried using tpm2_get_tpm_pt() to
read permanent flags and discovered that it was missing byte-ordering
conversion. The only place tpm2_get_tpm_pt() was used before was in
tpm2_gen_interrupt, which discarded the result. So, nobody noticed,
I guess.

Andrey

[toc] | [prev] | [next] | [standalone]

#1443918 — Re: [PATCH 2/2] tpm: fix byte-order for the value read by tpm2_get_tpm_pt

On Thu, Jul 14, 2016 at 08:17:01PM -0700, Andrey Pronin wrote:
> conversion. The only place tpm2_get_tpm_pt() was used before was in
> tpm2_gen_interrupt, which discarded the result. So, nobody noticed,
> I guess.

If you have a moment can you run sparse on this file before/after this
change and make sure it did complain and that there are not other
endian errors?

Jason

[toc] | [prev] | [next] | [standalone]

#1444534 — Re: [PATCH 2/2] tpm: fix byte-order for the value read by tpm2_get_tpm_pt

On Fri, Jul 15, 2016 at 12:31:43PM -0700, Andrey Pronin wrote:

> I even tried changing the code there to
> 	__be32 vv = cmd.params.get_tpm_pt_out.value;
> 	u32 ret = vv;
> to make it clear what's going on. Still, no complaints from sparse.

Hum, I'm not an expert at sparse, but I expect a warning from it here?

Maybe Jarkko knows when he gets back?

Jason

[toc] | [prev] | [next] | [standalone]

#1445736 — Re: [PATCH 2/2] tpm: fix byte-order for the value read by tpm2_get_tpm_pt

On Fri, Jul 15, 2016 at 01:39:28PM -0600, Jason Gunthorpe wrote:
> On Fri, Jul 15, 2016 at 12:31:43PM -0700, Andrey Pronin wrote:
> 
> > I even tried changing the code there to
> > 	__be32 vv = cmd.params.get_tpm_pt_out.value;
> > 	u32 ret = vv;
> > to make it clear what's going on. Still, no complaints from sparse.
> 
> Hum, I'm not an expert at sparse, but I expect a warning from it here?
> 
> Maybe Jarkko knows when he gets back?

I do run sparse regularly. If look at the commit log there are even
couple of recent bug fixes related to sparse errors.

/Jarkko

[toc] | [prev] | [next] | [standalone]

#1444535 — Re: [PATCH 2/2] tpm: fix byte-order for the value read by tpm2_get_tpm_pt

On Thu, Jul 14, 2016 at 09:35:53PM -0600, Jason Gunthorpe wrote:
> On Thu, Jul 14, 2016 at 08:17:01PM -0700, Andrey Pronin wrote:
> > conversion. The only place tpm2_get_tpm_pt() was used before was in
> > tpm2_gen_interrupt, which discarded the result. So, nobody noticed,
> > I guess.
> 
> If you have a moment can you run sparse on this file before/after this
> change and make sure it did complain and that there are not other
> endian errors?
> 

I ran sparse. In my case it didn't complain before or after the change.
It only complains about using cpu_to_be32() in tpm_capabilities and
tpm_sub_capabilities enum in tpm.h and the use of those enums in
tpm_getcap() in tpm-interface.c

I even tried changing the code there to
	__be32 vv = cmd.params.get_tpm_pt_out.value;
	u32 ret = vv;
to make it clear what's going on. Still, no complaints from sparse.

be32_to_cpu() should cleary be done in this case, though. As value
is defined as __be32 in struct tpm2_get_tpm_pt_out, and we return u32
in that parameter from tpm2_get_tpm_pt(). Plus, it's consistent with
the spec. It's just nobody has used this value yet in the existing
code, thus it went unnoticed.

To address "no dead code" comment, I'll drop defining various tpm2
properties and bits from this patchset and move it to the sysfs for
tpm2 patchset where they'll be needed for tpm2-specific attributes.
But this fix is valid on its own in my opinion.

[toc] | [prev] | [next] | [standalone]

#1443910 — Re: [PATCH 2/2] tpm: fix byte-order for the value read by tpm2_get_tpm_pt

On Thu, Jul 14, 2016 at 06:07:18PM -0700, Andrey Pronin wrote:
> Change-Id: I7d71cd379b1a3b7659d20a1b6008216762596590
> Signed-off-by: Andrey Pronin <apronin@chromium.org>
>  drivers/char/tpm/tpm2-cmd.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
> index a1673dc..a88b31e 100644
> +++ b/drivers/char/tpm/tpm2-cmd.c
> @@ -703,7 +703,7 @@ ssize_t tpm2_get_tpm_pt(struct tpm_chip *chip, u32 property_id,  u32 *value,
>  
>  	rc = tpm_transmit_cmd(chip, &cmd, sizeof(cmd), desc);
>  	if (!rc)
> -		*value = cmd.params.get_tpm_pt_out.value;
> +		*value = be32_to_cpu(cmd.params.get_tpm_pt_out.value);

Huh.

Jarkko: Are you running sparse on the tpm stuff? The annotations look
right here, sparse should have complained on this? Andrey, did sparse
complain here or is there something more serious wrong as well??

Reviewed-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>

Jason

[toc] | [prev] | [next] | [standalone]

#1445737 — Re: [PATCH 2/2] tpm: fix byte-order for the value read by tpm2_get_tpm_pt

On Mon, Jul 18, 2016 at 10:04:21PM +0300, Jarkko Sakkinen wrote:
> On Thu, Jul 14, 2016 at 06:07:18PM -0700, Andrey Pronin wrote:
> > Change-Id: I7d71cd379b1a3b7659d20a1b6008216762596590
> > Signed-off-by: Andrey Pronin <apronin@chromium.org>
> 
> Please remove the internal commit ID next time. This is also lacking a
> description but this time I can write it down.
> 
> This not needed for the stable for obvious reasons (only used for
> probing).
> 
> Fixes: 7a1d7e6dd76a ("tpm: TPM 2.0 baseline support")
> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

Applied.

/Jarkko

[toc] | [prev] | [next] | [standalone]

#1445738 — Re: [PATCH 2/2] tpm: fix byte-order for the value read by tpm2_get_tpm_pt

On Thu, Jul 14, 2016 at 06:07:18PM -0700, Andrey Pronin wrote:
> Change-Id: I7d71cd379b1a3b7659d20a1b6008216762596590
> Signed-off-by: Andrey Pronin <apronin@chromium.org>

Please remove the internal commit ID next time. This is also lacking a
description but this time I can write it down.

This not needed for the stable for obvious reasons (only used for
probing).

Fixes: 7a1d7e6dd76a ("tpm: TPM 2.0 baseline support")
Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

/Jarkko

> ---
>  drivers/char/tpm/tpm2-cmd.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
> index a1673dc..a88b31e 100644
> --- a/drivers/char/tpm/tpm2-cmd.c
> +++ b/drivers/char/tpm/tpm2-cmd.c
> @@ -703,7 +703,7 @@ ssize_t tpm2_get_tpm_pt(struct tpm_chip *chip, u32 property_id,  u32 *value,
>  
>  	rc = tpm_transmit_cmd(chip, &cmd, sizeof(cmd), desc);
>  	if (!rc)
> -		*value = cmd.params.get_tpm_pt_out.value;
> +		*value = be32_to_cpu(cmd.params.get_tpm_pt_out.value);
>  
>  	return rc;
>  }
> -- 
> 2.6.6
> 

[toc] | [prev] | [next] | [standalone]

#1445742 — Re: [PATCH 2/2] tpm: fix byte-order for the value read by tpm2_get_tpm_pt

On Mon, Jul 18, 2016 at 10:04:21PM +0300, Jarkko Sakkinen wrote:
> On Thu, Jul 14, 2016 at 06:07:18PM -0700, Andrey Pronin wrote:
> > Change-Id: I7d71cd379b1a3b7659d20a1b6008216762596590
> > Signed-off-by: Andrey Pronin <apronin@chromium.org>
> 
> Please remove the internal commit ID next time. This is also lacking a
> description but this time I can write it down.
>
Yes, sorry for the commit ID, missed it in this patch :(

> This not needed for the stable for obvious reasons (only used for
> probing).
> 

I intend to use tpm2_get_tpm_pt for tpm2 sysfs attributes
implementation (permanent and startup_clear flags). Just
submitted as a separate patch since it fixes a more generic
bug.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web