[v2,4/4] mkeficapsule: add FMP Payload Header

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.

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(-)

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'.

> 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.

>  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'.

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

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