Re: [PATCH net-next] net: Add options as a flexible array to struct ip_tunnel_info

[Gal Pressman <gal@nvidia.com>]

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.