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

Message-ID: <20250418183138.GE2676982@horms.kernel.org>
Date: Fri, 18 Apr 2025 19:31:38 +0100
From: Simon Horman <horms@...nel.org>
To: chia-yu.chang@...ia-bell-labs.com
Cc: 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, 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?

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

...