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: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20241023175121.36351-1-kuniyu@amazon.com>
Date: Wed, 23 Oct 2024 10:51:21 -0700
From: Kuniyuki Iwashima <kuniyu@...zon.com>
To: <dmantipov@...dex.ru>
CC: <dccp@...r.kernel.org>, <kuniyu@...zon.com>, <netdev@...r.kernel.org>,
	<syzbot+554ccde221001ab5479a@...kaller.appspotmail.com>
Subject: Re: KASAN: use-after-free Read in ccid2_hc_tx_packet_recv

From: Dmitry Antipov <dmantipov@...dex.ru>
Date: Wed, 23 Oct 2024 15:09:59 +0300
> 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.

I wouldn't touch DCCP anymore unless the change is required for TCP.
(see b144fcaf46d43)