Hi Quentin, On Wed, 3 Jun 2026 at 11:16, Quentin Schulz <[email protected]> wrote: > > Hi Simon, > > There's a v6 already, please post your reviews there again if applicable. > > On 6/3/26 6:43 PM, Simon Glass wrote: > > Hi Wojciech, > > > > On 2026-05-28T08:03:54, Wojciech Dubowik <[email protected]> wrote: > >> 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. > >> > >> Suggested-by: Tom Rini <[email protected]> > >> Cc: Franz Schnyder <[email protected]> > >> Signed-off-by: Wojciech Dubowik <[email protected]> > >> Acked-by: Quentin Schulz <[email protected]> > >> > >> tools/Makefile | 5 +++ > >> tools/mkeficapsule.c | 95 > >> +++++++++++++++++++++++++++++++++++++++------------- > >> 2 files changed, 77 insertions(+), 23 deletions(-) > > > >> diff --git a/tools/mkeficapsule.c b/tools/mkeficapsule.c > >> @@ -207,6 +207,71 @@ static int write_capsule_file(FILE *f, void *data, > >> size_t size, const char *msg) > >> +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 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; > > > > !obj_list_size > > > > Behaviour change: when obj_list_size == 0 but ret >= 0, the caller's > > 'if (ret < 0)' check passes and execution continues with an empty > > obj_list. Please return -1 explicitly in that case. > > > > Yes, good catch, we can have ret == 0 here when obj_list_size==0 and > thus return 0. > > >> +static int import_pkcs11_crt(gnutls_x509_crt_t *x509, struct auth_context > >> *ctx) > >> +{ > > > > ... > >> + ret = gnutls_x509_crt_import_pkcs11(*x509, obj_list[0]); > > ... > >> +static int import_pkcs11_key(gnutls_privkey_t *pkey, struct auth_context > >> *ctx) > >> +{ > >> + return gnutls_privkey_import_pkcs11_url(*pkey, ctx->key_file); > >> +} > > > > gnutls_x509_crt_t and gnutls_privkey_t are already pointer typedefs > > and neither function reassigns the handle, so the extra indirection > > buys nothing. Please pass the handles by value and drop the '&' at the > > call sites. > > > >> +#else > >> +static int pkcs11_init(void) > >> +{ > >> + fprintf(stderr, "Pkcs11 support is disabled\n"); > >> + return -1; > >> +} > >> + > >> +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 > > > > "PKCS#11 support..." > > > > import_pkcs11_crt() and import_pkcs11_key() are only reached after > > pkcs11_init() has already failed with the same message, so these two > > stubs look like dead code. > > > > How do you compile the code if you don't have those stubs?
I haven't tried building it, but just from the look of it, it seems that the procedure will fail if pkcs11_init() is called. So why do we need stubs for the other two functions, which are called later? I suggest putting the #if inside the functions rather than having entirely separate stubs. But anyway as this is already on v6, my feedback is minor and I should have noticed this earlier... Reviewed-by: Simon Glass <[email protected]> Regards, Simon
-=-=-=-=-=-=-=-=-=-=-=- Links: You receive all messages sent to this group. View/Reply Online (#238134): https://lists.openembedded.org/g/openembedded-core/message/238134 Mute This Topic: https://lists.openembedded.org/mt/119526819/21656 Group Owner: [email protected] Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub [[email protected]] -=-=-=-=-=-=-=-=-=-=-=-
