Home » Mailing list archives » linux-netdev » 2008 » March » 27

Re: [PATCH] SCTP: Fix Protocol violation when receiving a error length INIT ACK

Previous thread: [PATCH] 2.6.25-rc6 tulip_read_eeprom fixes for BUG 4420 by Grant Grundler on Monday, March 24, 2008 - 1:23 am. (2 messages)

Next thread: net GIT tree status by David Miller on Monday, March 24, 2008 - 2:13 am. (1 message)
[view original message]
From: Gui Jianfeng <guijianfeng@...>
To: vladislav <vladislav.yasevich@...>
Cc: netdev <netdev@...>, lksctp-dev <lksctp-developers@...>, David Miller <davem@...>
Subject: [PATCH] SCTP: Fix Protocol violation when receiving a error length INIT ACK
Date: Monday, March 24, 2008 - 1:39 am

Hi Vlad,

When kernel receives a INIT ACK which has an invalid length, it replies a 0 VerificationTag ABORT.

This violates sctp protocol apparently, and doesn't comply to RFC requirement. VerificationTag

is allowed to set to 0 only in INIT Chunk packet.

We need to record the VerificationTag from INIT ACK before sending out the ABORT Chunk.


Here is a patch for fixing this bug.


Signed-off-by: Guijianfeng <guijianfeng@cn.fujitsu.com>

---

 include/net/sctp/command.h |    1 +

 net/sctp/sm_sideeffect.c   |    5 ++++-

 net/sctp/sm_statefuns.c    |    9 +++++++++

 3 files changed, 14 insertions(+), 1 deletions(-)


diff --git a/include/net/sctp/command.h b/include/net/sctp/command.h

index 10ae2da..35b1e83 100644

--- a/include/net/sctp/command.h

+++ b/include/net/sctp/command.h

@@ -104,6 +104,7 @@ typedef enum {

 	SCTP_CMD_ADAPTATION_IND, /* generate and send adaptation event */

 	SCTP_CMD_ASSOC_SHKEY,    /* generate the association shared keys */

 	SCTP_CMD_T1_RETRAN,	 /* Mark for retransmission after T1 timeout  */

+	SCTP_CMD_UPDATE_INITTAG, /* Update peer inittag */

 	SCTP_CMD_LAST

 } sctp_verb_t;


diff --git a/net/sctp/sm_sideeffect.c b/net/sctp/sm_sideeffect.c

index 28eb38e..2dbc7bd 100644

--- a/net/sctp/sm_sideeffect.c

+++ b/net/sctp/sm_sideeffect.c

