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

On 20.11.24 15:55, Adriano Córdova wrote:

El mar, 19 nov 2024 a las 6:50, Heinrich Schuchardt (<heinrich.schuchardt@canonical.com mailto:heinrich.schuchardt@canonical.com>) escribió:

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
<mailto: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.

But as it is a device path it has to be allocated contiguously to the END node, see the next lines. It is then passed to efi_dp_concat where this is important.

struct { struct efi_device_path_ipv4 ipv4dp; struct efi_device_path end; } dp;

should give you a variable of adequate size.

Best regards

Heinrich

 > +     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

It was originally a char *, I will change it back

 > +     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,

Best, Adriano