[PATCH]: sctp: Fix skb_over_panic resulting from multiple invalid parameter errors (CVE-2010-1173)

[PATCH]: sctp: Fix skb_over_panic resulting from multiple invalid parameter errors (CVE-2010-1173)

[Neil Horman]

Hey-
	Recently, it was reported to me that the kernel could oops in the
following way:

<5> kernel BUG at net/core/skbuff.c:91!
<5> invalid operand: 0000 [#1]
<5> Modules linked in: sctp netconsole nls_utf8 autofs4 sunrpc iptable_filter
ip_tables cpufreq_powersave parport_pc lp parport vmblock(U) vsock(U) vmci(U)
vmxnet(U) vmmemctl(U) vmhgfs(U) acpiphp dm_mirror dm_mod button battery ac md5
ipv6 uhci_hcd ehci_hcd snd_ens1371 snd_rawmidi snd_seq_device snd_pcm_oss
snd_mixer_oss snd_pcm snd_timer snd_page_alloc snd_ac97_codec snd soundcore
pcnet32 mii floppy ext3 jbd ata_piix libata mptscsih mptsas mptspi mptscsi
mptbase sd_mod scsi_mod
<5> CPU:    0
<5> EIP:    0060:[<c02bff27>]    Not tainted VLI
<5> EFLAGS: 00010216   (2.6.9-89.0.25.EL) 
<5> EIP is at skb_over_panic+0x1f/0x2d
<5> eax: 0000002c   ebx: c033f461   ecx: c0357d96   edx: c040fd44
<5> esi: c033f461   edi: df653280   ebp: 00000000   esp: c040fd40
<5> ds: 007b   es: 007b   ss: 0068
<5> Process swapper (pid: 0, threadinfo=c040f000 task=c0370be0)
<5> Stack: c0357d96 e0c29478 00000084 00000004 c033f461 df653280 d7883180
e0c2947d 
<5>        00000000 00000080 df653490 00000004 de4f1ac0 de4f1ac0 00000004
df653490 
<5>        00000001 e0c2877a 08000800 de4f1ac0 df653490 00000000 e0c29d2e
00000004 
<5> Call Trace:
<5>  [<e0c29478>] sctp_addto_chunk+0xb0/0x128 [sctp]
<5>  [<e0c2947d>] sctp_addto_chunk+0xb5/0x128 [sctp]
<5>  [<e0c2877a>] sctp_init_cause+0x3f/0x47 [sctp]
<5>  [<e0c29d2e>] sctp_process_unk_param+0xac/0xb8 [sctp]
<5>  [<e0c29e90>] sctp_verify_init+0xcc/0x134 [sctp]
<5>  [<e0c20322>] sctp_sf_do_5_1B_init+0x83/0x28e [sctp]
<5>  [<e0c25333>] sctp_do_sm+0x41/0x77 [sctp]
<5>  [<c01555a4>] cache_grow+0x140/0x233
<5>  [<e0c26ba1>] sctp_endpoint_bh_rcv+0xc5/0x108 [sctp]
<5>  [<e0c2b863>] sctp_inq_push+0xe/0x10 [sctp]
<5>  [<e0c34600>] sctp_rcv+0x454/0x509 [sctp]
<5>  [<e084e017>] ipt_hook+0x17/0x1c [iptable_filter]
<5>  [<c02d005e>] nf_iterate+0x40/0x81
<5>  [<c02e0bb9>] ip_local_deliver_finish+0x0/0x151
<5>  [<c02e0c7f>] ip_local_deliver_finish+0xc6/0x151
<5>  [<c02d0362>] nf_hook_slow+0x83/0xb5
<5>  [<c02e0bb2>] ip_local_deliver+0x1a2/0x1a9
<5>  [<c02e0bb9>] ip_local_deliver_finish+0x0/0x151
<5>  [<c02e103e>] ip_rcv+0x334/0x3b4
<5>  [<c02c66fd>] netif_receive_skb+0x320/0x35b
<5>  [<e0a0928b>] init_stall_timer+0x67/0x6a [uhci_hcd]
<5>  [<c02c67a4>] process_backlog+0x6c/0xd9
<5>  [<c02c690f>] net_rx_action+0xfe/0x1f8
<5>  [<c012a7b1>] __do_softirq+0x35/0x79
<5>  [<c0107efb>] handle_IRQ_event+0x0/0x4f
<5>  [<c01094de>] do_softirq+0x46/0x4d

Its an skb_over_panic BUG halt that results from processing an init chunk in
which too many of its variable length parameters are in some way malformed.

The problem is in sctp_process_unk_param:
if (NULL == *errp)
	*errp = sctp_make_op_error_space(asoc, chunk,
					 ntohs(chunk->chunk_hdr->length));

	if (*errp) {
		sctp_init_cause(*errp, SCTP_ERROR_UNKNOWN_PARAM,
				 WORD_ROUND(ntohs(param.p->length)));
		sctp_addto_chunk(*errp,
			WORD_ROUND(ntohs(param.p->length)),
				  param.v);

When we allocate an error chunk, we assume that the worst case scenario requires
that we have chunk_hdr->length data allocated, which would be correct nominally,
given that we call sctp_addto_chunk for the violating parameter.  Unfortunately,
we also, in sctp_init_cause insert a sctp_errhdr_t structure into the error
chunk, so the worst case situation in which all parameters are in violation
requires chunk_hdr->length+(sizeof(sctp_errhdr_t)*param_count) bytes of data.

The result of this error is that a deliberately malformed packet sent to a
listening host can cause a remote DOS, described in CVE-2010-1173:
http://cve.mitre.org/cgi-bin/cvename.cgi?name=2010-1173

I've tested the below fix and confirmed that it fixes the issue.  It
pre-allocates the error chunk in sctp_verify_init, where we are able to count
the total number of variable length parameters, so we know how many error
headers we might need.  Then we simply use that chunk, if we find an error, or
discard/free it if all the parameters are valid.  Applies on top of the
lksctp-dev tree

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


 sm_make_chunk.c |   24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)


diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
index f592163..990457b 100644
--- a/net/sctp/sm_make_chunk.c
+++ b/net/sctp/sm_make_chunk.c
@@ -2134,6 +2134,8 @@ int sctp_verify_init(const struct sctp_association *asoc,
 	union sctp_params param;
 	int has_cookie = 0;
 	int result;
+	unsigned int param_cnt;
+	unsigned int len;
 
 	/* Verify stream values are non-zero. */
 	if ((0 == peer_init->init_hdr.num_outbound_streams) ||
@@ -2149,6 +2151,7 @@ int sctp_verify_init(const struct sctp_association *asoc,
 
 		if (SCTP_PARAM_STATE_COOKIE == param.p->type)
 			has_cookie = 1;
+		param_cnt++;
 
 	} /* for (loop through all parameters) */
 
@@ -2169,6 +2172,20 @@ int sctp_verify_init(const struct sctp_association *asoc,
 		return sctp_process_missing_param(asoc, SCTP_PARAM_STATE_COOKIE,
 						  chunk, errp);
 
+	if (!*errp) {
+		/*
+		 * Pre-allocate the error packet here
+		 * we do this as we need to reserve space
+		 * for the worst case scenario in which 
+		 * every parameter is in error and needs 
+		 * an errhdr attached to it
+		 */
+		len = ntohs(chunk->chunk_hdr->length);
+		len += sizeof(sctp_errhdr_t)*param_cnt;
+
+		*errp = sctp_make_op_error_space(asoc, chunk, len);
+	}
+
 	/* Verify all the variable length parameters */
 	sctp_walk_params(param, peer_init, init_hdr.params) {
 
@@ -2176,9 +2193,11 @@ int sctp_verify_init(const struct sctp_association *asoc,
 		switch (result) {
 		    case SCTP_IERROR_ABORT:
 		    case SCTP_IERROR_NOMEM:
-				return 0;
 		    case SCTP_IERROR_ERROR:
-				return 1;
+				len = ntohs((*errp)->chunk_hdr->length);
+				if ((*errp) && (len == sizeof(sctp_chunkhdr_t)))
+					sctp_chunk_free(*errp);
+				return (result == SCTP_IERROR_ERROR) ? 1 : 0;
 		    case SCTP_IERROR_NO_ERROR:
 		    default:
 				break;
@@ -2186,6 +2205,7 @@ int sctp_verify_init(const struct sctp_association *asoc,
 
 	} /* for (loop through all parameters) */
 
+	sctp_chunk_free(*errp);
 	return 1;
 }
 

Re: [PATCH]: sctp: Fix skb_over_panic resulting from multiple invalid parameter errors (CVE-2010-1173)

[Vlad Yasevich]

I have this patch and a few others already queued.

I was planning on sending these today for stable.

Here is the full list of stable patches I have:

sctp: Fix oops when sending queued ASCONF chunks
sctp: fix to calc the INIT/INIT-ACK chunk length correctly is set
sctp: per_cpu variables should be in bh_disabled section
sctp: fix potential reference of a freed pointer
sctp: avoid irq lock inversion while call sk->sk_data_ready()

-vlad

Neil Horman wrote:
> Hey-
> 	Recently, it was reported to me that the kernel could oops in the
> following way:
> 
> <5> kernel BUG at net/core/skbuff.c:91!
> <5> invalid operand: 0000 [#1]
> <5> Modules linked in: sctp netconsole nls_utf8 autofs4 sunrpc iptable_filter
> ip_tables cpufreq_powersave parport_pc lp parport vmblock(U) vsock(U) vmci(U)
> vmxnet(U) vmmemctl(U) vmhgfs(U) acpiphp dm_mirror dm_mod button battery ac md5
> ipv6 uhci_hcd ehci_hcd snd_ens1371 snd_rawmidi snd_seq_device snd_pcm_oss
> snd_mixer_oss snd_pcm snd_timer snd_page_alloc snd_ac97_codec snd soundcore
> pcnet32 mii floppy ext3 jbd ata_piix libata mptscsih mptsas mptspi mptscsi
> mptbase sd_mod scsi_mod
> <5> CPU:    0
> <5> EIP:    0060:[<c02bff27>]    Not tainted VLI
> <5> EFLAGS: 00010216   (2.6.9-89.0.25.EL) 
> <5> EIP is at skb_over_panic+0x1f/0x2d
> <5> eax: 0000002c   ebx: c033f461   ecx: c0357d96   edx: c040fd44
> <5> esi: c033f461   edi: df653280   ebp: 00000000   esp: c040fd40
> <5> ds: 007b   es: 007b   ss: 0068
> <5> Process swapper (pid: 0, threadinfo=c040f000 task=c0370be0)
> <5> Stack: c0357d96 e0c29478 00000084 00000004 c033f461 df653280 d7883180
> e0c2947d 
> <5>        00000000 00000080 df653490 00000004 de4f1ac0 de4f1ac0 00000004
> df653490 
> <5>        00000001 e0c2877a 08000800 de4f1ac0 df653490 00000000 e0c29d2e
> 00000004 
> <5> Call Trace:
> <5>  [<e0c29478>] sctp_addto_chunk+0xb0/0x128 [sctp]
> <5>  [<e0c2947d>] sctp_addto_chunk+0xb5/0x128 [sctp]
> <5>  [<e0c2877a>] sctp_init_cause+0x3f/0x47 [sctp]
> <5>  [<e0c29d2e>] sctp_process_unk_param+0xac/0xb8 [sctp]
> <5>  [<e0c29e90>] sctp_verify_init+0xcc/0x134 [sctp]
> <5>  [<e0c20322>] sctp_sf_do_5_1B_init+0x83/0x28e [sctp]
> <5>  [<e0c25333>] sctp_do_sm+0x41/0x77 [sctp]
> <5>  [<c01555a4>] cache_grow+0x140/0x233
> <5>  [<e0c26ba1>] sctp_endpoint_bh_rcv+0xc5/0x108 [sctp]
> <5>  [<e0c2b863>] sctp_inq_push+0xe/0x10 [sctp]
> <5>  [<e0c34600>] sctp_rcv+0x454/0x509 [sctp]
> <5>  [<e084e017>] ipt_hook+0x17/0x1c [iptable_filter]
> <5>  [<c02d005e>] nf_iterate+0x40/0x81
> <5>  [<c02e0bb9>] ip_local_deliver_finish+0x0/0x151
> <5>  [<c02e0c7f>] ip_local_deliver_finish+0xc6/0x151
> <5>  [<c02d0362>] nf_hook_slow+0x83/0xb5
> <5>  [<c02e0bb2>] ip_local_deliver+0x1a2/0x1a9
> <5>  [<c02e0bb9>] ip_local_deliver_finish+0x0/0x151
> <5>  [<c02e103e>] ip_rcv+0x334/0x3b4
> <5>  [<c02c66fd>] netif_receive_skb+0x320/0x35b
> <5>  [<e0a0928b>] init_stall_timer+0x67/0x6a [uhci_hcd]
> <5>  [<c02c67a4>] process_backlog+0x6c/0xd9
> <5>  [<c02c690f>] net_rx_action+0xfe/0x1f8
> <5>  [<c012a7b1>] __do_softirq+0x35/0x79
> <5>  [<c0107efb>] handle_IRQ_event+0x0/0x4f
> <5>  [<c01094de>] do_softirq+0x46/0x4d
> 
> Its an skb_over_panic BUG halt that results from processing an init chunk in
> which too many of its variable length parameters are in some way malformed.
> 
> The problem is in sctp_process_unk_param:
> if (NULL == *errp)
> 	*errp = sctp_make_op_error_space(asoc, chunk,
> 					 ntohs(chunk->chunk_hdr->length));
> 
> 	if (*errp) {
> 		sctp_init_cause(*errp, SCTP_ERROR_UNKNOWN_PARAM,
> 				 WORD_ROUND(ntohs(param.p->length)));
> 		sctp_addto_chunk(*errp,
> 			WORD_ROUND(ntohs(param.p->length)),
> 				  param.v);
> 
> When we allocate an error chunk, we assume that the worst case scenario requires
> that we have chunk_hdr->length data allocated, which would be correct nominally,
> given that we call sctp_addto_chunk for the violating parameter.  Unfortunately,
> we also, in sctp_init_cause insert a sctp_errhdr_t structure into the error
> chunk, so the worst case situation in which all parameters are in violation
> requires chunk_hdr->length+(sizeof(sctp_errhdr_t)*param_count) bytes of data.
> 
> The result of this error is that a deliberately malformed packet sent to a
> listening host can cause a remote DOS, described in CVE-2010-1173:
> http://cve.mitre.org/cgi-bin/cvename.cgi?name=2010-1173
> 
> I've tested the below fix and confirmed that it fixes the issue.  It
> pre-allocates the error chunk in sctp_verify_init, where we are able to count
> the total number of variable length parameters, so we know how many error
> headers we might need.  Then we simply use that chunk, if we find an error, or
> discard/free it if all the parameters are valid.  Applies on top of the
> lksctp-dev tree
> 
> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> 
> 
>  sm_make_chunk.c |   24 ++++++++++++++++++++++--
>  1 file changed, 22 insertions(+), 2 deletions(-)
> 
> 
> diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
> index f592163..990457b 100644
> --- a/net/sctp/sm_make_chunk.c
> +++ b/net/sctp/sm_make_chunk.c
> @@ -2134,6 +2134,8 @@ int sctp_verify_init(const struct sctp_association *asoc,
>  	union sctp_params param;
>  	int has_cookie = 0;
>  	int result;
> +	unsigned int param_cnt;
> +	unsigned int len;
>  
>  	/* Verify stream values are non-zero. */
>  	if ((0 == peer_init->init_hdr.num_outbound_streams) ||
> @@ -2149,6 +2151,7 @@ int sctp_verify_init(const struct sctp_association *asoc,
>  
>  		if (SCTP_PARAM_STATE_COOKIE == param.p->type)
>  			has_cookie = 1;
> +		param_cnt++;
>  
>  	} /* for (loop through all parameters) */
>  
> @@ -2169,6 +2172,20 @@ int sctp_verify_init(const struct sctp_association *asoc,
>  		return sctp_process_missing_param(asoc, SCTP_PARAM_STATE_COOKIE,
>  						  chunk, errp);
>  
> +	if (!*errp) {
> +		/*
> +		 * Pre-allocate the error packet here
> +		 * we do this as we need to reserve space
> +		 * for the worst case scenario in which 
> +		 * every parameter is in error and needs 
> +		 * an errhdr attached to it
> +		 */
> +		len = ntohs(chunk->chunk_hdr->length);
> +		len += sizeof(sctp_errhdr_t)*param_cnt;
> +
> +		*errp = sctp_make_op_error_space(asoc, chunk, len);
> +	}
> +
>  	/* Verify all the variable length parameters */
>  	sctp_walk_params(param, peer_init, init_hdr.params) {
>  
> @@ -2176,9 +2193,11 @@ int sctp_verify_init(const struct sctp_association *asoc,
>  		switch (result) {
>  		    case SCTP_IERROR_ABORT:
>  		    case SCTP_IERROR_NOMEM:
> -				return 0;
>  		    case SCTP_IERROR_ERROR:
> -				return 1;
> +				len = ntohs((*errp)->chunk_hdr->length);
> +				if ((*errp) && (len == sizeof(sctp_chunkhdr_t)))
> +					sctp_chunk_free(*errp);
> +				return (result == SCTP_IERROR_ERROR) ? 1 : 0;
>  		    case SCTP_IERROR_NO_ERROR:
>  		    default:
>  				break;
> @@ -2186,6 +2205,7 @@ int sctp_verify_init(const struct sctp_association *asoc,
>  
>  	} /* for (loop through all parameters) */
>  
> +	sctp_chunk_free(*errp);
>  	return 1;
>  }
>  
> --
> 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]: sctp: Fix skb_over_panic resulting from multiple invalid parameter errors (CVE-2010-1173)

[Vlad Yasevich]

Vlad Yasevich wrote:
> I have this patch and a few others already queued.

Scratch that.  I totally misread the description and the patch.

-vlad
> 
> I was planning on sending these today for stable.
> 
> Here is the full list of stable patches I have:
> 
> sctp: Fix oops when sending queued ASCONF chunks
> sctp: fix to calc the INIT/INIT-ACK chunk length correctly is set
> sctp: per_cpu variables should be in bh_disabled section
> sctp: fix potential reference of a freed pointer
> sctp: avoid irq lock inversion while call sk->sk_data_ready()
> 
> -vlad
> 
> Neil Horman wrote:
>> Hey-
>> 	Recently, it was reported to me that the kernel could oops in the
>> following way:
>>
>> <5> kernel BUG at net/core/skbuff.c:91!
>> <5> invalid operand: 0000 [#1]
>> <5> Modules linked in: sctp netconsole nls_utf8 autofs4 sunrpc iptable_filter
>> ip_tables cpufreq_powersave parport_pc lp parport vmblock(U) vsock(U) vmci(U)
>> vmxnet(U) vmmemctl(U) vmhgfs(U) acpiphp dm_mirror dm_mod button battery ac md5
>> ipv6 uhci_hcd ehci_hcd snd_ens1371 snd_rawmidi snd_seq_device snd_pcm_oss
>> snd_mixer_oss snd_pcm snd_timer snd_page_alloc snd_ac97_codec snd soundcore
>> pcnet32 mii floppy ext3 jbd ata_piix libata mptscsih mptsas mptspi mptscsi
>> mptbase sd_mod scsi_mod
>> <5> CPU:    0
>> <5> EIP:    0060:[<c02bff27>]    Not tainted VLI
>> <5> EFLAGS: 00010216   (2.6.9-89.0.25.EL) 
>> <5> EIP is at skb_over_panic+0x1f/0x2d
>> <5> eax: 0000002c   ebx: c033f461   ecx: c0357d96   edx: c040fd44
>> <5> esi: c033f461   edi: df653280   ebp: 00000000   esp: c040fd40
>> <5> ds: 007b   es: 007b   ss: 0068
>> <5> Process swapper (pid: 0, threadinfo=c040f000 task=c0370be0)
>> <5> Stack: c0357d96 e0c29478 00000084 00000004 c033f461 df653280 d7883180
>> e0c2947d 
>> <5>        00000000 00000080 df653490 00000004 de4f1ac0 de4f1ac0 00000004
>> df653490 
>> <5>        00000001 e0c2877a 08000800 de4f1ac0 df653490 00000000 e0c29d2e
>> 00000004 
>> <5> Call Trace:
>> <5>  [<e0c29478>] sctp_addto_chunk+0xb0/0x128 [sctp]
>> <5>  [<e0c2947d>] sctp_addto_chunk+0xb5/0x128 [sctp]
>> <5>  [<e0c2877a>] sctp_init_cause+0x3f/0x47 [sctp]
>> <5>  [<e0c29d2e>] sctp_process_unk_param+0xac/0xb8 [sctp]
>> <5>  [<e0c29e90>] sctp_verify_init+0xcc/0x134 [sctp]
>> <5>  [<e0c20322>] sctp_sf_do_5_1B_init+0x83/0x28e [sctp]
>> <5>  [<e0c25333>] sctp_do_sm+0x41/0x77 [sctp]
>> <5>  [<c01555a4>] cache_grow+0x140/0x233
>> <5>  [<e0c26ba1>] sctp_endpoint_bh_rcv+0xc5/0x108 [sctp]
>> <5>  [<e0c2b863>] sctp_inq_push+0xe/0x10 [sctp]
>> <5>  [<e0c34600>] sctp_rcv+0x454/0x509 [sctp]
>> <5>  [<e084e017>] ipt_hook+0x17/0x1c [iptable_filter]
>> <5>  [<c02d005e>] nf_iterate+0x40/0x81
>> <5>  [<c02e0bb9>] ip_local_deliver_finish+0x0/0x151
>> <5>  [<c02e0c7f>] ip_local_deliver_finish+0xc6/0x151
>> <5>  [<c02d0362>] nf_hook_slow+0x83/0xb5
>> <5>  [<c02e0bb2>] ip_local_deliver+0x1a2/0x1a9
>> <5>  [<c02e0bb9>] ip_local_deliver_finish+0x0/0x151
>> <5>  [<c02e103e>] ip_rcv+0x334/0x3b4
>> <5>  [<c02c66fd>] netif_receive_skb+0x320/0x35b
>> <5>  [<e0a0928b>] init_stall_timer+0x67/0x6a [uhci_hcd]
>> <5>  [<c02c67a4>] process_backlog+0x6c/0xd9
>> <5>  [<c02c690f>] net_rx_action+0xfe/0x1f8
>> <5>  [<c012a7b1>] __do_softirq+0x35/0x79
>> <5>  [<c0107efb>] handle_IRQ_event+0x0/0x4f
>> <5>  [<c01094de>] do_softirq+0x46/0x4d
>>
>> Its an skb_over_panic BUG halt that results from processing an init chunk in
>> which too many of its variable length parameters are in some way malformed.
>>
>> The problem is in sctp_process_unk_param:
>> if (NULL == *errp)
>> 	*errp = sctp_make_op_error_space(asoc, chunk,
>> 					 ntohs(chunk->chunk_hdr->length));
>>
>> 	if (*errp) {
>> 		sctp_init_cause(*errp, SCTP_ERROR_UNKNOWN_PARAM,
>> 				 WORD_ROUND(ntohs(param.p->length)));
>> 		sctp_addto_chunk(*errp,
>> 			WORD_ROUND(ntohs(param.p->length)),
>> 				  param.v);
>>
>> When we allocate an error chunk, we assume that the worst case scenario requires
>> that we have chunk_hdr->length data allocated, which would be correct nominally,
>> given that we call sctp_addto_chunk for the violating parameter.  Unfortunately,
>> we also, in sctp_init_cause insert a sctp_errhdr_t structure into the error
>> chunk, so the worst case situation in which all parameters are in violation
>> requires chunk_hdr->length+(sizeof(sctp_errhdr_t)*param_count) bytes of data.
>>
>> The result of this error is that a deliberately malformed packet sent to a
>> listening host can cause a remote DOS, described in CVE-2010-1173:
>> http://cve.mitre.org/cgi-bin/cvename.cgi?name=2010-1173
>>
>> I've tested the below fix and confirmed that it fixes the issue.  It
>> pre-allocates the error chunk in sctp_verify_init, where we are able to count
>> the total number of variable length parameters, so we know how many error
>> headers we might need.  Then we simply use that chunk, if we find an error, or
>> discard/free it if all the parameters are valid.  Applies on top of the
>> lksctp-dev tree
>>
>> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
>>
>>
>>  sm_make_chunk.c |   24 ++++++++++++++++++++++--
>>  1 file changed, 22 insertions(+), 2 deletions(-)
>>
>>
>> diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
>> index f592163..990457b 100644
>> --- a/net/sctp/sm_make_chunk.c
>> +++ b/net/sctp/sm_make_chunk.c
>> @@ -2134,6 +2134,8 @@ int sctp_verify_init(const struct sctp_association *asoc,
>>  	union sctp_params param;
>>  	int has_cookie = 0;
>>  	int result;
>> +	unsigned int param_cnt;
>> +	unsigned int len;
>>  
>>  	/* Verify stream values are non-zero. */
>>  	if ((0 == peer_init->init_hdr.num_outbound_streams) ||
>> @@ -2149,6 +2151,7 @@ int sctp_verify_init(const struct sctp_association *asoc,
>>  
>>  		if (SCTP_PARAM_STATE_COOKIE == param.p->type)
>>  			has_cookie = 1;
>> +		param_cnt++;
>>  
>>  	} /* for (loop through all parameters) */
>>  
>> @@ -2169,6 +2172,20 @@ int sctp_verify_init(const struct sctp_association *asoc,
>>  		return sctp_process_missing_param(asoc, SCTP_PARAM_STATE_COOKIE,
>>  						  chunk, errp);
>>  
>> +	if (!*errp) {
>> +		/*
>> +		 * Pre-allocate the error packet here
>> +		 * we do this as we need to reserve space
>> +		 * for the worst case scenario in which 
>> +		 * every parameter is in error and needs 
>> +		 * an errhdr attached to it
>> +		 */
>> +		len = ntohs(chunk->chunk_hdr->length);
>> +		len += sizeof(sctp_errhdr_t)*param_cnt;
>> +
>> +		*errp = sctp_make_op_error_space(asoc, chunk, len);
>> +	}
>> +
>>  	/* Verify all the variable length parameters */
>>  	sctp_walk_params(param, peer_init, init_hdr.params) {
>>  
>> @@ -2176,9 +2193,11 @@ int sctp_verify_init(const struct sctp_association *asoc,
>>  		switch (result) {
>>  		    case SCTP_IERROR_ABORT:
>>  		    case SCTP_IERROR_NOMEM:
>> -				return 0;
>>  		    case SCTP_IERROR_ERROR:
>> -				return 1;
>> +				len = ntohs((*errp)->chunk_hdr->length);
>> +				if ((*errp) && (len == sizeof(sctp_chunkhdr_t)))
>> +					sctp_chunk_free(*errp);
>> +				return (result == SCTP_IERROR_ERROR) ? 1 : 0;
>>  		    case SCTP_IERROR_NO_ERROR:
>>  		    default:
>>  				break;
>> @@ -2186,6 +2205,7 @@ int sctp_verify_init(const struct sctp_association *asoc,
>>  
>>  	} /* for (loop through all parameters) */
>>  
>> +	sctp_chunk_free(*errp);
>>  	return 1;
>>  }
>>  
>> --
>> 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]: sctp: Fix skb_over_panic resulting from multiple invalid parameter errors (CVE-2010-1173)

[Neil Horman]

On Wed, Apr 28, 2010 at 10:00:37AM -0400, Vlad Yasevich wrote:
> I have this patch and a few others already queued.
> 
> I was planning on sending these today for stable.
> 
> Here is the full list of stable patches I have:
> 
> sctp: Fix oops when sending queued ASCONF chunks
> sctp: fix to calc the INIT/INIT-ACK chunk length correctly is set
> sctp: per_cpu variables should be in bh_disabled section
> sctp: fix potential reference of a freed pointer
> sctp: avoid irq lock inversion while call sk->sk_data_ready()
> 
> -vlad
> 
Are you sure?  this oops looks _very_ simmilar to the INIT/INIT-ACK length
calculation oops described above, but is in fact different, and requires this
patch, from what I can see.  The right fix might be in the ASCONF chunk patch
you list above, but I don't see that in your tree at the moment, so I can't be
sure.

Neil

> Neil Horman wrote:
> > Hey-
> > 	Recently, it was reported to me that the kernel could oops in the
> > following way:
> > 
> > <5> kernel BUG at net/core/skbuff.c:91!
> > <5> invalid operand: 0000 [#1]
> > <5> Modules linked in: sctp netconsole nls_utf8 autofs4 sunrpc iptable_filter
> > ip_tables cpufreq_powersave parport_pc lp parport vmblock(U) vsock(U) vmci(U)
> > vmxnet(U) vmmemctl(U) vmhgfs(U) acpiphp dm_mirror dm_mod button battery ac md5
> > ipv6 uhci_hcd ehci_hcd snd_ens1371 snd_rawmidi snd_seq_device snd_pcm_oss
> > snd_mixer_oss snd_pcm snd_timer snd_page_alloc snd_ac97_codec snd soundcore
> > pcnet32 mii floppy ext3 jbd ata_piix libata mptscsih mptsas mptspi mptscsi
> > mptbase sd_mod scsi_mod
> > <5> CPU:    0
> > <5> EIP:    0060:[<c02bff27>]    Not tainted VLI
> > <5> EFLAGS: 00010216   (2.6.9-89.0.25.EL) 
> > <5> EIP is at skb_over_panic+0x1f/0x2d
> > <5> eax: 0000002c   ebx: c033f461   ecx: c0357d96   edx: c040fd44
> > <5> esi: c033f461   edi: df653280   ebp: 00000000   esp: c040fd40
> > <5> ds: 007b   es: 007b   ss: 0068
> > <5> Process swapper (pid: 0, threadinfo=c040f000 task=c0370be0)
> > <5> Stack: c0357d96 e0c29478 00000084 00000004 c033f461 df653280 d7883180
> > e0c2947d 
> > <5>        00000000 00000080 df653490 00000004 de4f1ac0 de4f1ac0 00000004
> > df653490 
> > <5>        00000001 e0c2877a 08000800 de4f1ac0 df653490 00000000 e0c29d2e
> > 00000004 
> > <5> Call Trace:
> > <5>  [<e0c29478>] sctp_addto_chunk+0xb0/0x128 [sctp]
> > <5>  [<e0c2947d>] sctp_addto_chunk+0xb5/0x128 [sctp]
> > <5>  [<e0c2877a>] sctp_init_cause+0x3f/0x47 [sctp]
> > <5>  [<e0c29d2e>] sctp_process_unk_param+0xac/0xb8 [sctp]
> > <5>  [<e0c29e90>] sctp_verify_init+0xcc/0x134 [sctp]
> > <5>  [<e0c20322>] sctp_sf_do_5_1B_init+0x83/0x28e [sctp]
> > <5>  [<e0c25333>] sctp_do_sm+0x41/0x77 [sctp]
> > <5>  [<c01555a4>] cache_grow+0x140/0x233
> > <5>  [<e0c26ba1>] sctp_endpoint_bh_rcv+0xc5/0x108 [sctp]
> > <5>  [<e0c2b863>] sctp_inq_push+0xe/0x10 [sctp]
> > <5>  [<e0c34600>] sctp_rcv+0x454/0x509 [sctp]
> > <5>  [<e084e017>] ipt_hook+0x17/0x1c [iptable_filter]
> > <5>  [<c02d005e>] nf_iterate+0x40/0x81
> > <5>  [<c02e0bb9>] ip_local_deliver_finish+0x0/0x151
> > <5>  [<c02e0c7f>] ip_local_deliver_finish+0xc6/0x151
> > <5>  [<c02d0362>] nf_hook_slow+0x83/0xb5
> > <5>  [<c02e0bb2>] ip_local_deliver+0x1a2/0x1a9
> > <5>  [<c02e0bb9>] ip_local_deliver_finish+0x0/0x151
> > <5>  [<c02e103e>] ip_rcv+0x334/0x3b4
> > <5>  [<c02c66fd>] netif_receive_skb+0x320/0x35b
> > <5>  [<e0a0928b>] init_stall_timer+0x67/0x6a [uhci_hcd]
> > <5>  [<c02c67a4>] process_backlog+0x6c/0xd9
> > <5>  [<c02c690f>] net_rx_action+0xfe/0x1f8
> > <5>  [<c012a7b1>] __do_softirq+0x35/0x79
> > <5>  [<c0107efb>] handle_IRQ_event+0x0/0x4f
> > <5>  [<c01094de>] do_softirq+0x46/0x4d
> > 
> > Its an skb_over_panic BUG halt that results from processing an init chunk in
> > which too many of its variable length parameters are in some way malformed.
> > 
> > The problem is in sctp_process_unk_param:
> > if (NULL == *errp)
> > 	*errp = sctp_make_op_error_space(asoc, chunk,
> > 					 ntohs(chunk->chunk_hdr->length));
> > 
> > 	if (*errp) {
> > 		sctp_init_cause(*errp, SCTP_ERROR_UNKNOWN_PARAM,
> > 				 WORD_ROUND(ntohs(param.p->length)));
> > 		sctp_addto_chunk(*errp,
> > 			WORD_ROUND(ntohs(param.p->length)),
> > 				  param.v);
> > 
> > When we allocate an error chunk, we assume that the worst case scenario requires
> > that we have chunk_hdr->length data allocated, which would be correct nominally,
> > given that we call sctp_addto_chunk for the violating parameter.  Unfortunately,
> > we also, in sctp_init_cause insert a sctp_errhdr_t structure into the error
> > chunk, so the worst case situation in which all parameters are in violation
> > requires chunk_hdr->length+(sizeof(sctp_errhdr_t)*param_count) bytes of data.
> > 
> > The result of this error is that a deliberately malformed packet sent to a
> > listening host can cause a remote DOS, described in CVE-2010-1173:
> > http://cve.mitre.org/cgi-bin/cvename.cgi?name=2010-1173
> > 
> > I've tested the below fix and confirmed that it fixes the issue.  It
> > pre-allocates the error chunk in sctp_verify_init, where we are able to count
> > the total number of variable length parameters, so we know how many error
> > headers we might need.  Then we simply use that chunk, if we find an error, or
> > discard/free it if all the parameters are valid.  Applies on top of the
> > lksctp-dev tree
> > 
> > Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> > 
> > 
> >  sm_make_chunk.c |   24 ++++++++++++++++++++++--
> >  1 file changed, 22 insertions(+), 2 deletions(-)
> > 
> > 
> > diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
> > index f592163..990457b 100644
> > --- a/net/sctp/sm_make_chunk.c
> > +++ b/net/sctp/sm_make_chunk.c
> > @@ -2134,6 +2134,8 @@ int sctp_verify_init(const struct sctp_association *asoc,
> >  	union sctp_params param;
> >  	int has_cookie = 0;
> >  	int result;
> > +	unsigned int param_cnt;
> > +	unsigned int len;
> >  
> >  	/* Verify stream values are non-zero. */
> >  	if ((0 == peer_init->init_hdr.num_outbound_streams) ||
> > @@ -2149,6 +2151,7 @@ int sctp_verify_init(const struct sctp_association *asoc,
> >  
> >  		if (SCTP_PARAM_STATE_COOKIE == param.p->type)
> >  			has_cookie = 1;
> > +		param_cnt++;
> >  
> >  	} /* for (loop through all parameters) */
> >  
> > @@ -2169,6 +2172,20 @@ int sctp_verify_init(const struct sctp_association *asoc,
> >  		return sctp_process_missing_param(asoc, SCTP_PARAM_STATE_COOKIE,
> >  						  chunk, errp);
> >  
> > +	if (!*errp) {
> > +		/*
> > +		 * Pre-allocate the error packet here
> > +		 * we do this as we need to reserve space
> > +		 * for the worst case scenario in which 
> > +		 * every parameter is in error and needs 
> > +		 * an errhdr attached to it
> > +		 */
> > +		len = ntohs(chunk->chunk_hdr->length);
> > +		len += sizeof(sctp_errhdr_t)*param_cnt;
> > +
> > +		*errp = sctp_make_op_error_space(asoc, chunk, len);
> > +	}
> > +
> >  	/* Verify all the variable length parameters */
> >  	sctp_walk_params(param, peer_init, init_hdr.params) {
> >  
> > @@ -2176,9 +2193,11 @@ int sctp_verify_init(const struct sctp_association *asoc,
> >  		switch (result) {
> >  		    case SCTP_IERROR_ABORT:
> >  		    case SCTP_IERROR_NOMEM:
> > -				return 0;
> >  		    case SCTP_IERROR_ERROR:
> > -				return 1;
> > +				len = ntohs((*errp)->chunk_hdr->length);
> > +				if ((*errp) && (len == sizeof(sctp_chunkhdr_t)))
> > +					sctp_chunk_free(*errp);
> > +				return (result == SCTP_IERROR_ERROR) ? 1 : 0;
> >  		    case SCTP_IERROR_NO_ERROR:
> >  		    default:
> >  				break;
> > @@ -2186,6 +2205,7 @@ int sctp_verify_init(const struct sctp_association *asoc,
> >  
> >  	} /* for (loop through all parameters) */
> >  
> > +	sctp_chunk_free(*errp);
> >  	return 1;
> >  }
> >  
> > --
> > 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]: sctp: Fix skb_over_panic resulting from multiple invalid parameter errors (CVE-2010-1173)

[Vlad Yasevich]

Neil Horman wrote:
> On Wed, Apr 28, 2010 at 10:00:37AM -0400, Vlad Yasevich wrote:
>> I have this patch and a few others already queued.
>>
>> I was planning on sending these today for stable.
>>
>> Here is the full list of stable patches I have:
>>
>> sctp: Fix oops when sending queued ASCONF chunks
>> sctp: fix to calc the INIT/INIT-ACK chunk length correctly is set
>> sctp: per_cpu variables should be in bh_disabled section
>> sctp: fix potential reference of a freed pointer
>> sctp: avoid irq lock inversion while call sk->sk_data_ready()
>>
>> -vlad
>>
> Are you sure?  this oops looks _very_ simmilar to the INIT/INIT-ACK length
> calculation oops described above, but is in fact different, and requires this
> patch, from what I can see.  The right fix might be in the ASCONF chunk patch
> you list above, but I don't see that in your tree at the moment, so I can't be
> sure.

As I said, I totally goofed when reading the description and I apologize.
However, I do one comment regarding the patch.

If the bad packet is REALLY long (I mean close to 65K IP limit), then
we'll end up allocating a supper huge skb in this case and potentially exceed
the IP length limitation.  Section 11.4 of rfc 4960 allows us to omit some
errors and limit the size of the packet.

I would recommend limiting this to MTU worth of potentiall errors.  This is
on top of what the INIT-ACK is going to carry, so at most we'll sent 2 MTUs
worth.  That's still a potential by amplification attack, but it's somewhat
mitigated.

Of course now we have to handle the case of checking for space before adding
an error cause. :)

-vlad

> 
> Neil
> 
>> Neil Horman wrote:
>>> Hey-
>>> 	Recently, it was reported to me that the kernel could oops in the
>>> following way:
>>>
>>> <5> kernel BUG at net/core/skbuff.c:91!
>>> <5> invalid operand: 0000 [#1]
>>> <5> Modules linked in: sctp netconsole nls_utf8 autofs4 sunrpc iptable_filter
>>> ip_tables cpufreq_powersave parport_pc lp parport vmblock(U) vsock(U) vmci(U)
>>> vmxnet(U) vmmemctl(U) vmhgfs(U) acpiphp dm_mirror dm_mod button battery ac md5
>>> ipv6 uhci_hcd ehci_hcd snd_ens1371 snd_rawmidi snd_seq_device snd_pcm_oss
>>> snd_mixer_oss snd_pcm snd_timer snd_page_alloc snd_ac97_codec snd soundcore
>>> pcnet32 mii floppy ext3 jbd ata_piix libata mptscsih mptsas mptspi mptscsi
>>> mptbase sd_mod scsi_mod
>>> <5> CPU:    0
>>> <5> EIP:    0060:[<c02bff27>]    Not tainted VLI
>>> <5> EFLAGS: 00010216   (2.6.9-89.0.25.EL) 
>>> <5> EIP is at skb_over_panic+0x1f/0x2d
>>> <5> eax: 0000002c   ebx: c033f461   ecx: c0357d96   edx: c040fd44
>>> <5> esi: c033f461   edi: df653280   ebp: 00000000   esp: c040fd40
>>> <5> ds: 007b   es: 007b   ss: 0068
>>> <5> Process swapper (pid: 0, threadinfo=c040f000 task=c0370be0)
>>> <5> Stack: c0357d96 e0c29478 00000084 00000004 c033f461 df653280 d7883180
>>> e0c2947d 
>>> <5>        00000000 00000080 df653490 00000004 de4f1ac0 de4f1ac0 00000004
>>> df653490 
>>> <5>        00000001 e0c2877a 08000800 de4f1ac0 df653490 00000000 e0c29d2e
>>> 00000004 
>>> <5> Call Trace:
>>> <5>  [<e0c29478>] sctp_addto_chunk+0xb0/0x128 [sctp]
>>> <5>  [<e0c2947d>] sctp_addto_chunk+0xb5/0x128 [sctp]
>>> <5>  [<e0c2877a>] sctp_init_cause+0x3f/0x47 [sctp]
>>> <5>  [<e0c29d2e>] sctp_process_unk_param+0xac/0xb8 [sctp]
>>> <5>  [<e0c29e90>] sctp_verify_init+0xcc/0x134 [sctp]
>>> <5>  [<e0c20322>] sctp_sf_do_5_1B_init+0x83/0x28e [sctp]
>>> <5>  [<e0c25333>] sctp_do_sm+0x41/0x77 [sctp]
>>> <5>  [<c01555a4>] cache_grow+0x140/0x233
>>> <5>  [<e0c26ba1>] sctp_endpoint_bh_rcv+0xc5/0x108 [sctp]
>>> <5>  [<e0c2b863>] sctp_inq_push+0xe/0x10 [sctp]
>>> <5>  [<e0c34600>] sctp_rcv+0x454/0x509 [sctp]
>>> <5>  [<e084e017>] ipt_hook+0x17/0x1c [iptable_filter]
>>> <5>  [<c02d005e>] nf_iterate+0x40/0x81
>>> <5>  [<c02e0bb9>] ip_local_deliver_finish+0x0/0x151
>>> <5>  [<c02e0c7f>] ip_local_deliver_finish+0xc6/0x151
>>> <5>  [<c02d0362>] nf_hook_slow+0x83/0xb5
>>> <5>  [<c02e0bb2>] ip_local_deliver+0x1a2/0x1a9
>>> <5>  [<c02e0bb9>] ip_local_deliver_finish+0x0/0x151
>>> <5>  [<c02e103e>] ip_rcv+0x334/0x3b4
>>> <5>  [<c02c66fd>] netif_receive_skb+0x320/0x35b
>>> <5>  [<e0a0928b>] init_stall_timer+0x67/0x6a [uhci_hcd]
>>> <5>  [<c02c67a4>] process_backlog+0x6c/0xd9
>>> <5>  [<c02c690f>] net_rx_action+0xfe/0x1f8
>>> <5>  [<c012a7b1>] __do_softirq+0x35/0x79
>>> <5>  [<c0107efb>] handle_IRQ_event+0x0/0x4f
>>> <5>  [<c01094de>] do_softirq+0x46/0x4d
>>>
>>> Its an skb_over_panic BUG halt that results from processing an init chunk in
>>> which too many of its variable length parameters are in some way malformed.
>>>
>>> The problem is in sctp_process_unk_param:
>>> if (NULL == *errp)
>>> 	*errp = sctp_make_op_error_space(asoc, chunk,
>>> 					 ntohs(chunk->chunk_hdr->length));
>>>
>>> 	if (*errp) {
>>> 		sctp_init_cause(*errp, SCTP_ERROR_UNKNOWN_PARAM,
>>> 				 WORD_ROUND(ntohs(param.p->length)));
>>> 		sctp_addto_chunk(*errp,
>>> 			WORD_ROUND(ntohs(param.p->length)),
>>> 				  param.v);
>>>
>>> When we allocate an error chunk, we assume that the worst case scenario requires
>>> that we have chunk_hdr->length data allocated, which would be correct nominally,
>>> given that we call sctp_addto_chunk for the violating parameter.  Unfortunately,
>>> we also, in sctp_init_cause insert a sctp_errhdr_t structure into the error
>>> chunk, so the worst case situation in which all parameters are in violation
>>> requires chunk_hdr->length+(sizeof(sctp_errhdr_t)*param_count) bytes of data.
>>>
>>> The result of this error is that a deliberately malformed packet sent to a
>>> listening host can cause a remote DOS, described in CVE-2010-1173:
>>> http://cve.mitre.org/cgi-bin/cvename.cgi?name=2010-1173
>>>
>>> I've tested the below fix and confirmed that it fixes the issue.  It
>>> pre-allocates the error chunk in sctp_verify_init, where we are able to count
>>> the total number of variable length parameters, so we know how many error
>>> headers we might need.  Then we simply use that chunk, if we find an error, or
>>> discard/free it if all the parameters are valid.  Applies on top of the
>>> lksctp-dev tree
>>>
>>> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
>>>
>>>
>>>  sm_make_chunk.c |   24 ++++++++++++++++++++++--
>>>  1 file changed, 22 insertions(+), 2 deletions(-)
>>>
>>>
>>> diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
>>> index f592163..990457b 100644
>>> --- a/net/sctp/sm_make_chunk.c
>>> +++ b/net/sctp/sm_make_chunk.c
>>> @@ -2134,6 +2134,8 @@ int sctp_verify_init(const struct sctp_association *asoc,
>>>  	union sctp_params param;
>>>  	int has_cookie = 0;
>>>  	int result;
>>> +	unsigned int param_cnt;
>>> +	unsigned int len;
>>>  
>>>  	/* Verify stream values are non-zero. */
>>>  	if ((0 == peer_init->init_hdr.num_outbound_streams) ||
>>> @@ -2149,6 +2151,7 @@ int sctp_verify_init(const struct sctp_association *asoc,
>>>  
>>>  		if (SCTP_PARAM_STATE_COOKIE == param.p->type)
>>>  			has_cookie = 1;
>>> +		param_cnt++;
>>>  
>>>  	} /* for (loop through all parameters) */
>>>  
>>> @@ -2169,6 +2172,20 @@ int sctp_verify_init(const struct sctp_association *asoc,
>>>  		return sctp_process_missing_param(asoc, SCTP_PARAM_STATE_COOKIE,
>>>  						  chunk, errp);
>>>  
>>> +	if (!*errp) {
>>> +		/*
>>> +		 * Pre-allocate the error packet here
>>> +		 * we do this as we need to reserve space
>>> +		 * for the worst case scenario in which 
>>> +		 * every parameter is in error and needs 
>>> +		 * an errhdr attached to it
>>> +		 */
>>> +		len = ntohs(chunk->chunk_hdr->length);
>>> +		len += sizeof(sctp_errhdr_t)*param_cnt;
>>> +
>>> +		*errp = sctp_make_op_error_space(asoc, chunk, len);
>>> +	}
>>> +
>>>  	/* Verify all the variable length parameters */
>>>  	sctp_walk_params(param, peer_init, init_hdr.params) {
>>>  
>>> @@ -2176,9 +2193,11 @@ int sctp_verify_init(const struct sctp_association *asoc,
>>>  		switch (result) {
>>>  		    case SCTP_IERROR_ABORT:
>>>  		    case SCTP_IERROR_NOMEM:
>>> -				return 0;
>>>  		    case SCTP_IERROR_ERROR:
>>> -				return 1;
>>> +				len = ntohs((*errp)->chunk_hdr->length);
>>> +				if ((*errp) && (len == sizeof(sctp_chunkhdr_t)))
>>> +					sctp_chunk_free(*errp);
>>> +				return (result == SCTP_IERROR_ERROR) ? 1 : 0;
>>>  		    case SCTP_IERROR_NO_ERROR:
>>>  		    default:
>>>  				break;
>>> @@ -2186,6 +2205,7 @@ int sctp_verify_init(const struct sctp_association *asoc,
>>>  
>>>  	} /* for (loop through all parameters) */
>>>  
>>> +	sctp_chunk_free(*errp);
>>>  	return 1;
>>>  }
>>>  
>>> --
>>> 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]: sctp: Fix skb_over_panic resulting from multiple invalid parameter errors (CVE-2010-1173) (v2)

[Neil Horman]

Ok, version 2.

Change notes:

1) in sctp_verify_init, when pre-allocating error chunk, limit size to pathmtu
of init chunk, or 1500 bytes (DEFAULT_MAXSEGMENT) if no pathmtu is discovered
yet or is otherwise unknown.

2) in process_unk_param, check to make sure we can add the next error chunk,
plus data in prior to adding.  If there isn't enough room, discard it, as per
rfc allowances.

Summary:
	Recently, it was reported to me that the kernel could oops in the
following way:

<5> kernel BUG at net/core/skbuff.c:91!
<5> invalid operand: 0000 [#1]
<5> Modules linked in: sctp netconsole nls_utf8 autofs4 sunrpc iptable_filter
ip_tables cpufreq_powersave parport_pc lp parport vmblock(U) vsock(U) vmci(U)
vmxnet(U) vmmemctl(U) vmhgfs(U) acpiphp dm_mirror dm_mod button battery ac md5
ipv6 uhci_hcd ehci_hcd snd_ens1371 snd_rawmidi snd_seq_device snd_pcm_oss
snd_mixer_oss snd_pcm snd_timer snd_page_alloc snd_ac97_codec snd soundcore
pcnet32 mii floppy ext3 jbd ata_piix libata mptscsih mptsas mptspi mptscsi
mptbase sd_mod scsi_mod
<5> CPU:    0
<5> EIP:    0060:[<c02bff27>]    Not tainted VLI
<5> EFLAGS: 00010216   (2.6.9-89.0.25.EL) 
<5> EIP is at skb_over_panic+0x1f/0x2d
<5> eax: 0000002c   ebx: c033f461   ecx: c0357d96   edx: c040fd44
<5> esi: c033f461   edi: df653280   ebp: 00000000   esp: c040fd40
<5> ds: 007b   es: 007b   ss: 0068
<5> Process swapper (pid: 0, threadinfo=c040f000 task=c0370be0)
<5> Stack: c0357d96 e0c29478 00000084 00000004 c033f461 df653280 d7883180
e0c2947d 
<5>        00000000 00000080 df653490 00000004 de4f1ac0 de4f1ac0 00000004
df653490 
<5>        00000001 e0c2877a 08000800 de4f1ac0 df653490 00000000 e0c29d2e
00000004 
<5> Call Trace:
<5>  [<e0c29478>] sctp_addto_chunk+0xb0/0x128 [sctp]
<5>  [<e0c2947d>] sctp_addto_chunk+0xb5/0x128 [sctp]
<5>  [<e0c2877a>] sctp_init_cause+0x3f/0x47 [sctp]
<5>  [<e0c29d2e>] sctp_process_unk_param+0xac/0xb8 [sctp]
<5>  [<e0c29e90>] sctp_verify_init+0xcc/0x134 [sctp]
<5>  [<e0c20322>] sctp_sf_do_5_1B_init+0x83/0x28e [sctp]
<5>  [<e0c25333>] sctp_do_sm+0x41/0x77 [sctp]
<5>  [<c01555a4>] cache_grow+0x140/0x233
<5>  [<e0c26ba1>] sctp_endpoint_bh_rcv+0xc5/0x108 [sctp]
<5>  [<e0c2b863>] sctp_inq_push+0xe/0x10 [sctp]
<5>  [<e0c34600>] sctp_rcv+0x454/0x509 [sctp]
<5>  [<e084e017>] ipt_hook+0x17/0x1c [iptable_filter]
<5>  [<c02d005e>] nf_iterate+0x40/0x81
<5>  [<c02e0bb9>] ip_local_deliver_finish+0x0/0x151
<5>  [<c02e0c7f>] ip_local_deliver_finish+0xc6/0x151
<5>  [<c02d0362>] nf_hook_slow+0x83/0xb5
<5>  [<c02e0bb2>] ip_local_deliver+0x1a2/0x1a9
<5>  [<c02e0bb9>] ip_local_deliver_finish+0x0/0x151
<5>  [<c02e103e>] ip_rcv+0x334/0x3b4
<5>  [<c02c66fd>] netif_receive_skb+0x320/0x35b
<5>  [<e0a0928b>] init_stall_timer+0x67/0x6a [uhci_hcd]
<5>  [<c02c67a4>] process_backlog+0x6c/0xd9
<5>  [<c02c690f>] net_rx_action+0xfe/0x1f8
<5>  [<c012a7b1>] __do_softirq+0x35/0x79
<5>  [<c0107efb>] handle_IRQ_event+0x0/0x4f
<5>  [<c01094de>] do_softirq+0x46/0x4d

Its an skb_over_panic BUG halt that results from processing an init chunk in
which too many of its variable length parameters are in some way malformed.

The problem is in sctp_process_unk_param:
if (NULL == *errp)
	*errp = sctp_make_op_error_space(asoc, chunk,
					 ntohs(chunk->chunk_hdr->length));

	if (*errp) {
		sctp_init_cause(*errp, SCTP_ERROR_UNKNOWN_PARAM,
				 WORD_ROUND(ntohs(param.p->length)));
		sctp_addto_chunk(*errp,
			WORD_ROUND(ntohs(param.p->length)),
				  param.v);

When we allocate an error chunk, we assume that the worst case scenario requires
that we have chunk_hdr->length data allocated, which would be correct nominally,
given that we call sctp_addto_chunk for the violating parameter.  Unfortunately,
we also, in sctp_init_cause insert a sctp_errhdr_t structure into the error
chunk, so the worst case situation in which all parameters are in violation
requires chunk_hdr->length+(sizeof(sctp_errhdr_t)*param_count) bytes of data.

The result of this error is that a deliberately malformed packet sent to a
listening host can cause a remote DOS, described in CVE-2010-1173:
http://cve.mitre.org/cgi-bin/cvename.cgi?name=2010-1173

I've tested the below fix and confirmed that it fixes the issue.  It
pre-allocates the error chunk in sctp_verify_init, where we are able to count
the total number of variable length parameters, so we know how many error
headers we might need.  Then we simply use that chunk, if we find an error, or
discard/free it if all the parameters are valid.  Applies on top of the
lksctp-dev tree

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


 sm_make_chunk.c |   46 ++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 44 insertions(+), 2 deletions(-)


diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
index f592163..65ed098 100644
--- a/net/sctp/sm_make_chunk.c
+++ b/net/sctp/sm_make_chunk.c
@@ -1960,6 +1960,7 @@ static sctp_ierror_t sctp_process_unk_param(const struct sctp_association *asoc,
 					    struct sctp_chunk *chunk,
 					    struct sctp_chunk **errp)
 {
+	unsigned int needed_bytes;
 	int retval = SCTP_IERROR_NO_ERROR;
 
 	switch (param.p->type & SCTP_PARAM_ACTION_MASK) {
@@ -1980,6 +1981,17 @@ static sctp_ierror_t sctp_process_unk_param(const struct sctp_association *asoc,
 					ntohs(chunk->chunk_hdr->length));
 
 		if (*errp) {
+			needed_bytes = sizeof(sctp_errhdr_t) +
+				       WORD_ROUND(ntohs(param.p->length));
+
+			if (skb_tailroom(chunk->skb) < needed_bytes)
+				/*
+				 * If we're over our packet size here, don't add
+				 * the error, this is allowed by section 11.4 of
+				 * RFC 4960
+				 */
+				break;
+
 			sctp_init_cause(*errp, SCTP_ERROR_UNKNOWN_PARAM,
 					WORD_ROUND(ntohs(param.p->length)));
 			sctp_addto_chunk(*errp,
@@ -2134,6 +2146,8 @@ int sctp_verify_init(const struct sctp_association *asoc,
 	union sctp_params param;
 	int has_cookie = 0;
 	int result;
+	unsigned int param_cnt = 0;
+	unsigned int len;
 
 	/* Verify stream values are non-zero. */
 	if ((0 == peer_init->init_hdr.num_outbound_streams) ||
@@ -2149,6 +2163,7 @@ int sctp_verify_init(const struct sctp_association *asoc,
 
 		if (SCTP_PARAM_STATE_COOKIE == param.p->type)
 			has_cookie = 1;
+		param_cnt++;
 
 	} /* for (loop through all parameters) */
 
@@ -2169,6 +2184,30 @@ int sctp_verify_init(const struct sctp_association *asoc,
 		return sctp_process_missing_param(asoc, SCTP_PARAM_STATE_COOKIE,
 						  chunk, errp);
 
+	if (!*errp) {
+		/*
+		 * Pre-allocate the error packet here
+		 * we do this as we need to reserve space
+		 * for the worst case scenario in which
+		 * every parameter is in error and needs
+		 * an errhdr attached to it
+		 */
+		len = ntohs(chunk->chunk_hdr->length);
+		len += sizeof(sctp_errhdr_t)*param_cnt;
+
+		/*
+		 * We need to prevent amplification attacks that
+		 * result from sending 65K init chunks with all bad
+		 * params maliciously, so lets limit our error response
+		 * to 1 MTU worth of errors, but at least 1500 bytes
+		 * in case our pathmtu hasn't been updated yet.
+		 */
+		len = min(len, asoc ? asoc->pathmtu : SCTP_DEFAULT_MAXSEGMENT);
+		len = len ? len : SCTP_DEFAULT_MAXSEGMENT;
+
+		*errp = sctp_make_op_error_space(asoc, chunk, len);
+	}
+
 	/* Verify all the variable length parameters */
 	sctp_walk_params(param, peer_init, init_hdr.params) {
 
@@ -2176,9 +2215,11 @@ int sctp_verify_init(const struct sctp_association *asoc,
 		switch (result) {
 		    case SCTP_IERROR_ABORT:
 		    case SCTP_IERROR_NOMEM:
-				return 0;
 		    case SCTP_IERROR_ERROR:
-				return 1;
+				len = ntohs((*errp)->chunk_hdr->length);
+				if ((*errp) && (len == sizeof(sctp_chunkhdr_t)))
+					sctp_chunk_free(*errp);
+				return (result == SCTP_IERROR_ERROR) ? 1 : 0;
 		    case SCTP_IERROR_NO_ERROR:
 		    default:
 				break;
@@ -2186,6 +2227,7 @@ int sctp_verify_init(const struct sctp_association *asoc,
 
 	} /* for (loop through all parameters) */
 
+	sctp_chunk_free(*errp);
 	return 1;
 }
 

Re: [PATCH]: sctp: Fix skb_over_panic resulting from multiple invalid parameter errors (CVE-2010-1173)

[Vlad Yasevich]

Vlad Yasevich wrote:
> 
> Neil Horman wrote:
>> On Wed, Apr 28, 2010 at 10:00:37AM -0400, Vlad Yasevich wrote:
>>> I have this patch and a few others already queued.
>>>
>>> I was planning on sending these today for stable.
>>>
>>> Here is the full list of stable patches I have:
>>>
>>> sctp: Fix oops when sending queued ASCONF chunks
>>> sctp: fix to calc the INIT/INIT-ACK chunk length correctly is set
>>> sctp: per_cpu variables should be in bh_disabled section
>>> sctp: fix potential reference of a freed pointer
>>> sctp: avoid irq lock inversion while call sk->sk_data_ready()
>>>
>>> -vlad
>>>
>> Are you sure?  this oops looks _very_ simmilar to the INIT/INIT-ACK length
>> calculation oops described above, but is in fact different, and requires this
>> patch, from what I can see.  The right fix might be in the ASCONF chunk patch
>> you list above, but I don't see that in your tree at the moment, so I can't be
>> sure.
> 
> As I said, I totally goofed when reading the description and I apologize.
> However, I do one comment regarding the patch.
> 
> If the bad packet is REALLY long (I mean close to 65K IP limit), then
> we'll end up allocating a supper huge skb in this case and potentially exceed
> the IP length limitation.  Section 11.4 of rfc 4960 allows us to omit some
> errors and limit the size of the packet.
> 
> I would recommend limiting this to MTU worth of potentiall errors.  This is
> on top of what the INIT-ACK is going to carry, so at most we'll sent 2 MTUs
> worth.  That's still a potential by amplification attack, but it's somewhat
> mitigated.
> 
> Of course now we have to handle the case of checking for space before adding
> an error cause. :)
> 

Hi Neil

I am also not crazy about the pre-allocation scheme.  In the case where you have
say 100 parameters that are all 'skip' parameters, you'd end up pre-allocating a
huge buffer for absolutely nothing.

This is another point toward a fixed error chunk size and let parameter
processing allocate it when it reaches a parameter that needs an error.

-vlad
> -vlad
> 
>> Neil
>>
>>> Neil Horman wrote:
>>>> Hey-
>>>> 	Recently, it was reported to me that the kernel could oops in the
>>>> following way:
>>>>
>>>> <5> kernel BUG at net/core/skbuff.c:91!
>>>> <5> invalid operand: 0000 [#1]
>>>> <5> Modules linked in: sctp netconsole nls_utf8 autofs4 sunrpc iptable_filter
>>>> ip_tables cpufreq_powersave parport_pc lp parport vmblock(U) vsock(U) vmci(U)
>>>> vmxnet(U) vmmemctl(U) vmhgfs(U) acpiphp dm_mirror dm_mod button battery ac md5
>>>> ipv6 uhci_hcd ehci_hcd snd_ens1371 snd_rawmidi snd_seq_device snd_pcm_oss
>>>> snd_mixer_oss snd_pcm snd_timer snd_page_alloc snd_ac97_codec snd soundcore
>>>> pcnet32 mii floppy ext3 jbd ata_piix libata mptscsih mptsas mptspi mptscsi
>>>> mptbase sd_mod scsi_mod
>>>> <5> CPU:    0
>>>> <5> EIP:    0060:[<c02bff27>]    Not tainted VLI
>>>> <5> EFLAGS: 00010216   (2.6.9-89.0.25.EL) 
>>>> <5> EIP is at skb_over_panic+0x1f/0x2d
>>>> <5> eax: 0000002c   ebx: c033f461   ecx: c0357d96   edx: c040fd44
>>>> <5> esi: c033f461   edi: df653280   ebp: 00000000   esp: c040fd40
>>>> <5> ds: 007b   es: 007b   ss: 0068
>>>> <5> Process swapper (pid: 0, threadinfo=c040f000 task=c0370be0)
>>>> <5> Stack: c0357d96 e0c29478 00000084 00000004 c033f461 df653280 d7883180
>>>> e0c2947d 
>>>> <5>        00000000 00000080 df653490 00000004 de4f1ac0 de4f1ac0 00000004
>>>> df653490 
>>>> <5>        00000001 e0c2877a 08000800 de4f1ac0 df653490 00000000 e0c29d2e
>>>> 00000004 
>>>> <5> Call Trace:
>>>> <5>  [<e0c29478>] sctp_addto_chunk+0xb0/0x128 [sctp]
>>>> <5>  [<e0c2947d>] sctp_addto_chunk+0xb5/0x128 [sctp]
>>>> <5>  [<e0c2877a>] sctp_init_cause+0x3f/0x47 [sctp]
>>>> <5>  [<e0c29d2e>] sctp_process_unk_param+0xac/0xb8 [sctp]
>>>> <5>  [<e0c29e90>] sctp_verify_init+0xcc/0x134 [sctp]
>>>> <5>  [<e0c20322>] sctp_sf_do_5_1B_init+0x83/0x28e [sctp]
>>>> <5>  [<e0c25333>] sctp_do_sm+0x41/0x77 [sctp]
>>>> <5>  [<c01555a4>] cache_grow+0x140/0x233
>>>> <5>  [<e0c26ba1>] sctp_endpoint_bh_rcv+0xc5/0x108 [sctp]
>>>> <5>  [<e0c2b863>] sctp_inq_push+0xe/0x10 [sctp]
>>>> <5>  [<e0c34600>] sctp_rcv+0x454/0x509 [sctp]
>>>> <5>  [<e084e017>] ipt_hook+0x17/0x1c [iptable_filter]
>>>> <5>  [<c02d005e>] nf_iterate+0x40/0x81
>>>> <5>  [<c02e0bb9>] ip_local_deliver_finish+0x0/0x151
>>>> <5>  [<c02e0c7f>] ip_local_deliver_finish+0xc6/0x151
>>>> <5>  [<c02d0362>] nf_hook_slow+0x83/0xb5
>>>> <5>  [<c02e0bb2>] ip_local_deliver+0x1a2/0x1a9
>>>> <5>  [<c02e0bb9>] ip_local_deliver_finish+0x0/0x151
>>>> <5>  [<c02e103e>] ip_rcv+0x334/0x3b4
>>>> <5>  [<c02c66fd>] netif_receive_skb+0x320/0x35b
>>>> <5>  [<e0a0928b>] init_stall_timer+0x67/0x6a [uhci_hcd]
>>>> <5>  [<c02c67a4>] process_backlog+0x6c/0xd9
>>>> <5>  [<c02c690f>] net_rx_action+0xfe/0x1f8
>>>> <5>  [<c012a7b1>] __do_softirq+0x35/0x79
>>>> <5>  [<c0107efb>] handle_IRQ_event+0x0/0x4f
>>>> <5>  [<c01094de>] do_softirq+0x46/0x4d
>>>>
>>>> Its an skb_over_panic BUG halt that results from processing an init chunk in
>>>> which too many of its variable length parameters are in some way malformed.
>>>>
>>>> The problem is in sctp_process_unk_param:
>>>> if (NULL == *errp)
>>>> 	*errp = sctp_make_op_error_space(asoc, chunk,
>>>> 					 ntohs(chunk->chunk_hdr->length));
>>>>
>>>> 	if (*errp) {
>>>> 		sctp_init_cause(*errp, SCTP_ERROR_UNKNOWN_PARAM,
>>>> 				 WORD_ROUND(ntohs(param.p->length)));
>>>> 		sctp_addto_chunk(*errp,
>>>> 			WORD_ROUND(ntohs(param.p->length)),
>>>> 				  param.v);
>>>>
>>>> When we allocate an error chunk, we assume that the worst case scenario requires
>>>> that we have chunk_hdr->length data allocated, which would be correct nominally,
>>>> given that we call sctp_addto_chunk for the violating parameter.  Unfortunately,
>>>> we also, in sctp_init_cause insert a sctp_errhdr_t structure into the error
>>>> chunk, so the worst case situation in which all parameters are in violation
>>>> requires chunk_hdr->length+(sizeof(sctp_errhdr_t)*param_count) bytes of data.
>>>>
>>>> The result of this error is that a deliberately malformed packet sent to a
>>>> listening host can cause a remote DOS, described in CVE-2010-1173:
>>>> http://cve.mitre.org/cgi-bin/cvename.cgi?name=2010-1173
>>>>
>>>> I've tested the below fix and confirmed that it fixes the issue.  It
>>>> pre-allocates the error chunk in sctp_verify_init, where we are able to count
>>>> the total number of variable length parameters, so we know how many error
>>>> headers we might need.  Then we simply use that chunk, if we find an error, or
>>>> discard/free it if all the parameters are valid.  Applies on top of the
>>>> lksctp-dev tree
>>>>
>>>> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
>>>>
>>>>
>>>>  sm_make_chunk.c |   24 ++++++++++++++++++++++--
>>>>  1 file changed, 22 insertions(+), 2 deletions(-)
>>>>
>>>>
>>>> diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
>>>> index f592163..990457b 100644
>>>> --- a/net/sctp/sm_make_chunk.c
>>>> +++ b/net/sctp/sm_make_chunk.c
>>>> @@ -2134,6 +2134,8 @@ int sctp_verify_init(const struct sctp_association *asoc,
>>>>  	union sctp_params param;
>>>>  	int has_cookie = 0;
>>>>  	int result;
>>>> +	unsigned int param_cnt;
>>>> +	unsigned int len;
>>>>  
>>>>  	/* Verify stream values are non-zero. */
>>>>  	if ((0 == peer_init->init_hdr.num_outbound_streams) ||
>>>> @@ -2149,6 +2151,7 @@ int sctp_verify_init(const struct sctp_association *asoc,
>>>>  
>>>>  		if (SCTP_PARAM_STATE_COOKIE == param.p->type)
>>>>  			has_cookie = 1;
>>>> +		param_cnt++;
>>>>  
>>>>  	} /* for (loop through all parameters) */
>>>>  
>>>> @@ -2169,6 +2172,20 @@ int sctp_verify_init(const struct sctp_association *asoc,
>>>>  		return sctp_process_missing_param(asoc, SCTP_PARAM_STATE_COOKIE,
>>>>  						  chunk, errp);
>>>>  
>>>> +	if (!*errp) {
>>>> +		/*
>>>> +		 * Pre-allocate the error packet here
>>>> +		 * we do this as we need to reserve space
>>>> +		 * for the worst case scenario in which 
>>>> +		 * every parameter is in error and needs 
>>>> +		 * an errhdr attached to it
>>>> +		 */
>>>> +		len = ntohs(chunk->chunk_hdr->length);
>>>> +		len += sizeof(sctp_errhdr_t)*param_cnt;
>>>> +
>>>> +		*errp = sctp_make_op_error_space(asoc, chunk, len);
>>>> +	}
>>>> +
>>>>  	/* Verify all the variable length parameters */
>>>>  	sctp_walk_params(param, peer_init, init_hdr.params) {
>>>>  
>>>> @@ -2176,9 +2193,11 @@ int sctp_verify_init(const struct sctp_association *asoc,
>>>>  		switch (result) {
>>>>  		    case SCTP_IERROR_ABORT:
>>>>  		    case SCTP_IERROR_NOMEM:
>>>> -				return 0;
>>>>  		    case SCTP_IERROR_ERROR:
>>>> -				return 1;
>>>> +				len = ntohs((*errp)->chunk_hdr->length);
>>>> +				if ((*errp) && (len == sizeof(sctp_chunkhdr_t)))
>>>> +					sctp_chunk_free(*errp);
>>>> +				return (result == SCTP_IERROR_ERROR) ? 1 : 0;
>>>>  		    case SCTP_IERROR_NO_ERROR:
>>>>  		    default:
>>>>  				break;
>>>> @@ -2186,6 +2205,7 @@ int sctp_verify_init(const struct sctp_association *asoc,
>>>>  
>>>>  	} /* for (loop through all parameters) */
>>>>  
>>>> +	sctp_chunk_free(*errp);
>>>>  	return 1;
>>>>  }
>>>>  
>>>> --
>>>> 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
>>>>
> --
> 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]: sctp: Fix skb_over_panic resulting from multiple invalid parameter errors (CVE-2010-1173)

[Neil Horman]

On Wed, Apr 28, 2010 at 01:52:05PM -0400, Vlad Yasevich wrote:
> 
> 
> Vlad Yasevich wrote:
> > 
> > Neil Horman wrote:
> >> On Wed, Apr 28, 2010 at 10:00:37AM -0400, Vlad Yasevich wrote:
> >>> I have this patch and a few others already queued.
> >>>
> >>> I was planning on sending these today for stable.
> >>>
> >>> Here is the full list of stable patches I have:
> >>>
> >>> sctp: Fix oops when sending queued ASCONF chunks
> >>> sctp: fix to calc the INIT/INIT-ACK chunk length correctly is set
> >>> sctp: per_cpu variables should be in bh_disabled section
> >>> sctp: fix potential reference of a freed pointer
> >>> sctp: avoid irq lock inversion while call sk->sk_data_ready()
> >>>
> >>> -vlad
> >>>
> >> Are you sure?  this oops looks _very_ simmilar to the INIT/INIT-ACK length
> >> calculation oops described above, but is in fact different, and requires this
> >> patch, from what I can see.  The right fix might be in the ASCONF chunk patch
> >> you list above, but I don't see that in your tree at the moment, so I can't be
> >> sure.
> > 
> > As I said, I totally goofed when reading the description and I apologize.
> > However, I do one comment regarding the patch.
> > 
> > If the bad packet is REALLY long (I mean close to 65K IP limit), then
> > we'll end up allocating a supper huge skb in this case and potentially exceed
> > the IP length limitation.  Section 11.4 of rfc 4960 allows us to omit some
> > errors and limit the size of the packet.
> > 
> > I would recommend limiting this to MTU worth of potentiall errors.  This is
> > on top of what the INIT-ACK is going to carry, so at most we'll sent 2 MTUs
> > worth.  That's still a potential by amplification attack, but it's somewhat
> > mitigated.
> > 
> > Of course now we have to handle the case of checking for space before adding
> > an error cause. :)
> > 
> 
> Hi Neil
> 
> I am also not crazy about the pre-allocation scheme.  In the case where you have
> say 100 parameters that are all 'skip' parameters, you'd end up pre-allocating a
> huge buffer for absolutely nothing.
> 
Would have been nice if you'd made your opinion known 4 hours ago when I was
testing version 2 of this. :)

> This is another point toward a fixed error chunk size and let parameter
> processing allocate it when it reaches a parameter that needs an error.
> 
Hmm, ok, what would you say to a pathmtu sized chunk allocation in parameter
processing that drops errors beyond its capacity
Neil


> 

Re: [PATCH]: sctp: Fix skb_over_panic resulting from multiple invalid parameter errors (CVE-2010-1173)

[Vlad Yasevich]

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



Neil Horman wrote:
> On Wed, Apr 28, 2010 at 01:52:05PM -0400, Vlad Yasevich wrote:
>>
>> Vlad Yasevich wrote:
>>> Neil Horman wrote:
>>>> On Wed, Apr 28, 2010 at 10:00:37AM -0400, Vlad Yasevich wrote:
>>>>> I have this patch and a few others already queued.
>>>>>
>>>>> I was planning on sending these today for stable.
>>>>>
>>>>> Here is the full list of stable patches I have:
>>>>>
>>>>> sctp: Fix oops when sending queued ASCONF chunks
>>>>> sctp: fix to calc the INIT/INIT-ACK chunk length correctly is set
>>>>> sctp: per_cpu variables should be in bh_disabled section
>>>>> sctp: fix potential reference of a freed pointer
>>>>> sctp: avoid irq lock inversion while call sk->sk_data_ready()
>>>>>
>>>>> -vlad
>>>>>
>>>> Are you sure?  this oops looks _very_ simmilar to the INIT/INIT-ACK length
>>>> calculation oops described above, but is in fact different, and requires this
>>>> patch, from what I can see.  The right fix might be in the ASCONF chunk patch
>>>> you list above, but I don't see that in your tree at the moment, so I can't be
>>>> sure.
>>> As I said, I totally goofed when reading the description and I apologize.
>>> However, I do one comment regarding the patch.
>>>
>>> If the bad packet is REALLY long (I mean close to 65K IP limit), then
>>> we'll end up allocating a supper huge skb in this case and potentially exceed
>>> the IP length limitation.  Section 11.4 of rfc 4960 allows us to omit some
>>> errors and limit the size of the packet.
>>>
>>> I would recommend limiting this to MTU worth of potentiall errors.  This is
>>> on top of what the INIT-ACK is going to carry, so at most we'll sent 2 MTUs
>>> worth.  That's still a potential by amplification attack, but it's somewhat
>>> mitigated.
>>>
>>> Of course now we have to handle the case of checking for space before adding
>>> an error cause. :)
>>>
>> Hi Neil
>>
>> I am also not crazy about the pre-allocation scheme.  In the case where you have
>> say 100 parameters that are all 'skip' parameters, you'd end up pre-allocating a
>> huge buffer for absolutely nothing.
>>
> Would have been nice if you'd made your opinion known 4 hours ago when I was
> testing version 2 of this. :)
> 

sorry, fighting a head cold and need drugs to think clearly... ;)


>> This is another point toward a fixed error chunk size and let parameter
>> processing allocate it when it reaches a parameter that needs an error.
>>
> Hmm, ok, what would you say to a pathmtu sized chunk allocation in parameter
> processing that drops errors beyond its capacity
> Neil

Here is my quick take on this.  Haven't tested it at all.

-vlad

[-- Attachment #2: neil --]
[-- Type: text/plain, Size: 3987 bytes --]

diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
index 0fd5b4c..74d8d21 100644
--- a/net/sctp/sm_make_chunk.c
+++ b/net/sctp/sm_make_chunk.c
@@ -1959,8 +1959,10 @@ static void sctp_process_ext_param(struct sctp_association *asoc,
 static sctp_ierror_t sctp_process_unk_param(const struct sctp_association *asoc,
 					    union sctp_params param,
 					    struct sctp_chunk *chunk,
-					    struct sctp_chunk **errp)
+					    struct sctp_chunk **errp,
+					    unsigned int param_cnt)
 {
+	unsigned int needed_bytes;
 	int retval = SCTP_IERROR_NO_ERROR;
 
 	switch (param.p->type & SCTP_PARAM_ACTION_MASK) {
@@ -1976,11 +1978,41 @@ static sctp_ierror_t sctp_process_unk_param(const struct sctp_association *asoc,
 		/* Make an ERROR chunk, preparing enough room for
 		 * returning multiple unknown parameters.
 		 */
-		if (NULL == *errp)
-			*errp = sctp_make_op_error_space(asoc, chunk,
-					ntohs(chunk->chunk_hdr->length));
+		if (NULL == *errp) {
+			unsigned int len;
+
+			/* Reserver space for the worst possible case
+			 * at this time.  We count incomming chunk length
+			 * since error parameters carry the bad parameter
+			 * inself, plus have space for error headers for
+			 * the remaining number of parameters.
+			 */
+			len = ntohs(chunk->chunk_hdr->length);
+			len += sizeof(sctp_errhdr_t) * param_cnt;
+
+			/* We need to prevent amplification attacks that
+			 * result from sending 65K init chunks with all bad
+			 * params maliciously, so lets limit our error response
+			 * to 1 MTU worth of errors, but at least 1500 bytes
+			 * in case our pathmtu hasn't been updated yet.
+			 */
+			len = min(len, asoc ? asoc->pathmtu :
+						SCTP_DEFAULT_MAXSEGMENT);
+			*errp = sctp_make_op_error_space(asoc, chunk, len);
+		}
 
 		if (*errp) {
+			needed_bytes = sizeof(sctp_errhdr_t) +
+				       WORD_ROUND(ntohs(param.p->length));
+
+			if (skb_tailroom((*errp)->skb) < needed_bytes)
+				/*
+				 * If we're over our packet size here, don't add
+				 * the error, this is allowed by section 11.4 of
+				 * RFC 4960
+				 */
+				break;
+
 			sctp_init_cause(*errp, SCTP_ERROR_UNKNOWN_PARAM,
 					WORD_ROUND(ntohs(param.p->length)));
 			sctp_addto_chunk(*errp,
@@ -2013,7 +2045,8 @@ static sctp_ierror_t sctp_verify_param(const struct sctp_association *asoc,
 					union sctp_params param,
 					sctp_cid_t cid,
 					struct sctp_chunk *chunk,
-					struct sctp_chunk **err_chunk)
+					struct sctp_chunk **err_chunk,
+					unsigned int param_cnt)
 {
 	struct sctp_hmac_algo_param *hmacs;
 	int retval = SCTP_IERROR_NO_ERROR;
@@ -2119,7 +2152,8 @@ fallthrough:
 	default:
 		SCTP_DEBUG_PRINTK("Unrecognized param: %d for chunk %d.\n",
 				ntohs(param.p->type), cid);
-		retval = sctp_process_unk_param(asoc, param, chunk, err_chunk);
+		retval = sctp_process_unk_param(asoc, param, chunk, err_chunk,
+						param_cnt);
 		break;
 	}
 	return retval;
@@ -2135,6 +2169,7 @@ int sctp_verify_init(const struct sctp_association *asoc,
 	union sctp_params param;
 	int has_cookie = 0;
 	int result;
+	unsigned int param_cnt = 0;
 
 	/* Verify stream values are non-zero. */
 	if ((0 == peer_init->init_hdr.num_outbound_streams) ||
@@ -2150,6 +2185,7 @@ int sctp_verify_init(const struct sctp_association *asoc,
 
 		if (SCTP_PARAM_STATE_COOKIE == param.p->type)
 			has_cookie = 1;
+		param_cnt++;
 
 	} /* for (loop through all parameters) */
 
@@ -2173,7 +2209,8 @@ int sctp_verify_init(const struct sctp_association *asoc,
 	/* Verify all the variable length parameters */
 	sctp_walk_params(param, peer_init, init_hdr.params) {
 
-		result = sctp_verify_param(asoc, param, cid, chunk, errp);
+		result = sctp_verify_param(asoc, param, cid, chunk, errp,
+					   param_cnt);
 		switch (result) {
 		    case SCTP_IERROR_ABORT:
 		    case SCTP_IERROR_NOMEM:
@@ -2184,6 +2221,7 @@ int sctp_verify_init(const struct sctp_association *asoc,
 		    default:
 				break;
 		}
+		param_cnt--;
 
 	} /* for (loop through all parameters) */
 

Re: [PATCH]: sctp: Fix skb_over_panic resulting from multiple invalid parameter errors (CVE-2010-1173)

[Neil Horman]

On Wed, Apr 28, 2010 at 02:27:11PM -0400, Vlad Yasevich wrote:
> 
> 
> Neil Horman wrote:
> > On Wed, Apr 28, 2010 at 01:52:05PM -0400, Vlad Yasevich wrote:
> >>
> >> Vlad Yasevich wrote:
> >>> Neil Horman wrote:
> >>>> On Wed, Apr 28, 2010 at 10:00:37AM -0400, Vlad Yasevich wrote:
> >>>>> I have this patch and a few others already queued.
> >>>>>
> >>>>> I was planning on sending these today for stable.
> >>>>>
> >>>>> Here is the full list of stable patches I have:
> >>>>>
> >>>>> sctp: Fix oops when sending queued ASCONF chunks
> >>>>> sctp: fix to calc the INIT/INIT-ACK chunk length correctly is set
> >>>>> sctp: per_cpu variables should be in bh_disabled section
> >>>>> sctp: fix potential reference of a freed pointer
> >>>>> sctp: avoid irq lock inversion while call sk->sk_data_ready()
> >>>>>
> >>>>> -vlad
> >>>>>
> >>>> Are you sure?  this oops looks _very_ simmilar to the INIT/INIT-ACK length
> >>>> calculation oops described above, but is in fact different, and requires this
> >>>> patch, from what I can see.  The right fix might be in the ASCONF chunk patch
> >>>> you list above, but I don't see that in your tree at the moment, so I can't be
> >>>> sure.
> >>> As I said, I totally goofed when reading the description and I apologize.
> >>> However, I do one comment regarding the patch.
> >>>
> >>> If the bad packet is REALLY long (I mean close to 65K IP limit), then
> >>> we'll end up allocating a supper huge skb in this case and potentially exceed
> >>> the IP length limitation.  Section 11.4 of rfc 4960 allows us to omit some
> >>> errors and limit the size of the packet.
> >>>
> >>> I would recommend limiting this to MTU worth of potentiall errors.  This is
> >>> on top of what the INIT-ACK is going to carry, so at most we'll sent 2 MTUs
> >>> worth.  That's still a potential by amplification attack, but it's somewhat
> >>> mitigated.
> >>>
> >>> Of course now we have to handle the case of checking for space before adding
> >>> an error cause. :)
> >>>
> >> Hi Neil
> >>
> >> I am also not crazy about the pre-allocation scheme.  In the case where you have
> >> say 100 parameters that are all 'skip' parameters, you'd end up pre-allocating a
> >> huge buffer for absolutely nothing.
> >>
> > Would have been nice if you'd made your opinion known 4 hours ago when I was
> > testing version 2 of this. :)
> > 
> 
> sorry, fighting a head cold and need drugs to think clearly... ;)
> 
Its ok, I'm apparently just feeling a bit short tempered today. Apologies, hope
your feeling better soon :)

> 
> >> This is another point toward a fixed error chunk size and let parameter
> >> processing allocate it when it reaches a parameter that needs an error.
> >>
> > Hmm, ok, what would you say to a pathmtu sized chunk allocation in parameter
> > processing that drops errors beyond its capacity
> > Neil
> 
> Here is my quick take on this.  Haven't tested it at all.
> 
I think somthing like this will work, I've got a variant that uses some helper
functions to create and manipulate fixed length op error chunks going right now.
It does basically the same thing that your doing, but consolidates the checking
of remaining space to a central place.  I think that might be better, as during
my looking at this version, I found two other points that might be vulnerable to
this error (haven't tested to confirm yet though).  I'll post shortly.

Thanks!
Neil

Re: [PATCH]: sctp: Fix skb_over_panic resulting from multiple invalid parameter errors (CVE-2010-1173) (v3)

[Neil Horman]

Ok, version 3.

Change notes:

1) As per our discussion, uses a fixed size alloation stragetgy, allocating only
a mtu sized chunk (up to 1500 bytes).

2) add sctp_init_cause_fixed and sctp_addto_chunk_fixed helpers that, instead of
oopsing on insufficient data in the skb, instead simple fail to preform the
copy, and return an appropriate error code.

Summary:
	Recently, it was reported to me that the kernel could oops in the
following way:

<5> kernel BUG at net/core/skbuff.c:91!
<5> invalid operand: 0000 [#1]
<5> Modules linked in: sctp netconsole nls_utf8 autofs4 sunrpc iptable_filter
ip_tables cpufreq_powersave parport_pc lp parport vmblock(U) vsock(U) vmci(U)
vmxnet(U) vmmemctl(U) vmhgfs(U) acpiphp dm_mirror dm_mod button battery ac md5
ipv6 uhci_hcd ehci_hcd snd_ens1371 snd_rawmidi snd_seq_device snd_pcm_oss
snd_mixer_oss snd_pcm snd_timer snd_page_alloc snd_ac97_codec snd soundcore
pcnet32 mii floppy ext3 jbd ata_piix libata mptscsih mptsas mptspi mptscsi
mptbase sd_mod scsi_mod
<5> CPU:    0
<5> EIP:    0060:[<c02bff27>]    Not tainted VLI
<5> EFLAGS: 00010216   (2.6.9-89.0.25.EL) 
<5> EIP is at skb_over_panic+0x1f/0x2d
<5> eax: 0000002c   ebx: c033f461   ecx: c0357d96   edx: c040fd44
<5> esi: c033f461   edi: df653280   ebp: 00000000   esp: c040fd40
<5> ds: 007b   es: 007b   ss: 0068
<5> Process swapper (pid: 0, threadinfo=c040f000 task=c0370be0)
<5> Stack: c0357d96 e0c29478 00000084 00000004 c033f461 df653280 d7883180
e0c2947d 
<5>        00000000 00000080 df653490 00000004 de4f1ac0 de4f1ac0 00000004
df653490 
<5>        00000001 e0c2877a 08000800 de4f1ac0 df653490 00000000 e0c29d2e
00000004 
<5> Call Trace:
<5>  [<e0c29478>] sctp_addto_chunk+0xb0/0x128 [sctp]
<5>  [<e0c2947d>] sctp_addto_chunk+0xb5/0x128 [sctp]
<5>  [<e0c2877a>] sctp_init_cause+0x3f/0x47 [sctp]
<5>  [<e0c29d2e>] sctp_process_unk_param+0xac/0xb8 [sctp]
<5>  [<e0c29e90>] sctp_verify_init+0xcc/0x134 [sctp]
<5>  [<e0c20322>] sctp_sf_do_5_1B_init+0x83/0x28e [sctp]
<5>  [<e0c25333>] sctp_do_sm+0x41/0x77 [sctp]
<5>  [<c01555a4>] cache_grow+0x140/0x233
<5>  [<e0c26ba1>] sctp_endpoint_bh_rcv+0xc5/0x108 [sctp]
<5>  [<e0c2b863>] sctp_inq_push+0xe/0x10 [sctp]
<5>  [<e0c34600>] sctp_rcv+0x454/0x509 [sctp]
<5>  [<e084e017>] ipt_hook+0x17/0x1c [iptable_filter]
<5>  [<c02d005e>] nf_iterate+0x40/0x81
<5>  [<c02e0bb9>] ip_local_deliver_finish+0x0/0x151
<5>  [<c02e0c7f>] ip_local_deliver_finish+0xc6/0x151
<5>  [<c02d0362>] nf_hook_slow+0x83/0xb5
<5>  [<c02e0bb2>] ip_local_deliver+0x1a2/0x1a9
<5>  [<c02e0bb9>] ip_local_deliver_finish+0x0/0x151
<5>  [<c02e103e>] ip_rcv+0x334/0x3b4
<5>  [<c02c66fd>] netif_receive_skb+0x320/0x35b
<5>  [<e0a0928b>] init_stall_timer+0x67/0x6a [uhci_hcd]
<5>  [<c02c67a4>] process_backlog+0x6c/0xd9
<5>  [<c02c690f>] net_rx_action+0xfe/0x1f8
<5>  [<c012a7b1>] __do_softirq+0x35/0x79
<5>  [<c0107efb>] handle_IRQ_event+0x0/0x4f
<5>  [<c01094de>] do_softirq+0x46/0x4d

Its an skb_over_panic BUG halt that results from processing an init chunk in
which too many of its variable length parameters are in some way malformed.

The problem is in sctp_process_unk_param:
if (NULL == *errp)
	*errp = sctp_make_op_error_space(asoc, chunk,
					 ntohs(chunk->chunk_hdr->length));

	if (*errp) {
		sctp_init_cause(*errp, SCTP_ERROR_UNKNOWN_PARAM,
				 WORD_ROUND(ntohs(param.p->length)));
		sctp_addto_chunk(*errp,
			WORD_ROUND(ntohs(param.p->length)),
				  param.v);

When we allocate an error chunk, we assume that the worst case scenario requires
that we have chunk_hdr->length data allocated, which would be correct nominally,
given that we call sctp_addto_chunk for the violating parameter.  Unfortunately,
we also, in sctp_init_cause insert a sctp_errhdr_t structure into the error
chunk, so the worst case situation in which all parameters are in violation
requires chunk_hdr->length+(sizeof(sctp_errhdr_t)*param_count) bytes of data.

The result of this error is that a deliberately malformed packet sent to a
listening host can cause a remote DOS, described in CVE-2010-1173:
http://cve.mitre.org/cgi-bin/cvename.cgi?name=2010-1173

This fix solves the problem by allowing our implementation to only report a
fixed number of errors.  When we encounter an error in parameter processing we
allocate a chunk that is min(asoc->pathmtu, SCTP_DEFAULT_MAXSEGMENT), limiting
our error reporting to a single mtu sized chunk.  Parameter errors that grow
beyond that value are discarded.

I've tested this fix using the reproducer that I was provided (send an init chunk to a
listening sctp socket with a few dozen parameters of type 0xc001 and length 4),
and it solves the problem nicely.

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


 include/net/sctp/structs.h |    1 
 net/sctp/sm_make_chunk.c   |   70 +++++++++++++++++++++++++++++++++++++++------
 2 files changed, 62 insertions(+), 9 deletions(-)


diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
index ff30177..597f8e2 100644
--- a/include/net/sctp/structs.h
+++ b/include/net/sctp/structs.h
@@ -778,6 +778,7 @@ int sctp_user_addto_chunk(struct sctp_chunk *chunk, int off, int len,
 			  struct iovec *data);
 void sctp_chunk_free(struct sctp_chunk *);
 void  *sctp_addto_chunk(struct sctp_chunk *, int len, const void *data);
+void  *sctp_addto_chunk_fixed(struct sctp_chunk *, int len, const void *data);
 struct sctp_chunk *sctp_chunkify(struct sk_buff *,
 				 const struct sctp_association *,
 				 struct sock *);
diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
index f592163..9623112 100644
--- a/net/sctp/sm_make_chunk.c
+++ b/net/sctp/sm_make_chunk.c
@@ -107,7 +107,7 @@ static const struct sctp_paramhdr prsctp_param = {
 	cpu_to_be16(sizeof(struct sctp_paramhdr)),
 };
 
-/* A helper to initialize to initialize an op error inside a
+/* A helper to initialize an op error inside a
  * provided chunk, as most cause codes will be embedded inside an
  * abort chunk.
  */
@@ -124,6 +124,29 @@ void  sctp_init_cause(struct sctp_chunk *chunk, __be16 cause_code,
 	chunk->subh.err_hdr = sctp_addto_chunk(chunk, sizeof(sctp_errhdr_t), &err);
 }
 
+/* A helper to initialize an op error inside a
+ * provided chunk, as most cause codes will be embedded inside an
+ * abort chunk.  Differs from sctp_init_cause in that it won't oops
+ * if there isn't enough space in the op error chunk
+ */
+int sctp_init_cause_fixed(struct sctp_chunk *chunk, __be16 cause_code,
+		      size_t paylen)
+{
+	sctp_errhdr_t err;
+	__u16 len;
+
+	/* Cause code constants are now defined in network order.  */
+	err.cause = cause_code;
+	len = sizeof(sctp_errhdr_t) + paylen;
+	err.length  = htons(len);
+
+	if (skb_tailroom(chunk->skb) >  len)
+		return -ENOSPC;
+	chunk->subh.err_hdr = sctp_addto_chunk_fixed(chunk,
+						     sizeof(sctp_errhdr_t),
+						     &err);
+	return 0;
+}
 /* 3.3.2 Initiation (INIT) (1)
  *
  * This chunk is used to initiate a SCTP association between two
@@ -1131,6 +1154,24 @@ nodata:
 	return retval;
 }
 
+/* Create an Operation Error chunk of a fixed size,
+ * specifically, max(asoc->pathmtu, SCTP_DEFAULT_MAXSEGMENT)
+ * This is a helper function to allocate an error chunk for
+ * for those invalid parameter codes in which we may not want
+ * to report all the errors, if the incomming chunk is large
+ */
+static inline struct sctp_chunk *sctp_make_op_error_fixed(
+	const struct sctp_association *asoc,
+	const struct sctp_chunk *chunk)
+{
+	size_t size = asoc ? asoc->pathmtu : 0;
+
+	if (size > SCTP_DEFAULT_MAXSEGMENT)
+		size = SCTP_DEFAULT_MAXSEGMENT;
+
+	return sctp_make_op_error_space(asoc, chunk, size);
+}
+
 /* Create an Operation Error chunk.  */
 struct sctp_chunk *sctp_make_op_error(const struct sctp_association *asoc,
 				 const struct sctp_chunk *chunk,
@@ -1373,6 +1414,18 @@ void *sctp_addto_chunk(struct sctp_chunk *chunk, int len, const void *data)
 	return target;
 }
 
+/* Append bytes to the end of a chunk. Returns NULL if there isn't sufficient
+ * space in the chunk
+ */
+void *sctp_addto_chunk_fixed(struct sctp_chunk *chunk,
+			     int len, const void *data)
+{
+	if (skb_tailroom(chunk->skb) > len)
+		return sctp_addto_chunk(chunk, len, data);
+	else
+		return NULL;
+}
+
 /* Append bytes from user space to the end of a chunk.  Will panic if
  * chunk is not big enough.
  * Returns a kernel err value.
@@ -1793,9 +1846,9 @@ static int sctp_process_missing_param(const struct sctp_association *asoc,
 	if (*errp) {
 		report.num_missing = htonl(1);
 		report.type = paramtype;
-		sctp_init_cause(*errp, SCTP_ERROR_MISS_PARAM,
-				sizeof(report));
-		sctp_addto_chunk(*errp, sizeof(report), &report);
+		sctp_init_cause_fixed(*errp, SCTP_ERROR_MISS_PARAM,
+				      sizeof(report));
+		sctp_addto_chunk_fixed(*errp, sizeof(report), &report);
 	}
 
 	/* Stop processing this chunk. */
@@ -1813,7 +1866,7 @@ static int sctp_process_inv_mandatory(const struct sctp_association *asoc,
 		*errp = sctp_make_op_error_space(asoc, chunk, 0);
 
 	if (*errp)
-		sctp_init_cause(*errp, SCTP_ERROR_INV_PARAM, 0);
+		sctp_init_cause_fixed(*errp, SCTP_ERROR_INV_PARAM, 0);
 
 	/* Stop processing this chunk. */
 	return 0;
@@ -1976,13 +2029,12 @@ static sctp_ierror_t sctp_process_unk_param(const struct sctp_association *asoc,
 		 * returning multiple unknown parameters.
 		 */
 		if (NULL == *errp)
-			*errp = sctp_make_op_error_space(asoc, chunk,
-					ntohs(chunk->chunk_hdr->length));
+			*errp = sctp_make_op_error_fixed(asoc, chunk);
 
 		if (*errp) {
-			sctp_init_cause(*errp, SCTP_ERROR_UNKNOWN_PARAM,
+			sctp_init_cause_fixed(*errp, SCTP_ERROR_UNKNOWN_PARAM,
 					WORD_ROUND(ntohs(param.p->length)));
-			sctp_addto_chunk(*errp,
+			sctp_addto_chunk_fixed(*errp,
 					WORD_ROUND(ntohs(param.p->length)),
 					param.v);
 		} else {

Re: [PATCH]: sctp: Fix skb_over_panic resulting from multiple invalid parameter errors (CVE-2010-1173) (v3)

[Vlad Yasevich]

Neil Horman wrote:

... snip description ...

> 
> 
>  include/net/sctp/structs.h |    1 
>  net/sctp/sm_make_chunk.c   |   70 +++++++++++++++++++++++++++++++++++++++------
>  2 files changed, 62 insertions(+), 9 deletions(-)
> 
> 
> diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
> index ff30177..597f8e2 100644
> --- a/include/net/sctp/structs.h
> +++ b/include/net/sctp/structs.h
> @@ -778,6 +778,7 @@ int sctp_user_addto_chunk(struct sctp_chunk *chunk, int off, int len,
>  			  struct iovec *data);
>  void sctp_chunk_free(struct sctp_chunk *);
>  void  *sctp_addto_chunk(struct sctp_chunk *, int len, const void *data);
> +void  *sctp_addto_chunk_fixed(struct sctp_chunk *, int len, const void *data);
>  struct sctp_chunk *sctp_chunkify(struct sk_buff *,
>  				 const struct sctp_association *,
>  				 struct sock *);
> diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
> index f592163..9623112 100644
> --- a/net/sctp/sm_make_chunk.c
> +++ b/net/sctp/sm_make_chunk.c
> @@ -107,7 +107,7 @@ static const struct sctp_paramhdr prsctp_param = {
>  	cpu_to_be16(sizeof(struct sctp_paramhdr)),
>  };
>  
> -/* A helper to initialize to initialize an op error inside a
> +/* A helper to initialize an op error inside a
>   * provided chunk, as most cause codes will be embedded inside an
>   * abort chunk.
>   */
> @@ -124,6 +124,29 @@ void  sctp_init_cause(struct sctp_chunk *chunk, __be16 cause_code,
>  	chunk->subh.err_hdr = sctp_addto_chunk(chunk, sizeof(sctp_errhdr_t), &err);
>  }
>  
> +/* A helper to initialize an op error inside a
> + * provided chunk, as most cause codes will be embedded inside an
> + * abort chunk.  Differs from sctp_init_cause in that it won't oops
> + * if there isn't enough space in the op error chunk
> + */
> +int sctp_init_cause_fixed(struct sctp_chunk *chunk, __be16 cause_code,
> +		      size_t paylen)
> +{
> +	sctp_errhdr_t err;
> +	__u16 len;
> +
> +	/* Cause code constants are now defined in network order.  */
> +	err.cause = cause_code;
> +	len = sizeof(sctp_errhdr_t) + paylen;
> +	err.length  = htons(len);
> +
> +	if (skb_tailroom(chunk->skb) >  len)
> +		return -ENOSPC;
> +	chunk->subh.err_hdr = sctp_addto_chunk_fixed(chunk,
> +						     sizeof(sctp_errhdr_t),
> +						     &err);
> +	return 0;
> +}
>  /* 3.3.2 Initiation (INIT) (1)
>   *
>   * This chunk is used to initiate a SCTP association between two
> @@ -1131,6 +1154,24 @@ nodata:
>  	return retval;
>  }
>  
> +/* Create an Operation Error chunk of a fixed size,
> + * specifically, max(asoc->pathmtu, SCTP_DEFAULT_MAXSEGMENT)
> + * This is a helper function to allocate an error chunk for
> + * for those invalid parameter codes in which we may not want
> + * to report all the errors, if the incomming chunk is large
> + */
> +static inline struct sctp_chunk *sctp_make_op_error_fixed(
> +	const struct sctp_association *asoc,
> +	const struct sctp_chunk *chunk)
> +{
> +	size_t size = asoc ? asoc->pathmtu : 0;
> +
> +	if (size > SCTP_DEFAULT_MAXSEGMENT)
> +		size = SCTP_DEFAULT_MAXSEGMENT;
> +

This doesn't look right.  If you don't have an association or if pmtu is
not initialized, you will end up with a size of 0.  I think you simply
want to a
	if (!size)

there.


> +	return sctp_make_op_error_space(asoc, chunk, size);
> +}
> +
>  /* Create an Operation Error chunk.  */
>  struct sctp_chunk *sctp_make_op_error(const struct sctp_association *asoc,
>  				 const struct sctp_chunk *chunk,
> @@ -1373,6 +1414,18 @@ void *sctp_addto_chunk(struct sctp_chunk *chunk, int len, const void *data)
>  	return target;
>  }
>  
> +/* Append bytes to the end of a chunk. Returns NULL if there isn't sufficient
> + * space in the chunk
> + */
> +void *sctp_addto_chunk_fixed(struct sctp_chunk *chunk,
> +			     int len, const void *data)
> +{
> +	if (skb_tailroom(chunk->skb) > len)
> +		return sctp_addto_chunk(chunk, len, data);
> +	else
> +		return NULL;
> +}
> +
>  /* Append bytes from user space to the end of a chunk.  Will panic if
>   * chunk is not big enough.
>   * Returns a kernel err value.
> @@ -1793,9 +1846,9 @@ static int sctp_process_missing_param(const struct sctp_association *asoc,
>  	if (*errp) {
>  		report.num_missing = htonl(1);
>  		report.type = paramtype;
> -		sctp_init_cause(*errp, SCTP_ERROR_MISS_PARAM,
> -				sizeof(report));
> -		sctp_addto_chunk(*errp, sizeof(report), &report);
> +		sctp_init_cause_fixed(*errp, SCTP_ERROR_MISS_PARAM,
> +				      sizeof(report));
> +		sctp_addto_chunk_fixed(*errp, sizeof(report), &report);
>  	}
>  
>  	/* Stop processing this chunk. */
> @@ -1813,7 +1866,7 @@ static int sctp_process_inv_mandatory(const struct sctp_association *asoc,
>  		*errp = sctp_make_op_error_space(asoc, chunk, 0);
>  
>  	if (*errp)
> -		sctp_init_cause(*errp, SCTP_ERROR_INV_PARAM, 0);
> +		sctp_init_cause_fixed(*errp, SCTP_ERROR_INV_PARAM, 0);
>  
>  	/* Stop processing this chunk. */
>  	return 0;

I don't think missing or mandatory parameters are effected by this.  They are a
once and done deal.  There don't report multiple errors and we don't add any
more error after them.

> @@ -1976,13 +2029,12 @@ static sctp_ierror_t sctp_process_unk_param(const struct sctp_association *asoc,
>  		 * returning multiple unknown parameters.
>  		 */
>  		if (NULL == *errp)
> -			*errp = sctp_make_op_error_space(asoc, chunk,
> -					ntohs(chunk->chunk_hdr->length));
> +			*errp = sctp_make_op_error_fixed(asoc, chunk);
>  
>  		if (*errp) {
> -			sctp_init_cause(*errp, SCTP_ERROR_UNKNOWN_PARAM,
> +			sctp_init_cause_fixed(*errp, SCTP_ERROR_UNKNOWN_PARAM,
>  					WORD_ROUND(ntohs(param.p->length)));
> -			sctp_addto_chunk(*errp,
> +			sctp_addto_chunk_fixed(*errp,
>  					WORD_ROUND(ntohs(param.p->length)),
>  					param.v);
>  		} else {
> 

So we completely get rid of variable size error chunk in this case, which I can
live with.  It simplifies the code.

-vlad

Re: [PATCH]: sctp: Fix skb_over_panic resulting from multiple invalid parameter errors (CVE-2010-1173) (v4)

[Neil Horman]

Ok, version 4

Change Notes:
1) Minor cleanups, from Vlads notes

Summary:


Hey-
	Recently, it was reported to me that the kernel could oops in the
following way:

<5> kernel BUG at net/core/skbuff.c:91!
<5> invalid operand: 0000 [#1]
<5> Modules linked in: sctp netconsole nls_utf8 autofs4 sunrpc iptable_filter
ip_tables cpufreq_powersave parport_pc lp parport vmblock(U) vsock(U) vmci(U)
vmxnet(U) vmmemctl(U) vmhgfs(U) acpiphp dm_mirror dm_mod button battery ac md5
ipv6 uhci_hcd ehci_hcd snd_ens1371 snd_rawmidi snd_seq_device snd_pcm_oss
snd_mixer_oss snd_pcm snd_timer snd_page_alloc snd_ac97_codec snd soundcore
pcnet32 mii floppy ext3 jbd ata_piix libata mptscsih mptsas mptspi mptscsi
mptbase sd_mod scsi_mod
<5> CPU:    0
<5> EIP:    0060:[<c02bff27>]    Not tainted VLI
<5> EFLAGS: 00010216   (2.6.9-89.0.25.EL) 
<5> EIP is at skb_over_panic+0x1f/0x2d
<5> eax: 0000002c   ebx: c033f461   ecx: c0357d96   edx: c040fd44
<5> esi: c033f461   edi: df653280   ebp: 00000000   esp: c040fd40
<5> ds: 007b   es: 007b   ss: 0068
<5> Process swapper (pid: 0, threadinfo=c040f000 task=c0370be0)
<5> Stack: c0357d96 e0c29478 00000084 00000004 c033f461 df653280 d7883180
e0c2947d 
<5>        00000000 00000080 df653490 00000004 de4f1ac0 de4f1ac0 00000004
df653490 
<5>        00000001 e0c2877a 08000800 de4f1ac0 df653490 00000000 e0c29d2e
00000004 
<5> Call Trace:
<5>  [<e0c29478>] sctp_addto_chunk+0xb0/0x128 [sctp]
<5>  [<e0c2947d>] sctp_addto_chunk+0xb5/0x128 [sctp]
<5>  [<e0c2877a>] sctp_init_cause+0x3f/0x47 [sctp]
<5>  [<e0c29d2e>] sctp_process_unk_param+0xac/0xb8 [sctp]
<5>  [<e0c29e90>] sctp_verify_init+0xcc/0x134 [sctp]
<5>  [<e0c20322>] sctp_sf_do_5_1B_init+0x83/0x28e [sctp]
<5>  [<e0c25333>] sctp_do_sm+0x41/0x77 [sctp]
<5>  [<c01555a4>] cache_grow+0x140/0x233
<5>  [<e0c26ba1>] sctp_endpoint_bh_rcv+0xc5/0x108 [sctp]
<5>  [<e0c2b863>] sctp_inq_push+0xe/0x10 [sctp]
<5>  [<e0c34600>] sctp_rcv+0x454/0x509 [sctp]
<5>  [<e084e017>] ipt_hook+0x17/0x1c [iptable_filter]
<5>  [<c02d005e>] nf_iterate+0x40/0x81
<5>  [<c02e0bb9>] ip_local_deliver_finish+0x0/0x151
<5>  [<c02e0c7f>] ip_local_deliver_finish+0xc6/0x151
<5>  [<c02d0362>] nf_hook_slow+0x83/0xb5
<5>  [<c02e0bb2>] ip_local_deliver+0x1a2/0x1a9
<5>  [<c02e0bb9>] ip_local_deliver_finish+0x0/0x151
<5>  [<c02e103e>] ip_rcv+0x334/0x3b4
<5>  [<c02c66fd>] netif_receive_skb+0x320/0x35b
<5>  [<e0a0928b>] init_stall_timer+0x67/0x6a [uhci_hcd]
<5>  [<c02c67a4>] process_backlog+0x6c/0xd9
<5>  [<c02c690f>] net_rx_action+0xfe/0x1f8
<5>  [<c012a7b1>] __do_softirq+0x35/0x79
<5>  [<c0107efb>] handle_IRQ_event+0x0/0x4f
<5>  [<c01094de>] do_softirq+0x46/0x4d

Its an skb_over_panic BUG halt that results from processing an init chunk in
which too many of its variable length parameters are in some way malformed.

The problem is in sctp_process_unk_param:
if (NULL == *errp)
	*errp = sctp_make_op_error_space(asoc, chunk,
					 ntohs(chunk->chunk_hdr->length));

	if (*errp) {
		sctp_init_cause(*errp, SCTP_ERROR_UNKNOWN_PARAM,
				 WORD_ROUND(ntohs(param.p->length)));
		sctp_addto_chunk(*errp,
			WORD_ROUND(ntohs(param.p->length)),
				  param.v);

When we allocate an error chunk, we assume that the worst case scenario requires
that we have chunk_hdr->length data allocated, which would be correct nominally,
given that we call sctp_addto_chunk for the violating parameter.  Unfortunately,
we also, in sctp_init_cause insert a sctp_errhdr_t structure into the error
chunk, so the worst case situation in which all parameters are in violation
requires chunk_hdr->length+(sizeof(sctp_errhdr_t)*param_count) bytes of data.

The result of this error is that a deliberately malformed packet sent to a
listening host can cause a remote DOS, described in CVE-2010-1173:
http://cve.mitre.org/cgi-bin/cvename.cgi?name=2010-1173

I've tested the below fix and confirmed that it fixes the issue.  We move to a
strategy whereby we allocate a fixed size error chunk and ignore errors we don't
have space to report.  Tested by me successfully

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


 include/net/sctp/structs.h |    1 
 net/sctp/sm_make_chunk.c   |   62 +++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 58 insertions(+), 5 deletions(-)


diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
index ff30177..597f8e2 100644
--- a/include/net/sctp/structs.h
+++ b/include/net/sctp/structs.h
@@ -778,6 +778,7 @@ int sctp_user_addto_chunk(struct sctp_chunk *chunk, int off, int len,
 			  struct iovec *data);
 void sctp_chunk_free(struct sctp_chunk *);
 void  *sctp_addto_chunk(struct sctp_chunk *, int len, const void *data);
+void  *sctp_addto_chunk_fixed(struct sctp_chunk *, int len, const void *data);
 struct sctp_chunk *sctp_chunkify(struct sk_buff *,
 				 const struct sctp_association *,
 				 struct sock *);
diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
index f592163..2971731 100644
--- a/net/sctp/sm_make_chunk.c
+++ b/net/sctp/sm_make_chunk.c
@@ -107,7 +107,7 @@ static const struct sctp_paramhdr prsctp_param = {
 	cpu_to_be16(sizeof(struct sctp_paramhdr)),
 };
 
-/* A helper to initialize to initialize an op error inside a
+/* A helper to initialize an op error inside a
  * provided chunk, as most cause codes will be embedded inside an
  * abort chunk.
  */
@@ -124,6 +124,29 @@ void  sctp_init_cause(struct sctp_chunk *chunk, __be16 cause_code,
 	chunk->subh.err_hdr = sctp_addto_chunk(chunk, sizeof(sctp_errhdr_t), &err);
 }
 
+/* A helper to initialize an op error inside a
+ * provided chunk, as most cause codes will be embedded inside an
+ * abort chunk.  Differs from sctp_init_cause in that it won't oops
+ * if there isn't enough space in the op error chunk
+ */
+int sctp_init_cause_fixed(struct sctp_chunk *chunk, __be16 cause_code,
+		      size_t paylen)
+{
+	sctp_errhdr_t err;
+	__u16 len;
+
+	/* Cause code constants are now defined in network order.  */
+	err.cause = cause_code;
+	len = sizeof(sctp_errhdr_t) + paylen;
+	err.length  = htons(len);
+
+	if (skb_tailroom(chunk->skb) >  len)
+		return -ENOSPC;
+	chunk->subh.err_hdr = sctp_addto_chunk_fixed(chunk,
+						     sizeof(sctp_errhdr_t),
+						     &err);
+	return 0;
+}
 /* 3.3.2 Initiation (INIT) (1)
  *
  * This chunk is used to initiate a SCTP association between two
@@ -1131,6 +1154,24 @@ nodata:
 	return retval;
 }
 
+/* Create an Operation Error chunk of a fixed size,
+ * specifically, max(asoc->pathmtu, SCTP_DEFAULT_MAXSEGMENT)
+ * This is a helper function to allocate an error chunk for
+ * for those invalid parameter codes in which we may not want
+ * to report all the errors, if the incomming chunk is large
+ */
+static inline struct sctp_chunk *sctp_make_op_error_fixed(
+	const struct sctp_association *asoc,
+	const struct sctp_chunk *chunk)
+{
+	size_t size = asoc ? asoc->pathmtu : 0;
+
+	if (!size)
+		size = SCTP_DEFAULT_MAXSEGMENT;
+
+	return sctp_make_op_error_space(asoc, chunk, size);
+}
+
 /* Create an Operation Error chunk.  */
 struct sctp_chunk *sctp_make_op_error(const struct sctp_association *asoc,
 				 const struct sctp_chunk *chunk,
@@ -1373,6 +1414,18 @@ void *sctp_addto_chunk(struct sctp_chunk *chunk, int len, const void *data)
 	return target;
 }
 
+/* Append bytes to the end of a chunk. Returns NULL if there isn't sufficient
+ * space in the chunk
+ */
+void *sctp_addto_chunk_fixed(struct sctp_chunk *chunk,
+			     int len, const void *data)
+{
+	if (skb_tailroom(chunk->skb) > len)
+		return sctp_addto_chunk(chunk, len, data);
+	else
+		return NULL;
+}
+
 /* Append bytes from user space to the end of a chunk.  Will panic if
  * chunk is not big enough.
  * Returns a kernel err value.
@@ -1976,13 +2029,12 @@ static sctp_ierror_t sctp_process_unk_param(const struct sctp_association *asoc,
 		 * returning multiple unknown parameters.
 		 */
 		if (NULL == *errp)
-			*errp = sctp_make_op_error_space(asoc, chunk,
-					ntohs(chunk->chunk_hdr->length));
+			*errp = sctp_make_op_error_fixed(asoc, chunk);
 
 		if (*errp) {
-			sctp_init_cause(*errp, SCTP_ERROR_UNKNOWN_PARAM,
+			sctp_init_cause_fixed(*errp, SCTP_ERROR_UNKNOWN_PARAM,
 					WORD_ROUND(ntohs(param.p->length)));
-			sctp_addto_chunk(*errp,
+			sctp_addto_chunk_fixed(*errp,
 					WORD_ROUND(ntohs(param.p->length)),
 					param.v);
 		} else {

Re: [PATCH]: sctp: Fix skb_over_panic resulting from multiple invalid parameter errors (CVE-2010-1173) (v4)

[Vlad Yasevich]

Looks good.

Acked-by: Vlad Yasevich <vladislav.yasevich@hp.com>

-vlad

Neil Horman wrote:
> Ok, version 4
> 
> Change Notes:
> 1) Minor cleanups, from Vlads notes
> 
> Summary:
> 
> 
> Hey-
> 	Recently, it was reported to me that the kernel could oops in the
> following way:
> 
> <5> kernel BUG at net/core/skbuff.c:91!
> <5> invalid operand: 0000 [#1]
> <5> Modules linked in: sctp netconsole nls_utf8 autofs4 sunrpc iptable_filter
> ip_tables cpufreq_powersave parport_pc lp parport vmblock(U) vsock(U) vmci(U)
> vmxnet(U) vmmemctl(U) vmhgfs(U) acpiphp dm_mirror dm_mod button battery ac md5
> ipv6 uhci_hcd ehci_hcd snd_ens1371 snd_rawmidi snd_seq_device snd_pcm_oss
> snd_mixer_oss snd_pcm snd_timer snd_page_alloc snd_ac97_codec snd soundcore
> pcnet32 mii floppy ext3 jbd ata_piix libata mptscsih mptsas mptspi mptscsi
> mptbase sd_mod scsi_mod
> <5> CPU:    0
> <5> EIP:    0060:[<c02bff27>]    Not tainted VLI
> <5> EFLAGS: 00010216   (2.6.9-89.0.25.EL) 
> <5> EIP is at skb_over_panic+0x1f/0x2d
> <5> eax: 0000002c   ebx: c033f461   ecx: c0357d96   edx: c040fd44
> <5> esi: c033f461   edi: df653280   ebp: 00000000   esp: c040fd40
> <5> ds: 007b   es: 007b   ss: 0068
> <5> Process swapper (pid: 0, threadinfo=c040f000 task=c0370be0)
> <5> Stack: c0357d96 e0c29478 00000084 00000004 c033f461 df653280 d7883180
> e0c2947d 
> <5>        00000000 00000080 df653490 00000004 de4f1ac0 de4f1ac0 00000004
> df653490 
> <5>        00000001 e0c2877a 08000800 de4f1ac0 df653490 00000000 e0c29d2e
> 00000004 
> <5> Call Trace:
> <5>  [<e0c29478>] sctp_addto_chunk+0xb0/0x128 [sctp]
> <5>  [<e0c2947d>] sctp_addto_chunk+0xb5/0x128 [sctp]
> <5>  [<e0c2877a>] sctp_init_cause+0x3f/0x47 [sctp]
> <5>  [<e0c29d2e>] sctp_process_unk_param+0xac/0xb8 [sctp]
> <5>  [<e0c29e90>] sctp_verify_init+0xcc/0x134 [sctp]
> <5>  [<e0c20322>] sctp_sf_do_5_1B_init+0x83/0x28e [sctp]
> <5>  [<e0c25333>] sctp_do_sm+0x41/0x77 [sctp]
> <5>  [<c01555a4>] cache_grow+0x140/0x233
> <5>  [<e0c26ba1>] sctp_endpoint_bh_rcv+0xc5/0x108 [sctp]
> <5>  [<e0c2b863>] sctp_inq_push+0xe/0x10 [sctp]
> <5>  [<e0c34600>] sctp_rcv+0x454/0x509 [sctp]
> <5>  [<e084e017>] ipt_hook+0x17/0x1c [iptable_filter]
> <5>  [<c02d005e>] nf_iterate+0x40/0x81
> <5>  [<c02e0bb9>] ip_local_deliver_finish+0x0/0x151
> <5>  [<c02e0c7f>] ip_local_deliver_finish+0xc6/0x151
> <5>  [<c02d0362>] nf_hook_slow+0x83/0xb5
> <5>  [<c02e0bb2>] ip_local_deliver+0x1a2/0x1a9
> <5>  [<c02e0bb9>] ip_local_deliver_finish+0x0/0x151
> <5>  [<c02e103e>] ip_rcv+0x334/0x3b4
> <5>  [<c02c66fd>] netif_receive_skb+0x320/0x35b
> <5>  [<e0a0928b>] init_stall_timer+0x67/0x6a [uhci_hcd]
> <5>  [<c02c67a4>] process_backlog+0x6c/0xd9
> <5>  [<c02c690f>] net_rx_action+0xfe/0x1f8
> <5>  [<c012a7b1>] __do_softirq+0x35/0x79
> <5>  [<c0107efb>] handle_IRQ_event+0x0/0x4f
> <5>  [<c01094de>] do_softirq+0x46/0x4d
> 
> Its an skb_over_panic BUG halt that results from processing an init chunk in
> which too many of its variable length parameters are in some way malformed.
> 
> The problem is in sctp_process_unk_param:
> if (NULL == *errp)
> 	*errp = sctp_make_op_error_space(asoc, chunk,
> 					 ntohs(chunk->chunk_hdr->length));
> 
> 	if (*errp) {
> 		sctp_init_cause(*errp, SCTP_ERROR_UNKNOWN_PARAM,
> 				 WORD_ROUND(ntohs(param.p->length)));
> 		sctp_addto_chunk(*errp,
> 			WORD_ROUND(ntohs(param.p->length)),
> 				  param.v);
> 
> When we allocate an error chunk, we assume that the worst case scenario requires
> that we have chunk_hdr->length data allocated, which would be correct nominally,
> given that we call sctp_addto_chunk for the violating parameter.  Unfortunately,
> we also, in sctp_init_cause insert a sctp_errhdr_t structure into the error
> chunk, so the worst case situation in which all parameters are in violation
> requires chunk_hdr->length+(sizeof(sctp_errhdr_t)*param_count) bytes of data.
> 
> The result of this error is that a deliberately malformed packet sent to a
> listening host can cause a remote DOS, described in CVE-2010-1173:
> http://cve.mitre.org/cgi-bin/cvename.cgi?name=2010-1173
> 
> I've tested the below fix and confirmed that it fixes the issue.  We move to a
> strategy whereby we allocate a fixed size error chunk and ignore errors we don't
> have space to report.  Tested by me successfully
> 
> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> 
> 
>  include/net/sctp/structs.h |    1 
>  net/sctp/sm_make_chunk.c   |   62 +++++++++++++++++++++++++++++++++++++++++----
>  2 files changed, 58 insertions(+), 5 deletions(-)
> 
> 
> diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
> index ff30177..597f8e2 100644
> --- a/include/net/sctp/structs.h
> +++ b/include/net/sctp/structs.h
> @@ -778,6 +778,7 @@ int sctp_user_addto_chunk(struct sctp_chunk *chunk, int off, int len,
>  			  struct iovec *data);
>  void sctp_chunk_free(struct sctp_chunk *);
>  void  *sctp_addto_chunk(struct sctp_chunk *, int len, const void *data);
> +void  *sctp_addto_chunk_fixed(struct sctp_chunk *, int len, const void *data);
>  struct sctp_chunk *sctp_chunkify(struct sk_buff *,
>  				 const struct sctp_association *,
>  				 struct sock *);
> diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
> index f592163..2971731 100644
> --- a/net/sctp/sm_make_chunk.c
> +++ b/net/sctp/sm_make_chunk.c
> @@ -107,7 +107,7 @@ static const struct sctp_paramhdr prsctp_param = {
>  	cpu_to_be16(sizeof(struct sctp_paramhdr)),
>  };
>  
> -/* A helper to initialize to initialize an op error inside a
> +/* A helper to initialize an op error inside a
>   * provided chunk, as most cause codes will be embedded inside an
>   * abort chunk.
>   */
> @@ -124,6 +124,29 @@ void  sctp_init_cause(struct sctp_chunk *chunk, __be16 cause_code,
>  	chunk->subh.err_hdr = sctp_addto_chunk(chunk, sizeof(sctp_errhdr_t), &err);
>  }
>  
> +/* A helper to initialize an op error inside a
> + * provided chunk, as most cause codes will be embedded inside an
> + * abort chunk.  Differs from sctp_init_cause in that it won't oops
> + * if there isn't enough space in the op error chunk
> + */
> +int sctp_init_cause_fixed(struct sctp_chunk *chunk, __be16 cause_code,
> +		      size_t paylen)
> +{
> +	sctp_errhdr_t err;
> +	__u16 len;
> +
> +	/* Cause code constants are now defined in network order.  */
> +	err.cause = cause_code;
> +	len = sizeof(sctp_errhdr_t) + paylen;
> +	err.length  = htons(len);
> +
> +	if (skb_tailroom(chunk->skb) >  len)
> +		return -ENOSPC;
> +	chunk->subh.err_hdr = sctp_addto_chunk_fixed(chunk,
> +						     sizeof(sctp_errhdr_t),
> +						     &err);
> +	return 0;
> +}
>  /* 3.3.2 Initiation (INIT) (1)
>   *
>   * This chunk is used to initiate a SCTP association between two
> @@ -1131,6 +1154,24 @@ nodata:
>  	return retval;
>  }
>  
> +/* Create an Operation Error chunk of a fixed size,
> + * specifically, max(asoc->pathmtu, SCTP_DEFAULT_MAXSEGMENT)
> + * This is a helper function to allocate an error chunk for
> + * for those invalid parameter codes in which we may not want
> + * to report all the errors, if the incomming chunk is large
> + */
> +static inline struct sctp_chunk *sctp_make_op_error_fixed(
> +	const struct sctp_association *asoc,
> +	const struct sctp_chunk *chunk)
> +{
> +	size_t size = asoc ? asoc->pathmtu : 0;
> +
> +	if (!size)
> +		size = SCTP_DEFAULT_MAXSEGMENT;
> +
> +	return sctp_make_op_error_space(asoc, chunk, size);
> +}
> +
>  /* Create an Operation Error chunk.  */
>  struct sctp_chunk *sctp_make_op_error(const struct sctp_association *asoc,
>  				 const struct sctp_chunk *chunk,
> @@ -1373,6 +1414,18 @@ void *sctp_addto_chunk(struct sctp_chunk *chunk, int len, const void *data)
>  	return target;
>  }
>  
> +/* Append bytes to the end of a chunk. Returns NULL if there isn't sufficient
> + * space in the chunk
> + */
> +void *sctp_addto_chunk_fixed(struct sctp_chunk *chunk,
> +			     int len, const void *data)
> +{
> +	if (skb_tailroom(chunk->skb) > len)
> +		return sctp_addto_chunk(chunk, len, data);
> +	else
> +		return NULL;
> +}
> +
>  /* Append bytes from user space to the end of a chunk.  Will panic if
>   * chunk is not big enough.
>   * Returns a kernel err value.
> @@ -1976,13 +2029,12 @@ static sctp_ierror_t sctp_process_unk_param(const struct sctp_association *asoc,
>  		 * returning multiple unknown parameters.
>  		 */
>  		if (NULL == *errp)
> -			*errp = sctp_make_op_error_space(asoc, chunk,
> -					ntohs(chunk->chunk_hdr->length));
> +			*errp = sctp_make_op_error_fixed(asoc, chunk);
>  
>  		if (*errp) {
> -			sctp_init_cause(*errp, SCTP_ERROR_UNKNOWN_PARAM,
> +			sctp_init_cause_fixed(*errp, SCTP_ERROR_UNKNOWN_PARAM,
>  					WORD_ROUND(ntohs(param.p->length)));
> -			sctp_addto_chunk(*errp,
> +			sctp_addto_chunk_fixed(*errp,
>  					WORD_ROUND(ntohs(param.p->length)),
>  					param.v);
>  		} else {
> 

Re: [PATCH]: sctp: Fix skb_over_panic resulting from multiple invalid parameter errors (CVE-2010-1173) (v4)

[David Miller]

From: Vlad Yasevich <vladislav.yasevich@hp.com>
Date: Wed, 28 Apr 2010 16:37:04 -0400

> 
> Looks good.
> 
> Acked-by: Vlad Yasevich <vladislav.yasevich@hp.com>

Applied, thanks Neil and Vlad.

Re: [PATCH]: sctp: Fix skb_over_panic resulting from multiple invalid parameter errors (CVE-2010-1173) (v4)

[Neil Horman]

On Wed, Apr 28, 2010 at 02:23:39PM -0700, David Miller wrote:
> From: Vlad Yasevich <vladislav.yasevich@hp.com>
> Date: Wed, 28 Apr 2010 16:37:04 -0400
> 
> > 
> > Looks good.
> > 
> > Acked-by: Vlad Yasevich <vladislav.yasevich@hp.com>
> 
> Applied, thanks Neil and Vlad.
> 
Thanks!
Neil

Re: [PATCH]: sctp: Fix skb_over_panic resulting from multiple invalid parameter errors (CVE-2010-1173) (v4)

[Eugene Teo]

On 04/29/2010 05:50 AM, Neil Horman wrote:
> On Wed, Apr 28, 2010 at 02:23:39PM -0700, David Miller wrote:
>> From: Vlad Yasevich<vladislav.yasevich@hp.com>
>> Date: Wed, 28 Apr 2010 16:37:04 -0400
>>
>>>
>>> Looks good.
>>>
>>> Acked-by: Vlad Yasevich<vladislav.yasevich@hp.com>
>>
>> Applied, thanks Neil and Vlad.
>>
> Thanks!

Thanks :)

Eugene