@@ -1536,7 +1536,10 @@ static int sctp_cmd_interpreter(sctp_event_t event_type,

 			error = sctp_auth_asoc_init_active_key(asoc,

 						GFP_ATOMIC);

 			break;

-

+		case SCTP_CMD_UPDATE_INITTAG:

+			asoc->peer.i.init_tag = cmd->obj.u32;

+			break;

+

 		default:

 			printk(KERN_WARNING "Impossible command: %u, %p\n",

 			       cmd->verb, cmd->obj.ptr);

diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c

index f2ed647..1bc2c49 100644

--- a/net/sctp/sm_statefuns.c

+++ b/net/sctp/sm_statefuns.c

@@ -4144,6 +4144,15 @@ static sctp_disposition_t sctp_sf_abort_violation(

 		goto nomem;


 	if (asoc) {

+		/* Treat INIT-ACK as a special case. */

+		if (chunk->chunk_hdr->type == SCTP_CID_IN...
[ message continues ]
[view original message]
From: Wei Yongjun <yjwei@...>
To: Gui Jianfeng <guijianfeng@...>
Cc: vladislav <vladislav.yasevich@...>, netdev <netdev@...>, lksctp-dev <lksctp-developers@...>, David Miller <davem@...>
Subject: Re: [PATCH] SCTP: Fix Protocol violation when receiving a error length INIT ACK
Date: Monday, March 24, 2008 - 2:32 am

NACK.


If the INIT-ACK chunk is too short to contain the init-tag, get the

init-tag of peer may get a unexpected value.

Such as this:

  CHUNK_INIT_ACK

   Type                             = 2

   Flags                            = 0

   Length                           = 4


So I think the better way is to set T bit of ABORT chunk and used the

own's Tag.


Regards.

Wei Yongjun


--
[view original message]
From: Gui Jianfeng <guijianfeng@...>
To: Wei Yongjun <yjwei@...>, vladislav <vladislav.yasevich@...>
Cc: netdev <netdev@...>, lksctp-dev <lksctp-developers@...>, David Miller <davem@...>
Subject: Re: [PATCH] SCTP: Fix Protocol violation when receiving a error length INIT ACK
Date: Monday, March 24, 2008 - 11:33 pm

Seems reasonable. Please ignore the previous one, here is a new patch.


Signed-off-by: Gui Jianfeng <guijianfeng@cn.fujitsu.com>

---

 net/sctp/outqueue.c     |    3 +++

 net/sctp/sm_statefuns.c |    5 +++++

 2 files changed, 8 insertions(+), 0 deletions(-)


diff --git a/net/sctp/outqueue.c b/net/sctp/outqueue.c

index 1bb3c5c..c071446 100644

--- a/net/sctp/outqueue.c

+++ b/net/sctp/outqueue.c

@@ -793,6 +793,9 @@ int sctp_outq_flush(struct sctp_outq *q, int rtx_timeout)

 			break;


 		case SCTP_CID_ABORT:

+			if (sctp_test_T_bit(chunk)) {

+				packet->vtag = asoc->c.my_vtag;

+			}

 		case SCTP_CID_SACK:

 		case SCTP_CID_HEARTBEAT:

 		case SCTP_CID_HEARTBEAT_ACK:

diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c

index f2ed647..85e1d63 100644

--- a/net/sctp/sm_statefuns.c

+++ b/net/sctp/sm_statefuns.c

@@ -4144,6 +4144,11 @@ static sctp_disposition_t sctp_sf_abort_violation(

 		goto nomem;


 	if (asoc) {

+		/* Treat INIT-ACK as a special case. */

+		if (chunk->chunk_hdr->type == SCTP_CID_INIT_ACK) {

+			abort->chunk_hdr->flags |= SCTP_CHUNK_FLAG_T;

+		}

+

 		sctp_add_cmd_sf(commands, SCTP_CMD_REPLY, SCTP_CHUNK(abort));

 		SCTP_INC_STATS(SCTP_MIB_OUTCTRLCHUNKS);


--

1.5.3


--

Regards

Gui Jianfeng


--
[view original message]
From: Wei Yongjun <yjwei@...>
To: Gui Jianfeng <guijianfeng@...>
Cc: vladislav <vladislav.yasevich@...>, netdev <netdev@...>, lksctp-dev <lksctp-developers@...>, David Miller <davem@...>
Subject: Re: [PATCH] SCTP: Fix Protocol violation when receiving a error length INIT ACK
Date: Tuesday, March 25, 2008 - 12:46 am

Hi Gui:


I looked the source code and found that only in the state *COOKIE_WAIT*

received INIT-ACK need to do this, not all of the states. The other

states we should have known the init-tag of peer.


Those code should be move to sctp_make_abort

<../cgi-bin/global.cgi?pattern=sctp_make_abort&type=reference>() for

common using. And may be need check whether we need the T flags.


--
[view original message]
From: Gui Jianfeng <guijianfeng@...>
To: Wei Yongjun <yjwei@...>
Cc: vladislav <vladislav.yasevich@...>, netdev <netdev@...>, lksctp-dev <lksctp-developers@...>, David Miller <davem@...>
Subject: Re: [PATCH] SCTP: Fix Protocol violation when receiving a error length INIT ACK
Date: Tuesday, March 25, 2008 - 3:10 am

This rarely happens, so i think it's sufficient to place this block of code here.


--

Regards

Gui Jianfeng


--
[view original message]
From: Vlad Yasevich <vladislav.yasevich@...>
To: Gui Jianfeng <guijianfeng@...>
Cc: Wei Yongjun <yjwei@...>, netdev <netdev@...>, lksctp-dev <lksctp-developers@...>, David Miller <davem@...>
Subject: Re: [PATCH] SCTP: Fix Protocol violation when receiving a error length INIT ACK
Date: Tuesday, March 25, 2008 - 11:10 am

There is a side-effect to this patch that now we will completely ignore the verification

tag in the INIT-ACK regardless of the violation.


In particular, if the INIT-ACK contains all the fixed parameters but violates structure

in some variable parameters, we'll currently use the Initiate Tag from the INIT-ACK in

the ABORT.   We should not be changing this behavior.


The simple hack is to add some conditional code into the the different violation functions, but

I'd like to see if there is a cleaner way to solve this.


-vlad

--
[view original message]
From: Gui Jianfeng <guijianfeng@...>
To: Vlad Yasevich <vladislav.yasevich@...>
Cc: Wei Yongjun <yjwei@...>, netdev <netdev@...>, lksctp-dev <lksctp-developers@...>, David Miller <davem@...>
Subject: Re: [PATCH] SCTP: Fix Protocol violation when receiving a error length INIT ACK
Date: Wednesday, March 26, 2008 - 9:40 pm

Vlad,

  The problem you said has been addressed, this is a new patch. Just treat

  an INIT-ACK received during COOKIE-WAIT as a special case. this patch will

  not break the normal behavior.


Signed-off-by: Gui Jianfeng <guijianfeng@cn.fujitsu.com>

---

 include/net/sctp/command.h |    1 +

 net/sctp/outqueue.c        |    3 +++

 net/sctp/sm_sideeffect.c   |    5 ++++-

 net/sctp/sm_statefuns.c    |   18 ++++++++++++++++++

 4 files changed, 26 insertions(+), 1 deletions(-)


diff --git a/include/net/sctp/command.h b/include/net/sctp/command.h

index 10ae2da..35b1e83 100644

--- a/include/net/sctp/command.h

+++ b/include/net/sctp/command.h

@@ -104,6 +104,7 @@ typedef enum {

 	SCTP_CMD_ADAPTATION_IND, /* generate and send adaptation event */

 	SCTP_CMD_ASSOC_SHKEY,    /* generate the association shared keys */

 	SCTP_CMD_T1_RETRAN,	 /* Mark for retransmission after T1 timeout  */

+	SCTP_CMD_UPDATE_INITTAG, /* Update peer inittag */

 	SCTP_CMD_LAST

 } sctp_verb_t;


diff --git a/net/sctp/outqueue.c b/net/sctp/outqueue.c

index 1bb3c5c..c071446 100644

--- a/net/sctp/outqueue.c

+++ b/net/sctp/outqueue.c

@@ -793,6 +793,9 @@ int sctp_outq_flush(struct sctp_outq *q, int rtx_timeout)

 			break;


 		case SCTP_CID_ABORT:

+			if (sctp_test_T_bit(chunk)) {

+				packet->vtag = asoc->c.my_vtag;

+			}

 		case SCTP_CID_SACK:

 		case SCTP_CID_HEARTBEAT:

 		case SCTP_CID_HEARTBEAT_ACK:

diff --git a/net/sctp/sm_sideeffect.c b/net/sctp/sm_sideeffect.c

index 28eb38e..2dbc7bd 100644

--- a/net/sctp/sm_sideeffect.c

+++ b/net/sctp/sm_sideeffect.c

@@ -1536,7 +1536,10 @@ static int sctp_cmd_interpreter(sctp_event_t event_type,

 			error = sctp_auth_asoc_init_active_key(asoc,

 						GFP_ATOMIC);

 			break;

-

+		case SCTP_CMD_UPDATE_INITTAG:

+			asoc->peer.i.init_tag = cmd->obj.u32;

+			break;

+

 		default:

 			printk(KERN_WARNING "Impossible command: %u, %p\n",

 			       cmd->verb, cmd->obj.ptr);

diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c

in...
[ message continues ]
[view original message]
From: Vlad Yasevich <vladislav.yasevich@...>
To: Gui Jianfeng <guijianfeng@...>
Cc: Wei Yongjun <yjwei@...>, netdev <netdev@...>, lksctp-dev <lksctp-developers@...>, David Miller <davem@...>
Subject: Re: [PATCH] SCTP: Fix Protocol violation when receiving a error length INIT ACK
Date: Thursday, March 27, 2008 - 3:55 pm

^^^^^^^^^


Can you also resend with a clean patch description.


Thanks

-vlad

--
Previous thread: [PATCH] 2.6.25-rc6 tulip_read_eeprom fixes for BUG 4420 by Grant Grundler on Monday, March 24, 2008 - 1:23 am. (2 messages)

Next thread: net GIT tree status by David Miller on Monday, March 24, 2008 - 2:13 am. (1 message)