| Message ID | 20211220050253.31163-2-takahiro.akashi@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | efi_loader: capsule: improve capsule authentication support | expand |
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@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 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; > } > > /*
On Sat, Jan 01, 2022 at 10:35:04PM +0100, Heinrich Schuchardt wrote: > 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@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 output should be to stderr. Ah, I've forgot to fix it. > > + 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? OK. -Takahiro Akashi > > + 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; > > } > > > > /* >
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); + 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); + 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) { + 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; } /*