Hi Heinrich, I'm about to send v4 patch series.
> 1) keep if (!IS_ENABLED(CONFIG_EFI_SECURE_BOOT)) condition I chose this option, but I reverted #ifdef statement instead of using "if (IS_ENABLED)" because I think it is better not to rely on compiler optimization. > 2) remove if (!IS_ENABLED(CONFIG_EFI_SECURE_BOOT)) condition and > always include efi_signature.c as compilation target. In this option, CONFIG_PKCS7_VERIFY is required for EFI_TCG2_PROTOCOL just for successful build. To minimize dependency, I did not proceed with 2). Please kindly review v4. Thanks, Masahisa On Tue, 11 May 2021 at 07:06, Masahisa Kojima <[email protected]> wrote: > > On Mon, 10 May 2021 at 11:07, Takahiro Akashi > <[email protected]> wrote: > > > > On Mon, May 10, 2021 at 09:49:03AM +0900, Masahisa Kojima wrote: > > > Hi Heinrich, > > > > > > Sorry for the late reply. > > > > > > On Sat, 8 May 2021 at 23:08, Heinrich Schuchardt <[email protected]> > > > wrote: > > > > > > > > On 4/28/21 3:16 PM, Heinrich Schuchardt wrote: > > > > > On 28.04.21 14:19, Masahisa Kojima wrote: > > > > <snip /> > > > > >> /** > > > > >> * cmp_pe_section() - compare virtual addresses of two PE image > > > > >> sections > > > > >> * @arg1: pointer to pointer to first section header > > > > >> @@ -504,6 +565,9 @@ static bool efi_image_authenticate(void *efi, > > > > >> size_t efi_size) > > > > >> > > > > >> EFI_PRINT("%s: Enter, %d\n", __func__, ret); > > > > >> > > > > >> + if (!IS_ENABLED(CONFIG_EFI_SECURE_BOOT)) > > > > >> + return true; > > > > >> + > > > > > > > > > > Why is this needed? Doesn't efi_secure_boot_enabled() return false in > > > > > this case? > > > > > > The original code is as follows. > > > > Heinrich's concern was, I guess, that > > > > > > >> + if (!IS_ENABLED(CONFIG_EFI_SECURE_BOOT)) > > > > >> + return true; > > > > and the succeeding check, > > > > if (!efi_secure_boot_enabled()) > > return true; > > > > are somehow redundant. > > But in the latter case, I'm afraid that a compiler cannot optimize out > > the rest of the logic in efi_image_authenticate(). > > Hi Heinrich, Takahiro, > > Sorry for the late reply. > I now understand Takahiro's concern. > If I remove following check, > > > + if (!IS_ENABLED(CONFIG_EFI_SECURE_BOOT)) > > + return true; > > compiler optimization does not work and link error occurs. > > lib/built-in.o: In function `efi_image_authenticate': > /home/ubuntu/SynQuacer/ledge/u-boot/lib/efi_loader/efi_image_loader.c:601: > undefined reference to `efi_sigstore_parse_sigdb' > /home/ubuntu/SynQuacer/ledge/u-boot/lib/efi_loader/efi_image_loader.c:607: > undefined reference to `efi_sigstore_parse_sigdb' > /home/ubuntu/SynQuacer/ledge/u-boot/lib/efi_loader/efi_image_loader.c:613: > undefined reference to `efi_signature_lookup_digest' > > I would like to propose two resolution. > > 1) keep if (!IS_ENABLED(CONFIG_EFI_SECURE_BOOT)) condition > 2) remove if (!IS_ENABLED(CONFIG_EFI_SECURE_BOOT)) condition and > always include efi_signature.c as compilation target. > > Please advise. > > Thanks, > Masahisa > > > > > > -Takahiro Akashi > > > > > > > #ifdef CONFIG_EFI_SECURE_BOOT > > > static bool efi_image_authenticate(void *efi, size_t efi_size) { > > > > > > < snip > > > > > > > } > > > #else > > > static bool efi_image_authenticate(void *efi, size_t efi_size) > > > { > > > return true; > > > } > > > #endif /* CONFIG_EFI_SECURE_BOOT */ > > > > > > The purpose of this commit is removing #if compilation switch, > > > so I keep the original implementation, always return true > > > if CONFIG_EFI_SECURE_BOOT is disabled. > > > > > > Thanks, > > > Masahisa > > > > > > > > > > > Hello Masahisa, > > > > > > > > I did not see any reply yet. Was a mail lost? > > > > > > > > Best regards > > > > > > > > Heinrich
