[PATCH] tpm: use tpm_pcr_read_dev() in tpm_do_selftest()

[PATCH] tpm: use tpm_pcr_read_dev() in tpm_do_selftest()

[Jarkko Sakkinen]

It does not make sense to construct the PCR read command in
tpm_do_selftest() when there is already a function that does
the job.

Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 drivers/char/tpm/tpm-interface.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 6af1700..ee0711b 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -759,7 +759,7 @@ int tpm_do_selftest(struct tpm_chip *chip)
 	unsigned int loops;
 	unsigned int delay_msec = 100;
 	unsigned long duration;
-	struct tpm_cmd_t cmd;
+	u8 digest[TPM_DIGEST_SIZE];
 
 	duration = tpm_calc_ordinal_duration(chip, TPM_ORD_CONTINUE_SELFTEST);
 
@@ -773,10 +773,7 @@ int tpm_do_selftest(struct tpm_chip *chip)
 		return rc;
 
 	do {
-		/* Attempt to read a PCR value */
-		cmd.header.in = pcrread_header;
-		cmd.params.pcrread_in.pcr_idx = cpu_to_be32(0);
-		rc = tpm_transmit(chip, (u8 *) &cmd, READ_PCR_RESULT_SIZE);
+		rc = tpm_pcr_read_dev(chip, 0, digest);
 		/* Some buggy TPMs will not respond to tpm_tis_ready() for
 		 * around 300ms while the self test is ongoing, keep trying
 		 * until the self test duration expires. */
@@ -786,10 +783,9 @@ int tpm_do_selftest(struct tpm_chip *chip)
 			continue;
 		}
 
-		if (rc < TPM_HEADER_SIZE)
+		if (rc < 0)
 			return -EFAULT;
 
