[PATCH v2 5/6] efi_loader: writing AuditMode, DeployedMode

[Heinrich Schuchardt]

On 8/27/21 5:05 AM, AKASHI Takahiro wrote:
> Heinrich,
>
> On Thu, Aug 26, 2021 at 03:48:04PM +0200, Heinrich Schuchardt wrote:
>> Writing variables AuditMode or Deployed Mode must update the secure boot
>> state.
>>
>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt at canonical.com>
>> ---
>> v2:
>> 	correct variable name in lib/efi_loader/efi_variable_tee.c
>> ---
>>   include/efi_variable.h            | 1 +
>>   lib/efi_loader/efi_var_common.c   | 2 ++
>>   lib/efi_loader/efi_variable.c     | 6 +++---
>>   lib/efi_loader/efi_variable_tee.c | 4 +++-
>>   4 files changed, 9 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/efi_variable.h b/include/efi_variable.h
>> index 2d97655e1f..0440d356bc 100644
>> --- a/include/efi_variable.h
>> +++ b/include/efi_variable.h
>> @@ -12,6 +12,7 @@
>>
>>   enum efi_auth_var_type {
>>   	EFI_AUTH_VAR_NONE = 0,
>> +	EFI_AUTH_MODE,
>>   	EFI_AUTH_VAR_PK,
>>   	EFI_AUTH_VAR_KEK,
>>   	EFI_AUTH_VAR_DB,
>> diff --git a/lib/efi_loader/efi_var_common.c b/lib/efi_loader/efi_var_common.c
>> index 63ad6fea9e..6fabcfe72c 100644
>> --- a/lib/efi_loader/efi_var_common.c
>> +++ b/lib/efi_loader/efi_var_common.c
>> @@ -34,6 +34,8 @@ static const struct efi_auth_var_name_type name_type[] = {
>>   	{u"dbx",  &efi_guid_image_security_database, EFI_AUTH_VAR_DBX},
>>   	{u"dbt",  &efi_guid_image_security_database, EFI_AUTH_VAR_DBT},
>>   	{u"dbr",  &efi_guid_image_security_database, EFI_AUTH_VAR_DBR},
>> +	{u"AuditMode", &efi_global_variable_guid, EFI_AUTH_MODE},
>> +	{u"DeployedMode", &efi_global_variable_guid, EFI_AUTH_MODE},
>>   };
>>
>>   static bool efi_secure_boot;
>> diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
>> index a7d305ffbc..80996d0f47 100644
>> --- a/lib/efi_loader/efi_variable.c
>> +++ b/lib/efi_loader/efi_variable.c
>> @@ -247,7 +247,7 @@ efi_status_t efi_set_variable_int(u16 *variable_name, const efi_guid_t *vendor,
>>   			return EFI_WRITE_PROTECTED;
>>
>>   		if (IS_ENABLED(CONFIG_EFI_VARIABLES_PRESEED)) {
>> -			if (var_type != EFI_AUTH_VAR_NONE)
>> +			if (var_type >= EFI_AUTH_VAR_PK)
>
> This change is irrelevant to the purpose of this commit.

Thank you for reviewing the series.

EFI_AUTH_MODE is needed in the implementation of this patch and requires
this change. But I can split the patch in two.

>
>>   				return EFI_WRITE_PROTECTED;
>>   		}
>>
>> @@ -268,7 +268,7 @@ efi_status_t efi_set_variable_int(u16 *variable_name, const efi_guid_t *vendor,
>>   			return EFI_NOT_FOUND;
>>   	}
>>
>> -	if (var_type != EFI_AUTH_VAR_NONE) {
>> +	if (var_type >= EFI_AUTH_VAR_PK) {
>>   		/* authentication is mandatory */
>>   		if (!(attributes &
>>   		      EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS)) {
>> @@ -328,7 +328,7 @@ efi_status_t efi_set_variable_int(u16 *variable_name, const efi_guid_t *vendor,
>>   	if (ret != EFI_SUCCESS)
>>   		return ret;
>>
>> -	if (var_type == EFI_AUTH_VAR_PK)
>> +	if (var_type == EFI_AUTH_VAR_PK || var_type == EFI_AUTH_MODE)
>>   		ret = efi_init_secure_state();
>
> As I said, calling efi_init_secure_state() is not a good idea.
>
> The scheme that I have in mind:
> * if some event takes place, then trigger the transition.
> * efi_transfer_secure_state() handles/take actions for the transition.
>
> Looking at "Figure 32-4 Secure Boot Modes", there are a couple of events
> defined:
> 1) Enroll PKpub
> 2) Platform Specific PKpub Clear/Delete PKpub
> 3) Audit := 1
> 4) DeployedMode := 1
> 5) Platform Specific DeployedMode Clear
>
> (Please note that "enroll/platform specific" operations should end up
> modifying a relevant UEFI variable, any way.)
>
> So, in the case above, we should do like this:
>    if ("PK" is added/modified)
>       if (SetupMode)
>          efi_transfer_secure_state(UserMode)
>       else (AuditMode)
>          efi_transfer_secure_state(DeployedMode)
>    else if ("AuditMode" is set)
>       if (SetupMode || UserMode)
>          efi_transfer_secure_state(AuditMode)
>    else if
>       and so on

Here we are in efi_set_variable_int(). efi_transfer_secure_state()
itself calls efi_set_variable_int() repeatedly.

Hence we need a way for a user to call SetVariable() with the side
effects you described above and a way to alter the state variables
without side effects.

There are different ways to implement this:

1) As we are on a single threaded system we can use a static
    state variable. This is the approach in patch 1.
