Re: [PATCH 1/2] xen/efi: Handle cases where file didn't come from ESP

[Frediano Ziglio <frediano.ziglio@xxxxxxxxx>]

On Tue, Jun 24, 2025 at 1:38 PM Marek Marczykowski-Górecki
<marmarek@xxxxxxxxxxxxxxxxxxxxxx> wrote:
>
> On Tue, Jun 24, 2025 at 09:31:54AM +0100, Frediano Ziglio wrote:
> > A boot loader can load files from outside ESP.
> > In these cases device could be not provided or path could
> > be something not supported.
> > In these cases allows to boot anyway, all information
> > could be provided using UKI or using other boot loader
> > features.
> >
> > Signed-off-by: Frediano Ziglio <frediano.ziglio@xxxxxxxxx>
> > ---
> >  xen/common/efi/boot.c | 27 +++++++++++++++++++++++++--
> >  1 file changed, 25 insertions(+), 2 deletions(-)
> >
> > diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
> > index 1a9b4e7dae..2a49c6d05d 100644
> > --- a/xen/common/efi/boot.c
> > +++ b/xen/common/efi/boot.c
> > @@ -443,6 +443,18 @@ static EFI_FILE_HANDLE __init get_parent_handle(const 
> > EFI_LOADED_IMAGE *loaded_i
> >      CHAR16 *pathend, *ptr;
> >      EFI_STATUS ret;
> >
> > +    /*
> > +     * In some cases the image could not come from a specific device.
> > +     * For instance this can happen if Xen was loaded using GRUB2 "linux"
> > +     * command.
> > +     */
> > +    *leaf = buffer;
>
> This feels wrong, if DeviceHandle is NULL, it will point at the
> empty buffer that shouldn't really be used for anything anyway.
>

Yes, this was done just to make the compiler happy, I changed to
assign NULL instead.

> IMO a better option would be to add "&& dir_handle" to the condition
> guarding use of file_name in efi_start() instead.
>

Yes, it makes sense. Done.

> BTW, by my reading of get_parent_handle() theoretically it should be
> possible to get _some_ name out of loaded_image->FilePath even without
> dir_handle. But since it isn't going to be used, it's not worth it.
>
> > +    if ( !loaded_image->DeviceHandle )
> > +    {
> > +        PrintStr(L"Xen image loaded without providing a device\r\n");
> > +        return NULL;
> > +    }
> > +
> >      do {
> >          EFI_FILE_IO_INTERFACE *fio;
> >
> > @@ -466,7 +478,15 @@ static EFI_FILE_HANDLE __init get_parent_handle(const 
> > EFI_LOADED_IMAGE *loaded_i
> >
> >          if ( DevicePathType(dp) != MEDIA_DEVICE_PATH ||
> >               DevicePathSubType(dp) != MEDIA_FILEPATH_DP )
> > -            blexit(L"Unsupported device path component");
> > +        {
> > +            /*
> > +             * The image could come from an unsupported device.
> > +             * For instance this can happen if Xen was loaded using GRUB2
> > +             * "chainloader" command and the file was not from ESP.
> > +             */
> > +            PrintStr(L"Unsupported device path component\r\n");
> > +            return NULL;
> > +        }
> >
> >          if ( *buffer )
> >          {
> > @@ -772,6 +792,8 @@ static bool __init read_file(EFI_FILE_HANDLE 
> > dir_handle, CHAR16 *name,
> >
> >      if ( !name )
> >          PrintErrMesg(L"No filename", EFI_OUT_OF_RESOURCES);
> > +    if ( !dir_handle )
> > +        return false;
>
> There are a lot of places where read_file() is used without checking its
> return value. Which made sense since before this change the only cases
> where read_file() would return false was for the config file, in all
> other cases it handled errors via blexit().
> Most of those read_file() calls seems to be fine (as in, will not
> explode), but may still be confusing. For example when you embed a
> config with "xsm=policy" (but the actual policy is not embedded) now the
> failure to load it will result just a warning ("Xen image loaded without
> providing a device") not even related to the file name and it will
> continue booting with unintended configuration.
>

Yes, it makes sense. Changing the code to handle dir_handle == NULL as
Open returning EFI_NOT_FOUND so all failures (beside configuration)
will become fatal as before.

OT: the flow of read_file (specifically "what" handling) looks
weird... can I change it?

> For me it looks like this change is wrong: if the config specified a
> file to load (and that blob was not embedded in the UKI), and yet it
> couldn't be loaded, it should fail early.
> Is there any (new) case where where read_file() failure (when it
> actually gets to calling it) should really be non-fatal now?
>
> In relation to the next patch - such UKI should simply not specify
> ramdisk in the embedded config, to allow loading it via "initrd"
> command. This would avoid calling read_file().
>
> >      ret = dir_handle->Open(dir_handle, &FileHandle, name,
> >                             EFI_FILE_MODE_READ, 0);
> >      if ( file == &cfg && ret == EFI_NOT_FOUND )
> > @@ -1515,7 +1537,8 @@ void EFIAPI __init noreturn efi_start(EFI_HANDLE 
> > ImageHandle,
> >          efi_bs->FreePages(cfg.addr, PFN_UP(cfg.size));
> >          cfg.addr = 0;
> >
> > -        dir_handle->Close(dir_handle);
> > +        if ( dir_handle )
> > +            dir_handle->Close(dir_handle);
> >
> >          if ( gop && !base_video )
> >          {

Frediano