From patchwork Fri Jan 27 00:29:02 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: James Bottomley X-Patchwork-Id: 720441 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.sourceforge.net (lists.sourceforge.net [216.34.181.88]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 3v8fmd1vYWz9t0J for ; Fri, 27 Jan 2017 11:29:21 +1100 (AEDT) Received: from localhost ([127.0.0.1] helo=sfs-ml-2.v29.ch3.sourceforge.com) by sfs-ml-2.v29.ch3.sourceforge.com with esmtp (Exim 4.76) (envelope-from ) id 1cWuPi-0000xA-2F; Fri, 27 Jan 2017 00:29:14 +0000 Received: from sog-mx-4.v43.ch3.sourceforge.com ([172.29.43.194] helo=mx.sourceforge.net) by sfs-ml-2.v29.ch3.sourceforge.com with esmtp (Exim 4.76) (envelope-from ) id 1cWuPg-0000x2-UB for tpmdd-devel@lists.sourceforge.net; Fri, 27 Jan 2017 00:29:13 +0000 Received-SPF: pass (sog-mx-4.v43.ch3.sourceforge.com: domain of HansenPartnership.com designates 66.63.167.143 as permitted sender) client-ip=66.63.167.143; envelope-from=James.Bottomley@HansenPartnership.com; helo=bedivere.hansenpartnership.com; Received: from bedivere.hansenpartnership.com ([66.63.167.143]) by sog-mx-4.v43.ch3.sourceforge.com with esmtps (TLSv1:AES256-SHA:256) (Exim 4.76) id 1cWuPe-000507-Su for tpmdd-devel@lists.sourceforge.net; Fri, 27 Jan 2017 00:29:12 +0000 Received: from localhost (localhost [127.0.0.1]) by bedivere.hansenpartnership.com (Postfix) with ESMTP id BB53D8EE0E7; Thu, 26 Jan 2017 16:29:04 -0800 (PST) Received: from bedivere.hansenpartnership.com ([127.0.0.1]) by localhost (bedivere.hansenpartnership.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id P71qy7zNn4T4; Thu, 26 Jan 2017 16:29:04 -0800 (PST) Received: from [153.66.254.194] (unknown [50.46.144.141]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by bedivere.hansenpartnership.com (Postfix) with ESMTPSA id EA65F8EE0CD; Thu, 26 Jan 2017 16:29:03 -0800 (PST) Message-ID: <1485476942.2457.45.camel@HansenPartnership.com> From: James Bottomley To: Jarkko Sakkinen Date: Thu, 26 Jan 2017 16:29:02 -0800 In-Reply-To: <20170125134004.g77ezmfkkhya643y@intel.com> References: <1485107342.2504.7.camel@HansenPartnership.com> <1485110892.2504.12.camel@HansenPartnership.com> <20170122203055.zg52yraahqvn4rbv@intel.com> <20170122210107.zwrvuhnelrvaoe5h@intel.com> <20170122210441.ivzujugb3urzndhj@intel.com> <1485120988.2504.21.camel@HansenPartnership.com> <20170123140942.svnurymdxsyo6cum@intel.com> <20170123165823.hbgwphsqqpn6jwiz@intel.com> <20170123214255.vt2dumjm6jo5oxd3@intel.com> <1485209797.2534.29.camel@HansenPartnership.com> <20170125134004.g77ezmfkkhya643y@intel.com> X-Mailer: Evolution 3.16.5 Mime-Version: 1.0 X-Spam-Score: -4.8 (----) X-Spam-Report: Spam Filtering performed by mx.sourceforge.net. See http://spamassassin.org/tag/ for more details. -1.5 SPF_CHECK_PASS SPF reports sender host as permitted sender for sender-domain -0.0 SPF_PASS SPF: sender matches SPF record -3.2 RP_MATCHES_RCVD Envelope sender domain matches handover relay domain -0.1 DKIM_VALID_AU Message has a valid DKIM or DK signature from author's domain 0.1 DKIM_SIGNED Message has a DKIM or DK signature, not necessarily valid -0.1 DKIM_VALID Message has at least one valid DKIM or DK signature X-Headers-End: 1cWuPe-000507-Su Cc: linux-security-module@vger.kernel.org, tpmdd-devel@lists.sourceforge.net, open list Subject: Re: [tpmdd-devel] [PATCH RFC v3 5/5] tpm2: expose resource manager via a device link /dev/tpms X-BeenThere: tpmdd-devel@lists.sourceforge.net X-Mailman-Version: 2.1.9 Precedence: list List-Id: Tpm Device Driver maintainance List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: tpmdd-devel-bounces@lists.sourceforge.net On Wed, 2017-01-25 at 15:40 +0200, Jarkko Sakkinen wrote: > On Mon, Jan 23, 2017 at 02:16:37PM -0800, James Bottomley wrote: > > 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) > > Right I see your point. > > And yes, also for RC_HANDLE the error handling was done incorrectly. > It should be masked like you said with 0xff to catch error number > and the F flag (bit 7). > > As it is format zero error code it should be fine to check without > any mask. > > Thanks for noting this. It is easy to shoot yourself into foot when > there's lot of stuff packed :-) OK, I've rebased and retested to tabrm5. Apart from the obvious introduced bug into the last patch (fix below if you need it) everything works, so you can add my tested and reviewed bys. James --- commit b63aa4b3a5dce31cbc874fa32bd7252b62f55813 Author: James Bottomley Date: Thu Jan 26 08:43:55 2017 -0800 fix compile failure ------------------------------------------------------------------------------ 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 5030f8c..5720885 100644 --- a/drivers/char/tpm/tpms-dev.c +++ b/drivers/char/tpm/tpms-dev.c @@ -39,7 +39,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); - tpm2_del_space(&priv->space): + tpm2_del_space(&priv->space); kfree(priv); return 0;