[PATCH] DCCP: Fix to handle invalid packets in REQUEST state

[PATCH] DCCP: Fix to handle invalid packets in REQUEST state

[Wei Yongjun]

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

Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>
---
 net/dccp/input.c |   21 ++++++++++++---------
 1 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/net/dccp/input.c b/net/dccp/input.c
index 0beb29e..7d7b4f7
--- a/net/dccp/input.c
+++ b/net/dccp/input.c
@@ -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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] DCCP: Fix to handle invalid packets in REQUEST state

[Gerrit Renker]

[-- Attachment #1: Type: text/plain, Size: 4450 bytes --]

Wei,

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

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

> 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=c077e000 task=c0700340 task.ti=c0734000)
> 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 `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.



[-- Attachment #2: sketch_for_retransmitted_request_bug.diff --]
[-- Type: text/x-diff, Size: 1058 bytes --]

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

Re: [PATCH] DCCP: Fix to handle invalid packets in REQUEST state

[Wei Yongjun]

Gerrit Renker wrote:
> Wei,
>
> thank you for reporting this. There is a differentiation here.
>
>   
>> 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)
>>
>>     
> 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.
>   

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?

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

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

Re: [PATCH] DCCP: Fix to handle invalid packets in REQUEST state

[Eddie Kohler]

Wei,

Wei Yongjun wrote:
>>> 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)
>>>
>>>     
>> 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.
> 
> 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?

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

Re: [PATCH] DCCP: Fix to handle invalid packets in REQUEST state

[Gerrit Renker]

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

Re: [PATCH] DCCP: Fix to handle invalid packets in REQUEST state

[Wei Yongjun]

Gerrit Renker wrote:
> | >> 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\x10, GSR=1)
Request        ---------------->
(SEQ\x10000)
               <---------------   Sync
                                  (SEQ\x11, ACK\x10000)
Reset          ---------------->  
(SEQ\x10001, ACK\x11)
               <---------------   Sync
                                  (SEQ\x12, ACK=1)
DATA          ---------------->  
(SEQ=2, ACK\x12)
              <---------------   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.

Re: [PATCH] DCCP: Fix to handle invalid packets in REQUEST state

[Gerrit Renker]

> Yes, it does, the last Reset (no connection) will cause client entering  
> TIMEWAIT state. Your patch is correct.
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

net-2.6 [BUG-FIX] [PATCH 1/1] dccp: Fix panic in retransmission mechanism

[Gerrit Renker]

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.

Signed-off-by: Gerrit Renker <gerrit@erg.abdn.ac.uk>
---
 net/dccp/input.c |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

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

Re: net-2.6 [BUG-FIX] [PATCH 1/1] dccp: Fix panic in retransmission mechanism

[David Miller]

From: Gerrit Renker <gerrit@erg.abdn.ac.uk>
Date: Mon, 18 Aug 2008 18:00:29 +0200

> can you please consider this net-2.6 bug fix - this fixes a panic()
> triggered for some packet exchanges.
> 
> ----------------------------> Patch <-----------------------------------
> dccp: Fix panic caused by too early termination of retransmission mechanism

Applied, thanks Gerrit.