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

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

  /*