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

Message ID 20201117002805.13902-9-takahiro.akashi@linaro.org
State Superseded
Headers show
Series
  • efi_loader: add capsule update support
Related show

Commit Message

AKASHI Takahiro Nov. 17, 2020, 12:28 a.m.
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

-- 
2.28.0

Comments

Heinrich Schuchardt Nov. 24, 2020, 8:23 p.m. | #1
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.

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;

> +}

>
AKASHI Takahiro Nov. 25, 2020, 1:05 a.m. | #2
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?

-Takahiro Akashi


> 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;

> > +}

> > 

>
AKASHI Takahiro Nov. 25, 2020, 1:36 a.m. | #3
On Wed, Nov 25, 2020 at 10:05:06AM +0900, AKASHI Takahiro wrote:
> 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.


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.

> So I wonder if it is mandatory that the code compiles without warnings,

> and if so, which compiler and which version are required?


Yet, this question remains.

-Takahiro Akashi
> 

> 

> > 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;

> > > +}

> > > 

> >
AKASHI Takahiro Nov. 25, 2020, 5:17 a.m. | #4
Sughosh,

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.

> 

> 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;


After some cleanup works, I found that the version was wrongly
set here. The value of 'version' in this structure must be the
one of this structure's layout, not the one for firmware itself.
So I decided to remove "--version" option from the command.
(Note U-Boot has no notion of "firmware version" anyway.)

I hope that this change won't hinder your effort in extending
this command for capsule authentication.

Thanks,
-Takahiro Akashi



> > +	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;

> > +}

> > 

>
Sughosh Ganu Nov. 25, 2020, 6:31 a.m. | #5
Takahiro,

On Wed, 25 Nov 2020 at 10:47, AKASHI Takahiro <takahiro.akashi@linaro.org>
wrote:

> Sughosh,

>

> 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.

> >

> > 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;

>

> After some cleanup works, I found that the version was wrongly

> set here. The value of 'version' in this structure must be the

> one of this structure's layout, not the one for firmware itself.

> So I decided to remove "--version" option from the command.

> (Note U-Boot has no notion of "firmware version" anyway.)

>

> I hope that this change won't hinder your effort in extending

> this command for capsule authentication.

>


Your change to remove the 'version' option would not hinder my changes to
add the public-key certificate to the dtb for capsule authentication.

-sughosh


> Thanks,

> -Takahiro Akashi

>

>

>

> > > +   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;

> > > +}

> > >

> >

>
Heinrich Schuchardt Nov. 25, 2020, 6:42 a.m. | #6
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?

>

>-Takahiro Akashi

>


Our settings for gitlab CI and Travis CI are that all warnings are treated as errors.

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;

>> > +}

>> > 

>>
AKASHI Takahiro Nov. 25, 2020, 7:28 a.m. | #7
On Wed, Nov 25, 2020 at 12:01:11PM +0530, Sughosh Ganu wrote:
> Takahiro,

> 

> On Wed, 25 Nov 2020 at 10:47, AKASHI Takahiro <takahiro.akashi@linaro.org>

> wrote:

> 

> > Sughosh,

> >

> > 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.

> > >

> > > 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;

> >

> > After some cleanup works, I found that the version was wrongly

> > set here. The value of 'version' in this structure must be the

> > one of this structure's layout, not the one for firmware itself.

> > So I decided to remove "--version" option from the command.

> > (Note U-Boot has no notion of "firmware version" anyway.)

> >

> > I hope that this change won't hinder your effort in extending

> > this command for capsule authentication.

> >

> 

> Your change to remove the 'version' option would not hinder my changes to

> add the public-key certificate to the dtb for capsule authentication.


Thank you for the confirmation.

-Takahiro Akashi

> -sughosh

> 

> 

> > Thanks,

> > -Takahiro Akashi

> >

> >

> >

> > > > +   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;

> > > > +}

> > > >

> > >

> >
AKASHI Takahiro Nov. 25, 2020, 7:32 a.m. | #8
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


> 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;

> >> > +}

> >> > 

> >> 

>
Heinrich Schuchardt Nov. 27, 2020, 2:22 p.m. | #9
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

>

>

>> 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;

>> >> > +}

>> >> > 

>> >> 

>>
Heinrich Schuchardt Nov. 29, 2020, 4:59 a.m. | #10
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.

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;

>>>>>> +}

>>>>>>

>>>>>

>>>

>
AKASHI Takahiro Nov. 29, 2020, 11:45 p.m. | #11
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;

> > > > > > > +}

> > > > > > > 

> > > > > > 

> > > > 

> > 

>

Patch

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);
+	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;
+}