[PATCH v3 1/2] efi_loader: expose efi_image_parse() even if UEFI Secure Boot is disabled

[Masahisa Kojima]

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
<masahisa.kojima at linaro.org> wrote:
>
> On Mon, 10 May 2021 at 11:07, Takahiro Akashi
> <takahiro.akashi at linaro.org> 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 <xypron.glpk at gmx.de> 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