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

Adriano Córdova
2024-11-20T14:55:51Z

El mar, 19 nov 2024 a las 6:50, Heinrich Schuchardt (<
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>
> > ---
> >
> > 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.

>

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