| 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: <da87ed1c-165d-fd21-7292-19468d1c8a8c@kernel.org>
Date: Tue, 23 Sep 2025 21:23:08 +0300 (EEST)
From: Ilpo Järvinen <ij@...nel.org>
To: Dan Carpenter <dan.carpenter@...aro.org>,
chia-yu.chang@...ia-bell-labs.com
cc: netdev@...r.kernel.org
Subject: Re: [bug report] tcp: accecn: AccECN option
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.
> 676 const u8 ect0_idx = INET_ECN_ECT_0 - 1;
> 677 const u8 ect1_idx = INET_ECN_ECT_1 - 1;
> 678 const u8 ce_idx = INET_ECN_CE - 1;
> 679 u32 e0b;
> 680 u32 e1b;
> 681 u32 ceb;
> 682 u8 len;
> 683
> 684 e0b = ecn_bytes[ect0_idx] + TCP_ACCECN_E0B_INIT_OFFSET;
> 685 e1b = ecn_bytes[ect1_idx] + TCP_ACCECN_E1B_INIT_OFFSET;
> 686 ceb = ecn_bytes[ce_idx] + TCP_ACCECN_CEB_INIT_OFFSET;
> 687 len = TCPOLEN_ACCECN_BASE +
> 688 opts->num_accecn_fields * TCPOLEN_ACCECN_PERFIELD;
> 689
> 690 if (opts->num_accecn_fields == 2) {
> 691 *ptr++ = htonl((TCPOPT_ACCECN1 << 24) | (len << 16) |
> 692 ((e1b >> 8) & 0xffff));
> 693 *ptr++ = htonl(((e1b & 0xff) << 24) |
> 694 (ceb & 0xffffff));
> 695 } else if (opts->num_accecn_fields == 1) {
> 696 *ptr++ = htonl((TCPOPT_ACCECN1 << 24) | (len << 16) |
> 697 ((e1b >> 8) & 0xffff));
> 698 leftover_highbyte = e1b & 0xff;
> 699 leftover_lowbyte = TCPOPT_NOP;
> 700 } else if (opts->num_accecn_fields == 0) {
> 701 leftover_highbyte = TCPOPT_ACCECN1;
> 702 leftover_lowbyte = len;
> 703 } else if (opts->num_accecn_fields == 3) {
> 704 *ptr++ = htonl((TCPOPT_ACCECN1 << 24) | (len << 16) |
> 705 ((e1b >> 8) & 0xffff));
> 706 *ptr++ = htonl(((e1b & 0xff) << 24) |
> 707 (ceb & 0xffffff));
> 708 *ptr++ = htonl(((e0b & 0xffffff) << 8) |
> 709 TCPOPT_NOP);
> 710 }
> 711 if (tp) {
> ^^
> Here we assume tp can be NULL
>
> 712 tp->accecn_minlen = 0;
> 713 tp->accecn_opt_tstamp = tp->tcp_mstamp;
> 714 if (tp->accecn_opt_demand)
> 715 tp->accecn_opt_demand--;
> 716 }
> 717 }
> 718
> 719 if (unlikely(OPTION_SACK_ADVERTISE & options)) {
> 720 *ptr++ = htonl((leftover_highbyte << 24) |
> 721 (leftover_lowbyte << 16) |
> 722 (TCPOPT_SACK_PERM << 8) |
> 723 TCPOLEN_SACK_PERM);
> 724 leftover_highbyte = TCPOPT_NOP;
> 725 leftover_lowbyte = TCPOPT_NOP;
> 726 }
> 727
> 728 if (unlikely(OPTION_WSCALE & options)) {
> 729 u8 highbyte = TCPOPT_NOP;
> 730
> 731 /* Do not split the leftover 2-byte to fit into a single
> 732 * NOP, i.e., replace this NOP only when 1 byte is leftover
> 733 * within leftover_highbyte.
> 734 */
> 735 if (unlikely(leftover_highbyte != TCPOPT_NOP &&
> 736 leftover_lowbyte == TCPOPT_NOP)) {
> 737 highbyte = leftover_highbyte;
> 738 leftover_highbyte = TCPOPT_NOP;
> 739 }
> 740 *ptr++ = htonl((highbyte << 24) |
> 741 (TCPOPT_WINDOW << 16) |
> 742 (TCPOLEN_WINDOW << 8) |
> 743 opts->ws);
> 744 }
> 745
> 746 if (unlikely(opts->num_sack_blocks)) {
> --> 747 struct tcp_sack_block *sp = tp->rx_opt.dsack ?
> ^^^^^^^^^^^^^^^^
> Unchecked dereference here.
>
> 748 tp->duplicate_sack : tp->selective_acks;
> 749 int this_sack;
> 750
> 751 *ptr++ = htonl((leftover_highbyte << 24) |
> 752 (leftover_lowbyte << 16) |
> 753 (TCPOPT_SACK << 8) |
> 754 (TCPOLEN_SACK_BASE + (opts->num_sack_blocks *
> 755 TCPOLEN_SACK_PERBLOCK)));
> 756 leftover_highbyte = TCPOPT_NOP;
> 757 leftover_lowbyte = TCPOPT_NOP;
> 758
> 759 for (this_sack = 0; this_sack < opts->num_sack_blocks;
> 760 ++this_sack) {
> 761 *ptr++ = htonl(sp[this_sack].start_seq);
> 762 *ptr++ = htonl(sp[this_sack].end_seq);
> 763 }
> 764
> 765 tp->rx_opt.dsack = 0;
> 766 } else if (unlikely(leftover_highbyte != TCPOPT_NOP ||
> 767 leftover_lowbyte != TCPOPT_NOP)) {
> 768 *ptr++ = htonl((leftover_highbyte << 24) |
> 769 (leftover_lowbyte << 16) |
> 770 (TCPOPT_NOP << 8) |
> 771 TCPOPT_NOP);
> 772 leftover_highbyte = TCPOPT_NOP;
> 773 leftover_lowbyte = TCPOPT_NOP;
> 774 }
> 775
> 776 if (unlikely(OPTION_FAST_OPEN_COOKIE & options)) {
> 777 struct tcp_fastopen_cookie *foc = opts->fastopen_cookie;
> 778 u8 *p = (u8 *)ptr;
> 779 u32 len; /* Fast Open option length */
> 780
> 781 if (foc->exp) {
> 782 len = TCPOLEN_EXP_FASTOPEN_BASE + foc->len;
> 783 *ptr = htonl((TCPOPT_EXP << 24) | (len << 16) |
> 784 TCPOPT_FASTOPEN_MAGIC);
> 785 p += TCPOLEN_EXP_FASTOPEN_BASE;
> 786 } else {
> 787 len = TCPOLEN_FASTOPEN_BASE + foc->len;
> 788 *p++ = TCPOPT_FASTOPEN;
> 789 *p++ = len;
> 790 }
> 791
> 792 memcpy(p, foc->val, foc->len);
> 793 if ((len & 3) == 2) {
> 794 p[foc->len] = TCPOPT_NOP;
> 795 p[foc->len + 1] = TCPOPT_NOP;
> 796 }
> 797 ptr += (len + 3) >> 2;
> 798 }
> 799
> 800 smc_options_write(ptr, &options);
> 801
> 802 mptcp_options_write(th, ptr, tp, opts);
> ^^
> The last dereference is checked for NULL but the others aren't.
--
i.
Powered by blists - more mailing lists