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 1947DC43334 for ; Wed, 6 Jul 2022 01:43:11 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id C467784511; Wed, 6 Jul 2022 03:43:09 +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="nBGFtRC8"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 7D8058451A; Wed, 6 Jul 2022 03:43:08 +0200 (CEST) Received: from mail-pl1-x62f.google.com (mail-pl1-x62f.google.com [IPv6:2607:f8b0:4864:20::62f]) (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 84A9682102 for ; Wed, 6 Jul 2022 03:43:05 +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=takahiro.akashi@linaro.org Received: by mail-pl1-x62f.google.com with SMTP id 5so6554436plk.9 for ; Tue, 05 Jul 2022 18:43:05 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:mail-followup-to:references :mime-version:content-disposition:in-reply-to; bh=D5/HqJDcGT6q7bKMsn+F/NnUnxcyRZ+2AJ2SzkoL7o4=; b=nBGFtRC80kP3URbFi+uT6uvkC2zATxgz27oWboUE93w2WVdNtHCq2hAhpRbLW7cRwD rtJRPg9Bpkvj6NRm2cCywzn9E4IHpj2/YdUjBqCw2hGTt+cj2qvolOnlllIyQi4pXbwZ gEJ5Aa+XmgKzitH+IYpOiomrtdYZlJTnJxkGVw8nZvjeaoC8a0Ij4CrGaTYcXNQf58mY TgU2zOaN5bu4w4MsQ+KEKRKrFzcl+q8D4eLKT1HCJcLxGd7bQlIVdpvtT/J6Okut9moh WW4Pf4c0B9N7kWlWd7u78fUwx0kJCMafxd9wR8y43sd9SYKoXKLkmIdPZfsPsYFePTy5 CLWg== 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:cc:subject:message-id :mail-followup-to:references:mime-version:content-disposition :in-reply-to; bh=D5/HqJDcGT6q7bKMsn+F/NnUnxcyRZ+2AJ2SzkoL7o4=; b=yTcjp8tUHXFzN2tNDzkNmPRgLL5rUiX5XFNVh46fFq+37ocDadShHU6zzzCL93T5ji MZl4Yrv9IQnzeXFk98V82LqrTLU+J4BS88RXKlcWJDFopAX9sbYsy4k264vfOyINnc25 8+QRj36krt1drPXx/6JEneB89Vzh62XvMtFoatOJud8t0w0phYM9wy0tYH4wvRW7N4r6 CkcZKlqcqnQ29/vHCNXJegs+qnjrnYYBn20FZZr65S87GWcLeZ6qnMreiY5prBxPpKKy ywM0N+peD+3oqp/C9A8k3RTAFTQJWkHYOplP6f3aYDHwUMd9bZTHT3b88Sf+nHiYoFqg lsOQ== X-Gm-Message-State: AJIora/iIxoP+jKVZenvd8el5dS0OPuPbs2jOt4xH/M04KeIUwknXuTh hTJFkGsfizkhFTYoqb4soxtxNQ== X-Google-Smtp-Source: AGRyM1v2MzViZcoel28d8KBjSHDKdweB7M/k0W1HYAo5/vFtQhVdK9ctvriceRjAXM7/J0QBJXhomQ== X-Received: by 2002:a17:903:24c:b0:16b:a320:1d31 with SMTP id j12-20020a170903024c00b0016ba3201d31mr37752596plh.115.1657071783736; Tue, 05 Jul 2022 18:43:03 -0700 (PDT) Received: from laputa ([2400:4050:c3e1:100:a4a2:c8c3:a3dd:4eca]) by smtp.gmail.com with ESMTPSA id u5-20020a170903124500b001690a7df347sm8293325plh.96.2022.07.05.18.43.01 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 05 Jul 2022 18:43:03 -0700 (PDT) Date: Wed, 6 Jul 2022 10:42:59 +0900 From: AKASHI Takahiro To: Heinrich Schuchardt Cc: ilias.apalodimas@linaro.org, baocheng.su@siemens.com, jan.kiszka@siemens.com, u-boot@lists.denx.de Subject: Re: [PATCH 3/5] efi_loader: image_loader: replace EFI_PRINT with log macros Message-ID: <20220706014259.GC42673@laputa> Mail-Followup-To: AKASHI Takahiro , Heinrich Schuchardt , ilias.apalodimas@linaro.org, baocheng.su@siemens.com, jan.kiszka@siemens.com, u-boot@lists.denx.de References: <20220705054815.30318-1-takahiro.akashi@linaro.org> <20220705054815.30318-4-takahiro.akashi@linaro.org> <0f2d88ab-f5ae-cdcf-6519-e791f2585c39@gmx.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <0f2d88ab-f5ae-cdcf-6519-e791f2585c39@gmx.de> 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.6 at phobos.denx.de X-Virus-Status: Clean 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 > > --- > > 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 >