[v3] tools: mkeficapsule: Rework pkcs11 support

Some distros like OpenEmbedded are using gnutls library
without pkcs11 support and linking of mkeficapsule will fail.
It would make maintenance of default configs a hurdle.
Add detection of pkcs11 support in gnutls so it's enabled
when available and doesn't need to be set explicitly.

Changes:
* remove config option for pkcs11 support and add auto
  detection in Makefile
* reduce amount of ifdefs by abstracting import pkcs11
  functions
* add missing free and deinit functions

Suggested-by: Tom Rini <trini@konsulko.com>
Cc: Franz Schnyder <fra.schnyder@gmail.com>
Signed-off-by: Wojciech Dubowik <Wojciech.Dubowik@mt.com>
---
Changes in v3:
- remove config option for pkcs11 support and add auto
  detection in Makefile
- reduce amount of ifdefs by abstracting import pkcs11
  functions
- add missing free and deinit functions
Changes in v2:
- make use of stderr more consistent
- add missing ifndef around pkcs11 deinit functions
---
 tools/Makefile       |   5 ++
 tools/mkeficapsule.c | 117 ++++++++++++++++++++++++++++---------------
 2 files changed, 81 insertions(+), 41 deletions(-)

Hi Wojciech,

On 4/21/26 2:14 PM, Wojciech Dubowik wrote:
> Some distros like OpenEmbedded are using gnutls library
> without pkcs11 support and linking of mkeficapsule will fail.
> It would make maintenance of default configs a hurdle.
> Add detection of pkcs11 support in gnutls so it's enabled
> when available and doesn't need to be set explicitly.
> 
> Changes:
> * remove config option for pkcs11 support and add auto
>    detection in Makefile
> * reduce amount of ifdefs by abstracting import pkcs11
>    functions
> * add missing free and deinit functions
No, this must be a separate patch.

We also don't need a changelog between patch versions in the commit log 
(place it below the ---).

