[PATCH v2 0/6] efi_loader: fix secure boot mode transitions

The UEFI specification 2.9 defines the different modes that secure boot may be in.
The patch series adds support for the "Deployed Mode" and the "Setup Mode".
Furthermore the secure boot signature database must only be loaded from tamper-resistant storage. So we must not load it from ubootefi.var on the EFI system partition but only from the preseed variables store or via the OP-TEE driver for the eMMC replay protected memory partition.
v2: correct variable name in lib/efi_loader/efi_variable_tee.c
Heinrich Schuchardt (6): efi_loader: stop recursion in efi_init_secure_state efi_loader: correct determination of secure boot state efi_loader: don't load signature database from file efi_loader: correct secure boot state transition efi_loader: writing AuditMode, DeployedMode efi_loader: always initialize the secure boot state
include/efi_variable.h | 6 ++- lib/efi_loader/efi_var_common.c | 66 +++++++++++++++++++++++-------- lib/efi_loader/efi_var_file.c | 41 +++++++++++-------- lib/efi_loader/efi_variable.c | 20 ++++++---- lib/efi_loader/efi_variable_tee.c | 4 +- 5 files changed, 95 insertions(+), 42 deletions(-)

efi_init_secure_state() calls efi_transfer_secure_state() which may delete variable "PK" which will result in calling efi_init_secure_state() again.
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com --- v2: no change --- lib/efi_loader/efi_var_common.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/lib/efi_loader/efi_var_common.c b/lib/efi_loader/efi_var_common.c index 3d92afe2eb..654ce81f9d 100644 --- a/lib/efi_loader/efi_var_common.c +++ b/lib/efi_loader/efi_var_common.c @@ -314,11 +314,15 @@ err:
efi_status_t efi_init_secure_state(void) { + static bool lock; enum efi_secure_mode mode = EFI_MODE_SETUP; u8 efi_vendor_keys = 0; efi_uintn_t size = 0; efi_status_t ret;
+ if (lock) + return EFI_SUCCESS; + ret = efi_get_variable_int(L"PK", &efi_global_variable_guid, NULL, &size, NULL, NULL); if (ret == EFI_BUFFER_TOO_SMALL) { @@ -326,7 +330,9 @@ efi_status_t efi_init_secure_state(void) mode = EFI_MODE_USER; }
+ lock = true; ret = efi_transfer_secure_state(mode); + lock = false; if (ret != EFI_SUCCESS) return ret;

Heinrich,
On Thu, Aug 26, 2021 at 03:48:00PM +0200, Heinrich Schuchardt wrote:
efi_init_secure_state() calls efi_transfer_secure_state() which may delete variable "PK" which will result in calling efi_init_secure_state() again.
I don't think it is a right thing to do. So I would say nak to this version. When I first implemented those functions, I intended to call efi_init_secure_state() only at the system initialization. Later on, all the transitions should be managed by efi_transfer_secure_state() as well as its callers.
Calling efi_init_secure_state() in efi_set_variable_int() is a bad idea. (then you see 'recursion'.) I will explain more in your patch#5.
-Takahiro Akashi
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com
v2: no change
lib/efi_loader/efi_var_common.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/lib/efi_loader/efi_var_common.c b/lib/efi_loader/efi_var_common.c index 3d92afe2eb..654ce81f9d 100644 --- a/lib/efi_loader/efi_var_common.c +++ b/lib/efi_loader/efi_var_common.c @@ -314,11 +314,15 @@ err:
efi_status_t efi_init_secure_state(void) {
static bool lock; enum efi_secure_mode mode = EFI_MODE_SETUP; u8 efi_vendor_keys = 0; efi_uintn_t size = 0; efi_status_t ret;
if (lock)
return EFI_SUCCESS;
ret = efi_get_variable_int(L"PK", &efi_global_variable_guid, NULL, &size, NULL, NULL); if (ret == EFI_BUFFER_TOO_SMALL) {
@@ -326,7 +330,9 @@ efi_status_t efi_init_secure_state(void) mode = EFI_MODE_USER; }
- lock = true; ret = efi_transfer_secure_state(mode);
- lock = false; if (ret != EFI_SUCCESS) return ret;
-- 2.30.2

When U-Boot is started we have to use the existing variables to determine in which secure boot state we are.
* If a platform key PK is present and DeployedMode=1, we are in deployed mode. * If no platform key PK is present and AuditMode=1, we are in audit mode. * Otherwise if a platform key is present, we are in user mode. * Otherwise if no platform key is present, we are in setup mode.
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com --- v2: no change --- lib/efi_loader/efi_var_common.c | 39 ++++++++++++++++++++++++++------- 1 file changed, 31 insertions(+), 8 deletions(-)
diff --git a/lib/efi_loader/efi_var_common.c b/lib/efi_loader/efi_var_common.c index 654ce81f9d..cf7afecd60 100644 --- a/lib/efi_loader/efi_var_common.c +++ b/lib/efi_loader/efi_var_common.c @@ -315,20 +315,43 @@ err: efi_status_t efi_init_secure_state(void) { static bool lock; - enum efi_secure_mode mode = EFI_MODE_SETUP; + enum efi_secure_mode mode; u8 efi_vendor_keys = 0; - efi_uintn_t size = 0; + efi_uintn_t size; efi_status_t ret;
if (lock) return EFI_SUCCESS; - - ret = efi_get_variable_int(L"PK", &efi_global_variable_guid, - NULL, &size, NULL, NULL); - if (ret == EFI_BUFFER_TOO_SMALL) { - if (IS_ENABLED(CONFIG_EFI_SECURE_BOOT)) - mode = EFI_MODE_USER; + u8 deployed_mode = 0; + u8 audit_mode = 0; + u8 setup_mode = 1; + + if (IS_ENABLED(CONFIG_EFI_SECURE_BOOT)) { + size = sizeof(deployed_mode); + ret = efi_get_variable_int(u"DeployedMode", &efi_global_variable_guid, + NULL, &size, &deployed_mode, NULL); + size = sizeof(audit_mode); + ret = efi_get_variable_int(u"AuditMode", &efi_global_variable_guid, + NULL, &size, &audit_mode, NULL); + size = 0; + ret = efi_get_variable_int(u"PK", &efi_global_variable_guid, + NULL, &size, NULL, NULL); + if (ret == EFI_BUFFER_TOO_SMALL) { + setup_mode = 0; + audit_mode = 0; + } else { + setup_mode = 1; + deployed_mode = 0; + } } + if (deployed_mode) + mode = EFI_MODE_DEPLOYED; + else if (audit_mode) + mode = EFI_MODE_AUDIT; + else if (setup_mode) + mode = EFI_MODE_SETUP; + else + mode = EFI_MODE_USER;
lock = true; ret = efi_transfer_secure_state(mode);

The UEFI specification requires that the signature database may only be stored in tamper-resistant storage. So these variable may not be read from an unsigned file.
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com --- v2: no change --- include/efi_variable.h | 5 +++- lib/efi_loader/efi_var_common.c | 2 -- lib/efi_loader/efi_var_file.c | 41 ++++++++++++++++++++------------- lib/efi_loader/efi_variable.c | 2 +- 4 files changed, 30 insertions(+), 20 deletions(-)
diff --git a/include/efi_variable.h b/include/efi_variable.h index 4623a64142..2d97655e1f 100644 --- a/include/efi_variable.h +++ b/include/efi_variable.h @@ -161,10 +161,13 @@ efi_status_t __maybe_unused efi_var_collect(struct efi_var_file **bufp, loff_t * /** * efi_var_restore() - restore EFI variables from buffer * + * Only if @safe is set secure boot related variables will be restored. + * * @buf: buffer + * @safe: restoring from tamper-resistant storage * Return: status code */ -efi_status_t efi_var_restore(struct efi_var_file *buf); +efi_status_t efi_var_restore(struct efi_var_file *buf, bool safe);
/** * efi_var_from_file() - read variables from file diff --git a/lib/efi_loader/efi_var_common.c b/lib/efi_loader/efi_var_common.c index cf7afecd60..b0c5b672c5 100644 --- a/lib/efi_loader/efi_var_common.c +++ b/lib/efi_loader/efi_var_common.c @@ -32,10 +32,8 @@ static const struct efi_auth_var_name_type name_type[] = { {u"KEK", &efi_global_variable_guid, EFI_AUTH_VAR_KEK}, {u"db", &efi_guid_image_security_database, EFI_AUTH_VAR_DB}, {u"dbx", &efi_guid_image_security_database, EFI_AUTH_VAR_DBX}, - /* not used yet {u"dbt", &efi_guid_image_security_database, EFI_AUTH_VAR_DBT}, {u"dbr", &efi_guid_image_security_database, EFI_AUTH_VAR_DBR}, - */ };
static bool efi_secure_boot; diff --git a/lib/efi_loader/efi_var_file.c b/lib/efi_loader/efi_var_file.c index de076b8cbc..c7c6805ed0 100644 --- a/lib/efi_loader/efi_var_file.c +++ b/lib/efi_loader/efi_var_file.c @@ -148,9 +148,10 @@ error: #endif }
-efi_status_t efi_var_restore(struct efi_var_file *buf) +efi_status_t efi_var_restore(struct efi_var_file *buf, bool safe) { struct efi_var_entry *var, *last_var; + u16 *data; efi_status_t ret;
if (buf->reserved || buf->magic != EFI_VAR_FILE_MAGIC || @@ -160,21 +161,29 @@ efi_status_t efi_var_restore(struct efi_var_file *buf) return EFI_INVALID_PARAMETER; }
- var = buf->var; last_var = (struct efi_var_entry *)((u8 *)buf + buf->length); - while (var < last_var) { - u16 *data = var->name + u16_strlen(var->name) + 1; - - if (var->attr & EFI_VARIABLE_NON_VOLATILE && var->length) { - ret = efi_var_mem_ins(var->name, &var->guid, var->attr, - var->length, data, 0, NULL, - var->time); - if (ret != EFI_SUCCESS) - log_err("Failed to set EFI variable %ls\n", - var->name); - } - var = (struct efi_var_entry *) - ALIGN((uintptr_t)data + var->length, 8); + for (var = buf->var; var < last_var; + var = (struct efi_var_entry *) + ALIGN((uintptr_t)data + var->length, 8)) { + + data = var->name + u16_strlen(var->name) + 1; + + /* + * Secure boot related and non-volatile variables shall only be + * restored from U-Boot's preseed. + */ + if (!safe && + (efi_auth_var_get_type(var->name, &var->guid) != + EFI_AUTH_VAR_NONE || + !(var->attr & EFI_VARIABLE_NON_VOLATILE))) + continue; + if (!var->length) + continue; + ret = efi_var_mem_ins(var->name, &var->guid, var->attr, + var->length, data, 0, NULL, + var->time); + if (ret != EFI_SUCCESS) + log_err("Failed to set EFI variable %ls\n", var->name); } return EFI_SUCCESS; } @@ -213,7 +222,7 @@ efi_status_t efi_var_from_file(void) log_err("Failed to load EFI variables\n"); goto error; } - if (buf->length != len || efi_var_restore(buf) != EFI_SUCCESS) + if (buf->length != len || efi_var_restore(buf, false) != EFI_SUCCESS) log_err("Invalid EFI variables file\n"); error: free(buf); diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c index ba0874e9e7..a7d305ffbc 100644 --- a/lib/efi_loader/efi_variable.c +++ b/lib/efi_loader/efi_variable.c @@ -426,7 +426,7 @@ 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); + __efi_var_file_begin, true); if (ret != EFI_SUCCESS) log_err("Invalid EFI variable seed\n"); }

On Thu, Aug 26, 2021 at 03:48:02PM +0200, Heinrich Schuchardt wrote:
The UEFI specification requires that the signature database may only be stored in tamper-resistant storage. So these variable may not be read from an unsigned file.
I don't have a strong opinion here, but it seems to be too restrictive. Nobody expects that file-based variable implementation is *safe*. Leave it as it is so that people can easily experiment secure boot.
-Takahiro Akashi
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com
v2: no change
include/efi_variable.h | 5 +++- lib/efi_loader/efi_var_common.c | 2 -- lib/efi_loader/efi_var_file.c | 41 ++++++++++++++++++++------------- lib/efi_loader/efi_variable.c | 2 +- 4 files changed, 30 insertions(+), 20 deletions(-)
diff --git a/include/efi_variable.h b/include/efi_variable.h index 4623a64142..2d97655e1f 100644 --- a/include/efi_variable.h +++ b/include/efi_variable.h @@ -161,10 +161,13 @@ efi_status_t __maybe_unused efi_var_collect(struct efi_var_file **bufp, loff_t * /**
- efi_var_restore() - restore EFI variables from buffer
- Only if @safe is set secure boot related variables will be restored.
- @buf: buffer
*/
- @safe: restoring from tamper-resistant storage
- Return: status code
-efi_status_t efi_var_restore(struct efi_var_file *buf); +efi_status_t efi_var_restore(struct efi_var_file *buf, bool safe);
/**
- efi_var_from_file() - read variables from file
diff --git a/lib/efi_loader/efi_var_common.c b/lib/efi_loader/efi_var_common.c index cf7afecd60..b0c5b672c5 100644 --- a/lib/efi_loader/efi_var_common.c +++ b/lib/efi_loader/efi_var_common.c @@ -32,10 +32,8 @@ static const struct efi_auth_var_name_type name_type[] = { {u"KEK", &efi_global_variable_guid, EFI_AUTH_VAR_KEK}, {u"db", &efi_guid_image_security_database, EFI_AUTH_VAR_DB}, {u"dbx", &efi_guid_image_security_database, EFI_AUTH_VAR_DBX},
- /* not used yet {u"dbt", &efi_guid_image_security_database, EFI_AUTH_VAR_DBT}, {u"dbr", &efi_guid_image_security_database, EFI_AUTH_VAR_DBR},
- */
};
static bool efi_secure_boot; diff --git a/lib/efi_loader/efi_var_file.c b/lib/efi_loader/efi_var_file.c index de076b8cbc..c7c6805ed0 100644 --- a/lib/efi_loader/efi_var_file.c +++ b/lib/efi_loader/efi_var_file.c @@ -148,9 +148,10 @@ error: #endif }
-efi_status_t efi_var_restore(struct efi_var_file *buf) +efi_status_t efi_var_restore(struct efi_var_file *buf, bool safe) { struct efi_var_entry *var, *last_var;
u16 *data; efi_status_t ret;
if (buf->reserved || buf->magic != EFI_VAR_FILE_MAGIC ||
@@ -160,21 +161,29 @@ efi_status_t efi_var_restore(struct efi_var_file *buf) return EFI_INVALID_PARAMETER; }
- var = buf->var; last_var = (struct efi_var_entry *)((u8 *)buf + buf->length);
- while (var < last_var) {
u16 *data = var->name + u16_strlen(var->name) + 1;
if (var->attr & EFI_VARIABLE_NON_VOLATILE && var->length) {
ret = efi_var_mem_ins(var->name, &var->guid, var->attr,
var->length, data, 0, NULL,
var->time);
if (ret != EFI_SUCCESS)
log_err("Failed to set EFI variable %ls\n",
var->name);
}
var = (struct efi_var_entry *)
ALIGN((uintptr_t)data + var->length, 8);
- for (var = buf->var; var < last_var;
var = (struct efi_var_entry *)
ALIGN((uintptr_t)data + var->length, 8)) {
data = var->name + u16_strlen(var->name) + 1;
/*
* Secure boot related and non-volatile variables shall only be
* restored from U-Boot's preseed.
*/
if (!safe &&
(efi_auth_var_get_type(var->name, &var->guid) !=
EFI_AUTH_VAR_NONE ||
!(var->attr & EFI_VARIABLE_NON_VOLATILE)))
continue;
if (!var->length)
continue;
ret = efi_var_mem_ins(var->name, &var->guid, var->attr,
var->length, data, 0, NULL,
var->time);
if (ret != EFI_SUCCESS)
} return EFI_SUCCESS;log_err("Failed to set EFI variable %ls\n", var->name);
} @@ -213,7 +222,7 @@ efi_status_t efi_var_from_file(void) log_err("Failed to load EFI variables\n"); goto error; }
- if (buf->length != len || efi_var_restore(buf) != EFI_SUCCESS)
- if (buf->length != len || efi_var_restore(buf, false) != EFI_SUCCESS) log_err("Invalid EFI variables file\n");
error: free(buf); diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c index ba0874e9e7..a7d305ffbc 100644 --- a/lib/efi_loader/efi_variable.c +++ b/lib/efi_loader/efi_variable.c @@ -426,7 +426,7 @@ 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);
if (ret != EFI_SUCCESS) log_err("Invalid EFI variable seed\n"); }__efi_var_file_begin, true);
-- 2.30.2

