netdev - Re: [PATCH v4 net-next 09/15] tcp: accecn: AccECN option

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] [day] [month] [year] [list]

Message-ID: <8b6f580b-d682-91f7-f958-1806ee6e8bbe@kernel.org>
Date: Fri, 18 Apr 2025 22:35:14 +0300 (EEST)
From: Ilpo Järvinen <ij@...nel.org>
To: Simon Horman <horms@...nel.org>
cc: chia-yu.chang@...ia-bell-labs.com, dsahern@...nel.org, kuniyu@...zon.com, 
    bpf@...r.kernel.org, netdev@...r.kernel.org, dave.taht@...il.com, 
    pabeni@...hat.com, jhs@...atatu.com, kuba@...nel.org, 
    stephen@...workplumber.org, xiyou.wangcong@...il.com, jiri@...nulli.us, 
    davem@...emloft.net, edumazet@...gle.com, andrew+netdev@...n.ch, 
    donald.hunter@...il.com, ast@...erby.net, liuhangbin@...il.com, 
    shuah@...nel.org, linux-kselftest@...r.kernel.org, ij@...nel.org, 
    ncardwell@...gle.com, koen.de_schepper@...ia-bell-labs.com, 
    g.white@...lelabs.com, ingemar.s.johansson@...csson.com, 
    mirja.kuehlewind@...csson.com, cheshire@...le.com, rs.ietf@....at, 
    Jason_Livingood@...cast.com, vidhi_goel@...le.com
Subject: Re: [PATCH v4 net-next 09/15] tcp: accecn: AccECN option

On Fri, 18 Apr 2025, Simon Horman wrote:

> On Fri, Apr 18, 2025 at 01:00:23AM +0200, chia-yu.chang@...ia-bell-labs.com wrote:
> 
> ...
> 
> > diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> 
> ...
> 
> > @@ -766,6 +769,47 @@ static void tcp_options_write(struct tcphdr *th, struct tcp_sock *tp,
> >  		*ptr++ = htonl(opts->tsecr);
> >  	}
> >  
> > +	if (OPTION_ACCECN & options) {
> > +		const u8 ect0_idx = INET_ECN_ECT_0 - 1;
> > +		const u8 ect1_idx = INET_ECN_ECT_1 - 1;
> > +		const u8 ce_idx = INET_ECN_CE - 1;
> > +		u32 e0b;
> > +		u32 e1b;
> > +		u32 ceb;
> > +		u8 len;
> > +
> > +		e0b = opts->ecn_bytes[ect0_idx] + TCP_ACCECN_E0B_INIT_OFFSET;
> > +		e1b = opts->ecn_bytes[ect1_idx] + TCP_ACCECN_E1B_INIT_OFFSET;
> > +		ceb = opts->ecn_bytes[ce_idx] + TCP_ACCECN_CEB_INIT_OFFSET;
> > +		len = TCPOLEN_ACCECN_BASE +
> > +		      opts->num_accecn_fields * TCPOLEN_ACCECN_PERFIELD;
> > +
> > +		if (opts->num_accecn_fields == 2) {
> > +			*ptr++ = htonl((TCPOPT_ACCECN1 << 24) | (len << 16) |
> > +				       ((e1b >> 8) & 0xffff));
> > +			*ptr++ = htonl(((e1b & 0xff) << 24) |
> > +				       (ceb & 0xffffff));
> > +		} else if (opts->num_accecn_fields == 1) {
> > +			*ptr++ = htonl((TCPOPT_ACCECN1 << 24) | (len << 16) |
> > +				       ((e1b >> 8) & 0xffff));
> > +			leftover_bytes = ((e1b & 0xff) << 8) |
> > +					 TCPOPT_NOP;
> > +			leftover_size = 1;
> > +		} else if (opts->num_accecn_fields == 0) {
> > +			leftover_bytes = (TCPOPT_ACCECN1 << 8) | len;
> > +			leftover_size = 2;
> > +		} else if (opts->num_accecn_fields == 3) {
> > +			*ptr++ = htonl((TCPOPT_ACCECN1 << 24) | (len << 16) |
> > +				       ((e1b >> 8) & 0xffff));
> > +			*ptr++ = htonl(((e1b & 0xff) << 24) |
> > +				       (ceb & 0xffffff));
> > +			*ptr++ = htonl(((e0b & 0xffffff) << 8) |
> > +				       TCPOPT_NOP);
> > +		}
> > +		if (tp)
> > +			tp->accecn_minlen = 0;
> 
> Hi,
> 
> I'm sorry if this is a false positive: Smatch flags that here we assume
> that tp might be NULL, while elsewhere in this function tp is dereferenced
> unconditionally. So my question is, can tp be NULL here?

Hi Simon,

Thanks for taking look!

This looks a false positive. It's because tcp_options_write() is shared by 
the handshake and established connections. A direct caller from the 
handshake path passes NULL as tp:

	tcp_options_write(th, NULL, tcp_rsk(req), &opts, &key);

The thing that smatch doesn't know is that some TCP options are not going 
to be present during handshake. Those code paths that only deal with 
options that are for an established connection can assume full sk/tp has 
been already instantiated so they don't need to check tp. In addition, 
some funcs tcp_options_write() calls to make the opposite check by 
checking tcprsk instead but it has the same effect.

-- 
 i.

> > +	}
> > +
> >  	if (unlikely(OPTION_SACK_ADVERTISE & options)) {
> >  		*ptr++ = htonl((leftover_bytes << 16) |
> >  			       (TCPOPT_SACK_PERM << 8) |
> 
> ...
>