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 889FAC432BE for ; Fri, 27 Aug 2021 03:06:00 +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 9974C60EFF for ; Fri, 27 Aug 2021 03:05:59 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 9974C60EFF 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 49CC28322E; Fri, 27 Aug 2021 05:05:57 +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="cWupIWj5"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 8987583231; Fri, 27 Aug 2021 05:05:55 +0200 (CEST) Received: from mail-pl1-x62f.google.com (mail-pl1-x62f.google.com [IPv6:2607:f8b0:4864:20::62f]) (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 B179783210 for ; Fri, 27 Aug 2021 05:05:50 +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=takahiro.akashi@linaro.org Received: by mail-pl1-x62f.google.com with SMTP id u15so3021532plg.13 for ; Thu, 26 Aug 2021 20:05:50 -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:mail-followup-to:references :mime-version:content-disposition:in-reply-to; bh=tjI2DawGT1dIWUMcWYJLEeatVowQJTJxrDEOIradOak=; b=cWupIWj5p8VCw7V2YXq5aoReeJitb6m9IUeWCi+SVSJkWESGryu7q0ZG44MVqYCiE8 almLDj6o0VPXT30hFJPDF0/0d9cWgnTwluD7FpLl37ZkuZhvPqXMcu6bNOYq1w10H1iP 16XtElLPex0NfV8JOpLXud02uakUjtqanvNPbNdVzEiG8ffK0k0xvreNuU80sK8fv9zL rff6fbM+KGHh8TA/IukRIAIG595BNLDyzDOOtLuJ5lcFA8nUIEVdNzUYd96Ywf2NXYj4 nUrFleqPHZfAXvDfzftKIiqyZJcS4m0ycJ7s5Do6Bed/TJFDV1Of/Sw9iHwpw+TYjMJU LlUw== 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 :mail-followup-to:references:mime-version:content-disposition :in-reply-to; bh=tjI2DawGT1dIWUMcWYJLEeatVowQJTJxrDEOIradOak=; b=iiIlVyaBcrskRQv+ET3RZ4t8z27uW5ceLKQFLyNcuSiwO9fdVVBfP1m6RrdZJ/sbwS 02EPvgvhJYlA6xA8cPDTgg/Fr+coASGnACCkPqMPSRJUgiCYPCOQDe3yTEEOH+GG0i4a 8OP+qqq5JN4N8HoNM51cHqNjBqF+7jpLDLtYsirAeTSPtWvEndiTJqm/cruAf0j+QVc0 x09ZieCNOdHrQR1mbyrKKa6RsCnjkHOiGd43uFWoBC+16lyhNFJfRoEa72/WDB3Z7Giy cVo+wQQj3QZ4BDM/zlmm9hLZKQV0wxRGdoUI612UtmUMDyjPEk6P+JQRtz6sJld0Xn+r WvgQ== X-Gm-Message-State: AOAM5309CtTyep0Gl3m/JPhYscW3AXoULm7WOvmFGN74UshcnsvbhHP0 6WPm6QA+DEO0XR4OnbNlMIUhTg== X-Google-Smtp-Source: ABdhPJwyDjLxarnpmjtsHlYXpJA/MDr8jQjYAIeHQzdP04iPIC3DNgDfaxrcEjgsbGVnKvYEqYfFPw== X-Received: by 2002:a17:90a:4d8d:: with SMTP id m13mr7955023pjh.190.1630033548753; Thu, 26 Aug 2021 20:05:48 -0700 (PDT) Received: from laputa (p784a6698.tkyea130.ap.so-net.ne.jp. [120.74.102.152]) by smtp.gmail.com with ESMTPSA id n41sm4579511pfv.43.2021.08.26.20.05.46 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 26 Aug 2021 20:05:48 -0700 (PDT) Date: Fri, 27 Aug 2021 12:05:44 +0900 From: AKASHI Takahiro To: Heinrich Schuchardt Cc: u-boot@lists.denx.de, Alexander Graf , Ilias Apalodimas , Heinrich Schuchardt Subject: Re: [PATCH v2 5/6] efi_loader: writing AuditMode, DeployedMode Message-ID: <20210827030544.GB52912@laputa> Mail-Followup-To: AKASHI Takahiro , Heinrich Schuchardt , u-boot@lists.denx.de, Alexander Graf , Ilias Apalodimas , Heinrich Schuchardt References: <20210826134805.148975-1-heinrich.schuchardt@canonical.com> <20210826134805.148975-6-heinrich.schuchardt@canonical.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210826134805.148975-6-heinrich.schuchardt@canonical.com> 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 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. > 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 >