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

[Wojciech Dubowik <Wojciech.Dubowik@mt.com>]

On Mon, Apr 20, 2026 at 12:16:38PM +0200, Quentin Schulz wrote:

Hello Quentin,

> 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.
Yeis. I could simplify it.
> 
> >   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.
> 

Well. The idea behind was that you can have mixed pkcs11/cert files when creating
capsule. This is real use case as some HSM are too expensive to store public stuff.
Rearranging it would go well behind solving the current problem of OE not being able
to compile. I can have a look into it but probably not before we solve the current
problem.

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

No idea. I have noticed it myself but I have turned a blind eye on this.
As it seems to draw more attention now maybe it would make sense to invest a
bit more time into it.

Cheers,
Wojtek

> 
> Cheers,
> Quentin