[PATCH v4 0/2] selftests: tpm2: Determine available PCR bank

[PATCH v4 0/2] selftests: tpm2: Determine available PCR bank

[Stefan Berger]

From: Stefan Berger <stefanb@linux.ibm.com>

This series of patches fixes two issues with TPM2 selftest.
- Determines available PCR banks for use by test cases
- Resets DA lock on TPM2 to avoid subsequent test failures

  Stefan

v4:
 - Switch to query TPM2_GET_CAP to determine the available PCR banks
 - Moved call to reset DA lock into finally branch at end of test
 - Dropped patch 3

v3:
 - Mention SHA-256 PCR bank as alternative in patch 1 description

v2:
 - Clarified patch 1 description 
 - Added patch 3 with support for SHA-384 and SHA-512



Stefan Berger (2):
  selftests: tpm2: Determine available PCR bank
  selftests: tpm2: Reset the dictionary attack lock

 tools/testing/selftests/tpm2/tpm2.py       | 31 ++++++++++++++++++++++
 tools/testing/selftests/tpm2/tpm2_tests.py | 31 ++++++++++++++++------
 2 files changed, 54 insertions(+), 8 deletions(-)

-- 
2.31.1

[PATCH v4 1/2] selftests: tpm2: Determine available PCR bank

[Stefan Berger]

From: Stefan Berger <stefanb@linux.ibm.com>

Determine an available PCR bank to be used by a test case by querying the
capability TPM2_GET_CAP. The TPM2 returns TPML_PCR_SELECTIONS that
contains an array of TPMS_PCR_SELECTIONs indicating available PCR banks
and the bitmasks that show which PCRs are enabled in each bank. Collect
the data in a dictionary. From the dictionary determine the PCR bank that
has the PCRs enabled that the test needs. This avoids test failures with
TPM2's that either to not have a SHA-1 bank or whose SHA-1 bank is
disabled.

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
---
 tools/testing/selftests/tpm2/tpm2.py       | 31 ++++++++++++++++++++++
 tools/testing/selftests/tpm2/tpm2_tests.py | 29 ++++++++++++++------
 2 files changed, 52 insertions(+), 8 deletions(-)

diff --git a/tools/testing/selftests/tpm2/tpm2.py b/tools/testing/selftests/tpm2/tpm2.py
index f34486cd7342..057a4f49c79d 100644
--- a/tools/testing/selftests/tpm2/tpm2.py
+++ b/tools/testing/selftests/tpm2/tpm2.py
@@ -56,6 +56,7 @@ TSS2_RESMGR_TPM_RC_LAYER = (11 << TSS2_RC_LAYER_SHIFT)
 
 TPM2_CAP_HANDLES = 0x00000001
 TPM2_CAP_COMMANDS = 0x00000002
+TPM2_CAP_PCRS = 0x00000005
 TPM2_CAP_TPM_PROPERTIES = 0x00000006
 
 TPM2_PT_FIXED = 0x100
@@ -712,3 +713,33 @@ class Client:
             pt += 1
 
         return handles
+
+    def get_cap_pcrs(self):
+        pcr_banks = {}
+
+        fmt = '>HII III'
+
+        cmd = struct.pack(fmt,
+                          TPM2_ST_NO_SESSIONS,
+                          struct.calcsize(fmt),
+                          TPM2_CC_GET_CAPABILITY,
+                          TPM2_CAP_PCRS, 0, 1)
+        rsp = self.send_cmd(cmd)[10:]
+        _, _, cnt = struct.unpack('>BII', rsp[:9])
+        rsp = rsp[9:]
+
+        # items are TPMS_PCR_SELECTION's
+        for i in range(0, cnt):
+              hash, sizeOfSelect = struct.unpack('>HB', rsp[:3])
+              rsp = rsp[3:]
+
+              pcrSelect = 0
+              if sizeOfSelect > 0:
+                  pcrSelect, = struct.unpack('%ds' % sizeOfSelect,
+                                             rsp[:sizeOfSelect])
+                  rsp = rsp[sizeOfSelect:]
+                  pcrSelect = int.from_bytes(pcrSelect, byteorder='big')
+
+              pcr_banks[hash] = pcrSelect
+
+        return pcr_banks
diff --git a/tools/testing/selftests/tpm2/tpm2_tests.py b/tools/testing/selftests/tpm2/tpm2_tests.py
index 9d764306887b..e63a37819978 100644
--- a/tools/testing/selftests/tpm2/tpm2_tests.py
+++ b/tools/testing/selftests/tpm2/tpm2_tests.py
@@ -27,7 +27,17 @@ class SmokeTest(unittest.TestCase):
         result = self.client.unseal(self.root_key, blob, auth, None)
         self.assertEqual(data, result)
 
