[RFC PATCH 1/3] efi_loader: Introduce helper functions for EFI

[Heinrich Schuchardt]

On 13.01.21 12:11, Ilias Apalodimas wrote:
> A following patch introduces a different logic for loading initrd's
> based on the EFI_LOAD_FILE2_PROTOCOL.
> Since similar logic can be applied in the future for other system files
> (i.e DTBs), let's add some helper functions which will retrieve and
> parse file paths stored in EFI load options.
>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas at linaro.org>
> ---
>  include/efi_helper.h        |  23 ++++++
>  lib/efi_loader/efi_helper.c | 146 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 169 insertions(+)
>  create mode 100644 include/efi_helper.h
>  create mode 100644 lib/efi_loader/efi_helper.c
>
> diff --git a/include/efi_helper.h b/include/efi_helper.h
> new file mode 100644
> index 000000000000..4e6bd2f036df
> --- /dev/null
> +++ b/include/efi_helper.h
> @@ -0,0 +1,23 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Copyright (c) 2020, Linaro Limited
> + */
> +
> +#if !defined _EFI_HELPER_H_
> +#define _EFI_HELPER_H
> +
> +#include <efi.h>
> +#include <efi_api.h>
> +
> +enum load_option_dp_type {
> +	BOOT_IMAGE_DP,
> +	INITRD_DP,
> +	DTB_DP,
> +};
> +
> +void *efi_get_var(u16 *name, const efi_guid_t *vendor, efi_uintn_t *size);
> +struct efi_device_path *efi_get_fp_from_boot(int idx);
> +struct efi_device_path *efi_dp_instance_by_idx(struct efi_device_path *dp,
> +					       efi_uintn_t *size, int idx);
> +
> +#endif
> diff --git a/lib/efi_loader/efi_helper.c b/lib/efi_loader/efi_helper.c
> new file mode 100644
> index 000000000000..e2437a4f698b
> --- /dev/null
> +++ b/lib/efi_loader/efi_helper.c
> @@ -0,0 +1,146 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (c) 2020, Linaro Limited
> + */
> +
> +#include <common.h>
> +#include <env.h>
> +#include <malloc.h>
> +#include <dm.h>
> +#include <fs.h>
> +#include <efi_helper.h>
> +#include <efi_loader.h>
> +#include <efi_variable.h>
> +
> +/**
> + * efi_get_var() - read value of an EFI variable
> + *
> + * @name:	variable name
> + * @start:	vendor GUID
> + * @size:	size of allocated buffer
> + *
> + * Return:	buffer with variable data or NULL
> + */
> +void *efi_get_var(u16 *name, const efi_guid_t *vendor, efi_uintn_t *size)
> +{
> +	efi_status_t ret;
> +	void *buf = NULL;
> +
> +	*size = 0;
> +	ret = efi_get_variable_int(name, vendor, NULL, size, buf, NULL);
> +	if (ret == EFI_BUFFER_TOO_SMALL) {
> +		buf = malloc(*size);

Please, always check the output of malloc(), e.g.

		if (!buf)
			ret = EFI_OUT_OF_RESOURCES;
		else

> +		ret = efi_get_variable_int(name, vendor, NULL, size, buf, NULL);
> +	}
> +
> +	if (ret != EFI_SUCCESS) {
> +		free(buf);
> +		*size = 0;
> +		return NULL;
> +	}
> +
> +	return buf;
> +}
> +
> +/**
> + * efi_dp_instance_by_idx() - Get a file path with a specific index
> + *
> + * @name:	device path array
> + * @size:	size of the discovered device path
> + * @idx:	index of the device path
> + *
> + * Return:	device path or NULL. Caller must free the returned value
> + */
> +
> +struct
> +efi_device_path *efi_dp_instance_by_idx(struct efi_device_path *dp,
> +					efi_uintn_t *size, int idx)
> +{

idx should be of an unsigned type as we cannot have negative indices.

> +	struct efi_device_path *instance = NULL;
> +	efi_uintn_t instance_size = 0;
> +
> +	if (!efi_dp_is_multi_instance(dp))

Given a device path with one instance, why don't you allow me to read
index 0? I assume this check can be removed.

> +		return NULL;
> +
> +	while (idx >= 0 && dp) {
> +		instance = efi_dp_get_next_instance(&dp, &instance_size);
> +		if (idx && instance) {
> +			efi_free_pool(instance);
> +			instance_size = 0;
> +			instance = NULL;
> +		}
> +		idx--;
> +	}
> +	*size = instance_size;
> +
> +	return instance;

This can be simplified with unsigned idx:

for (; dp; --idx) {
	instance = efi_dp_get_next_instance(&dp, size);
	if (!idx)  {
		return instance;
	efi_free_pool(instance);
}
return NULL;

> +}
> +
> +/**
> + * create_boot_var_indexed() - Return Boot#### name were #### is replaced by
> + *			       the value of BootCurrent
> + *
> + * @var_name:		variable name
> + * @var_name_size:	size of var_name
> + *
> + * Return:	Status code
> + */
> +static efi_status_t create_boot_var_indexed(u16 var_name[], size_t var_name_size)
> +{
> +	efi_uintn_t boot_order_size;

You are reading BootCurrent, not BootOrder.

> +	efi_status_t ret;
> +	u16 boot_order;

Same here.

> +	u16 *pos;
> +
> +	boot_order_size = sizeof(boot_order);
> +	ret = efi_get_variable_int(L"BootCurrent",
> +				   &efi_global_variable_guid, NULL,
> +				   &boot_order_size, &boot_order, NULL);
> +	if (ret != EFI_SUCCESS)
> +		goto out;
> +
> +	pos = efi_create_indexed_name(var_name, var_name_size, "Boot",
> +				      boot_order);
> +	if (!pos) {
> +		ret = EFI_OUT_OF_RESOURCES;
> +		goto out;
> +	}
> +
> +out:
> +	return ret;
> +}
> +
> +/**
> + * efi_get_fp_from_var() - Retrieve and return a device path from an EFI
> + *			   Boot### variable
> + *
> + * @idx:	index of the instance to retrieve
> + *
> + * Return:	device path of NULL. Caller must free the returned value

%s/of/or/

> + */
> +struct efi_device_path *efi_get_fp_from_boot(int idx)
> +{
> +	struct efi_device_path *file_path;
> +	struct efi_load_option lo;
> +	void *var_value = NULL;
> +	efi_uintn_t size;
> +	efi_status_t ret;
> +	u16 var_name[16];
> +
> +	ret = create_boot_var_indexed(var_name, sizeof(var_name));
> +	if (ret != EFI_SUCCESS)
> +		return NULL;
> +
> +	var_value = efi_get_var(var_name, &efi_global_variable_guid, &size);
> +	if (!var_value)
> +		return NULL;
> +
> +	efi_deserialize_load_option(&lo, var_value, &size);
> +	file_path = efi_dp_instance_by_idx(lo.file_path, &size, idx);
> +	if (!file_path) {
> +		printf("Instance not found\n");

This message is not enough for a user to take action. I suggest

("Missing file path with index %d in %ls", idx, var_name)

We want to use logging. I assume this is not an error. Can we use
log_debug() here?

> +		return NULL;
> +	}
> +
> +	return file_path;
> +}
>

Some other functions would fit into the same C module. Candidates are:

* efi_create_indexed_name()
* efi_deserialize_load_option()
* efi_serialize_load_option()

But that can be done in a separate patch.

Best regards

Heinrich