[PATCH v2 0/2] tpm: Some fixes

[PATCH v2 0/2] tpm: Some fixes

[Stefan Berger]

This series of patches fixes the TPM SPAPR device model so that it reacts
in the same way as the other device models do when the backend device did
not start up properly. It now calls exit(1).

Due to a change in the TPM 2 code, the pcrUpdateCounter (14th byte) in the
TPM2_Pcrread response now returns a different value than before. So it's
better to skip the 14th byte when comparing expected against actual responses.

   Stefan

v1->v2:
  - simplified skipping of 14th byte in response

Stefan Berger (2):
  tpm: tpm_spapr: Exit on TPM backend failures
  tests: Skip over pcrUpdateCounter byte in result comparison

 hw/tpm/tpm_spapr.c     | 5 ++++-
 tests/qtest/tpm-util.c | 6 +++++-
 2 files changed, 9 insertions(+), 2 deletions(-)

-- 
2.24.1

[PATCH v2 1/2] tpm: tpm_spapr: Exit on TPM backend failures

[Stefan Berger]

Exit on TPM backend failures in the same way as the TPM CRB and TIS device
models do.

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
---
 hw/tpm/tpm_spapr.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/hw/tpm/tpm_spapr.c b/hw/tpm/tpm_spapr.c
index cb4dfd1e6a..8288ab0a15 100644
--- a/hw/tpm/tpm_spapr.c
+++ b/hw/tpm/tpm_spapr.c
@@ -306,7 +306,10 @@ static void tpm_spapr_reset(SpaprVioDevice *dev)
                             TPM_SPAPR_BUFFER_MAX);
 
     tpm_backend_reset(s->be_driver);
-    tpm_spapr_do_startup_tpm(s, s->be_buffer_size);
+
+    if (tpm_spapr_do_startup_tpm(s, s->be_buffer_size) < 0) {
+        exit(1);
+    }
 }
 
 static enum TPMVersion tpm_spapr_get_version(TPMIf *ti)
-- 
2.24.1

Re: [PATCH v2 1/2] tpm: tpm_spapr: Exit on TPM backend failures

[Philippe Mathieu-Daudé]

Hi Stefan,

On 7/7/20 6:05 AM, Stefan Berger wrote:
> Exit on TPM backend failures in the same way as the TPM CRB and TIS device
> models do.

Maybe the other models are not the best examples ;)

> 
> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> ---
>  hw/tpm/tpm_spapr.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/tpm/tpm_spapr.c b/hw/tpm/tpm_spapr.c
> index cb4dfd1e6a..8288ab0a15 100644
> --- a/hw/tpm/tpm_spapr.c
> +++ b/hw/tpm/tpm_spapr.c
> @@ -306,7 +306,10 @@ static void tpm_spapr_reset(SpaprVioDevice *dev)
>                              TPM_SPAPR_BUFFER_MAX);
>  
>      tpm_backend_reset(s->be_driver);
> -    tpm_spapr_do_startup_tpm(s, s->be_buffer_size);
> +
> +    if (tpm_spapr_do_startup_tpm(s, s->be_buffer_size) < 0) {

I don't see error reported, how users can know the cause of the exit?

> +        exit(1);

What about using this instead?

           qemu_system_shutdown_request(SHUTDOWN_CAUSE_HOST_ERROR);

> +    }
>  }
>  
>  static enum TPMVersion tpm_spapr_get_version(TPMIf *ti)
> 

Re: [PATCH v2 1/2] tpm: tpm_spapr: Exit on TPM backend failures

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1563 bytes --]