+    def determine_bank_alg(self, mask):
+        pcr_banks = self.client.get_cap_pcrs()
+        for bank_alg, pcrSelection in pcr_banks.items():
+            if pcrSelection & mask == mask:
+                return bank_alg
+        return None
+
     def test_seal_with_policy(self):
+        bank_alg = self.determine_bank_alg(1 << 16)
+        self.assertIsNotNone(bank_alg)
+
         handle = self.client.start_auth_session(tpm2.TPM2_SE_TRIAL)
 
         data = ('X' * 64).encode()
@@ -35,7 +45,7 @@ class SmokeTest(unittest.TestCase):
         pcrs = [16]
 
         try:
-            self.client.policy_pcr(handle, pcrs)
+            self.client.policy_pcr(handle, pcrs, bank_alg=bank_alg)
             self.client.policy_password(handle)
 
             policy_dig = self.client.get_policy_digest(handle)
@@ -47,7 +57,7 @@ class SmokeTest(unittest.TestCase):
         handle = self.client.start_auth_session(tpm2.TPM2_SE_POLICY)
 
         try:
-            self.client.policy_pcr(handle, pcrs)
+            self.client.policy_pcr(handle, pcrs, bank_alg=bank_alg)
             self.client.policy_password(handle)
 
             result = self.client.unseal(self.root_key, blob, auth, handle)
@@ -72,6 +82,9 @@ class SmokeTest(unittest.TestCase):
         self.assertEqual(rc, tpm2.TPM2_RC_AUTH_FAIL)
 
     def test_unseal_with_wrong_policy(self):
+        bank_alg = self.determine_bank_alg(1 << 16 | 1 << 1)
+        self.assertIsNotNone(bank_alg)
+
         handle = self.client.start_auth_session(tpm2.TPM2_SE_TRIAL)
 
         data = ('X' * 64).encode()
@@ -79,7 +92,7 @@ class SmokeTest(unittest.TestCase):
         pcrs = [16]
 
         try:
-            self.client.policy_pcr(handle, pcrs)
+            self.client.policy_pcr(handle, pcrs, bank_alg=bank_alg)
             self.client.policy_password(handle)
 
             policy_dig = self.client.get_policy_digest(handle)
@@ -91,13 +104,13 @@ class SmokeTest(unittest.TestCase):
         # Extend first a PCR that is not part of the policy and try to unseal.
         # This should succeed.
 
-        ds = tpm2.get_digest_size(tpm2.TPM2_ALG_SHA1)
-        self.client.extend_pcr(1, ('X' * ds).encode())
+        ds = tpm2.get_digest_size(bank_alg)
+        self.client.extend_pcr(1, ('X' * ds).encode(), bank_alg=bank_alg)
 
         handle = self.client.start_auth_session(tpm2.TPM2_SE_POLICY)
 
         try:
-            self.client.policy_pcr(handle, pcrs)
+            self.client.policy_pcr(handle, pcrs, bank_alg=bank_alg)
             self.client.policy_password(handle)
 
             result = self.client.unseal(self.root_key, blob, auth, handle)
@@ -109,14 +122,14 @@ class SmokeTest(unittest.TestCase):
 
         # Then, extend a PCR that is part of the policy and try to unseal.
         # This should fail.
-        self.client.extend_pcr(16, ('X' * ds).encode())
+        self.client.extend_pcr(16, ('X' * ds).encode(), bank_alg=bank_alg)
 
         handle = self.client.start_auth_session(tpm2.TPM2_SE_POLICY)
 
         rc = 0
 
         try:
-            self.client.policy_pcr(handle, pcrs)
+            self.client.policy_pcr(handle, pcrs, bank_alg=bank_alg)
             self.client.policy_password(handle)
 
             result = self.client.unseal(self.root_key, blob, auth, handle)
-- 
2.31.1

Re: [PATCH v4 1/2] selftests: tpm2: Determine available PCR bank

[Jarkko Sakkinen]

On Sat, Nov 27, 2021 at 11:10:51PM -0500, Stefan Berger wrote:
> From: Stefan Berger <stefanb@linux.ibm.com>
> 
> Determine an available PCR bank to be used by a test case by querying the
> capability TPM2_GET_CAP. The TPM2 returns TPML_PCR_SELECTIONS that
> contains an array of TPMS_PCR_SELECTIONs indicating available PCR banks
> and the bitmasks that show which PCRs are enabled in each bank. Collect
> the data in a dictionary. From the dictionary determine the PCR bank that
> has the PCRs enabled that the test needs. This avoids test failures with
> TPM2's that either to not have a SHA-1 bank or whose SHA-1 bank is
> disabled.
> 
> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>

Acked-by: Jarkko Sakkinen <jarkko@kernel.org>

/Jarkko

Re: [PATCH v4 1/2] selftests: tpm2: Determine available PCR bank

[Stefan Berger]

Shuah,

   are you going to take this fix here - only 1/2 ?

https://lore.kernel.org/lkml/20211128041052.1395504-1-stefanb@linux.vnet.ibm.com/T/#m21209a978c237368499ce5f082f3c0fc03bcbbeb

   Stefan

On 11/29/21 18:39, Jarkko Sakkinen wrote:
> On Sat, Nov 27, 2021 at 11:10:51PM -0500, Stefan Berger wrote:
>> From: Stefan Berger <stefanb@linux.ibm.com>
>>
>> Determine an available PCR bank to be used by a test case by querying the
>> capability TPM2_GET_CAP. The TPM2 returns TPML_PCR_SELECTIONS that
>> contains an array of TPMS_PCR_SELECTIONs indicating available PCR banks
>> and the bitmasks that show which PCRs are enabled in each bank. Collect
>> the data in a dictionary. From the dictionary determine the PCR bank that
>> has the PCRs enabled that the test needs. This avoids test failures with
>> TPM2's that either to not have a SHA-1 bank or whose SHA-1 bank is
>> disabled.
>>
>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> Acked-by: Jarkko Sakkinen <jarkko@kernel.org>
>
> /Jarkko

Re: [PATCH v4 1/2] selftests: tpm2: Determine available PCR bank

[Stefan Berger]

Jarkko,

   can you take this patch 1/2?

  https://lore.kernel.org/lkml/20211128041052.1395504-1-stefanb@linux.vnet.ibm.com/T/#m21209a978c237368499ce5f082f3c0fc03bcbbeb

   Stefan


On 12/23/21 20:12, Stefan Berger wrote:
> Shuah,
>
>   are you going to take this fix here - only 1/2 ?
>
> https://lore.kernel.org/lkml/20211128041052.1395504-1-stefanb@linux.vnet.ibm.com/T/#m21209a978c237368499ce5f082f3c0fc03bcbbeb 
>
>
>   Stefan
>
> On 11/29/21 18:39, Jarkko Sakkinen wrote:
>> On Sat, Nov 27, 2021 at 11:10:51PM -0500, Stefan Berger wrote:
>>> From: Stefan Berger <stefanb@linux.ibm.com>
>>>
>>> Determine an available PCR bank to be used by a test case by 
>>> querying the
>>> capability TPM2_GET_CAP. The TPM2 returns TPML_PCR_SELECTIONS that
>>> contains an array of TPMS_PCR_SELECTIONs indicating available PCR banks
>>> and the bitmasks that show which PCRs are enabled in each bank. Collect
>>> the data in a dictionary. From the dictionary determine the PCR bank 
>>> that
>>> has the PCRs enabled that the test needs. This avoids test failures 
>>> with
>>> TPM2's that either to not have a SHA-1 bank or whose SHA-1 bank is
>>> disabled.
>>>
>>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
>> Acked-by: Jarkko Sakkinen <jarkko@kernel.org>
>>
>> /Jarkko

