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=-12.2 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 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 7572FC48BE5 for ; Mon, 21 Jun 2021 06:04:59 +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 201BF60FE7 for ; Mon, 21 Jun 2021 06:04:55 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 201BF60FE7 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=gmx.de 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 4E3C280C67; Mon, 21 Jun 2021 08:04:53 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=fail (p=none dis=none) header.from=gmx.de Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (1024-bit key; secure) header.d=gmx.net header.i=@gmx.net header.b="Sx7V0FDy"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id D758E8291E; Mon, 21 Jun 2021 08:04:51 +0200 (CEST) Received: from mout.gmx.net (mout.gmx.net [212.227.17.22]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id F0688801A0 for ; Mon, 21 Jun 2021 08:04:48 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=gmx.de Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=xypron.glpk@gmx.de DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=gmx.net; s=badeba3b8450; t=1624255486; bh=/QnQeTAtj1dkfYPZCtW8LLB+UmptcuR8XMR8AfjKVcA=; h=X-UI-Sender-Class:Subject:To:References:Cc:From:Date:In-Reply-To; b=Sx7V0FDy5IwhGAJ0bM3SmJ972pDCTeh/2TskDELhU3TKk08z/pA9tz9Z5PVA/Uqh0 zSGcnAi4x9l49GpckgGWuyZ/mrUTv58KMTgjZK+TZu6G1UA9gEpWiZH4mAXg3mBXu3 CCjIGXZCusJVRr/bUnd57R5Ll18g2mL3ULKAF8GE= X-UI-Sender-Class: 01bb95c1-4bf8-414a-932a-4f6e2808ef9c Received: from [192.168.0.141] ([88.152.144.157]) by mail.gmx.net (mrgmx105 [212.227.17.168]) with ESMTPSA (Nemesis) id 1M4JmT-1lvU231XAP-000IGQ; Mon, 21 Jun 2021 08:04:46 +0200 Subject: Re: [PATCH] efi_loader: check update capsule parameters To: Vincent Stehle References: <20210611161520.30315-1-vincent.stehle@arm.com> <822ab371-50b8-4279-4f8e-c29621bea7cf@gmx.de> <20210612001352.GA37144@laputa> Cc: AKASHI Takahiro , u-boot@lists.denx.de, Grant Likely , Alexander Graf , Ilias Apalodimas , Sughosh Ganu From: Heinrich Schuchardt Message-ID: <8b6a96cb-8bdc-7f0e-4222-76c08d41f375@gmx.de> Date: Mon, 21 Jun 2021 08:04:42 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 MIME-Version: 1.0 In-Reply-To: <20210612001352.GA37144@laputa> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: quoted-printable X-Provags-ID: V03:K1:hZBuzByeykvuvGfQ3zN2nMKHSPqEtnk/CqTA9b6duj8yn7vxibT rPybcB7h+H3hD+T+zDYeO1Pkfu7z5siIIOh0JvfFTTScm7HIAHWMl2ubZQ2bNuEwUKVTpFr rksoQhT5oJPsY8gId5rpsxqO5XevlYB7zaBhCMN4RP4+fUPdeueG2Rbs8G/xllUYu6B7h60 pAMP644WIBcFS7+GJ/JYA== X-UI-Out-Filterresults: notjunk:1;V03:K0:vjfVgnu020M=:/aLdh7UUoM1BHRSmNwTsiC AG00NOOpwJLISGDalXXwZQ1+FRfUMIsufoU0NXmdp+K50L4T0hi2EGo4gqGVJGQInfQO2SMyO QCVo3WwjnvFy2MdUwL7liezxtvkzn0epCENrjT7PnAXwmKEG94utMtGeBk9e79UV3+UPfnKqu F+Ek2dhyfYm5k0SFOxjcc/OjCXbCXdz5FB7K23LVTamvPgRHSUskZtd1mr26E+xR0XGV5eykR Nt0mH/u6B3NRuWmyvddfryx42anllHPoSMwwg0HuVwFGVHqwjo/Nl2gQeOGqv3Coka3IYwXJK FbfFi+JvGt/LHdmtL+2eG49+EcMzdqalmES5KEADZFZ4Py1UVr65cJdRz/8YojS2qED+B03lG t23Z4UI0N6dqnQMFABWBscI8iCgOjA6SGBr4ukXzW5mAtgBWE0DqsAYGn9YoNNcQWiDkEd02l uQvpRDG2U9TuNLIVJlKVvZ3HXXpv1XjHVTCrvT0lX2ca8HZNDxB+d3vz+MLK7s6BNkCmlOznz wyKJcbYoWIFmXOduZf9AYnVZpnGAJH4cz6lxOqrbj/1JUkpH+iXuOWJTiWyAEf5b1x3ulQPwX am1NU9xbof0jEKrz+cLawerK3l6LcneIKJs7wn91EOTZAFGq3zd2z7Gtydm5bWbH+nXD67f52 kbtygvpJyJvsZsEyROegS5RrVpoqPx0FPuo8XLIznyeWoDs0HcKOYoP+qQfYkNl7ihjmcDFyz RfGcH7AJ3D4bgqc9Ei+WMGCRFV+iJHDe14wtII/Rn/lyiFAAptvW9vou6et5Lb4oJUWkny98F S5EmBG7NPxi4NL4z6XcWOtjd7I2fA0xjy5ZnVH0i6QOYPQYv7DhB/ITgLNkFJ+0kKF6pSlOq4 TGyN6Osx1oqUR4ulTqN11egW2XQS1zMSOac5jL2vG5pdLZLF7dyNDFvhufHS0DFsmkyG4CBt9 fMC/+ax44er9FaTB7B5tDUPfagRkpL3RZQ6MkhSn1VbZlcFAgu6IJgLFayNfZFfsHGs8aCezs pXtNet5dYoeiSCsm7oJ0THaZXyqJmm/YtgWnbnMSMEf0l84XAxTo9rjq0DQPHHjks9de+HIGn q4awCMYcian4d5KkbByONDm9LqcwaA6BqpD 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 6/12/21 2:13 AM, AKASHI Takahiro wrote: > 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=C3=A9 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=C3=A9 >>> 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_proto= col 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 =3D 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 =3D EFI_INVALID_PARAMETER; >>> + ret =3D efi_valid_update_capsule_params(capsule_header_array, >>> + capsule_count, >>> + scatter_gather_list); >>> + if (ret !=3D EFI_SUCCESS) >>> goto out; >>> - } >>> >>> - ret =3D EFI_SUCCESS; >>> for (i =3D 0, capsule =3D *capsule_header_array; i < capsule_count= ; >>> i++, capsule =3D *(++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_caps= ule_unsupported( >>> efi_uintn_t capsule_count, >>> u64 scatter_gather_list) >>> { >>> + efi_status_t ret; >>> + >>> + ret =3D 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 Hello Vincent, I will mark this patch as "changes requested". Please, send an updated version. Best regards Heinrich > >>> + if (ret !=3D EFI_SUCCESS) >>> + return ret; >>> + >>> return EFI_UNSUPPORTED; >>> } >>> >>> >>