Re: [PATCH v2 4/4] mkeficapsule: add FMP Payload Header

On Thu, 2 Mar 2023 at 14:29, Takahiro Akashi takahiro.akashi@linaro.org wrote:

On Wed, Mar 01, 2023 at 06:15:22PM +0900, Masahisa Kojima wrote:

...

Current mkeficapsule tool does not provide firmware version management. EDK2 reference implementation inserts the FMP Payload Header right before the payload. It coutains the fw_version and lowest supported version.

This commit adds three new parameters required to generate the FMP Payload Header for mkeficapsule tool. '-f' indicates whether FMP Payload Header is inserted. '-v' indicates the firmware version. '-l' indicates the lowest supported version.

from user's point of view, '-f' looks redundant since '-v' or '-l' implicitly means '-f'.

I agree, thanks.

...

When mkeficapsule tool is invoked without '-f' option, FMP Payload Header is not inserted, the behavior is same as current implementation.

Signed-off-by: Masahisa Kojima masahisa.kojima@linaro.org

Newly created in v2

tools/mkeficapsule.c | 81 ++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 75 insertions(+), 6 deletions(-)

diff --git a/tools/mkeficapsule.c b/tools/mkeficapsule.c index b71537beee..e0a6948df8 100644 --- a/tools/mkeficapsule.c +++ b/tools/mkeficapsule.c @@ -29,7 +29,7 @@ static const char *tool_name = "mkeficapsule"; efi_guid_t efi_guid_fm_capsule = EFI_FIRMWARE_MANAGEMENT_CAPSULE_ID_GUID; efi_guid_t efi_guid_cert_type_pkcs7 = EFI_CERT_TYPE_PKCS7_GUID;

-static const char *opts_short = "g:i:I:v:p:c:m:o:dhAR"; +static const char *opts_short = "g:i:I:v:l:p:c:m:o:dfhAR";

enum { CAPSULE_NORMAL_BLOB = 0, @@ -41,6 +41,9 @@ static struct option options[] = { {"guid", required_argument, NULL, 'g'}, {"index", required_argument, NULL, 'i'}, {"instance", required_argument, NULL, 'I'},

• {"fmp-payload-header", no_argument, NULL, 'f'},
  

• {"fw-version", required_argument, NULL, 'v'},
  

• {"lsv", required_argument, NULL, 'l'},
  {"private-key", required_argument, NULL, 'p'},
  {"certificate", required_argument, NULL, 'c'},
  {"monotonic-count", required_argument, NULL, 'm'},
  

@@ -60,6 +63,9 @@ static void print_usage(void) "\t-g, --guid <guid string> guid for image blob type\n" "\t-i, --index <index> update image index\n" "\t-I, --instance <instance> update hardware instance\n"

•         "\t-f, --fmp-payload-header    insert fmp payload header\n"
  

•         "\t-v, --fw-version <version>  firmware version\n"
  

•         "\t-l, --lsv <version>         lowest supported version\n"
          "\t-p, --private-key <privkey file>  private key file\n"
          "\t-c, --certificate <cert file>     signer's certificate file\n"
          "\t-m, --monotonic-count <count>     monotonic count\n"
  

@@ -71,6 +77,30 @@ static void print_usage(void) tool_name); }

+#define SIGNATURE_16(A, B) ((A) | ((B) << 8)) +#define SIGNATURE_32(A, B, C, D) \

• (SIGNATURE_16(A, B) | (SIGNATURE_16(C, D) << 16))
  

+#define FMP_PAYLOAD_HDR_SIGNATURE SIGNATURE_32('M', 'S', 'S', '1')

+/**

• • struct fmp_payload_header - EDK2 header for the FMP payload
• • This structure describes the header which is preprended to the
• • FMP payload by the edk2 capsule generation scripts.
• • @signature: Header signature used to identify the header
• • @header_size: Size of the structure
• • @fw_version: Firmware versions used
• • @lowest_supported_version: Lowest supported version
• */

+struct fmp_payload_header {

• uint32_t signature;
  

• uint32_t header_size;
  

• uint32_t fw_version;
  

• uint32_t lowest_supported_version;
  

+};

/**

• auth_context - authentication context
• @key_file: Path to a private key file

@@ -95,6 +125,12 @@ struct auth_context { size_t sig_size; };

+struct fmp_payload_header_params {

• bool have_header;
  

• uint32_t fw_version;
  

• uint32_t lowest_supported_version;
  

+};

You may put those definitions in tools/eficapsule.h.

OK.

...

static int dump_sig;

/** @@ -402,6 +438,7 @@ static void free_sig_data(struct auth_context *ctx) */ static int create_fwbin(char *path, char *bin, efi_guid_t *guid, unsigned long index, unsigned long instance,

•                 struct fmp_payload_header_params *fmp_ph_params,
                  uint64_t mcount, char *privkey_file, char *cert_file,
                  uint16_t oemflags)
  

