[PATCH v9 08/11] tools: add mkeficapsule command for UEFI capsule update

[AKASHI Takahiro <takahiro.akashi@linaro.org>]

Heinrich,

On Sun, Nov 29, 2020 at 05:59:41AM +0100, Heinrich Schuchardt wrote:
> On 11/27/20 3:22 PM, Heinrich Schuchardt wrote:
> > Am 25. November 2020 08:32:08 MEZ schrieb AKASHI Takahiro <takahiro.akashi@linaro.org>:
> > > Heinrich,
> > > 
> > > On Wed, Nov 25, 2020 at 07:42:31AM +0100, Heinrich Schuchardt wrote:
> > > > Am 25. November 2020 02:05:06 MEZ schrieb AKASHI Takahiro
> > > <takahiro.akashi@linaro.org>:
> > > > > Heinrich,
> > > > > 
> > > > > On Tue, Nov 24, 2020 at 09:23:50PM +0100, Heinrich Schuchardt wrote:
> > > > > > On 11/17/20 1:28 AM, AKASHI Takahiro wrote:
> > > > > > > This is a utility mainly for test purpose.
> > > > > > >     mkeficapsule -f: create a test capsule file for FIT image
> > > > > firmware
> > > > > > > 
> > > > > > > Having said that, you will be able to customize the code to fit
> > > > > > > your specific requirements for your platform.
> > > > > > > 
> > > > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > > > > > > ---
> > > > > > >    tools/Makefile       |   2 +
> > > > > > >    tools/mkeficapsule.c | 238
> > > > > +++++++++++++++++++++++++++++++++++++++++++
> > > > > > >    2 files changed, 240 insertions(+)
> > > > > > >    create mode 100644 tools/mkeficapsule.c
> > > > > > > 
> > > > > > > diff --git a/tools/Makefile b/tools/Makefile
> > > > > > > index 51123fd92983..66d9376803e3 100644
> > > > > > > --- a/tools/Makefile
> > > > > > > +++ b/tools/Makefile
> > > > > > > @@ -218,6 +218,8 @@ hostprogs-$(CONFIG_MIPS) += mips-relocs
> > > > > > >    hostprogs-$(CONFIG_ASN1_COMPILER)	+= asn1_compiler
> > > > > > >    HOSTCFLAGS_asn1_compiler.o = -idirafter $(srctree)/include
> > > > > > > 
> > > > > > > +hostprogs-$(CONFIG_EFI_HAVE_CAPSULE_SUPPORT) += mkeficapsule
> > > > > > > +
> > > > > > >    # We build some files with extra pedantic flags to try to
> > > > > minimize things
> > > > > > >    # that won't build on some weird host compiler -- though there
> > > > > are lots of
> > > > > > >    # exceptions for files that aren't complaint.
> > > > > > > diff --git a/tools/mkeficapsule.c b/tools/mkeficapsule.c
> > > > > > > new file mode 100644
> > > > > > > index 000000000000..db95426457cc
> > > > > > > --- /dev/null
> > > > > > > +++ b/tools/mkeficapsule.c
> > > > > > > @@ -0,0 +1,238 @@
> > > > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > > > +/*
> > > > > > > + * Copyright 2018 Linaro Limited
> > > > > > > + *		Author: AKASHI Takahiro
> > > > > > > + */
> > > > > > > +
> > > > > > > +#include <getopt.h>
> > > > > > > +#include <malloc.h>
> > > > > > > +#include <stdbool.h>
> > > > > > > +#include <stdio.h>
> > > > > > > +#include <stdlib.h>
> > > > > > > +#include <string.h>
> > > > > > > +#include <linux/types.h>
> > > > > > > +#include <sys/stat.h>
> > > > > > > +#include <sys/types.h>
> > > > > > > +
> > > > > > > +typedef __u8 u8;
> > > > > > > +typedef __u16 u16;
> > > > > > > +typedef __u32 u32;
> > > > > > > +typedef __u64 u64;
> > > > > > > +typedef __s16 s16;
> > > > > > > +typedef __s32 s32;
> > > > > > > +
> > > > > > > +#define aligned_u64 __aligned_u64
> > > > > > > +
> > > > > > > +#ifndef __packed
> > > > > > > +#define __packed __attribute__((packed))
> > > > > > > +#endif
> > > > > > > +
> > > > > > > +#include <efi.h>
> > > > > > > +#include <efi_api.h>
> > > > > > > +
> > > > > > > +static const char *tool_name = "mkeficapsule";
> > > > > > > +
> > > > > > > +efi_guid_t efi_guid_fm_capsule =
> > > > > EFI_FIRMWARE_MANAGEMENT_CAPSULE_ID_GUID;
> > > > > > > +efi_guid_t efi_guid_image_type_uboot_fit =
> > > > > > > +		EFI_FIRMWARE_IMAGE_TYPE_UBOOT_FIT_GUID;
> > > > > > > +efi_guid_t efi_guid_image_type_uboot_raw =
> > > > > > > +		EFI_FIRMWARE_IMAGE_TYPE_UBOOT_RAW_GUID;
> > > > > > > +
> > > > > > > +static struct option options[] = {
> > > > > > > +	{"fit", required_argument, NULL, 'f'},
> > > > > > > +	{"raw", required_argument, NULL, 'r'},
> > > > > > > +	{"index", required_argument, NULL, 'i'},
> > > > > > > +	{"instance", required_argument, NULL, 'I'},
> > > > > > > +	{"version", required_argument, NULL, 'v'},
> > > > > > > +	{"help", no_argument, NULL, 'h'},
> > > > > > > +	{NULL, 0, NULL, 0},
> > > > > > > +};
> > > > > > > +
> > > > > > > +static void print_usage(void)
> > > > > > > +{
> > > > > > > +	printf("Usage: %s [options] <output file>\n"
> > > > > > > +	       "Options:\n"
> > > > > > > +	       "\t--fit <fit image>      new FIT image file\n"
> > > > > > > +	       "\t--raw <raw image>      new raw image file\n"
> > > > > > > +	       "\t--index <index>        update image index\n"
> > > > > > > +	       "\t--instance <instance>  update hardware instance\n"
> > > > > > > +	       "\t--version <version>    firmware version\n"
> > > > > > > +	       "\t--help                 print a help message\n",
> > > > > > > +	       tool_name);
> > > > > > > +}
> > > > > > > +
> > > > > > > +static int create_fwbin(char *path, char *bin, efi_guid_t
> > > *guid,
> > > > > > > +			unsigned long version, 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;
> > > > > > > +
> > > > > > > +#ifdef DEBUG
> > > > > > > +	printf("For output: %s\n", path);
> > > > > > > +	printf("\tbin: %s\n\ttype: %pUl\n" bin, guid);
> > > > > > > +	printf("\tversion: %ld\n\tindex: %ld\n\tinstance: %ld\n",
> > > > > > > +	       version, 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);
> > > > > > > +	if (!data) {
> > > > > > > +		printf("cannot allocate memory: %lx\n", bin_stat.st_size);
> > > > > > > +		goto err_1;
> > > > > > > +	}
> > > > > > > +	f = fopen(path, "w");
> > > > > > > +	if (!f) {
> > > > > > > +		printf("cannot open %s\n", path);
> > > > > > > +		goto err_2;
> > > > > > > +	}
> > > > > > > +	header.capsule_guid = efi_guid_fm_capsule;
> > > > > > > +	header.header_size = sizeof(header);
> > > > > > > +	header.flags = CAPSULE_FLAGS_PERSIST_ACROSS_RESET; /* TODO */
> > > > > > > +	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 (%lx)\n", size);
> > > > > > > +		goto err_3;
> > > > > > > +	}
> > > > > > > +
> > > > > > > +	capsule.version = 0x00000001;
> > > > > > > +	capsule.embedded_driver_count = 0;
> > > > > > > +	capsule.payload_item_count = 1;
> > > > > > > +	capsule.item_offset_list[0] = sizeof(capsule) + sizeof(u64);
> > > > > > 
> > > > > > With the complete series applied, building sandbox_defconfig:
> > > > > > 
> > > > > > tools/mkeficapsule.c: In function ?main?:
> > > > > > tools/mkeficapsule.c:120:26: warning: array subscript 0 is outside
> > > > > array
> > > > > > bounds of ?u64[]? {aka ?long long unsigned int[]?}
> > > [-Warray-bounds]
> > > > > >    120 |  capsule.item_offset_list[0] = sizeof(capsule) +
> > > sizeof(u64);
> > > > > >        |  ~~~~~~~~~~~~~~~~~~~~~~~~^~~
> > > > > > 
> > > > > > Please, ensure that the code compiles without warnings.
> > > > > 
> > > > > Fixing this warning would be easy, but I didn't see it
> > > > > with gcc9.2 (from Linaro/Arm) or even in the results of Travis CI.
> > > > > 
> > > > > So I wonder if it is mandatory that the code compiles without
> > > warnings,
> > > > > and if so, which compiler and which version are required?
> > > 
> > > First, can you please make a comment here against my question?
> > > 
> > > > > 
> > > > > -Takahiro Akashi
> > > > > 
> > > > 
> > > > Our settings for gitlab CI and Travis CI are that all warnings are
> > > treated as errors.
> > > 
> > > As I said, I've never seen this warning/error in Travis CI.
> > > I don't know how we can confirm the result of gitlab CI.
> > > 
> > > -Takahiro Akashi
> > 
> > 
> > Just make sure that GCC 10+ does not complain locally.
> > 
> > Tom will update the CI in January. I dont want to see a build error then.
> > 
> > Best regards
> > 
> > Heinrich
> 
> Dear Takahiro,
> 
> reading through your code it is obvious that this in not only a stray
> warning by GCC but a veritable bug in your patch.