2) We can add a parameter to efi_set_variable_int() indicating that
    the variable change shall not have side effects.
3) We can carve out a function for setting a variable without side
    effects.

We have two implementations of efi_set_variable_int():

* One for file based variables in lib/efi_loader/efi_variable.c.
* Another for StMM based variables in lib/efi_loader/efi_variable_tee.c.

Whatever we do must work for both implementations of variables.

lib/efi_loader/efi_variable_tee.c has two calls to
efi_init_secure_state() currently matching the calls in
lib/efi_loader/efi_variable.c.

@Ilias:
Which part of the logic described in Figure 32-4 Secure Boot Modes of
UEFI spec 2.9 exists inside StMM?
Where is it coded?

Best regards

Heinrich

>
> The logic is clear and the code directly renders what the figure 32-4 shows.
> What's more, it will make it much easier for reviewers (and users)
> to confirm the code is fully compliant with the specification
> in terms of the "conditions" vs. resultant system status.
>
> Then, each of the system's secure status can be always maintained
> within efi_transfer_secure_state().
>
> In addition, we will not have to have a hacky "lock" in
> efi_init_secure_state().
>
> Those are the reason why I want to stick to the scheme above.
>
> -Takahiro Akashi
>
>
>>   	else
>>   		ret = EFI_SUCCESS;
>> diff --git a/lib/efi_loader/efi_variable_tee.c b/lib/efi_loader/efi_variable_tee.c
>> index 51920bcb51..a6d5752045 100644
>> --- a/lib/efi_loader/efi_variable_tee.c
>> +++ b/lib/efi_loader/efi_variable_tee.c
>> @@ -512,6 +512,7 @@ efi_status_t efi_set_variable_int(u16 *variable_name, const efi_guid_t *vendor,
>>   	efi_uintn_t payload_size;
>>   	efi_uintn_t name_size;
>>   	u8 *comm_buf = NULL;
>> +	enum efi_auth_var_type var_type;
>>   	bool ro;
>>
>>   	if (!variable_name || variable_name[0] == 0 || !vendor) {
>> @@ -590,7 +591,8 @@ efi_status_t efi_set_variable_int(u16 *variable_name, const efi_guid_t *vendor,
>>   	if (alt_ret != EFI_SUCCESS)
>>   		goto out;
>>
>> -	if (!u16_strcmp(variable_name, L"PK"))
>> +	var_type = efi_auth_var_get_type(variable_name, vendor);
>> +	if (var_type == EFI_AUTH_VAR_PK || var_type == EFI_AUTH_MODE)
>>   		alt_ret = efi_init_secure_state();
>>   out:
>>   	free(comm_buf);
>> --
>> 2.30.2
>>