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

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

On Tue, Nov 16, 2021 at 01:19:10PM +0100, Heinrich Schuchardt wrote:
> On 11/16/21 05:32, AKASHI Takahiro wrote:
> > Abstract common routines to make the code easily understandable.
> > No functional change.
> > 
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > Reviewed-by: Simon Glass <sjg@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 messages should be written to stderr. Use perror(bin) to get a
> message like:
> 
>     <filename>: No such file or directory
> 
> Or use fprintf(stderr, ...).

OK.
But remember that I have used printf() throughout mkeficapsule.c
even before this patch.
So I will add similar changes in patch#1 for consistency. 

> > +		return -1;
> > +	}
> > +	if (stat(bin, &bin_stat) < 0) {
> > +		printf("cannot determine the size of %s\n", bin);
> > +		ret = -1;
> > +		goto err;
> > +	}
> > +	if (bin_stat.st_size > (u32)~0U) {
> > +		printf("file size is too large: %s\n", bin);
> > +		ret = -1;
> > +		goto err;
> > +	}
> > +	buf = malloc(bin_stat.st_size);
> 
> For a symbolic link st_size is the length of the pathname it contains.
> So this function is of no help here.

Not true.
According to stat() manpage,

"lstat() is identical to stat(), except that if pathname is  a  symbolic
 link,  then  it returns information about the link itself, not the file
 that it refers to."

So stat() is a right system call.


> > +	if (!buf) {
> > +		printf("cannot allocate memory: %zx\n",
> > +		       (size_t)bin_stat.st_size);
> > +		ret = -1;
> > +		goto err;
> > +	}
> > +
> > +	size = fread(buf, 1, bin_stat.st_size, g);
> > +	if (size < bin_stat.st_size) {
> 
> fread() reads from a stream which is not necessarily a file but can be a
> named pipe. stat() does not indicate when the stream will end.
> 
> You have to run fread() in a loop until feof() or ferror() indicate that
> there is nothing more to be read from the stream.

No.
"a stream which is not necessarily a file but can be a named pipe"
may be true, but we don't handle such a use case with this tool.

-Takahiro Akashi

> Best regards
> 
> Heinrich
> 
> > +		printf("read failed (%zx)\n", size);
> > +		ret = -1;
> > +		goto err;
> > +	}
> > +
> > +	*data = buf;
> > +	*bin_size = bin_stat.st_size;
> > +err:
> > +	fclose(g);
> > +
> > +	return ret;
> > +}
> > +
> > +/**
> > + * write_capsule_file - write a capsule file
> > + * @bin:	FILE stream
> > + * @data:	Pointer to data
> > + * @bin_size:	Size of data
> > + *
> > + * Write out data, @data, with the size @bin_size.
> > + *
> > + * Return:
> > + * * 0  - on success
> > + * * -1 - on failure
> > + */
> > +static int write_capsule_file(FILE *f, void *data, size_t size, const char *msg)
> > +{
> > +	size_t size_written;
> > +
> > +	size_written = fwrite(data, 1, size, f);
> > +	if (size_written < size) {
> > +		printf("%s: write failed (%zx != %zx)\n", msg,
> > +		       size_written, size);
> > +		return -1;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +/**
> > + * create_fwbin - create an uefi capsule file
> > + * @path:	Path to a created capsule file
> > + * @bin:	Path to a firmware binary to encapsulate
> > + * @guid:	GUID of related FMP driver
> > + * @index:	Index number in capsule
> > + * @instance:	Instance number in capsule
> > + * @mcount:	Monotonic count in authentication information
> > + * @private_file:	Path to a private key file
> > + * @cert_file:	Path to a certificate file
> > + *
> > + * This function actually does the job of creating an uefi capsule file.
> > + * All the arguments must be supplied.
> > + * If either @private_file ror @cert_file is NULL, the capsule file
> > + * won't be signed.
> > + *
> > + * Return:
> > + * * 0  - on success
> > + * * -1 - on failure
> > + */
> >   static int create_fwbin(char *path, char *bin, efi_guid_t *guid,
> >   			unsigned long index, unsigned long instance)
> >   {
> >   	struct efi_capsule_header header;
> >   	struct efi_firmware_management_capsule_header capsule;
> >   	struct efi_firmware_management_capsule_image_header image;
> > -	FILE *f, *g;
> > -	struct stat bin_stat;
> > -	u8 *data;
> > -	size_t size;
> > +	FILE *f;
> > +	void *data;
> > +	off_t bin_size;
> >   	u64 offset;
> > +	int ret;
> > 
> >   #ifdef DEBUG
> >   	printf("For output: %s\n", path);
> > @@ -79,25 +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);
> > -		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;
> >   }
> > 
> >   /*
> > 
>