DCCP: Fix to handle invalid packets in REQUEST state

This patch fix two problem in REQUEST state while received invalid packets.

1. If invalid Dccp-DataAck packets is received in the REQUEST state, the socket just send back Dccp-Rest with invalid packet error, but the socket is not reset, Dccp-Request will be continue retranmitted. The procedure is like this:

Endpoint A Endpint B

         <-----------------  Request
DataAck  ----------------->  
         <-----------------  Reset (invalid packet)
         <-----------------  Request (retranmit)

2. If sequence-invalid Dccp-Response is received in the REQUEST state, kernel will panic. This is because that after send reset when received sequence-invalid Dccp-Response, the state of socket not be changed. The procedure is like this:

Endpoint A Endpint B

         <-----------------  Request
Response ----------------->  
(sequence-invalid)
         <-----------------  Reset (invalid packet)

Response -----------------> kernel panic

Pid: 0, comm: swapper Not tainted (2.6.26 #1)
EIP: 0060:[<c05b426d>] EFLAGS: 00010282 CPU: 0
EIP is at skb_release_all+0x6/0x7e
EAX: 00000000 EBX: 00000000 ECX: 00000046 EDX: c4fdd000
ESI: c79aad80 EDI: c4fdd000 EBP: c077ee0c ESP: c077ee08
DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0068
Process swapper (pid: 0, ti�77e000 task�700340 task.ti�734000)

Stack: 00000000 c077ee18 c05b3bf3 c79a9234 c077ee58 c8ab1db5 c79aad80 c4fdd000
       02000015 c79aada0 00000c3c 00000854 c79aad80 c077ee50 c04260db c8ac176e
       0059b20e c4fdd000 c79aad80 c4fdd000 c077ee78 c8ac06e3 0000001c c79a9234
Call Trace:
 [<c05b3bf3>] ? __kfree_skb+0xb/0x66
 [<c8ab1db5>] ? dccp_rcv_state_process+0x195/0x4b0 [dccp]
 [<c04260db>] ? printk+0x15/0x17
 [<c8ac06e3>] ? dccp_v4_do_rcv+0xed/0x112 [dccp_ipv4]
 [<c05c257d>] ? sk_filter+0x66/0x6d
 [<c05b23bf>] ? sk_receive_skb+0x32/0x7c
 [<c8ac14d4>] ? dccp_v4_rcv+0x3b5/0x42d [dccp_ipv4]
 [<c05d400a>] ? ip_local_deliver_finish+0xb5/0x151
 [<c05d43f3>] ? ip_local_deliver+0x61/0x6a
 [<c05d3f36>] ? ip_rcv_finish+0x28e/0x2ad
 [<c05d4368>] ? ip_rcv+0x1f2/0x21c
 [<c05b8599>] ? netif_receive_skb+0x2ea/0x352
 [<c88cea9b>] ? pcnet32_poll+0x32f/0x689 [pcnet32]
 [<c0404257>] ? common_interrupt+0x23/0x28
 [<c05ba0f1>] ? net_rx_action+0x8f/0x146
 [<c0429a95>] ? __do_softirq+0x64/0xcd
 [<c0429a31>] ? __do_softirq+0x0/0xcd
 [<c0405788>] ? do_softirq+0x5c/0x92
 [<c0429a2f>] ? irq_exit+0x38/0x3a
 [<c0412eda>] ? smp_apic_timer_interrupt+0x71/0x7f
 [<c0404344>] ? apic_timer_interrupt+0x28/0x30
 [<c0408588>] ? default_idle+0x2d/0x42
 [<c040257d>] ? cpu_idle+0xab/0xc3
 [<c0614ba2>] ? rest_init+0x4e/0x50
 ===========

---
 net/dccp/input.c |   21 ++++++++++++---------
 1 files changed, 12 insertions(+), 9 deletions(-)

index 0beb29e..7d7b4f7
@@ -388,6 +388,14 @@ static int dccp_rcv_request_sent_state_process(struct sock *sk,
 					       const struct dccp_hdr *dh,
 					       const unsigned len)
 {
+	const struct inet_connection_sock *icsk = inet_csk(sk);
+
+	/* Stop the REQUEST timer */
+	inet_csk_clear_xmit_timer(sk, ICSK_TIME_RETRANS);
+	BUG_TRAP(sk->sk_send_head != NULL);
+	__kfree_skb(sk->sk_send_head);
+	sk->sk_send_head = NULL;
+
 	/*
 	 *  Step 4: Prepare sequence numbers in REQUEST
 	 *     If S.state = REQUEST,

@@ -400,16 +408,9 @@ static int dccp_rcv_request_sent_state_process(struct sock *sk,
 	 *		processing continues in Step 9 * /
 	*/
 	if (dh->dccph_type = DCCP_PKT_RESPONSE) {
-		const struct inet_connection_sock *icsk = inet_csk(sk);
 		struct dccp_sock *dp = dccp_sk(sk);
 		long tstamp = dccp_timestamp();
 
-		/* Stop the REQUEST timer */
-		inet_csk_clear_xmit_timer(sk, ICSK_TIME_RETRANS);
-		BUG_TRAP(sk->sk_send_head != NULL);
-		__kfree_skb(sk->sk_send_head);
-		sk->sk_send_head = NULL;
-
 		if (!between48(DCCP_SKB_CB(skb)->dccpd_ack_seq,
 			       dp->dccps_awl, dp->dccps_awh)) {
 			dccp_pr_debug("invalid ackno: S.AWL=%llu, "

@@ -426,7 +427,7 @@ static int dccp_rcv_request_sent_state_process(struct sock *sk,
 		 * the option type and is set in dccp_parse_options().
 		 */
 		if (dccp_parse_options(sk, NULL, skb))
-			return 1;
+			goto out_err;
 
 		/* Obtain usec RTT sample from SYN exchange (used by CCID 3) */
 		if (likely(dp->dccps_options_received.dccpor_timestamp_echo))

@@ -503,10 +504,12 @@ static int dccp_rcv_request_sent_state_process(struct sock *sk,
 out_invalid_packet:
 	/* dccp_v4_do_rcv will send a reset */
 	DCCP_SKB_CB(skb)->dccpd_reset_code = DCCP_RESET_CODE_PACKET_ERROR;
-	return 1;
+	goto out_err;
 
 unable_to_proceed:
 	DCCP_SKB_CB(skb)->dccpd_reset_code = DCCP_RESET_CODE_ABORTED;
+
+out_err:
 	/*
 	 * We mark this socket as no longer usable, so that the loop in
 	 * dccp_sendmsg() terminates and the application gets notified.

1.5.3.8



--
To unsubscribe from this list: send the line "unsubscribe dccp" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Wei,

thank you for reporting this. There is a differentiation here.

This is not a bug but as far as I know correct behaviour. The DataAck could be from a previous incarnation of the same connection, or it could be an erratic packet. RFC 4340, 5.6 says about this error code
"Packet Error"
A valid packet arrived with unexpected type. For example, a DCCP-Data packet with valid header checksum and sequence numbers arrived at a connection in the REQUEST state.

The client keeps on retransmitting the Request since this is a requirement from 8.1.3; the number of retransmissions is limited by net.dccp.default.request_retries.

Hence this case is in principle correct.

<snip>
This is clearly a bug. From your analysis above and from looking at the code the cause is in all likelihood:
1. sequence-invalid Response arrives;
2. retransmit timer is stopped and sk->sk_send_head is freed;
3. Reset (Packet Error) is sent as per the above diagram;
4. Next Response (valid or invalid) arrives
5. dccp_rcv_request_sent_state_process tries to __kfree_skb() the sk_send_head (which had already been freed in (2))
==> panic

In your approach the socket is closed for each error case. This seems to work, but there is a problem with it.

First, invalid or unexpected packets may arrive (as in the first scenario). It is even possible that for example a DataAck stems from a previous incarnation of the same connection. Closing the socket here will kill the new connection.

Secondly, it also opens the door to an easy denial-of-service attack:
all an attacker needs to do is to send an unexpected packet type to the client in state REQUEST, whereupon the client closes its socket.

The `unable_to_proceed' jump label was intended for the following (single) purpose: if feature negotiation fails between endpoints (e.g. if a CCID-3 only sender tries to connect to a CCID-2 only receiver).

Thus, I would like to suggest a different strategy: rather than closing the socket whenever a (valid or invalid) packet is received in the client-state REQUEST(ING), to move the chunk of code which
* stops the retransmit timer for the DCCP-Request and
* frees the sk_send_head containing the original skb
further below, after the error checking has been done.

This will ensure that this code is only executed when a valid Response arrives. If no valid Response arrives within the time budget set by the request_retries, the retransmit timer and send_head will eventually be cleared via the timer code (dccp_write_err()).

I attach a sketch which I think will fix the bug. Could you give this a try please? During the next two weeks it will be difficult for me to do actual testing (writing this while travelling), but I will do my best to respond.

@@ -404,12 +404,6 @@ static int dccp_rcv_request_sent_state_process(struct sock *sk,
 		struct dccp_sock *dp = dccp_sk(sk);
 		long tstamp = dccp_timestamp();
 
-		/* Stop the REQUEST timer */
-		inet_csk_clear_xmit_timer(sk, ICSK_TIME_RETRANS);
-		WARN_ON(sk->sk_send_head == NULL);
-		__kfree_skb(sk->sk_send_head);
-		sk->sk_send_head = NULL;
-
 		if (!between48(DCCP_SKB_CB(skb)->dccpd_ack_seq,
 			       dp->dccps_awl, dp->dccps_awh)) {
 			dccp_pr_debug("invalid ackno: S.AWL=%llu, "

@@ -428,6 +422,12 @@ static int dccp_rcv_request_sent_state_process(struct sock *sk,
 		if (dccp_parse_options(sk, NULL, skb))
 			return 1;
 
+		/* Stop the REQUEST timer */
+		inet_csk_clear_xmit_timer(sk, ICSK_TIME_RETRANS);
+		WARN_ON(sk->sk_send_head == NULL);
+		__kfree_skb(sk->sk_send_head);
+		sk->sk_send_head = NULL;
+
 		/* Obtain usec RTT sample from SYN exchange (used by CCID 3) */
 		if (likely(dp->dccps_options_received.dccpor_timestamp_echo))
 			dp->dccps_syn_rtt = dccp_sample_rtt(sk, 10 * (tstamp -

This case is correct. 7.5.6 has an final example demonstrates recovery from a half-open connection. The difference is that Dccp-Sync is received in REQUEST state.
This make me confusion. When to sent reset not close the connection, when to sent reset and need close the connection?

The problem still exists if we not close the client's socket. See the following:

The Client The Server

                                  (OPEN)
Request        ---------------->
               <---------------   Sync, DataAck etc.
Reset          ---------------->  enter TIMEWAIT state (8.5, Step 9)
Request        ---------------->  
               <----------------  Reset (no connection)

So I think if you'd like to retransmit Request and let Server to send Response, you should change the server not to enter TIMEWAIT state, just enter CLOSED state, this break the 8.5, Step 9.

Wei,

A sequence-valid Reset would close the connection. The easiest way to see this based on the specification (as opposed to the Linux code) is to follow the pseudocode in Section 8.5 of RFC4340. This pseudocode also makes clear that on some error conditions that generate a Reset, the socket should not be closed.

Eddie

| >> 2. If sequence-invalid Dccp-Response is received in the REQUEST state,  
| >> kernel will panic. This is because that after send reset when received  
| >> sequence-invalid Dccp-Response, the state of socket not be changed. The  
| >> procedure is like this:
| >>
| >> Endpoint A                   Endpint B
| >>         <-----------------  Request
| >> Response ----------------->  (sequence-invalid)
| >>         <-----------------  Reset (invalid packet)
| >> Response ----------------->  kernel panic
| >>
| >> Pid: 0, comm: swapper Not tainted (2.6.26 #1)
| >> EIP: 0060:[<c05b426d>] EFLAGS: 00010282 CPU: 0
| >> EIP is at skb_release_all+0x6/0x7e
| >> EAX: 00000000 EBX: 00000000 ECX: 00000046 EDX: c4fdd000
| >> ESI: c79aad80 EDI: c4fdd000 EBP: c077ee0c ESP: c077ee08
| >> DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0068
| >> Process swapper (pid: 0, tiÀ77e000 taskÀ700340 task.tiÀ734000)
| >> Stack: 00000000 c077ee18 c05b3bf3 c79a9234 c077ee58 c8ab1db5 c79aad80 c4fdd000
| >>       02000015 c79aada0 00000c3c 00000854 c79aad80 c077ee50 c04260db c8ac176e
| >>       0059b20e c4fdd000 c79aad80 c4fdd000 c077ee78 c8ac06e3 0000001c c79a9234
| >> Call Trace:
| >> [<c05b3bf3>] ? __kfree_skb+0xb/0x66
| >>     
| > <snip>
| > This is clearly a bug. From your analysis above and from looking at the code
| > the cause is in all likelihood:
| >  1. sequence-invalid Response arrives;
| >  2. retransmit timer is stopped and sk->sk_send_head is freed;
| >  3. Reset (Packet Error) is sent as per the above diagram;
| >  4. Next Response (valid or invalid) arrives
| >  5. dccp_rcv_request_sent_state_process tries to __kfree_skb() the
| >     sk_send_head (which had already been freed in (2))
| >     => panic
| >
| > In your approach the socket is closed for each error case. This seems to
| > work, but there is a problem with it. 
| >
| > First, invalid or unexpected packets may arrive (as in the first
| > scenario). It is even possible that for example a DataAck stems from a
| > previous incarnation of the same connection. Closing the socket here
| > will kill the new connection.
| >
| > Secondly, it also opens the door to an easy denial-of-service attack:
| > all an attacker needs to do is to send an unexpected packet type to the
| > client in state REQUEST, whereupon the client closes its socket.
| >   
| 
| The problem still exists if we not close the client's socket. See the
| following:
| 
| The Client                        The Server
|                                   (OPEN)
| Request        ---------------->
|                <---------------   Sync, DataAck etc.
| Reset          ---------------->  enter TIMEWAIT state (8.5, Step 9)
| Request        ---------------->  
|                <----------------  Reset (no connection)
| 
| 
| So I think if you'd like to retransmit Request and let Server to send
| Response, you should change the server not to enter TIMEWAIT state, just
| enter CLOSED state, this break the 8.5, Step 9.
| 

From looking through the code, a sequence-valide Reset packet will close the Client socket, via the following steps:
* packet is received via dccp_v{4,6}_do_rcv (dccp_child_process is not relevant here, since we are interested in the client);
* since the state is not LISTEN, control proceeds to
* dccp_rcv_state_process and from there to
* dccp_rcv_reset, which
- sets the state to TIMEWAIT,
- shuts the socket for both sending and receiving,
- sets the SOCK_DEAD flag.

In accordance with the note marked with the asterisk underneath the table in 7.5.3, no sequence number check is performed for the Reset packet, and an acknowledgment-window check is not applicable.

From this analysis, I think that the socket will be closed, with the client entering TIMEWAIT state as the last step.

However, this is by just looking through, I have not tested if this holds in practice as expected (it should).

Yes, it does, the last Reset (no connection) will cause client entering TIMEWAIT state. Your patch is correct.

BTW: The other interesting thing is that maybe the following testcase can be used for attack the sequence num:

The Client The Server

                                  (OPEN) (GSS, GSR=1)
Request        ---------------->
(SEQ000)
               <---------------   Sync
                                  (SEQ, ACK000)
Reset          ---------------->  
(SEQ001, ACK)
               <---------------   Sync
                                  (SEQ, ACK=1)
DATA          ---------------->  
(SEQ=2, ACK)
              <---------------   ACK
                 ..........

In this time, the client will know both the sever and the client's sequence num, this maybe used for attack I think.
If it is a attack, so the best way to avoid this is not sent Dccp-Sync after received the sequence-invalid reset.

Regards.

Dave,

can you please consider this net-2.6 bug fix - this fixes a panic() triggered for some packet exchanges.

Many thanks,
Gerrit

----------------------------> Patch <-----------------------------------
dccp: Fix panic caused by too early termination of retransmission mechanism

Thanks is due to Wei Yongjun for the detailed analysis and description of this bug at http://marc.info/?l=dccp&m=121739364909199&w=2

The problem is that invalid packets received by a client in state REQUEST cause the retransmission timer for the DCCP-Request to be reset. This includes freeing the Request-skb ( in dccp_rcv_request_sent_state_process() ). As a consequence,
* the arrival of further packets cause a double-free, triggering a panic(),
* the connection then may hang, since further retransmissions are blocked.

This patch changes the order of statements so that the retransmission timer is reset, and the pending Request freed, only if a valid Response has arrived (or the number of sysctl-retries has been exhausted).

Further changes:
----------------
To be on the safe side, replaced __kfree_skb with kfree_skb so that if due to unexpected circumstances the sk_send_head is NULL the WARN_ON is used instead.

---
 net/dccp/input.c |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

@@ -411,12 +411,6 @@ static int dccp_rcv_request_sent_state_p
 		struct dccp_sock *dp = dccp_sk(sk);
 		long tstamp = dccp_timestamp();
 
-		/* Stop the REQUEST timer */
-		inet_csk_clear_xmit_timer(sk, ICSK_TIME_RETRANS);
-		WARN_ON(sk->sk_send_head == NULL);
-		__kfree_skb(sk->sk_send_head);
-		sk->sk_send_head = NULL;
-
 		if (!between48(DCCP_SKB_CB(skb)->dccpd_ack_seq,
 			       dp->dccps_awl, dp->dccps_awh)) {
 			dccp_pr_debug("invalid ackno: S.AWL=%llu, "

@@ -441,6 +435,12 @@ static int dccp_rcv_request_sent_state_p
 				    DCCP_ACKVEC_STATE_RECEIVED))
 			goto out_invalid_packet; /* FIXME: change error code */
 
+		/* Stop the REQUEST timer */
+		inet_csk_clear_xmit_timer(sk, ICSK_TIME_RETRANS);
+		WARN_ON(sk->sk_send_head == NULL);
+		kfree_skb(sk->sk_send_head);
+		sk->sk_send_head = NULL;
+
 		dp->dccps_isr = DCCP_SKB_CB(skb)->dccpd_seq;
 		dccp_update_gsr(sk, dp->dccps_isr);
 		/*

From: Gerrit Renker <[email protected]>
Date: Mon, 18 Aug 2008 18:00:29 +0200

Applied, thanks Gerrit.

Thanks a lot for testing, this is much appreciated. I have put some more work in and reviewed the concept. I will submit the patch and update the test tree.

Thanks again
Gerrit