[net-2.6 PATCH 0/6] dccp: ICMP bug fixes by Wei Yongjun

[net-2.6 PATCH 0/6] dccp: ICMP bug fixes by Wei Yongjun

[Gerrit Renker]

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.	

[PATCH 1/6] dccp: Allow to distinguish original and retransmitted packets

[Gerrit Renker]

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.

Signed-off-by: Gerrit Renker <gerrit@erg.abdn.ac.uk>
---
 net/dccp/dccp.h   |    2 +-
 net/dccp/output.c |   20 ++++++++++++++++----
 net/dccp/timer.c  |   20 ++++----------------
 3 files changed, 21 insertions(+), 21 deletions(-)

--- a/net/dccp/dccp.h
+++ b/net/dccp/dccp.h
@@ -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);
--- a/net/dccp/output.c
+++ b/net/dccp/output.c
@@ -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,
--- a/net/dccp/timer.c
+++ b/net/dccp/timer.c
@@ -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)

[PATCH 2/6] dccp: Bug-Fix - AWL was never updated

[Gerrit Renker]

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.

Signed-off-by: Gerrit Renker <gerrit@erg.abdn.ac.uk>
Acked-by: Ian McDonald <ian.mcdonald@jandi.co.nz>
---
 net/dccp/output.c |   33 +++++++++++++++------------------
 1 files changed, 15 insertions(+), 18 deletions(-)

--- a/net/dccp/output.c
+++ b/net/dccp/output.c
@@ -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;

[PATCH 3/6] dccp: Fix sequence number check for ICMPv4 packets

[Gerrit Renker]

From: Wei Yongjun <yjwei@cn.fujitsu.com>

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)

Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>
Acked-by: Gerrit Renker <gerrit@erg.abdn.ac.uk>
---
 net/dccp/ipv4.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

--- a/net/dccp/ipv4.c
+++ b/net/dccp/ipv4.c
@@ -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;
 	}

[PATCH 4/6] dccp: Add check for sequence number in ICMPv6 message

[Gerrit Renker]

From: Wei Yongjun <yjwei@cn.fujitsu.com>

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.

Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>
Acked-by: Gerrit Renker <gerrit@erg.abdn.ac.uk>
---
 net/dccp/ipv6.c |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

--- a/net/dccp/ipv6.c
+++ b/net/dccp/ipv6.c
@@ -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;

[PATCH 5/6] dccp: Fix incorrect length check for ICMPv4 packets

[Gerrit Renker]

From: Wei Yongjun <yjwei@cn.fujitsu.com>

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).

Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>
Acked-by: Gerrit Renker <gerrit@erg.abdn.ac.uk>
---
 net/dccp/ipv4.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

--- a/net/dccp/ipv4.c
+++ b/net/dccp/ipv4.c
@@ -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;
 	}

[PATCH 6/6] dccp: Add check for truncated ICMPv6 DCCP error packets

[Gerrit Renker]

From: Wei Yongjun <yjwei@cn.fujitsu.com>

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.

Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>
Acked-by: Gerrit Renker <gerrit@erg.abdn.ac.uk>
---
 net/dccp/ipv6.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

--- a/net/dccp/ipv6.c
+++ b/net/dccp/ipv6.c
@@ -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));

Re: [PATCH 5/6] dccp: Fix incorrect length check for ICMPv4 packets

[David Miller]

From: Gerrit Renker <gerrit@erg.abdn.ac.uk>
Date: Fri, 25 Jul 2008 10:02:57 +0100

> --- a/net/dccp/ipv4.c
> +++ b/net/dccp/ipv4.c
> @@ -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;
>  	}

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.

The ipv6 patch has the same bug.

The test probably needs to be something like:

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

in order to make the tests safely.

Re: [net-2.6 PATCH 0/6] dccp: ICMP bug fixes by Wei Yongjun

[David Miller]

From: Gerrit Renker <gerrit@erg.abdn.ac.uk>
Date: Fri, 25 Jul 2008 10:02:52 +0100

> 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 

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

Thanks.

Re: [PATCH 5/6] dccp: Fix incorrect length check for ICMPv4 packets

[Gerrit Renker]

| > -	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

v2 [net-2.6 PATCH 0-4/7] dccp: Revised ICMP bug fixes

[Gerrit Renker]

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       

[PATCH 5/7] dccp: Pulling 12 bytes is necessary but not sufficient

[Gerrit Renker]

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.

Signed-off-by: Gerrit Renker <gerrit@erg.abdn.ac.uk>
---
 include/linux/dccp.h |   12 ++++++++++++
 net/dccp/ipv4.c      |    5 ++---
 2 files changed, 14 insertions(+), 3 deletions(-)

--- a/include/linux/dccp.h
+++ b/include/linux/dccp.h
@@ -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);
--- a/net/dccp/ipv4.c
+++ b/net/dccp/ipv4.c
@@ -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;
 	}
 

v2 [PATCH 6/7] dccp: Fix incorrect length check for ICMPv4 packets

[Gerrit Renker]

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.

Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>
Signed-off-by: Gerrit Renker <gerrit@erg.abdn.ac.uk>
---
 net/dccp/ipv4.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

--- a/net/dccp/ipv4.c
+++ b/net/dccp/ipv4.c
@@ -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;
 	}

v2 [PATCH 7/7] dccp: Add check for truncated ICMPv6 DCCP error packets

[Gerrit Renker]

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.

Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>
Signed-off-by: Gerrit Renker <gerrit@erg.abdn.ac.uk>
---
 net/dccp/ipv6.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

--- a/net/dccp/ipv6.c
+++ b/net/dccp/ipv6.c
@@ -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));

[pull-request] [net-2.6 PATCH 0/6] dccp: Revised ICMP / length fixes

[Gerrit Renker]

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

Re: [pull-request] [net-2.6 PATCH 0/6] dccp: Revised ICMP / length fixes

[David Miller]

From: Gerrit Renker <gerrit@erg.abdn.ac.uk>
Date: Sat, 26 Jul 2008 12:22:33 +0100

> 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')

Pulled, thanks Gerrit.

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

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