On 8/27/21 6:12 AM, AKASHI Takahiro wrote:
On Thu, Aug 26, 2021 at 03:48:02PM +0200, Heinrich Schuchardt wrote:
The UEFI specification requires that the signature database may only be stored in tamper-resistant storage. So these variable may not be read from an unsigned file.
I don't have a strong opinion here, but it seems to be too restrictive. Nobody expects that file-based variable implementation is *safe*. Leave it as it is so that people can easily experiment secure boot.
If the prior boot stage checks the integrity of the U-Boot binary, the file based implementation becomes 'safe' with this patch.
Users can still experiment with secure boot by setting the secure boot variables via the efidebug command.
I cannot see a use case for having the secure boot data base on an insecure medium.
Best regards
Heinrich
-Takahiro Akashi
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com
v2: no change
include/efi_variable.h | 5 +++- lib/efi_loader/efi_var_common.c | 2 -- lib/efi_loader/efi_var_file.c | 41 ++++++++++++++++++++------------- lib/efi_loader/efi_variable.c | 2 +- 4 files changed, 30 insertions(+), 20 deletions(-)
diff --git a/include/efi_variable.h b/include/efi_variable.h index 4623a64142..2d97655e1f 100644 --- a/include/efi_variable.h +++ b/include/efi_variable.h @@ -161,10 +161,13 @@ efi_status_t __maybe_unused efi_var_collect(struct efi_var_file **bufp, loff_t * /**
- efi_var_restore() - restore EFI variables from buffer
- Only if @safe is set secure boot related variables will be restored.
- @buf: buffer
*/
- @safe: restoring from tamper-resistant storage
- Return: status code
-efi_status_t efi_var_restore(struct efi_var_file *buf); +efi_status_t efi_var_restore(struct efi_var_file *buf, bool safe);
/**
- efi_var_from_file() - read variables from file
diff --git a/lib/efi_loader/efi_var_common.c b/lib/efi_loader/efi_var_common.c index cf7afecd60..b0c5b672c5 100644 --- a/lib/efi_loader/efi_var_common.c +++ b/lib/efi_loader/efi_var_common.c @@ -32,10 +32,8 @@ static const struct efi_auth_var_name_type name_type[] = { {u"KEK", &efi_global_variable_guid, EFI_AUTH_VAR_KEK}, {u"db", &efi_guid_image_security_database, EFI_AUTH_VAR_DB}, {u"dbx", &efi_guid_image_security_database, EFI_AUTH_VAR_DBX},
/* not used yet {u"dbt", &efi_guid_image_security_database, EFI_AUTH_VAR_DBT}, {u"dbr", &efi_guid_image_security_database, EFI_AUTH_VAR_DBR},
*/ };
static bool efi_secure_boot;
diff --git a/lib/efi_loader/efi_var_file.c b/lib/efi_loader/efi_var_file.c index de076b8cbc..c7c6805ed0 100644 --- a/lib/efi_loader/efi_var_file.c +++ b/lib/efi_loader/efi_var_file.c @@ -148,9 +148,10 @@ error: #endif }
-efi_status_t efi_var_restore(struct efi_var_file *buf) +efi_status_t efi_var_restore(struct efi_var_file *buf, bool safe) { struct efi_var_entry *var, *last_var;
u16 *data; efi_status_t ret;
if (buf->reserved || buf->magic != EFI_VAR_FILE_MAGIC ||
@@ -160,21 +161,29 @@ efi_status_t efi_var_restore(struct efi_var_file *buf) return EFI_INVALID_PARAMETER; }
- var = buf->var; last_var = (struct efi_var_entry *)((u8 *)buf + buf->length);
- while (var < last_var) {
u16 *data = var->name + u16_strlen(var->name) + 1;
if (var->attr & EFI_VARIABLE_NON_VOLATILE && var->length) {
ret = efi_var_mem_ins(var->name, &var->guid, var->attr,
var->length, data, 0, NULL,
var->time);
if (ret != EFI_SUCCESS)
log_err("Failed to set EFI variable %ls\n",
var->name);
}
var = (struct efi_var_entry *)
ALIGN((uintptr_t)data + var->length, 8);
- for (var = buf->var; var < last_var;
var = (struct efi_var_entry *)
ALIGN((uintptr_t)data + var->length, 8)) {
data = var->name + u16_strlen(var->name) + 1;
/*
* Secure boot related and non-volatile variables shall only be
* restored from U-Boot's preseed.
*/
if (!safe &&
(efi_auth_var_get_type(var->name, &var->guid) !=
EFI_AUTH_VAR_NONE ||
!(var->attr & EFI_VARIABLE_NON_VOLATILE)))
continue;
if (!var->length)
continue;
ret = efi_var_mem_ins(var->name, &var->guid, var->attr,
var->length, data, 0, NULL,
var->time);
if (ret != EFI_SUCCESS)
} return EFI_SUCCESS; }log_err("Failed to set EFI variable %ls\n", var->name);
@@ -213,7 +222,7 @@ efi_status_t efi_var_from_file(void) log_err("Failed to load EFI variables\n"); goto error; }
- if (buf->length != len || efi_var_restore(buf) != EFI_SUCCESS)
- if (buf->length != len || efi_var_restore(buf, false) != EFI_SUCCESS) log_err("Invalid EFI variables file\n"); error: free(buf);
diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c index ba0874e9e7..a7d305ffbc 100644 --- a/lib/efi_loader/efi_variable.c +++ b/lib/efi_loader/efi_variable.c @@ -426,7 +426,7 @@ 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);
if (ret != EFI_SUCCESS) log_err("Invalid EFI variable seed\n"); }__efi_var_file_begin, true);
-- 2.30.2

