Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1561876 > unrolled thread

Re: [tpmdd-devel] [PATCH RFC v3 5/5] tpm2: expose resource manager via a device link /dev/tpms<n>

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [tpmdd-devel] [PATCH RFC v3 5/5] tpm2: expose resource manager  via    a device link /dev/tpms<n> James Bottomley <James.Bottomley@HansenPartnership.com> - 2017-01-18 16:10 +0100
    Re: [tpmdd-devel] [PATCH RFC v3 5/5] tpm2: expose resource manager  via    a device link /dev/tpms<n> Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2017-01-19 12:00 +0100
      Re: [tpmdd-devel] [PATCH RFC v3 5/5] tpm2: expose resource manager  via    a device link /dev/tpms<n> James Bottomley <James.Bottomley@HansenPartnership.com> - 2017-01-19 13:30 +0100
        Re: [tpmdd-devel] [PATCH RFC v3 5/5] tpm2: expose resource manager  via    a device link /dev/tpms<n> Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2017-01-20 14:40 +0100
          Re: [tpmdd-devel] [PATCH RFC v3 5/5] tpm2: expose resource manager  via    a device link /dev/tpms<n> Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2017-01-20 22:10 +0100
            Re: [tpmdd-devel] [PATCH RFC v3 5/5] tpm2: expose resource manager  via    a device link /dev/tpms<n> James Bottomley <James.Bottomley@HansenPartnership.com> - 2017-01-21 20:30 +0100
              Re: [tpmdd-devel] [PATCH RFC v3 5/5] tpm2: expose resource manager  via    a device link /dev/tpms<n> Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2017-01-22 16:00 +0100
            Re: [tpmdd-devel] [PATCH RFC v3 5/5] tpm2: expose resource manager  via    a device link /dev/tpms<n> James Bottomley <James.Bottomley@HansenPartnership.com> - 2017-01-21 21:50 +0100
              Re: [tpmdd-devel] [PATCH RFC v3 5/5] tpm2: expose resource manager  via    a device link /dev/tpms<n> Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2017-01-22 16:00 +0100
            Re: [tpmdd-devel] [PATCH RFC v3 5/5] tpm2: expose resource manager  via    a device link /dev/tpms<n> James Bottomley <James.Bottomley@HansenPartnership.com> - 2017-01-22 18:50 +0100
              Re: [tpmdd-devel] [PATCH RFC v3 5/5] tpm2: expose resource manager  via    a device link /dev/tpms<n> James Bottomley <James.Bottomley@HansenPartnership.com> - 2017-01-22 19:50 +0100
                Re: [tpmdd-devel] [PATCH RFC v3 5/5] tpm2: expose resource manager  via    a device link /dev/tpms<n> Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2017-01-22 21:40 +0100
                  Re: [tpmdd-devel] [PATCH RFC v3 5/5] tpm2: expose resource manager  via    a device link /dev/tpms<n> Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2017-01-22 22:10 +0100
                    Re: [tpmdd-devel] [PATCH RFC v3 5/5] tpm2: expose resource manager  via    a device link /dev/tpms<n> James Bottomley <James.Bottomley@HansenPartnership.com> - 2017-01-22 22:40 +0100
                      Re: [tpmdd-devel] [PATCH RFC v3 5/5] tpm2: expose resource manager  via    a device link /dev/tpms<n> Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2017-01-23 15:10 +0100
                        Re: [tpmdd-devel] [PATCH RFC v3 5/5] tpm2: expose resource manager  via    a device link /dev/tpms<n> James Bottomley <James.Bottomley@HansenPartnership.com> - 2017-01-23 17:20 +0100
                          Re: [tpmdd-devel] [PATCH RFC v3 5/5] tpm2: expose resource manager  via    a device link /dev/tpms<n> Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2017-01-24 13:10 +0100
                        Re: [tpmdd-devel] [PATCH RFC v3 5/5] tpm2: expose resource manager  via    a device link /dev/tpms<n> Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2017-01-23 18:00 +0100
                          Re: [tpmdd-devel] [PATCH RFC v3 5/5] tpm2: expose resource manager  via    a device link /dev/tpms<n> Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2017-01-23 22:50 +0100
                            Re: [tpmdd-devel] [PATCH RFC v3 5/5] tpm2: expose resource manager  via    a device link /dev/tpms<n> James Bottomley <James.Bottomley@HansenPartnership.com> - 2017-01-23 23:20 +0100
                              Re: [tpmdd-devel] [PATCH RFC v3 5/5] tpm2: expose resource manager  via    a device link /dev/tpms<n> Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2017-01-25 14:50 +0100
                                Re: [tpmdd-devel] [PATCH RFC v3 5/5] tpm2: expose resource manager  via    a device link /dev/tpms<n> Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2017-01-25 14:50 +0100
                                Re: [tpmdd-devel] [PATCH RFC v3 5/5] tpm2: expose resource manager  via a device link /dev/tpms<n> James Bottomley <James.Bottomley@HansenPartnership.com> - 2017-01-27 03:00 +0100
                                  Re: [tpmdd-devel] [PATCH RFC v3 5/5] tpm2: expose resource manager  via a device link /dev/tpms<n> Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2017-01-27 07:50 +0100
                              Re: [tpmdd-devel] [PATCH RFC v3 5/5] tpm2: expose resource manager  via    a device link /dev/tpms<n> Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2017-01-25 21:30 +0100
                  Re: [tpmdd-devel] [PATCH RFC v3 5/5] tpm2: expose resource manager  via    a device link /dev/tpms<n> Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2017-01-22 22:10 +0100
              Re: [tpmdd-devel] [PATCH RFC v3 5/5] tpm2: expose resource manager  via    a device link /dev/tpms<n> Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2017-01-22 21:30 +0100

Page 1 of 2  [1] 2  Next page →

#1561876 — Re: [tpmdd-devel] [PATCH RFC v3 5/5] tpm2: expose resource manager via a device link /dev/tpms<n>

On Mon, 2017-01-16 at 15:12 +0200, Jarkko Sakkinen wrote:
> From: James Bottomley <James.Bottomley@HansenPartnership.com>
> 
> Currently the Resource Manager (RM) is not exposed to userspace. 
>  Make
> this exposure via a separate device, which can now be opened multiple
> times because each read/write transaction goes separately via the RM.
> 
> Concurrency is protected by the chip->tpm_mutex for each read/write
> transaction separately.  The TPM is cleared of all transient objects
> by the time the mutex is dropped, so there should be no interference
> between the kernel and userspace.

There's actually a missing kfree of context_buf on the tpms_release
path as well.  This patch fixes it up.

James

---

commit 778425973c532a0c1ec2b5b2ccd7ff995e2cc9db
Author: James Bottomley <James.Bottomley@HansenPartnership.com>
Date:   Wed Jan 18 09:58:23 2017 -0500

    add missing kfree to tpms_release

diff --git a/drivers/char/tpm/tpms-dev.c b/drivers/char/tpm/tpms-dev.c
index c10b308..6bb687f 100644
--- a/drivers/char/tpm/tpms-dev.c
+++ b/drivers/char/tpm/tpms-dev.c
@@ -37,6 +37,7 @@ static int tpms_release(struct inode *inode, struct file *file)
 	struct tpms_priv *priv = container_of(fpriv, struct tpms_priv, priv);
 
 	tpm_common_release(file, fpriv);
+	kfree(priv->space.context_buf);
 	kfree(priv);
 
 	return 0;

[toc] | [next] | [standalone]

#1562609

On Wed, Jan 18, 2017 at 10:01:03AM -0500, James Bottomley wrote:
> On Mon, 2017-01-16 at 15:12 +0200, Jarkko Sakkinen wrote:
> > From: James Bottomley <James.Bottomley@HansenPartnership.com>
> > 
> > Currently the Resource Manager (RM) is not exposed to userspace. 
> >  Make
> > this exposure via a separate device, which can now be opened multiple
> > times because each read/write transaction goes separately via the RM.
> > 
> > Concurrency is protected by the chip->tpm_mutex for each read/write
> > transaction separately.  The TPM is cleared of all transient objects
> > by the time the mutex is dropped, so there should be no interference
> > between the kernel and userspace.
> 
> There's actually a missing kfree of context_buf on the tpms_release
> path as well.  This patch fixes it up.

Can you send me a fresh version of the whole patch so that I can include
to v4 that includes also changes that I requested in my recent comments
+ all the fixes?

/Jarkko

> 
> James
> 
> ---
> 
> commit 778425973c532a0c1ec2b5b2ccd7ff995e2cc9db
> Author: James Bottomley <James.Bottomley@HansenPartnership.com>
> Date:   Wed Jan 18 09:58:23 2017 -0500
> 
>     add missing kfree to tpms_release
> 
> diff --git a/drivers/char/tpm/tpms-dev.c b/drivers/char/tpm/tpms-dev.c
> index c10b308..6bb687f 100644
> --- a/drivers/char/tpm/tpms-dev.c
> +++ b/drivers/char/tpm/tpms-dev.c
> @@ -37,6 +37,7 @@ static int tpms_release(struct inode *inode, struct file *file)
>  	struct tpms_priv *priv = container_of(fpriv, struct tpms_priv, priv);
>  
>  	tpm_common_release(file, fpriv);
> +	kfree(priv->space.context_buf);
>  	kfree(priv);
>  
>  	return 0;
> 