Re: [PATCH v4 1/2] selftests: tpm2: Determine available PCR bank

[Jarkko Sakkinen]

On Thu, Jan 13, 2022 at 01:04:03PM -0500, Stefan Berger wrote:
> Jarkko,
> 
>   can you take this patch 1/2?
> 
>  https://lore.kernel.org/lkml/20211128041052.1395504-1-stefanb@linux.vnet.ibm.com/T/#m21209a978c237368499ce5f082f3c0fc03bcbbeb
> 
>   Stefan

Oops. Sorry, I missed your request at 23rd.

Yes, we can for sure take that. I now tested by with SHA256 only
configuration so:

Tested-by: Jarkko Sakkinen <jarkko@kernel.org>

I'm considering 5.17-rc2 pull rquest but want to leave the final
decision to the time when it can be sent. If I'll make rc2 PR in
the first place, I'll include this to the pull request.

/Jarkko

Re: [PATCH v4 1/2] selftests: tpm2: Determine available PCR bank

[Jarkko Sakkinen]

On Sat, Jan 15, 2022 at 05:53:18PM +0200, Jarkko Sakkinen wrote:
> On Thu, Jan 13, 2022 at 01:04:03PM -0500, Stefan Berger wrote:
> > Jarkko,
> > 
> >   can you take this patch 1/2?
> > 
> >  https://lore.kernel.org/lkml/20211128041052.1395504-1-stefanb@linux.vnet.ibm.com/T/#m21209a978c237368499ce5f082f3c0fc03bcbbeb
> > 
> >   Stefan
> 
> Oops. Sorry, I missed your request at 23rd.
> 
> Yes, we can for sure take that. I now tested by with SHA256 only
> configuration so:
> 
> Tested-by: Jarkko Sakkinen <jarkko@kernel.org>
> 
> I'm considering 5.17-rc2 pull rquest but want to leave the final
> decision to the time when it can be sent. If I'll make rc2 PR in
> the first place, I'll include this to the pull request.

OK, it's now applied, thank you.

BR, Jarkko

[PATCH v4 2/2] selftests: tpm2: Reset the dictionary attack lock

[Stefan Berger]

From: Stefan Berger <stefanb@linux.ibm.com>

Reset the dictionary attack lock to avoid the following types of test
failures after running the test 2 times:

======================================================================
ERROR: test_unseal_with_wrong_policy (tpm2_tests.SmokeTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/root/linux-ima-namespaces/tools/testing/selftests/tpm2/tpm2_tests.py", line 105, in test_unseal_with_wrong_policy
    blob = self.client.seal(self.root_key, data, auth, policy_dig)
  File "/root/linux-ima-namespaces/tools/testing/selftests/tpm2/tpm2.py", line 620, in seal
    rsp = self.send_cmd(cmd)
  File "/root/linux-ima-namespaces/tools/testing/selftests/tpm2/tpm2.py", line 397, in send_cmd
    raise ProtocolError(cc, rc)
tpm2.ProtocolError: TPM_RC_LOCKOUT: cc=0x00000153, rc=0x00000921

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
---
 tools/testing/selftests/tpm2/tpm2_tests.py | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/testing/selftests/tpm2/tpm2_tests.py b/tools/testing/selftests/tpm2/tpm2_tests.py
index e63a37819978..ad6f54c01adf 100644
--- a/tools/testing/selftests/tpm2/tpm2_tests.py
+++ b/tools/testing/selftests/tpm2/tpm2_tests.py
@@ -139,6 +139,8 @@ class SmokeTest(unittest.TestCase):
         except:
             self.client.flush_context(handle)
             raise
+        finally:
+            self.client.reset_da_lock()
 
         self.assertEqual(rc, tpm2.TPM2_RC_POLICY_FAIL)
 
-- 
2.31.1

Re: [PATCH v4 2/2] selftests: tpm2: Reset the dictionary attack lock

[Jarkko Sakkinen]

On Sat, Nov 27, 2021 at 11:10:52PM -0500, Stefan Berger wrote:
> From: Stefan Berger <stefanb@linux.ibm.com>
> 
> Reset the dictionary attack lock to avoid the following types of test
> failures after running the test 2 times:
> 
> ======================================================================
> ERROR: test_unseal_with_wrong_policy (tpm2_tests.SmokeTest)
> ----------------------------------------------------------------------
> Traceback (most recent call last):
>   File "/root/linux-ima-namespaces/tools/testing/selftests/tpm2/tpm2_tests.py", line 105, in test_unseal_with_wrong_policy
>     blob = self.client.seal(self.root_key, data, auth, policy_dig)
>   File "/root/linux-ima-namespaces/tools/testing/selftests/tpm2/tpm2.py", line 620, in seal
>     rsp = self.send_cmd(cmd)
>   File "/root/linux-ima-namespaces/tools/testing/selftests/tpm2/tpm2.py", line 397, in send_cmd
>     raise ProtocolError(cc, rc)
> tpm2.ProtocolError: TPM_RC_LOCKOUT: cc=0x00000153, rc=0x00000921
> 
> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> ---
>  tools/testing/selftests/tpm2/tpm2_tests.py | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/tools/testing/selftests/tpm2/tpm2_tests.py b/tools/testing/selftests/tpm2/tpm2_tests.py
> index e63a37819978..ad6f54c01adf 100644
> --- a/tools/testing/selftests/tpm2/tpm2_tests.py
> +++ b/tools/testing/selftests/tpm2/tpm2_tests.py
> @@ -139,6 +139,8 @@ class SmokeTest(unittest.TestCase):
>          except:
>              self.client.flush_context(handle)
>              raise
> +        finally:
> +            self.client.reset_da_lock()
>  
>          self.assertEqual(rc, tpm2.TPM2_RC_POLICY_FAIL)
>  
> -- 
> 2.31.1
> 

I don't agree with this as a DA lock has legit use. This would be adequate
for systems dedicated for kernel testing only.

We could make this available in the folder where TPM2 tests are:

https://git.kernel.org/pub/scm/linux/kernel/git/jarkko/tpm2-scripts.git/tree/tpm2-reset-da-lock

/Jarkko

Re: [PATCH v4 2/2] selftests: tpm2: Reset the dictionary attack lock

[Stefan Berger]

On 11/29/21 18:43, Jarkko Sakkinen wrote:
> On Sat, Nov 27, 2021 at 11:10:52PM -0500, Stefan Berger wrote:
>> From: Stefan Berger <stefanb@linux.ibm.com>
>>
>> Reset the dictionary attack lock to avoid the following types of test
>> failures after running the test 2 times:
>>
>> ======================================================================
>> ERROR: test_unseal_with_wrong_policy (tpm2_tests.SmokeTest)
>> ----------------------------------------------------------------------
>> Traceback (most recent call last):
>>    File "/root/linux-ima-namespaces/tools/testing/selftests/tpm2/tpm2_tests.py", line 105, in test_unseal_with_wrong_policy
>>      blob = self.client.seal(self.root_key, data, auth, policy_dig)
>>    File "/root/linux-ima-namespaces/tools/testing/selftests/tpm2/tpm2.py", line 620, in seal
>>      rsp = self.send_cmd(cmd)
>>    File "/root/linux-ima-namespaces/tools/testing/selftests/tpm2/tpm2.py", line 397, in send_cmd
>>      raise ProtocolError(cc, rc)
>> tpm2.ProtocolError: TPM_RC_LOCKOUT: cc=0x00000153, rc=0x00000921
>>
>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
>> ---
>>   tools/testing/selftests/tpm2/tpm2_tests.py | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/tools/testing/selftests/tpm2/tpm2_tests.py b/tools/testing/selftests/tpm2/tpm2_tests.py
>> index e63a37819978..ad6f54c01adf 100644
>> --- a/tools/testing/selftests/tpm2/tpm2_tests.py
>> +++ b/tools/testing/selftests/tpm2/tpm2_tests.py
>> @@ -139,6 +139,8 @@ class SmokeTest(unittest.TestCase):
>>           except:
>>               self.client.flush_context(handle)
>>               raise
>> +        finally:
>> +            self.client.reset_da_lock()
>>   
>>           self.assertEqual(rc, tpm2.TPM2_RC_POLICY_FAIL)
>>   
>> -- 
>> 2.31.1
>>
> I don't agree with this as a DA lock has legit use. This would be adequate
> for systems dedicated for kernel testing only.

The problem is this particular test case I am patching here causes the 
above test failures upon rerun. We are testing the driver here 
presumably and not the TPM2, so I think we should leave the TPM2 as 
cleaned up as possible, thus my suggestion is to reset the DA lock and 
we won't hear any complaints after that.


> We could make this available in the folder where TPM2 tests are:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/jarkko/tpm2-scripts.git/tree/tpm2-reset-da-lock


The tss packages also have command line tools to reset the DA lock, but 
it shouldn't be necessary to use them after running a **driver** test case.


    stefan


>
> /Jarkko

Re: [PATCH v4 2/2] selftests: tpm2: Reset the dictionary attack lock

[Jarkko Sakkinen]

On Mon, Nov 29, 2021 at 07:26:12PM -0500, Stefan Berger wrote:
> 
> On 11/29/21 18:43, Jarkko Sakkinen wrote:
> > On Sat, Nov 27, 2021 at 11:10:52PM -0500, Stefan Berger wrote:
> > > From: Stefan Berger <stefanb@linux.ibm.com>
> > > 
> > > Reset the dictionary attack lock to avoid the following types of test
> > > failures after running the test 2 times:
> > > 
> > > ======================================================================
> > > ERROR: test_unseal_with_wrong_policy (tpm2_tests.SmokeTest)
> > > ----------------------------------------------------------------------
> > > Traceback (most recent call last):
> > >    File "/root/linux-ima-namespaces/tools/testing/selftests/tpm2/tpm2_tests.py", line 105, in test_unseal_with_wrong_policy
> > >      blob = self.client.seal(self.root_key, data, auth, policy_dig)
> > >    File "/root/linux-ima-namespaces/tools/testing/selftests/tpm2/tpm2.py", line 620, in seal
> > >      rsp = self.send_cmd(cmd)
> > >    File "/root/linux-ima-namespaces/tools/testing/selftests/tpm2/tpm2.py", line 397, in send_cmd
> > >      raise ProtocolError(cc, rc)
> > > tpm2.ProtocolError: TPM_RC_LOCKOUT: cc=0x00000153, rc=0x00000921
> > > 
> > > Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> > > ---
> > >   tools/testing/selftests/tpm2/tpm2_tests.py | 2 ++
> > >   1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/tools/testing/selftests/tpm2/tpm2_tests.py b/tools/testing/selftests/tpm2/tpm2_tests.py
> > > index e63a37819978..ad6f54c01adf 100644
> > > --- a/tools/testing/selftests/tpm2/tpm2_tests.py
> > > +++ b/tools/testing/selftests/tpm2/tpm2_tests.py
> > > @@ -139,6 +139,8 @@ class SmokeTest(unittest.TestCase):
> > >           except:
> > >               self.client.flush_context(handle)
> > >               raise
> > > +        finally:
> > > +            self.client.reset_da_lock()
> > >           self.assertEqual(rc, tpm2.TPM2_RC_POLICY_FAIL)
> > > -- 
> > > 2.31.1
> > > 
> > I don't agree with this as a DA lock has legit use. This would be adequate
> > for systems dedicated for kernel testing only.
> 
> The problem is this particular test case I am patching here causes the above
> test failures upon rerun. We are testing the driver here presumably and not
> the TPM2, so I think we should leave the TPM2 as cleaned up as possible,
> thus my suggestion is to reset the DA lock and we won't hear any complaints
> after that.

Ok.

> > We could make this available in the folder where TPM2 tests are:
> > 
> > https://git.kernel.org/pub/scm/linux/kernel/git/jarkko/tpm2-scripts.git/tree/tpm2-reset-da-lock
> 
> 
> The tss packages also have command line tools to reset the DA lock, but it
> shouldn't be necessary to use them after running a **driver** test case.

If you speak about TSS, please alway say which one :-)

