[PATCH 3/5] efi_loader: image_loader: replace EFI_PRINT with log macros
AKASHI Takahiro
takahiro.akashi at linaro.org
Wed Jul 6 03:42:59 CEST 2022
Heinrich,
On Tue, Jul 05, 2022 at 05:26:58PM +0200, Heinrich Schuchardt wrote:
> On 7/5/22 07:48, AKASHI Takahiro wrote:
> > Now We are migrating from EFI_PRINT() to log macro's.
>
> I don't understand your logic:
>
> log_err("Parsing image's signature failed\n");
> log_debug("Signature was rejected by \"dbx\"\n");
I distinctively use those two macro's.
I use log_err() when such an error is *normally* NOT expected.
I think that users should always be notified of these cases.
(Say, a signing tool generates a incompatible format of image, or
a image itself is corrupted.)
On the other hand, I use log_debug() to show the detailed reason
of why the signature verification process failed.
I always enable those messages when I debug image authentication code.
Please note there is always log_err("Image not authenticated\n") if
efi_image_authenticate() fails.
-Takahiro Akashi
> Shouldn't everything leading to a signature being rejected be treated
> the same (i.e. use log_err())?
>
> Best regards
>
> Heinrich
>
> >
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi at linaro.org>
> > ---
> > lib/efi_loader/efi_image_loader.c | 54 +++++++++++++++----------------
> > 1 file changed, 27 insertions(+), 27 deletions(-)
> >
> > diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c
> > index 961139888504..fe8e4a89082c 100644
> > --- a/lib/efi_loader/efi_image_loader.c
> > +++ b/lib/efi_loader/efi_image_loader.c
> > @@ -238,7 +238,7 @@ efi_status_t efi_image_region_add(struct efi_image_regions *regs,
> > int i, j;
> >
> > if (regs->num >= regs->max) {
> > - EFI_PRINT("%s: no more room for regions\n", __func__);
> > + log_err("%s: no more room for regions\n", __func__);
> > return EFI_OUT_OF_RESOURCES;
> > }
> >
> > @@ -263,7 +263,7 @@ efi_status_t efi_image_region_add(struct efi_image_regions *regs,
> > }
> >
> > /* new data overlapping registered region */
> > - EFI_PRINT("%s: new region already part of another\n", __func__);
> > + log_err("%s: new region already part of another\n", __func__);
> > return EFI_INVALID_PARAMETER;
> > }
> >
> > @@ -434,8 +434,8 @@ bool efi_image_parse(void *efi, size_t len, struct efi_image_regions **regp,
> > bytes_hashed = opt->SizeOfHeaders;
> > align = opt->FileAlignment;
> > } else {
> > - EFI_PRINT("%s: Invalid optional header magic %x\n", __func__,
> > - nt->OptionalHeader.Magic);
> > + log_err("%s: Invalid optional header magic %x\n", __func__,
> > + nt->OptionalHeader.Magic);
> > goto err;
> > }
> >
> > @@ -445,7 +445,7 @@ bool efi_image_parse(void *efi, size_t len, struct efi_image_regions **regp,
> > nt->FileHeader.SizeOfOptionalHeader);
> > sorted = calloc(sizeof(IMAGE_SECTION_HEADER *), num_sections);
> > if (!sorted) {
> > - EFI_PRINT("%s: Out of memory\n", __func__);
> > + log_err("%s: Out of memory\n", __func__);
> > goto err;
> > }
> >
> > @@ -464,7 +464,7 @@ bool efi_image_parse(void *efi, size_t len, struct efi_image_regions **regp,
> > efi_image_region_add(regs, efi + sorted[i]->PointerToRawData,
> > efi + sorted[i]->PointerToRawData + size,
> > 0);
> > - EFI_PRINT("section[%d](%s): raw: 0x%x-0x%x, virt: %x-%x\n",
> > + log_debug("section[%d](%s): raw: 0x%x-0x%x, virt: %x-%x\n",
> > i, sorted[i]->Name,
> > sorted[i]->PointerToRawData,
> > sorted[i]->PointerToRawData + size,
> > @@ -478,7 +478,7 @@ bool efi_image_parse(void *efi, size_t len, struct efi_image_regions **regp,
> >
> > /* 3. Extra data excluding Certificates Table */
> > if (bytes_hashed + authsz < len) {
> > - EFI_PRINT("extra data for hash: %zu\n",
> > + log_debug("extra data for hash: %zu\n",
> > len - (bytes_hashed + authsz));
> > efi_image_region_add(regs, efi + bytes_hashed,
> > efi + len - authsz, 0);
> > @@ -487,18 +487,18 @@ bool efi_image_parse(void *efi, size_t len, struct efi_image_regions **regp,
> > /* Return Certificates Table */
> > if (authsz) {
> > if (len < authoff + authsz) {
> > - EFI_PRINT("%s: Size for auth too large: %u >= %zu\n",
> > - __func__, authsz, len - authoff);
> > + log_err("%s: Size for auth too large: %u >= %zu\n",
> > + __func__, authsz, len - authoff);
> > goto err;
> > }
> > if (authsz < sizeof(*auth)) {
> > - EFI_PRINT("%s: Size for auth too small: %u < %zu\n",
> > - __func__, authsz, sizeof(*auth));
> > + log_err("%s: Size for auth too small: %u < %zu\n",
> > + __func__, authsz, sizeof(*auth));
> > goto err;
> > }
> > *auth = efi + authoff;
> > *auth_len = authsz;
> > - EFI_PRINT("WIN_CERTIFICATE: 0x%x, size: 0x%x\n", authoff,
> > + log_debug("WIN_CERTIFICATE: 0x%x, size: 0x%x\n", authoff,
> > authsz);
> > } else {
> > *auth = NULL;
> > @@ -549,7 +549,7 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
> > size_t auth_size;
> > bool ret = false;
> >
> > - EFI_PRINT("%s: Enter, %d\n", __func__, ret);
> > + log_debug("%s: Enter, %d\n", __func__, ret);
> >
> > if (!efi_secure_boot_enabled())
> > return true;
> > @@ -560,7 +560,7 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
> >
> > if (!efi_image_parse(new_efi, efi_size, ®s, &wincerts,
> > &wincerts_len)) {
> > - EFI_PRINT("Parsing PE executable image failed\n");
> > + log_err("Parsing PE executable image failed\n");
> > goto out;
> > }
> >
> > @@ -569,18 +569,18 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
> > */
> > db = efi_sigstore_parse_sigdb(u"db");
> > if (!db) {
> > - EFI_PRINT("Getting signature database(db) failed\n");
> > + log_err("Getting signature database(db) failed\n");
> > goto out;
> > }
> >
> > dbx = efi_sigstore_parse_sigdb(u"dbx");
> > if (!dbx) {
> > - EFI_PRINT("Getting signature database(dbx) failed\n");
> > + log_err("Getting signature database(dbx) failed\n");
> > goto out;
> > }
> >
> > if (efi_signature_lookup_digest(regs, dbx, true)) {
> > - EFI_PRINT("Image's digest was found in \"dbx\"\n");
> > + log_debug("Image's digest was found in \"dbx\"\n");
> > goto out;
> > }
> >
> > @@ -602,12 +602,12 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
> > break;
> >
> > if (wincert->dwLength <= sizeof(*wincert)) {
> > - EFI_PRINT("dwLength too small: %u < %zu\n",
> > + log_debug("dwLength too small: %u < %zu\n",
> > wincert->dwLength, sizeof(*wincert));
> > continue;
> > }
> >
> > - EFI_PRINT("WIN_CERTIFICATE_TYPE: 0x%x\n",
> > + log_debug("WIN_CERTIFICATE_TYPE: 0x%x\n",
> > wincert->wCertificateType);
> >
> > auth = (u8 *)wincert + sizeof(*wincert);
> > @@ -617,12 +617,12 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
> > break;
> >
> > if (auth_size <= sizeof(efi_guid_t)) {
> > - EFI_PRINT("dwLength too small: %u < %zu\n",
> > + log_debug("dwLength too small: %u < %zu\n",
> > wincert->dwLength, sizeof(*wincert));
> > continue;
> > }
> > if (guidcmp(auth, &efi_guid_cert_type_pkcs7)) {
> > - EFI_PRINT("Certificate type not supported: %pUs\n",
> > + log_debug("Certificate type not supported: %pUs\n",
> > auth);
> > ret = false;
> > goto out;
> > @@ -632,14 +632,14 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
> > auth_size -= sizeof(efi_guid_t);
> > } else if (wincert->wCertificateType
> > != WIN_CERT_TYPE_PKCS_SIGNED_DATA) {
> > - EFI_PRINT("Certificate type not supported\n");
> > + log_debug("Certificate type not supported\n");
> > ret = false;
> > goto out;
> > }
> >
> > msg = pkcs7_parse_message(auth, auth_size);
> > if (IS_ERR(msg)) {
> > - EFI_PRINT("Parsing image's signature failed\n");
> > + log_err("Parsing image's signature failed\n");
> > msg = NULL;
> > continue;
> > }
> > @@ -666,13 +666,13 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
> > /* try black-list first */
> > if (efi_signature_verify_one(regs, msg, dbx)) {
> > ret = false;
> > - EFI_PRINT("Signature was rejected by \"dbx\"\n");
> > + log_debug("Signature was rejected by \"dbx\"\n");
> > goto out;
> > }
> >
> > if (!efi_signature_check_signers(msg, dbx)) {
> > ret = false;
> > - EFI_PRINT("Signer(s) in \"dbx\"\n");
> > + log_debug("Signer(s) in \"dbx\"\n");
> > goto out;
> > }
> >
> > @@ -682,7 +682,7 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
> > continue;
> > }
> >
> > - EFI_PRINT("Signature was not verified by \"db\"\n");
> > + log_debug("Signature was not verified by \"db\"\n");
> > }
> >
> >
> > @@ -698,7 +698,7 @@ out:
> > if (new_efi != efi)
> > free(new_efi);
> >
> > - EFI_PRINT("%s: Exit, %d\n", __func__, ret);
> > + log_debug("%s: Exit, %d\n", __func__, ret);
> > return ret;
> > }
> > #else
>
More information about the U-Boot
mailing list