-		rc = be32_to_cpu(cmd.header.out.return_code);
 		if (rc == TPM_ERR_DISABLED || rc == TPM_ERR_DEACTIVATED) {
 			dev_info(chip->dev,
 				 "TPM is disabled/deactivated (0x%X)\n", rc);
-- 
2.1.0

Re: [tpmdd-devel] [PATCH] tpm: use tpm_pcr_read_dev() in tpm_do_selftest()

[Jason Gunthorpe]

On Fri, Sep 12, 2014 at 04:06:41PM +0300, Jarkko Sakkinen wrote:
> It does not make sense to construct the PCR read command in
> tpm_do_selftest() when there is already a function that does
> the job.

This would seem to undo an older patch, I don't think things have
changed enough for that to make sense.. Can you comment?

It looks to me like the aim was to not print a warning on faliure
while looping, tpm_pcr_read_dev will print an error.

Though, it would be better to use transmit_cmd with a null desc than
tpm_transmit.

Also:

-               if (rc < TPM_HEADER_SIZE)
+               if (rc < 0)
                        return -EFAULT;

Should be return rc;

commit 24ebe6670de3d1f0dca11c9eb372134c7ab05503
Author: Rajiv Andrade <srajiv@linux.vnet.ibm.com>
Date:   Tue Apr 24 17:38:17 2012 -0300

    TPM: chip disabled state erronously being reported as error
    
    tpm_do_selftest() attempts to read a PCR in order to
    decide if one can rely on the TPM being used or not.
    The function that's used by __tpm_pcr_read() does not
    expect the TPM to be disabled or deactivated, and if so,
    reports an error.
    
    It's fine if the TPM returns this error when trying to
    use it for the first time after a power cycle, but it's
    definitely not if it already returned success for a
    previous attempt to read one of its PCRs.
    
    The tpm_do_selftest() was modified so that the driver only
    reports this return code as an error when it really is.
    
    Reported-and-tested-by: Paul Bolle <pebolle@tiscali.nl>
    Cc: Stable <stable@vger.kernel.org>
    Signed-off-by: Rajiv Andrade <srajiv@linux.vnet.ibm.com>

diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c
index ad7c7320dd1b..08427abf5fa5 100644
--- a/drivers/char/tpm/tpm.c
+++ b/drivers/char/tpm/tpm.c
@@ -827,10 +827,10 @@ EXPORT_SYMBOL_GPL(tpm_pcr_extend);
 int tpm_do_selftest(struct tpm_chip *chip)
 {
        int rc;
-       u8 digest[TPM_DIGEST_SIZE];
        unsigned int loops;
        unsigned int delay_msec = 1000;
        unsigned long duration;
+       struct tpm_cmd_t cmd;
 
        duration = tpm_calc_ordinal_duration(chip,
                                             TPM_ORD_CONTINUE_SELFTEST);
@@ -845,7 +845,15 @@ int tpm_do_selftest(struct tpm_chip *chip)
                return rc;
 
        do {
-               rc = __tpm_pcr_read(chip, 0, digest);
+               /* Attempt to read a PCR value */
+               cmd.header.in = pcrread_header;
+               cmd.params.pcrread_in.pcr_idx = cpu_to_be32(0);
+               rc = tpm_transmit(chip, (u8 *) &cmd, READ_PCR_RESULT_SIZE);
+
+               if (rc < TPM_HEADER_SIZE)
+                       return -EFAULT;
+
+               rc = be32_to_cpu(cmd.header.out.return_code);
                if (rc == TPM_ERR_DISABLED || rc == TPM_ERR_DEACTIVATED) {
                        dev_info(chip->dev,
                                 "TPM is disabled/deactivated (0x%X)\n", rc);

Re: [tpmdd-devel] [PATCH] tpm: use tpm_pcr_read_dev() in tpm_do_selftest()

[Jarkko Sakkinen]

On Fri, Sep 12, 2014 at 09:59:05AM -0600, Jason Gunthorpe wrote:
> On Fri, Sep 12, 2014 at 04:06:41PM +0300, Jarkko Sakkinen wrote:
> > It does not make sense to construct the PCR read command in
> > tpm_do_selftest() when there is already a function that does
> > the job.
> 
> This would seem to undo an older patch, I don't think things have
> changed enough for that to make sense.. Can you comment?

Right. Sorry, I should have used git blame before jumping into this.

> It looks to me like the aim was to not print a warning on faliure
> while looping, tpm_pcr_read_dev will print an error.
> 
> Though, it would be better to use transmit_cmd with a null desc than
> tpm_transmit.
> 
> Also:
> 
> -               if (rc < TPM_HEADER_SIZE)
> +               if (rc < 0)
>                         return -EFAULT;
> 
> Should be return rc;
> 
> commit 24ebe6670de3d1f0dca11c9eb372134c7ab05503
> Author: Rajiv Andrade <srajiv@linux.vnet.ibm.com>
> Date:   Tue Apr 24 17:38:17 2012 -0300
> 
>     TPM: chip disabled state erronously being reported as error
>     
>     tpm_do_selftest() attempts to read a PCR in order to
>     decide if one can rely on the TPM being used or not.
>     The function that's used by __tpm_pcr_read() does not
>     expect the TPM to be disabled or deactivated, and if so,
>     reports an error.
>     
>     It's fine if the TPM returns this error when trying to
>     use it for the first time after a power cycle, but it's
>     definitely not if it already returned success for a
>     previous attempt to read one of its PCRs.
>     
>     The tpm_do_selftest() was modified so that the driver only
>     reports this return code as an error when it really is.
>     
>     Reported-and-tested-by: Paul Bolle <pebolle@tiscali.nl>
>     Cc: Stable <stable@vger.kernel.org>
>     Signed-off-by: Rajiv Andrade <srajiv@linux.vnet.ibm.com>
> 
> diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c
> index ad7c7320dd1b..08427abf5fa5 100644
> --- a/drivers/char/tpm/tpm.c
> +++ b/drivers/char/tpm/tpm.c
> @@ -827,10 +827,10 @@ EXPORT_SYMBOL_GPL(tpm_pcr_extend);
>  int tpm_do_selftest(struct tpm_chip *chip)
>  {
>         int rc;
> -       u8 digest[TPM_DIGEST_SIZE];
>         unsigned int loops;
>         unsigned int delay_msec = 1000;
>         unsigned long duration;
> +       struct tpm_cmd_t cmd;
>  
>         duration = tpm_calc_ordinal_duration(chip,
>                                              TPM_ORD_CONTINUE_SELFTEST);
> @@ -845,7 +845,15 @@ int tpm_do_selftest(struct tpm_chip *chip)
>                 return rc;
>  
>         do {
> -               rc = __tpm_pcr_read(chip, 0, digest);
> +               /* Attempt to read a PCR value */
> +               cmd.header.in = pcrread_header;
> +               cmd.params.pcrread_in.pcr_idx = cpu_to_be32(0);
> +               rc = tpm_transmit(chip, (u8 *) &cmd, READ_PCR_RESULT_SIZE);
> +
> +               if (rc < TPM_HEADER_SIZE)
> +                       return -EFAULT;
> +
> +               rc = be32_to_cpu(cmd.header.out.return_code);
>                 if (rc == TPM_ERR_DISABLED || rc == TPM_ERR_DEACTIVATED) {
>                         dev_info(chip->dev,
>                                  "TPM is disabled/deactivated (0x%X)\n", rc);
> 

/Jarkko