From: Dmitry Antipov <dmantipov@xxxxxxxxx> 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)
