[PATCH] efi_loader: Disable devices before handing over control

[Ilias Apalodimas]

Hi Heinrich

On Wed, 21 Oct 2020 at 15:42, Ilias Apalodimas
<ilias.apalodimas at linaro.org> wrote:
>
> Hi Heinrich,
>
>
> On Wed, 21 Oct 2020 at 15:35, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
> >
> > On 21.10.20 13:41, Ilias Apalodimas wrote:
> > > Hi Heinrich,
> > >
> > > On Wed, Oct 21, 2020 at 12:17:29PM +0200, Heinrich Schuchardt wrote:
> > >> On 10/21/20 9:32 AM, Ilias Apalodimas wrote:
> > >>> U-Boot Driver Model is supposed to remove devices with either
> > >>> DM_REMOVE_ACTIVE_DMA or DM_REMOVE_OS_PREPARE flags set, before exiting.
> > >>> Our bootm command does that by explicitly calling calling
> > >>> "dm_remove_devices_flags(DM_REMOVE_ACTIVE_ALL);" and we also disable any
> > >>> USB devices.
> > >>>
> > >>> The EFI equivalent is doing none of those at the moment. As a result
> > >>> probing an fTPM driver now renders it unusable in Linux. During our
> > >>> (*probe) callback we open a session with OP-TEE, which is supposed to
> > >>> close with our (*remove) callback. Since the (*remove) is never called,
> > >>> once we boot into Linux and try to probe the device again we are getting
> > >>> a busy error response. We also never free
> > >>>
> > >>> So let's fix this by mimicking what bootm does and disconnect devices
> > >>> when efi_exit_boot_services() is called. Note that for the OP-TEE case
> > >>> and in particular any subsequent bootloader that wants to use a device
> > >>> (e.g GRUB) will need to call exit_boot_services() in order to close the
> > >>> session.
> > >>
> > >> Hello Ilias,
> > >>
> > >> thanks for the patch. Adding the function calls looks correct to me,
> > >
> > > Well the only doubt I have is what if GRUB has to extend some PCRs before
> > > calling Linux? Any idea if it's currently calling ExitBootSevices?
> > > I was considering if it would be a better idea to call the device unbinding during
> > > some kind of "exit" from U-boot's EFI code. (i.e before StartImage)
> >
> > ExitBootServices() is called by the Linux EFI stub in function
> > allocate_new_fdt_and_exit_boot().
> >
> > If GRUB would call ExitBootServices(), it would not be able to launch
> > the EFI stub via StartImage().
>
> Yea that's my point. So with the current patch, you won't be able to
> access the fTPM driver from GRUB
> (or any other EFI application) until the Linux EFI stub calls exit
> boot services. Maybe calling those 2 functions in
> StartImage is a better idea?

So as it seems (over irc discussion), GRUB expects an EFI protocol to
use the fTPM device.
So removing the device on ExitBootServices is fine, since GRUB never
access the TPM directly.
U-Boot still doesn't have support for EFI_TCG2_PROTOCOL , but if we
add that GRUB will be able
to access the device via the EFI protocol.


I'll send a v2 fixing the compilation error shortly

Cheers
/Ilias
>
>
> /Ilias
> >
> > Best regards
> >
> > Heinrich
> >
> > >
> > >> but
> > >> sandbox_spl_defconfig gives me a compile time error with this patch, cf.
> > >> https://gitlab.denx.de/u-boot/custodians/u-boot-efi/-/jobs/166842
> > >>
> > >> You might want to use Travis CI before resubmitting.
> > >
> > > Ok will do.
> > >
> > > Thanks
> > > /Ilias
> > >
> > >>
> > >> Best regards
> > >>
> > >> Heinrich
> > >>
> > >>
> > >>>
> > >>> Signed-off-by: Ilias Apalodimas <ilias.apalodimas at linaro.org>
> > >>> ---
> > >>>  lib/efi_loader/efi_boottime.c | 5 +++++
> > >>>  1 file changed, 5 insertions(+)
> > >>>
> > >>> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> > >>> index bf78176217c6..25e6cf0fe719 100644
> > >>> --- a/lib/efi_loader/efi_boottime.c
> > >>> +++ b/lib/efi_loader/efi_boottime.c
> > >>> @@ -18,6 +18,8 @@
> > >>>  #include <pe.h>
> > >>>  #include <u-boot/crc.h>
> > >>>  #include <watchdog.h>
> > >>> +#include <dm/device.h>
> > >>> +#include <dm/root.h>
> > >>>
> > >>>  DECLARE_GLOBAL_DATA_PTR;
> > >>>
> > >>> @@ -1994,7 +1996,10 @@ static efi_status_t EFIAPI efi_exit_boot_services(efi_handle_t image_handle,
> > >>>                     list_del(&evt->link);
> > >>>     }
> > >>>
> > >>> +   if IS_ENABLED(CONFIG_USB_DEVICE)
> > >>> +           udc_disconnect();
> > >>>     board_quiesce_devices();
> > >>> +   dm_remove_devices_flags(DM_REMOVE_ACTIVE_ALL);
> > >>>
> > >>>     /* Patch out unsupported runtime function */
> > >>>     efi_runtime_detach();
> > >>>
> > >>
> >