On Tue, Jul 07, 2020 at 06:20:49AM +0200, Philippe Mathieu-Daudé wrote:
> Hi Stefan,
> 
> On 7/7/20 6:05 AM, Stefan Berger wrote:
> > Exit on TPM backend failures in the same way as the TPM CRB and TIS device
> > models do.
> 
> Maybe the other models are not the best examples ;)
> 
> > 
> > Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> > ---
> >  hw/tpm/tpm_spapr.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/tpm/tpm_spapr.c b/hw/tpm/tpm_spapr.c
> > index cb4dfd1e6a..8288ab0a15 100644
> > --- a/hw/tpm/tpm_spapr.c
> > +++ b/hw/tpm/tpm_spapr.c
> > @@ -306,7 +306,10 @@ static void tpm_spapr_reset(SpaprVioDevice *dev)
> >                              TPM_SPAPR_BUFFER_MAX);
> >  
> >      tpm_backend_reset(s->be_driver);
> > -    tpm_spapr_do_startup_tpm(s, s->be_buffer_size);
> > +
> > +    if (tpm_spapr_do_startup_tpm(s, s->be_buffer_size) < 0) {
> 
> I don't see error reported, how users can know the cause of the exit?
> 
> > +        exit(1);
> 
> What about using this instead?
> 
>            qemu_system_shutdown_request(SHUTDOWN_CAUSE_HOST_ERROR);

Hrm.  I'm not entirely convinced that's what we want.  But we
definitely need some sort of error reported.

> 
> > +    }
> >  }
> >  
> >  static enum TPMVersion tpm_spapr_get_version(TPMIf *ti)
> > 
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/2] tpm: tpm_spapr: Exit on TPM backend failures

[Stefan Berger]

On 7/7/20 12:20 AM, Philippe Mathieu-Daudé wrote:
> Hi Stefan,
>
> On 7/7/20 6:05 AM, Stefan Berger wrote:
>> Exit on TPM backend failures in the same way as the TPM CRB and TIS device
>> models do.
> Maybe the other models are not the best examples ;)

At least they are known to report the error...


>
>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
>> ---
>>   hw/tpm/tpm_spapr.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/tpm/tpm_spapr.c b/hw/tpm/tpm_spapr.c
>> index cb4dfd1e6a..8288ab0a15 100644
>> --- a/hw/tpm/tpm_spapr.c
>> +++ b/hw/tpm/tpm_spapr.c
>> @@ -306,7 +306,10 @@ static void tpm_spapr_reset(SpaprVioDevice *dev)
>>                               TPM_SPAPR_BUFFER_MAX);
>>   
>>       tpm_backend_reset(s->be_driver);
>> -    tpm_spapr_do_startup_tpm(s, s->be_buffer_size);
>> +
>> +    if (tpm_spapr_do_startup_tpm(s, s->be_buffer_size) < 0) {
> I don't see error reported, how users can know the cause of the exit?


virt-manager does report the error then. It seems to be taking it from 
the last error message reported in the emulator backend when TPM_INIT 
fails with error code 0x101:

error: internal error: qemu unexpectedly closed the monitor: 
2020-07-07T12:49:28.333928Z qemu-system-ppc64: tpm-emulator: TPM result 
for CMD_INIT: 0x101 operation failed


>
>> +        exit(1);
> What about using this instead?
>
>             qemu_system_shutdown_request(SHUTDOWN_CAUSE_HOST_ERROR);

It doesn't have any effect, the VM just keeps on running. So the exit(1) 
is better and does report an error.

Re: [PATCH v2 1/2] tpm: tpm_spapr: Exit on TPM backend failures

[Philippe Mathieu-Daudé]

On 7/7/20 2:52 PM, Stefan Berger wrote:
> On 7/7/20 12:20 AM, Philippe Mathieu-Daudé wrote:
>> Hi Stefan,
>>
>> On 7/7/20 6:05 AM, Stefan Berger wrote:
>>> Exit on TPM backend failures in the same way as the TPM CRB and TIS
>>> device
>>> models do.
>> Maybe the other models are not the best examples ;)
> 
> At least they are known to report the error...
> 
> 
>>
>>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
>>> ---
>>>   hw/tpm/tpm_spapr.c | 5 ++++-
>>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/tpm/tpm_spapr.c b/hw/tpm/tpm_spapr.c
>>> index cb4dfd1e6a..8288ab0a15 100644
>>> --- a/hw/tpm/tpm_spapr.c
>>> +++ b/hw/tpm/tpm_spapr.c
>>> @@ -306,7 +306,10 @@ static void tpm_spapr_reset(SpaprVioDevice *dev)
>>>                               TPM_SPAPR_BUFFER_MAX);
>>>         tpm_backend_reset(s->be_driver);
>>> -    tpm_spapr_do_startup_tpm(s, s->be_buffer_size);
>>> +
>>> +    if (tpm_spapr_do_startup_tpm(s, s->be_buffer_size) < 0) {
>> I don't see error reported, how users can know the cause of the exit?
> 
> 
> virt-manager does report the error then. It seems to be taking it from
> the last error message reported in the emulator backend when TPM_INIT
> fails with error code 0x101:
> 
> error: internal error: qemu unexpectedly closed the monitor:
> 2020-07-07T12:49:28.333928Z qemu-system-ppc64: tpm-emulator: TPM result
> for CMD_INIT: 0x101 operation failed

Ah, good.

> 
>>
>>> +        exit(1);
>> What about using this instead?
>>
>>             qemu_system_shutdown_request(SHUTDOWN_CAUSE_HOST_ERROR);
> 
> It doesn't have any effect, the VM just keeps on running. So the exit(1)
> is better and does report an error.
> 

Hmm maybe something is missing or it was never totally implemented?

Anyway since virt-manager is notified, I'm not objecting to this patch
:)

