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,URIBL_BLOCKED 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 66F4AC48BE6 for ; Sat, 12 Jun 2021 00:14:08 +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 43E6E611C9 for ; Sat, 12 Jun 2021 00:14:06 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 43E6E611C9 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linaro.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id C87F780799; Sat, 12 Jun 2021 02:14:04 +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="Qk/f8LrH"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id E924D80808; Sat, 12 Jun 2021 02:14:02 +0200 (CEST) Received: from mail-pl1-x636.google.com (mail-pl1-x636.google.com [IPv6:2607:f8b0:4864:20::636]) (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 384258047F for ; Sat, 12 Jun 2021 02:13:59 +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-x636.google.com with SMTP id b12so3618598plg.11 for ; Fri, 11 Jun 2021 17:13:59 -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:content-transfer-encoding :in-reply-to; bh=ocydEQfxfQUh2XcoOK1NBG3QrlOQ8OpbB7xXrsmeDmA=; b=Qk/f8LrHdAnSRK0/kzTcIyDvhFdKK2DE5MD3YMB3PcPrPitsJ9p6dWiomUCoWw+ajJ IYvrqCuQkBux4gI2t6MNBYu6mABb0qCr49Jo8L2OnICpXf5gu6qCn73GC4ql8RfYhtYS 9mBY6BfbzQfvMYADYhTXVYJRZ0kPjJYMkCU3Sb7p/17XoqaKpSiuHOjybMZgNFUyeuLC ICQJkLfNyYUrTcw4XDTVAW6zV7xBN0ieMRJNQBQPMMVAoDRE4bWrGQS6joLLuB/jD9Wl tx4ImRPeiMa9jRRQS23Q5uKTvrYkdIjEqT4mBPsbLO06+beMcUp9ClnzU0eg2jTZhohB Ho+w== 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 :content-transfer-encoding:in-reply-to; bh=ocydEQfxfQUh2XcoOK1NBG3QrlOQ8OpbB7xXrsmeDmA=; b=nCnksa/EHo+VJMzo5EtcyvhrU9IAEs0H1b2wI4t6iTYNWhWLQN9BdUYsXen5nRLSGs ra2MeulMnTc8SoQD31GDLCpS492BDTZv61seDcjVcX8hWLgyqrh5HSy7bKK7+nrkQAXO Rh/ApigKQoYJa21A+1k0is8G6Ky5KS9U8ioA2M0ZfpnbJWg7tPbEkZQqgWh7pDlAQlLW V4Yu3QqdQXaAYuYLwed5wW2rhAXSEa6I0/6g1OTf4n2FYECRUfRdJYJaKBxXnhGutsPF z/5Xe40RPFcWq7YthInexrKMes0KLLyK/UY1xwYKRaEKZ02VbuXrS/D4uqdb9m4sJP/Y EzGA== X-Gm-Message-State: AOAM530qMC4enTVIbc14oNCcMekMRwWpY3iinPCkd+M0CLNOGD5/j2Vz 1YjE4p95LBMJPolWX6ONLpqs6g== X-Google-Smtp-Source: ABdhPJxQzd9NBc49/QiNSafk+hfWDoYDl+w2oAM3sKLsrexQlcBEU4C/gPLDS9YFGXAxwD/2NMwuYA== X-Received: by 2002:a17:90a:4e0b:: with SMTP id n11mr6956817pjh.155.1623456837340; Fri, 11 Jun 2021 17:13:57 -0700 (PDT) Received: from laputa (p3dd30534.tkyea130.ap.so-net.ne.jp. [61.211.5.52]) by smtp.gmail.com with ESMTPSA id i22sm6041874pfq.6.2021.06.11.17.13.54 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 11 Jun 2021 17:13:56 -0700 (PDT) Date: Sat, 12 Jun 2021 09:13:52 +0900 From: AKASHI Takahiro To: Heinrich Schuchardt Cc: Vincent Stehl?? , Grant Likely , Alexander Graf , Sughosh Ganu , Ilias Apalodimas , u-boot@lists.denx.de Subject: Re: [PATCH] efi_loader: check update capsule parameters Message-ID: <20210612001352.GA37144@laputa> Mail-Followup-To: AKASHI Takahiro , Heinrich Schuchardt , Vincent Stehl?? , Grant Likely , Alexander Graf , Sughosh Ganu , Ilias Apalodimas , u-boot@lists.denx.de References: <20210611161520.30315-1-vincent.stehle@arm.com> <822ab371-50b8-4279-4f8e-c29621bea7cf@gmx.de> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <822ab371-50b8-4279-4f8e-c29621bea7cf@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 Hi Vincent, Thank you for the update. On Fri, Jun 11, 2021 at 07:06:01PM +0200, Heinrich Schuchardt wrote: > Cc: Takahiro, Sughosh, Ilias > > On 6/11/21 6:15 PM, Vincent Stehlé wrote: > > UpdateCapsule() must return EFI_INVALID_PARAMETER in a number of cases, > > listed by the UEFI specification and tested by the SCT. Add a common > > function to do that. First of all, I would like to make clear one thing: My initial implementation of capsule support does *not* intend to support "UpdateCapsule" as runtime API. Instead, the sole objective is to provide "delivery of capsules via file" (or capsule on disk) method. This is because the initially-expected use case was that we would adopt capsules as an alternative for updating UEFI variables without using SetVariable runtime API. (This idea is no longer upheld, though.) This is why the API does not handle nor check parameters in the current code. I even regret to have added EFI_RUNTIME_UPDATE_CAPSULE option, which makes people confused. Nevertheless, I'm more than happy if other folks implement full semantics of UpdateCapsule. > > This fixes SCT UpdateCapsule_Conf failures. > > > > Reviewed-by: Grant Likely > > Signed-off-by: Vincent Stehlé > > Cc: Heinrich Schuchardt > > Cc: Alexander Graf > > --- > > include/efi_loader.h | 24 ++++++++++++++++++++++++ > > lib/efi_loader/efi_capsule.c | 8 ++++---- > > lib/efi_loader/efi_runtime.c | 8 ++++++++ > > 3 files changed, 36 insertions(+), 4 deletions(-) > > > > diff --git a/include/efi_loader.h b/include/efi_loader.h > > index 0a9c82a257e..426d1c72d7d 100644 > > --- a/include/efi_loader.h > > +++ b/include/efi_loader.h > > @@ -910,6 +910,30 @@ extern const struct efi_firmware_management_protocol efi_fmp_fit; > > extern const struct efi_firmware_management_protocol efi_fmp_raw; > > > > /* Capsule update */ > > +static inline efi_status_t > > +efi_valid_update_capsule_params(struct efi_capsule_header > > + **capsule_header_array, > > + efi_uintn_t capsule_count, > > + u64 scatter_gather_list) > > +{ > > + u32 flags; > > + > > + if (!capsule_count) > > + return EFI_INVALID_PARAMETER; > > If capsule count > 1, don't you have to check all capsules headers? > > > + > > + flags = capsule_header_array[0]->flags; > > + > > + if (((flags & CAPSULE_FLAGS_PERSIST_ACROSS_RESET) && > > + !scatter_gather_list) || > > + ((flags & CAPSULE_FLAGS_POPULATE_SYSTEM_TABLE) && > > + !(flags & CAPSULE_FLAGS_PERSIST_ACROSS_RESET)) || > > + ((flags & CAPSULE_FLAGS_INITIATE_RESET) && > > + !(flags & CAPSULE_FLAGS_PERSIST_ACROSS_RESET))) > > + return EFI_INVALID_PARAMETER; > > What happens if capsule(0) has CAPSULE_FLAGS_INITIATE_RESET and > capsule(4) has !CAPSULE_FLAGS_PERSIST_ACROSS_RESET? > > Best regards > > Heinrich > > > + > > + return EFI_SUCCESS; > > +} > > + > > efi_status_t EFIAPI efi_update_capsule( > > struct efi_capsule_header **capsule_header_array, > > efi_uintn_t capsule_count, > > diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c > > index 60309d4a07d..380cfd70290 100644 > > --- a/lib/efi_loader/efi_capsule.c > > +++ b/lib/efi_loader/efi_capsule.c > > @@ -442,12 +442,12 @@ efi_status_t EFIAPI efi_update_capsule( > > EFI_ENTRY("%p, %zu, %llu\n", capsule_header_array, capsule_count, > > scatter_gather_list); > > > > - if (!capsule_count) { > > - ret = EFI_INVALID_PARAMETER; > > + ret = efi_valid_update_capsule_params(capsule_header_array, > > + capsule_count, > > + scatter_gather_list); > > + if (ret != EFI_SUCCESS) > > goto out; > > - } > > > > - ret = EFI_SUCCESS; > > for (i = 0, capsule = *capsule_header_array; i < capsule_count; > > i++, capsule = *(++capsule_header_array)) { > > /* sanity check */ > > diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c > > index 93a695fc27e..449ad8b9f36 100644 > > --- a/lib/efi_loader/efi_runtime.c > > +++ b/lib/efi_loader/efi_runtime.c > > @@ -467,6 +467,14 @@ efi_status_t __efi_runtime EFIAPI efi_update_capsule_unsupported( > > efi_uintn_t capsule_count, > > u64 scatter_gather_list) > > { > > + efi_status_t ret; > > + > > + ret = efi_valid_update_capsule_params(capsule_header_array, > > + capsule_count, > > + scatter_gather_list); This is "efi_update_capsule_unsupported" function. We don't have to check parameters. -Takahiro Akashi > > + if (ret != EFI_SUCCESS) > > + return ret; > > + > > return EFI_UNSUPPORTED; > > } > > > > >