> 
> Suggested-by: Tom Rini <trini@konsulko.com>
> Cc: Franz Schnyder <fra.schnyder@gmail.com>
> Signed-off-by: Wojciech Dubowik <Wojciech.Dubowik@mt.com>
> ---
> Changes in v3:
> - remove config option for pkcs11 support and add auto
>    detection in Makefile
> - reduce amount of ifdefs by abstracting import pkcs11
>    functions
> - add missing free and deinit functions
> Changes in v2:
> - make use of stderr more consistent
> - add missing ifndef around pkcs11 deinit functions
> ---
>   tools/Makefile       |   5 ++
>   tools/mkeficapsule.c | 117 ++++++++++++++++++++++++++++---------------
>   2 files changed, 81 insertions(+), 41 deletions(-)
> 
> diff --git a/tools/Makefile b/tools/Makefile
> index 1a5f425ecdaa..e85f5a354b81 100644
> --- a/tools/Makefile
> +++ b/tools/Makefile
> @@ -271,6 +271,11 @@ mkeficapsule-objs := generated/lib/uuid.o \
>   	$(LIBFDT_OBJS) \
>   	mkeficapsule.o
>   hostprogs-always-$(CONFIG_TOOLS_MKEFICAPSULE) += mkeficapsule
> +GNUTLS_SUPPORTS_P11KIT = $(shell pkg-config --libs gnutls --print-requires-private \
> +			 2> /dev/null | grep p11-kit-1)
> +ifeq ($(GNUTLS_SUPPORTS_P11KIT),p11-kit-1)
> +HOSTCFLAGS_mkeficapsule.o += -DMKEFICAPSULE_PKCS11
> +endif
>   
>   include tools/fwumdata_src/fwumdata.mk
>   
> diff --git a/tools/mkeficapsule.c b/tools/mkeficapsule.c
> index ec640c57e8a5..747431bce8fe 100644
> --- a/tools/mkeficapsule.c
> +++ b/tools/mkeficapsule.c
> @@ -207,6 +207,45 @@ static int write_capsule_file(FILE *f, void *data, size_t size, const char *msg)
>   	return 0;
>   }
>   
> +#ifdef MKEFICAPSULE_PKCS11
> +static int import_pkcs11_crt(gnutls_x509_crt_t *x509, struct auth_context *ctx)
> +{
> +	gnutls_pkcs11_obj_t *obj_list;
> +	unsigned int obj_list_size = 0;
> +	int i, ret;
> +
> +	ret = gnutls_pkcs11_obj_list_import_url4(&obj_list, &obj_list_size,
> +						 ctx->cert_file, 0);
> +	if (ret < 0 || obj_list_size == 0)
> +		return ret;
> +
> +	ret = gnutls_x509_crt_import_pkcs11(*x509, obj_list[0]);
> +
> +	for (i = 0; i < obj_list_size; i++)
> +                gnutls_pkcs11_obj_deinit(obj_list[i]);
> +	gnutls_free(obj_list);
> +
> +	return ret;
> +}
> +
> +static int import_pkcs11_key(gnutls_privkey_t *pkey, struct auth_context *ctx)
> +{
> +	return gnutls_privkey_import_pkcs11_url(*pkey, ctx->key_file);
> +}
> +#else
> +static int import_pkcs11_crt(gnutls_x509_crt_t *x509, struct auth_context *ctx)
> +{
> +	fprintf(stderr, "Pkcs11 support is disabled\n");
> +	return -1;
> +}
> +
> +static int import_pkcs11_key(gnutls_privkey_t *pkey, struct auth_context *ctx)
> +{
> +	fprintf(stderr, "Pkcs11 support is disabled\n");
> +	return -1;
> +}
> +#endif
> +
>   /**
>    * create_auth_data - compose authentication data in capsule
>    * @auth_context:	Pointer to authentication context
> @@ -221,17 +260,14 @@ static int write_capsule_file(FILE *f, void *data, size_t size, const char *msg)
>    */
>   static int create_auth_data(struct auth_context *ctx)
>   {
> -	gnutls_datum_t cert;
> -	gnutls_datum_t key;
> +	gnutls_datum_t cert = { NULL, 0 };
> +	gnutls_datum_t key = { NULL, 0 };
>   	off_t file_size;
> -	gnutls_privkey_t pkey;
> +	gnutls_privkey_t pkey = NULL;
>   	gnutls_x509_crt_t x509;
>   	gnutls_pkcs7_t pkcs7;
> -	gnutls_datum_t data;
> -	gnutls_datum_t signature;
> -	gnutls_pkcs11_obj_t *obj_list;
> -	unsigned int obj_list_size = 0;
> -	const char *lib;
> +	gnutls_datum_t data = { NULL, 0 };
> +	gnutls_datum_t signature = { NULL, 0 };
>   	int ret;
>   	bool pkcs11_cert = false;
>   	bool pkcs11_key = false;
> @@ -242,10 +278,12 @@ static int create_auth_data(struct auth_context *ctx)
>   	if (!strncmp(ctx->key_file, "pkcs11:", strlen("pkcs11:")))
>   		pkcs11_key = true;
>   
> +#ifdef MKEFICAPSULE_PKCS11
>   	if (pkcs11_cert || pkcs11_key) {

This one also in a separate function. As far as I could tell, we should 
return -1 here if PKCS11 support isn't available in gnutls. We shouldn't 
continue if we already know we are going to fail. We do need the code 
below to be ifdef'ed (in functions like you've done is fine), because 
somehow the compiler isn't smart enough to build those out and thus 
we'll fail to build if pkcs11 support is disabled, even if we can never 
reach that part of the code.

> +		const char *lib;
>   		lib = getenv("PKCS11_MODULE_PATH");
>   		if (!lib) {
> -			fprintf(stdout,
> +			fprintf(stderr,
>   				"PKCS11_MODULE_PATH not set in the environment\n");
>   			return -1;
>   		}
> @@ -255,10 +293,11 @@ static int create_auth_data(struct auth_context *ctx)
>   
>   		ret = gnutls_pkcs11_add_provider(lib, "trusted");
>   		if (ret < 0) {
> -			fprintf(stdout, "Failed to add pkcs11 provider\n");
> +			fprintf(stderr, "Failed to add pkcs11 provider\n");
>   			return -1;
>   		}
>   	}
> +#endif
>   
>   	if (!pkcs11_cert) {
>   		ret = read_bin_file(ctx->cert_file, &cert.data, &file_size);
> @@ -296,35 +335,33 @@ static int create_auth_data(struct auth_context *ctx)
>   	if (ret < 0) {
>   		fprintf(stderr, "error in gnutls_x509_crt_init(): %s\n",
>   			gnutls_strerror(ret));
> -		return -1;
> +		goto cleanup;
>   	}
>   
>   	/* load x509 certificate */
>   	if (pkcs11_cert) {
> -		ret = gnutls_pkcs11_obj_list_import_url4(&obj_list, &obj_list_size,
> -							 ctx->cert_file, 0);
> -		if (ret < 0 || obj_list_size == 0) {
> -			fprintf(stdout, "Failed to import crt_file URI objects\n");
> -			return -1;
> +		ret =  import_pkcs11_crt(&x509, ctx);
> +		if (ret < 0) {
> +			fprintf(stderr, "error in import_pkcs11_crt(): %s\n",
> +				gnutls_strerror(ret));
> +			goto cleanup;
>   		}
> -
> -		gnutls_x509_crt_import_pkcs11(x509, obj_list[0]);
>   	} else {
>   		ret = gnutls_x509_crt_import(x509, &cert, GNUTLS_X509_FMT_PEM);
>   		if (ret < 0) {
>   			fprintf(stderr, "error in gnutls_x509_crt_import(): %s\n",
>   				gnutls_strerror(ret));
> -			return -1;
> +			goto cleanup;
>   		}
>   	}
>   
>   	/* load a private key */
>   	if (pkcs11_key) {
> -		ret = gnutls_privkey_import_pkcs11_url(pkey, ctx->key_file);
> +		ret = import_pkcs11_key(&pkey, ctx);
>   		if (ret < 0) {
> -			fprintf(stderr, "error in %d: %s\n", __LINE__,
> +			fprintf(stderr,	"error in import_pkcs11_key(): %s\n",
>   				gnutls_strerror(ret));
> -			return -1;
> +			goto cleanup;
>   		}
>   	} else {
>   		ret = gnutls_privkey_import_x509_raw(pkey, &key, GNUTLS_X509_FMT_PEM,
> @@ -333,7 +370,7 @@ static int create_auth_data(struct auth_context *ctx)
>   			fprintf(stderr,
>   				"error in gnutls_privkey_import_x509_raw(): %s\n",
>   				gnutls_strerror(ret));
> -			return -1;
> +			goto cleanup;
>   		}
>   	}
>   
> @@ -342,7 +379,7 @@ static int create_auth_data(struct auth_context *ctx)
>   	if (ret < 0) {
>   		fprintf(stderr, "error in gnutls_pkcs7_init(): %s\n",
>   			gnutls_strerror(ret));
> -		return -1;
> +		goto cleanup;
>   	}
>   
>   	/* sign */
> @@ -357,7 +394,7 @@ static int create_auth_data(struct auth_context *ctx)
>   	data.data = malloc(data.size);
>   	if (!data.data) {
>   		fprintf(stderr, "allocating memory (0x%x) failed\n", data.size);
> -		return -1;
> +		goto cleanup;
>   	}
>   	memcpy(data.data, ctx->image_data, ctx->image_size);
>   	memcpy(data.data + ctx->image_size, &ctx->auth.monotonic_count,
> @@ -371,7 +408,7 @@ static int create_auth_data(struct auth_context *ctx)
>   	if (ret < 0) {
>   		fprintf(stderr, "error in gnutls_pkcs7)sign(): %s\n",
>   			gnutls_strerror(ret));
> -		return -1;
> +		goto cleanup;
>   	}
>   
>   	/* export */
> @@ -379,7 +416,8 @@ static int create_auth_data(struct auth_context *ctx)
>   	if (ret < 0) {
>   		fprintf(stderr, "error in gnutls_pkcs7_export2: %s\n",
>   			gnutls_strerror(ret));
> -		return -1;
> +		gnutls_free(signature.data);
> +		goto cleanup;
>   	}
>   	ctx->sig_data = signature.data;
>   	ctx->sig_size = signature.size;
> @@ -391,24 +429,21 @@ static int create_auth_data(struct auth_context *ctx)
>   	ctx->auth.auth_info.hdr.wCertificateType = WIN_CERT_TYPE_EFI_GUID;
>   	memcpy(&ctx->auth.auth_info.cert_type, &efi_guid_cert_type_pkcs7,
>   	       sizeof(efi_guid_cert_type_pkcs7));
> -
> -	/*
> -	 * For better clean-ups,
> -	 * gnutls_pkcs7_deinit(pkcs7);
> -	 * gnutls_privkey_deinit(pkey);
> -	 * gnutls_x509_crt_deinit(x509);
> -	 * free(cert.data);
> -	 * free(key.data);
> -	 * if error
> -	 *   gnutls_free(signature.data);
> -	 */
> -
> +cleanup:
> +	gnutls_x509_crt_deinit(x509);
> +	if (pkey)
> +                gnutls_privkey_deinit(pkey);
> +	gnutls_pkcs7_deinit(pkcs7);
> +	gnutls_free(cert.data);
> +	gnutls_free(key.data);
> +	gnutls_free(data.data);

No, this is unrelated to this pkcs11 fixup. Please have a separate patch 
for this (same for all gotos and variables being assigned a default 
value at the top). The idea is that we should be able to revert a patch 
if something is broken, without reverting unrelated functionality (here, 
either the ability to build without pkcs11 or correctly freeing things 
on error).

> +#ifdef MKEFICAPSULE_PKCS11
>   	if (pkcs11_cert || pkcs11_key) {
>   		gnutls_global_deinit();
>   		gnutls_pkcs11_deinit();
>   	}
> -
> -	return 0;
> +#endif
> +	return ret;
>   }
>   
>   /**