On Fri, Aug 27, 2021 at 06:42:39AM +0200, Heinrich Schuchardt wrote:
On 8/27/21 6:12 AM, AKASHI Takahiro wrote:
On Thu, Aug 26, 2021 at 03:48:02PM +0200, Heinrich Schuchardt wrote:
The UEFI specification requires that the signature database may only be stored in tamper-resistant storage. So these variable may not be read from an unsigned file.
I don't have a strong opinion here, but it seems to be too restrictive. Nobody expects that file-based variable implementation is *safe*. Leave it as it is so that people can easily experiment secure boot.
If the prior boot stage checks the integrity of the U-Boot binary, the file based implementation becomes 'safe' with this patch.
How safe (or secure) is it? That is a question. What is your thread model?
-Takahiro Akashi
Users can still experiment with secure boot by setting the secure boot variables via the efidebug command.
I cannot see a use case for having the secure boot data base on an insecure medium.
Best regards
Heinrich
-Takahiro Akashi
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com
v2: no change
include/efi_variable.h | 5 +++- lib/efi_loader/efi_var_common.c | 2 -- lib/efi_loader/efi_var_file.c | 41 ++++++++++++++++++++------------- lib/efi_loader/efi_variable.c | 2 +- 4 files changed, 30 insertions(+), 20 deletions(-)
diff --git a/include/efi_variable.h b/include/efi_variable.h index 4623a64142..2d97655e1f 100644 --- a/include/efi_variable.h +++ b/include/efi_variable.h @@ -161,10 +161,13 @@ efi_status_t __maybe_unused efi_var_collect(struct efi_var_file **bufp, loff_t * /**
- efi_var_restore() - restore EFI variables from buffer
- Only if @safe is set secure boot related variables will be restored.
- @buf: buffer
*/
- @safe: restoring from tamper-resistant storage
- Return: status code
-efi_status_t efi_var_restore(struct efi_var_file *buf); +efi_status_t efi_var_restore(struct efi_var_file *buf, bool safe);
/**
- efi_var_from_file() - read variables from file
diff --git a/lib/efi_loader/efi_var_common.c b/lib/efi_loader/efi_var_common.c index cf7afecd60..b0c5b672c5 100644 --- a/lib/efi_loader/efi_var_common.c +++ b/lib/efi_loader/efi_var_common.c @@ -32,10 +32,8 @@ static const struct efi_auth_var_name_type name_type[] = { {u"KEK", &efi_global_variable_guid, EFI_AUTH_VAR_KEK}, {u"db", &efi_guid_image_security_database, EFI_AUTH_VAR_DB}, {u"dbx", &efi_guid_image_security_database, EFI_AUTH_VAR_DBX},
/* not used yet {u"dbt", &efi_guid_image_security_database, EFI_AUTH_VAR_DBT}, {u"dbr", &efi_guid_image_security_database, EFI_AUTH_VAR_DBR},
*/ };
static bool efi_secure_boot;
diff --git a/lib/efi_loader/efi_var_file.c b/lib/efi_loader/efi_var_file.c index de076b8cbc..c7c6805ed0 100644 --- a/lib/efi_loader/efi_var_file.c +++ b/lib/efi_loader/efi_var_file.c @@ -148,9 +148,10 @@ error: #endif }
-efi_status_t efi_var_restore(struct efi_var_file *buf) +efi_status_t efi_var_restore(struct efi_var_file *buf, bool safe) { struct efi_var_entry *var, *last_var;
u16 *data; efi_status_t ret;
if (buf->reserved || buf->magic != EFI_VAR_FILE_MAGIC ||
@@ -160,21 +161,29 @@ efi_status_t efi_var_restore(struct efi_var_file *buf) return EFI_INVALID_PARAMETER; }
- var = buf->var; last_var = (struct efi_var_entry *)((u8 *)buf + buf->length);
- while (var < last_var) {
u16 *data = var->name + u16_strlen(var->name) + 1;
if (var->attr & EFI_VARIABLE_NON_VOLATILE && var->length) {
ret = efi_var_mem_ins(var->name, &var->guid, var->attr,
var->length, data, 0, NULL,
var->time);
if (ret != EFI_SUCCESS)
log_err("Failed to set EFI variable %ls\n",
var->name);
}
var = (struct efi_var_entry *)
ALIGN((uintptr_t)data + var->length, 8);
- for (var = buf->var; var < last_var;
var = (struct efi_var_entry *)
ALIGN((uintptr_t)data + var->length, 8)) {
data = var->name + u16_strlen(var->name) + 1;
/*
* Secure boot related and non-volatile variables shall only be
* restored from U-Boot's preseed.
*/
if (!safe &&
(efi_auth_var_get_type(var->name, &var->guid) !=
EFI_AUTH_VAR_NONE ||
!(var->attr & EFI_VARIABLE_NON_VOLATILE)))
continue;
if (!var->length)
continue;
ret = efi_var_mem_ins(var->name, &var->guid, var->attr,
var->length, data, 0, NULL,
var->time);
if (ret != EFI_SUCCESS)
} return EFI_SUCCESS; }log_err("Failed to set EFI variable %ls\n", var->name);
@@ -213,7 +222,7 @@ efi_status_t efi_var_from_file(void) log_err("Failed to load EFI variables\n"); goto error; }
- if (buf->length != len || efi_var_restore(buf) != EFI_SUCCESS)
- if (buf->length != len || efi_var_restore(buf, false) != EFI_SUCCESS) log_err("Invalid EFI variables file\n"); error: free(buf);
diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c index ba0874e9e7..a7d305ffbc 100644 --- a/lib/efi_loader/efi_variable.c +++ b/lib/efi_loader/efi_variable.c @@ -426,7 +426,7 @@ 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);
if (ret != EFI_SUCCESS) log_err("Invalid EFI variable seed\n"); }__efi_var_file_begin, true);
-- 2.30.2