{ @@ -410,10 +447,11 @@ static int create_fwbin(char *path, char *bin, efi_guid_t *guid, struct efi_firmware_management_capsule_image_header image; struct auth_context auth_context; FILE *f;

• uint8_t *data;
  

• uint8_t *data, *new_data, *buf;
  off_t bin_size;
  uint64_t offset;
  int ret;
  

• struct fmp_payload_header payload_header;
  
  

#ifdef DEBUG fprintf(stderr, "For output: %s\n", path); @@ -423,6 +461,7 @@ static int create_fwbin(char *path, char *bin, efi_guid_t *guid, auth_context.sig_size = 0; f = NULL; data = NULL;

• new_data = NULL;
  ret = -1;
  
  /*
  

@@ -431,12 +470,31 @@ static int create_fwbin(char *path, char *bin, efi_guid_t *guid, if (read_bin_file(bin, &data, &bin_size)) goto err;

• buf = data;
  

• /* insert fmp payload header right before the payload */
  

• if (fmp_ph_params->have_header) {
  

•         new_data = malloc(bin_size + sizeof(payload_header));
  

•         if (!new_data)
  

•                 goto err;
  

•         payload_header.signature = FMP_PAYLOAD_HDR_SIGNATURE;
  

•         payload_header.header_size = sizeof(payload_header);
  

•         payload_header.fw_version = fmp_ph_params->fw_version;
  

•         payload_header.lowest_supported_version =
  

•                 fmp_ph_params->lowest_supported_version;
  

•         memcpy(new_data, &payload_header, sizeof(payload_header));
  

•         memcpy(new_data + sizeof(payload_header), data, bin_size);
  

•         buf = new_data;
  

•         bin_size += sizeof(payload_header);
  

• }
  

• /* first, calculate signature to determine its size */
  if (privkey_file && cert_file) {
          auth_context.key_file = privkey_file;
          auth_context.cert_file = cert_file;
          auth_context.auth.monotonic_count = mcount;
  

•         auth_context.image_data = data;
  

•         auth_context.image_data = buf;
          auth_context.image_size = bin_size;
  
          if (create_auth_data(&auth_context)) {
  

@@ -536,7 +594,7 @@ static int create_fwbin(char *path, char *bin, efi_guid_t *guid, /* * firmware binary */

• if (write_capsule_file(f, data, bin_size, "Firmware binary"))
  

• if (write_capsule_file(f, buf, bin_size, "Firmware binary"))
          goto err;
  
  ret = 0;
  

@@ -545,6 +603,7 @@ err: fclose(f); free_sig_data(&auth_context); free(data);

• free(new_data);
  
  return ret;
  

} @@ -644,6 +703,7 @@ int main(int argc, char **argv) unsigned long oemflags; char *privkey_file, *cert_file; int c, idx;

• struct fmp_payload_header_params fmp_ph_params = { 0 };
  
  guid = NULL;
  index = 0;
  

@@ -679,6 +739,15 @@ int main(int argc, char **argv) case 'I': instance = strtoul(optarg, NULL, 0); break;

•         case 'f':
  

•                 fmp_ph_params.have_header = true;
  

•                 break;
  

•         case 'v':
  

•                 fmp_ph_params.fw_version = strtoul(optarg, NULL, 0);
  

•                 break;
  

•         case 'l':
  

•                 fmp_ph_params.lowest_supported_version = strtoul(optarg, NULL, 0);
  

•                 break;
  

Should we check dependencies between two options? Let's say, '-v' is required if '-l' is provided, and 'fw_version' must be greater than 'lowest_supported_version'.

Yes, I will add a dependency check in the next version. Thank you for your review.

Regards, Masahisa Kojima

-Takahiro Akashi

...

          case 'p':
                  if (privkey_file) {
                          fprintf(stderr,

@@ -747,11 +816,11 @@ int main(int argc, char **argv) if (capsule_type != CAPSULE_NORMAL_BLOB) { if (create_empty_capsule(argv[argc - 1], guid, capsule_type == CAPSULE_ACCEPT) < 0) {

•                 fprintf(stderr, "Creating empty capsule failed\n");
  

•                 fprintf(stderr, "Creating empty capsulefailed\n");
                  exit(EXIT_FAILURE);
          }
  } else  if (create_fwbin(argv[argc - 1], argv[argc - 2], guid,
  

•                          index, instance, mcount, privkey_file,
  

•                          index, instance, &fmp_ph_params, mcount, privkey_file,
                           cert_file, (uint16_t)oemflags) < 0) {
          fprintf(stderr, "Creating firmware capsule failed\n");
          exit(EXIT_FAILURE);
  

-- 2.17.1