| Message ID | 1484751663.2717.10.camel@HansenPartnership.com |
|---|---|
| State | New |
| Headers | show |
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; > ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
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 ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
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 ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
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 ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
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 ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
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 ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
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;