On Fri, Aug 27, 2021 at 01:49:41PM +0900, AKASHI Takahiro wrote:
On Fri, Aug 27, 2021 at 06:42:39AM +0200, Heinrich Schuchardt wrote:
On 8/27/21 6:12 AM, AKASHI Takahiro wrote:
On Thu, Aug 26, 2021 at 03:48:02PM +0200, Heinrich Schuchardt wrote:
The UEFI specification requires that the signature database may only be stored in tamper-resistant storage. So these variable may not be read from an unsigned file.
I don't have a strong opinion here, but it seems to be too restrictive. Nobody expects that file-based variable implementation is *safe*. Leave it as it is so that people can easily experiment secure boot.
If the prior boot stage checks the integrity of the U-Boot binary, the file based implementation becomes 'safe' with this patch.
How safe (or secure) is it? That is a question. What is your thread model?
Obviously, thread -> threat
-Takahiro Akashi
Users can still experiment with secure boot by setting the secure boot variables via the efidebug command.
I cannot see a use case for having the secure boot data base on an insecure medium.
Best regards
Heinrich
-Takahiro Akashi
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com
v2: no change
include/efi_variable.h | 5 +++- lib/efi_loader/efi_var_common.c | 2 -- lib/efi_loader/efi_var_file.c | 41 ++++++++++++++++++++------------- lib/efi_loader/efi_variable.c | 2 +- 4 files changed, 30 insertions(+), 20 deletions(-)
diff --git a/include/efi_variable.h b/include/efi_variable.h index 4623a64142..2d97655e1f 100644 --- a/include/efi_variable.h +++ b/include/efi_variable.h @@ -161,10 +161,13 @@ efi_status_t __maybe_unused efi_var_collect(struct efi_var_file **bufp, loff_t * /**
- efi_var_restore() - restore EFI variables from buffer
- Only if @safe is set secure boot related variables will be restored.
- @buf: buffer
*/
- @safe: restoring from tamper-resistant storage
- Return: status code
-efi_status_t efi_var_restore(struct efi_var_file *buf); +efi_status_t efi_var_restore(struct efi_var_file *buf, bool safe);
/**
- efi_var_from_file() - read variables from file
diff --git a/lib/efi_loader/efi_var_common.c b/lib/efi_loader/efi_var_common.c index cf7afecd60..b0c5b672c5 100644 --- a/lib/efi_loader/efi_var_common.c +++ b/lib/efi_loader/efi_var_common.c @@ -32,10 +32,8 @@ static const struct efi_auth_var_name_type name_type[] = { {u"KEK", &efi_global_variable_guid, EFI_AUTH_VAR_KEK}, {u"db", &efi_guid_image_security_database, EFI_AUTH_VAR_DB}, {u"dbx", &efi_guid_image_security_database, EFI_AUTH_VAR_DBX},
/* not used yet {u"dbt", &efi_guid_image_security_database, EFI_AUTH_VAR_DBT}, {u"dbr", &efi_guid_image_security_database, EFI_AUTH_VAR_DBR},
*/ };
static bool efi_secure_boot;
diff --git a/lib/efi_loader/efi_var_file.c b/lib/efi_loader/efi_var_file.c index de076b8cbc..c7c6805ed0 100644 --- a/lib/efi_loader/efi_var_file.c +++ b/lib/efi_loader/efi_var_file.c @@ -148,9 +148,10 @@ error: #endif }
-efi_status_t efi_var_restore(struct efi_var_file *buf) +efi_status_t efi_var_restore(struct efi_var_file *buf, bool safe) { struct efi_var_entry *var, *last_var;
u16 *data; efi_status_t ret;
if (buf->reserved || buf->magic != EFI_VAR_FILE_MAGIC ||
@@ -160,21 +161,29 @@ efi_status_t efi_var_restore(struct efi_var_file *buf) return EFI_INVALID_PARAMETER; }
- var = buf->var; last_var = (struct efi_var_entry *)((u8 *)buf + buf->length);
- while (var < last_var) {
u16 *data = var->name + u16_strlen(var->name) + 1;
if (var->attr & EFI_VARIABLE_NON_VOLATILE && var->length) {
ret = efi_var_mem_ins(var->name, &var->guid, var->attr,
var->length, data, 0, NULL,
var->time);
if (ret != EFI_SUCCESS)
log_err("Failed to set EFI variable %ls\n",
var->name);
}
var = (struct efi_var_entry *)
ALIGN((uintptr_t)data + var->length, 8);
- for (var = buf->var; var < last_var;
var = (struct efi_var_entry *)
ALIGN((uintptr_t)data + var->length, 8)) {
data = var->name + u16_strlen(var->name) + 1;
/*
* Secure boot related and non-volatile variables shall only be
* restored from U-Boot's preseed.
*/
if (!safe &&
(efi_auth_var_get_type(var->name, &var->guid) !=
EFI_AUTH_VAR_NONE ||
!(var->attr & EFI_VARIABLE_NON_VOLATILE)))
continue;
if (!var->length)
continue;
ret = efi_var_mem_ins(var->name, &var->guid, var->attr,
var->length, data, 0, NULL,
var->time);
if (ret != EFI_SUCCESS)
} return EFI_SUCCESS; }log_err("Failed to set EFI variable %ls\n", var->name);
@@ -213,7 +222,7 @@ efi_status_t efi_var_from_file(void) log_err("Failed to load EFI variables\n"); goto error; }
- if (buf->length != len || efi_var_restore(buf) != EFI_SUCCESS)
- if (buf->length != len || efi_var_restore(buf, false) != EFI_SUCCESS) log_err("Invalid EFI variables file\n"); error: free(buf);
diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c index ba0874e9e7..a7d305ffbc 100644 --- a/lib/efi_loader/efi_variable.c +++ b/lib/efi_loader/efi_variable.c @@ -426,7 +426,7 @@ 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);
if (ret != EFI_SUCCESS) log_err("Invalid EFI variable seed\n"); }__efi_var_file_begin, true);
-- 2.30.2