[PATCH v2 2/2] tests: Skip over pcrUpdateCounter byte in result comparison

[Stefan Berger]

Due to a change in the TPM 2 code the pcrUpdate counter in the
PCRRead response is now different, so we skip comparison of the
14th byte.

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
---
 tests/qtest/tpm-util.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/tests/qtest/tpm-util.c b/tests/qtest/tpm-util.c
index 34efae8f18..58a9593745 100644
--- a/tests/qtest/tpm-util.c
+++ b/tests/qtest/tpm-util.c
@@ -139,7 +139,11 @@ void tpm_util_pcrread(QTestState *s, tx_func *tx,
 
     tx(s, tpm_pcrread, sizeof(tpm_pcrread), buffer, sizeof(buffer));
 
-    g_assert_cmpmem(buffer, exp_resp_size, exp_resp, exp_resp_size);
+    /* skip pcrUpdateCounter (14th byte) in comparison */
+    g_assert(exp_resp_size >= 15);
+    g_assert_cmpmem(buffer, 13, exp_resp, 13);
+    g_assert_cmpmem(&buffer[14], exp_resp_size - 14,
+                    &exp_resp[14], exp_resp_size - 14);
 }
 
 bool tpm_util_swtpm_has_tpm2(void)
-- 
2.24.1

Re: [PATCH v2 2/2] tests: Skip over pcrUpdateCounter byte in result comparison

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1371 bytes --]

On Tue, Jul 07, 2020 at 12:05:22AM -0400, Stefan Berger wrote:
> Due to a change in the TPM 2 code the pcrUpdate counter in the
> PCRRead response is now different, so we skip comparison of the
> 14th byte.

Can you elaborate on this a bit, both in the code comment and the
commit message.

> 
> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> ---
>  tests/qtest/tpm-util.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/qtest/tpm-util.c b/tests/qtest/tpm-util.c
> index 34efae8f18..58a9593745 100644
> --- a/tests/qtest/tpm-util.c
> +++ b/tests/qtest/tpm-util.c
> @@ -139,7 +139,11 @@ void tpm_util_pcrread(QTestState *s, tx_func *tx,
>  
>      tx(s, tpm_pcrread, sizeof(tpm_pcrread), buffer, sizeof(buffer));
>  
> -    g_assert_cmpmem(buffer, exp_resp_size, exp_resp, exp_resp_size);
> +    /* skip pcrUpdateCounter (14th byte) in comparison */
> +    g_assert(exp_resp_size >= 15);
> +    g_assert_cmpmem(buffer, 13, exp_resp, 13);
> +    g_assert_cmpmem(&buffer[14], exp_resp_size - 14,
> +                    &exp_resp[14], exp_resp_size - 14);
>  }
>  
>  bool tpm_util_swtpm_has_tpm2(void)

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 2/2] tests: Skip over pcrUpdateCounter byte in result comparison

[Stefan Berger]

On 7/7/20 4:19 AM, David Gibson wrote:
> On Tue, Jul 07, 2020 at 12:05:22AM -0400, Stefan Berger wrote:
>> Due to a change in the TPM 2 code the pcrUpdate counter in the
>> PCRRead response is now different, so we skip comparison of the
>> 14th byte.
> Can you elaborate on this a bit, both in the code comment and the
> commit message.


Will do in v3. Basically the TPM 2 code has been 'fixed' to reflect the 
pcclient profile more closely and due to the change in PCRs that belong 
to the 'TCB group' the response of the TPM2_Pcrread command now returns 
a slightly different value for the pcrUpdateCounter value, which 
unfortunately breaks the test cases. Leaving the TPM 2 as it was wasn't 
a long-term option.

https://github.com/stefanberger/libtpms/commit/0f5d791a7d3431a6831086f5a186cc53149f695f