[PATCH] - TPM save state before suspending to ram

[PATCH] - TPM save state before suspending to ram

[Marcel Selhorst]

Dear list,

this patch fixes a bug, that prevents the TPM chip to resume correctly from a
suspended state.

Signed-off-by: Marcel Selhorst <tpm@selhorst.net>
---
--- linux-tpm/drivers/char/tpm/tpm.c	2008-01-03 20:44:43.000000000 +0100
+++ linux/drivers/char/tpm/tpm.c	2008-01-03 21:08:13.000000000 +0100
@@ -1041,7 +1041,7 @@ void tpm_remove_hardware(struct device *
 }
 EXPORT_SYMBOL_GPL(tpm_remove_hardware);

-static u8 savestate[] = {
+static const u8 savestate[] = {
 	0, 193,			/* TPM_TAG_RQU_COMMAND */
 	0, 0, 0, 10,		/* blob length (in bytes) */
 	0, 0, 0, 152		/* TPM_ORD_SaveState */
@@ -1053,11 +1053,13 @@ static u8 savestate[] = {
  */
 int tpm_pm_suspend(struct device *dev, pm_message_t pm_state)
 {
+	u8 data[max_t(int, max(ARRAY_SIZE(savestate), ARRAY_SIZE(savestate)), 10)];
 	struct tpm_chip *chip = dev_get_drvdata(dev);
 	if (chip == NULL)
 		return -ENODEV;

-	tpm_transmit(chip, savestate, sizeof(savestate));
+	memcpy(data, savestate, sizeof(savestate));
+	tpm_transmit(chip, data, sizeof(data));
 	return 0;
 }
 EXPORT_SYMBOL_GPL(tpm_pm_suspend);

Re: [PATCH] - TPM save state before suspending to ram

[Pavel Machek]

On Thu 2008-01-03 21:44:15, Marcel Selhorst wrote:
> Dear list,
> 
> this patch fixes a bug, that prevents the TPM chip to resume correctly from a
> suspended state.
> 
> Signed-off-by: Marcel Selhorst <tpm@selhorst.net>
> ---
> --- linux-tpm/drivers/char/tpm/tpm.c	2008-01-03 20:44:43.000000000 +0100
> +++ linux/drivers/char/tpm/tpm.c	2008-01-03 21:08:13.000000000 +0100
> @@ -1041,7 +1041,7 @@ void tpm_remove_hardware(struct device *
>  }
>  EXPORT_SYMBOL_GPL(tpm_remove_hardware);
> 
> -static u8 savestate[] = {
> +static const u8 savestate[] = {
>  	0, 193,			/* TPM_TAG_RQU_COMMAND */
>  	0, 0, 0, 10,		/* blob length (in bytes) */
>  	0, 0, 0, 152		/* TPM_ORD_SaveState */
> @@ -1053,11 +1053,13 @@ static u8 savestate[] = {
>   */
>  int tpm_pm_suspend(struct device *dev, pm_message_t pm_state)
>  {
> +	u8 data[max_t(int, max(ARRAY_SIZE(savestate), ARRAY_SIZE(savestate)), 10)];
>  	struct tpm_chip *chip = dev_get_drvdata(dev);
>  	if (chip == NULL)
>  		return -ENODEV;
> 
> -	tpm_transmit(chip, savestate, sizeof(savestate));
> +	memcpy(data, savestate, sizeof(savestate));
> +	tpm_transmit(chip, data, sizeof(data));
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(tpm_pm_suspend);

I'm not sure if we want to use variable-size array on stack. What
hacks are you doing with max_t/max?
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [tpmdd-devel] [PATCH] - TPM save state before suspending to ram

[Kent Yoder]

On Jan 4, 2008 10:39 AM, Pavel Machek <pavel@ucw.cz> wrote:
>
> On Thu 2008-01-03 21:44:15, Marcel Selhorst wrote:
> > Dear list,
> >
> > this patch fixes a bug, that prevents the TPM chip to resume correctly from a
> > suspended state.
> >
> > Signed-off-by: Marcel Selhorst <tpm@selhorst.net>
[cut]
>
> I'm not sure if we want to use variable-size array on stack. What
> hacks are you doing with max_t/max?
>                                                                         Pavel

Hi Pavel,

  Here's an alternate solution from David, signoffs pending --

commit 058f1e3c4d4e2de5a7188608a1c2e0722498fd4b
Author: David Smith <davidsmith@acm.org>
Date:   Fri Jan 4 03:33:11 2008 +0900

    Fix for TPM suspend/resume failure

diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c
index 9bb5429..79d2fd5 100644
--- a/drivers/char/tpm/tpm.c
+++ b/drivers/char/tpm/tpm.c
@@ -1046,12 +1046,6 @@ void tpm_remove_hardware(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(tpm_remove_hardware);

-static u8 savestate[] = {
-	0, 193,			/* TPM_TAG_RQU_COMMAND */
-	0, 0, 0, 10,		/* blob length (in bytes) */
-	0, 0, 0, 152		/* TPM_ORD_SaveState */
-};
-
 /*
  * We are about to suspend. Save the TPM state
  * so that it can be restored.
@@ -1059,8 +1053,14 @@ static u8 savestate[] = {
 int tpm_pm_suspend(struct device *dev, pm_message_t pm_state)
 {
 	struct tpm_chip *chip = dev_get_drvdata(dev);
+        u8 savestate[] = {
+          0, 193,		/* TPM_TAG_RQU_COMMAND */
+          0, 0, 0, 10,		/* blob length (in bytes) */
+          0, 0, 0, 152		/* TPM_ORD_SaveState */
+        };
+
 	if (chip == NULL)
-		return -ENODEV;
+          return -ENODEV;

 	tpm_transmit(chip, savestate, sizeof(savestate));
 	return 0;


Kent

-- 
Kent Yoder
IBM LTC Security Dev.

Re: [tpmdd-devel] [PATCH] - TPM save state before suspending to ram

[Marcel Selhorst]

Kent is right, the solution from David is way better. I wanted to be consistent
with the other functions in tpm.c, but it definitely makes more sense in Davids
way. So please consider my patch as reverted and his patch as

Signed-off-by: Marcel Selhorst <tpm@selhorst.net>

Thanks,
Marcel

Kent Yoder schrieb:
> On Jan 4, 2008 10:39 AM, Pavel Machek <pavel@ucw.cz> wrote:
>> On Thu 2008-01-03 21:44:15, Marcel Selhorst wrote:
>>> Dear list,
>>>
>>> this patch fixes a bug, that prevents the TPM chip to resume correctly from a
>>> suspended state.
>>>
>>> Signed-off-by: Marcel Selhorst <tpm@selhorst.net>
> [cut]
>> I'm not sure if we want to use variable-size array on stack. What
>> hacks are you doing with max_t/max?
>>                                                                         Pavel
> 
> Hi Pavel,
> 
>   Here's an alternate solution from David, signoffs pending --
> 
> commit 058f1e3c4d4e2de5a7188608a1c2e0722498fd4b
> Author: David Smith <davidsmith@acm.org>
> Date:   Fri Jan 4 03:33:11 2008 +0900
> 
>     Fix for TPM suspend/resume failure
> 
> diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c
> index 9bb5429..79d2fd5 100644
> --- a/drivers/char/tpm/tpm.c
> +++ b/drivers/char/tpm/tpm.c
> @@ -1046,12 +1046,6 @@ void tpm_remove_hardware(struct device *dev)
>  }
>  EXPORT_SYMBOL_GPL(tpm_remove_hardware);
> 
> -static u8 savestate[] = {
> -	0, 193,			/* TPM_TAG_RQU_COMMAND */
> -	0, 0, 0, 10,		/* blob length (in bytes) */
> -	0, 0, 0, 152		/* TPM_ORD_SaveState */
> -};
> -
>  /*
>   * We are about to suspend. Save the TPM state
>   * so that it can be restored.
> @@ -1059,8 +1053,14 @@ static u8 savestate[] = {
>  int tpm_pm_suspend(struct device *dev, pm_message_t pm_state)
>  {
>  	struct tpm_chip *chip = dev_get_drvdata(dev);
> +        u8 savestate[] = {
> +          0, 193,		/* TPM_TAG_RQU_COMMAND */
> +          0, 0, 0, 10,		/* blob length (in bytes) */
> +          0, 0, 0, 152		/* TPM_ORD_SaveState */
> +        };
> +
>  	if (chip == NULL)
> -		return -ENODEV;
> +          return -ENODEV;
> 
>  	tpm_transmit(chip, savestate, sizeof(savestate));
>  	return 0;

Re: [tpmdd-devel] [PATCH] - TPM save state before suspending to ram

[Pavel Machek]

On Fri 2008-01-04 14:09:01, Kent Yoder wrote:
> On Jan 4, 2008 10:39 AM, Pavel Machek <pavel@ucw.cz> wrote:
> >
> > On Thu 2008-01-03 21:44:15, Marcel Selhorst wrote:
> > > Dear list,
> > >
> > > this patch fixes a bug, that prevents the TPM chip to resume correctly from a
> > > suspended state.
> > >
> > > Signed-off-by: Marcel Selhorst <tpm@selhorst.net>
> [cut]
> >
> > I'm not sure if we want to use variable-size array on stack. What
> > hacks are you doing with max_t/max?
> >                                                                         Pavel
> 
> Hi Pavel,
> 
>   Here's an alternate solution from David, signoffs pending --
> 
> commit 058f1e3c4d4e2de5a7188608a1c2e0722498fd4b
> Author: David Smith <davidsmith@acm.org>
> Date:   Fri Jan 4 03:33:11 2008 +0900
> 
>     Fix for TPM suspend/resume failure
> 
> diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c
> index 9bb5429..79d2fd5 100644
> --- a/drivers/char/tpm/tpm.c
> +++ b/drivers/char/tpm/tpm.c
> @@ -1046,12 +1046,6 @@ void tpm_remove_hardware(struct device *dev)
>  }
>  EXPORT_SYMBOL_GPL(tpm_remove_hardware);
> 
> -static u8 savestate[] = {
> -	0, 193,			/* TPM_TAG_RQU_COMMAND */
> -	0, 0, 0, 10,		/* blob length (in bytes) */
> -	0, 0, 0, 152		/* TPM_ORD_SaveState */
> -};
> -
>  /*
>   * We are about to suspend. Save the TPM state
>   * so that it can be restored.
> @@ -1059,8 +1053,14 @@ static u8 savestate[] = {
>  int tpm_pm_suspend(struct device *dev, pm_message_t pm_state)
>  {
>  	struct tpm_chip *chip = dev_get_drvdata(dev);
> +        u8 savestate[] = {
> +          0, 193,		/* TPM_TAG_RQU_COMMAND */
> +          0, 0, 0, 10,		/* blob length (in bytes) */
> +          0, 0, 0, 152		/* TPM_ORD_SaveState */
> +        };
> +
>  	if (chip == NULL)
> -		return -ENODEV;
> +          return -ENODEV;
> 
>  	tpm_transmit(chip, savestate, sizeof(savestate));
>  	return 0;

Yep, but please fix the whitespace.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [tpmdd-devel] [PATCH] - TPM save state before suspending to ram

[David Smith]

[-- Attachment #1.1: Type: text/plain, Size: 2167 bytes --]

> On Fri 2008-01-04 14:09:01, Kent Yoder wrote:
> > On Jan 4, 2008 10:39 AM, Pavel Machek <pavel@ucw.cz> wrote:
> > > On Thu 2008-01-03 21:44:15, Marcel Selhorst wrote:
> > > > Dear list,
> > > >
> > > > this patch fixes a bug, that prevents the TPM chip to resume
> > > > correctly from a suspended state.
> > > >
> > > > Signed-off-by: Marcel Selhorst <tpm@selhorst.net>
> >
> > [cut]
> >
> > > I'm not sure if we want to use variable-size array on stack. What
> > > hacks are you doing with max_t/max?
> > >                                                                        
> > > Pavel
> >
> > Hi Pavel,
> >
> >   Here's an alternate solution from David, signoffs pending --
> >
> > commit 058f1e3c4d4e2de5a7188608a1c2e0722498fd4b
> > Author: David Smith <davidsmith@acm.org>
> > Date:   Fri Jan 4 03:33:11 2008 +0900
> >
> >     Fix for TPM suspend/resume failure
> >
> > diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c
> > index 9bb5429..79d2fd5 100644
> > --- a/drivers/char/tpm/tpm.c
> > +++ b/drivers/char/tpm/tpm.c
> > @@ -1046,12 +1046,6 @@ void tpm_remove_hardware(struct device *dev)
> >  }
> >  EXPORT_SYMBOL_GPL(tpm_remove_hardware);
> >
> > -static u8 savestate[] = {
> > -	0, 193,			/* TPM_TAG_RQU_COMMAND */
> > -	0, 0, 0, 10,		/* blob length (in bytes) */
> > -	0, 0, 0, 152		/* TPM_ORD_SaveState */
> > -};
> > -
> >  /*
> >   * We are about to suspend. Save the TPM state
> >   * so that it can be restored.
> > @@ -1059,8 +1053,14 @@ static u8 savestate[] = {
> >  int tpm_pm_suspend(struct device *dev, pm_message_t pm_state)
> >  {
> >  	struct tpm_chip *chip = dev_get_drvdata(dev);
> > +        u8 savestate[] = {
> > +          0, 193,		/* TPM_TAG_RQU_COMMAND */
> > +          0, 0, 0, 10,		/* blob length (in bytes) */
> > +          0, 0, 0, 152		/* TPM_ORD_SaveState */
> > +        };
> > +
> >  	if (chip == NULL)
> > -		return -ENODEV;
> > +          return -ENODEV;
> >
> >  	tpm_transmit(chip, savestate, sizeof(savestate));
> >  	return 0;
>
> Yep, but please fix the whitespace.
> 									Pavel

OK, attached.
-- 
man perl | tail -6 | head -2

[-- Attachment #1.2: tpm-suspend.patch --]
[-- Type: text/x-diff, Size: 1320 bytes --]

commit a17f28b93a6e11097ea806d576eb317263aa8768
Author: David Smith <dds@toriatama.tok.corp.google.com>
Date:   Fri Jan 4 16:12:58 2008 +0900

    Fix TPM suspend and resume failure.
    
    The savestate command structure was being overwritten by the
    result of running the TPM_SaveState command after one run, so
    make it a local variable to the function instead of a global
    variable that gets overwritten.

diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c
index 9bb5429..4f18ddf 100644
--- a/drivers/char/tpm/tpm.c
+++ b/drivers/char/tpm/tpm.c
@@ -1046,12 +1046,6 @@ void tpm_remove_hardware(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(tpm_remove_hardware);
 
-static u8 savestate[] = {
-	0, 193,			/* TPM_TAG_RQU_COMMAND */
-	0, 0, 0, 10,		/* blob length (in bytes) */
-	0, 0, 0, 152		/* TPM_ORD_SaveState */
-};
-
 /*
  * We are about to suspend. Save the TPM state
  * so that it can be restored.
@@ -1059,6 +1053,12 @@ static u8 savestate[] = {
 int tpm_pm_suspend(struct device *dev, pm_message_t pm_state)
 {
 	struct tpm_chip *chip = dev_get_drvdata(dev);
+	u8 savestate[] = {
+		0, 193,		/* TPM_TAG_RQU_COMMAND */
+		0, 0, 0, 10,	/* blob length (in bytes) */
+		0, 0, 0, 152	/* TPM_ORD_SaveState */
+	};
+
 	if (chip == NULL)
 		return -ENODEV;
 

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 481 bytes --]

Re: [tpmdd-devel] [PATCH] - TPM save state before suspending to ram

[Rafael J. Wysocki]

On Saturday, 5 of January 2008, David Smith wrote:
> > On Fri 2008-01-04 14:09:01, Kent Yoder wrote:
> > > On Jan 4, 2008 10:39 AM, Pavel Machek <pavel@ucw.cz> wrote:
> > > > On Thu 2008-01-03 21:44:15, Marcel Selhorst wrote:
> > > > > Dear list,
> > > > >
> > > > > this patch fixes a bug, that prevents the TPM chip to resume
> > > > > correctly from a suspended state.
> > > > >
> > > > > Signed-off-by: Marcel Selhorst <tpm@selhorst.net>
> > >
> > > [cut]
> > >
> > > > I'm not sure if we want to use variable-size array on stack. What
> > > > hacks are you doing with max_t/max?
> > > >                                                                        
> > > > Pavel
> > >
> > > Hi Pavel,
> > >
> > >   Here's an alternate solution from David, signoffs pending --
> > >
> > > commit 058f1e3c4d4e2de5a7188608a1c2e0722498fd4b
> > > Author: David Smith <davidsmith@acm.org>
> > > Date:   Fri Jan 4 03:33:11 2008 +0900
> > >
> > >     Fix for TPM suspend/resume failure
> > >
> > > diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c
> > > index 9bb5429..79d2fd5 100644
> > > --- a/drivers/char/tpm/tpm.c
> > > +++ b/drivers/char/tpm/tpm.c
> > > @@ -1046,12 +1046,6 @@ void tpm_remove_hardware(struct device *dev)
> > >  }
> > >  EXPORT_SYMBOL_GPL(tpm_remove_hardware);
> > >
> > > -static u8 savestate[] = {
> > > -	0, 193,			/* TPM_TAG_RQU_COMMAND */
> > > -	0, 0, 0, 10,		/* blob length (in bytes) */
> > > -	0, 0, 0, 152		/* TPM_ORD_SaveState */
> > > -};
> > > -
> > >  /*
> > >   * We are about to suspend. Save the TPM state
> > >   * so that it can be restored.
> > > @@ -1059,8 +1053,14 @@ static u8 savestate[] = {
> > >  int tpm_pm_suspend(struct device *dev, pm_message_t pm_state)
> > >  {
> > >  	struct tpm_chip *chip = dev_get_drvdata(dev);
> > > +        u8 savestate[] = {
> > > +          0, 193,		/* TPM_TAG_RQU_COMMAND */
> > > +          0, 0, 0, 10,		/* blob length (in bytes) */
> > > +          0, 0, 0, 152		/* TPM_ORD_SaveState */
> > > +        };
> > > +
> > >  	if (chip == NULL)
> > > -		return -ENODEV;
> > > +          return -ENODEV;
> > >
> > >  	tpm_transmit(chip, savestate, sizeof(savestate));
> > >  	return 0;
> >
> > Yep, but please fix the whitespace.
> > 									Pavel
> 
> OK, attached.

