diff mbox series

[U-Boot,v2,03/16] efi: Add error checking for efi_init_obj_list()

Message ID 20171204212832.130100-4-sjg@chromium.org
State Superseded
Delegated to: Alexander Graf
Headers show
Series efi: Enable basic sandbox support for EFI loader | expand

Commit Message

Simon Glass Dec. 4, 2017, 9:28 p.m. UTC 
This function calls a function which can fail. Print a message in this
case and abort the boot, rather than silently continuing to boot, which
will certainly fail.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

Changes in v2:
- Change return type of efi_init_obj_list() to efi_status_t

 cmd/bootefi.c | 32 +++++++++++++++++++++++++-------
 1 file changed, 25 insertions(+), 7 deletions(-)

Comments

Heinrich Schuchardt Dec. 4, 2017, 10:21 p.m. UTC | #1 
On 12/04/2017 10:28 PM, Simon Glass wrote:
> This function calls a function which can fail. Print a message in this
> case and abort the boot, rather than silently continuing to boot, which
> will certainly fail.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
> Changes in v2:
> - Change return type of efi_init_obj_list() to efi_status_t
> 
>   cmd/bootefi.c | 32 +++++++++++++++++++++++++-------
>   1 file changed, 25 insertions(+), 7 deletions(-)
> 
> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> index 17b26e6f4e..a2138f6075 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -25,11 +25,17 @@ static uint8_t efi_obj_list_initalized;
>   static struct efi_device_path *bootefi_image_path;
>   static struct efi_device_path *bootefi_device_path;
>   
> -/* Initialize and populate EFI object list */
> -static void efi_init_obj_list(void)
> +/**
> + * efi_init_obj_list() - Initialize and populate EFI object list
> + *
> + * @return 0 if OK, -ve on error (in which case it prints a message)
> + */
> +static efi_status_t efi_init_obj_list(void)
>   {
> +	efi_status_t ret;
> +
>   	if (efi_obj_list_initalized)
> -		return;
> +		return 0;
>   	efi_obj_list_initalized = 1;
>   
>   	efi_console_register();
> @@ -43,12 +49,19 @@ static void efi_init_obj_list(void)
>   	efi_net_register();

This function can also fail. Same is true for efi_gop_register.

>   #endif
>   #ifdef CONFIG_GENERATE_SMBIOS_TABLE
> -	efi_smbios_register();
> +	ret = efi_smbios_register();
> +	if (ret)

if (ret != EFI_SUCCESS)

> +		goto error;
>   #endif
>   
>   	/* Initialize EFI runtime services */
>   	efi_reset_system_init();
>   	efi_get_time_init();
> +
> +	return EFI_SUCCESS;
> +error:
> +	printf("Error: Cannot set up EFI object list (err=%d)\n", ret);

Now we have some objects initialized and others not.
Before returning we should clean up.
We should free all allocated objects.

Best regards

Heinrich

> +	return ret;
>   }
>   
>   static void *copy_fdt(void *fdt)
> @@ -137,6 +150,7 @@ static unsigned long do_bootefi_exec(void *efi, void *fdt,
>   	ulong fdt_pages, fdt_size, fdt_start, fdt_end;
>   	const efi_guid_t fdt_guid = EFI_FDT_GUID;
>   	bootm_headers_t img = { 0 };
> +	int ret;
>   
>   	/*
>   	 * Special case for efi payload not loaded from disk, such as
> @@ -211,7 +225,9 @@ static unsigned long do_bootefi_exec(void *efi, void *fdt,
>   		"{ro,boot}(blob)0000000000000000");
>   
>   	/* Initialize and populate EFI object list */
> -	efi_init_obj_list();
> +	ret = efi_init_obj_list();
> +	if (ret)
> +		return ret;
>   
>   	/* Call our payload! */
>   	debug("%s:%d Jumping to 0x%lx\n", __func__, __LINE__, (long)entry);
> @@ -313,10 +329,12 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>   		 */
>   		efi_save_gd();
>   		/* Initialize and populate EFI object list */
> -		if (!efi_obj_list_initalized)
> -			efi_init_obj_list();
> +		if (!efi_obj_list_initalized && efi_init_obj_list())
> +			return CMD_RET_FAILURE;
> +
>   		loaded_image_info.device_handle = bootefi_device_path;
>   		loaded_image_info.file_path = bootefi_image_path;
> +
>   		return efi_selftest(&loaded_image_info, &systab);
>   	} else
>   #endif
>
Simon Glass Dec. 5, 2017, 9:12 p.m. UTC | #2 
Hi Heinrich,