Yes, I know.
That is why I said:
(https://lists.denx.de/pipermail/u-boot/2020-November/433453.html)
> Oops, I found that this is not a warning, but a potential bug.
> The code does overrun a variable, capsule, allocated on the stack while it is
> apparently harmless as the neighboring variable, image, is initialized later.

-Takahiro Akashi


> You define capsule as:
> 
> 69 ????????struct efi_firmware_management_capsule_header capsule;
> 
> capsule.item_offset_list[] has zero bytes.
> 
> Here you write outside of your structure:
> 
> 120        capsule.item_offset_list[0] = sizeof(capsule) + sizeof(u64);
> 
> To solve this you could make capsule a pointer.
> 
> struct efi_firmware_management_capsule_header *capsule;
> 
> capsule = malloc(sizeof(struct efi_firmware_management_capsule_header) +
>           sizeof(u64));
> 
> if (!capusule)
> 	goto err;
> 
> capsule.item_offset_list[0] = sizeof(capsule) + sizeof(u64);
> 
> Best regards
> 
> Heinrich
> 
> > 
> > > 
> > > 
> > > > So we must build without warning.
> > > > 
> > > > Best regards
> > > > 
> > > > Heinrich
> > > > 
> > > > > 
> > > > > > I have been using GCC 10.2 as supplied by Debian Bullseye on an
> > > ARM64
> > > > > > system.
> > > > > > 
> > > > > > Best regards
> > > > > > 
> > > > > > Heinrich
> > > > > > 
> > > > > > > +	size = fwrite(&capsule, 1, sizeof(capsule) + sizeof(u64), f);
> > > > > > > +	if (size < (sizeof(capsule) + sizeof(u64))) {
> > > > > > > +		printf("write failed (%lx)\n", size);
> > > > > > > +		goto err_3;
> > > > > > > +	}
> > > > > > > +
> > > > > > > +	image.version = version;
> > > > > > > +	memcpy(&image.update_image_type_id, guid, sizeof(*guid));
> > > > > > > +	image.update_image_index = index;
> > > > > > > +	image.update_image_size = bin_stat.st_size;
> > > > > > > +	image.update_vendor_code_size = 0; /* none */
> > > > > > > +	image.update_hardware_instance = instance;
> > > > > > > +	image.image_capsule_support = 0;
> > > > > > > +
> > > > > > > +	size = fwrite(&image, 1, sizeof(image), f);
> > > > > > > +	if (size < sizeof(image)) {
> > > > > > > +		printf("write failed (%lx)\n", size);
> > > > > > > +		goto err_3;
> > > > > > > +	}
> > > > > > > +	size = fread(data, 1, bin_stat.st_size, g);
> > > > > > > +	if (size < bin_stat.st_size) {
> > > > > > > +		printf("read failed (%lx)\n", size);
> > > > > > > +		goto err_3;
> > > > > > > +	}
> > > > > > > +	size = fwrite(data, 1, bin_stat.st_size, f);
> > > > > > > +	if (size < bin_stat.st_size) {
> > > > > > > +		printf("write failed (%lx)\n", size);
> > > > > > > +		goto err_3;
> > > > > > > +	}
> > > > > > > +
> > > > > > > +	fclose(f);
> > > > > > > +	fclose(g);
> > > > > > > +	free(data);
> > > > > > > +
> > > > > > > +	return 0;
> > > > > > > +
> > > > > > > +err_3:
> > > > > > > +	fclose(f);
> > > > > > > +err_2:
> > > > > > > +	free(data);
> > > > > > > +err_1:
> > > > > > > +	fclose(g);
> > > > > > > +
> > > > > > > +	return -1;
> > > > > > > +}
> > > > > > > +
> > > > > > > +/*
> > > > > > > + * Usage:
> > > > > > > + *   $ mkeficapsule -f <firmware binary> <output file>
> > > > > > > + */
> > > > > > > +int main(int argc, char **argv)
> > > > > > > +{
> > > > > > > +	char *file;
> > > > > > > +	efi_guid_t *guid;
> > > > > > > +	unsigned long index, instance, version;
> > > > > > > +	int c, idx;
> > > > > > > +
> > > > > > > +	file = NULL;
> > > > > > > +	guid = NULL;
> > > > > > > +	index = 0;
> > > > > > > +	instance = 0;
> > > > > > > +	version = 0;
> > > > > > > +	for (;;) {
> > > > > > > +		c = getopt_long(argc, argv, "f:r:i:I:v:h", options, &idx);
> > > > > > > +		if (c == -1)
> > > > > > > +			break;
> > > > > > > +
> > > > > > > +		switch (c) {
> > > > > > > +		case 'f':
> > > > > > > +			if (file) {
> > > > > > > +				printf("Image already specified\n");
> > > > > > > +				return -1;
> > > > > > > +			}
> > > > > > > +			file = optarg;
> > > > > > > +			guid = &efi_guid_image_type_uboot_fit;
> > > > > > > +			break;
> > > > > > > +		case 'r':
> > > > > > > +			if (file) {
> > > > > > > +				printf("Image already specified\n");
> > > > > > > +				return -1;
> > > > > > > +			}
> > > > > > > +			file = optarg;
> > > > > > > +			guid = &efi_guid_image_type_uboot_raw;
> > > > > > > +			break;
> > > > > > > +		case 'i':
> > > > > > > +			index = strtoul(optarg, NULL, 0);
> > > > > > > +			break;
> > > > > > > +		case 'I':
> > > > > > > +			instance = strtoul(optarg, NULL, 0);
> > > > > > > +			break;
> > > > > > > +		case 'v':
> > > > > > > +			version = strtoul(optarg, NULL, 0);
> > > > > > > +			break;
> > > > > > > +		case 'h':
> > > > > > > +			print_usage();
> > > > > > > +			return 0;
> > > > > > > +		}
> > > > > > > +	}
> > > > > > > +
> > > > > > > +	/* need a output file */
> > > > > > > +	if (argc != optind + 1) {
> > > > > > > +		print_usage();
> > > > > > > +		return -1;
> > > > > > > +	}
> > > > > > > +
> > > > > > > +	/* need a fit image file or raw image file */
> > > > > > > +	if (!file) {
> > > > > > > +		print_usage();
> > > > > > > +		return -1;
> > > > > > > +	}
> > > > > > > +
> > > > > > > +	if (create_fwbin(argv[optind], file, guid, version, index,
> > > > > instance)
> > > > > > > +			< 0) {
> > > > > > > +		printf("Creating firmware capsule failed\n");
> > > > > > > +		return -1;
> > > > > > > +	}
> > > > > > > +
> > > > > > > +	return 0;
> > > > > > > +}
> > > > > > > 
> > > > > > 
> > > > 
> > 
>