dccp: ICMP bug fixes by Wei Yongjun

Hi Dave,

can you please have a look at this set of ICMP/DCCP bug fixes by Wei Yongjun, augmented by two inter-related test tree patches that solve a related bug.

Patch #1: Provides a way to distinguish original from retransmitted packets. Patch #2: Fixes a bug - AWL was never updated. Needed by the third patch. Patch #3: Corrects ICMPv4 sequence number check to use AWL/H instead of SWL/H. Patch #4: Implements check from patch #3 for ICMPv6.
Patch #5: Fixes an insufficient-length check for

All patches have been compile-tested, the first two have been in the test tree for over at least one month.

Can you please consider pulling from

	git://eden-feed.erg.abdn.ac.uk/net-2.6		(subtree `master')

At your discretion, as none of the bugs is severe,

git://eden-feed.erg.abdn.ac.uk/net-next-2.6 (subtree `master')

contains the same set of patches. Both trees have been freshly cloned today.

This patch allows the sender to distinguish original and retransmitted packets. That is in particular needed for the retransmission of Request packets, since
* the first packet uses ISS (generated in net/dccp/ipv?.c), and sets GSS = ISS;
* all retransmitted packets use GSS' = GSS + 1, so that the n-th retransmitted packet has sequence number ISS + n (mod 48).

To add this support, the patch reorganises existing code in such a manner that
* icsk_retransmits = 0 for the original packet and
* icsk_retransmits = n > 0 for the n-th retransmitted packet
at the time dccp_transmit_skb() is called via dccp_retransmit_skb().

This patch is thanks to Wei Yongjun, who pointed this problem out, and to Arnaldo for further discussion.

