Re: [PATCH v2 2/2] x86: Call Shim Verify in the multiboot2 path

[Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>]

On Mon, Apr 8, 2024 at 11:42 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 28.03.2024 16:11, Ross Lagerwall wrote:
> > --- a/xen/arch/x86/efi/efi-boot.h
> > +++ b/xen/arch/x86/efi/efi-boot.h
> > @@ -3,6 +3,7 @@
> >   * is intended to be included by common/efi/boot.c _only_, and
> >   * therefore can define arch specific global variables.
> >   */
> > +#include <xen/multiboot2.h>
> >  #include <xen/vga.h>
> >  #include <asm/e820.h>
> >  #include <asm/edd.h>
> > @@ -808,9 +809,69 @@ static const char *__init get_option(const char *cmd, 
> > const char *opt)
> >      return o;
> >  }
> >
> > +#define ALIGN_UP(arg, align) \
> > +                (((arg) + (align) - 1) & ~((typeof(arg))(align) - 1))
>
> Nit: I don't think aligning the opening parentheses is an appropriate
> criteria here. Imo either
>
> #define ALIGN_UP(arg, align) \
>             (((arg) + (align) - 1) & ~((typeof(arg))(align) - 1))
>
> or
>
> #define ALIGN_UP(arg, align) \
>         (((arg) + (align) - 1) & ~((typeof(arg))(align) - 1))
>
> or
>
> #define ALIGN_UP(arg, align) \
>     (((arg) + (align) - 1) & ~((typeof(arg))(align) - 1))
>
> .

OK, will fix.

>
> > +static void __init efi_verify_dom0(uint64_t mbi_in)
> > +{
> > +    uint64_t ptr;
> > +    const multiboot2_tag_t *tag;
> > +    EFI_SHIM_LOCK_PROTOCOL *shim_lock;
> > +    EFI_STATUS status;
> > +    const multiboot2_tag_module_t *kernel = NULL;
> > +    const multiboot2_fixed_t *mbi_fix = _p(mbi_in);
> > +    static EFI_GUID __initdata shim_lock_guid = SHIM_LOCK_PROTOCOL_GUID;
> > +    static EFI_GUID __initdata global_variable_guid = EFI_GLOBAL_VARIABLE;
> > +
> > +    ptr = ALIGN_UP(mbi_in + sizeof(*mbi_fix), MULTIBOOT2_TAG_ALIGN);
> > +
> > +    for ( tag = _p(ptr); (uint64_t)tag - mbi_in < mbi_fix->total_size;
> > +          tag = _p(ALIGN_UP((uint64_t)tag + tag->size, 
> > MULTIBOOT2_TAG_ALIGN)) )
> > +    {
> > +        if ( tag->type == MULTIBOOT2_TAG_TYPE_MODULE )
> > +        {
> > +            kernel = (const multiboot2_tag_module_t *)tag;
> > +            break;
>
> This could do with a comment along the lines of what __start_xen() has
> ("Dom0 kernel is always first").

Will add.

>
> > +        }
> > +        else if ( tag->type == MULTIBOOT2_TAG_TYPE_END )
>
> Not need for "else" here (personally I find such irritating).

OK, I'll remove it.

>
> > +            break;
> > +    }
> > +
> > +    if ( !kernel )
> > +        return;
> > +
> > +    if ( (status = efi_bs->LocateProtocol(&shim_lock_guid, NULL,
> > +                                          (void **)&shim_lock)) != 
> > EFI_SUCCESS )
> > +    {
> > +        UINT32 attr;
> > +        UINT8 data;
> > +        UINTN size = sizeof(data);
> > +
> > +        status = efi_rs->GetVariable((CHAR16 *)L"SecureBoot", 
> > &global_variable_guid,
> > +                                     &attr, &size, &data);
> > +        if ( status == EFI_NOT_FOUND )
> > +            return;
> > +
> > +        if ( EFI_ERROR(status) )
> > +            PrintErrMesg(L"Could not get SecureBoot variable", status);
> > +
> > +        if ( attr != (EFI_VARIABLE_BOOTSERVICE_ACCESS | 
> > EFI_VARIABLE_RUNTIME_ACCESS) )
> > +            PrintErrMesg(L"Unexpected SecureBoot attributes", attr);
>
> This wants to be blexit(), not PrintErrMesg().

blexit() doesn't allow printing the attributes but I can call some
other function like DisplayUint() to do that before calling blexit().

>
> > +        if ( size == 1 && data == 0 )
> > +            return;
> > +
> > +        blexit(L"Could not locate shim but Secure Boot is enabled");
> > +    }
> > +
> > +    if ( (status = shim_lock->Verify(_p(kernel->mod_start),
> > +                                     kernel->mod_end - kernel->mod_start)) 
> > != EFI_SUCCESS )
> > +        PrintErrMesg(L"Dom0 kernel image could not be verified", status);
> > +}
>
> Overall this is a superset of what efi_start() does. What I'm missing from
> the description is some discussion of why what's done there is not
> sufficient (beyond the env var check, which iirc there once was a patch to
> add it). One could only then judge whether it wouldn't make sense to make
> this function uniformly used by both paths (with mbi_in suitably dealt with
> for the other case).
>

Hmm, I wasn't really looking at efi_start() verification for the
purpose of this patch series. I can update the patch so that
both paths use a common verification function and also describe
how it differs from the simple call to shim verify that currently
exists in efi_start().

Thanks,
Ross