[PATCH 1/1] efi_loader: validate load option

[AKASHI Takahiro]

Heinrich,

On Sun, May 31, 2020 at 10:52:16PM +0200, Heinrich Schuchardt wrote:
> For passing the optional data of the load option to the loaded imaged
> protocol we need its size.
> 
> efi_deserialize_load_option() is changed to return the size of the optional
> data.
> 
> As a by-product we get a partial validation of the load option.
> Checking the length of the device path remains to be implemented.
> 
> Signed-off-by: Heinrich Schuchardt <xypron.glpk at gmx.de>

Please add "Reported-by: Coverity (CID xxx)" for tracing
as you requested me before.
(even if it doesn't fully address the issue.)

Reported-by: Coverity (CID 303760)
Reported-by: Coverity (CID 303768)
Reported-by: Coverity (CID 303776)

-Takahiro Akashi


> ---
>  cmd/efidebug.c               | 21 +++++++++++-----
>  include/efi_loader.h         |  3 ++-
>  lib/efi_loader/efi_bootmgr.c | 48 +++++++++++++++++++++++++++++-------
>  3 files changed, 56 insertions(+), 16 deletions(-)
> 
> diff --git a/cmd/efidebug.c b/cmd/efidebug.c
> index 32430e62f0..58018f700c 100644
> --- a/cmd/efidebug.c
> +++ b/cmd/efidebug.c
> @@ -694,14 +694,19 @@ static int do_efi_boot_rm(struct cmd_tbl *cmdtp, int flag,
>   *
>   * Decode the value of UEFI load option variable and print information.
>   */
> -static void show_efi_boot_opt_data(u16 *varname16, void *data, size_t size)
> +static void show_efi_boot_opt_data(u16 *varname16, void *data, size_t *size)
>  {
>  	struct efi_load_option lo;
>  	char *label, *p;
>  	size_t label_len16, label_len;
>  	u16 *dp_str;
> +	efi_status_t ret;
> 
> -	efi_deserialize_load_option(&lo, data);
> +	ret = efi_deserialize_load_option(&lo, data, size);
> +	if (ret != EFI_SUCCESS) {
> +		printf("%ls: invalid load option\n", varname16);
> +		return;
> +	}
> 
>  	label_len16 = u16_strlen(lo.label);
>  	label_len = utf16_utf8_strnlen(lo.label, label_len16);
> @@ -728,8 +733,7 @@ static void show_efi_boot_opt_data(u16 *varname16, void *data, size_t size)
> 
>  	printf("  data:\n");
>  	print_hex_dump("    ", DUMP_PREFIX_OFFSET, 16, 1,
> -		       lo.optional_data, size + (u8 *)data -
> -		       (u8 *)lo.optional_data, true);
> +		       lo.optional_data, *size, true);
>  	free(label);
>  }
> 
> @@ -759,7 +763,7 @@ static void show_efi_boot_opt(u16 *varname16)
>  						&efi_global_variable_guid,
>  						NULL, &size, data));
>  		if (ret == EFI_SUCCESS)
> -			show_efi_boot_opt_data(varname16, data, size);
> +			show_efi_boot_opt_data(varname16, data, &size);
>  		free(data);
>  	}
>  }
> @@ -920,7 +924,12 @@ static int show_efi_boot_order(void)
>  			goto out;
>  		}
> 
> -		efi_deserialize_load_option(&lo, data);
> +		ret = efi_deserialize_load_option(&lo, data, &size);
> +		if (ret != EFI_SUCCESS) {
> +			printf("%ls: invalid load option\n", var_name16);
> +			ret = CMD_RET_FAILURE;
> +			goto out;
> +		}
> 
>  		label_len16 = u16_strlen(lo.label);
>  		label_len = utf16_utf8_strnlen(lo.label, label_len16);
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 9533df26dc..c2cae814b6 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -708,7 +708,8 @@ struct efi_load_option {
>  	const u8 *optional_data;
>  };
> 
> -void efi_deserialize_load_option(struct efi_load_option *lo, u8 *data);
> +efi_status_t efi_deserialize_load_option(struct efi_load_option *lo, u8 *data,
> +					 efi_uintn_t *size);
>  unsigned long efi_serialize_load_option(struct efi_load_option *lo, u8 **data);
>  efi_status_t efi_bootmgr_load(efi_handle_t *handle);
> 
> diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> index fa65445c12..e268e9c4b8 100644
> --- a/lib/efi_loader/efi_bootmgr.c
> +++ b/lib/efi_loader/efi_bootmgr.c
> @@ -38,24 +38,50 @@ static const struct efi_runtime_services *rs;
>   *
>   * @lo:		pointer to target
>   * @data:	serialized data
> + * @size:	size of the load option, on return size of the optional data
> + * Return:	status code
>   */
> -void efi_deserialize_load_option(struct efi_load_option *lo, u8 *data)
> +efi_status_t efi_deserialize_load_option(struct efi_load_option *lo, u8 *data,
> +					 efi_uintn_t *size)
>  {
> +	efi_uintn_t len;
> +
> +	len = sizeof(u32);
> +	if (*size < len + 2 * sizeof(u16))
> +		return EFI_INVALID_PARAMETER;
>  	lo->attributes = get_unaligned_le32(data);
> -	data += sizeof(u32);
> +	data += len;
> +	*size -= len;
> 
> +	len = sizeof(u16);
>  	lo->file_path_length = get_unaligned_le16(data);
> -	data += sizeof(u16);
> +	data += len;
> +	*size -= len;
> 
> -	/* FIXME */
>  	lo->label = (u16 *)data;
> -	data += (u16_strlen(lo->label) + 1) * sizeof(u16);
> -
> -	/* FIXME */
> +	len = u16_strnlen(lo->label, *size / sizeof(u16) - 1);
> +	if (lo->label[len])
> +		return EFI_INVALID_PARAMETER;
> +	len = (len + 1) * sizeof(u16);
> +	if (*size < len)
> +		return EFI_INVALID_PARAMETER;
> +	data += len;
> +	*size -= len;
> +
> +	len = lo->file_path_length;
> +	if (*size < len)
> +		return EFI_INVALID_PARAMETER;
>  	lo->file_path = (struct efi_device_path *)data;
> -	data += lo->file_path_length;
> +	 /*
> +	  * TODO: validate device path. There should be an end node within
> +	  * the indicated file_path_length.
> +	  */
> +	data += len;
> +	*size -= len;
> 
>  	lo->optional_data = data;
> +
> +	return EFI_SUCCESS;
>  }
> 
>  /**
> @@ -170,7 +196,11 @@ static efi_status_t try_load_entry(u16 n, efi_handle_t *handle)
>  	if (!load_option)
>  		return EFI_LOAD_ERROR;
> 
> -	efi_deserialize_load_option(&lo, load_option);
> +	ret = efi_deserialize_load_option(&lo, load_option, &size);
> +	if (ret != EFI_SUCCESS) {
> +		log_warning("Invalid load option for %ls\n", varname);
> +		goto error;
> +	}
> 
>  	if (lo.attributes & LOAD_OPTION_ACTIVE) {
>  		u32 attributes;
> --
> 2.26.2
>