[PATCH 1/2] sctp: do not enable peer features if we can't do them.

[PATCH 1/2] sctp: do not enable peer features if we can't do them.

[Vlad Yasevich]

Do not enable peer features like addip and auth, if they
are administratively disabled localy.  If the peer resports
that he supports something that we don't, neither end can
use it so enabling it is pointless.  This solves a problem
when talking to a peer that has auth and addip enabled while
we do not.  Found by Andrei Pelinescu-Onciul <andrei@iptel.org>.

Signed-off-by: Vlad Yasevich <vladislav.yasevich@hp.com>
---
 net/sctp/sm_make_chunk.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
index e8ca4e5..fe94f42 100644
--- a/net/sctp/sm_make_chunk.c
+++ b/net/sctp/sm_make_chunk.c
@@ -1886,11 +1886,13 @@ static void sctp_process_ext_param(struct sctp_association *asoc,
 			    /* if the peer reports AUTH, assume that he
 			     * supports AUTH.
 			     */
-			    asoc->peer.auth_capable = 1;
+			    if (sctp_auth_enable)
+				    asoc->peer.auth_capable = 1;
 			    break;
 		    case SCTP_CID_ASCONF:
 		    case SCTP_CID_ASCONF_ACK:
-			    asoc->peer.asconf_capable = 1;
+			    if (sctp_addip_enable)
+				    asoc->peer.asconf_capable = 1;
 			    break;
 		    default:
 			    break;
@@ -2460,6 +2462,9 @@ do_addr_param:
 		break;
 
 	case SCTP_PARAM_SET_PRIMARY:
+		if (!sctp_addip_enable)
+			goto fall_through;
+
 		addr_param = param.v + sizeof(sctp_addip_param_t);
 
 		af = sctp_get_af_specific(param_type2af(param.p->type));
-- 
1.5.3.5

[PATCH 2/2] sctp: Fix oops when INIT-ACK indicates that peer doesn't support AUTH

[Vlad Yasevich]

If INIT-ACK is received with SupportedExtensions parameter which
indicates that the peer does not support AUTH, the packet will be
silently ignore, and sctp_process_init() do cleanup all of the
transports in the association.
When T1-Init timer is expires, OOPS happen while we try to choose
a different init transport.

The solution is to only clean up the non-active transports, i.e
the ones that the peer added.  However, that introduces a problem
with sctp_connectx(), because we don't mark the proper state for
the transports provided by the user.  So, we'll simply mark
user-provided transports as ACTIVE.  That will allow INIT
retransmissions to work properly in the sctp_connectx() context
and prevent the crash.

Signed-off-by: Vlad Yasevich <vladislav.yasevich@hp.com>
---
 net/sctp/associola.c     |    9 +++++----
 net/sctp/sm_make_chunk.c |    6 ++----
 2 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/net/sctp/associola.c b/net/sctp/associola.c
index 8472b8b..abd51ce 100644
--- a/net/sctp/associola.c
+++ b/net/sctp/associola.c
@@ -599,11 +599,12 @@ struct sctp_transport *sctp_assoc_add_peer(struct sctp_association *asoc,
 	/* Check to see if this is a duplicate. */
 	peer = sctp_assoc_lookup_paddr(asoc, addr);
 	if (peer) {
+		/* An UNKNOWN state is only set on transports added by
+		 * user in sctp_connectx() call.  Such transports should be
+		 * considered CONFIRMED per RFC 4960, Section 5.4.
+		 */
 		if (peer->state == SCTP_UNKNOWN) {
-			if (peer_state == SCTP_ACTIVE)
-				peer->state = SCTP_ACTIVE;
-			if (peer_state == SCTP_UNCONFIRMED)
-				peer->state = SCTP_UNCONFIRMED;
+			peer->state = SCTP_ACTIVE;
 		}
 		return peer;
 	}
diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
index fe94f42..b599cbb 100644
--- a/net/sctp/sm_make_chunk.c
+++ b/net/sctp/sm_make_chunk.c
@@ -2321,12 +2321,10 @@ clean_up:
 	/* Release the transport structures. */
 	list_for_each_safe(pos, temp, &asoc->peer.transport_addr_list) {
 		transport = list_entry(pos, struct sctp_transport, transports);
-		list_del_init(pos);
-		sctp_transport_free(transport);
+		if (transport->state != SCTP_ACTIVE)
+			sctp_assoc_rm_peer(asoc, transport);
 	}
 
-	asoc->peer.transport_count = 0;
-
 nomem:
 	return 0;
 }
-- 
1.5.3.5

Re: [PATCH 2/2] sctp: Fix oops when INIT-ACK indicates that peer doesn't support AUTH

[David Miller]

From: Vlad Yasevich <vladislav.yasevich@hp.com>
Date: Thu, 18 Sep 2008 17:31:04 -0400

> If INIT-ACK is received with SupportedExtensions parameter which
> indicates that the peer does not support AUTH, the packet will be
> silently ignore, and sctp_process_init() do cleanup all of the
> transports in the association.
> When T1-Init timer is expires, OOPS happen while we try to choose
> a different init transport.
> 
> The solution is to only clean up the non-active transports, i.e
> the ones that the peer added.  However, that introduces a problem
> with sctp_connectx(), because we don't mark the proper state for
> the transports provided by the user.  So, we'll simply mark
> user-provided transports as ACTIVE.  That will allow INIT
> retransmissions to work properly in the sctp_connectx() context
> and prevent the crash.
> 
> Signed-off-by: Vlad Yasevich <vladislav.yasevich@hp.com>

Applied, thanks.

Re: [Lksctp-developers] [PATCH 1/2] sctp: do not enable peer features if we can't do them.

[Vlad Yasevich]

David

Can you also queue this one and Patch 2/2 to stable.  The problems
are there as well.

Thanks
-vlad

Vlad Yasevich wrote:
> Do not enable peer features like addip and auth, if they
> are administratively disabled localy.  If the peer resports
> that he supports something that we don't, neither end can
> use it so enabling it is pointless.  This solves a problem
> when talking to a peer that has auth and addip enabled while
> we do not.  Found by Andrei Pelinescu-Onciul <andrei@iptel.org>.
> 
> Signed-off-by: Vlad Yasevich <vladislav.yasevich@hp.com>
> ---
>  net/sctp/sm_make_chunk.c |    9 +++++++--
>  1 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
> index e8ca4e5..fe94f42 100644
> --- a/net/sctp/sm_make_chunk.c
> +++ b/net/sctp/sm_make_chunk.c
> @@ -1886,11 +1886,13 @@ static void sctp_process_ext_param(struct sctp_association *asoc,
>  			    /* if the peer reports AUTH, assume that he
>  			     * supports AUTH.
>  			     */
> -			    asoc->peer.auth_capable = 1;
> +			    if (sctp_auth_enable)
> +				    asoc->peer.auth_capable = 1;
>  			    break;
>  		    case SCTP_CID_ASCONF:
>  		    case SCTP_CID_ASCONF_ACK:
> -			    asoc->peer.asconf_capable = 1;
> +			    if (sctp_addip_enable)
> +				    asoc->peer.asconf_capable = 1;
>  			    break;
>  		    default:
>  			    break;
> @@ -2460,6 +2462,9 @@ do_addr_param:
>  		break;
>  
>  	case SCTP_PARAM_SET_PRIMARY:
> +		if (!sctp_addip_enable)
> +			goto fall_through;
> +
>  		addr_param = param.v + sizeof(sctp_addip_param_t);
>  
>  		af = sctp_get_af_specific(param_type2af(param.p->type));

Re: [Lksctp-developers] [PATCH 1/2] sctp: do not enable peer features if we can't do them.

[David Miller]

From: Vlad Yasevich <vladislav.yasevich@hp.com>
Date: Thu, 18 Sep 2008 19:01:23 -0400

> Can you also queue this one and Patch 2/2 to stable.  The problems
> are there as well.

Sure thing.

Re: [PATCH 1/2] sctp: do not enable peer features if we can't do them.

[David Miller]

From: Vlad Yasevich <vladislav.yasevich@hp.com>
Date: Thu, 18 Sep 2008 17:31:03 -0400

> Do not enable peer features like addip and auth, if they
> are administratively disabled localy.  If the peer resports
> that he supports something that we don't, neither end can
> use it so enabling it is pointless.  This solves a problem
> when talking to a peer that has auth and addip enabled while
> we do not.  Found by Andrei Pelinescu-Onciul <andrei@iptel.org>.
> 
> Signed-off-by: Vlad Yasevich <vladislav.yasevich@hp.com>

I applied this, but it is at best borderline for outside the
merge window.  It doesn't fix an OOPS nor a security issue nor
an entry in the 2.6.x regression list, therefore strictly speaking
this fix is not appropriate at this time.

Please apply this criteria when deciding whether to submit future
fixes for net-2.6 inclusion.

Re: [PATCH 1/2] sctp: do not enable peer features if we can't do them.

[Vlad Yasevich]

David Miller wrote:
> From: Vlad Yasevich <vladislav.yasevich@hp.com>
> Date: Thu, 18 Sep 2008 17:31:03 -0400
> 
>> Do not enable peer features like addip and auth, if they
>> are administratively disabled localy.  If the peer resports
>> that he supports something that we don't, neither end can
>> use it so enabling it is pointless.  This solves a problem
>> when talking to a peer that has auth and addip enabled while
>> we do not.  Found by Andrei Pelinescu-Onciul <andrei@iptel.org>.
>>
>> Signed-off-by: Vlad Yasevich <vladislav.yasevich@hp.com>
> 
> I applied this, but it is at best borderline for outside the
> merge window.  It doesn't fix an OOPS nor a security issue nor
> an entry in the 2.6.x regression list, therefore strictly speaking
> this fix is not appropriate at this time.
> 
> Please apply this criteria when deciding whether to submit future
> fixes for net-2.6 inclusion.
> 

It is a major interoperability issue.  With the default sysctl settings,
we can not establish connection to BSD systems.  Yes, there is a workaround
of turning on the 2 required sysctl settings, but that is totally suboptimal.

I've thought about this fix for a while, and in my opinion, the interoperability
problem is large enough to warrant the fix at this time and the backport to
table.

Of course you are free to not include this in net-2.6, but I hope you will.

Thanks
-vlad