From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-15.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 9F881C432BE for ; Fri, 27 Aug 2021 09:23:27 +0000 (UTC) Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id D997760EBD for ; Fri, 27 Aug 2021 09:23:26 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org D997760EBD Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linaro.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=lists.denx.de Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id C35278328E; Fri, 27 Aug 2021 11:23:24 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (2048-bit key; unprotected) header.d=linaro.org header.i=@linaro.org header.b="QnPPnFGv"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 2995B83290; Fri, 27 Aug 2021 11:23:22 +0200 (CEST) Received: from mail-wm1-x32d.google.com (mail-wm1-x32d.google.com [IPv6:2a00:1450:4864:20::32d]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id E4C0C8328B for ; Fri, 27 Aug 2021 11:23:16 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=ilias.apalodimas@linaro.org Received: by mail-wm1-x32d.google.com with SMTP id x2-20020a1c7c02000000b002e6f1f69a1eso8715276wmc.5 for ; Fri, 27 Aug 2021 02:23:16 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=wwDV/uZ4ylo4EUS0UDRdtviVwUgbm4t5v3Az3eb2y5A=; b=QnPPnFGvzqLN8FV8wW62kXdMjpRnJIQnXD+aBTKT5pACoEpuqp/dntIDBphXET2v3o TSLa+6J2M5hv4RDapkY1AIXG2VJg1ZZ0J+Hv4F5mdBvFcEQWzNL6/fkJ/BGZfswgPeOI LosjXmDXiErrPUHHB4449P3SMI2tpLJmzEuEuhCoaSTzFSdd8KPDQ4RIa1IErrTm94Or ywmnH8bG9CQWI4MzgnrhJP5Cb0mKjNQmwewuKcfWBtIKTdF0mW0SHlevCEe2kD5FtY8v iDCJgPkNQA+D20b8Z7q/w48PEImhM87/yV+M/B0sIFowlwYOYHSlsEwPSnVce5nOAxsq 9gHw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=wwDV/uZ4ylo4EUS0UDRdtviVwUgbm4t5v3Az3eb2y5A=; b=PUDb+1BDH8Fakdsezb42pYpnpr0TfSggNoGk+FwSZ4UjqBBwAGBJTpKTDvmnoJ0xnG OZv9MQJ+lAImOvtrBTIL0tN6d54Q25oAVK1nEjOnGRg+pBzdsmg7GRLUbqmG455vNANY 3hYrnBZvFvuNOh5UGX2gkMhckDUihNj5RWMe2u2F5YM7As/dxzzYZBl2TT0e6pX4jxNP +9KBw7LqvN0TMG5zSOrPK64ic1jBYHaerfQad1ETgXr+EffY/V1kxvLUsQeYILyEfjK6 UabFCBDAp1/NwSxC0EuwAeBSvZ7+Ma5aBsrsoMiYaiz0+jLiO7Is2OjaP4buRCl+3X1V kclA== X-Gm-Message-State: AOAM532d+cwaAGjhCEFocr7Y6QKgTv8FsAMULkxZhkbMNVcAktPFCEz+ DGff2Oks0O0vpuPmrM8YMI9JMw== X-Google-Smtp-Source: ABdhPJwh1ATlHu37XGUIoFAUfDj8QRgmQGiH383979Laq6LUQG/kUUl/Oo9uTpJbVBEkEDTnQaFGOQ== X-Received: by 2002:a7b:c442:: with SMTP id l2mr18687849wmi.131.1630056196351; Fri, 27 Aug 2021 02:23:16 -0700 (PDT) Received: from enceladus (ppp-94-66-250-220.home.otenet.gr. [94.66.250.220]) by smtp.gmail.com with ESMTPSA id q195sm5141117wme.37.2021.08.27.02.23.14 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 27 Aug 2021 02:23:15 -0700 (PDT) Date: Fri, 27 Aug 2021 12:23:12 +0300 From: Ilias Apalodimas To: Heinrich Schuchardt Cc: AKASHI Takahiro , Heinrich Schuchardt , Alexander Graf , u-boot@lists.denx.de Subject: Re: [PATCH v2 5/6] efi_loader: writing AuditMode, DeployedMode Message-ID: References: <20210826134805.148975-1-heinrich.schuchardt@canonical.com> <20210826134805.148975-6-heinrich.schuchardt@canonical.com> <20210827030544.GB52912@laputa> <4aa439e3-490b-5a0f-d43c-ac587ecc5ac3@gmx.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4aa439e3-490b-5a0f-d43c-ac587ecc5ac3@gmx.de> X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.34 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.2 at phobos.denx.de X-Virus-Status: Clean 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", &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? 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 > > > >