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 mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 72B2EC433F5 for ; Mon, 8 Nov 2021 04:18:44 +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 D44286109D for ; Mon, 8 Nov 2021 04:18:43 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org D44286109D 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 6DBDC8380A; Mon, 8 Nov 2021 05:18:41 +0100 (CET) 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="VQmAH/EP"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 4892883816; Mon, 8 Nov 2021 05:18:39 +0100 (CET) Received: from mail-pf1-x436.google.com (mail-pf1-x436.google.com [IPv6:2607:f8b0:4864:20::436]) (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 33D7A83800 for ; Mon, 8 Nov 2021 05:18:35 +0100 (CET) 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-x436.google.com with SMTP id m26so14790505pff.3 for ; Sun, 07 Nov 2021 20:18:35 -0800 (PST) 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=Lj1EA2W7qZoHfj0KbTDEkBnDXYTaMyenOH89E7z+p5Q=; b=VQmAH/EP1RZYiJBo81/7pf0/hoXVr3lmi1eNyzG9gEYWpuOmjNYN/fwG/PvGPjWBVq iyD46MYl/W+jKRQxR/Cp0OpRpCQGMhvhX0pKnrwr79Ht0rSpnHcT9cMby0TAo7wCAaW9 oNYZd5Qb2Eo2m/wCKOQ6hb0+N4n6YDFExs8XQq77UfdOiGL297wjAyFoNyD+sSrq9A1E OWaAAoCG9jgpMXenudV+72AIsDKcP+Ab5YjqEc4SDpDDSPi2eEKwe0S+Ci8sKRXw5+tw bUc4IA5og8W+zMGNruSnx5LKWdjJiyk8RctBd/pL24TsdJCkiewzx6wmXanBZ2ZZDcWe Pmeg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id :mail-followup-to:references:mime-version:content-disposition :in-reply-to; bh=Lj1EA2W7qZoHfj0KbTDEkBnDXYTaMyenOH89E7z+p5Q=; b=3aZg80gLZZWMtq9/HuETyAVZcXN0nVcZZPQH3vrXG/VHB6W1Qz49r9P3uJ0d+vKozk jvwzVCvQQT9hfeSO8hKkRVrXZzsgc+zLMWM1AHt8U0frllTTGOsvGG6I/b4o7jJM7olE zHoQHg0LAhoFfGJhDu5uzpHjmgYm5NdQluBNz9AnprSIrMPVYj4kupa5AqRB9bKXaD9B YnNbO820NElTQlOnVzQoLjLII4y7kAViEulqhDaO8v3FjOSN/q851Mv5KfwgqZi7M8fm hL+ASOG81DT/qYd8y8ztdZ6DQXBuv5LVvG+pLAdBiehfvZi86gd3Td/zq2Kncj6Zn3SG 1BPw== X-Gm-Message-State: AOAM531P/3n2nVclIWa3LcP/dePmMMJqHMwH2NS/NifKYxynTzYeAj0y uA+vKFxAP2FJyowCq9ajGdrz7A== X-Google-Smtp-Source: ABdhPJyuknRRzXnxFTInba2w1V5+/OSaJAH9aggRQWnxkTEiAVUYG/CVvoxvfw4J/zrJupGS9XMSaQ== X-Received: by 2002:a63:854a:: with SMTP id u71mr44899503pgd.174.1636345113363; Sun, 07 Nov 2021 20:18:33 -0800 (PST) Received: from laputa ([2400:4050:c3e1:100:98bf:5be1:75ff:1c8a]) by smtp.gmail.com with ESMTPSA id i10sm10177142pfe.180.2021.11.07.20.18.30 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 07 Nov 2021 20:18:32 -0800 (PST) Date: Mon, 8 Nov 2021 13:18:28 +0900 From: AKASHI Takahiro To: Heinrich Schuchardt Cc: ilias.apalodimas@linaro.org, sughosh.ganu@linaro.org, masami.hiramatsu@linaro.org, u-boot@lists.denx.de, agraf@csgraf.de, sjg@chromium.org Subject: Re: [PATCH v6 02/12] tools: mkeficapsule: rework the code a little bit Message-ID: <20211108041828.GB16401@laputa> Mail-Followup-To: AKASHI Takahiro , Heinrich Schuchardt , ilias.apalodimas@linaro.org, sughosh.ganu@linaro.org, masami.hiramatsu@linaro.org, u-boot@lists.denx.de, agraf@csgraf.de, sjg@chromium.org References: <20211102005512.96019-1-takahiro.akashi@linaro.org> <20211102005512.96019-3-takahiro.akashi@linaro.org> <49a84006-ac07-85f2-6dae-3a72312055a4@gmx.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <49a84006-ac07-85f2-6dae-3a72312055a4@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 On Sun, Nov 07, 2021 at 07:35:04AM +0100, Heinrich Schuchardt wrote: > On 11/2/21 01:55, AKASHI Takahiro wrote: > > Abstract common routines to make the code easily understandable. > > No functional change. > > > > Signed-off-by: AKASHI Takahiro > > Reviewed-by: Simon Glass > > --- > > tools/mkeficapsule.c | 219 ++++++++++++++++++++++++++++++------------- > > 1 file changed, 155 insertions(+), 64 deletions(-) > > > > diff --git a/tools/mkeficapsule.c b/tools/mkeficapsule.c > > index 4995ba4e0c2a..8427fedd941c 100644 > > --- a/tools/mkeficapsule.c > > +++ b/tools/mkeficapsule.c > > @@ -61,17 +61,117 @@ static void print_usage(void) > > tool_name); > > } > > > > +/** > > + * read_bin_file - read a firmware binary file > > + * @bin: Path to a firmware binary file > > + * @data: Pointer to pointer of allocated buffer > > + * @bin_size: Size of allocated buffer > > + * > > + * Read out a content of binary, @bin, into @data. > > + * A caller should free @data. > > + * > > + * Return: > > + * * 0 - on success > > + * * -1 - on failure > > + */ > > +static int read_bin_file(char *bin, void **data, off_t *bin_size) > > +{ > > + FILE *g; > > + struct stat bin_stat; > > + void *buf; > > + size_t size; > > + int ret = 0; > > + > > + g = fopen(bin, "r"); > > + if (!g) { > > + printf("cannot open %s\n", bin); > > + return -1; > > + } > > + if (stat(bin, &bin_stat) < 0) { > > + printf("cannot determine the size of %s\n", bin); > > + ret = -1; > > + goto err; > > + } > > + buf = malloc(bin_stat.st_size); > > bin_stat.st_size is of type off_t which is a 64bit type if the operating > system supports file sizes exceeding 4 GiB. > > malloc() expects a size_t argument. You should check for an overflow > here before casting from 64bit to 32bit on a 32bit system. OK, will fix it. -Takahiro Akashi > > + if (!buf) { > > + printf("cannot allocate memory: %zx\n", > > + (size_t)bin_stat.st_size); > > + ret = -1; > > + goto err; > > + } > > + > > + size = fread(buf, 1, bin_stat.st_size, g); > > + if (size < bin_stat.st_size) { > > + printf("read failed (%zx)\n", size); > > + ret = -1; > > + goto err; > > + } > > + > > + *data = buf; > > + *bin_size = bin_stat.st_size; > > +err: > > + fclose(g); > > + > > + return ret; > > +} > > + > > +/** > > + * write_capsule_file - write a capsule file > > + * @bin: FILE stream > > + * @data: Pointer to data > > + * @bin_size: Size of data > > + * > > + * Write out data, @data, with the size @bin_size. > > + * > > + * Return: > > + * * 0 - on success > > + * * -1 - on failure > > + */ > > +static int write_capsule_file(FILE *f, void *data, size_t size, const char *msg) > > +{ > > + size_t size_written; > > + > > + size_written = fwrite(data, 1, size, f); > > + if (size_written < size) { > > + printf("%s: write failed (%zx != %zx)\n", msg, > > + size_written, size); > > + return -1; > > + } > > + > > + return 0; > > +} > > + > > +/** > > + * create_fwbin - create an uefi capsule file > > + * @path: Path to a created capsule file > > + * @bin: Path to a firmware binary to encapsulate > > + * @guid: GUID of related FMP driver > > + * @index: Index number in capsule > > + * @instance: Instance number in capsule > > + * @mcount: Monotonic count in authentication information > > + * @private_file: Path to a private key file > > + * @cert_file: Path to a certificate file > > + * > > + * This function actually does the job of creating an uefi capsule file. > > + * All the arguments must be supplied. > > + * If either @private_file ror @cert_file is NULL, the capsule file > > + * won't be signed. > > + * > > + * Return: > > + * * 0 - on success > > + * * -1 - on failure > > + */ > > static int create_fwbin(char *path, char *bin, efi_guid_t *guid, > > unsigned long index, unsigned long instance) > > { > > struct efi_capsule_header header; > > struct efi_firmware_management_capsule_header capsule; > > struct efi_firmware_management_capsule_image_header image; > > - FILE *f, *g; > > - struct stat bin_stat; > > - u8 *data; > > - size_t size; > > + FILE *f; > > + void *data; > > + off_t bin_size; > > u64 offset; > > + int ret; > > > > #ifdef DEBUG > > printf("For output: %s\n", path); > > @@ -79,25 +179,28 @@ static int create_fwbin(char *path, char *bin, efi_guid_t *guid, > > printf("\tindex: %ld\n\tinstance: %ld\n", index, instance); > > #endif > > > > - g = fopen(bin, "r"); > > - if (!g) { > > - printf("cannot open %s\n", bin); > > - return -1; > > - } > > - if (stat(bin, &bin_stat) < 0) { > > - printf("cannot determine the size of %s\n", bin); > > - goto err_1; > > - } > > - data = malloc(bin_stat.st_size); > > Please, check for overflow before casting from off_t to size_t. > > Best regards > > Heinrich > > > - if (!data) { > > - printf("cannot allocate memory: %zx\n", (size_t)bin_stat.st_size); > > - goto err_1; > > - } > > + f = NULL; > > + data = NULL; > > + ret = -1; > > + > > + /* > > + * read a firmware binary > > + */ > > + if (read_bin_file(bin, &data, &bin_size)) > > + goto err; > > + > > + /* > > + * write a capsule file > > + */ > > f = fopen(path, "w"); > > if (!f) { > > printf("cannot open %s\n", path); > > - goto err_2; > > + goto err; > > } > > + > > + /* > > + * capsule file header > > + */ > > header.capsule_guid = efi_guid_fm_capsule; > > header.header_size = sizeof(header); > > /* TODO: The current implementation ignores flags */ > > @@ -105,70 +208,58 @@ static int create_fwbin(char *path, char *bin, efi_guid_t *guid, > > header.capsule_image_size = sizeof(header) > > + sizeof(capsule) + sizeof(u64) > > + sizeof(image) > > - + bin_stat.st_size; > > - > > - size = fwrite(&header, 1, sizeof(header), f); > > - if (size < sizeof(header)) { > > - printf("write failed (%zx)\n", size); > > - goto err_3; > > - } > > + + bin_size; > > + if (write_capsule_file(f, &header, sizeof(header), > > + "Capsule header")) > > + goto err; > > > > + /* > > + * firmware capsule header > > + * This capsule has only one firmware capsule image. > > + */ > > capsule.version = 0x00000001; > > capsule.embedded_driver_count = 0; > > capsule.payload_item_count = 1; > > - size = fwrite(&capsule, 1, sizeof(capsule), f); > > - if (size < (sizeof(capsule))) { > > - printf("write failed (%zx)\n", size); > > - goto err_3; > > - } > > + if (write_capsule_file(f, &capsule, sizeof(capsule), > > + "Firmware capsule header")) > > + goto err; > > + > > offset = sizeof(capsule) + sizeof(u64); > > - size = fwrite(&offset, 1, sizeof(offset), f); > > - if (size < sizeof(offset)) { > > - printf("write failed (%zx)\n", size); > > - goto err_3; > > - } > > + if (write_capsule_file(f, &offset, sizeof(offset), > > + "Offset to capsule image")) > > + goto err; > > > > + /* > > + * firmware capsule image header > > + */ > > image.version = 0x00000003; > > memcpy(&image.update_image_type_id, guid, sizeof(*guid)); > > image.update_image_index = index; > > image.reserved[0] = 0; > > image.reserved[1] = 0; > > image.reserved[2] = 0; > > - image.update_image_size = bin_stat.st_size; > > + image.update_image_size = bin_size; > > image.update_vendor_code_size = 0; /* none */ > > image.update_hardware_instance = instance; > > image.image_capsule_support = 0; > > + if (write_capsule_file(f, &image, sizeof(image), > > + "Firmware capsule image header")) > > + goto err; > > > > - size = fwrite(&image, 1, sizeof(image), f); > > - if (size < sizeof(image)) { > > - printf("write failed (%zx)\n", size); > > - goto err_3; > > - } > > - size = fread(data, 1, bin_stat.st_size, g); > > - if (size < bin_stat.st_size) { > > - printf("read failed (%zx)\n", size); > > - goto err_3; > > - } > > - size = fwrite(data, 1, bin_stat.st_size, f); > > - if (size < bin_stat.st_size) { > > - printf("write failed (%zx)\n", size); > > - goto err_3; > > - } > > - > > - fclose(f); > > - fclose(g); > > - free(data); > > - > > - return 0; > > + /* > > + * firmware binary > > + */ > > + if (write_capsule_file(f, data, bin_size, "Firmware binary")) > > + goto err; > > > > -err_3: > > - fclose(f); > > -err_2: > > + ret = 0; > > +err: > > + if (f) > > + fclose(f); > > + free_sig_data(&auth_context); > > free(data); > > -err_1: > > - fclose(g); > > > > - return -1; > > + return ret; > > } > > > > /* > >