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

[Heinrich Schuchardt <xypron.glpk@gmx.de>]

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@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. If this
store is inconsistent, failing is required to ensure secure boot.

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