[PATCH v8 01/12] tools: mkeficapsule: rework the code a little bit

[Heinrich Schuchardt]

On 12/20/21 06:02, AKASHI Takahiro wrote:
> Abstract common routines to make the code easily understandable.
> No functional change.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi at linaro.org>
> Reviewed-by: Simon Glass <sjg at chromium.org>
> ---
>   tools/mkeficapsule.c | 223 ++++++++++++++++++++++++++++++-------------
>   1 file changed, 159 insertions(+), 64 deletions(-)
>
> diff --git a/tools/mkeficapsule.c b/tools/mkeficapsule.c
> index 4995ba4e0c2a..afdcaf7e7933 100644
> --- a/tools/mkeficapsule.c
> +++ b/tools/mkeficapsule.c
> @@ -61,17 +61,122 @@ static void print_usage(void)
>   	       tool_name);
>   }
>
> +/**
> + * read_bin_file - read a firmware binary file
> + * @bin:	Path to a firmware binary file
> + * @data:	Pointer to pointer of allocated buffer
> + * @bin_size:	Size of allocated buffer
> + *
> + * Read out a content of binary, @bin, into @data.
> + * A caller should free @data.
> + *
> + * Return:
> + * * 0  - on success
> + * * -1 - on failure
> + */
> +static int read_bin_file(char *bin, void **data, off_t *bin_size)
> +{
> +	FILE *g;
> +	struct stat bin_stat;
> +	void *buf;
> +	size_t size;
> +	int ret = 0;
> +
> +	g = fopen(bin, "r");
> +	if (!g) {
> +		printf("cannot open %s\n", bin);

Error output should be to stderr.

> +		return -1;
> +	}
> +	if (stat(bin, &bin_stat) < 0) {
> +		printf("cannot determine the size of %s\n", bin);

Error output should be to stderr.

> +		ret = -1;
> +		goto err;
> +	}
> +	if (bin_stat.st_size > (u32)~0U) {

malloc() expects a size_t argument. Why don't you compare to SIZE_MAX
defined in stdint.h here?

> +		printf("file size is too large: %s\n", bin);

Error output should be to stderr.

> +		ret = -1;
> +		goto err;
> +	}
> +	buf = malloc(bin_stat.st_size);
> +	if (!buf) {
> +		printf("cannot allocate memory: %zx\n",

Error output should be to stderr.

> +		       (size_t)bin_stat.st_size);
> +		ret = -1;
> +		goto err;
> +	}
> +
> +	size = fread(buf, 1, bin_stat.st_size, g);
> +	if (size < bin_stat.st_size) {
> +		printf("read failed (%zx)\n", size);

Error output should be to stderr.

> +		ret = -1;
> +		goto err;
> +	}
> +
> +	*data = buf;
> +	*bin_size = bin_stat.st_size;
> +err:
> +	fclose(g);
> +
> +	return ret;
> +}
> +
> +/**
> + * write_capsule_file - write a capsule file
> + * @bin:	FILE stream
> + * @data:	Pointer to data
> + * @bin_size:	Size of data
> + *
> + * Write out data, @data, with the size @bin_size.
> + *
> + * Return:
> + * * 0  - on success
> + * * -1 - on failure
> + */
> +static int write_capsule_file(FILE *f, void *data, size_t size, const char *msg)
> +{
> +	size_t size_written;
> +
> +	size_written = fwrite(data, 1, size, f);
> +	if (size_written < size) {
> +		printf("%s: write failed (%zx != %zx)\n", msg,

Error output should be to stderr.

> +		       size_written, size);
> +		return -1;
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * create_fwbin - create an uefi capsule file
> + * @path:	Path to a created capsule file
> + * @bin:	Path to a firmware binary to encapsulate
> + * @guid:	GUID of related FMP driver
> + * @index:	Index number in capsule
> + * @instance:	Instance number in capsule
> + * @mcount:	Monotonic count in authentication information
> + * @private_file:	Path to a private key file
> + * @cert_file:	Path to a certificate file
> + *
> + * This function actually does the job of creating an uefi capsule file.
> + * All the arguments must be supplied.
> + * If either @private_file ror @cert_file is NULL, the capsule file
> + * won't be signed.
> + *
> + * Return:
> + * * 0  - on success
> + * * -1 - on failure
> + */
>   static int create_fwbin(char *path, char *bin, efi_guid_t *guid,
>   			unsigned long index, unsigned long instance)
>   {
>   	struct efi_capsule_header header;
>   	struct efi_firmware_management_capsule_header capsule;
>   	struct efi_firmware_management_capsule_image_header image;
> -	FILE *f, *g;
> -	struct stat bin_stat;
> -	u8 *data;
> -	size_t size;
> +	FILE *f;
> +	void *data;
> +	off_t bin_size;
>   	u64 offset;
> +	int ret;
>
>   #ifdef DEBUG
>   	printf("For output: %s\n", path);
> @@ -79,25 +184,28 @@ static int create_fwbin(char *path, char *bin, efi_guid_t *guid,
>   	printf("\tindex: %ld\n\tinstance: %ld\n", index, instance);
>   #endif
>
> -	g = fopen(bin, "r");
> -	if (!g) {
> -		printf("cannot open %s\n", bin);
> -		return -1;
> -	}
> -	if (stat(bin, &bin_stat) < 0) {
> -		printf("cannot determine the size of %s\n", bin);
> -		goto err_1;
> -	}
> -	data = malloc(bin_stat.st_size);
> -	if (!data) {
> -		printf("cannot allocate memory: %zx\n", (size_t)bin_stat.st_size);
> -		goto err_1;
> -	}
> +	f = NULL;
> +	data = NULL;
> +	ret = -1;
> +
> +	/*
> +	 * read a firmware binary
> +	 */
> +	if (read_bin_file(bin, &data, &bin_size))
> +		goto err;
> +
> +	/*
> +	 * write a capsule file
> +	 */
>   	f = fopen(path, "w");
>   	if (!f) {
>   		printf("cannot open %s\n", path);

Error output should be to stderr.

Best regards

Heinrich

> -		goto err_2;
> +		goto err;
>   	}
> +
> +	/*
> +	 * capsule file header
> +	 */
>   	header.capsule_guid = efi_guid_fm_capsule;
>   	header.header_size = sizeof(header);
>   	/* TODO: The current implementation ignores flags */
> @@ -105,70 +213,57 @@ static int create_fwbin(char *path, char *bin, efi_guid_t *guid,
>   	header.capsule_image_size = sizeof(header)
>   					+ sizeof(capsule) + sizeof(u64)
>   					+ sizeof(image)
> -					+ bin_stat.st_size;
> -
> -	size = fwrite(&header, 1, sizeof(header), f);
> -	if (size < sizeof(header)) {
> -		printf("write failed (%zx)\n", size);
> -		goto err_3;
> -	}
> +					+ bin_size;
> +	if (write_capsule_file(f, &header, sizeof(header),
> +			       "Capsule header"))
> +		goto err;
>
> +	/*
> +	 * firmware capsule header
> +	 * This capsule has only one firmware capsule image.
> +	 */
>   	capsule.version = 0x00000001;
>   	capsule.embedded_driver_count = 0;
>   	capsule.payload_item_count = 1;
> -	size = fwrite(&capsule, 1, sizeof(capsule), f);
> -	if (size < (sizeof(capsule))) {
> -		printf("write failed (%zx)\n", size);
> -		goto err_3;
> -	}
> +	if (write_capsule_file(f, &capsule, sizeof(capsule),
> +			       "Firmware capsule header"))
> +		goto err;
> +
>   	offset = sizeof(capsule) + sizeof(u64);
> -	size = fwrite(&offset, 1, sizeof(offset), f);
> -	if (size < sizeof(offset)) {
> -		printf("write failed (%zx)\n", size);
> -		goto err_3;
> -	}
> +	if (write_capsule_file(f, &offset, sizeof(offset),
> +			       "Offset to capsule image"))
> +		goto err;
>
> +	/*
> +	 * firmware capsule image header
> +	 */
>   	image.version = 0x00000003;
>   	memcpy(&image.update_image_type_id, guid, sizeof(*guid));
>   	image.update_image_index = index;
>   	image.reserved[0] = 0;
>   	image.reserved[1] = 0;
>   	image.reserved[2] = 0;
> -	image.update_image_size = bin_stat.st_size;
> +	image.update_image_size = bin_size;
>   	image.update_vendor_code_size = 0; /* none */
>   	image.update_hardware_instance = instance;
>   	image.image_capsule_support = 0;
> +	if (write_capsule_file(f, &image, sizeof(image),
> +			       "Firmware capsule image header"))
> +		goto err;
>
> -	size = fwrite(&image, 1, sizeof(image), f);
> -	if (size < sizeof(image)) {
> -		printf("write failed (%zx)\n", size);
> -		goto err_3;
> -	}
> -	size = fread(data, 1, bin_stat.st_size, g);
> -	if (size < bin_stat.st_size) {
> -		printf("read failed (%zx)\n", size);
> -		goto err_3;
> -	}
> -	size = fwrite(data, 1, bin_stat.st_size, f);
> -	if (size < bin_stat.st_size) {
> -		printf("write failed (%zx)\n", size);
> -		goto err_3;
> -	}
> -
> -	fclose(f);
> -	fclose(g);
> -	free(data);
> -
> -	return 0;
> +	/*
> +	 * firmware binary
> +	 */
> +	if (write_capsule_file(f, data, bin_size, "Firmware binary"))
> +		goto err;
>
> -err_3:
> -	fclose(f);
> -err_2:
> +	ret = 0;
> +err:
> +	if (f)
> +		fclose(f);
>   	free(data);
> -err_1:
> -	fclose(g);
>
> -	return -1;
> +	return ret;
>   }
>
>   /*