| Message ID | 1485110892.2504.12.camel@HansenPartnership.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 -- 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
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 -- 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
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 -- 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
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 -- 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
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 -- 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
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 -- 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
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 -- 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
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 -- 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
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 -- 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
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 -- 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
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 :-) /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
On Wed, Jan 25, 2017 at 03:40:04PM +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 :-) > > /Jarkko I think I actually add a helper to tpm.h to get the error number. /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
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) > > 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 Can you check if it is now what you would expect? There's now a helper called tpm2_rc_value(). I also created tabrm5 branch where everything is nicely squashed together... /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
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);