On 8/27/21 6:49 AM, AKASHI Takahiro wrote:
On Fri, Aug 27, 2021 at 06:42:39AM +0200, Heinrich Schuchardt wrote:
On 8/27/21 6:12 AM, AKASHI Takahiro wrote:
On Thu, Aug 26, 2021 at 03:48:02PM +0200, Heinrich Schuchardt wrote:
The UEFI specification requires that the signature database may only be stored in tamper-resistant storage. So these variable may not be read from an unsigned file.
I don't have a strong opinion here, but it seems to be too restrictive. Nobody expects that file-based variable implementation is *safe*. Leave it as it is so that people can easily experiment secure boot.
If the prior boot stage checks the integrity of the U-Boot binary, the file based implementation becomes 'safe' with this patch.
How safe (or secure) is it? That is a question. What is your thread model?
The preseed store is as safe as the capsule updates that Linaro is working on where the certificate for verifying the capsule is baked into U-Boot or the StMM based variables.
They all require that an attacker can neither load a manipulated U-Boot nor that he can alter the memory containing U-Boot at runtime.
Best regards
Heinrich
-Takahiro Akashi
Users can still experiment with secure boot by setting the secure boot variables via the efidebug command.
I cannot see a use case for having the secure boot data base on an insecure medium.
Best regards
Heinrich
-Takahiro Akashi
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com
v2: no change
include/efi_variable.h | 5 +++- lib/efi_loader/efi_var_common.c | 2 -- lib/efi_loader/efi_var_file.c | 41 ++++++++++++++++++++------------- lib/efi_loader/efi_variable.c | 2 +- 4 files changed, 30 insertions(+), 20 deletions(-)
diff --git a/include/efi_variable.h b/include/efi_variable.h index 4623a64142..2d97655e1f 100644 --- a/include/efi_variable.h +++ b/include/efi_variable.h @@ -161,10 +161,13 @@ efi_status_t __maybe_unused efi_var_collect(struct efi_var_file **bufp, loff_t * /** * efi_var_restore() - restore EFI variables from buffer *
- Only if @safe is set secure boot related variables will be restored.
- @buf: buffer
- @safe: restoring from tamper-resistant storage
*/
- Return: status code
-efi_status_t efi_var_restore(struct efi_var_file *buf); +efi_status_t efi_var_restore(struct efi_var_file *buf, bool safe);
/** * efi_var_from_file() - read variables from file diff --git a/lib/efi_loader/efi_var_common.c b/lib/efi_loader/efi_var_common.c index cf7afecd60..b0c5b672c5 100644 --- a/lib/efi_loader/efi_var_common.c +++ b/lib/efi_loader/efi_var_common.c @@ -32,10 +32,8 @@ static const struct efi_auth_var_name_type name_type[] = { {u"KEK", &efi_global_variable_guid, EFI_AUTH_VAR_KEK}, {u"db", &efi_guid_image_security_database, EFI_AUTH_VAR_DB}, {u"dbx", &efi_guid_image_security_database, EFI_AUTH_VAR_DBX},
/* not used yet {u"dbt", &efi_guid_image_security_database, EFI_AUTH_VAR_DBT}, {u"dbr", &efi_guid_image_security_database, EFI_AUTH_VAR_DBR},
*/ };
static bool efi_secure_boot;
diff --git a/lib/efi_loader/efi_var_file.c b/lib/efi_loader/efi_var_file.c index de076b8cbc..c7c6805ed0 100644 --- a/lib/efi_loader/efi_var_file.c +++ b/lib/efi_loader/efi_var_file.c @@ -148,9 +148,10 @@ error: #endif }
-efi_status_t efi_var_restore(struct efi_var_file *buf) +efi_status_t efi_var_restore(struct efi_var_file *buf, bool safe) { struct efi_var_entry *var, *last_var;
u16 *data; efi_status_t ret;
if (buf->reserved || buf->magic != EFI_VAR_FILE_MAGIC ||
@@ -160,21 +161,29 @@ efi_status_t efi_var_restore(struct efi_var_file *buf) return EFI_INVALID_PARAMETER; }
- var = buf->var; last_var = (struct efi_var_entry *)((u8 *)buf + buf->length);
- while (var < last_var) {
u16 *data = var->name + u16_strlen(var->name) + 1;
if (var->attr & EFI_VARIABLE_NON_VOLATILE && var->length) {
ret = efi_var_mem_ins(var->name, &var->guid, var->attr,
var->length, data, 0, NULL,
var->time);
if (ret != EFI_SUCCESS)
log_err("Failed to set EFI variable %ls\n",
var->name);
}
var = (struct efi_var_entry *)
ALIGN((uintptr_t)data + var->length, 8);
- for (var = buf->var; var < last_var;
var = (struct efi_var_entry *)
ALIGN((uintptr_t)data + var->length, 8)) {
data = var->name + u16_strlen(var->name) + 1;
/*
* Secure boot related and non-volatile variables shall only be
* restored from U-Boot's preseed.
*/
if (!safe &&
(efi_auth_var_get_type(var->name, &var->guid) !=
EFI_AUTH_VAR_NONE ||
!(var->attr & EFI_VARIABLE_NON_VOLATILE)))
continue;
if (!var->length)
continue;
ret = efi_var_mem_ins(var->name, &var->guid, var->attr,
var->length, data, 0, NULL,
var->time);
if (ret != EFI_SUCCESS)
} return EFI_SUCCESS; }log_err("Failed to set EFI variable %ls\n", var->name);
@@ -213,7 +222,7 @@ efi_status_t efi_var_from_file(void) log_err("Failed to load EFI variables\n"); goto error; }
- if (buf->length != len || efi_var_restore(buf) != EFI_SUCCESS)
- if (buf->length != len || efi_var_restore(buf, false) != EFI_SUCCESS) log_err("Invalid EFI variables file\n"); error: free(buf);
diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c index ba0874e9e7..a7d305ffbc 100644 --- a/lib/efi_loader/efi_variable.c +++ b/lib/efi_loader/efi_variable.c @@ -426,7 +426,7 @@ 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);
}__efi_var_file_begin, true); if (ret != EFI_SUCCESS) log_err("Invalid EFI variable seed\n");
-- 2.30.2

