| 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
| ||
|
Message-ID: <aNPwBM5MvpPojt9F@stanley.mountain>
Date: Wed, 24 Sep 2025 16:20:04 +0300
From: Dan Carpenter <dan.carpenter@...aro.org>
To: "Chia-Yu Chang (Nokia)" <chia-yu.chang@...ia-bell-labs.com>
Cc: Ilpo Järvinen <ij@...nel.org>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [bug report] tcp: accecn: AccECN option
On Wed, Sep 24, 2025 at 12:25:16PM +0000, Chia-Yu Chang (Nokia) wrote:
> > -----Original Message-----
> > From: Ilpo Järvinen <ij@...nel.org>
> > Sent: Tuesday, September 23, 2025 8:23 PM
> > To: Dan Carpenter <dan.carpenter@...aro.org>; Chia-Yu Chang (Nokia) <chia-yu.chang@...ia-bell-labs.com>
> > Cc: netdev@...r.kernel.org
> > Subject: Re: [bug report] tcp: accecn: AccECN option
> >
> >
> > CAUTION: This is an external email. Please be very careful when clicking links or opening attachments. See the URL nok.it/ext for additional information.
> >
> >
> >
> > On Tue, 23 Sep 2025, Dan Carpenter wrote:
> >
> > > Hello Ilpo Järvinen,
> > >
> > > Commit b5e74132dfbe ("tcp: accecn: AccECN option") from Sep 16, 2025
> > > (linux-next), leads to the following Smatch static checker warning:
> > >
> > > net/ipv4/tcp_output.c:747 tcp_options_write()
> > > error: we previously assumed 'tp' could be null (see line 711)
> > >
> > > net/ipv4/tcp_output.c
> > > 630 static void tcp_options_write(struct tcphdr *th, struct tcp_sock *tp,
> > > 631 const struct tcp_request_sock *tcprsk,
> > > 632 struct tcp_out_options *opts,
> > > 633 struct tcp_key *key)
> > > 634 {
> > > 635 u8 leftover_highbyte = TCPOPT_NOP; /* replace 1st NOP if avail */
> > > 636 u8 leftover_lowbyte = TCPOPT_NOP; /* replace 2nd NOP in succession */
> > > 637 __be32 *ptr = (__be32 *)(th + 1);
> > > 638 u16 options = opts->options; /* mungable copy */
> > > 639
> > > 640 if (tcp_key_is_md5(key)) {
> > > 641 *ptr++ = htonl((TCPOPT_NOP << 24) | (TCPOPT_NOP << 16) |
> > > 642 (TCPOPT_MD5SIG << 8) | TCPOLEN_MD5SIG);
> > > 643 /* overload cookie hash location */
> > > 644 opts->hash_location = (__u8 *)ptr;
> > > 645 ptr += 4;
> > > 646 } else if (tcp_key_is_ao(key)) {
> > > 647 ptr = process_tcp_ao_options(tp, tcprsk, opts, key, ptr);
> > > ^^ Sometimes
> > > dereferenced here.
> > >
> > > 648 }
> > > 649 if (unlikely(opts->mss)) {
> > > 650 *ptr++ = htonl((TCPOPT_MSS << 24) |
> > > 651 (TCPOLEN_MSS << 16) |
> > > 652 opts->mss);
> > > 653 }
> > > 654
> > > 655 if (likely(OPTION_TS & options)) {
> > > 656 if (unlikely(OPTION_SACK_ADVERTISE & options)) {
> > > 657 *ptr++ = htonl((TCPOPT_SACK_PERM << 24) |
> > > 658 (TCPOLEN_SACK_PERM << 16) |
> > > 659 (TCPOPT_TIMESTAMP << 8) |
> > > 660 TCPOLEN_TIMESTAMP);
> > > 661 options &= ~OPTION_SACK_ADVERTISE;
> > > 662 } else {
> > > 663 *ptr++ = htonl((TCPOPT_NOP << 24) |
> > > 664 (TCPOPT_NOP << 16) |
> > > 665 (TCPOPT_TIMESTAMP << 8) |
> > > 666 TCPOLEN_TIMESTAMP);
> > > 667 }
> > > 668 *ptr++ = htonl(opts->tsval);
> > > 669 *ptr++ = htonl(opts->tsecr);
> > > 670 }
> > > 671
> > > 672 if (OPTION_ACCECN & options) {
> > > 673 const u32 *ecn_bytes = opts->use_synack_ecn_bytes ?
> > > 674 synack_ecn_bytes :
> > > 675 tp->received_ecn_bytes;
> > > ^^^^ Dereference
> >
> > Hi Dan,
> >
> > While it is long ago I made these changes (they might have changed a little from that), I can say this part is going to be extremely tricky for static checkers because TCP state machine(s) are quite complex.
> >
> > TCP options can be written to a packet when tp has not yet been created (during handshake) as well as after creation of tp using this same function. Not all combinations are possible because handshake has to complete before some things are enabled.
> >
> > Without checking this myself, my assumption is that ->use_synack_ecn_bytes is set when we don't have tp available yet as SYNACKs relate to handshake.
> > So the tp check is likely there even if not literally written.
> >
> > Chia-Yu, could you please check these cases for the parts that new code was introduced whether tp can be NULL? I think this particular line is the most likely one to be wrong if something is, that is, can OPTION_ACCECN be set while use_synack_ecn_bytes is not when tp is not yet there.
>
> Hi Ilpo and Dan,
>
> I've checked that OPTION_ACCECN and use_synack_ecn_bytes will always be set at the same time.
> The case you said (OPTION_ACCECN is 1, but use_synack_ecn_bytes is 0) can only happen when tp is set, because this is already ESTABLISHED state (see tcp_established_options in tcp_output.c).
>
> So, I would say this is ok.
Thanks!
> But if this is indeed a concern for the checker, just add another "if (tp)".
>
Please, don't add anything just for the checker. These are a one time
only email. Old warnings are false positives because kernel developers
are good about fixing actual issues.
regards,
dan carpenter
Powered by blists - more mailing lists