From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 315E5C433EF for ; Tue, 19 Apr 2022 05:49:25 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id E2B19838AE; Tue, 19 Apr 2022 07:49:22 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (2048-bit key; unprotected) header.d=linaro.org header.i=@linaro.org header.b="WurqBEVK"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 796C283A9D; Tue, 19 Apr 2022 07:49:21 +0200 (CEST) Received: from mail-wm1-x335.google.com (mail-wm1-x335.google.com [IPv6:2a00:1450:4864:20::335]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id D56F3833EC for ; Tue, 19 Apr 2022 07:49:18 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=ilias.apalodimas@linaro.org Received: by mail-wm1-x335.google.com with SMTP id o20-20020a05600c511400b0038ebbbb2ad8so776381wms.0 for ; Mon, 18 Apr 2022 22:49:18 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=xwFtFKECqTMQnJb0nW8y1m/fpGzvGGAGr+S+KBL9oHo=; b=WurqBEVKXGu9ONp0dGKerU6/PZI0IGAr94TfhD0PCNQ0rW3swVfW+U3IsVG8y0KFcC Q1JHXpTiOzjZzrcXpdvPk4d5Py1pykPKjb7Hs8KJk2kgUGNJdaZGHaV6wKC6P+JVr9aF qfu3LgZp8EPAppYRHT0XbMvnvskI7pcgeLHrChWxz5GJ0aeLEWrDjLnctI/IIq1zlfnm k2+nodAOB7RRIzwHbegjH3qiT22DtKFLAHlhLCF0l5rOyywIsCmtvp2NtPu2Lh8mi6gl oWCYExo8DCzlvFyUJHBlsZnO7pVool023c3pgb5HAhVQmgmqF43hqmdPHSt6aDMXDhbn 8SZA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=xwFtFKECqTMQnJb0nW8y1m/fpGzvGGAGr+S+KBL9oHo=; b=rvGOJaleqPhz2ULKyxlvViWYZ1Q5UiZ9Qa5dGukDlyOXZQweAWJiaB8yRuH5TtQBkz sFEw/f55caFuIGra18W5sS33iFbDU6JQH6EOm28IvfcGXh9O/q/3oNgEyxdXDng9IZis PPr5s8ciyIeyr24BYDTFUVBXv3GThbLAV80rXIenk7rEGck9KaXIFyp+RnVRDPh6ay4d BMLPSW/uIzW8CkdgmcuMiagCxp3UgApVdhYMDt38vjw9VcJjweq0lH0+hUkQIfTu2fli TttupGFc5J/U0n5RiJS9OaXKcmu3aXOaw0dQFwVyOzQZ2f8WBOu6t/L8FWlprbkdwYS7 aBWw== X-Gm-Message-State: AOAM531KZLnmdLl4SjTWyDDLTZeA8h7VLbYRktPXWgFLZGGmLpeJq5++ o637/EHsWNarxIbT/cEQNHvcXxq4CNcIuQ== X-Google-Smtp-Source: ABdhPJyIxrNReaWU48p0HdabXJ3OpeXY52kaB1t9p0cMxkhijKmkmDFNc6ASVOArR5Uz99UcOWf3cw== X-Received: by 2002:a1c:7707:0:b0:38e:b8bc:f0a9 with SMTP id t7-20020a1c7707000000b0038eb8bcf0a9mr18458991wmi.125.1650347358462; Mon, 18 Apr 2022 22:49:18 -0700 (PDT) Received: from hera (athedsl-4461779.home.otenet.gr. [94.71.4.195]) by smtp.gmail.com with ESMTPSA id v14-20020a7bcb4e000000b0034492fa24c6sm15130237wmj.34.2022.04.18.22.49.17 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 18 Apr 2022 22:49:17 -0700 (PDT) Date: Tue, 19 Apr 2022 08:49:15 +0300 From: Ilias Apalodimas To: AKASHI Takahiro , xypron.glpk@gmx.de, Stuart.Yoder@arm.com, paul.liu@linaro.org, u-boot@lists.denx.de Subject: Re: [PATCH 1/2] efi_loader: add sha384/512 on certificate revocation Message-ID: References: <20220418180724.1855888-1-ilias.apalodimas@linaro.org> <20220419014643.GA47455@laputa> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20220419014643.GA47455@laputa> X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.5 at phobos.denx.de X-Virus-Status: Clean Akashi-san, > > + { > > + EFI_CERT_X509_SHA256_GUID, > > + "sha256", > > + 256, > > => SHA256_SUM_LEN * 8 > Sure > > + }, > > + { > > + EFI_CERT_SHA256_GUID, > > + "sha256", > > + 256, > > + }, > > + { > > + EFI_CERT_X509_SHA384_GUID, > > + "sha384", > > + 384, > > + }, > > + { > > + EFI_CERT_X509_SHA512_GUID, > > + "sha512", > > + 512, > > + }, > > +}; > > + > > +#define MAX_GUID_TO_HASH_COUNT ARRAY_SIZE(guid_to_hash) > > + > > +/** guid_to_sha_str - return the sha string e.g "sha256" for a given guid > > + * used on EFI security databases > > + * > > + * @guid: guid to check > > + * > > + * Return: len or 0 if no match is found > > + */ > > +const char *guid_to_sha_str(const efi_guid_t *guid) > > +{ > > + size_t i; > > + > > + for (i = 0; i < MAX_GUID_TO_HASH_COUNT; i++) { > > + if (!guidcmp(guid, &guid_to_hash[i].guid)) > > + return guid_to_hash[i].algo; > > + } > > + > > + return NULL; > > +} > > + > > +/** guid_to_sha_len - return the sha size in bytes for a given guid > > + * used on EFI security databases > > + * > > + * @guid: guid to check > > + * > > + * Return: len or 0 if no match is found > > + */ > > +int guid_to_sha_len(const efi_guid_t *guid) > > +{ > > + size_t i; > > + > > + for (i = 0; i < MAX_GUID_TO_HASH_COUNT; i++) { > > + if (!guidcmp(guid, &guid_to_hash[i].guid)) > > + return guid_to_hash[i].bits / 8; > > + } > > + > > + return 0; > > +} > > If we have algo_to_len(), we don't need guid_to_sha_len(). True, I'll get rid of this > > > + > > +/** algo_to_len - return the sha size in bytes for a given string > > + * > > + * @guid: string to check > > + * > > + * Return: len or 0 if no match is found > > + */ > > +int algo_to_len(const char *algo) > > This function seems to be quite generic and useful for others. > Why not implement it along with hash_calculate() in hash-checksum.c > (without using guid_to_hash[]). > Tbh I have similar stuff in efi_tcg2.c, but they are all bound to EFI atm. We could refactor all of the structures a bit and have a more generic version. Can we leave it for now and I can send a refactoring series after this gets merged? > > +{ > > + size_t i; > > + > > + for (i = 0; i < MAX_GUID_TO_HASH_COUNT; i++) { > > + if (!strcmp(algo, guid_to_hash[i].algo)) > > + return guid_to_hash[i].bits / 8; > > + } > > + > > + return 0; > > +} > > diff --git a/lib/efi_loader/efi_signature.c b/lib/efi_loader/efi_signature.c > > index 79ed077ae7dd..cf01f21b4d04 100644 > > --- a/lib/efi_loader/efi_signature.c > > +++ b/lib/efi_loader/efi_signature.c > > @@ -24,6 +24,8 @@ const efi_guid_t efi_guid_sha256 = EFI_CERT_SHA256_GUID; > > const efi_guid_t efi_guid_cert_rsa2048 = EFI_CERT_RSA2048_GUID; > > const efi_guid_t efi_guid_cert_x509 = EFI_CERT_X509_GUID; > > const efi_guid_t efi_guid_cert_x509_sha256 = EFI_CERT_X509_SHA256_GUID; > > +const efi_guid_t efi_guid_cert_x509_sha384 = EFI_CERT_X509_SHA384_GUID; > > +const efi_guid_t efi_guid_cert_x509_sha512 = EFI_CERT_X509_SHA512_GUID; > > const efi_guid_t efi_guid_cert_type_pkcs7 = EFI_CERT_TYPE_PKCS7_GUID; > > > > static u8 pkcs7_hdr[] = { > > @@ -124,23 +126,32 @@ struct pkcs7_message *efi_parse_pkcs7_header(const void *buf, > > * Return: true on success, false on error > > */ > > static bool efi_hash_regions(struct image_region *regs, int count, > > - void **hash, size_t *size) > > + void **hash, const char *hash_algo, int *len) > > { > > + int ret; > > + > > + if (!hash_algo || !len) > > + return false; > > + > > + *len = algo_to_len(hash_algo); > > Please modify it this way; > int hash_len; > > hash_len = algo_to_len(hash_algo); > if (len) > *len = hash_len; > > Then use hash_len instead of *len hereafterr. > > This way, we don't have to define a unused local variable below code. Sure [...] > > + > > + /* We just need any sha256 algo to start the matching */ > > + hash_algo = guid_to_sha_str(&efi_guid_sha256); > > Why not check if hash_algo is NULL? > > > + len = guid_to_sha_len(&efi_guid_sha256); > > => len = algo_to_len(hash_algo); > > Or even we don't need this line as efi_has_regions() will set it for us. > Yea good catch. I was playing with a version where len wasn't a ptr in efi_hash_regions() and that's a leftover (as well as guid_to_sha_len()). [...] Thanks for having a look /Ilias