[toc] | [prev] | [next] | [standalone]

#1562674

[Multipart message — attachments visible in raw view] — view raw

On Thu, 2017-01-19 at 12:49 +0200, Jarkko Sakkinen wrote:
> On Wed, Jan 18, 2017 at 10:01:03AM -0500, James Bottomley wrote:
> > On Mon, 2017-01-16 at 15:12 +0200, Jarkko Sakkinen wrote:
> > > From: James Bottomley <James.Bottomley@HansenPartnership.com>
> > > 
> > > Currently the Resource Manager (RM) is not exposed to userspace. 
> > >  Make this exposure via a separate device, which can now be 
> > > opened multiple times because each read/write transaction goes 
> > > separately via the RM.
> > > 
> > > Concurrency is protected by the chip->tpm_mutex for each 
> > > read/write transaction separately.  The TPM is cleared of all 
> > > transient objects by the time the mutex is dropped, so there 
> > > should be no interference between the kernel and userspace.
> > 
> > There's actually a missing kfree of context_buf on the tpms_release
> > path as well.  This patch fixes it up.
> 
> Can you send me a fresh version of the whole patch so that I can 
> include to v4 that includes also changes that I requested in my 
> recent comments + all the fixes?

Sure, I think the attached is basically it

James

[toc] | [prev] | [next] | [standalone]

#1563581

On Thu, Jan 19, 2017 at 07:19:40AM -0500, James Bottomley wrote:
> On Thu, 2017-01-19 at 12:49 +0200, Jarkko Sakkinen wrote:
> > On Wed, Jan 18, 2017 at 10:01:03AM -0500, James Bottomley wrote:
> > > On Mon, 2017-01-16 at 15:12 +0200, Jarkko Sakkinen wrote:
> > > > From: James Bottomley <James.Bottomley@HansenPartnership.com>
> > > > 
> > > > Currently the Resource Manager (RM) is not exposed to userspace. 
> > > >  Make this exposure via a separate device, which can now be 
> > > > opened multiple times because each read/write transaction goes 
> > > > separately via the RM.
> > > > 
> > > > Concurrency is protected by the chip->tpm_mutex for each 
> > > > read/write transaction separately.  The TPM is cleared of all 
> > > > transient objects by the time the mutex is dropped, so there 
> > > > should be no interference between the kernel and userspace.
> > > 
> > > There's actually a missing kfree of context_buf on the tpms_release
> > > path as well.  This patch fixes it up.
> > 
> > Can you send me a fresh version of the whole patch so that I can 
> > include to v4 that includes also changes that I requested in my 
> > recent comments + all the fixes?
> 
> Sure, I think the attached is basically it
> 
> James

Thank you!

/Jarkko

[toc] | [prev] | [next] | [standalone]

#1563916

On Fri, Jan 20, 2017 at 03:39:14PM +0200, Jarkko Sakkinen wrote:
> On Thu, Jan 19, 2017 at 07:19:40AM -0500, James Bottomley wrote:
> > On Thu, 2017-01-19 at 12:49 +0200, Jarkko Sakkinen wrote:
> > > On Wed, Jan 18, 2017 at 10:01:03AM -0500, James Bottomley wrote:
> > > > On Mon, 2017-01-16 at 15:12 +0200, Jarkko Sakkinen wrote:
> > > > > From: James Bottomley <James.Bottomley@HansenPartnership.com>
> > > > > 
> > > > > Currently the Resource Manager (RM) is not exposed to userspace. 
> > > > >  Make this exposure via a separate device, which can now be 
> > > > > opened multiple times because each read/write transaction goes 
> > > > > separately via the RM.
> > > > > 
> > > > > Concurrency is protected by the chip->tpm_mutex for each 
> > > > > read/write transaction separately.  The TPM is cleared of all 
> > > > > transient objects by the time the mutex is dropped, so there 
> > > > > should be no interference between the kernel and userspace.
> > > > 
> > > > There's actually a missing kfree of context_buf on the tpms_release
> > > > path as well.  This patch fixes it up.
> > > 
> > > Can you send me a fresh version of the whole patch so that I can 
> > > include to v4 that includes also changes that I requested in my 
> > > recent comments + all the fixes?
> > 
> > Sure, I think the attached is basically it
> > 
> > James
> 
> Thank you!

'tabrm4' branch has been now rebased. It's now on top of master branch
that contains Stefan's latest patch (min body length check) that I've
reviewed and tested. It also contains your updated /dev/tpms patch.

I guess the 5 commits that are there now are such that we have fairly
good consensus, don't we? If so, can I add your reviewed-by and
tested-by to my commits and vice versa?

/Jarkko

[toc] | [prev] | [next] | [standalone]

#1564223

On Fri, 2017-01-20 at 23:05 +0200, Jarkko Sakkinen wrote:
> On Fri, Jan 20, 2017 at 03:39:14PM +0200, Jarkko Sakkinen wrote:
> > On Thu, Jan 19, 2017 at 07:19:40AM -0500, James Bottomley wrote:
> > > On Thu, 2017-01-19 at 12:49 +0200, Jarkko Sakkinen wrote:
> > > > On Wed, Jan 18, 2017 at 10:01:03AM -0500, James Bottomley
> > > > wrote:
> > > > > On Mon, 2017-01-16 at 15:12 +0200, Jarkko Sakkinen wrote:
> > > > > > From: James Bottomley <
> > > > > > James.Bottomley@HansenPartnership.com>
> > > > > > 
> > > > > > Currently the Resource Manager (RM) is not exposed to
> > > > > > userspace. 
> > > > > >  Make this exposure via a separate device, which can now be
> > > > > > opened multiple times because each read/write transaction
> > > > > > goes 
> > > > > > separately via the RM.
> > > > > > 
> > > > > > Concurrency is protected by the chip->tpm_mutex for each 
> > > > > > read/write transaction separately.  The TPM is cleared of
> > > > > > all 
> > > > > > transient objects by the time the mutex is dropped, so
> > > > > > there 
> > > > > > should be no interference between the kernel and userspace.
> > > > > 
> > > > > There's actually a missing kfree of context_buf on the
> > > > > tpms_release
> > > > > path as well.  This patch fixes it up.
> > > > 
> > > > Can you send me a fresh version of the whole patch so that I
> > > > can 
> > > > include to v4 that includes also changes that I requested in my
> > > > recent comments + all the fixes?
> > > 
> > > Sure, I think the attached is basically it
> > > 
> > > James
> > 
> > Thank you!
> 
> 'tabrm4' branch has been now rebased. It's now on top of master
> branch
> that contains Stefan's latest patch (min body length check) that I've
> reviewed and tested. It also contains your updated /dev/tpms patch.
> 
> I guess the 5 commits that are there now are such that we have fairly
> good consensus, don't we? If so, can I add your reviewed-by and
> tested-by to my commits and vice versa?

Did you actually test it?  It doesn't work for me.  The bisected fault
commit is this one (newly introduced into the tabrm4 branch)

commit 9b7f4252655228c8d0b86e1492cc7fb3feaa5686
Author: Stefan Berger <stefanb@linux.vnet.ibm.com>
Date:   Thu Jan 19 07:19:12 2017 -0500

    tpm: Check size of response before accessing data
 
The specific problem is that our min_rsp_length in
tpm_{load,save}_context includes a header size and the check this
introduces does the check is against the body size, meaning the load
fails because tpm_transmit_cmd thinks the response is too short.

The patch to fix this is below.

James

---
commit 480f2bb484f5a7e6100c6b0d1c79f72a05a0ca88
Author: James Bottomley <James.Bottomley@HansenPartnership.com>
Date:   Sat Jan 21 11:26:24 2017 -0800

    fix tpm_transmit_cmd min response size problem

