[PATCH 2/6] efi: Convert device_path allocation to use malloc()

[Heinrich Schuchardt]

On 25.07.24 15:56, Simon Glass wrote:
> Currently this calls efi_alloc() which reserves a page for each
> allocation and this can overwrite memory that will be used for reading
> images.

We already agreed to integrate LMB and UEFI memory management to avoid
this sort of problem.

Please, have a look at chapter 10.5, "Device Path Utilities Protocol" of
the UEFI specification.

EFI_DEVICE_PATH_UTILITIES_PROTOCOL.DuplicateDevicePath() must return
memory that an EFI application can release by calling
EFI_BOOT_SERVICES.FreePool().

Your patch does not comply to the specification.

Best regards

Heinrich

>
> Switch the code to use malloc(), as with other parts of EFI, such as
> efi_add_protocol().
>
> Signed-off-by: Simon Glass <sjg at chromium.org>
> ---
>
>   lib/efi_loader/efi_device_path.c         | 43 ++++++++++++------------
>   lib/efi_loader/efi_device_path_to_text.c |  2 +-
>   2 files changed, 22 insertions(+), 23 deletions(-)
>
> diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c
> index 0f684590f22..47038e8e585 100644
> --- a/lib/efi_loader/efi_device_path.c
> +++ b/lib/efi_loader/efi_device_path.c
> @@ -262,7 +262,7 @@ struct efi_device_path *efi_dp_dup(const struct efi_device_path *dp)
>   	if (!dp)
>   		return NULL;
>
> -	ndp = efi_alloc(sz);
> +	ndp = malloc(sz);
>   	if (!ndp)
>   		return NULL;
>   	memcpy(ndp, dp, sz);
> @@ -315,7 +315,7 @@ efi_device_path *efi_dp_concat(const struct efi_device_path *dp1,
>   			end_size = 2 * sizeof(END);
>   		else
>   			end_size = sizeof(END);
> -		p = efi_alloc(sz1 + sz2 + end_size);
> +		p = malloc(sz1 + sz2 + end_size);
>   		if (!p)
>   			return NULL;
>   		ret = p;
> @@ -347,7 +347,7 @@ struct efi_device_path *efi_dp_append_node(const struct efi_device_path *dp,
>   		ret = efi_dp_dup(dp);
>   	} else if (!dp) {
>   		size_t sz = node->length;
> -		void *p = efi_alloc(sz + sizeof(END));
> +		void *p = malloc(sz + sizeof(END));
>   		if (!p)
>   			return NULL;
>   		memcpy(p, node, sz);
> @@ -356,7 +356,7 @@ struct efi_device_path *efi_dp_append_node(const struct efi_device_path *dp,
>   	} else {
>   		/* both dp and node are non-null */
>   		size_t sz = efi_dp_size(dp);
> -		void *p = efi_alloc(sz + node->length + sizeof(END));
> +		void *p = malloc(sz + node->length + sizeof(END));
>   		if (!p)
>   			return NULL;
>   		memcpy(p, dp, sz);
> @@ -377,7 +377,7 @@ struct efi_device_path *efi_dp_create_device_node(const u8 type,
>   	if (length < sizeof(struct efi_device_path))
>   		return NULL;
>
> -	ret = efi_alloc(length);
> +	ret = malloc(length);
>   	if (!ret)
>   		return ret;
>   	ret->type = type;
> @@ -399,7 +399,7 @@ struct efi_device_path *efi_dp_append_instance(
>   		return efi_dp_dup(dpi);
>   	sz = efi_dp_size(dp);
>   	szi = efi_dp_instance_size(dpi);
> -	p = efi_alloc(sz + szi + 2 * sizeof(END));
> +	p = malloc(sz + szi + 2 * sizeof(END));
>   	if (!p)
>   		return NULL;
>   	ret = p;
> @@ -424,7 +424,7 @@ struct efi_device_path *efi_dp_get_next_instance(struct efi_device_path **dp,
>   	if (!dp || !*dp)
>   		return NULL;
>   	sz = efi_dp_instance_size(*dp);
> -	p = efi_alloc(sz + sizeof(END));
> +	p = malloc(sz + sizeof(END));
>   	if (!p)
>   		return NULL;
>   	memcpy(p, *dp, sz + sizeof(END));
> @@ -825,11 +825,11 @@ struct efi_device_path *efi_dp_from_part(struct blk_desc *desc, int part)
>   {
>   	void *buf, *start;
>
> -	start = buf = efi_alloc(dp_part_size(desc, part) + sizeof(END));
> -	if (!buf)
> +	start = malloc(dp_part_size(desc, part) + sizeof(END));
> +	if (!start)
>   		return NULL;
>
> -	buf = dp_part_fill(buf, desc, part);
> +	buf = dp_part_fill(start, desc, part);
>
>   	*((struct efi_device_path *)buf) = END;
>
> @@ -852,7 +852,7 @@ struct efi_device_path *efi_dp_part_node(struct blk_desc *desc, int part)
>   		dpsize = sizeof(struct efi_device_path_cdrom_path);
>   	else
>   		dpsize = sizeof(struct efi_device_path_hard_drive_path);
> -	buf = efi_alloc(dpsize);
> +	buf = malloc(dpsize);
>
>   	if (buf)
>   		dp_part_node(buf, desc, part);
> @@ -912,7 +912,7 @@ struct efi_device_path *efi_dp_from_file(const struct efi_device_path *dp,
>   	if (fpsize > U16_MAX)
>   		return NULL;
>
> -	buf = efi_alloc(dpsize + fpsize + sizeof(END));
> +	buf = malloc(dpsize + fpsize + sizeof(END));
>   	if (!buf)
>   		return NULL;
>
> @@ -940,7 +940,7 @@ struct efi_device_path *efi_dp_from_uart(void)
>   	struct efi_device_path_uart *uart;
>   	size_t dpsize = dp_size(dm_root()) + sizeof(*uart) + sizeof(END);
>
> -	buf = efi_alloc(dpsize);
> +	buf = malloc(dpsize);
>   	if (!buf)
>   		return NULL;
>   	pos = dp_fill(buf, dm_root());
> @@ -963,11 +963,11 @@ struct efi_device_path __maybe_unused *efi_dp_from_eth(void)
>
>   	dpsize += dp_size(eth_get_dev());
>
> -	start = buf = efi_alloc(dpsize + sizeof(END));
> -	if (!buf)
> +	start = malloc(dpsize + sizeof(END));
> +	if (!start)
>   		return NULL;
>
> -	buf = dp_fill(buf, eth_get_dev());
> +	buf = dp_fill(start, eth_get_dev());
>
>   	*((struct efi_device_path *)buf) = END;
>
> @@ -980,22 +980,21 @@ struct efi_device_path *efi_dp_from_mem(uint32_t memory_type,
>   					uint64_t end_address)
>   {
>   	struct efi_device_path_memory *mdp;
> -	void *buf, *start;
> +	void *start;
>
> -	start = buf = efi_alloc(sizeof(*mdp) + sizeof(END));
> -	if (!buf)
> +	start = malloc(sizeof(*mdp) + sizeof(END));
> +	if (!start)
>   		return NULL;
>
> -	mdp = buf;
> +	mdp = start;
>   	mdp->dp.type = DEVICE_PATH_TYPE_HARDWARE_DEVICE;
>   	mdp->dp.sub_type = DEVICE_PATH_SUB_TYPE_MEMORY;
>   	mdp->dp.length = sizeof(*mdp);
>   	mdp->memory_type = memory_type;
>   	mdp->start_address = start_address;
>   	mdp->end_address = end_address;
> -	buf = &mdp[1];
>
> -	*((struct efi_device_path *)buf) = END;
> +	*((struct efi_device_path *)&mdp[1]) = END;
>
>   	return start;
>   }
> diff --git a/lib/efi_loader/efi_device_path_to_text.c b/lib/efi_loader/efi_device_path_to_text.c
> index 0c7b30a26e7..4192581ac45 100644
> --- a/lib/efi_loader/efi_device_path_to_text.c
> +++ b/lib/efi_loader/efi_device_path_to_text.c
> @@ -33,7 +33,7 @@ static u16 *efi_str_to_u16(char *str)
>   	u16 *out, *dst;
>
>   	len = sizeof(u16) * (utf8_utf16_strlen(str) + 1);
> -	out = efi_alloc(len);
> +	out = malloc(len);
>   	if (!out)
>   		return NULL;
>   	dst = out;