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

[Heinrich Schuchardt]

On 19.11.24 11:34, Ilias Apalodimas wrote:
> On Tue, 19 Nov 2024 at 11:50, Heinrich Schuchardt
> <heinrich.schuchardt at 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 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.
> 
> 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,
>>