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

Message ID 1484751663.2717.10.camel@HansenPartnership.com
State New
Headers show

Commit Message

James Bottomley Jan. 18, 2017, 3:01 p.m. UTC
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


--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Jarkko Sakkinen Jan. 19, 2017, 10:49 a.m. UTC | #1
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;
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
James Bottomley Jan. 19, 2017, 12:19 p.m. UTC | #2
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
Jarkko Sakkinen Jan. 20, 2017, 1:39 p.m. UTC | #3
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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jarkko Sakkinen Jan. 20, 2017, 9:05 p.m. UTC | #4
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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
James Bottomley Jan. 22, 2017, 5:49 p.m. UTC | #5
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

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jarkko Sakkinen Jan. 22, 2017, 8:24 p.m. UTC | #6
On Sun, Jan 22, 2017 at 09:49:02AM -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.

I'll add interception of flush to the next patch set version.

/Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

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;