Variable PK must be deleted when switching either to setup mode or to audit mode. Variable AuditMode must be writable in setup mode and user mode. Variable DeployedMode must only be writable in user mode; simplify the logic.
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com --- v2: no change --- lib/efi_loader/efi_var_common.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-)
diff --git a/lib/efi_loader/efi_var_common.c b/lib/efi_loader/efi_var_common.c index b0c5b672c5..63ad6fea9e 100644 --- a/lib/efi_loader/efi_var_common.c +++ b/lib/efi_loader/efi_var_common.c @@ -240,7 +240,7 @@ static efi_status_t efi_set_secure_state(u8 secure_boot, u8 setup_mode, goto err;
ret = efi_set_variable_int(L"AuditMode", &efi_global_variable_guid, - audit_mode || setup_mode ? + audit_mode || deployed_mode ? attributes_ro : attributes_rw, sizeof(audit_mode), &audit_mode, false); if (ret != EFI_SUCCESS) @@ -248,7 +248,7 @@ static efi_status_t efi_set_secure_state(u8 secure_boot, u8 setup_mode,
ret = efi_set_variable_int(L"DeployedMode", &efi_global_variable_guid, - audit_mode || deployed_mode || setup_mode ? + deployed_mode || setup_mode ? attributes_ro : attributes_rw, sizeof(deployed_mode), &deployed_mode, false); @@ -273,17 +273,20 @@ static efi_status_t efi_transfer_secure_state(enum efi_secure_mode mode) EFI_PRINT("Switching secure state from %d to %d\n", efi_secure_mode, mode);
- if (mode == EFI_MODE_DEPLOYED) { - ret = efi_set_secure_state(1, 0, 0, 1); - if (ret != EFI_SUCCESS) - goto err; - } else if (mode == EFI_MODE_AUDIT) { + if (mode == EFI_MODE_SETUP || mode == EFI_MODE_AUDIT) { ret = efi_set_variable_int(L"PK", &efi_global_variable_guid, EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS, 0, NULL, false); + if (ret != EFI_NOT_FOUND && ret != EFI_SUCCESS) + goto err; + } + + if (mode == EFI_MODE_DEPLOYED) { + ret = efi_set_secure_state(1, 0, 0, 1); if (ret != EFI_SUCCESS) goto err; + } else if (mode == EFI_MODE_AUDIT) {
ret = efi_set_secure_state(0, 1, 1, 0); if (ret != EFI_SUCCESS)

Writing variables AuditMode or Deployed Mode must update the secure boot state.
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@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) 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(); 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);

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@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.
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
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

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@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:
- Enroll PKpub
- Platform Specific PKpub Clear/Delete PKpub
- Audit := 1
- DeployedMode := 1
- 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

On Fri, Aug 27, 2021 at 06:09:25AM +0200, Heinrich Schuchardt wrote:
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@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:
- Enroll PKpub
- Platform Specific PKpub Clear/Delete PKpub
- Audit := 1
- DeployedMode := 1
- 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:
- As we are on a single threaded system we can use a static state variable. This is the approach in patch 1.
- We can add a parameter to efi_set_variable_int() indicating that the variable change shall not have side effects.
- 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?
I found no support for the DeployedMode variable in the current upstream
Where is it coded?
It seems to be spread around EDK2 sources.
SecurityPkg/Library/AuthVariableLib/AuthVariableLib.c SecurityPkg/Library/AuthVariableLib/AuthService.c contain some info and are included in the source we use for the StandaloneMM that we run from op-tee.
There's more references for SetupMode in EDK2 (in files that aren't included in StMM) and commit 560ac77ea155 seems to have removed the support of what you are looking for.
In any case I think the state machine and the logic should just be coded in one place in U-Boot, regardless of the variable backend, and strictly use StMM for storing/reading/authenticating variables.
Regards /Ilias
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

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(); + + return EFI_SUCCESS; }

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.
- Takahiro Akashi
- return EFI_SUCCESS;
}
2.30.2

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:
- there is no "variable store" file available.
- 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

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@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:
- there is no "variable store" file available.
- 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.
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

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@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:
- there is no "variable store" file available.
- 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