Is anyone taking care of this patch or should I do that?

Rafael




-- 
"Premature optimization is the root of all evil." - Donald Knuth

Re: [tpmdd-devel] [PATCH] - TPM save state before suspending to ram

[Andrew Morton]

On Mon, 14 Jan 2008 22:40:58 +0100
"Rafael J. Wysocki" <rjw@sisk.pl> wrote:

> > >
> > > Yep, but please fix the whitespace.
> > > 									Pavel
> > 
> > OK, attached.
> 
> Is anyone taking care of this patch or should I do that?

Linus just merged it as 2490c681ea3d7f5ac3fb876f14567bf1a9e0aa87.  That's
if we're talking about the same thing (there were a few iterations). 

Re: [tpmdd-devel] [PATCH] - TPM save state before suspending to ram

[Rafael J. Wysocki]

On Monday, 14 of January 2008, Andrew Morton wrote:
> On Mon, 14 Jan 2008 22:40:58 +0100
> "Rafael J. Wysocki" <rjw@sisk.pl> wrote:
> 
> > > >
> > > > Yep, but please fix the whitespace.
> > > > 									Pavel
> > > 
> > > OK, attached.
> > 
> > Is anyone taking care of this patch or should I do that?
> 
> Linus just merged it as 2490c681ea3d7f5ac3fb876f14567bf1a9e0aa87.  That's
> if we're talking about the same thing (there were a few iterations). 

Yes, that's the same thing, thanks.