[PATCH v4 03/14] efi_loader: device_path: add efi_dp_from_ipv4

[Heinrich Schuchardt]

On 18.11.24 22:08, Adriano Cordova wrote:
> Add efi_dp_from_ipv4 to form a device path from an ipv4 address.
> 
> Signed-off-by: Adriano Cordova <adrianox at gmail.com>
> ---
> 
> Changes in v4:
> - Fixed memcpy mistake
> 
> Changes in v3:
> - Removed some unnecessary void* casts.
> - Changed sizeof(struct efi_device_path_ipv4) to sizeof(*ipv4dp)
>    in efi_dp_from_ipv4.
>    
>   lib/efi_loader/efi_device_path.c | 38 ++++++++++++++++++++++++++++++++
>   1 file changed, 38 insertions(+)
> 
> diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c
> index ee387e1dfd..cdeea4791f 100644
> --- a/lib/efi_loader/efi_device_path.c
> +++ b/lib/efi_loader/efi_device_path.c
> @@ -974,6 +974,44 @@ struct efi_device_path __maybe_unused *efi_dp_from_eth(void)
>   	return start;
>   }
>   

A function description is missing.

> +struct efi_device_path *efi_dp_from_ipv4(struct efi_ipv4_address *ip,
> +					 struct efi_ipv4_address *mask,
> +					 struct efi_ipv4_address *srv)
> +{
> +	struct efi_device_path *dp1, *dp2;
> +	efi_uintn_t ipv4dp_len;
> +	struct efi_device_path_ipv4 *ipv4dp;
> +	char *pos;
> +
> +	ipv4dp_len = sizeof(*ipv4dp);
> +	ipv4dp = efi_alloc(ipv4dp_len + sizeof(END));

ipv4dp is a small structure and is freed below.
Allocating it on the stack would reduce the code size.

> +	if (!ipv4dp) {
> +		log_err("Out of memory\n");

Writing messages inside a protocol implementation should be avoided.

> +		return NULL;
> +	}
> +
> +	ipv4dp->dp.type = DEVICE_PATH_TYPE_MESSAGING_DEVICE;
> +	ipv4dp->dp.sub_type = DEVICE_PATH_SUB_TYPE_MSG_IPV4;
> +	ipv4dp->dp.length = ipv4dp_len;
> +	ipv4dp->protocol = 6;
> +	if (ip)
> +		memcpy(&ipv4dp->local_ip_address, ip, sizeof(*ip));
> +	if (mask)
> +		memcpy(&ipv4dp->subnet_mask, mask, sizeof(*mask));
> +	if (srv)
> +		memcpy(&ipv4dp->remote_ip_address, srv, sizeof(*srv));
> +	pos = (void *)(uintptr_t)ipv4dp + ipv4dp_len;

Here you

1 - convert ipv4dp to uintptr_t
2 - convert the uintptr_t value to void *
3 - add an offset to the void * value

In the C standard adding an offset to a void * value results in 
undefined behavior. Unfortunately U-Boot code uses it a lot.

pos = (char *)ipv4dp + ipv4dp_len;

is all it takes to have a defined behavior here.

Best regards

Heinrich

> +	memcpy(pos, &END, sizeof(END));
> +
> +	dp1 = efi_dp_from_eth();
> +	dp2 = efi_dp_concat(dp1, (const struct efi_device_path *)ipv4dp, 0);
> +
> +	efi_free_pool(ipv4dp);
> +	efi_free_pool(dp1);
> +
> +	return dp2;
> +}
> +
>   /* Construct a device-path for memory-mapped image */
>   struct efi_device_path *efi_dp_from_mem(uint32_t memory_type,
>   					uint64_t start_address,