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

On 19.11.24 11:34, Ilias Apalodimas wrote:

On Tue, 19 Nov 2024 at 11:50, Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:

...

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@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.

It's a known support gcc extension that treats void* similar to char*. Do you think it's worth changing all of the call sites we have?

No, I don't request this. But as pos is already char*, we should not use void* here.

Best regards

Heinrich

...

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,