[tpmdd-devel,6/6] tpm: fix calculation of ordinal duration
diff mbox

Message ID 1446740353-15235-7-git-send-email-martin.wilck@ts.fujitsu.com
State New
Headers show

Commit Message

Martin Wilck Nov. 5, 2015, 4:19 p.m. UTC
From: Martin Wilck <Martin.Wilck@ts.fujitsu.com>

commit 07b133e6060b
("char/tpm: simplify duration calculation and eliminate smatch warning.")
separated out the high bits from the duration calculation for
ordinals but forgot to actually mask them out when looking at
the ordinal index. Fix it.

Signed-off-by: Martin Wilck <Martin.Wilck@ts.fujitsu.com>
---
 drivers/char/tpm/tpm-interface.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Jarkko Sakkinen Nov. 5, 2015, 9:40 p.m. UTC | #1
On Thu, Nov 05, 2015 at 05:19:13PM +0100, martin.wilck@ts.fujitsu.com wrote:
> From: Martin Wilck <Martin.Wilck@ts.fujitsu.com>
> 
> commit 07b133e6060b
> ("char/tpm: simplify duration calculation and eliminate smatch warning.")
> separated out the high bits from the duration calculation for
> ordinals but forgot to actually mask them out when looking at
> the ordinal index. Fix it.
> 
> Signed-off-by: Martin Wilck <Martin.Wilck@ts.fujitsu.com>
> ---
>  drivers/char/tpm/tpm-interface.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index c50637d..e4bceba 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -312,6 +312,7 @@ unsigned long tpm_calc_ordinal_duration(struct tpm_chip *chip,
>  	int duration = 0;
>  	u8 category = (ordinal >> 24) & 0xFF;
>  
> +	ordinal &= 0xFFFF; /* command ordinal index - low 16 bits */

Does this matter since there are range checks after?

>  	if ((category == TPM_PROTECTED_COMMAND && ordinal < TPM_MAX_ORDINAL) ||
>  	    (category == TPM_CONNECTION_COMMAND && ordinal < TSC_MAX_ORDINAL))
>  		duration_idx = tpm_ordinal_duration[ordinal];
> -- 
> 1.8.3.1

/Jarkko

------------------------------------------------------------------------------
Martin Wilck Nov. 6, 2015, 7:10 a.m. UTC | #2
Hi Jarkko,
> > +	ordinal &= 0xFFFF; /* command ordinal index - low 16 bits */
> 
> Does this matter since there are range checks after?

It matters in the expression "(ordinal < TPM_MAX_ORDINAL) below, which
will be false for any value with higher bits set.

Regards
Martin

> 
> >  	if ((category == TPM_PROTECTED_COMMAND && ordinal < TPM_MAX_ORDINAL) ||
> >  	    (category == TPM_CONNECTION_COMMAND && ordinal < TSC_MAX_ORDINAL))
> >  		duration_idx = tpm_ordinal_duration[ordinal];
> > -- 
> > 1.8.3.1
> 
> /Jarkko
------------------------------------------------------------------------------
Jarkko Sakkinen Nov. 6, 2015, 1:57 p.m. UTC | #3
On Fri, Nov 06, 2015 at 08:10:09AM +0100, Wilck, Martin wrote:
> Hi Jarkko,
> > > +	ordinal &= 0xFFFF; /* command ordinal index - low 16 bits */
> > 
> > Does this matter since there are range checks after?
> 
> It matters in the expression "(ordinal < TPM_MAX_ORDINAL) below, which
> will be false for any value with higher bits set.

I don't see how this would matter for protected cmmands.

I think it matters for this:

  category == TPM_CONNECTION_COMMAND && ordinal < TSC_MAX_ORDINAL

This will always evaluate to 0 and therefore for these kinds of commands
the duration is always 2 seconds. Applying your fix would add only more
clutter.

There are two things one could do:

* Simplify to condition simply to
  'category == TPM_PROTECTED_COMMAND && ordinal < TPM_MAX_ORDINAL'
* Create a separate duration table for connection commands.

> Regards
> Martin

/Jarkko

------------------------------------------------------------------------------
Martin Wilck Nov. 9, 2015, 11:25 a.m. UTC | #4
> * Simplify to condition simply to
>   'category == TPM_PROTECTED_COMMAND && ordinal < TPM_MAX_ORDINAL'

Yes, that is reasonable. The expression with TPM_CONNECTION_COMMAND is
simply wrong.

Regards
Martin

------------------------------------------------------------------------------
Presto, an open source distributed SQL query engine for big data, initially
developed by Facebook, enables you to easily query your data on Hadoop in a 
more interactive manner. Teradata is also now providing full enterprise
support for Presto. Download a free open source copy now.
http://pubads.g.doubleclick.net/gampad/clk?id=250295911&iu=/4140
Jarkko Sakkinen Nov. 9, 2015, 2:13 p.m. UTC | #5
On Mon, Nov 09, 2015 at 12:25:53PM +0100, Wilck, Martin wrote:
> 
> > * Simplify to condition simply to
> >   'category == TPM_PROTECTED_COMMAND && ordinal < TPM_MAX_ORDINAL'
> 
> Yes, that is reasonable. The expression with TPM_CONNECTION_COMMAND is
> simply wrong.

I'm happy to apply patch where this gets removed. Thanks.

> Regards
> Martin

/Jarkko

------------------------------------------------------------------------------
Presto, an open source distributed SQL query engine for big data, initially
developed by Facebook, enables you to easily query your data on Hadoop in a 
more interactive manner. Teradata is also now providing full enterprise
support for Presto. Download a free open source copy now.
http://pubads.g.doubleclick.net/gampad/clk?id=250295911&iu=/4140
Martin Wilck Nov. 17, 2015, 9:39 a.m. UTC | #6
On Mo, 2015-11-09 at 16:13 +0200, Jarkko Sakkinen wrote:
> On Mon, Nov 09, 2015 at 12:25:53PM +0100, Wilck, Martin wrote:
> > 
> > > * Simplify to condition simply to
> > >   'category == TPM_PROTECTED_COMMAND && ordinal < TPM_MAX_ORDINAL'
> > 
> > Yes, that is reasonable. The expression with TPM_CONNECTION_COMMAND is
> > simply wrong.
> 
> I'm happy to apply patch where this gets removed. Thanks.

For code readability, it's good to leave the ordinal &= 0xFFFF statement
in place. Not everybody knows from the top of his head that
TPM_PROTECTED_COMMAND == 0.

Regards
Martin

> 
> > Regards
> > Martin
> 
> /Jarkko
------------------------------------------------------------------------------
Martin Wilck Nov. 17, 2015, 12:31 p.m. UTC | #7
On Di, 2015-11-17 at 10:39 +0100, Wilck, Martin wrote:
> On Mo, 2015-11-09 at 16:13 +0200, Jarkko Sakkinen wrote:
> > On Mon, Nov 09, 2015 at 12:25:53PM +0100, Wilck, Martin wrote:
> > > 
> > > > * Simplify to condition simply to
> > > >   'category == TPM_PROTECTED_COMMAND && ordinal < TPM_MAX_ORDINAL'
> > > 
> > > Yes, that is reasonable. The expression with TPM_CONNECTION_COMMAND is
> > > simply wrong.
> > 
> > I'm happy to apply patch where this gets removed. Thanks.
> 
> For code readability, it's good to leave the ordinal &= 0xFFFF statement
> in place. Not everybody knows from the top of his head that
> TPM_PROTECTED_COMMAND == 0.

Forget this. I'll resubmit my patch series soon, with a simplified
approach to this issue.
------------------------------------------------------------------------------
Jarkko Sakkinen Nov. 17, 2015, 12:58 p.m. UTC | #8
On Tue, Nov 17, 2015 at 01:31:12PM +0100, Wilck, Martin wrote:
> On Di, 2015-11-17 at 10:39 +0100, Wilck, Martin wrote:
> > On Mo, 2015-11-09 at 16:13 +0200, Jarkko Sakkinen wrote:
> > > On Mon, Nov 09, 2015 at 12:25:53PM +0100, Wilck, Martin wrote:
> > > > 
> > > > > * Simplify to condition simply to
> > > > >   'category == TPM_PROTECTED_COMMAND && ordinal < TPM_MAX_ORDINAL'
> > > > 
> > > > Yes, that is reasonable. The expression with TPM_CONNECTION_COMMAND is
> > > > simply wrong.
> > > 
> > > I'm happy to apply patch where this gets removed. Thanks.
> > 
> > For code readability, it's good to leave the ordinal &= 0xFFFF statement
> > in place. Not everybody knows from the top of his head that
> > TPM_PROTECTED_COMMAND == 0.
> 
> Forget this. I'll resubmit my patch series soon, with a simplified
> approach to this issue.

Please note that two of your patches went already to -rc1.

/Jarkko

------------------------------------------------------------------------------

Patch
diff mbox

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index c50637d..e4bceba 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -312,6 +312,7 @@  unsigned long tpm_calc_ordinal_duration(struct tpm_chip *chip,
 	int duration = 0;
 	u8 category = (ordinal >> 24) & 0xFF;
 
+	ordinal &= 0xFFFF; /* command ordinal index - low 16 bits */
 	if ((category == TPM_PROTECTED_COMMAND && ordinal < TPM_MAX_ORDINAL) ||
 	    (category == TPM_CONNECTION_COMMAND && ordinal < TSC_MAX_ORDINAL))
 		duration_idx = tpm_ordinal_duration[ordinal];