On 11/02/2025 19:49, Kees Cook wrote:
>> @@ -659,7 +654,7 @@ static inline void ip_tunnel_info_opts_set(struct
>> ip_tunnel_info *info,
>> {
>> info->options_len = len;
>> if (len > 0) {
>> - memcpy(ip_tunnel_info_opts(info), from, len);
>> + memcpy(info->options, from, len);
>> ip_tunnel_flags_or(info->key.tun_flags, info->key.tun_flags,
>> flags);
>> }
>
> And I see info->options_len being set here before the copy into
> info_>options. What validates that "info" was allocated with enough
> space to hold "len" here? I would have expected this as allocation time
> instead of here.
Agree.
>
>> diff --git a/net/core/dst.c b/net/core/dst.c
>> index 9552a90d4772..d981c295a48e 100644
>> --- a/net/core/dst.c
>> +++ b/net/core/dst.c
>> @@ -286,7 +286,8 @@ struct metadata_dst *metadata_dst_alloc(u8 optslen, enum
>> metadata_type type,
>> {
>> struct metadata_dst *md_dst;
>>
>> - md_dst = kmalloc(sizeof(*md_dst) + optslen, flags);
>> + md_dst = kmalloc(struct_size(md_dst, u.tun_info.options, optslen),
>> + flags);
>> if (!md_dst)
>> return NULL;
>>
>
> I don't see options_len being set here? I assume since it's sub-type
> specific. I'd expect the type to be validated (i.e. optslen must == 0
> unless this is a struct ip_tunnel_info, and if so, set options_len here
> instead of in ip_tunnel_info_opts_set().
I think all callers of ip_tunnel_info_opts_set() do it right after
calling metadata_dst_alloc() (except for bpf_skb_set_tunnel_opt()?), so
in theory it's probably fine moving the assignment to the allocation.
But TBH, I'm not feeling comfortable changing this, many flows are
affected. And anyway, this should be done as a separate patch, unrelated
to this.
>
> Everything else looks very good, though, yes, I would agree with the
> alignment comments made down-thread. This was "accidentally correct"
> before in the sense that the end of the struct would be padded for
> alignment, but isn't guaranteed to be true with an explicit u8 array.
> The output of "pahole -C struct ip_tunnel_info" before/after should
> answer any questions, though.
Thanks, I will attach pahole's output in the commit message.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
