[tpmdd-devel,RFC,v2,3/5] tpm: infrastructure for TPM spaces

On Thu, 2017-01-12 at 19:46 +0200, Jarkko Sakkinen wrote:
> +static int tpm2_map_response(struct tpm_chip *chip, u32 cc, u8 *rsp,
> size_t len)
> +{
> +       struct tpm_space *space = &chip->work_space;
> +       u32 phandle;
> +       u32 vhandle;
> +       u32 attrs;
> +       int i;
> +       int rc;
> +
> +       if (!tpm2_find_cc_attrs(chip, cc, &attrs)) {
> +               /* should never happen */
> +               dev_err(&chip->dev, "TPM returned a different CC:
> 0x%04x\n",
> +                       cc);
> +               rc = -EFAULT;
> +               goto out_err;
> +       }
> +
> +       if (!((attrs >> TPM2_CC_ATTR_RHANDLE) & 1))
> +               return 0;
> +
> +       phandle = be32_to_cpup((__be32 *)&rsp[TPM_HEADER_SIZE]);

I think we have to check the command return code here.  We can't
blindly fish handles out of the response if the TPM returned an error
because they won't exist and we'll pull rubbish from the buffer. 
 Incremental patch below.

Note I think we should use get_unaligned_be32 because we're pulling a
32 bit word from something that's on byte 6 (so misaligned): some
architectures will trigger an unaligned trap for this (it's not a
problem: they trap handle it, it just slows down processing a lot).

James

---

commit d17ad905ff7b114f7efd23f930e9a541ccdf7621
Author: James Bottomley <James.Bottomley@HansenPartnership.com>
Date:   Wed Jan 11 22:01:29 2017 -0800

    check return code



------------------------------------------------------------------------------
Developer Access Program for Intel Xeon Phi Processors
Access to Intel Xeon Phi processor-based developer platforms.
With one year of Intel Parallel Studio XE.
Training and support from Colfax.
Order your platform today. http://sdm.link/xeonphi

On Thu, Jan 12, 2017 at 12:38:52PM -0800, James Bottomley wrote:
> On Thu, 2017-01-12 at 19:46 +0200, Jarkko Sakkinen wrote:
> > +static int tpm2_map_response(struct tpm_chip *chip, u32 cc, u8 *rsp,
> > size_t len)
> > +{
> > +       struct tpm_space *space = &chip->work_space;
> > +       u32 phandle;
> > +       u32 vhandle;
> > +       u32 attrs;
> > +       int i;
> > +       int rc;
> > +
> > +       if (!tpm2_find_cc_attrs(chip, cc, &attrs)) {
> > +               /* should never happen */
> > +               dev_err(&chip->dev, "TPM returned a different CC:
> > 0x%04x\n",
> > +                       cc);
> > +               rc = -EFAULT;
> > +               goto out_err;
> > +       }
> > +
> > +       if (!((attrs >> TPM2_CC_ATTR_RHANDLE) & 1))
> > +               return 0;
> > +
> > +       phandle = be32_to_cpup((__be32 *)&rsp[TPM_HEADER_SIZE]);
> 
> I think we have to check the command return code here.  We can't
> blindly fish handles out of the response if the TPM returned an error
> because they won't exist and we'll pull rubbish from the buffer. 
>  Incremental patch below.
> 
> Note I think we should use get_unaligned_be32 because we're pulling a
> 32 bit word from something that's on byte 6 (so misaligned): some
> architectures will trigger an unaligned trap for this (it's not a
> problem: they trap handle it, it just slows down processing a lot).
> 
> James
> 
> ---
> 
> commit d17ad905ff7b114f7efd23f930e9a541ccdf7621
> Author: James Bottomley <James.Bottomley@HansenPartnership.com>
> Date:   Wed Jan 11 22:01:29 2017 -0800
> 
>     check return code
> 
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index 61422e6..8009ed4 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -90,6 +90,7 @@ enum tpm2_structures {
>  };
>  
>  enum tpm2_return_codes {
> +	TPM2_RC_SUCCESS		= 0x0000,
>  	TPM2_RC_HASH		= 0x0083, /* RC_FMT1 */
>  	TPM2_RC_HANDLE		= 0x008B,
>  	TPM2_RC_INITIALIZE	= 0x0100, /* RC_VER1 */
> diff --git a/drivers/char/tpm/tpm2-space.c b/drivers/char/tpm/tpm2-space.c
> index ca55feb..44e5501 100644
> --- a/drivers/char/tpm/tpm2-space.c
> +++ b/drivers/char/tpm/tpm2-space.c
> @@ -16,6 +16,7 @@
>   */
>  
>  #include <linux/gfp.h>
> +#include <asm/unaligned.h>
>  #include "tpm.h"
>  
>  enum tpm2_handle_types {
> @@ -167,9 +168,13 @@ static int tpm2_map_response(struct tpm_chip *chip, u32 cc, u8 *rsp, size_t len)
>  	u32 phandle;
>  	u32 vhandle;
>  	u32 attrs;
> +	u32 return_code = get_unaligned_be32((__be32 *)&rsp[6]);
>  	int i;
>  	int rc;
>  
> +	if (return_code != TPM2_RC_SUCCESS)
> +		return 0;
> +
>  	if (!tpm2_find_cc_attrs(chip, cc, &attrs)) {
>  		/* should never happen */
>  		dev_err(&chip->dev, "TPM returned a different CC: 0x%04x\n",
> 

[x]

I'll squash this to the next patch set version.

/Jarkko

------------------------------------------------------------------------------
Developer Access Program for Intel Xeon Phi Processors
Access to Intel Xeon Phi processor-based developer platforms.
With one year of Intel Parallel Studio XE.
Training and support from Colfax.
Order your platform today. http://sdm.link/xeonphi

On Sat, Jan 14, 2017 at 12:53:15PM -0500, Ken Goldman wrote:
> On 1/13/2017 11:28 AM, Jarkko Sakkinen wrote:
> 
> > > > +
> > > > +       if (!tpm2_find_cc_attrs(chip, cc, &attrs)) {
> > > > +               /* should never happen */
> > > > +               dev_err(&chip->dev, "TPM returned a different CC:
> > > > 0x%04x\n",
> > > > +                       cc);
> > > > +               rc = -EFAULT;
> > > > +               goto out_err;
> > > > +       }
> 
> Something is strange here.  Is "CC" command code?
> 
> The TPM does not return a command code.  The mapping should use the
> command code from the command.
> 
> It could be the code is correct - the command code mapped OK in the command
> but then failed in the response.  But the error message is strange.

Thanks. I'll update that message.

/Jarkko

------------------------------------------------------------------------------
Developer Access Program for Intel Xeon Phi Processors
Access to Intel Xeon Phi processor-based developer platforms.
With one year of Intel Parallel Studio XE.
Training and support from Colfax.
Order your platform today. http://sdm.link/xeonphi