[PATCH net 0/3] SCTP fixes

[PATCH net 0/3] SCTP fixes

[Daniel Borkmann]

Here are some SCTP fixes.

[ Note, immediate workaround would be to disable ASCONF (it
  is sysctl disabled by default). It is actually only used
  together with chunk authentication. ]

Thanks!

Daniel Borkmann (3):
  net: sctp: fix skb_over_panic when receiving malformed ASCONF chunks
  net: sctp: fix panic on duplicate ASCONF chunks
  net: sctp: fix remote memory pressure from excessive queueing

 include/net/sctp/sctp.h  |  5 +++
 include/net/sctp/sm.h    |  6 +--
 net/sctp/associola.c     |  2 +
 net/sctp/inqueue.c       | 33 ++++------------
 net/sctp/sm_make_chunk.c | 99 +++++++++++++++++++++++++++---------------------
 net/sctp/sm_statefuns.c  | 21 +++-------
 6 files changed, 77 insertions(+), 89 deletions(-)

-- 
1.7.11.7

[PATCH net 0/3] SCTP fixes

[Daniel Borkmann]

Here are some SCTP fixes.

[ Note, immediate workaround would be to disable ASCONF (it
  is sysctl disabled by default). It is actually only used
  together with chunk authentication. ]

Thanks!

Daniel Borkmann (3):
  net: sctp: fix skb_over_panic when receiving malformed ASCONF chunks
  net: sctp: fix panic on duplicate ASCONF chunks
  net: sctp: fix remote memory pressure from excessive queueing

 include/net/sctp/sctp.h  |  5 +++
 include/net/sctp/sm.h    |  6 +--
 net/sctp/associola.c     |  2 +
 net/sctp/inqueue.c       | 33 ++++------------
 net/sctp/sm_make_chunk.c | 99 +++++++++++++++++++++++++++---------------------
 net/sctp/sm_statefuns.c  | 21 +++-------
 6 files changed, 77 insertions(+), 89 deletions(-)

-- 
1.7.11.7

[PATCH net 1/3] net: sctp: fix skb_over_panic when receiving malformed ASCONF chunks

[Daniel Borkmann]

Commit 6f4c618ddb0 ("SCTP : Add paramters validity check for
ASCONF chunk") added basic verification of ASCONF chunks, however,
it is still possible to remotely crash a server by sending a
special crafted ASCONF chunk, even up to pre 2.6.12 kernels:

skb_over_panic: text:ffffffffa01ea1c3 len:31056 put:30768
 head:ffff88011bd81800 data:ffff88011bd81800 tail:0x7950
 end:0x440 dev:<NULL>
 ------------[ cut here ]------------
kernel BUG at net/core/skbuff.c:129!
[...]
Call Trace:
 <IRQ>
 [<ffffffff8144fb1c>] skb_put+0x5c/0x70
 [<ffffffffa01ea1c3>] sctp_addto_chunk+0x63/0xd0 [sctp]
 [<ffffffffa01eadaf>] sctp_process_asconf+0x1af/0x540 [sctp]
 [<ffffffff8152d025>] ? _read_unlock_bh+0x15/0x20
 [<ffffffffa01e0038>] sctp_sf_do_asconf+0x168/0x240 [sctp]
 [<ffffffffa01e3751>] sctp_do_sm+0x71/0x1210 [sctp]
 [<ffffffff8147645d>] ? fib_rules_lookup+0xad/0xf0
 [<ffffffffa01e6b22>] ? sctp_cmp_addr_exact+0x32/0x40 [sctp]
 [<ffffffffa01e8393>] sctp_assoc_bh_rcv+0xd3/0x180 [sctp]
 [<ffffffffa01ee986>] sctp_inq_push+0x56/0x80 [sctp]
 [<ffffffffa01fcc42>] sctp_rcv+0x982/0xa10 [sctp]
 [<ffffffffa01d5123>] ? ipt_local_in_hook+0x23/0x28 [iptable_filter]
 [<ffffffff8148bdc9>] ? nf_iterate+0x69/0xb0
 [<ffffffff81496d10>] ? ip_local_deliver_finish+0x0/0x2d0
 [<ffffffff8148bf86>] ? nf_hook_slow+0x76/0x120
 [<ffffffff81496d10>] ? ip_local_deliver_finish+0x0/0x2d0
 [<ffffffff81496ded>] ip_local_deliver_finish+0xdd/0x2d0
 [<ffffffff81497078>] ip_local_deliver+0x98/0xa0
 [<ffffffff8149653d>] ip_rcv_finish+0x12d/0x440
 [<ffffffff81496ac5>] ip_rcv+0x275/0x350
 [<ffffffff8145c88b>] __netif_receive_skb+0x4ab/0x750
 [<ffffffff81460588>] netif_receive_skb+0x58/0x60

This can be triggered e.g., through a simple scripted nmap
connection scan injecting the chunk after the handshake, for
example, ...

  -------------- INIT[ASCONF; ASCONF_ACK] ------------->
  <----------- INIT-ACK[ASCONF; ASCONF_ACK] ------------
  -------------------- COOKIE-ECHO -------------------->
  <-------------------- COOKIE-ACK ---------------------
  ------------------ ASCONF; UNKNOWN ------------------>

... where ASCONF chunk of length 280 contains 2 parameters ...

  1) Add IP address parameter (param length: 16)
  2) Add/del IP address parameter (param length: 255)

... followed by an UNKNOWN chunk of e.g. 4 bytes. Here, the
Address Parameter in the ASCONF chunk is even missing, too.
This is just an example and similarly-crafted ASCONF chunks
could be used just as well.

The ASCONF chunk passes through sctp_verify_asconf() as all
parameters passed sanity checks, and after walking, we ended
up successfully at the chunk end boundary, and thus may invoke
sctp_process_asconf(). Parameter walking is done with
WORD_ROUND() to take padding into account.

In sctp_process_asconf()'s TLV processing, we may fail in
sctp_process_asconf_param() e.g., due to removal of the IP
address that is also the source address of the packet containing
the ASCONF chunk, and thus we need to add all TLVs after the
failure to our ASCONF response to remote via helper function
sctp_add_asconf_response(), which basically invokes a
sctp_addto_chunk() adding the error parameters to the given
skb.

When walking to the next parameter this time, we proceed
with ...

  length = ntohs(asconf_param->param_hdr.length);
  asconf_param = (void *)asconf_param + length;

... instead of the WORD_ROUND()'ed length, thus resulting here
in an off-by-one that leads to reading the follow-up garbage
parameter length of 12336, and thus throwing an skb_over_panic
for the reply when trying to sctp_addto_chunk() next time,
which implicitly calls the skb_put() with that length.

Fix it by using sctp_walk_params() [ which is also used in
INIT parameter processing ] macro in the verification *and*
in ASCONF processing: it will make sure we don't spill over,
that we walk parameters WORD_ROUND()'ed. Moreover, we're being
more defensive and guard against unknown parameter types and
missized addresses.

Joint work with Vlad Yasevich.

Fixes: b896b82be4ae ("[SCTP] ADDIP: Support for processing incoming ASCONF_ACK chunks.")
Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
Signed-off-by: Vlad Yasevich <vyasevich@gmail.com>
---
 include/net/sctp/sm.h    |  6 +--
 net/sctp/sm_make_chunk.c | 99 +++++++++++++++++++++++++++---------------------
 net/sctp/sm_statefuns.c  | 18 +--------
 3 files changed, 60 insertions(+), 63 deletions(-)

diff --git a/include/net/sctp/sm.h b/include/net/sctp/sm.h
index 7f4eeb3..72a31db 100644
--- a/include/net/sctp/sm.h
+++ b/include/net/sctp/sm.h
@@ -248,9 +248,9 @@ struct sctp_chunk *sctp_make_asconf_update_ip(struct sctp_association *,
 					      int, __be16);
 struct sctp_chunk *sctp_make_asconf_set_prim(struct sctp_association *asoc,
 					     union sctp_addr *addr);