Further changes:
----------------
* exploits that, since sk_send_head always contains the original skb (enqueued by dccp_entail()), skb_cloned() never evaluated to true;
* removed the `skb' argument from dccp_retransmit_skb(), since sk_send_head is used for all retransmissions (the exception is client-Acks in PARTOPEN state, but these are not put onto the sk_send_head);
* updated documentation.

---
 net/dccp/dccp.h   |    2 +-
 net/dccp/output.c |   20 ++++++++++++++++----
 net/dccp/timer.c  |   20 ++++----------------
 3 files changed, 21 insertions(+), 21 deletions(-)

@@ -226,7 +226,7 @@ static inline void dccp_csum_outgoing(struct sk_buff *skb)
 
 extern void dccp_v4_send_check(struct sock *sk, int len, struct sk_buff *skb);
 
-extern int  dccp_retransmit_skb(struct sock *sk, struct sk_buff *skb);
+extern int  dccp_retransmit_skb(struct sock *sk);
 
 extern void dccp_send_ack(struct sock *sk);
 extern void dccp_reqsk_send_ack(struct sk_buff *sk, struct request_sock *rsk);

@@ -284,14 +284,26 @@ void dccp_write_xmit(struct sock *sk, int block)
 	}
 }
 
-int dccp_retransmit_skb(struct sock *sk, struct sk_buff *skb)
+/**
+ * dccp_retransmit_skb  -  Retransmit Request, Close, or CloseReq packets
+ * There are only four retransmittable packet types in DCCP:
+ * - Request  in client-REQUEST  state (sec. 8.1.1),
+ * - CloseReq in server-CLOSEREQ state (sec. 8.3),
+ * - Close    in   node-CLOSING  state (sec. 8.3),
+ * - Acks in client-PARTOPEN state (sec. 8.1.5, handled by dccp_delack_timer()).
+ * This function expects sk->sk_send_head to contain the original skb.
+ */
+int dccp_retransmit_skb(struct sock *sk)
 {
+	BUG_TRAP(sk->sk_send_head != NULL);
+
 	if (inet_csk(sk)->icsk_af_ops->rebuild_header(sk) != 0)
 		return -EHOSTUNREACH; /* Routing failure or similar. */
 
-	return dccp_transmit_skb(sk, (skb_cloned(skb) ?
-				      pskb_copy(skb, GFP_ATOMIC):
-				      skb_clone(skb, GFP_ATOMIC)));
+	/* this count is used to distinguish original and retransmitted skb */
+	inet_csk(sk)->icsk_retransmits++;
+
+	return dccp_transmit_skb(sk, skb_clone(sk->sk_send_head, GFP_ATOMIC));
 }
 
 struct sk_buff *dccp_make_response(struct sock *sk, struct dst_entry *dst,

@@ -99,21 +99,11 @@ static void dccp_retransmit_timer(struct sock *sk)
 	}
 
 	/*
-	 * sk->sk_send_head has to have one skb with
-	 * DCCP_SKB_CB(skb)->dccpd_type set to one of the retransmittable DCCP
-	 * packet types. The only packets eligible for retransmission are:
-	 *	-- Requests in client-REQUEST  state (sec. 8.1.1)
-	 *	-- Acks     in client-PARTOPEN state (sec. 8.1.5)
-	 *	-- CloseReq in server-CLOSEREQ state (sec. 8.3)
-	 *	-- Close    in   node-CLOSING  state (sec. 8.3)                */
-	BUG_TRAP(sk->sk_send_head != NULL);
-
-	/*
 	 * More than than 4MSL (8 minutes) has passed, a RESET(aborted) was
 	 * sent, no need to retransmit, this sock is dead.
 	 */
 	if (dccp_write_timeout(sk))
-		goto out;
+		return;
 
 	/*
 	 * We want to know the number of packets retransmitted, not the

@@ -122,30 +112,28 @@ static void dccp_retransmit_timer(struct sock *sk)
 	if (icsk->icsk_retransmits = 0)
 		DCCP_INC_STATS_BH(DCCP_MIB_TIMEOUTS);
 
-	if (dccp_retransmit_skb(sk, sk->sk_send_head) < 0) {
+	if (dccp_retransmit_skb(sk) != 0) {
 		/*
 		 * Retransmission failed because of local congestion,
 		 * do not backoff.
 		 */
-		if (icsk->icsk_retransmits = 0)
+		if (--icsk->icsk_retransmits = 0)
 			icsk->icsk_retransmits = 1;
 		inet_csk_reset_xmit_timer(sk, ICSK_TIME_RETRANS,
 					  min(icsk->icsk_rto,
 					      TCP_RESOURCE_PROBE_INTERVAL),
 					  DCCP_RTO_MAX);
-		goto out;
+		return;
 	}
 
 backoff:
 	icsk->icsk_backoff++;
-	icsk->icsk_retransmits++;
 
 	icsk->icsk_rto = min(icsk->icsk_rto << 1, DCCP_RTO_MAX);
 	inet_csk_reset_xmit_timer(sk, ICSK_TIME_RETRANS, icsk->icsk_rto,
 				  DCCP_RTO_MAX);
 	if (icsk->icsk_retransmits > sysctl_dccp_retries1)
 		__sk_dst_reset(sk);
-out:;
 }
 
 static void dccp_write_timer(unsigned long data)

This patch was triggered by finding the following message in the syslog:
"kernel: dccp_check_seqno: DCCP: Step 6 failed for DATAACK packet, [...]
P.ackno exists or LAWL(82947089) <= P.ackno(82948208)

                                   <= S.AWH(82948728), sending SYNC..."

Note the difference between AWH and AWL: it is 1639 packets - the Sequence Window was actually just 100. A closer look at the trace showed that LAWL = AWL = 82947089 equalled the ISS on the Response.

The cause of the bug was that AWL was only ever set on the first packet - the DCCP-Request sent by dccp_v{4,6}_connect().

The fix is to continually update AWL/AWH with each new packet (as GSS=AWH).

Notes:
------
icsk_retransmits is used to distinguish original and retransmitted packets (introduced in previous patch).

AWL/AWH are now updated to enforce more stringent checks on the initial sequence numbers when connecting:

* AWL is initialised to ISS and remains at this value;
* AWH is always set to GSS (via dccp_update_gss());
* so on the first Request: AWL = AWH = ISS,
and on the n-th Request: AWL = ISS, AWH = ISS + n.

As a consequence, only Response packets that refer to Requests sent by this host will pass, all others are discarded. This is the intention and in effect implements the initial adjustments for AWL as specified in RFC 4340, 7.5.1.

---
 net/dccp/output.c |   33 +++++++++++++++------------------
 1 files changed, 15 insertions(+), 18 deletions(-)

@@ -53,8 +53,11 @@ static int dccp_transmit_skb(struct sock *sk, struct sk_buff *skb)
 					  dccp_packet_hdr_len(dcb->dccpd_type);
 		int err, set_ack = 1;
 		u64 ackno = dp->dccps_gsr;
-
-		dccp_inc_seqno(&dp->dccps_gss);
+		/*
+		 * Increment GSS here already in case the option code needs it.
+		 * Update GSS for real only if option processing below succeeds.
+		 */
+		dcb->dccpd_seq = ADD48(dp->dccps_gss, 1);
 
 		switch (dcb->dccpd_type) {
 		case DCCP_PKT_DATA:

@@ -66,6 +69,9 @@ static int dccp_transmit_skb(struct sock *sk, struct sk_buff *skb)
 
 		case DCCP_PKT_REQUEST:
 			set_ack = 0;
+			/* Use ISS on the first (non-retransmitted) Request. */
+			if (icsk->icsk_retransmits = 0)
+				dcb->dccpd_seq = dp->dccps_iss;
 			/* fall through */
 
 		case DCCP_PKT_SYNC:

@@ -84,8 +90,6 @@ static int dccp_transmit_skb(struct sock *sk, struct sk_buff *skb)
 			break;
 		}
 
-		dcb->dccpd_seq = dp->dccps_gss;
-
 		if (dccp_insert_options(sk, skb)) {
 			kfree_skb(skb);
 			return -EPROTO;

@@ -103,7 +107,7 @@ static int dccp_transmit_skb(struct sock *sk, struct sk_buff *skb)
 		/* XXX For now we're using only 48 bits sequence numbers */
 		dh->dccph_x	= 1;
 
-		dp->dccps_awh = dp->dccps_gss;
+		dccp_update_gss(sk, dcb->dccpd_seq);
 		dccp_hdr_set_seq(dh, dp->dccps_gss);
 		if (set_ack)
 			dccp_hdr_set_ack(dccp_hdr_ack_bits(skb), ackno);

@@ -112,6 +116,11 @@ static int dccp_transmit_skb(struct sock *sk, struct sk_buff *skb)
 		case DCCP_PKT_REQUEST:
 			dccp_hdr_request(skb)->dccph_req_service  							dp->dccps_service;
+			/*
+			 * Limit Ack window to ISS <= P.ackno <= GSS, so that
+			 * only Responses to Requests we sent are considered.
+			 */
+			dp->dccps_awl = dp->dccps_iss;
 			break;
 		case DCCP_PKT_RESET:
 			dccp_hdr_reset(skb)->dccph_reset_code @@ -449,19 +458,7 @@ static inline void dccp_connect_init(struct sock *sk)
 

dccp_sync_mss(sk, dst_mtu(dst));

- /*
- * SWL and AWL are initially adjusted so that they are not less than
- * the initial Sequence Numbers received and sent, respectively:
- * SWL := max(GSR + 1 - floor(W/4), ISR),
- * AWL := max(GSS - W' + 1, ISS).
- * These adjustments MUST be applied only at the beginning of the
- * connection.
- */
- dccp_update_gss(sk, dp->dccps_iss);
- dccp_set_seqno(&dp->dccps_awl, max48(dp->dccps_awl, dp->dccps_iss));
-
- /* S.GAR - greatest valid acknowledgement number received on a non-Sync;
- * initialized to S.ISS (sec. 8.5) */
+ /* Initialise GAR as per 8.5; AWL/AWH are set in dccp_transmit_skb() */
dp->dccps_gar = dp->dccps_iss;

icsk->icsk_retransmits = 0;

From: Wei Yongjun <[email protected]>

The payload of ICMP message is a part of the packet sent by ourself, so the sequence number check must use AWL and AWH, not SWL and SWH.

For example:

     Endpoint A                  Endpoint B

     DATA-ACK       -------->
     (SEQ=X)
                    <--------    ICMP (Fragmentation Needed)
                                 (SEQ=X)

---
 net/dccp/ipv4.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

@@ -238,7 +238,7 @@ static void dccp_v4_err(struct sk_buff *skb, u32 info)
 	dp = dccp_sk(sk);
 	seq = dccp_hdr_seq(dh);
 	if ((1 << sk->sk_state) & ~(DCCPF_REQUESTING | DCCPF_LISTEN) &&
-	    !between48(seq, dp->dccps_swl, dp->dccps_swh)) {
+	    !between48(seq, dp->dccps_awl, dp->dccps_awh)) {
 		NET_INC_STATS_BH(net, LINUX_MIB_OUTOFWINDOWICMPS);
 		goto out;
 	}

From: Wei Yongjun <[email protected]>

This adds a sequence number check for ICMPv6 DCCP error packets, in the same manner as it has been done for ICMPv4 in the previous patch.

---
 net/dccp/ipv6.c |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

@@ -89,6 +89,7 @@ static void dccp_v6_err(struct sk_buff *skb, struct inet6_skb_parm *opt,
 {
 	struct ipv6hdr *hdr = (struct ipv6hdr *)skb->data;
 	const struct dccp_hdr *dh = (struct dccp_hdr *)(skb->data + offset);
+	struct dccp_sock *dp;
 	struct ipv6_pinfo *np;
 	struct sock *sk;
 	int err;

@@ -116,6 +117,14 @@ static void dccp_v6_err(struct sk_buff *skb, struct inet6_skb_parm *opt,
 	if (sk->sk_state = DCCP_CLOSED)
 		goto out;
 
+	dp = dccp_sk(sk);
+	seq = dccp_hdr_seq(dh);
+	if ((1 << sk->sk_state) & ~(DCCPF_REQUESTING | DCCPF_LISTEN) &&
+	    !between48(seq, dp->dccps_awl, dp->dccps_awh)) {
+		NET_INC_STATS_BH(net, LINUX_MIB_OUTOFWINDOWICMPS);
+		goto out;
+	}
+
 	np = inet6_sk(sk);
 
 	if (type = ICMPV6_PKT_TOOBIG) {

@@ -168,7 +177,6 @@ static void dccp_v6_err(struct sk_buff *skb, struct inet6_skb_parm *opt,
 
 	icmpv6_err_convert(type, code, &err);
 
-	seq = dccp_hdr_seq(dh);
 	/* Might be for an request_sock */
 	switch (sk->sk_state) {
 		struct request_sock *req, **prev;

From: Wei Yongjun <[email protected]>

Unlike TCP, which only needs 8 octets of original packet data, DCCP requires minimally 12 bytes for ICMP-payload sequence number checks.

This patch replaces the insufficient length constant of 8 with a dynamic value of __dccp_basic_hdr_len(). This is safer, since it takes into account the switch between 24bit/48bit sequence numbers (`X' bit in RFC 4340, 5.1).

