[PATCH 0/4] efi_loader: fix EFI_LOAD_FILE2_PROTOCOL

[PATCH 0/4] efi_loader: fix EFI_LOAD_FILE2_PROTOCOL

[Heinrich Schuchardt]

The EFI_LOAD_FILE2_PROTOCOL is used to provide an initial RAM disk to the
Linux EFI stub.

The following problems in the implementation are addressed:

* illegal free()
* insufficient function descriptions
* CRC32 in unit test printed as decimal instead of hexadecimal

Heinrich Schuchardt (4):
  efi_loader: description EFI_LOAD_FILE2_PROTOCOL
  efi_loader: illegal free in EFI_LOAD_FILE2_PROTOCOL
  efi_selftest: enable printing hexadecimal numbers
  efi_selftest: print CRC32 of initrd as hexadecimal

 lib/efi_loader/efi_load_initrd.c            | 59 +++++++++++----------
 lib/efi_selftest/efi_selftest_console.c     | 35 +++++++-----
 lib/efi_selftest/efi_selftest_load_initrd.c |  2 +-
 3 files changed, 55 insertions(+), 41 deletions(-)

--
2.28.0

[PATCH 1/4] efi_loader: description EFI_LOAD_FILE2_PROTOCOL

[Heinrich Schuchardt]

U-Boot offers a EFI_LOAD_FILE2_PROTOCOL which the Linux EFI stub can use to
load an initial RAM disk. Update the function comments of the
implementation.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 lib/efi_loader/efi_load_initrd.c | 42 +++++++++++++++++---------------
 1 file changed, 23 insertions(+), 19 deletions(-)

