| Message ID | 1504166040-16531-3-git-send-email-cclaudio@linux.vnet.ibm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | libstb: add support to ibm,secureboot-v2 | expand |
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.
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.
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. >
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. >
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 }, };
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(+)