Re: [PATCH v2] tools: mkeficapsule: Add disable pkcs11 menu option

[Quentin Schulz <quentin.schulz@cherry.de>]

Hi Wojciech,

On 4/20/26 10:38 AM, Wojciech Dubowik wrote:
> Some distros are using gnutls library without pkcs11 support
> and linking of mkeficapsule will fail. Add disable pkcs11
> option with default set to no so distros can control this
> feature with config option.
> 
> 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 v2:
> - make use of stderr more consistent
> - add missing ifndef around pkcs11 deinit functions
> ---
>   tools/Kconfig        |  8 ++++++++
>   tools/Makefile       |  3 +++
>   tools/mkeficapsule.c | 17 ++++++++++++++++-
>   3 files changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/Kconfig b/tools/Kconfig
> index ef33295b8ecd..ccc878595d3b 100644
> --- a/tools/Kconfig
> +++ b/tools/Kconfig
> @@ -114,6 +114,14 @@ config TOOLS_MKEFICAPSULE
>   	  optionally sign that file. If you want to enable UEFI capsule
>   	  update feature on your target, you certainly need this.
>   
> +config MKEFICAPSULE_DISABLE_PKCS11
> +	bool "Disable pkcs11 support"
> +	depends on TOOLS_MKEFICAPSULE
> +	default n

n is the default, so please don't specify it.

> +	help
> +	  Disable pkcs11 support. Can be used in cases when host GnuTLS
> +	  library doesn't support it.
> +
>   menuconfig FSPI_CONF_HEADER
>   	bool "FlexSPI Header Configuration"
>   	help
> diff --git a/tools/Makefile b/tools/Makefile
> index 1a5f425ecdaa..60e84bfbf20d 100644
> --- a/tools/Makefile
> +++ b/tools/Makefile
> @@ -271,6 +271,9 @@ mkeficapsule-objs := generated/lib/uuid.o \
>   	$(LIBFDT_OBJS) \
>   	mkeficapsule.o
>   hostprogs-always-$(CONFIG_TOOLS_MKEFICAPSULE) += mkeficapsule
> +ifeq ($(CONFIG_MKEFICAPSULE_DISABLE_PKCS11),y)
> +HOSTCFLAGS_mkeficapsule.o += -DCONFIG_MKEFICAPSULE_DISABLE_PKCS11
> +endif
>   

Is this really needed?

Have

config TOOLS_MKEFICAPSULE_DISABLE_PKCS11

in the Kconfig. Then in the code simply use

#if !CONFIG_IS_ENABLED(MKEFICAPSULE_DISABLE_PKCS11)

and it'll be fine.

>   include tools/fwumdata_src/fwumdata.mk
>   
> diff --git a/tools/mkeficapsule.c b/tools/mkeficapsule.c
> index ec640c57e8a5..2f6e22626c51 100644
> --- a/tools/mkeficapsule.c
> +++ b/tools/mkeficapsule.c
> @@ -229,9 +229,11 @@ static int create_auth_data(struct auth_context *ctx)
>   	gnutls_pkcs7_t pkcs7;
>   	gnutls_datum_t data;
>   	gnutls_datum_t signature;
> +#ifndef CONFIG_MKEFICAPSULE_DISABLE_PKCS11
>   	gnutls_pkcs11_obj_t *obj_list;
>   	unsigned int obj_list_size = 0;
>   	const char *lib;

Reduce the scope of those variables so we don't have to have an ifdef here.

> +#endif
>   	int ret;
>   	bool pkcs11_cert = false;
>   	bool pkcs11_key = false;
> @@ -242,6 +244,7 @@ static int create_auth_data(struct auth_context *ctx)
>   	if (!strncmp(ctx->key_file, "pkcs11:", strlen("pkcs11:")))
>   		pkcs11_key = true;
>   
> +#ifndef CONFIG_MKEFICAPSULE_DISABLE_PKCS11
>   	if (pkcs11_cert || pkcs11_key) {
>   		lib = getenv("PKCS11_MODULE_PATH");
>   		if (!lib) {
> @@ -259,6 +262,7 @@ static int create_auth_data(struct auth_context *ctx)
>   			return -1;
>   		}
>   	}
> +#endif
>   

This is getting kinda ugly. I'm wondering if it wouldn't be more 
readable to move the pkcs11-specific code into specific functions. You 
call the function from create_auth_data() and you have two definitions 
of the function, one when CONFIG_MKEFICAPSULE_DISABLE_PKCS11 is enabled, 
one for when it's not.

Something like

#if CONFIG_IS_ENABLED(MKEFICAPSULE_DISABLE_PKCS11)
static int mkeficapsule_import_pkcs11_crt(...)
{
     fprintf(stdout, "Pkcs11 support is disabled\n");
     return -1;
}
#else
static int mkeficapsule_import_pkcs11_crt(...)
{
[...]
}
#endif

[...]

static int create_auth_data(struct auth_context *ctx)
{
[...]

     if (pkcs11_cert) {
         ret = mkeficapsule_import_pkcs11_crt(...);
         if (ret < 0) {
             fprintf(stdout, "Failed to import crt: %d\n", ret);
             return ret;
         }
     }
[...]
}

Also, I think there's a missing free() after the data.data malloc if 
there's a fail (or maybe in the event of a success, I haven't followed 
if it gets freed later on). I see a comment of a few lines saying 
"better cleanups" and I'm wondering why we don't do them? Any idea why?

Cheers,
Quentin