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 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 smtp.lore.kernel.org (Postfix) with ESMTPS id 77C18C76196 for ; Tue, 28 Mar 2023 11:53:21 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id DB11A85844; Tue, 28 Mar 2023 13:53:18 +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="UQ53+ivz"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 6A44785B57; Tue, 28 Mar 2023 13:53:16 +0200 (CEST) Received: from mail-wm1-x336.google.com (mail-wm1-x336.google.com [IPv6:2a00:1450:4864:20::336]) (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 B8E8385802 for ; Tue, 28 Mar 2023 13:53:12 +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-x336.google.com with SMTP id m6-20020a05600c3b0600b003ee6e324b19so7248112wms.1 for ; Tue, 28 Mar 2023 04:53:12 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1680004392; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=3tvmjsRAf3ib+VqynWmdUlhu+sNBZ8q/kajnI4AzzWI=; b=UQ53+ivzo9TJSZ0lS/ORdPbxOKn5LE3fl9g1PapFe0eYg++Yy0yOJKtr0HUldjM4JL mUJ2/7dL6RvTZhod/g9Gf79ApfEzY2754LjYpXOqacFtSg5xGsAezArb/QkmuocCYTqC +llZY2MlIsrb02l4T2nKy+yqyLEH4q03ZPVtygFyOx+i6Io6sXX8x3TunbMdIPj3HSke myxcm8eaA3Rt/zttJvc1nSf8iEjTPDmP/MidRYuCLAMGCMMANE3nOzh+vjtio273prck 6YjX7lLxrqqS9k5NLVw+gLyll6SRqyXFwZ2EWvndVwjra9LslaFGAMqvh9OsLgvdIort KA8A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1680004392; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=3tvmjsRAf3ib+VqynWmdUlhu+sNBZ8q/kajnI4AzzWI=; b=H3b9gHOsjTMZrZ+0lR0Q5V5e4d80slCdEUzYRlPzW/FZhU9ITrcoRqt3D0HiRtbqKF fM+qqrQyQO612W6hdAFAKgjY1SprvH1AKUYmB3qjFC7z8rZ5816nFRfJd2lsd5fb9dAE ibOwZ65Tfjdop/Ay8U1Xa/8gLNvUA4yih4ZrbJpwO8V/0UyqJ5JojMu6TDo9FPSD3FZu WDvD3s7M/WST9ij7puKvJbIkj0inJ843hcB5lSf+cYMmtf/De2gKK2SDv8PJxztx2OHN m39h6FS5y4kuocX0KmXdeR72XDuyu+/AtFqnebp/q0j0xfx0oEkyEPMrHjfRwtVtaER/ Nl8w== X-Gm-Message-State: AO0yUKXsHQPLrUd/RzjjcZPEqaHF7ia3cwONe1w6YspMtioUHQ4SlW41 O1ptFtTh4TBDVU5xlDuTNkx6Qg== X-Google-Smtp-Source: AK7set9APe5mmX1wYv2FybGkclQg8XTykSKasBDONoDNoVtLH1DUhYclas9KnrtGS1PgCRI1rRP6mw== X-Received: by 2002:a1c:7c17:0:b0:3ed:2ae9:6c75 with SMTP id x23-20020a1c7c17000000b003ed2ae96c75mr11947493wmc.37.1680004392045; Tue, 28 Mar 2023 04:53:12 -0700 (PDT) Received: from hades (ppp176092130041.access.hol.gr. [176.92.130.41]) by smtp.gmail.com with ESMTPSA id h7-20020a05600c350700b003dd1bd0b915sm12433863wmq.22.2023.03.28.04.53.11 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 28 Mar 2023 04:53:11 -0700 (PDT) Date: Tue, 28 Mar 2023 14:53:09 +0300 From: Ilias Apalodimas To: Masahisa Kojima Cc: u-boot@lists.denx.de, Heinrich Schuchardt , Takahiro Akashi Subject: Re: [PATCH v4 1/5] efi_loader: store firmware version into FmpState variable Message-ID: References: <20230323110906.23783-1-masahisa.kojima@linaro.org> <20230323110906.23783-2-masahisa.kojima@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20230323110906.23783-2-masahisa.kojima@linaro.org> X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 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.8 at phobos.denx.de X-Virus-Status: Clean Kojima-san, On Thu, Mar 23, 2023 at 08:09:01PM +0900, Masahisa Kojima wrote: > Firmware version management is not implemented in the current > FMP protocol. > EDK II reference implementation capsule generation script inserts > the FMP Payload Header right before the payload, it contains the > firmware version and lowest supported version. > > This commit utilizes the FMP Payload Header, reads the header and > stores the firmware version, lowest supported version, > last attempt version and last attempt status into "FmpStateXXXX" > EFI non-volatile variable. XXXX indicates the image index, > since FMP protocol handles multiple image indexes. > > This change is compatible with the existing FMP implementation. > This change does not mandate the FMP Payload Header. > If no FMP Payload Header is found in the capsule file, fw_version, > lowest supported version, last attempt version and last attempt > status is 0 and this is the same behavior as existing FMP > implementation. > > Signed-off-by: Masahisa Kojima > --- > Changes in v4: > - move lines that are the same in both branches out of the if statement > - s/EDK2/EDK II/ > - create print result function > - set last_attempt_version when capsule authentication failed > - use log_err() instead of printf() > > Changes in v3: > - exclude CONFIG_FWU_MULTI_BANK_UPDATE case > - set image_type_id as a vendor field of FmpStateXXXX variable > - set READ_ONLY flag for FmpStateXXXX variable > - add error code for FIT image case > > Changes in v2: > - modify indent > > lib/efi_loader/efi_firmware.c | 250 ++++++++++++++++++++++++++++++---- > 1 file changed, 222 insertions(+), 28 deletions(-) > > diff --git a/lib/efi_loader/efi_firmware.c b/lib/efi_loader/efi_firmware.c > index 93e2b01c07..fb5f7906d3 100644 > --- a/lib/efi_loader/efi_firmware.c > +++ b/lib/efi_loader/efi_firmware.c > @@ -10,6 +10,7 @@ > #include > #include > #include > +#include [...] > +void efi_firmware_parse_payload_header(const void **p_image, > + efi_uintn_t *p_image_size, > + struct fmp_state *state) > +{ > + const void *image = *p_image; > + efi_uintn_t image_size = *p_image_size; > + const struct fmp_payload_header *header; > + u32 fmp_hdr_signature = FMP_PAYLOAD_HDR_SIGNATURE; > + > + header = image; > if (!memcmp(&header->signature, &fmp_hdr_signature, > sizeof(fmp_hdr_signature))) { Why is this a memcmp? if (header->signature == FMP_PAYLOAD_HDR_SIGNATURE) ? > - /* > - * When building the capsule with the scripts in > - * edk2, a FMP header is inserted above the capsule > - * payload. Compensate for this header to get the > - * actual payload that is to be updated. > - */ > + /* FMP header is inserted above the capsule payload */ > + state->fw_version = header->fw_version; > + state->lowest_supported_version = header->lowest_supported_version; > + state->last_attempt_version = header->fw_version; > image += header->header_size; > image_size -= header->header_size; > } > > *p_image = image; > *p_image_size = image_size; > - return EFI_SUCCESS; > +} > + > +/** > + * efi_firmware_verify_image - verify image > + * @p_image: Pointer to new image > + * @p_image_size: Pointer to size of new image > + * @image_index Image index > + * @state Pointer to fmp state > + * > + * Verify the capsule file > + * > + * Return: status code > + */ > +static > +efi_status_t efi_firmware_verify_image(const void **p_image, > + efi_uintn_t *p_image_size, > + u8 image_index, > + struct fmp_state *state) > +{ > + efi_status_t ret; > + > + ret = efi_firmware_capsule_authenticate(p_image, p_image_size, state); > + efi_firmware_parse_payload_header(p_image, p_image_size, state); I guess this needs to run regardless of the result to set fw_version etc? /efi_firmware_parse_payload_header() is a bit misleading though. It doesn't parse the fmp payload header. Instead it sets some values that are contained in the header and adjusts the image size accordingly. Can we come up with a better name? > + > + return ret; > +} > + > +/** > + * efi_firmware_print_result - print the firmware update result > + * @status: status code > + * @state Pointer to fmp state > + * > + * Print the firmware update result > + */ > +void efi_firmware_print_result(efi_status_t status, struct fmp_state *state) > +{ > + if (status == EFI_SUCCESS) { > + if (IS_ENABLED(CONFIG_FWU_MULTI_BANK_UPDATE)) > + log_info("Firmware successfully written\n"); > + else > + log_info("Firmware updated to version %u\n", > + state->fw_version); > + } > } This is an overkill tbh. Even if A/B updates are supported the firmware is updated to a newer version. Whether or not the new version will be *accepted* on a subsequent reboot is irrelevant. Moreover if we define a function it should be static, but I would prefer just getting rid of it. > > /** > @@ -330,7 +494,9 @@ efi_status_t EFIAPI efi_firmware_fit_set_image( > efi_status_t (*progress)(efi_uintn_t completion), > u16 **abort_reason) > { > + bool updated; > efi_status_t status; > + struct fmp_state state = { 0 }; > > EFI_ENTRY("%p %d %p %zu %p %p %p\n", this, image_index, image, > image_size, vendor_code, progress, abort_reason); > @@ -338,14 +504,25 @@ efi_status_t EFIAPI efi_firmware_fit_set_image( > if (!image || image_index != 1) > return EFI_EXIT(EFI_INVALID_PARAMETER); > > - status = efi_firmware_capsule_authenticate(&image, &image_size); > + status = efi_firmware_verify_image(&image, &image_size, image_index, > + &state); > if (status != EFI_SUCCESS) > - return EFI_EXIT(status); > + goto err; > + > + if (fit_update(image)) { > + status = EFI_DEVICE_ERROR; > + state.last_attempt_status = LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL; > + goto err; > + } > > - if (fit_update(image)) > - return EFI_EXIT(EFI_DEVICE_ERROR); > + state.last_attempt_status = LAST_ATTEMPT_STATUS_SUCCESS; > +err: > + updated = (status == EFI_SUCCESS) ? true : false; > + efi_firmware_set_fmp_state_var(&state, image_index, updated); > > - return EFI_EXIT(EFI_SUCCESS); > + efi_firmware_print_result(status, &state); > + > + return EFI_EXIT(status); > } > > const struct efi_firmware_management_protocol efi_fmp_fit = { > @@ -391,7 +568,9 @@ efi_status_t EFIAPI efi_firmware_raw_set_image( > u16 **abort_reason) > { > int ret; > + bool updated; > efi_status_t status; > + struct fmp_state state = { 0 }; > > EFI_ENTRY("%p %d %p %zu %p %p %p\n", this, image_index, image, > image_size, vendor_code, progress, abort_reason); > @@ -399,9 +578,10 @@ efi_status_t EFIAPI efi_firmware_raw_set_image( > if (!image) > return EFI_EXIT(EFI_INVALID_PARAMETER); > > - status = efi_firmware_capsule_authenticate(&image, &image_size); > + status = efi_firmware_verify_image(&image, &image_size, image_index, > + &state); > if (status != EFI_SUCCESS) > - return EFI_EXIT(status); > + goto err; > > if (IS_ENABLED(CONFIG_FWU_MULTI_BANK_UPDATE)) { > /* > @@ -411,15 +591,29 @@ efi_status_t EFIAPI efi_firmware_raw_set_image( > ret = fwu_get_image_index(&image_index); > if (ret) { > log_debug("Unable to get FWU image_index\n"); > - return EFI_EXIT(EFI_DEVICE_ERROR); > + status = EFI_DEVICE_ERROR; > + goto err; > } > } > > if (dfu_write_by_alt(image_index - 1, (void *)image, image_size, > - NULL, NULL)) > - return EFI_EXIT(EFI_DEVICE_ERROR); > + NULL, NULL)) { > + status = EFI_DEVICE_ERROR; > + state.last_attempt_status = LAST_ATTEMPT_STATUS_ERROR_UNSUCCESSFUL; > + goto err; > + } > + > + state.last_attempt_status = LAST_ATTEMPT_STATUS_SUCCESS; > +err: > + updated = (status == EFI_SUCCESS) ? true : false; > + > + /* TODO: implement versioning for FWU multi bank update */ > + if (!IS_ENABLED(CONFIG_FWU_MULTI_BANK_UPDATE)) > + efi_firmware_set_fmp_state_var(&state, image_index, updated); > + > + efi_firmware_print_result(status, &state); > > - return EFI_EXIT(EFI_SUCCESS); > + return EFI_EXIT(status); > } > > const struct efi_firmware_management_protocol efi_fmp_raw = { > -- > 2.17.1 > Thanks /Ilias