On 4 December 2017 at 15:21, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
>
> On 12/04/2017 10:28 PM, Simon Glass wrote:
>>
>> This function calls a function which can fail. Print a message in this
>> case and abort the boot, rather than silently continuing to boot, which
>> will certainly fail.
>>
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>> ---
>>
>> Changes in v2:
>> - Change return type of efi_init_obj_list() to efi_status_t
>>
>>   cmd/bootefi.c | 32 +++++++++++++++++++++++++-------
>>   1 file changed, 25 insertions(+), 7 deletions(-)
>>
>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
>> index 17b26e6f4e..a2138f6075 100644
>> --- a/cmd/bootefi.c
>> +++ b/cmd/bootefi.c
>> @@ -25,11 +25,17 @@ static uint8_t efi_obj_list_initalized;
>>   static struct efi_device_path *bootefi_image_path;
>>   static struct efi_device_path *bootefi_device_path;
>>   -/* Initialize and populate EFI object list */
>> -static void efi_init_obj_list(void)
>> +/**
>> + * efi_init_obj_list() - Initialize and populate EFI object list
>> + *
>> + * @return 0 if OK, -ve on error (in which case it prints a message)
>> + */
>> +static efi_status_t efi_init_obj_list(void)
>>   {
>> +       efi_status_t ret;
>> +
>>         if (efi_obj_list_initalized)
>> -               return;
>> +               return 0;
>>         efi_obj_list_initalized = 1;
>>         efi_console_register();
>> @@ -43,12 +49,19 @@ static void efi_init_obj_list(void)
>>         efi_net_register();
>
>
> This function can also fail. Same is true for efi_gop_register.
>
>>   #endif
>>   #ifdef CONFIG_GENERATE_SMBIOS_TABLE
>> -       efi_smbios_register();
>> +       ret = efi_smbios_register();
>> +       if (ret)
>
>
> if (ret != EFI_SUCCESS)
>

Please can we avoid this obfuscation? It really pains me to see what I
consider to be stupidity in the code. Can we just agree that success
is 0 in U-Boot/?

>> +               goto error;
>>   #endif
>>         /* Initialize EFI runtime services */
>>         efi_reset_system_init();
>>         efi_get_time_init();
>> +
>> +       return EFI_SUCCESS;
>> +error:
>> +       printf("Error: Cannot set up EFI object list (err=%d)\n", ret);
>
>
> Now we have some objects initialized and others not.
> Before returning we should clean up.
> We should free all allocated objects.

OK let me take another look at this.

>
> Best regards
>
> Heinrich
>
>
>> +       return ret;
>>   }
>>     static void *copy_fdt(void *fdt)
>> @@ -137,6 +150,7 @@ static unsigned long do_bootefi_exec(void *efi, void
>> *fdt,
>>         ulong fdt_pages, fdt_size, fdt_start, fdt_end;
>>         const efi_guid_t fdt_guid = EFI_FDT_GUID;
>>         bootm_headers_t img = { 0 };
>> +       int ret;
>>         /*
>>          * Special case for efi payload not loaded from disk, such as
>> @@ -211,7 +225,9 @@ static unsigned long do_bootefi_exec(void *efi, void
>> *fdt,
>>                 "{ro,boot}(blob)0000000000000000");
>>         /* Initialize and populate EFI object list */
>> -       efi_init_obj_list();
>> +       ret = efi_init_obj_list();
>> +       if (ret)
>> +               return ret;
>>         /* Call our payload! */
>>         debug("%s:%d Jumping to 0x%lx\n", __func__, __LINE__,
>> (long)entry);
>> @@ -313,10 +329,12 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag,
>> int argc, char * const argv[])
>>                  */
>>                 efi_save_gd();
>>                 /* Initialize and populate EFI object list */
>> -               if (!efi_obj_list_initalized)
>> -                       efi_init_obj_list();
>> +               if (!efi_obj_list_initalized && efi_init_obj_list())
>> +                       return CMD_RET_FAILURE;
>> +
>>                 loaded_image_info.device_handle = bootefi_device_path;
>>                 loaded_image_info.file_path = bootefi_image_path;
>> +
>>                 return efi_selftest(&loaded_image_info, &systab);
>>         } else
>>   #endif
>>
>


