netdev - Re: KASAN: use-after-free Read in ccid2_hc_tx_packet

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 for Android: free password hash cracker in your pocket

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

Message-ID: <938106e0-269a-4d5a-995f-2314fecedb3a@yandex.ru>
Date: Wed, 23 Oct 2024 15:09:59 +0300
From: Dmitry Antipov <dmantipov@...dex.ru>
To: Kuniyuki Iwashima <kuniyu@...zon.com>
Cc: dccp@...r.kernel.org, "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
 syzbot+554ccde221001ab5479a@...kaller.appspotmail.com
Subject: Re: KASAN: use-after-free Read in ccid2_hc_tx_packet_recv

Looking through https://syzkaller.appspot.com/bug?extid=554ccde221001ab5479a,
I've found the problem which may be illustrated with the following patch:

diff --git a/net/dccp/ipv4.c b/net/dccp/ipv4.c
index 5926159a6f20..eb551872170c 100644
--- a/net/dccp/ipv4.c
+++ b/net/dccp/ipv4.c
@@ -678,6 +678,7 @@ int dccp_v4_do_rcv(struct sock *sk, struct sk_buff *skb)

         if (sk->sk_state == DCCP_OPEN) { /* Fast path */
                 if (dccp_rcv_established(sk, skb, dh, skb->len))
+                       /* Go to reset here */
                         goto reset;
                 return 0;
         }
@@ -712,6 +713,7 @@ int dccp_v4_do_rcv(struct sock *sk, struct sk_buff *skb)

  reset:
         dccp_v4_ctl_send_reset(sk, skb, SK_RST_REASON_NOT_SPECIFIED);
+       /* Freeing skb may leave dangling pointers in ack vectors */
         kfree_skb(skb);
         return 0;
  }

I'm not an expert with DCCP protocol innards and have no idea whether ack
vectors still needs to be processed after sending reset. But if it is so,
the solution might be to copy all of the data from the relevant skbs instead
of just saving the pointers, e.g.:

diff --git a/net/dccp/ackvec.c b/net/dccp/ackvec.c
index 1cba001bb4c8..24c6ad06d896 100644
--- a/net/dccp/ackvec.c
+++ b/net/dccp/ackvec.c
@@ -347,17 +347,18 @@ void dccp_ackvec_clear_state(struct dccp_ackvec *av, const u64 ackno)
  }

  /*
- *	Routines to keep track of Ack Vectors received in an skb
+ *	Routines to keep track of Ack Vectors copied from the received skb
   */
  int dccp_ackvec_parsed_add(struct list_head *head, u8 *vec, u8 len, u8 nonce)
  {
-	struct dccp_ackvec_parsed *new = kmalloc(sizeof(*new), GFP_ATOMIC);
-
+	struct dccp_ackvec_parsed *new = kmalloc(struct_size(new, vec, len),
+						 GFP_ATOMIC);
  	if (new == NULL)
  		return -ENOBUFS;
-	new->vec   = vec;
-	new->len   = len;
+
+	new->len = len;
  	new->nonce = nonce;
+	memcpy(new->vec, vec, len);

  	list_add_tail(&new->node, head);
  	return 0;
diff --git a/net/dccp/ackvec.h b/net/dccp/ackvec.h
index d2c4220fb377..491fd587de90 100644
--- a/net/dccp/ackvec.h
+++ b/net/dccp/ackvec.h
@@ -117,18 +117,18 @@ static inline bool dccp_ackvec_is_empty(const struct dccp_ackvec *av)

  /**
   * struct dccp_ackvec_parsed  -  Record offsets of Ack Vectors in skb
- * @vec:	start of vector (offset into skb)
+ * @vec:	contents of ack vector (copied from skb)
   * @len:	length of @vec
   * @nonce:	whether @vec had an ECN nonce of 0 or 1
   * @node:	FIFO - arranged in descending order of ack_ackno
   *
- * This structure is used by CCIDs to access Ack Vectors in a received skb.
+ * This structure is used by CCIDs to access Ack Vectors from the received skb.
   */
  struct dccp_ackvec_parsed {
-	u8		 *vec,
-			 len,
-			 nonce:1;
  	struct list_head node;
+	u8 len;
+	u8 nonce:1;
+	u8 vec[] __counted_by(len);
  };

  int dccp_ackvec_parsed_add(struct list_head *head, u8 *vec, u8 len, u8 nonce);
diff --git a/net/dccp/ccids/ccid2.c b/net/dccp/ccids/ccid2.c
index d6b30700af67..a1f2da3c4fa9 100644
--- a/net/dccp/ccids/ccid2.c
+++ b/net/dccp/ccids/ccid2.c
@@ -589,14 +589,15 @@ static void ccid2_hc_tx_packet_recv(struct sock *sk, struct sk_buff *skb)
  	/* go through all ack vectors */
  	list_for_each_entry(avp, &hc->tx_av_chunks, node) {
  		/* go through this ack vector */
-		for (; avp->len--; avp->vec++) {
+		u8 *v;
+		for (v = avp->vec; v < avp->vec + avp->len--; v++) {
  			u64 ackno_end_rl = SUB48(ackno,
-						 dccp_ackvec_runlen(avp->vec));
+						 dccp_ackvec_runlen(v));

  			ccid2_pr_debug("ackvec %llu |%u,%u|\n",
  				       (unsigned long long)ackno,
-				       dccp_ackvec_state(avp->vec) >> 6,
-				       dccp_ackvec_runlen(avp->vec));
+				       dccp_ackvec_state(v) >> 6,
+				       dccp_ackvec_runlen(v));
  			/* if the seqno we are analyzing is larger than the
  			 * current ackno, then move towards the tail of our
  			 * seqnos.
@@ -615,7 +616,7 @@ static void ccid2_hc_tx_packet_recv(struct sock *sk, struct sk_buff *skb)
  			 * run length
  			 */
  			while (between48(seqp->ccid2s_seq,ackno_end_rl,ackno)) {
-				const u8 state = dccp_ackvec_state(avp->vec);
+				const u8 state = dccp_ackvec_state(v);

  				/* new packet received or marked */
  				if (state != DCCPAV_NOT_RECEIVED &&

Comments are highly appreciated.

Dmitry