Re: [PATCH 1/1] efi_loader: remove efi_disk_is_system_part()

[AKASHI Takahiro <takahiro.akashi@linaro.org>]

On Sat, Mar 05, 2022 at 10:37:15AM +0100, Heinrich Schuchardt wrote:
> 
> 
> On 3/5/22 02:03, AKASHI Takahiro wrote:
> > Heinrich,
> > 
> > On Sat, Mar 05, 2022 at 12:51:00AM +0100, Heinrich Schuchardt wrote:
> > > The block IO protocol may be installed on any handle. We should make
> > > no assumption about the structure the handle points to.
> > > 
> > > efi_disk_is_system_part() makes an illegal widening cast from a handle
> > > to a struct efi_disk_obj. Remove the function.
> > 
> > NAK.
> > If this is a problem, please fix the function rather than remove it.
> 
> The first priority is that the code cannot crash. Keeping the current
> implementation is not an option.
> 
> As you introduced the problematic code I would appreciate your contribution
> to the solution.
> 
> The UEFI specification requires to find the ESP on the device of the active
> boot partition. But this is not what efi_disk_is_system_part() ever did. It
> only checked if the active boot option pointed to a file on an ESP.
> 
> IsEfiSysPartitionDevicePath() in EDK II indentifies an ESP by either a
> protocol using the EFI system partition GUID or a GPT HD() device path node
> with the same GUID. I think just using the protocol should be good enough.

You seem to already know the solution.
Why not fix the problem by yourself?

-Takahiro Akashi

> Best regards
> 
> Heinrich
> 
> > 
> > > As side effect capsules might be read from non-ESP partitions.
> > 
> > UEFI specification requires that capsules be loaded from ESP.
> > 
> > -Takahiro Akashi
> > 
> > > 
> > > Fixes: Fixes: 41fd506842c2 ("efi_loader: disk: add efi_disk_is_system_part()")
> > > Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> > > ---
> > >   include/efi_loader.h         |  2 --
> > >   lib/efi_loader/efi_capsule.c |  4 +++-
> > >   lib/efi_loader/efi_disk.c    | 29 -----------------------------
> > >   3 files changed, 3 insertions(+), 32 deletions(-)
> > > 
> > > diff --git a/include/efi_loader.h b/include/efi_loader.h
> > > index e390d323a9..5ac736ebce 100644
> > > --- a/include/efi_loader.h
> > > +++ b/include/efi_loader.h
> > > @@ -539,8 +539,6 @@ efi_status_t tcg2_measure_pe_image(void *efi, u64 efi_size,
> > >   int efi_disk_create_partitions(efi_handle_t parent, struct blk_desc *desc,
> > >   			       const char *if_typename, int diskid,
> > >   			       const char *pdevname);
> > > -/* Check if it is EFI system partition */
> > > -bool efi_disk_is_system_part(efi_handle_t handle);
> > >   /* Called by bootefi to make GOP (graphical) interface available */
> > >   efi_status_t efi_gop_register(void);
> > >   /* Called by bootefi to make the network interface available */
> > > diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
> > > index 613b531b82..2e7474a5d0 100644
> > > --- a/lib/efi_loader/efi_capsule.c
> > > +++ b/lib/efi_loader/efi_capsule.c
> > > @@ -684,7 +684,9 @@ static bool device_is_present_and_system_part(struct efi_device_path *dp)
> > >   	if (!handle)
> > >   		return false;
> > > -	return efi_disk_is_system_part(handle);
> > > +	/* TODO: add system partition check */
> > > +
> > > +	return true;
> > >   }
> > >   /**
> > > diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
> > > index 45127d1768..97223537a1 100644
> > > --- a/lib/efi_loader/efi_disk.c
> > > +++ b/lib/efi_loader/efi_disk.c
> > > @@ -587,32 +587,3 @@ efi_status_t efi_disk_register(void)
> > >   	return EFI_SUCCESS;
> > >   }
> > > -
> > > -/**
> > > - * efi_disk_is_system_part() - check if handle refers to an EFI system partition
> > > - *
> > > - * @handle:	handle of partition
> > > - *
> > > - * Return:	true if handle refers to an EFI system partition
> > > - */
> > > -bool efi_disk_is_system_part(efi_handle_t handle)
> > > -{
> > > -	struct efi_handler *handler;
> > > -	struct efi_disk_obj *diskobj;
> > > -	struct disk_partition info;
> > > -	efi_status_t ret;
> > > -	int r;
> > > -
> > > -	/* check if this is a block device */
> > > -	ret = efi_search_protocol(handle, &efi_block_io_guid, &handler);
> > > -	if (ret != EFI_SUCCESS)
> > > -		return false;
> > > -
> > > -	diskobj = container_of(handle, struct efi_disk_obj, header);
> > > -
> > > -	r = part_get_info(diskobj->desc, diskobj->part, &info);
> > > -	if (r)
> > > -		return false;
> > > -
> > > -	return !!(info.bootable & PART_EFI_SYSTEM_PARTITION);
> > > -}
> > > -- 
> > > 2.34.1
> > >