Adding non-volatile state changes explicitly is to a test case is both
intrusive and wrong. These type of choices are not to be done in the
test case implementation for sure.

An improvement that does not add extra side-effect, would be to read the
TPM_PT_LOCKOUT_COUNTER and roll back with a proper error message for the
lockout condition.

You can also configure the maximum number of tries up to (2 << 31) - 1
= 4294967295 with TPM2_DictionaryAttackParameters.

/Jarkko

Re: [PATCH v4 2/2] selftests: tpm2: Reset the dictionary attack lock

[Jarkko Sakkinen]

On Wed, Dec 01, 2021 at 12:16:11PM +0200, Jarkko Sakkinen wrote:
> On Mon, Nov 29, 2021 at 07:26:12PM -0500, Stefan Berger wrote:
> > 
> > On 11/29/21 18:43, Jarkko Sakkinen wrote:
> > > On Sat, Nov 27, 2021 at 11:10:52PM -0500, Stefan Berger wrote:
> > > > From: Stefan Berger <stefanb@linux.ibm.com>
> > > > 
> > > > Reset the dictionary attack lock to avoid the following types of test
> > > > failures after running the test 2 times:
> > > > 
> > > > ======================================================================
> > > > ERROR: test_unseal_with_wrong_policy (tpm2_tests.SmokeTest)
> > > > ----------------------------------------------------------------------
> > > > Traceback (most recent call last):
> > > >    File "/root/linux-ima-namespaces/tools/testing/selftests/tpm2/tpm2_tests.py", line 105, in test_unseal_with_wrong_policy
> > > >      blob = self.client.seal(self.root_key, data, auth, policy_dig)
> > > >    File "/root/linux-ima-namespaces/tools/testing/selftests/tpm2/tpm2.py", line 620, in seal
> > > >      rsp = self.send_cmd(cmd)
> > > >    File "/root/linux-ima-namespaces/tools/testing/selftests/tpm2/tpm2.py", line 397, in send_cmd
> > > >      raise ProtocolError(cc, rc)
> > > > tpm2.ProtocolError: TPM_RC_LOCKOUT: cc=0x00000153, rc=0x00000921
> > > > 
> > > > Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> > > > ---
> > > >   tools/testing/selftests/tpm2/tpm2_tests.py | 2 ++
> > > >   1 file changed, 2 insertions(+)
> > > > 
> > > > diff --git a/tools/testing/selftests/tpm2/tpm2_tests.py b/tools/testing/selftests/tpm2/tpm2_tests.py
> > > > index e63a37819978..ad6f54c01adf 100644
> > > > --- a/tools/testing/selftests/tpm2/tpm2_tests.py
> > > > +++ b/tools/testing/selftests/tpm2/tpm2_tests.py
> > > > @@ -139,6 +139,8 @@ class SmokeTest(unittest.TestCase):
> > > >           except:
> > > >               self.client.flush_context(handle)
> > > >               raise
> > > > +        finally:
> > > > +            self.client.reset_da_lock()
> > > >           self.assertEqual(rc, tpm2.TPM2_RC_POLICY_FAIL)
> > > > -- 
> > > > 2.31.1
> > > > 
> > > I don't agree with this as a DA lock has legit use. This would be adequate
> > > for systems dedicated for kernel testing only.
> > 
> > The problem is this particular test case I am patching here causes the above
> > test failures upon rerun. We are testing the driver here presumably and not
> > the TPM2, so I think we should leave the TPM2 as cleaned up as possible,
> > thus my suggestion is to reset the DA lock and we won't hear any complaints
> > after that.
> 
> Ok.
> 
> > > We could make this available in the folder where TPM2 tests are:
> > > 
> > > https://git.kernel.org/pub/scm/linux/kernel/git/jarkko/tpm2-scripts.git/tree/tpm2-reset-da-lock
> > 
> > 
> > The tss packages also have command line tools to reset the DA lock, but it
> > shouldn't be necessary to use them after running a **driver** test case.
> 
> If you speak about TSS, please alway say which one :-)
> 
> Adding non-volatile state changes explicitly is to a test case is both

