[PATCH v3 0/2] efi_loader: identify EFI system partition

[PATCH v3 0/2] efi_loader: identify EFI system partition

[Heinrich Schuchardt]

For storing UEFI variables we need to know where the EFI system partition
is located.

With the patches the first available EFI system partition is determined
both for MBR and GPT partition tables.

v3:
	adjust gpt command
	adjust commit message
v2:
	BIT() macro to define bit mask

Heinrich Schuchardt (2):
  part: detect EFI system partition
  efi_loader: identify EFI system partition

 cmd/gpt.c                 |  4 ++--
 disk/part_dos.c           | 14 ++++++++++----
 disk/part_efi.c           | 16 ++++++++++------
 include/efi_loader.h      |  7 +++++++
 include/part.h            | 11 ++++++++++-
 lib/efi_loader/efi_disk.c | 20 ++++++++++++++++++++
 6 files changed, 59 insertions(+), 13 deletions(-)

--
2.26.1

[PATCH v3 1/2] part: detect EFI system partition

[Heinrich Schuchardt]

Up to now for MBR and GPT partitions the info field 'bootable' was set to 1
if either the partition was an EFI system partition or the bootable flag
was set.

Turn info field 'bootable' into a bit mask with separate bits for bootable
and EFI system partition.

This will allow us to identify the EFI system partition in the UEFI
sub-system.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
v3:
	adjust gpt command to use only the boot flag not the EFI system
	partition
v2:
	used BIT() macro to define bit mask
---
 cmd/gpt.c       |  4 ++--
 disk/part_dos.c | 14 ++++++++++----
 disk/part_efi.c | 16 ++++++++++------
 include/part.h  | 11 ++++++++++-
 4 files changed, 32 insertions(+), 13 deletions(-)

diff --git a/cmd/gpt.c b/cmd/gpt.c
index efaf1bcecb..b94f0051cd 100644
--- a/cmd/gpt.c
+++ b/cmd/gpt.c
@@ -245,7 +245,7 @@ static void print_gpt_info(void)
 		printf("Block size %lu, name %s\n", curr->gpt_part_info.blksz,
 		       curr->gpt_part_info.name);
 		printf("Type %s, bootable %d\n", curr->gpt_part_info.type,
-		       curr->gpt_part_info.bootable);
+		       curr->gpt_part_info.bootable & PART_BOOTABLE);
 #ifdef CONFIG_PARTITION_UUIDS
 		printf("UUID %s\n", curr->gpt_part_info.uuid);
 #endif
@@ -535,7 +535,7 @@ static int set_gpt_info(struct blk_desc *dev_desc,

 		/* bootable */
 		if (found_key(tok, "bootable"))
-			parts[i].bootable = 1;
+			parts[i].bootable = PART_BOOTABLE;
 	}

 	*parts_count = p_count;
diff --git a/disk/part_dos.c b/disk/part_dos.c
index 83ff40d310..813379f851 100644
--- a/disk/part_dos.c
+++ b/disk/part_dos.c
@@ -45,9 +45,15 @@ static inline int is_extended(int part_type)
 	    part_type == 0x85);
 }

-static inline int is_bootable(dos_partition_t *p)
+static int get_bootable(dos_partition_t *p)
 {
-	return (p->sys_ind == 0xef) || (p->boot_ind == 0x80);
+	int ret = 0;
+
+	if (p->sys_ind == 0xef)
+		ret |= PART_EFI_SYSTEM_PARTITION;
+	if (p->boot_ind == 0x80)
+		ret |= PART_BOOTABLE;
+	return ret;
 }

 static void print_one_part(dos_partition_t *p, lbaint_t ext_part_sector,
@@ -60,7 +66,7 @@ static void print_one_part(dos_partition_t *p, lbaint_t ext_part_sector,
 		"u\t%08x-%02x\t%02x%s%s\n",
 		part_num, lba_start, lba_size, disksig, part_num, p->sys_ind,
 		(is_extended(p->sys_ind) ? " Extd" : ""),
-		(is_bootable(p) ? " Boot" : ""));
+		(get_bootable(p) ? " Boot" : ""));
 }

 static int test_block_type(unsigned char *buffer)
@@ -258,7 +264,7 @@ static int part_get_info_extended(struct blk_desc *dev_desc,
 					      (char *)info->name);
 			/* sprintf(info->type, "%d, pt->sys_ind); */
 			strcpy((char *)info->type, "U-Boot");
-			info->bootable = is_bootable(pt);
+			info->bootable = get_bootable(pt);
 #if CONFIG_IS_ENABLED(PARTITION_UUIDS)
 			sprintf(info->uuid, "%08x-%02x", disksig, part_num);
 #endif