Regards,
Simon
Heinrich Schuchardt Jan. 9, 2018, 3:20 p.m. UTC | #3 
On 12/04/2017 10:28 PM, Simon Glass wrote:
> This function calls a function which can fail. Print a message in this
> case and abort the boot, rather than silently continuing to boot, which
> will certainly fail.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
> Changes in v2:
> - Change return type of efi_init_obj_list() to efi_status_t
> 
>  cmd/bootefi.c | 32 +++++++++++++++++++++++++-------
>  1 file changed, 25 insertions(+), 7 deletions(-)
> 
> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> index 17b26e6f4e..a2138f6075 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -25,11 +25,17 @@ static uint8_t efi_obj_list_initalized;
>  static struct efi_device_path *bootefi_image_path;
>  static struct efi_device_path *bootefi_device_path;
>  
> -/* Initialize and populate EFI object list */
> -static void efi_init_obj_list(void)
> +/**
> + * efi_init_obj_list() - Initialize and populate EFI object list
> + *
> + * @return 0 if OK, -ve on error (in which case it prints a message)
> + */
> +static efi_status_t efi_init_obj_list(void)
>  {
> +	efi_status_t ret;
> +
>  	if (efi_obj_list_initalized)
> -		return;
> +		return 0;
>  	efi_obj_list_initalized = 1;
>  
>  	efi_console_register();
> @@ -43,12 +49,19 @@ static void efi_init_obj_list(void)
>  	efi_net_register();
>  #endif
>  #ifdef CONFIG_GENERATE_SMBIOS_TABLE
> -	efi_smbios_register();
> +	ret = efi_smbios_register();
> +	if (ret)
> +		goto error;
>  #endif
>  
>  	/* Initialize EFI runtime services */
>  	efi_reset_system_init();
>  	efi_get_time_init();
> +
> +	return EFI_SUCCESS;
> +error:
> +	printf("Error: Cannot set up EFI object list (err=%d)\n", ret);

warning: format ‘%d’ expects argument of type ‘int’, but argument 2 has
type ‘efi_status_t {aka long unsigned int}’ [-Wformat=]

Please, use %lu.

Regards

Heinrich

> +	return ret;
>  }
>  
>  static void *copy_fdt(void *fdt)
> @@ -137,6 +150,7 @@ static unsigned long do_bootefi_exec(void *efi, void *fdt,
>  	ulong fdt_pages, fdt_size, fdt_start, fdt_end;
>  	const efi_guid_t fdt_guid = EFI_FDT_GUID;
>  	bootm_headers_t img = { 0 };
> +	int ret;
>  
>  	/*
>  	 * Special case for efi payload not loaded from disk, such as
> @@ -211,7 +225,9 @@ static unsigned long do_bootefi_exec(void *efi, void *fdt,
>  		"{ro,boot}(blob)0000000000000000");
>  
>  	/* Initialize and populate EFI object list */
> -	efi_init_obj_list();
> +	ret = efi_init_obj_list();
> +	if (ret)
> +		return ret;
>  
>  	/* Call our payload! */
>  	debug("%s:%d Jumping to 0x%lx\n", __func__, __LINE__, (long)entry);
> @@ -313,10 +329,12 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>  		 */
>  		efi_save_gd();
>  		/* Initialize and populate EFI object list */
> -		if (!efi_obj_list_initalized)
> -			efi_init_obj_list();
> +		if (!efi_obj_list_initalized && efi_init_obj_list())
> +			return CMD_RET_FAILURE;
> +
>  		loaded_image_info.device_handle = bootefi_device_path;
>  		loaded_image_info.file_path = bootefi_image_path;
> +
>  		return efi_selftest(&loaded_image_info, &systab);
>  	} else
>  #endif
>
diff mbox series

Patch

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index 17b26e6f4e..a2138f6075 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -25,11 +25,17 @@  static uint8_t efi_obj_list_initalized;
 static struct efi_device_path *bootefi_image_path;
 static struct efi_device_path *bootefi_device_path;
 
-/* Initialize and populate EFI object list */
-static void efi_init_obj_list(void)
+/**
+ * efi_init_obj_list() - Initialize and populate EFI object list
+ *
+ * @return 0 if OK, -ve on error (in which case it prints a message)
+ */
+static efi_status_t efi_init_obj_list(void)
 {
+	efi_status_t ret;
+
 	if (efi_obj_list_initalized)
-		return;
+		return 0;
 	efi_obj_list_initalized = 1;
 
 	efi_console_register();
@@ -43,12 +49,19 @@  static void efi_init_obj_list(void)
 	efi_net_register();
 #endif
 #ifdef CONFIG_GENERATE_SMBIOS_TABLE
-	efi_smbios_register();
+	ret = efi_smbios_register();
+	if (ret)
+		goto error;
 #endif
 
 	/* Initialize EFI runtime services */
 	efi_reset_system_init();
 	efi_get_time_init();
+
+	return EFI_SUCCESS;
+error:
+	printf("Error: Cannot set up EFI object list (err=%d)\n", ret);
+	return ret;
 }
 
 static void *copy_fdt(void *fdt)
@@ -137,6 +150,7 @@  static unsigned long do_bootefi_exec(void *efi, void *fdt,
 	ulong fdt_pages, fdt_size, fdt_start, fdt_end;
 	const efi_guid_t fdt_guid = EFI_FDT_GUID;
 	bootm_headers_t img = { 0 };
+	int ret;
 
 	/*
 	 * Special case for efi payload not loaded from disk, such as
@@ -211,7 +225,9 @@  static unsigned long do_bootefi_exec(void *efi, void *fdt,
 		"{ro,boot}(blob)0000000000000000");
 
 	/* Initialize and populate EFI object list */
-	efi_init_obj_list();
+	ret = efi_init_obj_list();
+	if (ret)
+		return ret;
 
 	/* Call our payload! */
 	debug("%s:%d Jumping to 0x%lx\n", __func__, __LINE__, (long)entry);
@@ -313,10 +329,12 @@  static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 		 */
 		efi_save_gd();
 		/* Initialize and populate EFI object list */
-		if (!efi_obj_list_initalized)
-			efi_init_obj_list();
+		if (!efi_obj_list_initalized && efi_init_obj_list())
+			return CMD_RET_FAILURE;
+
 		loaded_image_info.device_handle = bootefi_device_path;
 		loaded_image_info.file_path = bootefi_image_path;
+
 		return efi_selftest(&loaded_image_info, &systab);
 	} else
 #endif