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

[Claudio Carvalho]

On 13/12/2017 22:56, Stewart Smith wrote:
> 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)

Yep, fair enough. You can remove the secure_mode check as part of 
merging if a V3 is not required.

Thanks,
Claudio