[PATCH v2 6/6] efi_loader: always initialize the secure boot state

[Heinrich Schuchardt]

On 8/27/21 6:47 AM, AKASHI Takahiro wrote:
> On Fri, Aug 27, 2021 at 06:34:30AM +0200, Heinrich Schuchardt wrote:
>> On 8/27/21 5:53 AM, AKASHI Takahiro wrote:
>>> On Thu, Aug 26, 2021 at 03:48:05PM +0200, Heinrich Schuchardt wrote:
>>>> Even if we cannot read the variable store from disk we still need to
>>>> initialize the secure boot state.
>>>>
>>>> Don't continue to boot if the variable preseed is invalid as this indicates
>>>> that the variable store has been tampered.
>>>>
>>>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt at canonical.com>
>>>> ---
>>>> v2:
>>>> 	no change
>>>> ---
>>>>    lib/efi_loader/efi_variable.c | 12 ++++++++----
>>>>    1 file changed, 8 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
>>>> index 80996d0f47..6d92229e2a 100644
>>>> --- a/lib/efi_loader/efi_variable.c
>>>> +++ b/lib/efi_loader/efi_variable.c
>>>> @@ -427,13 +427,17 @@ efi_status_t efi_init_variables(void)
>>>>    	if (IS_ENABLED(CONFIG_EFI_VARIABLES_PRESEED)) {
>>>>    		ret = efi_var_restore((struct efi_var_file *)
>>>>    				      __efi_var_file_begin, true);
>>>> -		if (ret != EFI_SUCCESS)
>>>> +		if (ret != EFI_SUCCESS) {
>>>>    			log_err("Invalid EFI variable seed\n");
>>>> +			return ret;
>>>> +		}
>>>>    	}
>>>> -
>>>> -	ret = efi_var_from_file();
>>>> +	ret = efi_init_secure_state();
>>>>    	if (ret != EFI_SUCCESS)
>>>>    		return ret;
>>>>
>>>> -	return efi_init_secure_state();
>>>> +	/* Don't stop booting if variable store is not available */
>>>> +	efi_var_from_file();
>>>
>>> I think we have to think about two different cases:
>>> 1) there is no "variable store" file available.
>>> 2) it does exists, but reading from it (efi_var_restore()) failed
>>>
>>> For (2), we should return with an error as in the case of
>>> CONFIG_EFI_VARIABLES_PRESEED.
>>> Otherwise, the behavior is inconsistent.
>>
>> The preseed store is used for secure boot related variables.
>
> Where does this restriction come from?
> Kconfig says:
> Include a file with the initial values for non-volatile UEFI variables
> into the U-Boot binary. If this configuration option is set, changes
> to authentication related variables (PK, KEK, db, dbx) are not
> allowed.
>
> # oops: I didn't notice the last sentence.
> # but anyhow, it seems too restrictive.

The UEFI spec requires that the secure boot database is stored in
tamper-resistant storage. So it cannot be on the ESP.

Best regards

Heinrich

>
>
>> If this
>> store is inconsistent, failing is required to ensure secure boot.
>
> As I said, a file-based variable configuration cannot work
> as a secure platform any way.
>
> Why do we need this kind of restriction?
>
> -Takahiro Akashi
>
>> With my patches applied the file store can not contain any secure boot
>> related variables but it may contain boot options.
>>
>> Your suggestion could mean that if the file store is corrupted the board
>> is bricked.
>>
>> If the boot options cannot be read either because the file does not
>> exist or because the file is corrupt, I still want the user to have a
>> chance to load shim, GRUB, or the kernel and boot via the bootefi command.
>>
>> I cannot see any inconsistency here.
>>
>> Best regards
>>
>> Heinrich