---
 net/dccp/ipv4.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

@@ -207,7 +207,7 @@ static void dccp_v4_err(struct sk_buff *skb, u32 info)
 	int err;
 	struct net *net = dev_net(skb->dev);
 
-	if (skb->len < (iph->ihl << 2) + 8) {
+	if (skb->len < (iph->ihl << 2) + __dccp_basic_hdr_len(dh)) {
 		ICMP_INC_STATS_BH(net, ICMP_MIB_INERRORS);
 		return;
 	}

From: Wei Yongjun <[email protected]>

This patch adds a minimum-length check for ICMPv6 packets, to allow meaningful DCCP sequence number checks, as per the previous patch for ICMPv4 payloads.

---
 net/dccp/ipv6.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

@@ -96,6 +96,11 @@ static void dccp_v6_err(struct sk_buff *skb, struct inet6_skb_parm *opt,
 	__u64 seq;
 	struct net *net = dev_net(skb->dev);
 
+	if (skb->len < offset + __dccp_basic_hdr_len(dh)) {
+		ICMP6_INC_STATS_BH(__in6_dev_get(skb->dev), ICMP6_MIB_INERRORS);
+		return;
+	}
+
 	sk = inet6_lookup(net, &dccp_hashinfo,
 			&hdr->daddr, dh->dccph_dport,
 			&hdr->saddr, dh->dccph_sport, inet6_iif(skb));

From: Gerrit Renker <[email protected]>
Date: Fri, 25 Jul 2008 10:02:57 +0100

	if (skb->len < (iph->ihl << 2) + sizeof(*dh) ||
	    skb->len < (iph->ihl << 2) + __dccp_basic_hdr_len(dh))

From: Gerrit Renker <[email protected]>
Date: Fri, 25 Jul 2008 10:02:52 +0100

As stated in my other reply, the new ICMP length checks are buggy. Please fix those up and resend this pull request.

Thanks.

| > -	if (skb->len < (iph->ihl << 2) + 8) {
| > +	if (skb->len < (iph->ihl << 2) + __dccp_basic_hdr_len(dh)) {
| >  		ICMP_INC_STATS_BH(net, ICMP_MIB_INERRORS);
| >  		return;
| >  	}
| 
| You can't dereference "dh" before you know there is even
| space past offset "iph->ihl << 2".  Yet that is what doing
| an unconditional __dccp_basic_hdr_len() call here is going
| to do.
| 

Oh that was my fault. Thanks a lot for pointing this out.

Will work out a fixed/improved version for both patches, test and then resubmit.

Gerrit

Hi Dave,

this patch set addresses the two bugs you pointed out in the first revision.

And that was very good, because the same problem loomed in another corner: the normal DCCP input receive path did the same unchecked de-referencing. This is documented in patch #5, and increases the number of bugs fixed by this changeset.

Patches 1..4 have not changed, they can be viewed online at
http://eden-feed.erg.abdn.ac.uk/cgi-bin/gitweb.cgi?p=net-2.6.git;a=summary

These patches are new or revised:
---------------------------------
Patch #5: Fixes minimum-required length check for DCCP packet input path. Patch #6: Fixes minimum-required length check for ICMPv4 embedded DCCP datagrams Patch #7: Same as patch #5, but for ICMPv6.

I have tested these on two different architectures.

As per the first revision, this changeset can be pulled both from

	git://eden-feed.erg.abdn.ac.uk/net-2.6		(subtree `master')

and from

git://eden-feed.erg.abdn.ac.uk/net-next-2.6 (subtree `master')

Thanks a lot for pointing out the bugs,
Gerrit

This patch fixes a bug in testing the minimum header length and encapsulates the fixed test into a routine for further use.

In TCP, it is safe to use pskb_may_pull(skb, sizeof(struct tcphdr)) and then assume that all header fields can be accessed.

DCCP however has a dynamic header length:

* the minimum header length is 12 bytes (required by Step 1 in RFC 4340, 8.5);
* but if the `X' bit is set within this base header, then the minimum required length is 16 bytes (__dccp_basic_hdr_len() computes that);
* so that accessing P.seqno in Step 6 of RFC 4340, 8.5 will fail.

Hence we need to first test whether 12 bytes are available, and only then can dereference the X-bit to see if (likely) 4 more bytes need to be read.

Many thanks to David Miller who pointed this problem out.

---
 include/linux/dccp.h |   12 ++++++++++++
 net/dccp/ipv4.c      |    5 ++---
 2 files changed, 14 insertions(+), 3 deletions(-)

@@ -302,6 +302,18 @@ static inline unsigned int dccp_basic_hdr_len(const struct sk_buff *skb)
 	return __dccp_basic_hdr_len(dh);
 }
 
+/*
+ * RFC 4340, 8.5. Step 1 says that 12 bytes is too short. However, to do Step 6
+ * we need at least __dccp_basic_hdr_len() (up to 16 bytes) to access P.seqno.
+ */
+static inline bool dccp_skb_too_short(struct sk_buff *skb, const u32 offset)
+{
+	const struct dccp_hdr *dh = (struct dccp_hdr *)(skb->data + offset);
+
+	return !(pskb_may_pull(skb, offset + sizeof(*dh)) &&
+		 pskb_may_pull(skb, offset + __dccp_basic_hdr_len(dh)));
+}
+
 static inline __u64 dccp_hdr_seq(const struct dccp_hdr *dh)
 {
 	__u64 seq_nr =  ntohs(dh->dccph_seq);

@@ -706,9 +706,8 @@ int dccp_invalid_packet(struct sk_buff *skb)
 	if (skb->pkt_type != PACKET_HOST)
 		return 1;
 
-	/* If the packet is shorter than 12 bytes, drop packet and return */
-	if (!pskb_may_pull(skb, sizeof(struct dccp_hdr))) {
-		DCCP_WARN("pskb_may_pull failed\n");
+	if (dccp_skb_too_short(skb, 0)) {
+		DCCP_WARN("Packet too short (%u)\n", skb->len);
 		return 1;
 	}
 

Unlike TCP, which only needs 8 octets of original packet data, DCCP requires minimally 12 or 16 bytes for ICMP-payload sequence number checks.

This patch replaces the insufficient length constant of 8 with a dynamic value, using the routine introduced previously.

---
 net/dccp/ipv4.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

@@ -196,8 +196,8 @@ static inline void dccp_do_pmtu_discovery(struct sock *sk,
 static void dccp_v4_err(struct sk_buff *skb, u32 info)
 {
 	const struct iphdr *iph = (struct iphdr *)skb->data;
-	const struct dccp_hdr *dh = (struct dccp_hdr *)(skb->data +
-							(iph->ihl << 2));
+	const u8 offset = (iph->ihl << 2);
+	const struct dccp_hdr *dh = (struct dccp_hdr *)(skb->data + offset);
 	struct dccp_sock *dp;
 	struct inet_sock *inet;
 	const int type = icmp_hdr(skb)->type;

@@ -207,7 +207,7 @@ static void dccp_v4_err(struct sk_buff *skb, u32 info)
 	int err;
 	struct net *net = dev_net(skb->dev);
 
-	if (skb->len < (iph->ihl << 2) + 8) {
+	if (dccp_skb_too_short(skb, offset)) {
 		ICMP_INC_STATS_BH(net, ICMP_MIB_INERRORS);
 		return;
 	}

This patch adds a minimum-length check for ICMPv6 packets, to allow meaningful DCCP sequence number checks, as per the previous patch for ICMPv4 payloads.

---
 net/dccp/ipv6.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

@@ -96,6 +96,11 @@ static void dccp_v6_err(struct sk_buff *skb, struct inet6_skb_parm *opt,
 	__u64 seq;
 	struct net *net = dev_net(skb->dev);
 
+	if (dccp_skb_too_short(skb, offset)) {
+		ICMP6_INC_STATS_BH(__in6_dev_get(skb->dev), ICMP6_MIB_INERRORS);
+		return;
+	}
+
 	sk = inet6_lookup(net, &dccp_hashinfo,
 			&hdr->daddr, dh->dccph_dport,
 			&hdr->saddr, dh->dccph_sport, inet6_iif(skb));

Hi Dave,

this is an update on yesterday's submission, which was unnecessarily complex.

I have checked the whole set again and looked through dccp_invalid_packet() in net/dccp/ipv4.c. There is no need for additional protection: the routine makes sure that the skb is long enough for the Data Offset (header length), which is more than the __dccp_basic_hdr_len().

The ICMPv4/6 packet length checks now in effect use the two-stage test you suggested, to ensure that the ICMP payload is long enough to access the first 12 bytes that __dccp_basic_hdr_len() dereferences.

Please let me know if individual patches should be resubmitted again. I have not done this to reduce noise; in any case the changes are also online:
http://eden-feed.erg.abdn.ac.uk/cgi-bin/gitweb.cgi?p=net-2.6.git;a=log

Patch #1: Implements support to distinguish original from retransmitted packets. Patch #2: Fixes a bug - AWL was never updated. Used by the third patch. Patch #3: Corrects ICMPv4 sequence number check to use AWL/H instead of SWL/H. Patch #4: Implements the check from patch #3 for ICMPv6.
Patch #5: Fixes minimum-required length check for ICMPv4 embedded DCCP datagrams Patch #6: Same as patch #5, but for ICMPv6.

These patches apply to net-2.6 (BUG_TRAP conversion) and can be pulled from

      git://eden-feed.erg.abdn.ac.uk/net-2.6		(subtree `master')

If necessary, I can prepare an upload for net-next-2.6 later.

Gerrit

From: Gerrit Renker <[email protected]>
Date: Sat, 26 Jul 2008 12:22:33 +0100

Pulled, thanks Gerrit.

net-next-2.6 can be ignored for at least another week or so.