| Message ID | 1446740353-15235-7-git-send-email-martin.wilck@ts.fujitsu.com |
|---|---|
| State | New |
| Headers | show |
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 ------------------------------------------------------------------------------
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 ------------------------------------------------------------------------------
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 ------------------------------------------------------------------------------
> * 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
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
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 ------------------------------------------------------------------------------
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. ------------------------------------------------------------------------------
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 ------------------------------------------------------------------------------
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];