Re: [PATCH v2 5/6] efi_loader: writing AuditMode, DeployedMode
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
> > > ---
> > > 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", _guid_image_security_database, EFI_AUTH_VAR_DBX},
> > > {u"dbt", _guid_image_security_database, EFI_AUTH_VAR_DBT},
> > > {u"dbr", _guid_image_security_database, EFI_AUTH_VAR_DBR},
> > > + {u"AuditMode", _global_variable_guid, EFI_AUTH_MODE},
> > > + {u"DeployedMode", _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
Re: [PATCH v2 5/6] efi_loader: writing AuditMode, DeployedMode
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
---
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", _guid_image_security_database, EFI_AUTH_VAR_DBX},
{u"dbt", _guid_image_security_database, EFI_AUTH_VAR_DBT},
{u"dbr", _guid_image_security_database, EFI_AUTH_VAR_DBR},
+ {u"AuditMode", _global_variable_guid, EFI_AUTH_MODE},
+ {u"DeployedMode", _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
Re: [PATCH v2 5/6] efi_loader: writing AuditMode, DeployedMode
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
> ---
> 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", _guid_image_security_database, EFI_AUTH_VAR_DBX},
> {u"dbt", _guid_image_security_database, EFI_AUTH_VAR_DBT},
> {u"dbr", _guid_image_security_database, EFI_AUTH_VAR_DBR},
> + {u"AuditMode", _global_variable_guid, EFI_AUTH_MODE},
> + {u"DeployedMode", _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