A typo, should be:

"Adding non-volatile state changes explicitly to a test case is both"

/Jarkko

Re: [PATCH v4 2/2] selftests: tpm2: Reset the dictionary attack lock

[Stefan Berger]

On 12/1/21 05:16, Jarkko Sakkinen wrote:

> On Mon, Nov 29, 2021 at 07:26:12PM -0500, Stefan Berger wrote:
>> On 11/29/21 18:43, Jarkko Sakkinen wrote:
>>> On Sat, Nov 27, 2021 at 11:10:52PM -0500, Stefan Berger wrote:
>>>> From: Stefan Berger <stefanb@linux.ibm.com>
>>>>
>>>> Reset the dictionary attack lock to avoid the following types of test
>>>> failures after running the test 2 times:
>>>>
>>>> ======================================================================
>>>> ERROR: test_unseal_with_wrong_policy (tpm2_tests.SmokeTest)
>>>> ----------------------------------------------------------------------
>>>> Traceback (most recent call last):
>>>>     File "/root/linux-ima-namespaces/tools/testing/selftests/tpm2/tpm2_tests.py", line 105, in test_unseal_with_wrong_policy
>>>>       blob = self.client.seal(self.root_key, data, auth, policy_dig)
>>>>     File "/root/linux-ima-namespaces/tools/testing/selftests/tpm2/tpm2.py", line 620, in seal
>>>>       rsp = self.send_cmd(cmd)
>>>>     File "/root/linux-ima-namespaces/tools/testing/selftests/tpm2/tpm2.py", line 397, in send_cmd
>>>>       raise ProtocolError(cc, rc)
>>>> tpm2.ProtocolError: TPM_RC_LOCKOUT: cc=0x00000153, rc=0x00000921
>>>>
>>>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
>>>> ---
>>>>    tools/testing/selftests/tpm2/tpm2_tests.py | 2 ++
>>>>    1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/tools/testing/selftests/tpm2/tpm2_tests.py b/tools/testing/selftests/tpm2/tpm2_tests.py
>>>> index e63a37819978..ad6f54c01adf 100644
>>>> --- a/tools/testing/selftests/tpm2/tpm2_tests.py
>>>> +++ b/tools/testing/selftests/tpm2/tpm2_tests.py
>>>> @@ -139,6 +139,8 @@ class SmokeTest(unittest.TestCase):
>>>>            except:
>>>>                self.client.flush_context(handle)
>>>>                raise
>>>> +        finally:
>>>> +            self.client.reset_da_lock()
>>>>            self.assertEqual(rc, tpm2.TPM2_RC_POLICY_FAIL)
>>>> -- 
>>>> 2.31.1
>>>>
>>> I don't agree with this as a DA lock has legit use. This would be adequate
>>> for systems dedicated for kernel testing only.
>> The problem is this particular test case I am patching here causes the above
>> test failures upon rerun. We are testing the driver here presumably and not
>> the TPM2, so I think we should leave the TPM2 as cleaned up as possible,
>> thus my suggestion is to reset the DA lock and we won't hear any complaints
>> after that.
>
>> The tss packages also have command line tools to reset the DA lock, but it
>> shouldn't be necessary to use them after running a **driver** test case.
> If you speak about TSS, please alway say which one :-)

packages : both

tpm2_dictionarylockout -c [ -p password ]

tssdictionaryattacklockreset [-pwd password]


>
> Adding non-volatile state changes explicitly is to a test case is both
> intrusive and wrong. These type of choices are not to be done in the
> test case implementation for sure.

I drop this patch because if someone changed the password the lock reset 
will not work as expected.

One can also argue with having a test case that triggers a failure 
condition in the TPM2 is both intrusive and wrong. That said, the 
offending test cases should probably not even exist, especially since 
they test a user's knowledge about the device afterwards...


>
> An improvement that does not add extra side-effect, would be to read the
> TPM_PT_LOCKOUT_COUNTER and roll back with a proper error message for the
> lockout condition.
>
> You can also configure the maximum number of tries up to (2 << 31) - 1
> = 4294967295 with TPM2_DictionaryAttackParameters...

... which I think the user of a device driver test should not have to do.


   Stefan

>
> /Jarkko