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 7053DC433EF for ; Mon, 6 Sep 2021 00:13:14 +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 7681860EFD for ; Mon, 6 Sep 2021 00:13:13 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 7681860EFD 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 D767282DB0; Mon, 6 Sep 2021 02:13:10 +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="IcavwmV0"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 7BAB382E03; Mon, 6 Sep 2021 02:13:09 +0200 (CEST) Received: from mail-pf1-x42e.google.com (mail-pf1-x42e.google.com [IPv6:2607:f8b0:4864:20::42e]) (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 C19DB82D92 for ; Mon, 6 Sep 2021 02:13:05 +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-pf1-x42e.google.com with SMTP id s29so4284939pfw.5 for ; Sun, 05 Sep 2021 17:13:05 -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=KCxg/YJPA0+XQ5oT3esPIRQjrmi0QQi7Hz6t7mpu+3I=; b=IcavwmV0YuNlYtallCF/qVcBxtr0L4Jar7VhoEPpSGx2YXQw9XmT5WJpJOrom466/C Zm73wJQNRUr+hLVCtFoLtHTSlFDv9JHiDW2afIBEU8gxVb9aSaGDeZB7bVwCIoZwPYgZ teU5woy5aWWXuRNfI3MNHU0B83LDyf2A+OCyJONHBmxZy7wNsPT4dgFI3qYbJytLhjzc n5JCOQjpDTkA3OlRAr6KEuSvdFeE5sysup+Ke9zuRCGei/cplUXN7WR+9J95XbuWiROE W+pv3fq+W/RsDEM8Nn28iVBqlCFEa3zLo8Pe+HLq6+vwp9zHCIuD5eKFpmFJTBeCBx0j sX2A== 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=KCxg/YJPA0+XQ5oT3esPIRQjrmi0QQi7Hz6t7mpu+3I=; b=B/qPLN4cPr0QY8B+J107IsIYwMssIlyQp2jWyO7CYnrAZlmVT7mv1PsXncaol9ao+g 3wrbfus8IiAmTxUCHGOHcjq8mQ5E0W3qmeeckrZyk3KFLhCZBdEJ7h8keqV8fVelRXb1 rbvduXRKrtMf8c9cMUdUJq+tV+pMkl+qCXRJyHhKhT6Y5y6EP9DIRF69WHeof9a/g8da iJFCKfeofkdoXj7pqlufx0REUTOC0FfBeLpiIgd5E9ZRK6csNDGNRNLIcQewcNigppQU zPzZrm8MN1eyr2+NskRz82+R4lT2+0f0/aATniDhgv4PRUh1FL0jIgmK+CbjP/fxULc0 hvxA== X-Gm-Message-State: AOAM531KNJz3C8kTIuTZP1nKadVX+w/oKJNCnPvK8Y84f0OIYwoThZU3 kFnVR2RYpuWTDQQcL8fre+3pXw== X-Google-Smtp-Source: ABdhPJy5ama5zaG324bPg1XnumXMZmlOZbEZpB1GDP/CUkBiiUtz/FL7Y/SorYdeBxXyg1PUpaRoLA== X-Received: by 2002:a63:9d06:: with SMTP id i6mr9661198pgd.199.1630887183920; Sun, 05 Sep 2021 17:13:03 -0700 (PDT) Received: from laputa (p784a2304.tkyea130.ap.so-net.ne.jp. [120.74.35.4]) by smtp.gmail.com with ESMTPSA id j14sm5401489pjg.29.2021.09.05.17.13.01 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 05 Sep 2021 17:13:03 -0700 (PDT) Date: Mon, 6 Sep 2021 09:12:59 +0900 From: AKASHI Takahiro To: Heinrich Schuchardt Cc: u-boot@lists.denx.de, Heinrich Schuchardt , Alexander Graf , Ilias Apalodimas Subject: Re: [PATCH v3 1/3] efi_loader: don't load signature database from file Message-ID: <20210906001259.GA40187@laputa> Mail-Followup-To: AKASHI Takahiro , Heinrich Schuchardt , u-boot@lists.denx.de, Heinrich Schuchardt , Alexander Graf , Ilias Apalodimas References: <20210902093531.17883-1-heinrich.schuchardt@canonical.com> <20210902093531.17883-2-heinrich.schuchardt@canonical.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210902093531.17883-2-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, Sep 02, 2021 at 11:35:29AM +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. Even with TF-A (or other methods) assumed, I think the behavior here is too restrictive. If you think TF-A provides the base for "chain of trust", all what you need to do is to protect PK and all the other auth variables should be allowed to be restored even from an unsafe file because the changes of values will be verified anyway by UEFI system as long as SetVariable() is used. Please think about why UEFI specification defines both PK and KEK. The ability of setting/modifying KEK will add more flexibility of system configuration. -Takahiro Akashi > Signed-off-by: Heinrich Schuchardt > --- > 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 3d92afe2eb..005c03ea5f 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"); > } > -- > 2.32.0 >