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

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

[Heinrich Schuchardt]

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.

As side effect capsules might be read from non-ESP partitions.

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

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

[AKASHI Takahiro]

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.

> 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
> 

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

[Heinrich Schuchardt]

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.

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
>>

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

[AKASHI Takahiro]

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
> > >