From mboxrd@z Thu Jan 1 00:00:00 1970 From: Vlad Yasevich Date: Wed, 28 Apr 2010 20:37:04 +0000 Subject: Re: [PATCH]: sctp: Fix skb_over_panic resulting from multiple invalid Message-Id: <4BD89C70.6080406@hp.com> List-Id: References: <20100428134748.GA4818@hmsreliant.think-freely.org> <4BD83F85.8090308@hp.com> <20100428142147.GB4818@hmsreliant.think-freely.org> <4BD8481E.3010509@hp.com> <20100428174711.GC4818@hmsreliant.think-freely.org> <20100428193755.GF4818@hmsreliant.think-freely.org> <4BD897B6.5040405@hp.com> <20100428203059.GG4818@hmsreliant.think-freely.org> In-Reply-To: <20100428203059.GG4818@hmsreliant.think-freely.org> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable To: Neil Horman Cc: sri@us.ibm.com, linux-sctp@vger.kernel.org, eteo@redhat.com, netdev@vger.kernel.org, davem@davemloft.net, security@kernel.org Looks good. Acked-by: Vlad Yasevich -vlad Neil Horman wrote: > Ok, version 4 >=20 > Change Notes: > 1) Minor cleanups, from Vlads notes >=20 > Summary: >=20 >=20 > Hey- > Recently, it was reported to me that the kernel could oops in the > following way: >=20 > <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_fi= lter > ip_tables cpufreq_powersave parport_pc lp parport vmblock(U) vsock(U) vmc= i(U) > vmxnet(U) vmmemctl(U) vmhgfs(U) acpiphp dm_mirror dm_mod button battery a= c 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 soundco= re > pcnet32 mii floppy ext3 jbd ata_piix libata mptscsih mptsas mptspi mptscsi > mptbase sd_mod scsi_mod > <5> CPU: 0 > <5> EIP: 0060:[] Not tainted VLI > <5> EFLAGS: 00010216 (2.6.9-89.0.25.EL)=20 > <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=20 > <5> 00000000 00000080 df653490 00000004 de4f1ac0 de4f1ac0 00000004 > df653490=20 > <5> 00000001 e0c2877a 08000800 de4f1ac0 df653490 00000000 e0c29d2e > 00000004=20 > <5> Call Trace: > <5> [] sctp_addto_chunk+0xb0/0x128 [sctp] > <5> [] sctp_addto_chunk+0xb5/0x128 [sctp] > <5> [] sctp_init_cause+0x3f/0x47 [sctp] > <5> [] sctp_process_unk_param+0xac/0xb8 [sctp] > <5> [] sctp_verify_init+0xcc/0x134 [sctp] > <5> [] sctp_sf_do_5_1B_init+0x83/0x28e [sctp] > <5> [] sctp_do_sm+0x41/0x77 [sctp] > <5> [] cache_grow+0x140/0x233 > <5> [] sctp_endpoint_bh_rcv+0xc5/0x108 [sctp] > <5> [] sctp_inq_push+0xe/0x10 [sctp] > <5> [] sctp_rcv+0x454/0x509 [sctp] > <5> [] ipt_hook+0x17/0x1c [iptable_filter] > <5> [] nf_iterate+0x40/0x81 > <5> [] ip_local_deliver_finish+0x0/0x151 > <5> [] ip_local_deliver_finish+0xc6/0x151 > <5> [] nf_hook_slow+0x83/0xb5 > <5> [] ip_local_deliver+0x1a2/0x1a9 > <5> [] ip_local_deliver_finish+0x0/0x151 > <5> [] ip_rcv+0x334/0x3b4 > <5> [] netif_receive_skb+0x320/0x35b > <5> [] init_stall_timer+0x67/0x6a [uhci_hcd] > <5> [] process_backlog+0x6c/0xd9 > <5> [] net_rx_action+0xfe/0x1f8 > <5> [] __do_softirq+0x35/0x79 > <5> [] handle_IRQ_event+0x0/0x4f > <5> [] do_softirq+0x46/0x4d >=20 > 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 malforme= d. >=20 > The problem is in sctp_process_unk_param: > if (NULL =3D *errp) > *errp =3D sctp_make_op_error_space(asoc, chunk, > ntohs(chunk->chunk_hdr->length)); >=20 > 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); >=20 > When we allocate an error chunk, we assume that the worst case scenario r= equires > that we have chunk_hdr->length data allocated, which would be correct nom= inally, > given that we call sctp_addto_chunk for the violating parameter. Unfortu= nately, > we also, in sctp_init_cause insert a sctp_errhdr_t structure into the err= or > chunk, so the worst case situation in which all parameters are in violati= on > requires chunk_hdr->length+(sizeof(sctp_errhdr_t)*param_count) bytes of d= ata. >=20 > 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 10-1173 >=20 > 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 w= e don't > have space to report. Tested by me successfully >=20 > Signed-off-by: Neil Horman >=20 >=20 > include/net/sctp/structs.h | 1=20 > net/sctp/sm_make_chunk.c | 62 ++++++++++++++++++++++++++++++++++++++= +++---- > 2 files changed, 58 insertions(+), 5 deletions(-) >=20 >=20 > 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, i= nt 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 =3D { > cpu_to_be16(sizeof(struct sctp_paramhdr)), > }; > =20 > -/* 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, __be= 16 cause_code, > chunk->subh.err_hdr =3D sctp_addto_chunk(chunk, sizeof(sctp_errhdr_t), = &err); > } > =20 > +/* 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 =3D cause_code; > + len =3D sizeof(sctp_errhdr_t) + paylen; > + err.length =3D htons(len); > + > + if (skb_tailroom(chunk->skb) > len) > + return -ENOSPC; > + chunk->subh.err_hdr =3D 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; > } > =20 > +/* 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 =3D asoc ? asoc->pathmtu : 0; > + > + if (!size) > + size =3D 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 *aso= c, > const struct sctp_chunk *chunk, > @@ -1373,6 +1414,18 @@ void *sctp_addto_chunk(struct sctp_chunk *chunk, i= nt len, const void *data) > return target; > } > =20 > +/* Append bytes to the end of a chunk. Returns NULL if there isn't suffi= cient > + * 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 =3D *errp) > - *errp =3D sctp_make_op_error_space(asoc, chunk, > - ntohs(chunk->chunk_hdr->length)); > + *errp =3D sctp_make_op_error_fixed(asoc, chunk); > =20 > 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 { >=20 From mboxrd@z Thu Jan 1 00:00:00 1970 From: Vlad Yasevich Subject: Re: [PATCH]: sctp: Fix skb_over_panic resulting from multiple invalid parameter errors (CVE-2010-1173) (v4) Date: Wed, 28 Apr 2010 16:37:04 -0400 Message-ID: <4BD89C70.6080406@hp.com> References: <20100428134748.GA4818@hmsreliant.think-freely.org> <4BD83F85.8090308@hp.com> <20100428142147.GB4818@hmsreliant.think-freely.org> <4BD8481E.3010509@hp.com> <20100428174711.GC4818@hmsreliant.think-freely.org> <20100428193755.GF4818@hmsreliant.think-freely.org> <4BD897B6.5040405@hp.com> <20100428203059.GG4818@hmsreliant.think-freely.org> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Cc: sri@us.ibm.com, linux-sctp@vger.kernel.org, eteo@redhat.com, netdev@vger.kernel.org, davem@davemloft.net, security@kernel.org To: Neil Horman Return-path: Received: from g1t0026.austin.hp.com ([15.216.28.33]:29792 "EHLO g1t0026.austin.hp.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751089Ab0D1UhI (ORCPT ); Wed, 28 Apr 2010 16:37:08 -0400 In-Reply-To: <20100428203059.GG4818@hmsreliant.think-freely.org> Sender: netdev-owner@vger.kernel.org List-ID: Looks good. Acked-by: Vlad Yasevich -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:[] 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> [] sctp_addto_chunk+0xb0/0x128 [sctp] > <5> [] sctp_addto_chunk+0xb5/0x128 [sctp] > <5> [] sctp_init_cause+0x3f/0x47 [sctp] > <5> [] sctp_process_unk_param+0xac/0xb8 [sctp] > <5> [] sctp_verify_init+0xcc/0x134 [sctp] > <5> [] sctp_sf_do_5_1B_init+0x83/0x28e [sctp] > <5> [] sctp_do_sm+0x41/0x77 [sctp] > <5> [] cache_grow+0x140/0x233 > <5> [] sctp_endpoint_bh_rcv+0xc5/0x108 [sctp] > <5> [] sctp_inq_push+0xe/0x10 [sctp] > <5> [] sctp_rcv+0x454/0x509 [sctp] > <5> [] ipt_hook+0x17/0x1c [iptable_filter] > <5> [] nf_iterate+0x40/0x81 > <5> [] ip_local_deliver_finish+0x0/0x151 > <5> [] ip_local_deliver_finish+0xc6/0x151 > <5> [] nf_hook_slow+0x83/0xb5 > <5> [] ip_local_deliver+0x1a2/0x1a9 > <5> [] ip_local_deliver_finish+0x0/0x151 > <5> [] ip_rcv+0x334/0x3b4 > <5> [] netif_receive_skb+0x320/0x35b > <5> [] init_stall_timer+0x67/0x6a [uhci_hcd] > <5> [] process_backlog+0x6c/0xd9 > <5> [] net_rx_action+0xfe/0x1f8 > <5> [] __do_softirq+0x35/0x79 > <5> [] handle_IRQ_event+0x0/0x4f > <5> [] 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 > > > 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 { >