On Thu, Aug 26, 2021 at 03:47:59PM +0200, Heinrich Schuchardt wrote:
The UEFI specification 2.9 defines the different modes that secure boot may be in.
The patch series adds support for the "Deployed Mode" and the "Setup Mode".
This sentence seems to be wrong, or at least inaccurate. "Setup Mode" has been supported from the beginning when I implemented secure boot. In other word, I implemented only the transition between "Setup Mode" and "User Mode" only.
-Takahiro Akashi
Furthermore the secure boot signature database must only be loaded from tamper-resistant storage. So we must not load it from ubootefi.var on the EFI system partition but only from the preseed variables store or via the OP-TEE driver for the eMMC replay protected memory partition.
v2: correct variable name in lib/efi_loader/efi_variable_tee.c
Heinrich Schuchardt (6): efi_loader: stop recursion in efi_init_secure_state efi_loader: correct determination of secure boot state efi_loader: don't load signature database from file efi_loader: correct secure boot state transition efi_loader: writing AuditMode, DeployedMode efi_loader: always initialize the secure boot state
include/efi_variable.h | 6 ++- lib/efi_loader/efi_var_common.c | 66 +++++++++++++++++++++++-------- lib/efi_loader/efi_var_file.c | 41 +++++++++++-------- lib/efi_loader/efi_variable.c | 20 ++++++---- lib/efi_loader/efi_variable_tee.c | 4 +- 5 files changed, 95 insertions(+), 42 deletions(-)
-- 2.30.2
participants (4)
-
AKASHI Takahiro
-
Heinrich Schuchardt
-
Heinrich Schuchardt
-
Ilias Apalodimas