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

[Heinrich Schuchardt]

On 20.11.24 15:55, Adriano Córdova wrote:
> 
> 
> El mar, 19 nov 2024 a las 6:50, Heinrich Schuchardt 
> (<heinrich.schuchardt at canonical.com 
> <mailto:heinrich.schuchardt at 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 at gmail.com
>     <mailto: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.
> 
> 
> 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