diff --git a/drivers/char/tpm/tpm2-space.c b/drivers/char/tpm/tpm2-space.c
index 4b5c714..3237d7c 100644
--- a/drivers/char/tpm/tpm2-space.c
+++ b/drivers/char/tpm/tpm2-space.c
@@ -53,7 +53,7 @@ static int tpm2_load_context(struct tpm_chip *chip, u8 *buf,
 	tpm_buf_append(&tbuf, &buf[*offset], body_size);
 
 	rc = tpm_transmit_cmd(chip, NULL, tbuf.data, PAGE_SIZE,
-			      TPM_HEADER_SIZE + 4, TPM_TRANSMIT_UNLOCKED, "load context");
+			      4, TPM_TRANSMIT_UNLOCKED, "load context");
 	if ((rc & TPM2_RC_HANDLE) == TPM2_RC_HANDLE) {
 		rc = -ENOENT;
 		tpm_buf_destroy(&tbuf);
@@ -89,7 +89,7 @@ static int tpm2_save_context(struct tpm_chip *chip, u32 handle, u8 *buf,
 
 	tpm_buf_append_u32(&tbuf, handle);
 
-	rc = tpm_transmit_cmd(chip, NULL, tbuf.data, PAGE_SIZE, TPM_HEADER_SIZE,
+	rc = tpm_transmit_cmd(chip, NULL, tbuf.data, PAGE_SIZE, 0,
 			      TPM_TRANSMIT_UNLOCKED, NULL);
 	if (rc < 0) {
 		dev_warn(&chip->dev, "%s: saving failed with a system error %d\n",

[toc] | [prev] | [next] | [standalone]

#1564425

On Sat, Jan 21, 2017 at 11:28:55AM -0800, James Bottomley wrote:
> On Fri, 2017-01-20 at 23:05 +0200, Jarkko Sakkinen wrote:
> > On Fri, Jan 20, 2017 at 03:39:14PM +0200, Jarkko Sakkinen wrote:
> > > On Thu, Jan 19, 2017 at 07:19:40AM -0500, James Bottomley wrote:
> > > > On Thu, 2017-01-19 at 12:49 +0200, Jarkko Sakkinen wrote:
> > > > > On Wed, Jan 18, 2017 at 10:01:03AM -0500, James Bottomley
> > > > > wrote:
> > > > > > On Mon, 2017-01-16 at 15:12 +0200, Jarkko Sakkinen wrote:
> > > > > > > From: James Bottomley <
> > > > > > > James.Bottomley@HansenPartnership.com>
> > > > > > > 
> > > > > > > Currently the Resource Manager (RM) is not exposed to
> > > > > > > userspace. 
> > > > > > >  Make this exposure via a separate device, which can now be
> > > > > > > opened multiple times because each read/write transaction
> > > > > > > goes 
> > > > > > > separately via the RM.
> > > > > > > 
> > > > > > > Concurrency is protected by the chip->tpm_mutex for each 
> > > > > > > read/write transaction separately.  The TPM is cleared of
> > > > > > > all 
> > > > > > > transient objects by the time the mutex is dropped, so
> > > > > > > there 
> > > > > > > should be no interference between the kernel and userspace.
> > > > > > 
> > > > > > There's actually a missing kfree of context_buf on the
> > > > > > tpms_release
> > > > > > path as well.  This patch fixes it up.
> > > > > 
> > > > > Can you send me a fresh version of the whole patch so that I
> > > > > can 
> > > > > include to v4 that includes also changes that I requested in my
> > > > > recent comments + all the fixes?
> > > > 
> > > > Sure, I think the attached is basically it
> > > > 
> > > > James
> > > 
> > > Thank you!
> > 
> > 'tabrm4' branch has been now rebased. It's now on top of master
> > branch
> > that contains Stefan's latest patch (min body length check) that I've
> > reviewed and tested. It also contains your updated /dev/tpms patch.
> > 
> > I guess the 5 commits that are there now are such that we have fairly
> > good consensus, don't we? If so, can I add your reviewed-by and
> > tested-by to my commits and vice versa?
> 
> Did you actually test it?  It doesn't work for me.  The bisected fault
> commit is this one (newly introduced into the tabrm4 branch)
> 
> commit 9b7f4252655228c8d0b86e1492cc7fb3feaa5686
> Author: Stefan Berger <stefanb@linux.vnet.ibm.com>
> Date:   Thu Jan 19 07:19:12 2017 -0500
> 
>     tpm: Check size of response before accessing data
>  
> The specific problem is that our min_rsp_length in
> tpm_{load,save}_context includes a header size and the check this
> introduces does the check is against the body size, meaning the load
> fails because tpm_transmit_cmd thinks the response is too short.
> 
> The patch to fix this is below.
> 
> James

I noticed the same thing last night. Sorry about that. It's now been
fixed. I did test it but somehow what went to my remote tree does not
have matching contents so I screwed something up at some point.

/Jarkko

> 
> ---
> commit 480f2bb484f5a7e6100c6b0d1c79f72a05a0ca88
> Author: James Bottomley <James.Bottomley@HansenPartnership.com>
> Date:   Sat Jan 21 11:26:24 2017 -0800
> 
>     fix tpm_transmit_cmd min response size problem
> 
> diff --git a/drivers/char/tpm/tpm2-space.c b/drivers/char/tpm/tpm2-space.c
> index 4b5c714..3237d7c 100644
> --- a/drivers/char/tpm/tpm2-space.c
> +++ b/drivers/char/tpm/tpm2-space.c
> @@ -53,7 +53,7 @@ static int tpm2_load_context(struct tpm_chip *chip, u8 *buf,
>  	tpm_buf_append(&tbuf, &buf[*offset], body_size);
>  
>  	rc = tpm_transmit_cmd(chip, NULL, tbuf.data, PAGE_SIZE,
> -			      TPM_HEADER_SIZE + 4, TPM_TRANSMIT_UNLOCKED, "load context");
> +			      4, TPM_TRANSMIT_UNLOCKED, "load context");
>  	if ((rc & TPM2_RC_HANDLE) == TPM2_RC_HANDLE) {
>  		rc = -ENOENT;
>  		tpm_buf_destroy(&tbuf);
> @@ -89,7 +89,7 @@ static int tpm2_save_context(struct tpm_chip *chip, u32 handle, u8 *buf,
>  
>  	tpm_buf_append_u32(&tbuf, handle);
>  
> -	rc = tpm_transmit_cmd(chip, NULL, tbuf.data, PAGE_SIZE, TPM_HEADER_SIZE,
> +	rc = tpm_transmit_cmd(chip, NULL, tbuf.data, PAGE_SIZE, 0,
>  			      TPM_TRANSMIT_UNLOCKED, NULL);
>  	if (rc < 0) {
>  		dev_warn(&chip->dev, "%s: saving failed with a system error %d\n",
> 
> 
> 

[toc] | [prev] | [next] | [standalone]

#1564243

On Fri, 2017-01-20 at 23:05 +0200, Jarkko Sakkinen wrote:
> On Fri, Jan 20, 2017 at 03:39:14PM +0200, Jarkko Sakkinen wrote:
> > On Thu, Jan 19, 2017 at 07:19:40AM -0500, James Bottomley wrote:
> > > On Thu, 2017-01-19 at 12:49 +0200, Jarkko Sakkinen wrote:
> > > > On Wed, Jan 18, 2017 at 10:01:03AM -0500, James Bottomley
> > > > wrote:
> > > > > On Mon, 2017-01-16 at 15:12 +0200, Jarkko Sakkinen wrote:
> > > > > > From: James Bottomley <
> > > > > > James.Bottomley@HansenPartnership.com>
> > > > > > 
> > > > > > Currently the Resource Manager (RM) is not exposed to
> > > > > > userspace. 
> > > > > >  Make this exposure via a separate device, which can now be
> > > > > > opened multiple times because each read/write transaction
> > > > > > goes 
> > > > > > separately via the RM.
> > > > > > 
> > > > > > Concurrency is protected by the chip->tpm_mutex for each 
> > > > > > read/write transaction separately.  The TPM is cleared of
> > > > > > all 
> > > > > > transient objects by the time the mutex is dropped, so
> > > > > > there 
> > > > > > should be no interference between the kernel and userspace.
> > > > > 
> > > > > There's actually a missing kfree of context_buf on the
> > > > > tpms_release
> > > > > path as well.  This patch fixes it up.
> > > > 
> > > > Can you send me a fresh version of the whole patch so that I
> > > > can 
> > > > include to v4 that includes also changes that I requested in my
> > > > recent comments + all the fixes?
> > > 
> > > Sure, I think the attached is basically it
> > > 
> > > James
> > 
> > Thank you!
> 
> 'tabrm4' branch has been now rebased. It's now on top of master 
> branch that contains Stefan's latest patch (min body length check) 
> that I've reviewed and tested. It also contains your updated
> /dev/tpms patch.
> 
> I guess the 5 commits that are there now are such that we have fairly
> good consensus, don't we? If so, can I add your reviewed-by and
> tested-by to my commits and vice versa?

It looks like there's another problem: you need a continue after the
transient object is garbage collected otherwise the code falls through,
does a flush which fails and then adds a ~0 as the handle meaning we'll
have a mismatch between the saved contexts and the handles.

James

---

commit 0da3f83ce889379bd1741a11b07a30818a223924
Author: James Bottomley <James.Bottomley@HansenPartnership.com>
Date:   Sat Jan 21 12:19:06 2017 -0800

    continue after lazy reclaim

diff --git a/drivers/char/tpm/tpm2-space.c b/drivers/char/tpm/tpm2-space.c
index 8713d7f..9d87537 100644
--- a/drivers/char/tpm/tpm2-space.c
+++ b/drivers/char/tpm/tpm2-space.c
@@ -288,9 +288,10 @@ static int tpm2_save_space(struct tpm_chip *chip)
 		rc = tpm2_save_context(chip, space->context_tbl[i],
 				       space->context_buf, PAGE_SIZE,
 				       &offset);
-		if (rc == -ENOENT)
+		if (rc == -ENOENT) {
 			space->context_tbl[i] = 0;
-		else if (rc) {
+			continue;
+		} else if (rc) {
 			tpm2_flush_space(chip);
 			return rc;
 		}

[toc] | [prev] | [next] | [standalone]

#1564428

On Sat, Jan 21, 2017 at 12:38:56PM -0800, James Bottomley wrote:
> On Fri, 2017-01-20 at 23:05 +0200, Jarkko Sakkinen wrote:
> > On Fri, Jan 20, 2017 at 03:39:14PM +0200, Jarkko Sakkinen wrote:
> > > On Thu, Jan 19, 2017 at 07:19:40AM -0500, James Bottomley wrote:
> > > > On Thu, 2017-01-19 at 12:49 +0200, Jarkko Sakkinen wrote:
> > > > > On Wed, Jan 18, 2017 at 10:01:03AM -0500, James Bottomley
> > > > > wrote:
> > > > > > On Mon, 2017-01-16 at 15:12 +0200, Jarkko Sakkinen wrote:
> > > > > > > From: James Bottomley <
> > > > > > > James.Bottomley@HansenPartnership.com>
> > > > > > > 
> > > > > > > Currently the Resource Manager (RM) is not exposed to
> > > > > > > userspace. 
> > > > > > >  Make this exposure via a separate device, which can now be
> > > > > > > opened multiple times because each read/write transaction
> > > > > > > goes 
> > > > > > > separately via the RM.
> > > > > > > 
> > > > > > > Concurrency is protected by the chip->tpm_mutex for each 
> > > > > > > read/write transaction separately.  The TPM is cleared of
> > > > > > > all 
> > > > > > > transient objects by the time the mutex is dropped, so
> > > > > > > there 
> > > > > > > should be no interference between the kernel and userspace.
> > > > > > 
> > > > > > There's actually a missing kfree of context_buf on the
> > > > > > tpms_release
> > > > > > path as well.  This patch fixes it up.
> > > > > 
> > > > > Can you send me a fresh version of the whole patch so that I
> > > > > can 
> > > > > include to v4 that includes also changes that I requested in my
> > > > > recent comments + all the fixes?
> > > > 
> > > > Sure, I think the attached is basically it
> > > > 
> > > > James
> > > 
> > > Thank you!
> > 
> > 'tabrm4' branch has been now rebased. It's now on top of master 
> > branch that contains Stefan's latest patch (min body length check) 
> > that I've reviewed and tested. It also contains your updated
> > /dev/tpms patch.
> > 
> > I guess the 5 commits that are there now are such that we have fairly
> > good consensus, don't we? If so, can I add your reviewed-by and
> > tested-by to my commits and vice versa?
> 
> It looks like there's another problem: you need a continue after the
> transient object is garbage collected otherwise the code falls through,
> does a flush which fails and then adds a ~0 as the handle meaning we'll
> have a mismatch between the saved contexts and the handles.
> 
> James

Oops, my bad. It's now fixed. Thank you!

/Jarkko

> 
> ---
> 
> commit 0da3f83ce889379bd1741a11b07a30818a223924
> Author: James Bottomley <James.Bottomley@HansenPartnership.com>
> Date:   Sat Jan 21 12:19:06 2017 -0800
> 
>     continue after lazy reclaim
> 
> diff --git a/drivers/char/tpm/tpm2-space.c b/drivers/char/tpm/tpm2-space.c
> index 8713d7f..9d87537 100644
> --- a/drivers/char/tpm/tpm2-space.c
> +++ b/drivers/char/tpm/tpm2-space.c
> @@ -288,9 +288,10 @@ static int tpm2_save_space(struct tpm_chip *chip)
>  		rc = tpm2_save_context(chip, space->context_tbl[i],
>  				       space->context_buf, PAGE_SIZE,
>  				       &offset);
> -		if (rc == -ENOENT)
> +		if (rc == -ENOENT) {
>  			space->context_tbl[i] = 0;
> -		else if (rc) {
> +			continue;
> +		} else if (rc) {
>  			tpm2_flush_space(chip);
>  			return rc;
>  		}

[toc] | [prev] | [next] | [standalone]

#1564478

On Fri, 2017-01-20 at 23:05 +0200, Jarkko Sakkinen wrote:
> 'tabrm4' branch has been now rebased. It's now on top of master
> branch
> that contains Stefan's latest patch (min body length check) that I've
> reviewed and tested. It also contains your updated /dev/tpms patch.
> 
> I guess the 5 commits that are there now are such that we have fairly
> good consensus, don't we? If so, can I add your reviewed-by and
> tested-by to my commits and vice versa?

We're still failing my test_transients.  This is the full python of the
test case:


    def test_transients(self):
        k = self.open_transients()
        self.c.flush_context(k[0])
        self.c.change_auth(self.c.SRK, k[1], None, pwd1)
        ...

It's failing at self.c.flush_context(k[0]) with TPM_RC_VALUE.  It's the
same problem Ken complained about: TPM2_FlushContext doesn't have a
declared handle area so we don't translate the handle being sent down. 
 We have to fix this either by intercepting the flush and manually
translating the context, or by being dangerously clever and marking
flush as a command which takes one handle.

James

[toc] | [prev] | [next] | [standalone]

#1564548

On Sun, 2017-01-22 at 09:49 -0800, James Bottomley wrote:
> On Fri, 2017-01-20 at 23:05 +0200, Jarkko Sakkinen wrote:
> > 'tabrm4' branch has been now rebased. It's now on top of master
> > branch that contains Stefan's latest patch (min body length check) 
> > that I've reviewed and tested. It also contains your updated
> > /dev/tpms patch.
> > 
> > I guess the 5 commits that are there now are such that we have 
> > fairly good consensus, don't we? If so, can I add your reviewed-by 
> > and tested-by to my commits and vice versa?
> 
> We're still failing my test_transients.  This is the full python of 
> the test case:
> 
> 
>     def test_transients(self):
>         k = self.open_transients()
>         self.c.flush_context(k[0])
>         self.c.change_auth(self.c.SRK, k[1], None, pwd1)
>         ...
> 
> It's failing at self.c.flush_context(k[0]) with TPM_RC_VALUE.  It's 
> the same problem Ken complained about: TPM2_FlushContext doesn't have 
> a declared handle area so we don't translate the handle being sent
> down.  We have to fix this either by intercepting the flush and 
> manually translating the context, or by being dangerously clever and 
> marking flush as a command which takes one handle.

This is what the dangerously clever fix looks like.  With this and a
few other changes, my smoke tests now pass.

James

---

diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index f54226d..d5517f1 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -1053,9 +1053,16 @@ int tpm2_auto_startup(struct tpm_chip *chip)
 		goto out;
 	}
 
-	for (i = 0; i < nr_commands; i++)
-		chip->cc_attrs_tbl[i] = be32_to_cpup(
-			(u32 *)&buf.data[TPM_HEADER_SIZE + 9 + 4 * i]);
+	for (i = 0; i < nr_commands; i++) {
+		u32 attr = be32_to_cpup((u32 *)&buf.data[TPM_HEADER_SIZE +
+							 9 + 4 * i]);
+		if ((attr & GENMASK(15,0)) == TPM2_CC_FLUSH_CONTEXT)
+			/* Warning: dangerous cleverness here.
+			 * Mark flush as taking a handle argument so
+			 * it gets correctly translated */
+			attr |= 1 << TPM2_CC_ATTR_CHANDLES;
+		chip->cc_attrs_tbl[i] = attr;
+	}
 
 	chip->nr_commands = nr_commands;
 	tpm_buf_destroy(&buf);

[toc] | [prev] | [next] | [standalone]

#1564565

On Sun, Jan 22, 2017 at 10:48:12AM -0800, James Bottomley wrote:
> On Sun, 2017-01-22 at 09:49 -0800, James Bottomley wrote:
> > On Fri, 2017-01-20 at 23:05 +0200, Jarkko Sakkinen wrote:
> > > 'tabrm4' branch has been now rebased. It's now on top of master
> > > branch that contains Stefan's latest patch (min body length check) 
> > > that I've reviewed and tested. It also contains your updated
> > > /dev/tpms patch.
> > > 
> > > I guess the 5 commits that are there now are such that we have 
> > > fairly good consensus, don't we? If so, can I add your reviewed-by 
> > > and tested-by to my commits and vice versa?
> > 
> > We're still failing my test_transients.  This is the full python of 
> > the test case:
> > 
> > 
> >     def test_transients(self):
> >         k = self.open_transients()
> >         self.c.flush_context(k[0])
> >         self.c.change_auth(self.c.SRK, k[1], None, pwd1)
> >         ...
> > 
> > It's failing at self.c.flush_context(k[0]) with TPM_RC_VALUE.  It's 
> > the same problem Ken complained about: TPM2_FlushContext doesn't have 
> > a declared handle area so we don't translate the handle being sent
> > down.  We have to fix this either by intercepting the flush and 
> > manually translating the context, or by being dangerously clever and 
> > marking flush as a command which takes one handle.
> 
> This is what the dangerously clever fix looks like.  With this and a
> few other changes, my smoke tests now pass.
> 
> James

I don't want to be clever here. I will rather intercept the body and
try to keep the core code simple and easy to understand.

/Jarkko

[toc] | [prev] | [next] | [standalone]

#1564567

On Sun, Jan 22, 2017 at 11:01:07PM +0200, Jarkko Sakkinen wrote:
> On Sun, Jan 22, 2017 at 10:30:55PM +0200, Jarkko Sakkinen wrote:
> > On Sun, Jan 22, 2017 at 10:48:12AM -0800, James Bottomley wrote:
> > > On Sun, 2017-01-22 at 09:49 -0800, James Bottomley wrote:
> > > > On Fri, 2017-01-20 at 23:05 +0200, Jarkko Sakkinen wrote:
> > > > > 'tabrm4' branch has been now rebased. It's now on top of master
> > > > > branch that contains Stefan's latest patch (min body length check) 
> > > > > that I've reviewed and tested. It also contains your updated
> > > > > /dev/tpms patch.
> > > > > 
> > > > > I guess the 5 commits that are there now are such that we have 
> > > > > fairly good consensus, don't we? If so, can I add your reviewed-by 
> > > > > and tested-by to my commits and vice versa?
> > > > 
> > > > We're still failing my test_transients.  This is the full python of 
> > > > the test case:
> > > > 
> > > > 
> > > >     def test_transients(self):
> > > >         k = self.open_transients()
> > > >         self.c.flush_context(k[0])
> > > >         self.c.change_auth(self.c.SRK, k[1], None, pwd1)
> > > >         ...
> > > > 
> > > > It's failing at self.c.flush_context(k[0]) with TPM_RC_VALUE.  It's 
> > > > the same problem Ken complained about: TPM2_FlushContext doesn't have 
> > > > a declared handle area so we don't translate the handle being sent
> > > > down.  We have to fix this either by intercepting the flush and 
> > > > manually translating the context, or by being dangerously clever and 
> > > > marking flush as a command which takes one handle.
> > > 
> > > This is what the dangerously clever fix looks like.  With this and a
> > > few other changes, my smoke tests now pass.
> > > 
> > > James
> > 
> > I don't want to be clever here. I will rather intercept the body and
> > try to keep the core code simple and easy to understand.
> 
> It came out quite clean actually.
> 
> I just encapsulated handle mapping and have this in the beginning of
> tpm2_map_command:
> 
> if (cc == TPM2_CC_FLUSH_CONTEXT)
> 	return tpm2_map_to_phandle(space, &cmd[TPM_HEADER_SIZE]);
> 
> I think this documents better what is actually going on than tinkering
> cc_attr_tbl.
> 
> /Jarkko

Actually what you suggested is much better idea because it will also
take care of validation. I'm still going to keep tpm2_map_to_phandle
because it makes the code flow a lot cleaner and probably sessions
have to anyway make it even more complicated.

/Jarkko

[toc] | [prev] | [next] | [standalone]

#1564572

On Sun, 2017-01-22 at 23:04 +0200, Jarkko Sakkinen wrote:
> On Sun, Jan 22, 2017 at 11:01:07PM +0200, Jarkko Sakkinen wrote:
> > On Sun, Jan 22, 2017 at 10:30:55PM +0200, Jarkko Sakkinen wrote:
> > > On Sun, Jan 22, 2017 at 10:48:12AM -0800, James Bottomley wrote:
> > > > On Sun, 2017-01-22 at 09:49 -0800, James Bottomley wrote:
> > > > > On Fri, 2017-01-20 at 23:05 +0200, Jarkko Sakkinen wrote:
> > > > > > 'tabrm4' branch has been now rebased. It's now on top of
> > > > > > master
> > > > > > branch that contains Stefan's latest patch (min body length
> > > > > > check) 
> > > > > > that I've reviewed and tested. It also contains your
> > > > > > updated
> > > > > > /dev/tpms patch.
> > > > > > 
> > > > > > I guess the 5 commits that are there now are such that we
> > > > > > have 
> > > > > > fairly good consensus, don't we? If so, can I add your
> > > > > > reviewed-by 
> > > > > > and tested-by to my commits and vice versa?
> > > > > 
> > > > > We're still failing my test_transients.  This is the full
> > > > > python of 
> > > > > the test case:
> > > > > 
> > > > > 
> > > > >     def test_transients(self):
> > > > >         k = self.open_transients()
> > > > >         self.c.flush_context(k[0])
> > > > >         self.c.change_auth(self.c.SRK, k[1], None, pwd1)
> > > > >         ...
> > > > > 
> > > > > It's failing at self.c.flush_context(k[0]) with TPM_RC_VALUE.
> > > > >   It's 
> > > > > the same problem Ken complained about: TPM2_FlushContext
> > > > > doesn't have 
> > > > > a declared handle area so we don't translate the handle being
> > > > > sent
> > > > > down.  We have to fix this either by intercepting the flush
> > > > > and 
> > > > > manually translating the context, or by being dangerously
> > > > > clever and 
> > > > > marking flush as a command which takes one handle.
> > > > 
> > > > This is what the dangerously clever fix looks like.  With this
> > > > and a
> > > > few other changes, my smoke tests now pass.
> > > > 
> > > > James
> > > 
> > > I don't want to be clever here. I will rather intercept the body
> > > and
> > > try to keep the core code simple and easy to understand.
> > 
> > It came out quite clean actually.
> > 
> > I just encapsulated handle mapping and have this in the beginning
> > of
> > tpm2_map_command:
> > 
> > if (cc == TPM2_CC_FLUSH_CONTEXT)
> > 	return tpm2_map_to_phandle(space, &cmd[TPM_HEADER_SIZE]);
> > 
> > I think this documents better what is actually going on than
> > tinkering
> > cc_attr_tbl.
> > 
> > /Jarkko
> 
> Actually what you suggested is much better idea because it will also
> take care of validation.

Yes, that's why it's clever ... I'm just always wary of clever code
because of the Kernighan principle.

>  I'm still going to keep tpm2_map_to_phandle because it makes the 
> code flow a lot cleaner and probably sessions have to anyway make it
> even more complicated.

OK, there's one more thing that seems to be causing problems: when
tpm2_save_context fails because the handle no longer exists (like it's
been flushed) it returns TPM_RC_REFERENCE_H0 not TPM_RC_HANDLE (the
session code does seem to return TPM_RC_HANDLE under some
circumstances).

James

[toc] | [prev] | [next] | [standalone]

#1564986

On Sun, Jan 22, 2017 at 01:36:28PM -0800, James Bottomley wrote:
> On Sun, 2017-01-22 at 23:04 +0200, Jarkko Sakkinen wrote:
> > On Sun, Jan 22, 2017 at 11:01:07PM +0200, Jarkko Sakkinen wrote:
> > > On Sun, Jan 22, 2017 at 10:30:55PM +0200, Jarkko Sakkinen wrote:
> > > > On Sun, Jan 22, 2017 at 10:48:12AM -0800, James Bottomley wrote:
> > > > > On Sun, 2017-01-22 at 09:49 -0800, James Bottomley wrote:
> > > > > > On Fri, 2017-01-20 at 23:05 +0200, Jarkko Sakkinen wrote:
> > > > > > > 'tabrm4' branch has been now rebased. It's now on top of
> > > > > > > master
> > > > > > > branch that contains Stefan's latest patch (min body length
> > > > > > > check) 
> > > > > > > that I've reviewed and tested. It also contains your
> > > > > > > updated
> > > > > > > /dev/tpms patch.
> > > > > > > 
> > > > > > > I guess the 5 commits that are there now are such that we
> > > > > > > have 
> > > > > > > fairly good consensus, don't we? If so, can I add your
> > > > > > > reviewed-by 
> > > > > > > and tested-by to my commits and vice versa?
> > > > > > 
> > > > > > We're still failing my test_transients.  This is the full
> > > > > > python of 
> > > > > > the test case:
> > > > > > 
> > > > > > 
> > > > > >     def test_transients(self):
> > > > > >         k = self.open_transients()
> > > > > >         self.c.flush_context(k[0])
> > > > > >         self.c.change_auth(self.c.SRK, k[1], None, pwd1)
> > > > > >         ...
> > > > > > 
> > > > > > It's failing at self.c.flush_context(k[0]) with TPM_RC_VALUE.
> > > > > >   It's 
> > > > > > the same problem Ken complained about: TPM2_FlushContext
> > > > > > doesn't have 
> > > > > > a declared handle area so we don't translate the handle being
> > > > > > sent
> > > > > > down.  We have to fix this either by intercepting the flush
> > > > > > and 
> > > > > > manually translating the context, or by being dangerously
> > > > > > clever and 
> > > > > > marking flush as a command which takes one handle.
> > > > > 
> > > > > This is what the dangerously clever fix looks like.  With this
> > > > > and a
> > > > > few other changes, my smoke tests now pass.
> > > > > 
> > > > > James
> > > > 
> > > > I don't want to be clever here. I will rather intercept the body
> > > > and
> > > > try to keep the core code simple and easy to understand.
> > > 
> > > It came out quite clean actually.
> > > 
> > > I just encapsulated handle mapping and have this in the beginning
> > > of
> > > tpm2_map_command:
> > > 
> > > if (cc == TPM2_CC_FLUSH_CONTEXT)
> > > 	return tpm2_map_to_phandle(space, &cmd[TPM_HEADER_SIZE]);
> > > 
> > > I think this documents better what is actually going on than
> > > tinkering
> > > cc_attr_tbl.
> > > 
> > > /Jarkko
> > 
> > Actually what you suggested is much better idea because it will also
> > take care of validation.
> 
> Yes, that's why it's clever ... I'm just always wary of clever code
> because of the Kernighan principle.
> 
> >  I'm still going to keep tpm2_map_to_phandle because it makes the 
> > code flow a lot cleaner and probably sessions have to anyway make it
> > even more complicated.
> 
> OK, there's one more thing that seems to be causing problems: when
> tpm2_save_context fails because the handle no longer exists (like it's
> been flushed) it returns TPM_RC_REFERENCE_H0 not TPM_RC_HANDLE (the
> session code does seem to return TPM_RC_HANDLE under some
> circumstances).
> 
> James

What is your way for reproducing this issue? Just want to add
a test case for my smoke test suite so that I can verify that
the issue is fixed once I've fixed it.

/Jarkko

[toc] | [prev] | [next] | [standalone]

#1565107

On Mon, 2017-01-23 at 16:09 +0200, Jarkko Sakkinen wrote:
> On Sun, Jan 22, 2017 at 01:36:28PM -0800, James Bottomley wrote:
> > On Sun, 2017-01-22 at 23:04 +0200, Jarkko Sakkinen wrote:
> > > On Sun, Jan 22, 2017 at 11:01:07PM +0200, Jarkko Sakkinen wrote:
> > > > On Sun, Jan 22, 2017 at 10:30:55PM +0200, Jarkko Sakkinen
> > > > wrote:
> > > > > On Sun, Jan 22, 2017 at 10:48:12AM -0800, James Bottomley
> > > > > wrote:
> > > > > > On Sun, 2017-01-22 at 09:49 -0800, James Bottomley wrote:
> > > > > > > On Fri, 2017-01-20 at 23:05 +0200, Jarkko Sakkinen wrote:
> > > > > > > > 'tabrm4' branch has been now rebased. It's now on top
> > > > > > > > of
> > > > > > > > master
> > > > > > > > branch that contains Stefan's latest patch (min body
> > > > > > > > length
> > > > > > > > check) 
> > > > > > > > that I've reviewed and tested. It also contains your
> > > > > > > > updated
> > > > > > > > /dev/tpms patch.
> > > > > > > > 
> > > > > > > > I guess the 5 commits that are there now are such that
> > > > > > > > we
> > > > > > > > have 
> > > > > > > > fairly good consensus, don't we? If so, can I add your
> > > > > > > > reviewed-by 
> > > > > > > > and tested-by to my commits and vice versa?
> > > > > > > 
> > > > > > > We're still failing my test_transients.  This is the full
> > > > > > > python of 
> > > > > > > the test case:
> > > > > > > 
> > > > > > > 
> > > > > > >     def test_transients(self):
> > > > > > >         k = self.open_transients()
> > > > > > >         self.c.flush_context(k[0])
> > > > > > >         self.c.change_auth(self.c.SRK, k[1], None, pwd1)
> > > > > > >         ...
> > > > > > > 
> > > > > > > It's failing at self.c.flush_context(k[0]) with
> > > > > > > TPM_RC_VALUE.
> > > > > > >   It's 
> > > > > > > the same problem Ken complained about: TPM2_FlushContext
> > > > > > > doesn't have 
> > > > > > > a declared handle area so we don't translate the handle
> > > > > > > being
> > > > > > > sent
> > > > > > > down.  We have to fix this either by intercepting the
> > > > > > > flush
> > > > > > > and 
> > > > > > > manually translating the context, or by being dangerously
> > > > > > > clever and 
> > > > > > > marking flush as a command which takes one handle.
> > > > > > 
> > > > > > This is what the dangerously clever fix looks like.  With
> > > > > > this
> > > > > > and a
> > > > > > few other changes, my smoke tests now pass.
> > > > > > 
> > > > > > James
> > > > > 
> > > > > I don't want to be clever here. I will rather intercept the
> > > > > body
> > > > > and
> > > > > try to keep the core code simple and easy to understand.
> > > > 
> > > > It came out quite clean actually.
> > > > 
> > > > I just encapsulated handle mapping and have this in the
> > > > beginning
> > > > of
> > > > tpm2_map_command:
> > > > 
> > > > if (cc == TPM2_CC_FLUSH_CONTEXT)
> > > > 	return tpm2_map_to_phandle(space,
> > > > &cmd[TPM_HEADER_SIZE]);
> > > > 
> > > > I think this documents better what is actually going on than
> > > > tinkering
> > > > cc_attr_tbl.
> > > > 
> > > > /Jarkko
> > > 
> > > Actually what you suggested is much better idea because it will
> > > also
> > > take care of validation.
> > 
> > Yes, that's why it's clever ... I'm just always wary of clever code
> > because of the Kernighan principle.
> > 
> > >  I'm still going to keep tpm2_map_to_phandle because it makes the
> > > code flow a lot cleaner and probably sessions have to anyway make
> > > it
> > > even more complicated.
> > 
> > OK, there's one more thing that seems to be causing problems: when
> > tpm2_save_context fails because the handle no longer exists (like
> > it's
> > been flushed) it returns TPM_RC_REFERENCE_H0 not TPM_RC_HANDLE (the
> > session code does seem to return TPM_RC_HANDLE under some
> > circumstances).
> > 
> > James
> 
> What is your way for reproducing this issue? Just want to add
> a test case for my smoke test suite so that I can verify that
> the issue is fixed once I've fixed it.


It's the test_handle_clearing test in tpm2_sessions_smoke.py.  It's
probably easier if I publish the current state of my mods to your tpm2
-scripts, so here they are:

http://git.kernel.org/cgit/linux/kernel/git/jejb/tpm2-scripts.git/

James

[toc] | [prev] | [next] | [standalone]

#1565802

On Mon, Jan 23, 2017 at 08:14:55AM -0800, James Bottomley wrote:
> On Mon, 2017-01-23 at 16:09 +0200, Jarkko Sakkinen wrote:
> > On Sun, Jan 22, 2017 at 01:36:28PM -0800, James Bottomley wrote:
> > > On Sun, 2017-01-22 at 23:04 +0200, Jarkko Sakkinen wrote:
> > > > On Sun, Jan 22, 2017 at 11:01:07PM +0200, Jarkko Sakkinen wrote:
> > > > > On Sun, Jan 22, 2017 at 10:30:55PM +0200, Jarkko Sakkinen
> > > > > wrote:
> > > > > > On Sun, Jan 22, 2017 at 10:48:12AM -0800, James Bottomley
> > > > > > wrote:
> > > > > > > On Sun, 2017-01-22 at 09:49 -0800, James Bottomley wrote:
> > > > > > > > On Fri, 2017-01-20 at 23:05 +0200, Jarkko Sakkinen wrote:
> > > > > > > > > 'tabrm4' branch has been now rebased. It's now on top
> > > > > > > > > of
> > > > > > > > > master
> > > > > > > > > branch that contains Stefan's latest patch (min body
> > > > > > > > > length
> > > > > > > > > check) 
> > > > > > > > > that I've reviewed and tested. It also contains your
> > > > > > > > > updated
> > > > > > > > > /dev/tpms patch.
> > > > > > > > > 
> > > > > > > > > I guess the 5 commits that are there now are such that
> > > > > > > > > we
> > > > > > > > > have 
> > > > > > > > > fairly good consensus, don't we? If so, can I add your
> > > > > > > > > reviewed-by 
> > > > > > > > > and tested-by to my commits and vice versa?
> > > > > > > > 
> > > > > > > > We're still failing my test_transients.  This is the full
> > > > > > > > python of 
> > > > > > > > the test case:
> > > > > > > > 
> > > > > > > > 
> > > > > > > >     def test_transients(self):
> > > > > > > >         k = self.open_transients()
> > > > > > > >         self.c.flush_context(k[0])
> > > > > > > >         self.c.change_auth(self.c.SRK, k[1], None, pwd1)
> > > > > > > >         ...
> > > > > > > > 
> > > > > > > > It's failing at self.c.flush_context(k[0]) with
> > > > > > > > TPM_RC_VALUE.
> > > > > > > >   It's 
> > > > > > > > the same problem Ken complained about: TPM2_FlushContext
> > > > > > > > doesn't have 
> > > > > > > > a declared handle area so we don't translate the handle
> > > > > > > > being
> > > > > > > > sent
> > > > > > > > down.  We have to fix this either by intercepting the
> > > > > > > > flush
> > > > > > > > and 
> > > > > > > > manually translating the context, or by being dangerously
> > > > > > > > clever and 
> > > > > > > > marking flush as a command which takes one handle.
> > > > > > > 
> > > > > > > This is what the dangerously clever fix looks like.  With
> > > > > > > this
> > > > > > > and a
> > > > > > > few other changes, my smoke tests now pass.
> > > > > > > 
> > > > > > > James
> > > > > > 
> > > > > > I don't want to be clever here. I will rather intercept the
> > > > > > body
> > > > > > and
> > > > > > try to keep the core code simple and easy to understand.
> > > > > 
> > > > > It came out quite clean actually.
> > > > > 
> > > > > I just encapsulated handle mapping and have this in the
> > > > > beginning
> > > > > of
> > > > > tpm2_map_command:
> > > > > 
> > > > > if (cc == TPM2_CC_FLUSH_CONTEXT)
> > > > > 	return tpm2_map_to_phandle(space,
> > > > > &cmd[TPM_HEADER_SIZE]);
> > > > > 
> > > > > I think this documents better what is actually going on than
> > > > > tinkering
> > > > > cc_attr_tbl.
> > > > > 
> > > > > /Jarkko
> > > > 
> > > > Actually what you suggested is much better idea because it will
> > > > also
> > > > take care of validation.
> > > 
> > > Yes, that's why it's clever ... I'm just always wary of clever code
> > > because of the Kernighan principle.
> > > 
> > > >  I'm still going to keep tpm2_map_to_phandle because it makes the
> > > > code flow a lot cleaner and probably sessions have to anyway make
> > > > it
> > > > even more complicated.
> > > 
> > > OK, there's one more thing that seems to be causing problems: when
> > > tpm2_save_context fails because the handle no longer exists (like
> > > it's
> > > been flushed) it returns TPM_RC_REFERENCE_H0 not TPM_RC_HANDLE (the
> > > session code does seem to return TPM_RC_HANDLE under some
> > > circumstances).
> > > 
> > > James
> > 
> > What is your way for reproducing this issue? Just want to add
> > a test case for my smoke test suite so that I can verify that
> > the issue is fixed once I've fixed it.
> 
> 
> It's the test_handle_clearing test in tpm2_sessions_smoke.py.  It's
> probably easier if I publish the current state of my mods to your tpm2
> -scripts, so here they are:
> 
> http://git.kernel.org/cgit/linux/kernel/git/jejb/tpm2-scripts.git/

Thanks. I think you can use also this list to send updates to my
test scripts in future.

> James

/Jarkko

[toc] | [prev] | [next] | [standalone]

#1565139

On Mon, Jan 23, 2017 at 04:09:42PM +0200, Jarkko Sakkinen wrote:
> On Sun, Jan 22, 2017 at 01:36:28PM -0800, James Bottomley wrote:
> > On Sun, 2017-01-22 at 23:04 +0200, Jarkko Sakkinen wrote:
> > > On Sun, Jan 22, 2017 at 11:01:07PM +0200, Jarkko Sakkinen wrote:
> > > > On Sun, Jan 22, 2017 at 10:30:55PM +0200, Jarkko Sakkinen wrote:
> > > > > On Sun, Jan 22, 2017 at 10:48:12AM -0800, James Bottomley wrote:
> > > > > > On Sun, 2017-01-22 at 09:49 -0800, James Bottomley wrote:
> > > > > > > On Fri, 2017-01-20 at 23:05 +0200, Jarkko Sakkinen wrote:
> > > > > > > > 'tabrm4' branch has been now rebased. It's now on top of
> > > > > > > > master
> > > > > > > > branch that contains Stefan's latest patch (min body length
> > > > > > > > check) 
> > > > > > > > that I've reviewed and tested. It also contains your
> > > > > > > > updated
> > > > > > > > /dev/tpms patch.
> > > > > > > > 
> > > > > > > > I guess the 5 commits that are there now are such that we
> > > > > > > > have 
> > > > > > > > fairly good consensus, don't we? If so, can I add your
> > > > > > > > reviewed-by 
> > > > > > > > and tested-by to my commits and vice versa?
> > > > > > > 
> > > > > > > We're still failing my test_transients.  This is the full
> > > > > > > python of 
> > > > > > > the test case:
> > > > > > > 
> > > > > > > 
> > > > > > >     def test_transients(self):
> > > > > > >         k = self.open_transients()
> > > > > > >         self.c.flush_context(k[0])
> > > > > > >         self.c.change_auth(self.c.SRK, k[1], None, pwd1)
> > > > > > >         ...
> > > > > > > 
> > > > > > > It's failing at self.c.flush_context(k[0]) with TPM_RC_VALUE.
> > > > > > >   It's 
> > > > > > > the same problem Ken complained about: TPM2_FlushContext
> > > > > > > doesn't have 
> > > > > > > a declared handle area so we don't translate the handle being
> > > > > > > sent
> > > > > > > down.  We have to fix this either by intercepting the flush
> > > > > > > and 
> > > > > > > manually translating the context, or by being dangerously
> > > > > > > clever and 
> > > > > > > marking flush as a command which takes one handle.
> > > > > > 
> > > > > > This is what the dangerously clever fix looks like.  With this
> > > > > > and a
> > > > > > few other changes, my smoke tests now pass.
> > > > > > 
> > > > > > James
> > > > > 
> > > > > I don't want to be clever here. I will rather intercept the body
> > > > > and
> > > > > try to keep the core code simple and easy to understand.
> > > > 
> > > > It came out quite clean actually.
> > > > 
> > > > I just encapsulated handle mapping and have this in the beginning
> > > > of
> > > > tpm2_map_command:
> > > > 
> > > > if (cc == TPM2_CC_FLUSH_CONTEXT)
> > > > 	return tpm2_map_to_phandle(space, &cmd[TPM_HEADER_SIZE]);
> > > > 
> > > > I think this documents better what is actually going on than
> > > > tinkering
> > > > cc_attr_tbl.
> > > > 
> > > > /Jarkko
> > > 
> > > Actually what you suggested is much better idea because it will also
> > > take care of validation.
> > 
> > Yes, that's why it's clever ... I'm just always wary of clever code
> > because of the Kernighan principle.
> > 
> > >  I'm still going to keep tpm2_map_to_phandle because it makes the 
> > > code flow a lot cleaner and probably sessions have to anyway make it
> > > even more complicated.
> > 
> > OK, there's one more thing that seems to be causing problems: when
> > tpm2_save_context fails because the handle no longer exists (like it's
> > been flushed) it returns TPM_RC_REFERENCE_H0 not TPM_RC_HANDLE (the
> > session code does seem to return TPM_RC_HANDLE under some
> > circumstances).
> > 
> > James
> 
> What is your way for reproducing this issue? Just want to add
> a test case for my smoke test suite so that I can verify that
> the issue is fixed once I've fixed it.

Right. Too easy. Sorry about this. I'll push a fix for this to
tabrm4 branch.

/Jarkko

[toc] | [prev] | [next] | [standalone]

#1565322

On Mon, Jan 23, 2017 at 06:58:23PM +0200, Jarkko Sakkinen wrote:
> On Mon, Jan 23, 2017 at 04:09:42PM +0200, Jarkko Sakkinen wrote:
> > On Sun, Jan 22, 2017 at 01:36:28PM -0800, James Bottomley wrote:
> > > On Sun, 2017-01-22 at 23:04 +0200, Jarkko Sakkinen wrote:
> > > > On Sun, Jan 22, 2017 at 11:01:07PM +0200, Jarkko Sakkinen wrote:
> > > > > On Sun, Jan 22, 2017 at 10:30:55PM +0200, Jarkko Sakkinen wrote:
> > > > > > On Sun, Jan 22, 2017 at 10:48:12AM -0800, James Bottomley wrote:
> > > > > > > On Sun, 2017-01-22 at 09:49 -0800, James Bottomley wrote:
> > > > > > > > On Fri, 2017-01-20 at 23:05 +0200, Jarkko Sakkinen wrote:
> > > > > > > > > 'tabrm4' branch has been now rebased. It's now on top of
> > > > > > > > > master
> > > > > > > > > branch that contains Stefan's latest patch (min body length
> > > > > > > > > check) 
> > > > > > > > > that I've reviewed and tested. It also contains your
> > > > > > > > > updated
> > > > > > > > > /dev/tpms patch.
> > > > > > > > > 
> > > > > > > > > I guess the 5 commits that are there now are such that we
> > > > > > > > > have 
> > > > > > > > > fairly good consensus, don't we? If so, can I add your
> > > > > > > > > reviewed-by 
> > > > > > > > > and tested-by to my commits and vice versa?
> > > > > > > > 
> > > > > > > > We're still failing my test_transients.  This is the full
> > > > > > > > python of 
> > > > > > > > the test case:
> > > > > > > > 
> > > > > > > > 
> > > > > > > >     def test_transients(self):
> > > > > > > >         k = self.open_transients()
> > > > > > > >         self.c.flush_context(k[0])
> > > > > > > >         self.c.change_auth(self.c.SRK, k[1], None, pwd1)
> > > > > > > >         ...
> > > > > > > > 
> > > > > > > > It's failing at self.c.flush_context(k[0]) with TPM_RC_VALUE.
> > > > > > > >   It's 
> > > > > > > > the same problem Ken complained about: TPM2_FlushContext
> > > > > > > > doesn't have 
> > > > > > > > a declared handle area so we don't translate the handle being
> > > > > > > > sent
> > > > > > > > down.  We have to fix this either by intercepting the flush
> > > > > > > > and 
> > > > > > > > manually translating the context, or by being dangerously
> > > > > > > > clever and 
> > > > > > > > marking flush as a command which takes one handle.
> > > > > > > 
> > > > > > > This is what the dangerously clever fix looks like.  With this
> > > > > > > and a
> > > > > > > few other changes, my smoke tests now pass.
> > > > > > > 
> > > > > > > James
> > > > > > 
> > > > > > I don't want to be clever here. I will rather intercept the body
> > > > > > and
> > > > > > try to keep the core code simple and easy to understand.
> > > > > 
> > > > > It came out quite clean actually.
> > > > > 
> > > > > I just encapsulated handle mapping and have this in the beginning
> > > > > of
> > > > > tpm2_map_command:
> > > > > 
> > > > > if (cc == TPM2_CC_FLUSH_CONTEXT)
> > > > > 	return tpm2_map_to_phandle(space, &cmd[TPM_HEADER_SIZE]);
> > > > > 
> > > > > I think this documents better what is actually going on than
> > > > > tinkering
> > > > > cc_attr_tbl.
> > > > > 
> > > > > /Jarkko
> > > > 
> > > > Actually what you suggested is much better idea because it will also
> > > > take care of validation.
> > > 
> > > Yes, that's why it's clever ... I'm just always wary of clever code
> > > because of the Kernighan principle.
> > > 
> > > >  I'm still going to keep tpm2_map_to_phandle because it makes the 
> > > > code flow a lot cleaner and probably sessions have to anyway make it
> > > > even more complicated.
> > > 
> > > OK, there's one more thing that seems to be causing problems: when
> > > tpm2_save_context fails because the handle no longer exists (like it's
> > > been flushed) it returns TPM_RC_REFERENCE_H0 not TPM_RC_HANDLE (the
> > > session code does seem to return TPM_RC_HANDLE under some
> > > circumstances).
> > > 
> > > James
> > 
> > What is your way for reproducing this issue? Just want to add
> > a test case for my smoke test suite so that I can verify that
> > the issue is fixed once I've fixed it.
> 
> Right. Too easy. Sorry about this. I'll push a fix for this to
> tabrm4 branch.

1. I pushed a fix to the repository.
2. My smoke test suite has a test case for flushing a context now.

/Jarkko

[toc] | [prev] | [next] | [standalone]

#1565331

On Mon, 2017-01-23 at 23:42 +0200, Jarkko Sakkinen wrote:
> On Mon, Jan 23, 2017 at 06:58:23PM +0200, Jarkko Sakkinen wrote:
> > On Mon, Jan 23, 2017 at 04:09:42PM +0200, Jarkko Sakkinen wrote:
> > > On Sun, Jan 22, 2017 at 01:36:28PM -0800, James Bottomley wrote:
> > > > On Sun, 2017-01-22 at 23:04 +0200, Jarkko Sakkinen wrote:
> > > > > On Sun, Jan 22, 2017 at 11:01:07PM +0200, Jarkko Sakkinen
> > > > > wrote:
> > > > > > On Sun, Jan 22, 2017 at 10:30:55PM +0200, Jarkko Sakkinen
> > > > > > wrote:
> > > > > > > On Sun, Jan 22, 2017 at 10:48:12AM -0800, James Bottomley
> > > > > > > wrote:
> > > > > > > > On Sun, 2017-01-22 at 09:49 -0800, James Bottomley
> > > > > > > > wrote:
> > > > > > > > > On Fri, 2017-01-20 at 23:05 +0200, Jarkko Sakkinen
> > > > > > > > > wrote:
> > > > > > > > > > 'tabrm4' branch has been now rebased. It's now on
> > > > > > > > > > top of
> > > > > > > > > > master
> > > > > > > > > > branch that contains Stefan's latest patch (min
> > > > > > > > > > body length
> > > > > > > > > > check) 
> > > > > > > > > > that I've reviewed and tested. It also contains
> > > > > > > > > > your
> > > > > > > > > > updated
> > > > > > > > > > /dev/tpms patch.
> > > > > > > > > > 
> > > > > > > > > > I guess the 5 commits that are there now are such
> > > > > > > > > > that we
> > > > > > > > > > have 
> > > > > > > > > > fairly good consensus, don't we? If so, can I add
> > > > > > > > > > your
> > > > > > > > > > reviewed-by 
> > > > > > > > > > and tested-by to my commits and vice versa?
> > > > > > > > > 
> > > > > > > > > We're still failing my test_transients.  This is the
> > > > > > > > > full
> > > > > > > > > python of 
> > > > > > > > > the test case:
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > >     def test_transients(self):
> > > > > > > > >         k = self.open_transients()
> > > > > > > > >         self.c.flush_context(k[0])
> > > > > > > > >         self.c.change_auth(self.c.SRK, k[1], None,
> > > > > > > > > pwd1)
> > > > > > > > >         ...
> > > > > > > > > 
> > > > > > > > > It's failing at self.c.flush_context(k[0]) with
> > > > > > > > > TPM_RC_VALUE.
> > > > > > > > >   It's 
> > > > > > > > > the same problem Ken complained about:
> > > > > > > > > TPM2_FlushContext
> > > > > > > > > doesn't have 
> > > > > > > > > a declared handle area so we don't translate the
> > > > > > > > > handle being
> > > > > > > > > sent
> > > > > > > > > down.  We have to fix this either by intercepting the
> > > > > > > > > flush
> > > > > > > > > and 
> > > > > > > > > manually translating the context, or by being
> > > > > > > > > dangerously
> > > > > > > > > clever and 
> > > > > > > > > marking flush as a command which takes one handle.
> > > > > > > > 
> > > > > > > > This is what the dangerously clever fix looks like. 
> > > > > > > >  With this
> > > > > > > > and a
> > > > > > > > few other changes, my smoke tests now pass.
> > > > > > > > 
> > > > > > > > James
> > > > > > > 
> > > > > > > I don't want to be clever here. I will rather intercept
> > > > > > > the body
> > > > > > > and
> > > > > > > try to keep the core code simple and easy to understand.
> > > > > > 
> > > > > > It came out quite clean actually.
> > > > > > 
> > > > > > I just encapsulated handle mapping and have this in the
> > > > > > beginning
> > > > > > of
> > > > > > tpm2_map_command:
> > > > > > 
> > > > > > if (cc == TPM2_CC_FLUSH_CONTEXT)
> > > > > > 	return tpm2_map_to_phandle(space,
> > > > > > &cmd[TPM_HEADER_SIZE]);
> > > > > > 
> > > > > > I think this documents better what is actually going on
> > > > > > than
> > > > > > tinkering
> > > > > > cc_attr_tbl.
> > > > > > 
> > > > > > /Jarkko
> > > > > 
> > > > > Actually what you suggested is much better idea because it
> > > > > will also
> > > > > take care of validation.
> > > > 
> > > > Yes, that's why it's clever ... I'm just always wary of clever
> > > > code
> > > > because of the Kernighan principle.
> > > > 
> > > > >  I'm still going to keep tpm2_map_to_phandle because it makes
> > > > > the 
> > > > > code flow a lot cleaner and probably sessions have to anyway
> > > > > make it
> > > > > even more complicated.
> > > > 
> > > > OK, there's one more thing that seems to be causing problems:
> > > > when
> > > > tpm2_save_context fails because the handle no longer exists
> > > > (like it's
> > > > been flushed) it returns TPM_RC_REFERENCE_H0 not TPM_RC_HANDLE
> > > > (the
> > > > session code does seem to return TPM_RC_HANDLE under some
> > > > circumstances).
> > > > 
> > > > James
> > > 
> > > What is your way for reproducing this issue? Just want to add
> > > a test case for my smoke test suite so that I can verify that
> > > the issue is fixed once I've fixed it.
> > 
> > Right. Too easy. Sorry about this. I'll push a fix for this to
> > tabrm4 branch.
> 
> 1. I pushed a fix to the repository.

I don't think the fix is right; this is what you now have

	} else if ((rc & TPM2_RC_REFERENCE_H0) == TPM2_RC_REFERENCE_H0)
{

That should be

} else if (rc == TPM2_RC_REFERENCE_H0)

because the 0x9XX return codes don't have any parameter information
that needs stripping and (rc & TPM2_RC_REFERENCE_H0) ==
TPM2_RC_REFERENCE_H0)  will match any error code that has bits 11, 8
and 5 set.

I think the handle check was wrong too, it should have been

if (rc & 0xff) == TPM2_RC_HANDLE)

Because all you need to do is strip off the parameter information

James

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web