[Skiboot] [PATCH v2 02/20] libstb: import stb_init() breaking it into multiple files

[Stewart Smith]

Claudio Carvalho <cclaudio at linux.vnet.ibm.com> writes:
> On 13/12/2017 07:51, Stewart Smith wrote:
>> Claudio Carvalho <cclaudio at linux.vnet.ibm.com> writes:
>>> diff --git a/libstb/secureboot.c b/libstb/secureboot.c
>>> new file mode 100644
>>> index 0000000..cdb6ea5
>>> --- /dev/null
>>> +++ b/libstb/secureboot.c
>> <snip>
>>> +void secureboot_init(void)
>>> +{
>>> +	struct dt_node *node;
>>> +	const char *hash_algo;
>>> +	const char *compat = NULL;
>>> +	int version;
>>> +	size_t size;
>>> +
>>> +	node = dt_find_by_path(dt_root, "/ibm,secureboot");
>>> +	if (!node) {
>>> +		prlog(PR_NOTICE, "secure boot not supported\n");
>>> +		return;
>>> +	}
>>> +
>>> +	if (!secureboot_is_compatible(node, &version, &compat)) {
>>> +		/**
>>> +		 * @fwts-label SecureBootNotCompatible
>>> +		 * @fwts-advice Compatible secureboot driver not found. Probably,
>>> +		 * hostboot/mambo/skiboot has updated the
>>> +		 * /ibm,secureboot/compatible without adding a driver that
>>> +		 * supports it.
>>> +		 */
>>> +		prlog(PR_ERR, "%s FAILED, /ibm,secureboot not compatible.\n",
>>> +		      __func__);
>>> +		return;
>>> +	}
>>> +
>>> +	prlog(PR_NOTICE, "Found %s\n", compat);
>>> +
>>> +	if (nvram_query_eq("force-secure-mode", "always")) {
>>> +		secure_mode = true;
>>> +		prlog(PR_NOTICE, "secure mode on (FORCED by nvram)\n");
>>> +	} else {
>>> +		secure_mode = dt_has_node_property(node, "secure-enabled", NULL);
>>> +		prlog(PR_NOTICE, "secure mode %s\n",
>>> +		      secure_mode ? "on" : "off");
>>> +	}
>>> +
>>> +	if (!secure_mode)
>>> +		return;
>> I'm thinking we should skip this check here, and do the verification and
>> measurement no matter what, keeping only the check in
>> secureboot_enforce(). IIRC this is a change from P8 behavior, but
>> considering we're aiming for "secure boot everywhere", it seems like a
>> good idea.
>>
>> (It's also useful for testing that things are still working well without
>> having to use the nvram trick).
>>
>> I can do that as part of merging if you like.
>>
>
> I think we still need to keep this check here. If the system is *not* 
> booting in secure mode, why do we need to spend cpu cycles and resources 
> initializing the secureboot dependencies such as the Container 
> Verification Code? The dependencies won't be used during boot (and at 
> runtime).

I was going along the line of thought that we may be able to spot some
corruption/problem using that infrastructure or at least *warn* the user
that their firmware *might* not be what they want it to be.

Also, it saves me having to go and flip the jumper on remote machines to
ensure that the code still works :)

(to be honest, the "not having to go flip a jumper" motivation is pretty
high for me)

-- 
Stewart Smith
OPAL Architect, IBM.