[2/3] libstb/stb.c: measure the IMA_CATALOG partition

Message ID 1504166040-16531-3-git-send-email-cclaudio@linux.vnet.ibm.com
State Under Review
Headers show
Series
  • libstb: add support to ibm,secureboot-v2
Related show

Commit Message

Claudio Carvalho Aug. 31, 2017, 7:53 a.m.
This maps a PCR number for the IMA_CATALOG partition so that it can be
measured (extended to the mapped PCR).

Signed-off-by: Claudio Carvalho <cclaudio@linux.vnet.ibm.com>
---
 libstb/stb.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Stewart Smith Sept. 20, 2017, 6:19 a.m. | #1
Claudio Carvalho <cclaudio@linux.vnet.ibm.com> writes:
> This maps a PCR number for the IMA_CATALOG partition so that it can be
> measured (extended to the mapped PCR).
>
> Signed-off-by: Claudio Carvalho <cclaudio@linux.vnet.ibm.com>
> ---
>  libstb/stb.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/libstb/stb.c b/libstb/stb.c
> index eab04eb..15aa682 100644
> --- a/libstb/stb.c
> +++ b/libstb/stb.c
> @@ -58,6 +58,7 @@ static struct {
>  	enum resource_id id;
>  	TPM_Pcr pcr;
>  } resources[] = {
> +	{ RESOURCE_ID_IMA_CATALOG, PCR_4 },
>  	{ RESOURCE_ID_KERNEL, PCR_4 },
>  	{ RESOURCE_ID_CAPP,   PCR_2 },
>  };

Our current async resource loading *currently* does so serially,
although there's no real requirement that this would be the
case in the future. Thus, we probably want something here to enforce
order if we're extending the same PCR?