diff --git a/disk/part_efi.c b/disk/part_efi.c
index b2e157d9c1..83876a7bd9 100644
--- a/disk/part_efi.c
+++ b/disk/part_efi.c
@@ -71,11 +71,15 @@ static char *print_efiname(gpt_entry *pte)

 static const efi_guid_t system_guid = PARTITION_SYSTEM_GUID;

-static inline int is_bootable(gpt_entry *p)
+static int get_bootable(gpt_entry *p)
 {
-	return p->attributes.fields.legacy_bios_bootable ||
-		!memcmp(&(p->partition_type_guid), &system_guid,
-			sizeof(efi_guid_t));
+	int ret = 0;
+
+	if (!memcmp(&p->partition_type_guid, &system_guid, sizeof(efi_guid_t)))
+		ret |=  PART_EFI_SYSTEM_PARTITION;
+	if (p->attributes.fields.legacy_bios_bootable)
+		ret |=  PART_BOOTABLE;
+	return ret;
 }

 static int validate_gpt_header(gpt_header *gpt_h, lbaint_t lba,
@@ -286,7 +290,7 @@ int part_get_info_efi(struct blk_desc *dev_desc, int part,
 	snprintf((char *)info->name, sizeof(info->name), "%s",
 		 print_efiname(&gpt_pte[part - 1]));
 	strcpy((char *)info->type, "U-Boot");
-	info->bootable = is_bootable(&gpt_pte[part - 1]);
+	info->bootable = get_bootable(&gpt_pte[part - 1]);
 #if CONFIG_IS_ENABLED(PARTITION_UUIDS)
 	uuid_bin_to_str(gpt_pte[part - 1].unique_partition_guid.b, info->uuid,
 			UUID_STR_FORMAT_GUID);
@@ -501,7 +505,7 @@ int gpt_fill_pte(struct blk_desc *dev_desc,
 		memset(&gpt_e[i].attributes, 0,
 		       sizeof(gpt_entry_attributes));

-		if (partitions[i].bootable)
+		if (partitions[i].bootable & PART_BOOTABLE)
 			gpt_e[i].attributes.fields.legacy_bios_bootable = 1;

 		/* partition name */
diff --git a/include/part.h b/include/part.h
index 0b5cf3d5e8..3693527397 100644
--- a/include/part.h
+++ b/include/part.h
@@ -51,13 +51,22 @@ struct block_drvr {
 #define PART_TYPE_LEN 32
 #define MAX_SEARCH_PARTITIONS 64

+#define PART_BOOTABLE			((int)BIT(0))
+#define PART_EFI_SYSTEM_PARTITION	((int)BIT(1))
+
 typedef struct disk_partition {
 	lbaint_t	start;	/* # of first block in partition	*/
 	lbaint_t	size;	/* number of blocks in partition	*/
 	ulong	blksz;		/* block size in bytes			*/
 	uchar	name[PART_NAME_LEN];	/* partition name			*/
 	uchar	type[PART_TYPE_LEN];	/* string type description		*/
-	int	bootable;	/* Active/Bootable flag is set		*/
+	/*
+	 * The bootable is a bitmask with the following fields:
+	 *
+	 * PART_BOOTABLE		the MBR bootable flag is set
+	 * PART_EFI_SYSTEM_PARTITION	the partition is an EFI system partition
+	 */
+	int	bootable;
 #if CONFIG_IS_ENABLED(PARTITION_UUIDS)
 	char	uuid[UUID_STR_LEN + 1];	/* filesystem UUID as string, if exists	*/
 #endif
--
2.26.1

[PATCH v3 2/2] efi_loader: identify EFI system partition

[Heinrich Schuchardt]

In subsequent patches UEFI variables shalled be stored on the EFI system
partition. Hence we need to identify the EFI system partition.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
v3:
	adjust commit message
v2:
	no change
---
 include/efi_loader.h      |  7 +++++++
 lib/efi_loader/efi_disk.c | 20 ++++++++++++++++++++
 2 files changed, 27 insertions(+)

diff --git a/include/efi_loader.h b/include/efi_loader.h
index 0ba9a1f702..b7bccf50b3 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -47,6 +47,13 @@ static inline void *guidcpy(void *dst, const void *src)
 /* Root node */
 extern efi_handle_t efi_root;

+/* EFI system partition */
+extern struct efi_system_partition {
+	enum if_type if_type;
+	int devnum;
+	u8 part;
+} efi_system_partition;
+
 int __efi_entry_check(void);
 int __efi_exit_check(void);
 const char *__efi_nesting(void);
diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
index fd8fe17567..fd3df80b0b 100644
--- a/lib/efi_loader/efi_disk.c
+++ b/lib/efi_loader/efi_disk.c
@@ -13,6 +13,8 @@
 #include <part.h>
 #include <malloc.h>

+struct efi_system_partition efi_system_partition;
+
 const efi_guid_t efi_block_io_guid = EFI_BLOCK_IO_PROTOCOL_GUID;

 /**
@@ -418,6 +420,24 @@ static efi_status_t efi_disk_add_dev(
 	diskobj->ops.media = &diskobj->media;
 	if (disk)
 		*disk = diskobj;
+
+	/* Store first EFI system partition */
+	if (part && !efi_system_partition.if_type) {
+		int r;
+		disk_partition_t info;
+
+		r = part_get_info(desc, part, &info);
+		if (r)
+			return EFI_DEVICE_ERROR;
+		if (info.bootable & PART_EFI_SYSTEM_PARTITION) {
+			efi_system_partition.if_type = desc->if_type;
+			efi_system_partition.devnum = desc->devnum;
+			efi_system_partition.part = part;
+			EFI_PRINT("EFI system partition: %s %d:%d\n",
+				  blk_get_if_type_name(desc->if_type),
+				  desc->devnum, part);
+		}
+	}
 	return EFI_SUCCESS;
 }

--
2.26.1

[PATCH v3 2/2] efi_loader: identify EFI system partition

[AKASHI Takahiro]

Heinrich,

On Wed, Apr 22, 2020 at 07:51:33PM +0200, Heinrich Schuchardt wrote:
> In subsequent patches UEFI variables shalled be stored on the EFI system
> partition. Hence we need to identify the EFI system partition.
> 
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
> v3:
> 	adjust commit message


I said 'no' along with my counter proposals[1], and you haven't
replied yet.

[1] https://lists.denx.de/pipermail/u-boot/2020-April/406678.html

-Takahiro Akashi

> v2:
> 	no change
> ---
>  include/efi_loader.h      |  7 +++++++
>  lib/efi_loader/efi_disk.c | 20 ++++++++++++++++++++
>  2 files changed, 27 insertions(+)
> 
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 0ba9a1f702..b7bccf50b3 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -47,6 +47,13 @@ static inline void *guidcpy(void *dst, const void *src)
>  /* Root node */
>  extern efi_handle_t efi_root;
> 
> +/* EFI system partition */
> +extern struct efi_system_partition {
> +	enum if_type if_type;
> +	int devnum;
> +	u8 part;
> +} efi_system_partition;
> +
>  int __efi_entry_check(void);
>  int __efi_exit_check(void);
>  const char *__efi_nesting(void);
> diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
> index fd8fe17567..fd3df80b0b 100644
> --- a/lib/efi_loader/efi_disk.c
> +++ b/lib/efi_loader/efi_disk.c
> @@ -13,6 +13,8 @@
>  #include <part.h>
>  #include <malloc.h>
> 
> +struct efi_system_partition efi_system_partition;
> +
>  const efi_guid_t efi_block_io_guid = EFI_BLOCK_IO_PROTOCOL_GUID;
> 
>  /**
> @@ -418,6 +420,24 @@ static efi_status_t efi_disk_add_dev(
>  	diskobj->ops.media = &diskobj->media;
>  	if (disk)
>  		*disk = diskobj;
> +
> +	/* Store first EFI system partition */
> +	if (part && !efi_system_partition.if_type) {
> +		int r;
> +		disk_partition_t info;
> +
> +		r = part_get_info(desc, part, &info);
> +		if (r)
> +			return EFI_DEVICE_ERROR;
> +		if (info.bootable & PART_EFI_SYSTEM_PARTITION) {
> +			efi_system_partition.if_type = desc->if_type;
> +			efi_system_partition.devnum = desc->devnum;
> +			efi_system_partition.part = part;
> +			EFI_PRINT("EFI system partition: %s %d:%d\n",
> +				  blk_get_if_type_name(desc->if_type),
> +				  desc->devnum, part);
> +		}
> +	}
>  	return EFI_SUCCESS;
>  }
> 
> --
> 2.26.1
> 

[PATCH v3 2/2] efi_loader: identify EFI system partition

[Heinrich Schuchardt]

On 4/23/20 2:19 AM, AKASHI Takahiro wrote:
> Heinrich,
>
> On Wed, Apr 22, 2020 at 07:51:33PM +0200, Heinrich Schuchardt wrote:
>> In subsequent patches UEFI variables shalled be stored on the EFI system
>> partition. Hence we need to identify the EFI system partition.
>>
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> ---
>> v3:
>> 	adjust commit message
>
>
> I said 'no' along with my counter proposals[1], and you haven't
> replied yet.
>
> [1] https://lists.denx.de/pipermail/u-boot/2020-April/406678.html

For some scenarios it may make sense to be able to specify a boot device
containing variables via a Kconfig option. This patch does not stop us
for implementing this.

If you take a Linux distribution that can either be installed on eMMC or
on an SD-card of the same board, autodetection of the EFI system
partition may be a more a better choice than providing multiple U-Boot
builds.

Both approaches have there merits and both could be enabled via Kconfig.

Best regards

Heinrich

>
> -Takahiro Akashi
>
>> v2:
>> 	no change
>> ---
>>  include/efi_loader.h      |  7 +++++++
>>  lib/efi_loader/efi_disk.c | 20 ++++++++++++++++++++
>>  2 files changed, 27 insertions(+)
>>
>> diff --git a/include/efi_loader.h b/include/efi_loader.h
>> index 0ba9a1f702..b7bccf50b3 100644
>> --- a/include/efi_loader.h
>> +++ b/include/efi_loader.h
>> @@ -47,6 +47,13 @@ static inline void *guidcpy(void *dst, const void *src)
>>  /* Root node */
>>  extern efi_handle_t efi_root;
>>
>> +/* EFI system partition */
>> +extern struct efi_system_partition {
>> +	enum if_type if_type;
>> +	int devnum;
>> +	u8 part;
>> +} efi_system_partition;
>> +
>>  int __efi_entry_check(void);
>>  int __efi_exit_check(void);
>>  const char *__efi_nesting(void);
>> diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
>> index fd8fe17567..fd3df80b0b 100644
>> --- a/lib/efi_loader/efi_disk.c
>> +++ b/lib/efi_loader/efi_disk.c
>> @@ -13,6 +13,8 @@
>>  #include <part.h>
>>  #include <malloc.h>
>>
>> +struct efi_system_partition efi_system_partition;
>> +
>>  const efi_guid_t efi_block_io_guid = EFI_BLOCK_IO_PROTOCOL_GUID;
>>
>>  /**
>> @@ -418,6 +420,24 @@ static efi_status_t efi_disk_add_dev(
>>  	diskobj->ops.media = &diskobj->media;
>>  	if (disk)
>>  		*disk = diskobj;
>> +
>> +	/* Store first EFI system partition */
>> +	if (part && !efi_system_partition.if_type) {
>> +		int r;
>> +		disk_partition_t info;
>> +
>> +		r = part_get_info(desc, part, &info);
>> +		if (r)
>> +			return EFI_DEVICE_ERROR;
>> +		if (info.bootable & PART_EFI_SYSTEM_PARTITION) {
>> +			efi_system_partition.if_type = desc->if_type;
>> +			efi_system_partition.devnum = desc->devnum;
>> +			efi_system_partition.part = part;
>> +			EFI_PRINT("EFI system partition: %s %d:%d\n",
>> +				  blk_get_if_type_name(desc->if_type),
>> +				  desc->devnum, part);
>> +		}
>> +	}
>>  	return EFI_SUCCESS;
>>  }
>>
>> --
>> 2.26.1
>>

[PATCH v3 2/2] efi_loader: identify EFI system partition

[Patrick Wildt]

On Wed, Apr 22, 2020 at 07:51:33PM +0200, Heinrich Schuchardt wrote:
> In subsequent patches UEFI variables shalled be stored on the EFI system
> partition. Hence we need to identify the EFI system partition.

Hi,

I'm sorry, but, I'm wondering if this is a good idea?  The EFI system
partition is just some FAT-Partition, and if the system is using secure
boot and someone happens to manage to mount that partition, then the
variables can be changed pretty easily.

Also I guess changing variables using the Runtime Services would then
try to access the partition?  What if the OS is accessing the partition
as well at the same time?

I'm currently storing the U-Boot environment, including the UEFI Secure
Boot environment, on a eMMC partition with a temporary write protect.
This means I cannot change the variables with Runtime Services after
leaving U-Boot, but it also means that an exploit on my OS doesn't
allow the attacker to change the variables, because they are write-
protected until the machine reboots and enters U-Boot again.

I hope we will keep the possibility to store the UEFI variables in
the U-Boot environment, or in some raw sector on the MMC partition,
since otherwise the safety of those variables could be in danger.

Best regards,
Patrick