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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ee761ca9-5aa0-4f48-96b9-295ed2c444c4@nvidia.com>
Date: Tue, 11 Feb 2025 20:59:24 +0200
From: Gal Pressman <gal@...dia.com>
To: Kees Cook <kees@...nel.org>
Cc: netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>,
 Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>, Andrew Lunn <andrew+netdev@...n.ch>,
 Tariq Toukan <tariqt@...dia.com>, Louis Peens <louis.peens@...igine.com>,
 Simon Horman <horms@...nel.org>, David Ahern <dsahern@...nel.org>,
 Pravin B Shelar <pshelar@....org>, Yotam Gigi <yotam.gi@...il.com>,
 Jamal Hadi Salim <jhs@...atatu.com>, Cong Wang <xiyou.wangcong@...il.com>,
 Jiri Pirko <jiri@...nulli.us>, "Gustavo A. R. Silva"
 <gustavoars@...nel.org>, dev@...nvswitch.org,
 linux-hardening@...r.kernel.org, Cosmin Ratiu <cratiu@...dia.com>
Subject: Re: [PATCH net-next] net: Add options as a flexible array to struct
 ip_tunnel_info

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.