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

[Masahisa Kojima]

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