Otherwise I forsee accepting an amazing patch that subtley makes the
order non-deterministic and we only find out ages later when somebody is
looking at PCR values and wondering why they're only consistent 99/100
boots.
Stewart Smith Sept. 20, 2017, 6:20 a.m. | #2
Claudio Carvalho <cclaudio@linux.vnet.ibm.com> writes:
> This maps a PCR number for the IMA_CATALOG partition so that it can be
> measured (extended to the mapped PCR).
>
> Signed-off-by: Claudio Carvalho <cclaudio@linux.vnet.ibm.com>
> ---
>  libstb/stb.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/libstb/stb.c b/libstb/stb.c
> index eab04eb..15aa682 100644
> --- a/libstb/stb.c
> +++ b/libstb/stb.c
> @@ -58,6 +58,7 @@ static struct {
>  	enum resource_id id;
>  	TPM_Pcr pcr;
>  } resources[] = {
> +	{ RESOURCE_ID_IMA_CATALOG, PCR_4 },
>  	{ RESOURCE_ID_KERNEL, PCR_4 },
>  	{ RESOURCE_ID_CAPP,   PCR_2 },

Any reason why PCR4 rather than PCR2?

The IMA_CATALOG seems more like CAPP than KERNEL, as in, bits of
data/microcode rather than other firmware component.
Claudio Carvalho Sept. 27, 2017, 10:21 p.m. | #3
On 20/09/2017 03:19, Stewart Smith wrote:
> Claudio Carvalho <cclaudio@linux.vnet.ibm.com> writes:
>> This maps a PCR number for the IMA_CATALOG partition so that it can be
>> measured (extended to the mapped PCR).
>>
>> Signed-off-by: Claudio Carvalho <cclaudio@linux.vnet.ibm.com>
>> ---
>>   libstb/stb.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/libstb/stb.c b/libstb/stb.c
>> index eab04eb..15aa682 100644
>> --- a/libstb/stb.c
>> +++ b/libstb/stb.c
>> @@ -58,6 +58,7 @@ static struct {
>>   	enum resource_id id;
>>   	TPM_Pcr pcr;
>>   } resources[] = {
>> +	{ RESOURCE_ID_IMA_CATALOG, PCR_4 },
>>   	{ RESOURCE_ID_KERNEL, PCR_4 },
>>   	{ RESOURCE_ID_CAPP,   PCR_2 },
>>   };
> Our current async resource loading *currently* does so serially,
> although there's no real requirement that this would be the
> case in the future. Thus, we probably want something here to enforce
> order if we're extending the same PCR?

Good catch. Ideally, we should measure the resource just before it is 
consumed.
Perhaps we could verify and measure the resource in the 
flash_resource_loaded() function. If so, we may need to find the 
sub-partition from there, in case the request is for a sub-partition. 
What do you think?

> Otherwise I forsee accepting an amazing patch that subtley makes the
> order non-deterministic and we only find out ages later when somebody is
> looking at PCR values and wondering why they're only consistent 99/100
> boots.
>
Claudio Carvalho Sept. 27, 2017, 10:58 p.m. | #4
On 20/09/2017 03:20, Stewart Smith wrote:
> Claudio Carvalho <cclaudio@linux.vnet.ibm.com> writes:
>> This maps a PCR number for the IMA_CATALOG partition so that it can be
>> measured (extended to the mapped PCR).
>>
>> Signed-off-by: Claudio Carvalho <cclaudio@linux.vnet.ibm.com>
>> ---
>>   libstb/stb.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/libstb/stb.c b/libstb/stb.c
>> index eab04eb..15aa682 100644
>> --- a/libstb/stb.c
>> +++ b/libstb/stb.c
>> @@ -58,6 +58,7 @@ static struct {
>>   	enum resource_id id;
>>   	TPM_Pcr pcr;
>>   } resources[] = {
>> +	{ RESOURCE_ID_IMA_CATALOG, PCR_4 },
>>   	{ RESOURCE_ID_KERNEL, PCR_4 },
>>   	{ RESOURCE_ID_CAPP,   PCR_2 },
> Any reason why PCR4 rather than PCR2?

The TCG PC Client spec for tpm 2.0 defines the PCR usage (Table 1):
PCR2 : UEFI driver and application code
PCR4 : UEFI Boot Manager Code (usually the MBR) and Boot Attempts

As you can see even PCR 2 and 4 are not a perfect match for CAPP and 
BOOTKERNEL. We have actively discussed about PCR usage and event types 
these days and the current proposal we are discussing is to start to 
measure all skiboot events in PCR 4 because the only event type that 
could be used with PCR 2 are EV_EFI_BOOT_SERVICES_APPLICATION, 
EV_EFI_BOOT_SERVICES_DRIVER and EV_EFI_RUNTIME_SERVICES_DRIVER. However, 
all of them refers to UEFI and the event field MUST contain a 
UEFI_IMAGE_LOAD_EVENT structure.

If we measure all skiboot events in PCR 4 we can use the EV_COMPACT_HASH 
event type, which says that the content of the event field is specified 
by the caller. In other words we could continue to put a string in the 
event field that describes the event, for example:

---------- EVENT 14 ----------
pcr_index            4
event_type           12 (EV_COMPACT_HASH)
digests.count        2
algorithm_id         11 (SHA1)
digest
83 3c 20 b9 f4 fc 0c 18 33 4f
88 0a 94 2f 02 a1 47 77 df 1f
a1 3f 66 3d f5 72 61 18 73 0c
6f c3
algorithm_id         4 (SHA256)
digest
83 3c 20 b9 f4 fc 0c 18 33 4f
88 0a 94 2f 02 a1 47 77 df 1f
event_size           4
event                'CAPP'

> The IMA_CATALOG seems more like CAPP than KERNEL, as in, bits of
> data/microcode rather than other firmware component.
>

Patch

diff --git a/libstb/stb.c b/libstb/stb.c
index eab04eb..15aa682 100644
--- a/libstb/stb.c
+++ b/libstb/stb.c
@@ -58,6 +58,7 @@  static struct {
 	enum resource_id id;
 	TPM_Pcr pcr;
 } resources[] = {
+	{ RESOURCE_ID_IMA_CATALOG, PCR_4 },
 	{ RESOURCE_ID_KERNEL, PCR_4 },
 	{ RESOURCE_ID_CAPP,   PCR_2 },
 };