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

[Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>]

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