diff --git a/lib/efi_loader/efi_load_initrd.c b/lib/efi_loader/efi_load_initrd.c
index 574a83d7e3..ff69e6eb79 100644
--- a/lib/efi_loader/efi_load_initrd.c
+++ b/lib/efi_loader/efi_load_initrd.c
@@ -47,9 +47,9 @@ static const struct efi_initrd_dp dp = {
 /**
  * get_file_size() - retrieve the size of initramfs, set efi status on error
  *
- * @dev:			device to read from. i.e "mmc"
- * @part:			device partition. i.e "0:1"
- * @file:			name fo file
+ * @dev:			device to read from, e.g. "mmc"
+ * @part:			device partition, e.g. "0:1"
+ * @file:			name of file
  * @status:			EFI exit code in case of failure
  *
  * Return:			size of file
@@ -78,15 +78,16 @@ out:
 }

 /**
- * load_file2() - get information about random number generation
+ * efi_load_file2initrd() - load initial RAM disk
+ *
+ * This function implements the LoadFile service of the EFI_LOAD_FILE2_PROTOCOL
+ * in order to load an initial RAM disk requested by the Linux kernel stub.
  *
- * This function implement the LoadFile2() service in order to load an initram
- * disk requested by the Linux kernel stub.
  * See the UEFI spec for details.
  *
- * @this:			loadfile2 protocol instance
- * @file_path:			relative path of the file. "" in this case
- * @boot_policy:		must be false for Loadfile2
+ * @this:			EFI_LOAD_FILE2_PROTOCOL instance
+ * @file_path:			media device path of the file, "" in this case
+ * @boot_policy:		must be false
  * @buffer_size:		size of allocated buffer
  * @buffer:			buffer to load the file
  *
@@ -128,7 +129,13 @@ efi_load_file2_initrd(struct efi_load_file_protocol *this,
 		goto out;
 	}

-	/* expect something like 'mmc 0:1 initrd.cpio.gz' */
+	/*
+	 * expect a string with three space separated parts:
+	 *
+	 * * a block device type, e.g. "mmc"
+	 * * a device and partition identifier, e.g. "0:1"
+	 * * a file path on the block device, e.g. "/boot/initrd.cpio.gz"
+	 */
 	dev = strsep(&s, " ");
 	if (!dev)
 		goto out;
@@ -168,23 +175,20 @@ out:
 }

 /**
- * efi_initrd_register() - Register a handle and loadfile2 protocol
+ * efi_initrd_register() - create handle for loading initial RAM disk
  *
- * This function creates a new handle and installs a linux specific GUID
- * to handle initram disk loading during boot.
- * See the UEFI spec for details.
+ * This function creates a new handle and installs a Linux specific vendor
+ * device path and an EFI_LOAD_FILE_2_PROTOCOL. Linux uses the device path
+ * to identify the handle and then calls the LoadFile service of the
+ * EFI_LOAD_FILE_2_PROTOCOL to read the initial RAM disk.
  *
- * Return:			status code
+ * Return:	status code
  */
 efi_status_t efi_initrd_register(void)
 {
 	efi_handle_t efi_initrd_handle = NULL;
 	efi_status_t ret;

-	/*
-	 * Set up the handle with the EFI_LOAD_FILE2_PROTOCOL which Linux may
-	 * use to load the initial ramdisk.
-	 */
 	ret = EFI_CALL(efi_install_multiple_protocol_interfaces
 		       (&efi_initrd_handle,
 			/* initramfs */
--
2.28.0

[PATCH 1/4] efi_loader: description EFI_LOAD_FILE2_PROTOCOL

[Ilias Apalodimas]

On Sat, Oct 03, 2020 at 01:57:13PM +0200, Heinrich Schuchardt wrote:
> U-Boot offers a EFI_LOAD_FILE2_PROTOCOL which the Linux EFI stub can use to
> load an initial RAM disk. Update the function comments of the
> implementation.
> 
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  lib/efi_loader/efi_load_initrd.c | 42 +++++++++++++++++---------------
>  1 file changed, 23 insertions(+), 19 deletions(-)
> 
> diff --git a/lib/efi_loader/efi_load_initrd.c b/lib/efi_loader/efi_load_initrd.c
> index 574a83d7e3..ff69e6eb79 100644
> --- a/lib/efi_loader/efi_load_initrd.c
> +++ b/lib/efi_loader/efi_load_initrd.c
> @@ -47,9 +47,9 @@ static const struct efi_initrd_dp dp = {
>  /**
>   * get_file_size() - retrieve the size of initramfs, set efi status on error
>   *
> - * @dev:			device to read from. i.e "mmc"
> - * @part:			device partition. i.e "0:1"
> - * @file:			name fo file
> + * @dev:			device to read from, e.g. "mmc"
> + * @part:			device partition, e.g. "0:1"
> + * @file:			name of file
>   * @status:			EFI exit code in case of failure
>   *
>   * Return:			size of file
> @@ -78,15 +78,16 @@ out:
>  }
> 
>  /**
> - * load_file2() - get information about random number generation
> + * efi_load_file2initrd() - load initial RAM disk
> + *
> + * This function implements the LoadFile service of the EFI_LOAD_FILE2_PROTOCOL
> + * in order to load an initial RAM disk requested by the Linux kernel stub.
>   *
> - * This function implement the LoadFile2() service in order to load an initram
> - * disk requested by the Linux kernel stub.
>   * See the UEFI spec for details.
>   *
> - * @this:			loadfile2 protocol instance
> - * @file_path:			relative path of the file. "" in this case
> - * @boot_policy:		must be false for Loadfile2
> + * @this:			EFI_LOAD_FILE2_PROTOCOL instance
> + * @file_path:			media device path of the file, "" in this case
> + * @boot_policy:		must be false
>   * @buffer_size:		size of allocated buffer
>   * @buffer:			buffer to load the file
>   *
> @@ -128,7 +129,13 @@ efi_load_file2_initrd(struct efi_load_file_protocol *this,
>  		goto out;
>  	}
> 
> -	/* expect something like 'mmc 0:1 initrd.cpio.gz' */
> +	/*
> +	 * expect a string with three space separated parts:
> +	 *
> +	 * * a block device type, e.g. "mmc"
> +	 * * a device and partition identifier, e.g. "0:1"
> +	 * * a file path on the block device, e.g. "/boot/initrd.cpio.gz"
> +	 */
>  	dev = strsep(&s, " ");
>  	if (!dev)
>  		goto out;
> @@ -168,23 +175,20 @@ out:
>  }
> 
>  /**
> - * efi_initrd_register() - Register a handle and loadfile2 protocol
> + * efi_initrd_register() - create handle for loading initial RAM disk
>   *
> - * This function creates a new handle and installs a linux specific GUID
> - * to handle initram disk loading during boot.
> - * See the UEFI spec for details.
> + * This function creates a new handle and installs a Linux specific vendor
> + * device path and an EFI_LOAD_FILE_2_PROTOCOL. Linux uses the device path
> + * to identify the handle and then calls the LoadFile service of the
> + * EFI_LOAD_FILE_2_PROTOCOL to read the initial RAM disk.
>   *
> - * Return:			status code
> + * Return:	status code
>   */
>  efi_status_t efi_initrd_register(void)
>  {
>  	efi_handle_t efi_initrd_handle = NULL;
>  	efi_status_t ret;
> 
> -	/*
> -	 * Set up the handle with the EFI_LOAD_FILE2_PROTOCOL which Linux may
> -	 * use to load the initial ramdisk.
> -	 */
>  	ret = EFI_CALL(efi_install_multiple_protocol_interfaces
>  		       (&efi_initrd_handle,
>  			/* initramfs */
> --
> 2.28.0
> 

Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

[PATCH 2/4] efi_loader: illegal free in EFI_LOAD_FILE2_PROTOCOL

[Heinrich Schuchardt]

strsep() changes the address that its first argument points to.
We cannot use the changed address as argument of free().

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 lib/efi_loader/efi_load_initrd.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/lib/efi_loader/efi_load_initrd.c b/lib/efi_loader/efi_load_initrd.c
index ff69e6eb79..d517d686c3 100644
--- a/lib/efi_loader/efi_load_initrd.c
+++ b/lib/efi_loader/efi_load_initrd.c
@@ -98,19 +98,20 @@ efi_load_file2_initrd(struct efi_load_file_protocol *this,
 		      struct efi_device_path *file_path, bool boot_policy,
 		      efi_uintn_t *buffer_size, void *buffer)
 {
-	const char *filespec = CONFIG_EFI_INITRD_FILESPEC;
+	char *filespec;
 	efi_status_t status = EFI_NOT_FOUND;
 	loff_t file_sz = 0, read_sz = 0;
 	char *dev, *part, *file;
-	char *s;
+	char *pos;
 	int ret;

 	EFI_ENTRY("%p, %p, %d, %p, %p", this, file_path, boot_policy,
 		  buffer_size, buffer);

-	s = strdup(filespec);
-	if (!s)
+	filespec = strdup(CONFIG_EFI_INITRD_FILESPEC);
+	if (!filespec)
 		goto out;
+	pos = filespec;

 	if (!this || this != &efi_lf2_protocol ||
 	    !buffer_size) {
@@ -136,13 +137,13 @@ efi_load_file2_initrd(struct efi_load_file_protocol *this,
 	 * * a device and partition identifier, e.g. "0:1"
 	 * * a file path on the block device, e.g. "/boot/initrd.cpio.gz"
 	 */
-	dev = strsep(&s, " ");
+	dev = strsep(&pos, " ");
 	if (!dev)
 		goto out;
-	part = strsep(&s, " ");
+	part = strsep(&pos, " ");
 	if (!part)
 		goto out;
-	file = strsep(&s, " ");
+	file = strsep(&pos, " ");
 	if (!file)
 		goto out;

@@ -170,7 +171,7 @@ efi_load_file2_initrd(struct efi_load_file_protocol *this,
 	}

 out:
-	free(s);
+	free(filespec);
 	return EFI_EXIT(status);
 }

--
2.28.0

[PATCH 2/4] efi_loader: illegal free in EFI_LOAD_FILE2_PROTOCOL

[Ilias Apalodimas]

On Sat, Oct 03, 2020 at 01:57:14PM +0200, Heinrich Schuchardt wrote:
> strsep() changes the address that its first argument points to.
> We cannot use the changed address as argument of free().
> 
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  lib/efi_loader/efi_load_initrd.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/lib/efi_loader/efi_load_initrd.c b/lib/efi_loader/efi_load_initrd.c
> index ff69e6eb79..d517d686c3 100644
> --- a/lib/efi_loader/efi_load_initrd.c
> +++ b/lib/efi_loader/efi_load_initrd.c
> @@ -98,19 +98,20 @@ efi_load_file2_initrd(struct efi_load_file_protocol *this,
>  		      struct efi_device_path *file_path, bool boot_policy,
>  		      efi_uintn_t *buffer_size, void *buffer)
>  {
> -	const char *filespec = CONFIG_EFI_INITRD_FILESPEC;
> +	char *filespec;
>  	efi_status_t status = EFI_NOT_FOUND;
>  	loff_t file_sz = 0, read_sz = 0;
>  	char *dev, *part, *file;
> -	char *s;
> +	char *pos;
>  	int ret;
> 
>  	EFI_ENTRY("%p, %p, %d, %p, %p", this, file_path, boot_policy,
>  		  buffer_size, buffer);
> 
> -	s = strdup(filespec);
> -	if (!s)
> +	filespec = strdup(CONFIG_EFI_INITRD_FILESPEC);
> +	if (!filespec)
>  		goto out;
> +	pos = filespec;
> 
>  	if (!this || this != &efi_lf2_protocol ||
>  	    !buffer_size) {
> @@ -136,13 +137,13 @@ efi_load_file2_initrd(struct efi_load_file_protocol *this,
>  	 * * a device and partition identifier, e.g. "0:1"
>  	 * * a file path on the block device, e.g. "/boot/initrd.cpio.gz"
>  	 */
> -	dev = strsep(&s, " ");
> +	dev = strsep(&pos, " ");
>  	if (!dev)
>  		goto out;
> -	part = strsep(&s, " ");
> +	part = strsep(&pos, " ");
>  	if (!part)
>  		goto out;
> -	file = strsep(&s, " ");
> +	file = strsep(&pos, " ");
>  	if (!file)
>  		goto out;
> 
> @@ -170,7 +171,7 @@ efi_load_file2_initrd(struct efi_load_file_protocol *this,
>  	}
> 
>  out:
> -	free(s);
> +	free(filespec);
>  	return EFI_EXIT(status);
>  }
> 
> --
> 2.28.0
> 

Not changing the variable names would make this an one liner to read.
The changes do make sense though so 

Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

[PATCH 3/4] efi_selftest: enable printing hexadecimal numbers

[Heinrich Schuchardt]

Add code to use %x in efi_st_print().

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 lib/efi_selftest/efi_selftest_console.c | 35 ++++++++++++++++---------
 1 file changed, 22 insertions(+), 13 deletions(-)

diff --git a/lib/efi_selftest/efi_selftest_console.c b/lib/efi_selftest/efi_selftest_console.c
index 13f3ee6bc1..e3f4e39348 100644
--- a/lib/efi_selftest/efi_selftest_console.c
+++ b/lib/efi_selftest/efi_selftest_console.c
@@ -44,25 +44,28 @@ static void mac(void *pointer, u16 **buf)
 }

 /*
- * Print a pointer to an u16 string
+ * printx() - print hexadecimal number to an u16 string
  *
- * @pointer: pointer
- * @buf: pointer to buffer address
- * on return position of terminating zero word
+ * @pointer:	pointer
+ * @prec:	minimum number of digits to print
+ * @buf:	pointer to buffer address,
+ *		on return position of terminating zero word
+ * @size:	size of value to be printed in bytes
  */
-static void pointer(void *pointer, u16 **buf)
+static void printx(u64 p, int prec, u16 **buf)
 {
 	int i;
 	u16 c;
-	uintptr_t p = (uintptr_t)pointer;
 	u16 *pos = *buf;

-	for (i = 8 * sizeof(p) - 4; i >= 0; i -= 4) {
-		c = (p >> i) & 0x0f;
-		c += '0';
-		if (c > '9')
-			c += 'a' - '9' - 1;
-		*pos++ = c;
+	for (i = 2 * sizeof(p) - 1; i >= 0; --i) {
+		c = (p >> (4 * i)) & 0x0f;
+		if (c || pos != *buf || !i || i < prec) {
+			c += '0';
+			if (c > '9')
+				c += 'a' - '9' - 1;
+			*pos++ = c;
+		}
 	}
 	*pos = 0;
 	*buf = pos;
@@ -212,7 +215,9 @@ void efi_st_printc(int color, const char *fmt, ...)
 					break;
 				default:
 					--c;
-					pointer(va_arg(args, void*), &pos);
+					printx((u64)va_arg(args, void *),
+					       2 * sizeof(void *), &pos);
+					break;
 				}
 				break;
 			case 's':
@@ -223,6 +228,10 @@ void efi_st_printc(int color, const char *fmt, ...)
 			case 'u':
 				uint2dec(va_arg(args, u32), prec, &pos);
 				break;
+			case 'x':
+				printx((u64)va_arg(args, unsigned int),
+				       prec, &pos);
+				break;
 			default:
 				break;
 			}
--
2.28.0

[PATCH 4/4] efi_selftest: print CRC32 of initrd as hexadecimal

[Heinrich Schuchardt]

Print the CRC32 loaded via the EFI_LOAD_FILE2_PROTOCOL as a hexadecimal
number.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 lib/efi_selftest/efi_selftest_load_initrd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/efi_selftest/efi_selftest_load_initrd.c b/lib/efi_selftest/efi_selftest_load_initrd.c
index e16163caca..fe060a6644 100644
--- a/lib/efi_selftest/efi_selftest_load_initrd.c
+++ b/lib/efi_selftest/efi_selftest_load_initrd.c
@@ -200,7 +200,7 @@ static int execute(void)
 		efi_st_error("Could not determine CRC32\n");
 		return EFI_ST_FAILURE;
 	}
-	efi_st_printf("CRC32 %u\n", (unsigned int)crc32);
+	efi_st_printf("CRC32 %.8x\n", (unsigned int)crc32);

 	status = boottime->free_pool(buf);
 	if (status != EFI_SUCCESS) {
--
2.28.0

[PATCH 4/4] efi_selftest: print CRC32 of initrd as hexadecimal

[Ilias Apalodimas]

On Sat, Oct 03, 2020 at 01:57:16PM +0200, Heinrich Schuchardt wrote:
> Print the CRC32 loaded via the EFI_LOAD_FILE2_PROTOCOL as a hexadecimal
> number.
> 
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  lib/efi_selftest/efi_selftest_load_initrd.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/efi_selftest/efi_selftest_load_initrd.c b/lib/efi_selftest/efi_selftest_load_initrd.c
> index e16163caca..fe060a6644 100644
> --- a/lib/efi_selftest/efi_selftest_load_initrd.c
> +++ b/lib/efi_selftest/efi_selftest_load_initrd.c
> @@ -200,7 +200,7 @@ static int execute(void)
>  		efi_st_error("Could not determine CRC32\n");
>  		return EFI_ST_FAILURE;
>  	}
> -	efi_st_printf("CRC32 %u\n", (unsigned int)crc32);
> +	efi_st_printf("CRC32 %.8x\n", (unsigned int)crc32);
> 
>  	status = boottime->free_pool(buf);
>  	if (status != EFI_SUCCESS) {
> --
> 2.28.0
> 

Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>