-int sctp_verify_asconf(const struct sctp_association *asoc,
-		       struct sctp_paramhdr *param_hdr, void *chunk_end,
-		       struct sctp_paramhdr **errp);
+bool sctp_verify_asconf(const struct sctp_association *asoc,
+			struct sctp_chunk *chunk, bool addr_param_needed,
+			struct sctp_paramhdr **errp);
 struct sctp_chunk *sctp_process_asconf(struct sctp_association *asoc,
 				       struct sctp_chunk *asconf);
 int sctp_process_asconf_ack(struct sctp_association *asoc,
diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
index ae0e616..ab734be 100644
--- a/net/sctp/sm_make_chunk.c
+++ b/net/sctp/sm_make_chunk.c
@@ -3110,50 +3110,63 @@ static __be16 sctp_process_asconf_param(struct sctp_association *asoc,
 	return SCTP_ERROR_NO_ERROR;
 }
 
-/* Verify the ASCONF packet before we process it.  */
-int sctp_verify_asconf(const struct sctp_association *asoc,
-		       struct sctp_paramhdr *param_hdr, void *chunk_end,
-		       struct sctp_paramhdr **errp) {
-	sctp_addip_param_t *asconf_param;
+/* Verify the ASCONF packet before we process it. */
+bool sctp_verify_asconf(const struct sctp_association *asoc,
+			struct sctp_chunk *chunk, bool addr_param_needed,
+			struct sctp_paramhdr **errp)
+{
+	sctp_addip_chunk_t *addip = (sctp_addip_chunk_t *) chunk->chunk_hdr;
 	union sctp_params param;
-	int length, plen;
-
-	param.v = (sctp_paramhdr_t *) param_hdr;
-	while (param.v <= chunk_end - sizeof(sctp_paramhdr_t)) {
-		length = ntohs(param.p->length);
-		*errp = param.p;
+	bool addr_param_seen = false;
 
-		if (param.v > chunk_end - length ||
-		    length < sizeof(sctp_paramhdr_t))
-			return 0;
+	sctp_walk_params(param, addip, addip_hdr.params) {
+		size_t length = ntohs(param.p->length);
 
+		*errp = param.p;
 		switch (param.p->type) {
+		case SCTP_PARAM_ERR_CAUSE:
+			break;
+		case SCTP_PARAM_IPV4_ADDRESS:
+			if (length != sizeof(sctp_ipv4addr_param_t))
+				return false;
+			addr_param_seen = true;
+			break;
+		case SCTP_PARAM_IPV6_ADDRESS:
+			if (length != sizeof(sctp_ipv6addr_param_t))
+				return false;
+			addr_param_seen = true;
+			break;
 		case SCTP_PARAM_ADD_IP:
 		case SCTP_PARAM_DEL_IP:
 		case SCTP_PARAM_SET_PRIMARY:
-			asconf_param = (sctp_addip_param_t *)param.v;
-			plen = ntohs(asconf_param->param_hdr.length);
-			if (plen < sizeof(sctp_addip_param_t) +
-			    sizeof(sctp_paramhdr_t))
-				return 0;
+			/* In ASCONF chunks, these need to be first. */
+			if (addr_param_needed && !addr_param_seen)
+				return false;
+			length = ntohs(param.addip->param_hdr.length);
+			if (length < sizeof(sctp_addip_param_t) +
+				     sizeof(sctp_paramhdr_t))
+				return false;
 			break;
 		case SCTP_PARAM_SUCCESS_REPORT:
 		case SCTP_PARAM_ADAPTATION_LAYER_IND:
 			if (length != sizeof(sctp_addip_param_t))
-				return 0;
-
+				return false;
 			break;
 		default:
-			break;
+			/* This is unkown to us, reject! */
+			return false;
 		}
-
-		param.v += WORD_ROUND(length);
 	}
 
-	if (param.v != chunk_end)
-		return 0;
+	/* Remaining sanity checks. */
+	if (addr_param_needed && !addr_param_seen)
+		return false;
+	if (!addr_param_needed && addr_param_seen)
+		return false;
+	if (param.v != chunk->chunk_end)
+		return false;
 
-	return 1;
+	return true;
 }
 
 /* Process an incoming ASCONF chunk with the next expected serial no. and
@@ -3162,16 +3175,17 @@ int sctp_verify_asconf(const struct sctp_association *asoc,
 struct sctp_chunk *sctp_process_asconf(struct sctp_association *asoc,
 				       struct sctp_chunk *asconf)
 {
+	sctp_addip_chunk_t *addip = (sctp_addip_chunk_t *) asconf->chunk_hdr;
+	bool all_param_pass = true;
+	union sctp_params param;
 	sctp_addiphdr_t		*hdr;
 	union sctp_addr_param	*addr_param;
 	sctp_addip_param_t	*asconf_param;
 	struct sctp_chunk	*asconf_ack;
-
 	__be16	err_code;
 	int	length = 0;
 	int	chunk_len;
 	__u32	serial;
-	int	all_param_pass = 1;
 
 	chunk_len = ntohs(asconf->chunk_hdr->length) - sizeof(sctp_chunkhdr_t);
 	hdr = (sctp_addiphdr_t *)asconf->skb->data;
@@ -3199,9 +3213,14 @@ struct sctp_chunk *sctp_process_asconf(struct sctp_association *asoc,
 		goto done;
 
 	/* Process the TLVs contained within the ASCONF chunk. */
-	while (chunk_len > 0) {
+	sctp_walk_params(param, addip, addip_hdr.params) {
+		/* Skip preceeding address parameters. */
+		if (param.p->type == SCTP_PARAM_IPV4_ADDRESS ||
+		    param.p->type == SCTP_PARAM_IPV6_ADDRESS)
+			continue;
+
 		err_code = sctp_process_asconf_param(asoc, asconf,
-						     asconf_param);
+						     param.addip);
 		/* ADDIP 4.1 A7)
 		 * If an error response is received for a TLV parameter,
 		 * all TLVs with no response before the failed TLV are
@@ -3209,28 +3228,20 @@ struct sctp_chunk *sctp_process_asconf(struct sctp_association *asoc,
 		 * the failed response are considered unsuccessful unless
 		 * a specific success indication is present for the parameter.
 		 */
-		if (SCTP_ERROR_NO_ERROR != err_code)
-			all_param_pass = 0;
-
+		if (err_code != SCTP_ERROR_NO_ERROR)
+			all_param_pass = false;
 		if (!all_param_pass)
-			sctp_add_asconf_response(asconf_ack,
-						 asconf_param->crr_id, err_code,
-						 asconf_param);
+			sctp_add_asconf_response(asconf_ack, param.addip->crr_id,
+						 err_code, param.addip);
 
 		/* ADDIP 4.3 D11) When an endpoint receiving an ASCONF to add
 		 * an IP address sends an 'Out of Resource' in its response, it
 		 * MUST also fail any subsequent add or delete requests bundled
 		 * in the ASCONF.
 		 */
-		if (SCTP_ERROR_RSRC_LOW == err_code)
+		if (err_code == SCTP_ERROR_RSRC_LOW)
 			goto done;
-
-		/* Move to the next ASCONF param. */
-		length = ntohs(asconf_param->param_hdr.length);
-		asconf_param = (void *)asconf_param + length;
-		chunk_len -= length;
 	}
-
 done:
 	asoc->peer.addip_serial++;
 
diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
index c8f6063..bdea3df 100644
--- a/net/sctp/sm_statefuns.c
+++ b/net/sctp/sm_statefuns.c
@@ -3591,9 +3591,7 @@ sctp_disposition_t sctp_sf_do_asconf(struct net *net,
 	struct sctp_chunk	*asconf_ack = NULL;
 	struct sctp_paramhdr	*err_param = NULL;
 	sctp_addiphdr_t		*hdr;
-	union sctp_addr_param	*addr_param;
 	__u32			serial;
-	int			length;
 
 	if (!sctp_vtag_verify(chunk, asoc)) {
 		sctp_add_cmd_sf(commands, SCTP_CMD_REPORT_BAD_TAG,
@@ -3618,17 +3616,8 @@ sctp_disposition_t sctp_sf_do_asconf(struct net *net,
 	hdr = (sctp_addiphdr_t *)chunk->skb->data;
 	serial = ntohl(hdr->serial);
 
-	addr_param = (union sctp_addr_param *)hdr->params;
-	length = ntohs(addr_param->p.length);
-	if (length < sizeof(sctp_paramhdr_t))
-		return sctp_sf_violation_paramlen(net, ep, asoc, type, arg,
-			   (void *)addr_param, commands);
-
 	/* Verify the ASCONF chunk before processing it. */
-	if (!sctp_verify_asconf(asoc,
-			    (sctp_paramhdr_t *)((void *)addr_param + length),
-			    (void *)chunk->chunk_end,
-			    &err_param))
+	if (!sctp_verify_asconf(asoc, chunk, true, &err_param))
 		return sctp_sf_violation_paramlen(net, ep, asoc, type, arg,
 						  (void *)err_param, commands);
 
@@ -3745,10 +3734,7 @@ sctp_disposition_t sctp_sf_do_asconf_ack(struct net *net,
 	rcvd_serial = ntohl(addip_hdr->serial);
 
 	/* Verify the ASCONF-ACK chunk before processing it. */
-	if (!sctp_verify_asconf(asoc,
-	    (sctp_paramhdr_t *)addip_hdr->params,
-	    (void *)asconf_ack->chunk_end,
-	    &err_param))
+	if (!sctp_verify_asconf(asoc, asconf_ack, false, &err_param))
 		return sctp_sf_violation_paramlen(net, ep, asoc, type, arg,
 			   (void *)err_param, commands);
 
-- 
1.7.11.7

[PATCH net 1/3] net: sctp: fix skb_over_panic when receiving malformed ASCONF chunks

[Daniel Borkmann]

Commit 6f4c618ddb0 ("SCTP : Add paramters validity check for
ASCONF chunk") added basic verification of ASCONF chunks, however,
it is still possible to remotely crash a server by sending a
special crafted ASCONF chunk, even up to pre 2.6.12 kernels:

skb_over_panic: text:ffffffffa01ea1c3 len:31056 put:30768
 head:ffff88011bd81800 data:ffff88011bd81800 tail:0x7950
 end:0x440 dev:<NULL>
 ------------[ cut here ]------------
kernel BUG at net/core/skbuff.c:129!
[...]
Call Trace:
 <IRQ>
 [<ffffffff8144fb1c>] skb_put+0x5c/0x70
 [<ffffffffa01ea1c3>] sctp_addto_chunk+0x63/0xd0 [sctp]
 [<ffffffffa01eadaf>] sctp_process_asconf+0x1af/0x540 [sctp]
 [<ffffffff8152d025>] ? _read_unlock_bh+0x15/0x20
 [<ffffffffa01e0038>] sctp_sf_do_asconf+0x168/0x240 [sctp]
 [<ffffffffa01e3751>] sctp_do_sm+0x71/0x1210 [sctp]
 [<ffffffff8147645d>] ? fib_rules_lookup+0xad/0xf0
 [<ffffffffa01e6b22>] ? sctp_cmp_addr_exact+0x32/0x40 [sctp]
 [<ffffffffa01e8393>] sctp_assoc_bh_rcv+0xd3/0x180 [sctp]
 [<ffffffffa01ee986>] sctp_inq_push+0x56/0x80 [sctp]
 [<ffffffffa01fcc42>] sctp_rcv+0x982/0xa10 [sctp]
 [<ffffffffa01d5123>] ? ipt_local_in_hook+0x23/0x28 [iptable_filter]
 [<ffffffff8148bdc9>] ? nf_iterate+0x69/0xb0
 [<ffffffff81496d10>] ? ip_local_deliver_finish+0x0/0x2d0
 [<ffffffff8148bf86>] ? nf_hook_slow+0x76/0x120
 [<ffffffff81496d10>] ? ip_local_deliver_finish+0x0/0x2d0
 [<ffffffff81496ded>] ip_local_deliver_finish+0xdd/0x2d0
 [<ffffffff81497078>] ip_local_deliver+0x98/0xa0
 [<ffffffff8149653d>] ip_rcv_finish+0x12d/0x440
 [<ffffffff81496ac5>] ip_rcv+0x275/0x350
 [<ffffffff8145c88b>] __netif_receive_skb+0x4ab/0x750
 [<ffffffff81460588>] netif_receive_skb+0x58/0x60

This can be triggered e.g., through a simple scripted nmap
connection scan injecting the chunk after the handshake, for
example, ...

  -------------- INIT[ASCONF; ASCONF_ACK] ------------->
  <----------- INIT-ACK[ASCONF; ASCONF_ACK] ------------
  -------------------- COOKIE-ECHO -------------------->
  <-------------------- COOKIE-ACK ---------------------
  ------------------ ASCONF; UNKNOWN ------------------>

... where ASCONF chunk of length 280 contains 2 parameters ...

  1) Add IP address parameter (param length: 16)
  2) Add/del IP address parameter (param length: 255)

... followed by an UNKNOWN chunk of e.g. 4 bytes. Here, the
Address Parameter in the ASCONF chunk is even missing, too.
This is just an example and similarly-crafted ASCONF chunks
could be used just as well.

The ASCONF chunk passes through sctp_verify_asconf() as all
parameters passed sanity checks, and after walking, we ended
up successfully at the chunk end boundary, and thus may invoke
sctp_process_asconf(). Parameter walking is done with
WORD_ROUND() to take padding into account.

In sctp_process_asconf()'s TLV processing, we may fail in
sctp_process_asconf_param() e.g., due to removal of the IP
address that is also the source address of the packet containing
the ASCONF chunk, and thus we need to add all TLVs after the
failure to our ASCONF response to remote via helper function
sctp_add_asconf_response(), which basically invokes a
sctp_addto_chunk() adding the error parameters to the given
skb.

When walking to the next parameter this time, we proceed
with ...

  length = ntohs(asconf_param->param_hdr.length);
  asconf_param = (void *)asconf_param + length;

... instead of the WORD_ROUND()'ed length, thus resulting here
in an off-by-one that leads to reading the follow-up garbage
parameter length of 12336, and thus throwing an skb_over_panic
for the reply when trying to sctp_addto_chunk() next time,
which implicitly calls the skb_put() with that length.

Fix it by using sctp_walk_params() [ which is also used in
INIT parameter processing ] macro in the verification *and*
in ASCONF processing: it will make sure we don't spill over,
that we walk parameters WORD_ROUND()'ed. Moreover, we're being
more defensive and guard against unknown parameter types and
missized addresses.

Joint work with Vlad Yasevich.

Fixes: b896b82be4ae ("[SCTP] ADDIP: Support for processing incoming ASCONF_ACK chunks.")
Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
Signed-off-by: Vlad Yasevich <vyasevich@gmail.com>
---
 include/net/sctp/sm.h    |  6 +--
 net/sctp/sm_make_chunk.c | 99 +++++++++++++++++++++++++++---------------------
 net/sctp/sm_statefuns.c  | 18 +--------
 3 files changed, 60 insertions(+), 63 deletions(-)

diff --git a/include/net/sctp/sm.h b/include/net/sctp/sm.h
index 7f4eeb3..72a31db 100644
--- a/include/net/sctp/sm.h
+++ b/include/net/sctp/sm.h
@@ -248,9 +248,9 @@ struct sctp_chunk *sctp_make_asconf_update_ip(struct sctp_association *,
 					      int, __be16);
 struct sctp_chunk *sctp_make_asconf_set_prim(struct sctp_association *asoc,
 					     union sctp_addr *addr);
-int sctp_verify_asconf(const struct sctp_association *asoc,
-		       struct sctp_paramhdr *param_hdr, void *chunk_end,
-		       struct sctp_paramhdr **errp);
+bool sctp_verify_asconf(const struct sctp_association *asoc,
+			struct sctp_chunk *chunk, bool addr_param_needed,
+			struct sctp_paramhdr **errp);
 struct sctp_chunk *sctp_process_asconf(struct sctp_association *asoc,
 				       struct sctp_chunk *asconf);
 int sctp_process_asconf_ack(struct sctp_association *asoc,
diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
index ae0e616..ab734be 100644
--- a/net/sctp/sm_make_chunk.c
+++ b/net/sctp/sm_make_chunk.c
@@ -3110,50 +3110,63 @@ static __be16 sctp_process_asconf_param(struct sctp_association *asoc,
 	return SCTP_ERROR_NO_ERROR;
 }
 
-/* Verify the ASCONF packet before we process it.  */
-int sctp_verify_asconf(const struct sctp_association *asoc,
-		       struct sctp_paramhdr *param_hdr, void *chunk_end,
-		       struct sctp_paramhdr **errp) {
-	sctp_addip_param_t *asconf_param;
+/* Verify the ASCONF packet before we process it. */
+bool sctp_verify_asconf(const struct sctp_association *asoc,
+			struct sctp_chunk *chunk, bool addr_param_needed,
+			struct sctp_paramhdr **errp)
+{
+	sctp_addip_chunk_t *addip = (sctp_addip_chunk_t *) chunk->chunk_hdr;
 	union sctp_params param;
-	int length, plen;
-
-	param.v = (sctp_paramhdr_t *) param_hdr;
-	while (param.v <= chunk_end - sizeof(sctp_paramhdr_t)) {
-		length = ntohs(param.p->length);
-		*errp = param.p;
+	bool addr_param_seen = false;
 
-		if (param.v > chunk_end - length ||
-		    length < sizeof(sctp_paramhdr_t))
-			return 0;
+	sctp_walk_params(param, addip, addip_hdr.params) {
+		size_t length = ntohs(param.p->length);
 
+		*errp = param.p;
 		switch (param.p->type) {
+		case SCTP_PARAM_ERR_CAUSE:
+			break;
+		case SCTP_PARAM_IPV4_ADDRESS:
+			if (length != sizeof(sctp_ipv4addr_param_t))
+				return false;
+			addr_param_seen = true;
+			break;
+		case SCTP_PARAM_IPV6_ADDRESS:
+			if (length != sizeof(sctp_ipv6addr_param_t))
+				return false;
+			addr_param_seen = true;
+			break;
 		case SCTP_PARAM_ADD_IP:
 		case SCTP_PARAM_DEL_IP:
 		case SCTP_PARAM_SET_PRIMARY:
-			asconf_param = (sctp_addip_param_t *)param.v;
-			plen = ntohs(asconf_param->param_hdr.length);
-			if (plen < sizeof(sctp_addip_param_t) +
-			    sizeof(sctp_paramhdr_t))
-				return 0;
+			/* In ASCONF chunks, these need to be first. */
+			if (addr_param_needed && !addr_param_seen)
+				return false;
+			length = ntohs(param.addip->param_hdr.length);
+			if (length < sizeof(sctp_addip_param_t) +
+				     sizeof(sctp_paramhdr_t))
+				return false;
 			break;
 		case SCTP_PARAM_SUCCESS_REPORT:
 		case SCTP_PARAM_ADAPTATION_LAYER_IND:
 			if (length != sizeof(sctp_addip_param_t))
-				return 0;
-
+				return false;
 			break;
 		default:
-			break;
+			/* This is unkown to us, reject! */
+			return false;
 		}
-
-		param.v += WORD_ROUND(length);
 	}
 
-	if (param.v != chunk_end)
-		return 0;
+	/* Remaining sanity checks. */
+	if (addr_param_needed && !addr_param_seen)
+		return false;
+	if (!addr_param_needed && addr_param_seen)
+		return false;
+	if (param.v != chunk->chunk_end)
+		return false;
 
-	return 1;
+	return true;
 }
 
 /* Process an incoming ASCONF chunk with the next expected serial no. and
@@ -3162,16 +3175,17 @@ int sctp_verify_asconf(const struct sctp_association *asoc,
 struct sctp_chunk *sctp_process_asconf(struct sctp_association *asoc,
 				       struct sctp_chunk *asconf)
 {
+	sctp_addip_chunk_t *addip = (sctp_addip_chunk_t *) asconf->chunk_hdr;
+	bool all_param_pass = true;
+	union sctp_params param;
 	sctp_addiphdr_t		*hdr;
 	union sctp_addr_param	*addr_param;
 	sctp_addip_param_t	*asconf_param;
 	struct sctp_chunk	*asconf_ack;
-
 	__be16	err_code;
 	int	length = 0;
 	int	chunk_len;
 	__u32	serial;
-	int	all_param_pass = 1;
 
 	chunk_len = ntohs(asconf->chunk_hdr->length) - sizeof(sctp_chunkhdr_t);
 	hdr = (sctp_addiphdr_t *)asconf->skb->data;
@@ -3199,9 +3213,14 @@ struct sctp_chunk *sctp_process_asconf(struct sctp_association *asoc,
 		goto done;
 
 	/* Process the TLVs contained within the ASCONF chunk. */
-	while (chunk_len > 0) {
+	sctp_walk_params(param, addip, addip_hdr.params) {
+		/* Skip preceeding address parameters. */
+		if (param.p->type = SCTP_PARAM_IPV4_ADDRESS ||
+		    param.p->type = SCTP_PARAM_IPV6_ADDRESS)
+			continue;
+
 		err_code = sctp_process_asconf_param(asoc, asconf,
-						     asconf_param);
+						     param.addip);
 		/* ADDIP 4.1 A7)
 		 * If an error response is received for a TLV parameter,
 		 * all TLVs with no response before the failed TLV are
@@ -3209,28 +3228,20 @@ struct sctp_chunk *sctp_process_asconf(struct sctp_association *asoc,
 		 * the failed response are considered unsuccessful unless
 		 * a specific success indication is present for the parameter.
 		 */
-		if (SCTP_ERROR_NO_ERROR != err_code)
-			all_param_pass = 0;
-
+		if (err_code != SCTP_ERROR_NO_ERROR)
+			all_param_pass = false;
 		if (!all_param_pass)
-			sctp_add_asconf_response(asconf_ack,
-						 asconf_param->crr_id, err_code,
-						 asconf_param);
+			sctp_add_asconf_response(asconf_ack, param.addip->crr_id,
+						 err_code, param.addip);
 
 		/* ADDIP 4.3 D11) When an endpoint receiving an ASCONF to add
 		 * an IP address sends an 'Out of Resource' in its response, it
 		 * MUST also fail any subsequent add or delete requests bundled
 		 * in the ASCONF.
 		 */
-		if (SCTP_ERROR_RSRC_LOW = err_code)
+		if (err_code = SCTP_ERROR_RSRC_LOW)
 			goto done;
-
-		/* Move to the next ASCONF param. */
-		length = ntohs(asconf_param->param_hdr.length);
-		asconf_param = (void *)asconf_param + length;
-		chunk_len -= length;
 	}
-
 done:
 	asoc->peer.addip_serial++;
 
diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
index c8f6063..bdea3df 100644
--- a/net/sctp/sm_statefuns.c
+++ b/net/sctp/sm_statefuns.c
@@ -3591,9 +3591,7 @@ sctp_disposition_t sctp_sf_do_asconf(struct net *net,
 	struct sctp_chunk	*asconf_ack = NULL;
 	struct sctp_paramhdr	*err_param = NULL;
 	sctp_addiphdr_t		*hdr;
-	union sctp_addr_param	*addr_param;
 	__u32			serial;
-	int			length;
 
 	if (!sctp_vtag_verify(chunk, asoc)) {
 		sctp_add_cmd_sf(commands, SCTP_CMD_REPORT_BAD_TAG,
@@ -3618,17 +3616,8 @@ sctp_disposition_t sctp_sf_do_asconf(struct net *net,
 	hdr = (sctp_addiphdr_t *)chunk->skb->data;
 	serial = ntohl(hdr->serial);
 
-	addr_param = (union sctp_addr_param *)hdr->params;
-	length = ntohs(addr_param->p.length);
-	if (length < sizeof(sctp_paramhdr_t))
-		return sctp_sf_violation_paramlen(net, ep, asoc, type, arg,
-			   (void *)addr_param, commands);
-
 	/* Verify the ASCONF chunk before processing it. */
-	if (!sctp_verify_asconf(asoc,
-			    (sctp_paramhdr_t *)((void *)addr_param + length),
-			    (void *)chunk->chunk_end,
-			    &err_param))
+	if (!sctp_verify_asconf(asoc, chunk, true, &err_param))
 		return sctp_sf_violation_paramlen(net, ep, asoc, type, arg,
 						  (void *)err_param, commands);
 
@@ -3745,10 +3734,7 @@ sctp_disposition_t sctp_sf_do_asconf_ack(struct net *net,
 	rcvd_serial = ntohl(addip_hdr->serial);
 
 	/* Verify the ASCONF-ACK chunk before processing it. */
-	if (!sctp_verify_asconf(asoc,
-	    (sctp_paramhdr_t *)addip_hdr->params,
-	    (void *)asconf_ack->chunk_end,
-	    &err_param))
+	if (!sctp_verify_asconf(asoc, asconf_ack, false, &err_param))
 		return sctp_sf_violation_paramlen(net, ep, asoc, type, arg,
 			   (void *)err_param, commands);
 
-- 
1.7.11.7

[PATCH net 2/3] net: sctp: fix panic on duplicate ASCONF chunks

[Daniel Borkmann]

When receiving a e.g. semi-good formed connection scan in the
form of ...

  -------------- INIT[ASCONF; ASCONF_ACK] ------------->
  <----------- INIT-ACK[ASCONF; ASCONF_ACK] ------------
  -------------------- COOKIE-ECHO -------------------->
  <-------------------- COOKIE-ACK ---------------------
  ---------------- ASCONF_a; ASCONF_b ----------------->

... where ASCONF_a equals ASCONF_b chunk (at least both serials
need to be equal), we panic an SCTP server!

The problem is that good-formed ASCONF chunks that we reply with
ASCONF_ACK chunks are cached per serial. Thus, when we receive a
same ASCONF chunk twice (e.g. through a lost ASCONF_ACK), we do
not need to process them again on the server side (that was the
idea, also proposed in the RFC). Instead, we know it was cached
and we just resend the cached chunk instead. So far, so good.

Where things get nasty is in SCTP's side effect interpreter, that
is, sctp_cmd_interpreter():

While incoming ASCONF_a (chunk = event_arg) is being marked
!end_of_packet and !singleton, and we have an association context,
we do not flush the outqueue the first time after processing the
ASCONF_ACK singleton chunk via SCTP_CMD_REPLY. Instead, we keep it
queued up, although we set local_cork to 1. Commit 2e3216cd54b1
changed the precedence, so that as long as we get bundled, incoming
chunks we try possible bundling on outgoing queue as well. Before
this commit, we would just flush the output queue.

Now, while ASCONF_a's ASCONF_ACK sits in the corked outq, we
continue to process the same ASCONF_b chunk from the packet. As
we have cached the previous ASCONF_ACK, we find it, grab it and
do another SCTP_CMD_REPLY command on it. So, effectively, we rip
the chunk->list pointers and requeue the same ASCONF_ACK chunk
another time. Since we process ASCONF_b, it's correctly marked
with end_of_packet and we enforce an uncork, and thus flush, thus
crashing the kernel.

Fix it by testing if the ASCONF_ACK is currently pending and if
that is the case, do not requeue it. When flushing the output
queue we may relink the chunk for preparing an outgoing packet,
but eventually unlink it when it's copied into the skb right
before transmission.

Joint work with Vlad Yasevich.

Fixes: 2e3216cd54b1 ("sctp: Follow security requirement of responding with 1 packet")
Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
Signed-off-by: Vlad Yasevich <vyasevich@gmail.com>
---
 include/net/sctp/sctp.h | 5 +++++
 net/sctp/associola.c    | 2 ++
 2 files changed, 7 insertions(+)

diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
index 9fbd856..856f01c 100644
--- a/include/net/sctp/sctp.h
+++ b/include/net/sctp/sctp.h
@@ -426,6 +426,11 @@ static inline void sctp_assoc_pending_pmtu(struct sock *sk, struct sctp_associat
 	asoc->pmtu_pending = 0;
 }
 
+static inline bool sctp_chunk_pending(const struct sctp_chunk *chunk)
+{
+	return !list_empty(&chunk->list);
+}
+
 /* Walk through a list of TLV parameters.  Don't trust the
  * individual parameter lengths and instead depend on
  * the chunk length to indicate when to stop.  Make sure
diff --git a/net/sctp/associola.c b/net/sctp/associola.c
index a88b852..f791edd 100644
--- a/net/sctp/associola.c
+++ b/net/sctp/associola.c
@@ -1668,6 +1668,8 @@ struct sctp_chunk *sctp_assoc_lookup_asconf_ack(
 	 * ack chunk whose serial number matches that of the request.
 	 */
 	list_for_each_entry(ack, &asoc->asconf_ack_list, transmitted_list) {
+		if (sctp_chunk_pending(ack))
+			continue;
 		if (ack->subh.addip_hdr->serial == serial) {
 			sctp_chunk_hold(ack);
 			return ack;
-- 
1.7.11.7

[PATCH net 2/3] net: sctp: fix panic on duplicate ASCONF chunks

[Daniel Borkmann]

When receiving a e.g. semi-good formed connection scan in the
form of ...

  -------------- INIT[ASCONF; ASCONF_ACK] ------------->
  <----------- INIT-ACK[ASCONF; ASCONF_ACK] ------------
  -------------------- COOKIE-ECHO -------------------->
  <-------------------- COOKIE-ACK ---------------------
  ---------------- ASCONF_a; ASCONF_b ----------------->

... where ASCONF_a equals ASCONF_b chunk (at least both serials
need to be equal), we panic an SCTP server!

The problem is that good-formed ASCONF chunks that we reply with
ASCONF_ACK chunks are cached per serial. Thus, when we receive a
same ASCONF chunk twice (e.g. through a lost ASCONF_ACK), we do
not need to process them again on the server side (that was the
idea, also proposed in the RFC). Instead, we know it was cached
and we just resend the cached chunk instead. So far, so good.

Where things get nasty is in SCTP's side effect interpreter, that
is, sctp_cmd_interpreter():

While incoming ASCONF_a (chunk = event_arg) is being marked
!end_of_packet and !singleton, and we have an association context,
we do not flush the outqueue the first time after processing the
ASCONF_ACK singleton chunk via SCTP_CMD_REPLY. Instead, we keep it
queued up, although we set local_cork to 1. Commit 2e3216cd54b1
changed the precedence, so that as long as we get bundled, incoming
chunks we try possible bundling on outgoing queue as well. Before
this commit, we would just flush the output queue.

Now, while ASCONF_a's ASCONF_ACK sits in the corked outq, we
continue to process the same ASCONF_b chunk from the packet. As
we have cached the previous ASCONF_ACK, we find it, grab it and
do another SCTP_CMD_REPLY command on it. So, effectively, we rip
the chunk->list pointers and requeue the same ASCONF_ACK chunk
another time. Since we process ASCONF_b, it's correctly marked
with end_of_packet and we enforce an uncork, and thus flush, thus
crashing the kernel.

Fix it by testing if the ASCONF_ACK is currently pending and if
that is the case, do not requeue it. When flushing the output
queue we may relink the chunk for preparing an outgoing packet,
but eventually unlink it when it's copied into the skb right
before transmission.

Joint work with Vlad Yasevich.

Fixes: 2e3216cd54b1 ("sctp: Follow security requirement of responding with 1 packet")
Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
Signed-off-by: Vlad Yasevich <vyasevich@gmail.com>
---
 include/net/sctp/sctp.h | 5 +++++
 net/sctp/associola.c    | 2 ++
 2 files changed, 7 insertions(+)

diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
index 9fbd856..856f01c 100644
--- a/include/net/sctp/sctp.h
+++ b/include/net/sctp/sctp.h
@@ -426,6 +426,11 @@ static inline void sctp_assoc_pending_pmtu(struct sock *sk, struct sctp_associat
 	asoc->pmtu_pending = 0;
 }
 
+static inline bool sctp_chunk_pending(const struct sctp_chunk *chunk)
+{
+	return !list_empty(&chunk->list);
+}
+
 /* Walk through a list of TLV parameters.  Don't trust the
  * individual parameter lengths and instead depend on
  * the chunk length to indicate when to stop.  Make sure
diff --git a/net/sctp/associola.c b/net/sctp/associola.c
index a88b852..f791edd 100644
--- a/net/sctp/associola.c
+++ b/net/sctp/associola.c
@@ -1668,6 +1668,8 @@ struct sctp_chunk *sctp_assoc_lookup_asconf_ack(
 	 * ack chunk whose serial number matches that of the request.
 	 */
 	list_for_each_entry(ack, &asoc->asconf_ack_list, transmitted_list) {
+		if (sctp_chunk_pending(ack))
+			continue;
 		if (ack->subh.addip_hdr->serial = serial) {
 			sctp_chunk_hold(ack);
 			return ack;
-- 
1.7.11.7

[PATCH net 3/3] net: sctp: fix remote memory pressure from excessive queueing

[Daniel Borkmann]

This scenario is not limited to ASCONF, just taken as one
example triggering the issue. When receiving ASCONF probes
in the form of ...

  -------------- INIT[ASCONF; ASCONF_ACK] ------------->
  <----------- INIT-ACK[ASCONF; ASCONF_ACK] ------------
  -------------------- COOKIE-ECHO -------------------->
  <-------------------- COOKIE-ACK ---------------------
  ---- ASCONF_a; [ASCONF_b; ...; ASCONF_n;] JUNK ------>
  [...]
  ---- ASCONF_m; [ASCONF_o; ...; ASCONF_z;] JUNK ------>

... where ASCONF_a, ASCONF_b, ..., ASCONF_z are good-formed
ASCONFs and have increasing serial numbers, we process such
ASCONF chunk(s) marked with !end_of_packet and !singleton,
since we have not yet reached the SCTP packet end. SCTP does
only do verification on a chunk by chunk basis, as an SCTP
packet is nothing more than just a container of a stream of
chunks which it eats up one by one.

We could run into the case that we receive a packet with a
malformed tail, above marked as trailing JUNK. All previous
chunks are here goodformed, so the stack will eat up all
previous chunks up to this point. In case JUNK does not fit
into a chunk header and there are no more other chunks in
the input queue, or in case JUNK contains a garbage chunk
header, but the encoded chunk length would exceed the skb
tail, or we came here from an entirely different scenario
and the chunk has pdiscard=1 mark (without having had a flush
point), it will happen, that we will excessively queue up
the association's output queue (a correct final chunk may
then turn it into a response flood when flushing the
queue ;)): I ran a simple script with incremental ASCONF
serial numbers and could see the server side consuming
excessive amount of RAM [before/after: up to 2GB and more].

The issue at heart is that the chunk train basically ends
with !end_of_packet and !singleton markers and since commit
2e3216cd54b1 ("sctp: Follow security requirement of responding
with 1 packet") therefore preventing an output queue flush
point in sctp_do_sm() -> sctp_cmd_interpreter() on the input
chunk (chunk = event_arg) even though local_cork is set,
but its precedence has changed since then. In the normal
case, the last chunk with end_of_packet=1 would trigger the
queue flush to accommodate possible outgoing bundling.

In the input queue, sctp_inq_pop() seems to do the right thing
in terms of discarding invalid chunks. So, above JUNK will
not enter the state machine and instead be released and exit
the sctp_assoc_bh_rcv() chunk processing loop. It's simply
the flush point being missing at loop exit. Adding a try-flush
approach on the output queue might not work as the underlying
infrastructure might be long gone at this point due to the
side-effect interpreter run.

One possibility, albeit a bit of a kludge, would be to defer
invalid chunk freeing into the state machine in order to
possibly trigger packet discards and thus indirectly a queue
flush on error. It would surely be better to discard chunks
as in the current, perhaps better controlled environment, but
going back and forth, it's simply architecturally not possible.
I tried various trailing JUNK attack cases and it seems to
look good now.

Joint work with Vlad Yasevich.

Fixes: 2e3216cd54b1 ("sctp: Follow security requirement of responding with 1 packet")
Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
Signed-off-by: Vlad Yasevich <vyasevich@gmail.com>
---
 net/sctp/inqueue.c      | 33 +++++++--------------------------
 net/sctp/sm_statefuns.c |  3 +++
 2 files changed, 10 insertions(+), 26 deletions(-)

diff --git a/net/sctp/inqueue.c b/net/sctp/inqueue.c
index 4de12af..7e8a16c 100644
--- a/net/sctp/inqueue.c
+++ b/net/sctp/inqueue.c
@@ -140,18 +140,9 @@ struct sctp_chunk *sctp_inq_pop(struct sctp_inq *queue)
 		} else {
 			/* Nothing to do. Next chunk in the packet, please. */
 			ch = (sctp_chunkhdr_t *) chunk->chunk_end;
-
 			/* Force chunk->skb->data to chunk->chunk_end.  */
-			skb_pull(chunk->skb,
-				 chunk->chunk_end - chunk->skb->data);
-
-			/* Verify that we have at least chunk headers
-			 * worth of buffer left.
-			 */
-			if (skb_headlen(chunk->skb) < sizeof(sctp_chunkhdr_t)) {
-				sctp_chunk_free(chunk);
-				chunk = queue->in_progress = NULL;
-			}
+			skb_pull(chunk->skb, chunk->chunk_end - chunk->skb->data);
+			/* We are guaranteed to pull a SCTP header. */
 		}
 	}
 
@@ -187,24 +178,14 @@ struct sctp_chunk *sctp_inq_pop(struct sctp_inq *queue)
 	skb_pull(chunk->skb, sizeof(sctp_chunkhdr_t));
 	chunk->subh.v = NULL; /* Subheader is no longer valid.  */
 
-	if (chunk->chunk_end < skb_tail_pointer(chunk->skb)) {
+	if (chunk->chunk_end + sizeof(sctp_chunkhdr_t) <
+	    skb_tail_pointer(chunk->skb)) {
 		/* This is not a singleton */
 		chunk->singleton = 0;
 	} else if (chunk->chunk_end > skb_tail_pointer(chunk->skb)) {
-		/* RFC 2960, Section 6.10  Bundling
-		 *
-		 * Partial chunks MUST NOT be placed in an SCTP packet.
-		 * If the receiver detects a partial chunk, it MUST drop
-		 * the chunk.
-		 *
-		 * Since the end of the chunk is past the end of our buffer
-		 * (which contains the whole packet, we can freely discard
-		 * the whole packet.
-		 */
-		sctp_chunk_free(chunk);
-		chunk = queue->in_progress = NULL;
-
-		return NULL;
+		/* Discard inside state machine. */
+		chunk->pdiscard = 1;
+		chunk->chunk_end = skb_tail_pointer(chunk->skb);
 	} else {
 		/* We are at the end of the packet, so mark the chunk
 		 * in case we need to send a SACK.
diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
index bdea3df..3ee27b7 100644
--- a/net/sctp/sm_statefuns.c
+++ b/net/sctp/sm_statefuns.c
@@ -170,6 +170,9 @@ sctp_chunk_length_valid(struct sctp_chunk *chunk,
 {
 	__u16 chunk_length = ntohs(chunk->chunk_hdr->length);
 
+	/* Previously already marked? */
+	if (unlikely(chunk->pdiscard))
+		return 0;
 	if (unlikely(chunk_length < required_length))
 		return 0;
 
-- 
1.7.11.7

[PATCH net 3/3] net: sctp: fix remote memory pressure from excessive queueing

[Daniel Borkmann]

This scenario is not limited to ASCONF, just taken as one
example triggering the issue. When receiving ASCONF probes
in the form of ...

  -------------- INIT[ASCONF; ASCONF_ACK] ------------->
  <----------- INIT-ACK[ASCONF; ASCONF_ACK] ------------
  -------------------- COOKIE-ECHO -------------------->
  <-------------------- COOKIE-ACK ---------------------
  ---- ASCONF_a; [ASCONF_b; ...; ASCONF_n;] JUNK ------>
  [...]
  ---- ASCONF_m; [ASCONF_o; ...; ASCONF_z;] JUNK ------>

... where ASCONF_a, ASCONF_b, ..., ASCONF_z are good-formed
ASCONFs and have increasing serial numbers, we process such
ASCONF chunk(s) marked with !end_of_packet and !singleton,
since we have not yet reached the SCTP packet end. SCTP does
only do verification on a chunk by chunk basis, as an SCTP
packet is nothing more than just a container of a stream of
chunks which it eats up one by one.

We could run into the case that we receive a packet with a
malformed tail, above marked as trailing JUNK. All previous
chunks are here goodformed, so the stack will eat up all
previous chunks up to this point. In case JUNK does not fit
into a chunk header and there are no more other chunks in
the input queue, or in case JUNK contains a garbage chunk
header, but the encoded chunk length would exceed the skb
tail, or we came here from an entirely different scenario
and the chunk has pdiscard=1 mark (without having had a flush
point), it will happen, that we will excessively queue up
the association's output queue (a correct final chunk may
then turn it into a response flood when flushing the
queue ;)): I ran a simple script with incremental ASCONF
serial numbers and could see the server side consuming
excessive amount of RAM [before/after: up to 2GB and more].

The issue at heart is that the chunk train basically ends
with !end_of_packet and !singleton markers and since commit
2e3216cd54b1 ("sctp: Follow security requirement of responding
with 1 packet") therefore preventing an output queue flush
point in sctp_do_sm() -> sctp_cmd_interpreter() on the input
chunk (chunk = event_arg) even though local_cork is set,
but its precedence has changed since then. In the normal
case, the last chunk with end_of_packet=1 would trigger the
queue flush to accommodate possible outgoing bundling.

In the input queue, sctp_inq_pop() seems to do the right thing
in terms of discarding invalid chunks. So, above JUNK will
not enter the state machine and instead be released and exit
the sctp_assoc_bh_rcv() chunk processing loop. It's simply
the flush point being missing at loop exit. Adding a try-flush
approach on the output queue might not work as the underlying
infrastructure might be long gone at this point due to the
side-effect interpreter run.

One possibility, albeit a bit of a kludge, would be to defer
invalid chunk freeing into the state machine in order to
possibly trigger packet discards and thus indirectly a queue
flush on error. It would surely be better to discard chunks
as in the current, perhaps better controlled environment, but
going back and forth, it's simply architecturally not possible.
I tried various trailing JUNK attack cases and it seems to
look good now.

Joint work with Vlad Yasevich.

Fixes: 2e3216cd54b1 ("sctp: Follow security requirement of responding with 1 packet")
Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
Signed-off-by: Vlad Yasevich <vyasevich@gmail.com>
---
 net/sctp/inqueue.c      | 33 +++++++--------------------------
 net/sctp/sm_statefuns.c |  3 +++
 2 files changed, 10 insertions(+), 26 deletions(-)

diff --git a/net/sctp/inqueue.c b/net/sctp/inqueue.c
index 4de12af..7e8a16c 100644
--- a/net/sctp/inqueue.c
+++ b/net/sctp/inqueue.c
@@ -140,18 +140,9 @@ struct sctp_chunk *sctp_inq_pop(struct sctp_inq *queue)
 		} else {
 			/* Nothing to do. Next chunk in the packet, please. */
 			ch = (sctp_chunkhdr_t *) chunk->chunk_end;
-
 			/* Force chunk->skb->data to chunk->chunk_end.  */
-			skb_pull(chunk->skb,
-				 chunk->chunk_end - chunk->skb->data);
-
-			/* Verify that we have at least chunk headers
-			 * worth of buffer left.
-			 */
-			if (skb_headlen(chunk->skb) < sizeof(sctp_chunkhdr_t)) {
-				sctp_chunk_free(chunk);
-				chunk = queue->in_progress = NULL;
-			}
+			skb_pull(chunk->skb, chunk->chunk_end - chunk->skb->data);
+			/* We are guaranteed to pull a SCTP header. */
 		}
 	}
 
@@ -187,24 +178,14 @@ struct sctp_chunk *sctp_inq_pop(struct sctp_inq *queue)
 	skb_pull(chunk->skb, sizeof(sctp_chunkhdr_t));
 	chunk->subh.v = NULL; /* Subheader is no longer valid.  */
 
-	if (chunk->chunk_end < skb_tail_pointer(chunk->skb)) {
+	if (chunk->chunk_end + sizeof(sctp_chunkhdr_t) <
+	    skb_tail_pointer(chunk->skb)) {
 		/* This is not a singleton */
 		chunk->singleton = 0;
 	} else if (chunk->chunk_end > skb_tail_pointer(chunk->skb)) {
-		/* RFC 2960, Section 6.10  Bundling
-		 *
-		 * Partial chunks MUST NOT be placed in an SCTP packet.
-		 * If the receiver detects a partial chunk, it MUST drop
-		 * the chunk.
-		 *
-		 * Since the end of the chunk is past the end of our buffer
-		 * (which contains the whole packet, we can freely discard
-		 * the whole packet.
-		 */
-		sctp_chunk_free(chunk);
-		chunk = queue->in_progress = NULL;
-
-		return NULL;
+		/* Discard inside state machine. */
+		chunk->pdiscard = 1;
+		chunk->chunk_end = skb_tail_pointer(chunk->skb);
 	} else {
 		/* We are at the end of the packet, so mark the chunk
 		 * in case we need to send a SACK.
diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
index bdea3df..3ee27b7 100644
--- a/net/sctp/sm_statefuns.c
+++ b/net/sctp/sm_statefuns.c
@@ -170,6 +170,9 @@ sctp_chunk_length_valid(struct sctp_chunk *chunk,
 {
 	__u16 chunk_length = ntohs(chunk->chunk_hdr->length);
 
+	/* Previously already marked? */
+	if (unlikely(chunk->pdiscard))
+		return 0;
 	if (unlikely(chunk_length < required_length))
 		return 0;
 
-- 
1.7.11.7

Re: [PATCH net 1/3] net: sctp: fix skb_over_panic when receiving malformed ASCONF chunks

[Joshua Kinard]

On 10/09/2014 16:55, Daniel Borkmann wrote:
> Commit 6f4c618ddb0 ("SCTP : Add paramters validity check for
> ASCONF chunk") added basic verification of ASCONF chunks, however,
> it is still possible to remotely crash a server by sending a
> special crafted ASCONF chunk, even up to pre 2.6.12 kernels:
> 
> skb_over_panic: text:ffffffffa01ea1c3 len:31056 put:30768
>  head:ffff88011bd81800 data:ffff88011bd81800 tail:0x7950
>  end:0x440 dev:<NULL>
>  ------------[ cut here ]------------
> kernel BUG at net/core/skbuff.c:129!
> [...]
> Call Trace:
>  <IRQ>
>  [<ffffffff8144fb1c>] skb_put+0x5c/0x70
>  [<ffffffffa01ea1c3>] sctp_addto_chunk+0x63/0xd0 [sctp]
[snip]
> 
> This can be triggered e.g., through a simple scripted nmap
> connection scan injecting the chunk after the handshake, for
> example, ...
> 
>   -------------- INIT[ASCONF; ASCONF_ACK] ------------->
>   <----------- INIT-ACK[ASCONF; ASCONF_ACK] ------------
>   -------------------- COOKIE-ECHO -------------------->
>   <-------------------- COOKIE-ACK ---------------------
>   ------------------ ASCONF; UNKNOWN ------------------>
> 
> ... where ASCONF chunk of length 280 contains 2 parameters ...
> 
>   1) Add IP address parameter (param length: 16)
>   2) Add/del IP address parameter (param length: 255)
> 
> ... followed by an UNKNOWN chunk of e.g. 4 bytes. Here, the
> Address Parameter in the ASCONF chunk is even missing, too.
> This is just an example and similarly-crafted ASCONF chunks
> could be used just as well.

If I am reading correctly, this crash can only be triggered by actually getting
through the SCTP handshake, then sending this specially-crafted ASCONF chunk?
Meaning a blind nmap scan using this tactic against a random netblock wouldn't
just randomly knock servers offline?  This would seem to reduce the attack
surface a quite bit by requiring the remote endpoint to actually respond.

Is there a CVE # for this?

Thanks!,

-- 
Joshua Kinard
Gentoo/MIPS
kumba@gentoo.org
4096R/D25D95E3 2011-03-28

"The past tempts us, the present confuses us, the future frightens us.  And our
lives slip away, moment by moment, lost in that vast, terrible in-between."

--Emperor Turhan, Centauri Republic

Re: [PATCH net 1/3] net: sctp: fix skb_over_panic when receiving malformed ASCONF chunks

[Joshua Kinard]

On 10/09/2014 16:55, Daniel Borkmann wrote:
> Commit 6f4c618ddb0 ("SCTP : Add paramters validity check for
> ASCONF chunk") added basic verification of ASCONF chunks, however,
> it is still possible to remotely crash a server by sending a
> special crafted ASCONF chunk, even up to pre 2.6.12 kernels:
> 
> skb_over_panic: text:ffffffffa01ea1c3 len:31056 put:30768
>  head:ffff88011bd81800 data:ffff88011bd81800 tail:0x7950
>  end:0x440 dev:<NULL>
>  ------------[ cut here ]------------
> kernel BUG at net/core/skbuff.c:129!
> [...]
> Call Trace:
>  <IRQ>
>  [<ffffffff8144fb1c>] skb_put+0x5c/0x70
>  [<ffffffffa01ea1c3>] sctp_addto_chunk+0x63/0xd0 [sctp]
[snip]
> 
> This can be triggered e.g., through a simple scripted nmap
> connection scan injecting the chunk after the handshake, for
> example, ...
> 
>   -------------- INIT[ASCONF; ASCONF_ACK] ------------->
>   <----------- INIT-ACK[ASCONF; ASCONF_ACK] ------------
>   -------------------- COOKIE-ECHO -------------------->
>   <-------------------- COOKIE-ACK ---------------------
>   ------------------ ASCONF; UNKNOWN ------------------>
> 
> ... where ASCONF chunk of length 280 contains 2 parameters ...
> 
>   1) Add IP address parameter (param length: 16)
>   2) Add/del IP address parameter (param length: 255)
> 
> ... followed by an UNKNOWN chunk of e.g. 4 bytes. Here, the
> Address Parameter in the ASCONF chunk is even missing, too.
> This is just an example and similarly-crafted ASCONF chunks
> could be used just as well.

If I am reading correctly, this crash can only be triggered by actually getting
through the SCTP handshake, then sending this specially-crafted ASCONF chunk?
Meaning a blind nmap scan using this tactic against a random netblock wouldn't
just randomly knock servers offline?  This would seem to reduce the attack
surface a quite bit by requiring the remote endpoint to actually respond.

Is there a CVE # for this?

Thanks!,

-- 
Joshua Kinard
Gentoo/MIPS
kumba@gentoo.org
4096R/D25D95E3 2011-03-28

"The past tempts us, the present confuses us, the future frightens us.  And our
lives slip away, moment by moment, lost in that vast, terrible in-between."

--Emperor Turhan, Centauri Republic

Re: [PATCH net 1/3] net: sctp: fix skb_over_panic when receiving malformed ASCONF chunks

[Neil Horman]

On Thu, Oct 09, 2014 at 10:55:31PM +0200, Daniel Borkmann wrote:
> Commit 6f4c618ddb0 ("SCTP : Add paramters validity check for
> ASCONF chunk") added basic verification of ASCONF chunks, however,
> it is still possible to remotely crash a server by sending a
> special crafted ASCONF chunk, even up to pre 2.6.12 kernels:
> 
> skb_over_panic: text:ffffffffa01ea1c3 len:31056 put:30768
>  head:ffff88011bd81800 data:ffff88011bd81800 tail:0x7950
>  end:0x440 dev:<NULL>
>  ------------[ cut here ]------------
> kernel BUG at net/core/skbuff.c:129!
> [...]
> Call Trace:
>  <IRQ>
>  [<ffffffff8144fb1c>] skb_put+0x5c/0x70
>  [<ffffffffa01ea1c3>] sctp_addto_chunk+0x63/0xd0 [sctp]
>  [<ffffffffa01eadaf>] sctp_process_asconf+0x1af/0x540 [sctp]
>  [<ffffffff8152d025>] ? _read_unlock_bh+0x15/0x20
>  [<ffffffffa01e0038>] sctp_sf_do_asconf+0x168/0x240 [sctp]
>  [<ffffffffa01e3751>] sctp_do_sm+0x71/0x1210 [sctp]
>  [<ffffffff8147645d>] ? fib_rules_lookup+0xad/0xf0
>  [<ffffffffa01e6b22>] ? sctp_cmp_addr_exact+0x32/0x40 [sctp]
>  [<ffffffffa01e8393>] sctp_assoc_bh_rcv+0xd3/0x180 [sctp]
>  [<ffffffffa01ee986>] sctp_inq_push+0x56/0x80 [sctp]
>  [<ffffffffa01fcc42>] sctp_rcv+0x982/0xa10 [sctp]
>  [<ffffffffa01d5123>] ? ipt_local_in_hook+0x23/0x28 [iptable_filter]
>  [<ffffffff8148bdc9>] ? nf_iterate+0x69/0xb0
>  [<ffffffff81496d10>] ? ip_local_deliver_finish+0x0/0x2d0
>  [<ffffffff8148bf86>] ? nf_hook_slow+0x76/0x120
>  [<ffffffff81496d10>] ? ip_local_deliver_finish+0x0/0x2d0
>  [<ffffffff81496ded>] ip_local_deliver_finish+0xdd/0x2d0
>  [<ffffffff81497078>] ip_local_deliver+0x98/0xa0
>  [<ffffffff8149653d>] ip_rcv_finish+0x12d/0x440
>  [<ffffffff81496ac5>] ip_rcv+0x275/0x350
>  [<ffffffff8145c88b>] __netif_receive_skb+0x4ab/0x750
>  [<ffffffff81460588>] netif_receive_skb+0x58/0x60
> 
> This can be triggered e.g., through a simple scripted nmap
> connection scan injecting the chunk after the handshake, for
> example, ...
> 
>   -------------- INIT[ASCONF; ASCONF_ACK] ------------->
>   <----------- INIT-ACK[ASCONF; ASCONF_ACK] ------------
>   -------------------- COOKIE-ECHO -------------------->
>   <-------------------- COOKIE-ACK ---------------------
>   ------------------ ASCONF; UNKNOWN ------------------>
> 
> ... where ASCONF chunk of length 280 contains 2 parameters ...
> 
>   1) Add IP address parameter (param length: 16)
>   2) Add/del IP address parameter (param length: 255)
> 
> ... followed by an UNKNOWN chunk of e.g. 4 bytes. Here, the
> Address Parameter in the ASCONF chunk is even missing, too.
> This is just an example and similarly-crafted ASCONF chunks
> could be used just as well.
> 
> The ASCONF chunk passes through sctp_verify_asconf() as all
> parameters passed sanity checks, and after walking, we ended
> up successfully at the chunk end boundary, and thus may invoke
> sctp_process_asconf(). Parameter walking is done with
> WORD_ROUND() to take padding into account.
> 
> In sctp_process_asconf()'s TLV processing, we may fail in
> sctp_process_asconf_param() e.g., due to removal of the IP
> address that is also the source address of the packet containing
> the ASCONF chunk, and thus we need to add all TLVs after the
> failure to our ASCONF response to remote via helper function
> sctp_add_asconf_response(), which basically invokes a
> sctp_addto_chunk() adding the error parameters to the given
> skb.
> 
> When walking to the next parameter this time, we proceed
> with ...
> 
>   length = ntohs(asconf_param->param_hdr.length);
>   asconf_param = (void *)asconf_param + length;
> 
> ... instead of the WORD_ROUND()'ed length, thus resulting here
> in an off-by-one that leads to reading the follow-up garbage
> parameter length of 12336, and thus throwing an skb_over_panic
> for the reply when trying to sctp_addto_chunk() next time,
> which implicitly calls the skb_put() with that length.
> 
> Fix it by using sctp_walk_params() [ which is also used in
> INIT parameter processing ] macro in the verification *and*
> in ASCONF processing: it will make sure we don't spill over,
> that we walk parameters WORD_ROUND()'ed. Moreover, we're being
> more defensive and guard against unknown parameter types and
> missized addresses.
> 
> Joint work with Vlad Yasevich.
> 
> Fixes: b896b82be4ae ("[SCTP] ADDIP: Support for processing incoming ASCONF_ACK chunks.")
> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
> Signed-off-by: Vlad Yasevich <vyasevich@gmail.com>

Acked-by: Neil Horman <nhorman@tuxdriver.com>

Re: [PATCH net 1/3] net: sctp: fix skb_over_panic when receiving malformed ASCONF chunks

[Neil Horman]

On Thu, Oct 09, 2014 at 10:55:31PM +0200, Daniel Borkmann wrote:
> Commit 6f4c618ddb0 ("SCTP : Add paramters validity check for
> ASCONF chunk") added basic verification of ASCONF chunks, however,
> it is still possible to remotely crash a server by sending a
> special crafted ASCONF chunk, even up to pre 2.6.12 kernels:
> 
> skb_over_panic: text:ffffffffa01ea1c3 len:31056 put:30768
>  head:ffff88011bd81800 data:ffff88011bd81800 tail:0x7950
>  end:0x440 dev:<NULL>
>  ------------[ cut here ]------------
> kernel BUG at net/core/skbuff.c:129!
> [...]
> Call Trace:
>  <IRQ>
>  [<ffffffff8144fb1c>] skb_put+0x5c/0x70
>  [<ffffffffa01ea1c3>] sctp_addto_chunk+0x63/0xd0 [sctp]
>  [<ffffffffa01eadaf>] sctp_process_asconf+0x1af/0x540 [sctp]
>  [<ffffffff8152d025>] ? _read_unlock_bh+0x15/0x20
>  [<ffffffffa01e0038>] sctp_sf_do_asconf+0x168/0x240 [sctp]
>  [<ffffffffa01e3751>] sctp_do_sm+0x71/0x1210 [sctp]
>  [<ffffffff8147645d>] ? fib_rules_lookup+0xad/0xf0
>  [<ffffffffa01e6b22>] ? sctp_cmp_addr_exact+0x32/0x40 [sctp]
>  [<ffffffffa01e8393>] sctp_assoc_bh_rcv+0xd3/0x180 [sctp]
>  [<ffffffffa01ee986>] sctp_inq_push+0x56/0x80 [sctp]
>  [<ffffffffa01fcc42>] sctp_rcv+0x982/0xa10 [sctp]
>  [<ffffffffa01d5123>] ? ipt_local_in_hook+0x23/0x28 [iptable_filter]
>  [<ffffffff8148bdc9>] ? nf_iterate+0x69/0xb0
>  [<ffffffff81496d10>] ? ip_local_deliver_finish+0x0/0x2d0
>  [<ffffffff8148bf86>] ? nf_hook_slow+0x76/0x120
>  [<ffffffff81496d10>] ? ip_local_deliver_finish+0x0/0x2d0
>  [<ffffffff81496ded>] ip_local_deliver_finish+0xdd/0x2d0
>  [<ffffffff81497078>] ip_local_deliver+0x98/0xa0
>  [<ffffffff8149653d>] ip_rcv_finish+0x12d/0x440
>  [<ffffffff81496ac5>] ip_rcv+0x275/0x350
>  [<ffffffff8145c88b>] __netif_receive_skb+0x4ab/0x750
>  [<ffffffff81460588>] netif_receive_skb+0x58/0x60
> 
> This can be triggered e.g., through a simple scripted nmap
> connection scan injecting the chunk after the handshake, for
> example, ...
> 
>   -------------- INIT[ASCONF; ASCONF_ACK] ------------->
>   <----------- INIT-ACK[ASCONF; ASCONF_ACK] ------------
>   -------------------- COOKIE-ECHO -------------------->
>   <-------------------- COOKIE-ACK ---------------------
>   ------------------ ASCONF; UNKNOWN ------------------>
> 
> ... where ASCONF chunk of length 280 contains 2 parameters ...
> 
>   1) Add IP address parameter (param length: 16)
>   2) Add/del IP address parameter (param length: 255)
> 
> ... followed by an UNKNOWN chunk of e.g. 4 bytes. Here, the
> Address Parameter in the ASCONF chunk is even missing, too.
> This is just an example and similarly-crafted ASCONF chunks
> could be used just as well.
> 
> The ASCONF chunk passes through sctp_verify_asconf() as all
> parameters passed sanity checks, and after walking, we ended
> up successfully at the chunk end boundary, and thus may invoke
> sctp_process_asconf(). Parameter walking is done with
> WORD_ROUND() to take padding into account.
> 
> In sctp_process_asconf()'s TLV processing, we may fail in
> sctp_process_asconf_param() e.g., due to removal of the IP
> address that is also the source address of the packet containing
> the ASCONF chunk, and thus we need to add all TLVs after the
> failure to our ASCONF response to remote via helper function
> sctp_add_asconf_response(), which basically invokes a
> sctp_addto_chunk() adding the error parameters to the given
> skb.
> 
> When walking to the next parameter this time, we proceed
> with ...
> 
>   length = ntohs(asconf_param->param_hdr.length);
>   asconf_param = (void *)asconf_param + length;
> 
> ... instead of the WORD_ROUND()'ed length, thus resulting here
> in an off-by-one that leads to reading the follow-up garbage
> parameter length of 12336, and thus throwing an skb_over_panic
> for the reply when trying to sctp_addto_chunk() next time,
> which implicitly calls the skb_put() with that length.
> 
> Fix it by using sctp_walk_params() [ which is also used in
> INIT parameter processing ] macro in the verification *and*
> in ASCONF processing: it will make sure we don't spill over,
> that we walk parameters WORD_ROUND()'ed. Moreover, we're being
> more defensive and guard against unknown parameter types and
> missized addresses.
> 
> Joint work with Vlad Yasevich.
> 
> Fixes: b896b82be4ae ("[SCTP] ADDIP: Support for processing incoming ASCONF_ACK chunks.")
> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
> Signed-off-by: Vlad Yasevich <vyasevich@gmail.com>

Acked-by: Neil Horman <nhorman@tuxdriver.com>

Re: [PATCH net 2/3] net: sctp: fix panic on duplicate ASCONF chunks

[Neil Horman]

On Thu, Oct 09, 2014 at 10:55:32PM +0200, Daniel Borkmann wrote:
> When receiving a e.g. semi-good formed connection scan in the
> form of ...
> 
>   -------------- INIT[ASCONF; ASCONF_ACK] ------------->
>   <----------- INIT-ACK[ASCONF; ASCONF_ACK] ------------
>   -------------------- COOKIE-ECHO -------------------->
>   <-------------------- COOKIE-ACK ---------------------
>   ---------------- ASCONF_a; ASCONF_b ----------------->
> 
> ... where ASCONF_a equals ASCONF_b chunk (at least both serials
> need to be equal), we panic an SCTP server!
> 
> The problem is that good-formed ASCONF chunks that we reply with
> ASCONF_ACK chunks are cached per serial. Thus, when we receive a
> same ASCONF chunk twice (e.g. through a lost ASCONF_ACK), we do
> not need to process them again on the server side (that was the
> idea, also proposed in the RFC). Instead, we know it was cached
> and we just resend the cached chunk instead. So far, so good.
> 
> Where things get nasty is in SCTP's side effect interpreter, that
> is, sctp_cmd_interpreter():
> 
> While incoming ASCONF_a (chunk = event_arg) is being marked
> !end_of_packet and !singleton, and we have an association context,
> we do not flush the outqueue the first time after processing the
> ASCONF_ACK singleton chunk via SCTP_CMD_REPLY. Instead, we keep it
> queued up, although we set local_cork to 1. Commit 2e3216cd54b1
> changed the precedence, so that as long as we get bundled, incoming
> chunks we try possible bundling on outgoing queue as well. Before
> this commit, we would just flush the output queue.
> 
> Now, while ASCONF_a's ASCONF_ACK sits in the corked outq, we
> continue to process the same ASCONF_b chunk from the packet. As
> we have cached the previous ASCONF_ACK, we find it, grab it and
> do another SCTP_CMD_REPLY command on it. So, effectively, we rip
> the chunk->list pointers and requeue the same ASCONF_ACK chunk
> another time. Since we process ASCONF_b, it's correctly marked
> with end_of_packet and we enforce an uncork, and thus flush, thus
> crashing the kernel.
> 
> Fix it by testing if the ASCONF_ACK is currently pending and if
> that is the case, do not requeue it. When flushing the output
> queue we may relink the chunk for preparing an outgoing packet,
> but eventually unlink it when it's copied into the skb right
> before transmission.
> 
> Joint work with Vlad Yasevich.
> 
> Fixes: 2e3216cd54b1 ("sctp: Follow security requirement of responding with 1 packet")
> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
> Signed-off-by: Vlad Yasevich <vyasevich@gmail.com>
> ---
>  include/net/sctp/sctp.h | 5 +++++
>  net/sctp/associola.c    | 2 ++
>  2 files changed, 7 insertions(+)
> 
> diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
> index 9fbd856..856f01c 100644
> --- a/include/net/sctp/sctp.h
> +++ b/include/net/sctp/sctp.h
> @@ -426,6 +426,11 @@ static inline void sctp_assoc_pending_pmtu(struct sock *sk, struct sctp_associat
>  	asoc->pmtu_pending = 0;
>  }
>  
> +static inline bool sctp_chunk_pending(const struct sctp_chunk *chunk)
> +{
> +	return !list_empty(&chunk->list);
> +}
> +
>  /* Walk through a list of TLV parameters.  Don't trust the
>   * individual parameter lengths and instead depend on
>   * the chunk length to indicate when to stop.  Make sure
> diff --git a/net/sctp/associola.c b/net/sctp/associola.c
> index a88b852..f791edd 100644
> --- a/net/sctp/associola.c
> +++ b/net/sctp/associola.c
> @@ -1668,6 +1668,8 @@ struct sctp_chunk *sctp_assoc_lookup_asconf_ack(
>  	 * ack chunk whose serial number matches that of the request.
>  	 */
>  	list_for_each_entry(ack, &asoc->asconf_ack_list, transmitted_list) {
> +		if (sctp_chunk_pending(ack))
> +			continue;
>  		if (ack->subh.addip_hdr->serial == serial) {
>  			sctp_chunk_hold(ack);
>  			return ack;
> -- 
> 1.7.11.7
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
Is it worth adding a WARN_ON, to indicate that two ASCONF chunks have been
received with duplicate serials?

Neil

Re: [PATCH net 2/3] net: sctp: fix panic on duplicate ASCONF chunks

[Neil Horman]

On Thu, Oct 09, 2014 at 10:55:32PM +0200, Daniel Borkmann wrote:
> When receiving a e.g. semi-good formed connection scan in the
> form of ...
> 
>   -------------- INIT[ASCONF; ASCONF_ACK] ------------->
>   <----------- INIT-ACK[ASCONF; ASCONF_ACK] ------------
>   -------------------- COOKIE-ECHO -------------------->
>   <-------------------- COOKIE-ACK ---------------------
>   ---------------- ASCONF_a; ASCONF_b ----------------->
> 
> ... where ASCONF_a equals ASCONF_b chunk (at least both serials
> need to be equal), we panic an SCTP server!
> 
> The problem is that good-formed ASCONF chunks that we reply with
> ASCONF_ACK chunks are cached per serial. Thus, when we receive a
> same ASCONF chunk twice (e.g. through a lost ASCONF_ACK), we do
> not need to process them again on the server side (that was the
> idea, also proposed in the RFC). Instead, we know it was cached
> and we just resend the cached chunk instead. So far, so good.
> 
> Where things get nasty is in SCTP's side effect interpreter, that
> is, sctp_cmd_interpreter():
> 
> While incoming ASCONF_a (chunk = event_arg) is being marked
> !end_of_packet and !singleton, and we have an association context,
> we do not flush the outqueue the first time after processing the
> ASCONF_ACK singleton chunk via SCTP_CMD_REPLY. Instead, we keep it
> queued up, although we set local_cork to 1. Commit 2e3216cd54b1
> changed the precedence, so that as long as we get bundled, incoming
> chunks we try possible bundling on outgoing queue as well. Before
> this commit, we would just flush the output queue.
> 
> Now, while ASCONF_a's ASCONF_ACK sits in the corked outq, we
> continue to process the same ASCONF_b chunk from the packet. As
> we have cached the previous ASCONF_ACK, we find it, grab it and
> do another SCTP_CMD_REPLY command on it. So, effectively, we rip
> the chunk->list pointers and requeue the same ASCONF_ACK chunk
> another time. Since we process ASCONF_b, it's correctly marked
> with end_of_packet and we enforce an uncork, and thus flush, thus
> crashing the kernel.
> 
> Fix it by testing if the ASCONF_ACK is currently pending and if
> that is the case, do not requeue it. When flushing the output
> queue we may relink the chunk for preparing an outgoing packet,
> but eventually unlink it when it's copied into the skb right
> before transmission.
> 
> Joint work with Vlad Yasevich.
> 
> Fixes: 2e3216cd54b1 ("sctp: Follow security requirement of responding with 1 packet")
> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
> Signed-off-by: Vlad Yasevich <vyasevich@gmail.com>
> ---
>  include/net/sctp/sctp.h | 5 +++++
>  net/sctp/associola.c    | 2 ++
>  2 files changed, 7 insertions(+)
> 
> diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
> index 9fbd856..856f01c 100644
> --- a/include/net/sctp/sctp.h
> +++ b/include/net/sctp/sctp.h
> @@ -426,6 +426,11 @@ static inline void sctp_assoc_pending_pmtu(struct sock *sk, struct sctp_associat
>  	asoc->pmtu_pending = 0;
>  }
>  
> +static inline bool sctp_chunk_pending(const struct sctp_chunk *chunk)
> +{
> +	return !list_empty(&chunk->list);
> +}
> +
>  /* Walk through a list of TLV parameters.  Don't trust the
>   * individual parameter lengths and instead depend on
>   * the chunk length to indicate when to stop.  Make sure
> diff --git a/net/sctp/associola.c b/net/sctp/associola.c
> index a88b852..f791edd 100644
> --- a/net/sctp/associola.c
> +++ b/net/sctp/associola.c
> @@ -1668,6 +1668,8 @@ struct sctp_chunk *sctp_assoc_lookup_asconf_ack(
>  	 * ack chunk whose serial number matches that of the request.
>  	 */
>  	list_for_each_entry(ack, &asoc->asconf_ack_list, transmitted_list) {
> +		if (sctp_chunk_pending(ack))
> +			continue;
>  		if (ack->subh.addip_hdr->serial = serial) {
>  			sctp_chunk_hold(ack);
>  			return ack;
> -- 
> 1.7.11.7
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
Is it worth adding a WARN_ON, to indicate that two ASCONF chunks have been
received with duplicate serials?

Neil

Re: [PATCH net 2/3] net: sctp: fix panic on duplicate ASCONF chunks

[Daniel Borkmann]

On 10/10/2014 05:39 PM, Neil Horman wrote:
...
> Is it worth adding a WARN_ON, to indicate that two ASCONF chunks have been
> received with duplicate serials?

Don't think so, as this would be triggerable from outside.

Thanks,
Daniel

Re: [PATCH net 2/3] net: sctp: fix panic on duplicate ASCONF chunks

[Daniel Borkmann]

On 10/10/2014 05:39 PM, Neil Horman wrote:
...
> Is it worth adding a WARN_ON, to indicate that two ASCONF chunks have been
> received with duplicate serials?

Don't think so, as this would be triggerable from outside.

Thanks,
Daniel

Re: [PATCH net 1/3] net: sctp: fix skb_over_panic when receiving malformed ASCONF chunks

[Daniel Borkmann]

On 10/10/2014 12:04 PM, Joshua Kinard wrote:
...
> If I am reading correctly, this crash can only be triggered by actually getting
> through the SCTP handshake, then sending this specially-crafted ASCONF chunk?
> Meaning a blind nmap scan using this tactic against a random netblock wouldn't
> just randomly knock servers offline?  This would seem to reduce the attack
> surface a quite bit by requiring the remote endpoint to actually respond.

Sorry, have been on travel almost whole day ... yes, handshake has to be
completed before that. So a scan/probe would need to establish a connection
first and ASCONF would need to be supported.

> Is there a CVE # for this?

CVE-2014-3673

Re: [PATCH net 1/3] net: sctp: fix skb_over_panic when receiving malformed ASCONF chunks

[Daniel Borkmann]

On 10/10/2014 12:04 PM, Joshua Kinard wrote:
...
> If I am reading correctly, this crash can only be triggered by actually getting
> through the SCTP handshake, then sending this specially-crafted ASCONF chunk?
> Meaning a blind nmap scan using this tactic against a random netblock wouldn't
> just randomly knock servers offline?  This would seem to reduce the attack
> surface a quite bit by requiring the remote endpoint to actually respond.

Sorry, have been on travel almost whole day ... yes, handshake has to be
completed before that. So a scan/probe would need to establish a connection
first and ASCONF would need to be supported.

> Is there a CVE # for this?

CVE-2014-3673

Re: [PATCH net 2/3] net: sctp: fix panic on duplicate ASCONF chunks

[Neil Horman]

On Sat, Oct 11, 2014 at 12:02:31AM +0200, Daniel Borkmann wrote:
> On 10/10/2014 05:39 PM, Neil Horman wrote:
> ...
> >Is it worth adding a WARN_ON, to indicate that two ASCONF chunks have been
> >received with duplicate serials?
> 
> Don't think so, as this would be triggerable from outside.
> 
WARN_ON_ONCE then, per serial number?
Neil

> Thanks,
> Daniel
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH net 2/3] net: sctp: fix panic on duplicate ASCONF chunks

[Neil Horman]

On Sat, Oct 11, 2014 at 12:02:31AM +0200, Daniel Borkmann wrote:
> On 10/10/2014 05:39 PM, Neil Horman wrote:
> ...
> >Is it worth adding a WARN_ON, to indicate that two ASCONF chunks have been
> >received with duplicate serials?
> 
> Don't think so, as this would be triggerable from outside.
> 
WARN_ON_ONCE then, per serial number?
Neil

> Thanks,
> Daniel
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH net 2/3] net: sctp: fix panic on duplicate ASCONF chunks

[Vladislav Yasevich]

I prefer we stay away from WARN-ONs, but pr_debug might be useful.

-vlad

On Sat, Oct 11, 2014 at 9:42 PM, Neil Horman <nhorman@tuxdriver.com> wrote:
> On Sat, Oct 11, 2014 at 12:02:31AM +0200, Daniel Borkmann wrote:
>> On 10/10/2014 05:39 PM, Neil Horman wrote:
>> ...
>> >Is it worth adding a WARN_ON, to indicate that two ASCONF chunks have been
>> >received with duplicate serials?
>>
>> Don't think so, as this would be triggerable from outside.
>>
> WARN_ON_ONCE then, per serial number?
> Neil
>
>> Thanks,
>> Daniel
>> --
>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>

Re: [PATCH net 2/3] net: sctp: fix panic on duplicate ASCONF chunks

[Vladislav Yasevich]

I prefer we stay away from WARN-ONs, but pr_debug might be useful.

-vlad

On Sat, Oct 11, 2014 at 9:42 PM, Neil Horman <nhorman@tuxdriver.com> wrote:
> On Sat, Oct 11, 2014 at 12:02:31AM +0200, Daniel Borkmann wrote:
>> On 10/10/2014 05:39 PM, Neil Horman wrote:
>> ...
>> >Is it worth adding a WARN_ON, to indicate that two ASCONF chunks have been
>> >received with duplicate serials?
>>
>> Don't think so, as this would be triggerable from outside.
>>
> WARN_ON_ONCE then, per serial number?
> Neil
>
>> Thanks,
>> Daniel
>> --
>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>

Re: [PATCH net 2/3] net: sctp: fix panic on duplicate ASCONF chunks

[Daniel Borkmann]

On 10/12/2014 03:42 AM, Neil Horman wrote:
> On Sat, Oct 11, 2014 at 12:02:31AM +0200, Daniel Borkmann wrote:
>> On 10/10/2014 05:39 PM, Neil Horman wrote:
>> ...
>>> Is it worth adding a WARN_ON, to indicate that two ASCONF chunks have been
>>> received with duplicate serials?
>>
>> Don't think so, as this would be triggerable from outside.
>>
> WARN_ON_ONCE then, per serial number?

Sorry, but no. If someone seriously runs that in production and it
triggers a WARN() from outside, admins will start sending us bug
reports that apparently something with the kernel code is wrong.

WARN() should only be used if we have some *internal* unexpected bug,
but can still fail gracefully. This would neither be an actual code bug
nor would it be an internally triggered one, plus we add unnecessary
complexity to the code. Similarly, for those reasons we don't WARN()
and throw a stack trace when we receive, say, an skb of invalid length
elsewhere.

I'd also like to avoid any additional pr_debug().

I don't think people enable them in production, and if they really do,
it's too late anyway as we already have received this chunk. If anything,
I'd rather like to see debugging code further removed as we have already
different facilities in the kernel for runtime debugging that are much
more powerful.

Re: [PATCH net 2/3] net: sctp: fix panic on duplicate ASCONF chunks

[Daniel Borkmann]

On 10/12/2014 03:42 AM, Neil Horman wrote:
> On Sat, Oct 11, 2014 at 12:02:31AM +0200, Daniel Borkmann wrote:
>> On 10/10/2014 05:39 PM, Neil Horman wrote:
>> ...
>>> Is it worth adding a WARN_ON, to indicate that two ASCONF chunks have been
>>> received with duplicate serials?
>>
>> Don't think so, as this would be triggerable from outside.
>>
> WARN_ON_ONCE then, per serial number?

Sorry, but no. If someone seriously runs that in production and it
triggers a WARN() from outside, admins will start sending us bug
reports that apparently something with the kernel code is wrong.

WARN() should only be used if we have some *internal* unexpected bug,
but can still fail gracefully. This would neither be an actual code bug
nor would it be an internally triggered one, plus we add unnecessary
complexity to the code. Similarly, for those reasons we don't WARN()
and throw a stack trace when we receive, say, an skb of invalid length
elsewhere.

I'd also like to avoid any additional pr_debug().

I don't think people enable them in production, and if they really do,
it's too late anyway as we already have received this chunk. If anything,
I'd rather like to see debugging code further removed as we have already
different facilities in the kernel for runtime debugging that are much
more powerful.

Re: [PATCH net 0/3] SCTP fixes

[David Miller]

From: Daniel Borkmann <dborkman@redhat.com>
Date: Thu,  9 Oct 2014 22:55:30 +0200

> Here are some SCTP fixes.
> 
> [ Note, immediate workaround would be to disable ASCONF (it
>   is sysctl disabled by default). It is actually only used
>   together with chunk authentication. ]

Series applied, thanks guys.

Re: [PATCH net 0/3] SCTP fixes

[David Miller]

From: Daniel Borkmann <dborkman@redhat.com>
Date: Thu,  9 Oct 2014 22:55:30 +0200

> Here are some SCTP fixes.
> 
> [ Note, immediate workaround would be to disable ASCONF (it
>   is sysctl disabled by default). It is actually only used
>   together with chunk authentication. ]

Series applied, thanks guys.

Re: [PATCH net 2/3] net: sctp: fix panic on duplicate ASCONF chunks

[Neil Horman]

On Mon, Oct 13, 2014 at 01:25:11AM +0200, Daniel Borkmann wrote:
> On 10/12/2014 03:42 AM, Neil Horman wrote:
> >On Sat, Oct 11, 2014 at 12:02:31AM +0200, Daniel Borkmann wrote:
> >>On 10/10/2014 05:39 PM, Neil Horman wrote:
> >>...
> >>>Is it worth adding a WARN_ON, to indicate that two ASCONF chunks have been
> >>>received with duplicate serials?
> >>
> >>Don't think so, as this would be triggerable from outside.
> >>
> >WARN_ON_ONCE then, per serial number?
> 
> Sorry, but no. If someone seriously runs that in production and it
> triggers a WARN() from outside, admins will start sending us bug
> reports that apparently something with the kernel code is wrong.
> 
> WARN() should only be used if we have some *internal* unexpected bug,
> but can still fail gracefully. This would neither be an actual code bug
> nor would it be an internally triggered one, plus we add unnecessary
> complexity to the code. Similarly, for those reasons we don't WARN()
> and throw a stack trace when we receive, say, an skb of invalid length
> elsewhere.
> 
> I'd also like to avoid any additional pr_debug().
> 
> I don't think people enable them in production, and if they really do,
> it's too late anyway as we already have received this chunk. If anything,
> I'd rather like to see debugging code further removed as we have already
> different facilities in the kernel for runtime debugging that are much
> more powerful.
What do you suggest then?  It seems like this is a protocol error that an
administrator will want to be made aware of.  I'm open to other options, but
just saying "no" isn't sufficient for me.
Neil

> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH net 2/3] net: sctp: fix panic on duplicate ASCONF chunks

[Neil Horman]

On Mon, Oct 13, 2014 at 01:25:11AM +0200, Daniel Borkmann wrote:
> On 10/12/2014 03:42 AM, Neil Horman wrote:
> >On Sat, Oct 11, 2014 at 12:02:31AM +0200, Daniel Borkmann wrote:
> >>On 10/10/2014 05:39 PM, Neil Horman wrote:
> >>...
> >>>Is it worth adding a WARN_ON, to indicate that two ASCONF chunks have been
> >>>received with duplicate serials?
> >>
> >>Don't think so, as this would be triggerable from outside.
> >>
> >WARN_ON_ONCE then, per serial number?
> 
> Sorry, but no. If someone seriously runs that in production and it
> triggers a WARN() from outside, admins will start sending us bug
> reports that apparently something with the kernel code is wrong.
> 
> WARN() should only be used if we have some *internal* unexpected bug,
> but can still fail gracefully. This would neither be an actual code bug
> nor would it be an internally triggered one, plus we add unnecessary
> complexity to the code. Similarly, for those reasons we don't WARN()
> and throw a stack trace when we receive, say, an skb of invalid length
> elsewhere.
> 
> I'd also like to avoid any additional pr_debug().
> 
> I don't think people enable them in production, and if they really do,
> it's too late anyway as we already have received this chunk. If anything,
> I'd rather like to see debugging code further removed as we have already
> different facilities in the kernel for runtime debugging that are much
> more powerful.
What do you suggest then?  It seems like this is a protocol error that an
administrator will want to be made aware of.  I'm open to other options, but
just saying "no" isn't sufficient for me.
Neil

> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH net 0/3] SCTP fixes

[Neil Horman]

On Tue, Oct 14, 2014 at 12:46:57PM -0400, David Miller wrote:
> From: Daniel Borkmann <dborkman@redhat.com>
> Date: Thu,  9 Oct 2014 22:55:30 +0200
> 
> > Here are some SCTP fixes.
> > 
> > [ Note, immediate workaround would be to disable ASCONF (it
> >   is sysctl disabled by default). It is actually only used
> >   together with chunk authentication. ]
> 
> Series applied, thanks guys.
Why did you apply this Dave? There was ongoing discussion around it.
Neil

> --
> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH net 0/3] SCTP fixes

[Neil Horman]

On Tue, Oct 14, 2014 at 12:46:57PM -0400, David Miller wrote:
> From: Daniel Borkmann <dborkman@redhat.com>
> Date: Thu,  9 Oct 2014 22:55:30 +0200
> 
> > Here are some SCTP fixes.
> > 
> > [ Note, immediate workaround would be to disable ASCONF (it
> >   is sysctl disabled by default). It is actually only used
> >   together with chunk authentication. ]
> 
> Series applied, thanks guys.
Why did you apply this Dave? There was ongoing discussion around it.
Neil

> --
> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH net 0/3] SCTP fixes

[David Miller]

From: Neil Horman <nhorman@tuxdriver.com>
Date: Tue, 14 Oct 2014 23:06:32 -0400

> On Tue, Oct 14, 2014 at 12:46:57PM -0400, David Miller wrote:
>> From: Daniel Borkmann <dborkman@redhat.com>
>> Date: Thu,  9 Oct 2014 22:55:30 +0200
>> 
>> > Here are some SCTP fixes.
>> > 
>> > [ Note, immediate workaround would be to disable ASCONF (it
>> >   is sysctl disabled by default). It is actually only used
>> >   together with chunk authentication. ]
>> 
>> Series applied, thanks guys.
> Why did you apply this Dave? There was ongoing discussion around it.

Sorry Neil, that wasn't my impression.

Worry not I can revert or apply relative fixes on top.
:-)

Thanks.

Re: [PATCH net 0/3] SCTP fixes

[David Miller]

From: Neil Horman <nhorman@tuxdriver.com>
Date: Tue, 14 Oct 2014 23:06:32 -0400

> On Tue, Oct 14, 2014 at 12:46:57PM -0400, David Miller wrote:
>> From: Daniel Borkmann <dborkman@redhat.com>
>> Date: Thu,  9 Oct 2014 22:55:30 +0200
>> 
>> > Here are some SCTP fixes.
>> > 
>> > [ Note, immediate workaround would be to disable ASCONF (it
>> >   is sysctl disabled by default). It is actually only used
>> >   together with chunk authentication. ]
>> 
>> Series applied, thanks guys.
> Why did you apply this Dave? There was ongoing discussion around it.

Sorry Neil, that wasn't my impression.

Worry not I can revert or apply relative fixes on top.
:-)

Thanks.

Re: [PATCH net 2/3] net: sctp: fix panic on duplicate ASCONF chunks

[Vlad Yasevich]

On 10/13/2014 01:25 AM, Daniel Borkmann wrote:
> On 10/12/2014 03:42 AM, Neil Horman wrote:
>> On Sat, Oct 11, 2014 at 12:02:31AM +0200, Daniel Borkmann wrote:
>>> On 10/10/2014 05:39 PM, Neil Horman wrote:
>>> ...
>>>> Is it worth adding a WARN_ON, to indicate that two ASCONF chunks have been
>>>> received with duplicate serials?
>>>
>>> Don't think so, as this would be triggerable from outside.
>>>
>> WARN_ON_ONCE then, per serial number?
> 
> Sorry, but no. If someone seriously runs that in production and it
> triggers a WARN() from outside, admins will start sending us bug
> reports that apparently something with the kernel code is wrong.
> 
> WARN() should only be used if we have some *internal* unexpected bug,
> but can still fail gracefully. This would neither be an actual code bug
> nor would it be an internally triggered one, plus we add unnecessary
> complexity to the code. Similarly, for those reasons we don't WARN()
> and throw a stack trace when we receive, say, an skb of invalid length
> elsewhere.
> 
> I'd also like to avoid any additional pr_debug().

Why avoid additional pr_debugs()?  The seem cheap, and in this particular
case, they would be rather useful.

> 
> I don't think people enable them in production, and if they really do,
> it's too late anyway as we already have received this chunk. If anything,
> I'd rather like to see debugging code further removed as we have already
> different facilities in the kernel for runtime debugging that are much
> more powerful.

Such as?  pr_debug is part of dynamic debugging which is what we want here.
In prod environments, we don't want any output, but when someone needs to
figure out why something doesn't work, it is very useful to have.

-vlad

Re: [PATCH net 2/3] net: sctp: fix panic on duplicate ASCONF chunks

[Vlad Yasevich]

On 10/13/2014 01:25 AM, Daniel Borkmann wrote:
> On 10/12/2014 03:42 AM, Neil Horman wrote:
>> On Sat, Oct 11, 2014 at 12:02:31AM +0200, Daniel Borkmann wrote:
>>> On 10/10/2014 05:39 PM, Neil Horman wrote:
>>> ...
>>>> Is it worth adding a WARN_ON, to indicate that two ASCONF chunks have been
>>>> received with duplicate serials?
>>>
>>> Don't think so, as this would be triggerable from outside.
>>>
>> WARN_ON_ONCE then, per serial number?
> 
> Sorry, but no. If someone seriously runs that in production and it
> triggers a WARN() from outside, admins will start sending us bug
> reports that apparently something with the kernel code is wrong.
> 
> WARN() should only be used if we have some *internal* unexpected bug,
> but can still fail gracefully. This would neither be an actual code bug
> nor would it be an internally triggered one, plus we add unnecessary
> complexity to the code. Similarly, for those reasons we don't WARN()
> and throw a stack trace when we receive, say, an skb of invalid length
> elsewhere.
> 
> I'd also like to avoid any additional pr_debug().

Why avoid additional pr_debugs()?  The seem cheap, and in this particular
case, they would be rather useful.

> 
> I don't think people enable them in production, and if they really do,
> it's too late anyway as we already have received this chunk. If anything,
> I'd rather like to see debugging code further removed as we have already
> different facilities in the kernel for runtime debugging that are much
> more powerful.

Such as?  pr_debug is part of dynamic debugging which is what we want here.
In prod environments, we don't want any output, but when someone needs to
figure out why something doesn't work, it is very useful to have.

-vlad

Re: [PATCH net 0/3] SCTP fixes

[Neil Horman]

On Wed, Oct 15, 2014 at 12:21:43AM -0400, David Miller wrote:
> From: Neil Horman <nhorman@tuxdriver.com>
> Date: Tue, 14 Oct 2014 23:06:32 -0400
> 
> > On Tue, Oct 14, 2014 at 12:46:57PM -0400, David Miller wrote:
> >> From: Daniel Borkmann <dborkman@redhat.com>
> >> Date: Thu,  9 Oct 2014 22:55:30 +0200
> >> 
> >> > Here are some SCTP fixes.
> >> > 
> >> > [ Note, immediate workaround would be to disable ASCONF (it
> >> >   is sysctl disabled by default). It is actually only used
> >> >   together with chunk authentication. ]
> >> 
> >> Series applied, thanks guys.
> > Why did you apply this Dave? There was ongoing discussion around it.
> 
> Sorry Neil, that wasn't my impression.
> 
> Worry not I can revert or apply relative fixes on top.
> :-)
> 
No, no worries, just trying to figure out how to inform admins of duplicate
ASCONF serial numbers.  A follow on patch to add pr_debugs will be easy here.
Neil

> Thanks.
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH net 0/3] SCTP fixes

[Neil Horman]

On Wed, Oct 15, 2014 at 12:21:43AM -0400, David Miller wrote:
> From: Neil Horman <nhorman@tuxdriver.com>
> Date: Tue, 14 Oct 2014 23:06:32 -0400
> 
> > On Tue, Oct 14, 2014 at 12:46:57PM -0400, David Miller wrote:
> >> From: Daniel Borkmann <dborkman@redhat.com>
> >> Date: Thu,  9 Oct 2014 22:55:30 +0200
> >> 
> >> > Here are some SCTP fixes.
> >> > 
> >> > [ Note, immediate workaround would be to disable ASCONF (it
> >> >   is sysctl disabled by default). It is actually only used
> >> >   together with chunk authentication. ]
> >> 
> >> Series applied, thanks guys.
> > Why did you apply this Dave? There was ongoing discussion around it.
> 
> Sorry Neil, that wasn't my impression.
> 
> Worry not I can revert or apply relative fixes on top.
> :-)
> 
No, no worries, just trying to figure out how to inform admins of duplicate
ASCONF serial numbers.  A follow on patch to add pr_debugs will be easy here.
Neil

> Thanks.
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH net 2/3] net: sctp: fix panic on duplicate ASCONF chunks

[Daniel Borkmann]

On 10/15/2014 04:58 AM, Neil Horman wrote:
> On Mon, Oct 13, 2014 at 01:25:11AM +0200, Daniel Borkmann wrote:
>> On 10/12/2014 03:42 AM, Neil Horman wrote:
>>> On Sat, Oct 11, 2014 at 12:02:31AM +0200, Daniel Borkmann wrote:
>>>> On 10/10/2014 05:39 PM, Neil Horman wrote:
>>>> ...
>>>>> Is it worth adding a WARN_ON, to indicate that two ASCONF chunks have been
>>>>> received with duplicate serials?
>>>>
>>>> Don't think so, as this would be triggerable from outside.
>>>>
>>> WARN_ON_ONCE then, per serial number?
>>
>> Sorry, but no. If someone seriously runs that in production and it
>> triggers a WARN() from outside, admins will start sending us bug
>> reports that apparently something with the kernel code is wrong.
>>
>> WARN() should only be used if we have some *internal* unexpected bug,
>> but can still fail gracefully. This would neither be an actual code bug
>> nor would it be an internally triggered one, plus we add unnecessary
>> complexity to the code. Similarly, for those reasons we don't WARN()
>> and throw a stack trace when we receive, say, an skb of invalid length
>> elsewhere.
>>
>> I'd also like to avoid any additional pr_debug().
>>
>> I don't think people enable them in production, and if they really do,
>> it's too late anyway as we already have received this chunk. If anything,
>> I'd rather like to see debugging code further removed as we have already
>> different facilities in the kernel for runtime debugging that are much
>> more powerful.
 >
> What do you suggest then?  It seems like this is a protocol error that an
> administrator will want to be made aware of.  I'm open to other options, but
> just saying "no" isn't sufficient for me.

So what do you want an admin to do with this information?

Currently, we don't throw WARN() stack traces or whatever on other 'malformed'
inputs, we either discard them silently in SCTP or send an ABORT, for example.

Do you suggest the kernel should now start throwing a WARN() on every possible
skb that we discard in order to inform the admin?

Given that we can handle this gracefully by basically ignoring the 2nd identical
ASCONF chunk in a packet and send out a single reply, that's just handled fine.

Re: [PATCH net 2/3] net: sctp: fix panic on duplicate ASCONF chunks

[Daniel Borkmann]

On 10/15/2014 04:58 AM, Neil Horman wrote:
> On Mon, Oct 13, 2014 at 01:25:11AM +0200, Daniel Borkmann wrote:
>> On 10/12/2014 03:42 AM, Neil Horman wrote:
>>> On Sat, Oct 11, 2014 at 12:02:31AM +0200, Daniel Borkmann wrote:
>>>> On 10/10/2014 05:39 PM, Neil Horman wrote:
>>>> ...
>>>>> Is it worth adding a WARN_ON, to indicate that two ASCONF chunks have been
>>>>> received with duplicate serials?
>>>>
>>>> Don't think so, as this would be triggerable from outside.
>>>>
>>> WARN_ON_ONCE then, per serial number?
>>
>> Sorry, but no. If someone seriously runs that in production and it
>> triggers a WARN() from outside, admins will start sending us bug
>> reports that apparently something with the kernel code is wrong.
>>
>> WARN() should only be used if we have some *internal* unexpected bug,
>> but can still fail gracefully. This would neither be an actual code bug
>> nor would it be an internally triggered one, plus we add unnecessary
>> complexity to the code. Similarly, for those reasons we don't WARN()
>> and throw a stack trace when we receive, say, an skb of invalid length
>> elsewhere.
>>
>> I'd also like to avoid any additional pr_debug().
>>
>> I don't think people enable them in production, and if they really do,
>> it's too late anyway as we already have received this chunk. If anything,
>> I'd rather like to see debugging code further removed as we have already
>> different facilities in the kernel for runtime debugging that are much
>> more powerful.
 >
> What do you suggest then?  It seems like this is a protocol error that an
> administrator will want to be made aware of.  I'm open to other options, but
> just saying "no" isn't sufficient for me.

So what do you want an admin to do with this information?

Currently, we don't throw WARN() stack traces or whatever on other 'malformed'
inputs, we either discard them silently in SCTP or send an ABORT, for example.

Do you suggest the kernel should now start throwing a WARN() on every possible
skb that we discard in order to inform the admin?

Given that we can handle this gracefully by basically ignoring the 2nd identical
ASCONF chunk in a packet and send out a single reply, that's just handled fine.