| Message ID | 4D46EADC.7050002@canonical.com |
|---|---|
| State | Accepted |
| Delegated to: | Stefan Bader |
| Headers | show |
On Mon, Jan 31, 2011 at 10:01:16AM -0700, Tim Gardner wrote: > On 01/31/2011 03:25 AM, Andy Whitcroft wrote: > >On Fri, Jan 28, 2011 at 01:35:26PM -0700, Tim Gardner wrote: > >>The following changes since commit 9f646039dcba8632e8d68f73652ce6949d6b20ba: > >> David S. Miller (1): > >> net: Limit socket I/O iovec total length to INT_MAX., CVE-2010-3859 > >> > >>are available in the git repository at: > >> > >> git://kernel.ubuntu.com/rtg/ubuntu-dapper.git CVE-2010-3873 > >> > >>Tim Gardner (1): > >> memory corruption in X.25 facilities parsing, CVE-2010-3873 > >> > >> net/x25/x25_in.c | 10 +++++++--- > >> 1 files changed, 7 insertions(+), 3 deletions(-) > >> > >> From 92700cf43dcb6ffdd998550a2ecadb7a2343e222 Mon Sep 17 00:00:00 2001 > >>From: Tim Gardner<tim.gardner@canonical.com> > >>Date: Fri, 28 Jan 2011 11:17:01 -0700 > >>Subject: [PATCH] memory corruption in X.25 facilities parsing, CVE-2010-3873 > >> > >>BugLink: http://bugs.launchpad.net/bugs/709372 > >> > >>CVE-2010-3873 > >> > >>Partial backport from a6331d6f9a4298173b413cf99a40cc86a9d92c37 > >>by Tim Gardner<tim.gardner@canonical.com> > >> > >>Signed-of-by: Andrew Hendry<andrew.hendry@gmail.com> > >> > >>Signed-off-by: David S. Miller<davem@davemloft.net> > >>Signed-off-by: Tim Gardner<tim.gardner@canonical.com> > >>--- > >> net/x25/x25_in.c | 10 +++++++--- > >> 1 files changed, 7 insertions(+), 3 deletions(-) > >> > >>diff --git a/net/x25/x25_in.c b/net/x25/x25_in.c > >>index 2614687..659252b 100644 > >>--- a/net/x25/x25_in.c > >>+++ b/net/x25/x25_in.c > >>@@ -90,6 +90,7 @@ static int x25_state1_machine(struct sock *sk, struct sk_buff *skb, int frametyp > >> switch (frametype) { > >> case X25_CALL_ACCEPTED: { > >> struct x25_sock *x25 = x25_sk(sk); > >>+ int len; > >> > >> x25_stop_timer(sk); > >> x25->condition = 0x00; > >>@@ -104,9 +105,12 @@ static int x25_state1_machine(struct sock *sk, struct sk_buff *skb, int frametyp > >> */ > >> skb_pull(skb, X25_STD_MIN_LEN); > >> skb_pull(skb, x25_addr_ntoa(skb->data,&source_addr,&dest_addr)); > >>- skb_pull(skb, > >>- x25_parse_facilities(skb,&x25->facilities, > >>- &x25->vc_facil_mask)); > >>+ len = x25_parse_facilities(skb,&x25->facilities, > >>+ &x25->vc_facil_mask); > >>+ if (len<= 0) > >>+ return -1; > >>+ skb_pull(skb, len); > >>+ > >> /* > >> * Copy any Call User Data. > >> */ > > > >As we do not support CLASS_D at all in Dapper it is not clear that we > >can ever return a negative or zero length from x25_parse_facilities(). > >This is however safe and matches upstream. > > > >I do have one concern, and this applies to the change as applied to > >upstream and not a specific concern with any of the backports. I think > >that returning -1 here will trigger the calling backlog handler to think > >that we have queued the packet for further processing (which we have not) > >and thus we will leak the skb in the code below: > > > > int x25_backlog_rcv(struct sock *sk, struct sk_buff *skb) > > { > > int queued = x25_process_rx_frame(sk, skb); > > > > if (!queued) > > kfree_skb(skb); > > > > return 0; > > } > > > >I will separatly ask about this upstream, as this may mean we are > >replacing a remote corrupter with a remote DOS. > > > >-apw > > > > Your observation seems obviously correct to me. Without waiting for > upstream, how about the attached ? > > rtg > -- > Tim Gardner tim.gardner@canonical.com > From 6ca583d619cacd4b0fe9cd183de2608ca8780b9d Mon Sep 17 00:00:00 2001 > From: Tim Gardner <tim.gardner@canonical.com> > Date: Fri, 28 Jan 2011 11:17:01 -0700 > Subject: [PATCH] memory corruption in X.25 facilities parsing, CVE-2010-3873 > > BugLink: http://bugs.launchpad.net/bugs/709372 > > CVE-2010-3873 > > Partial backport from a6331d6f9a4298173b413cf99a40cc86a9d92c37 > by Tim Gardner <tim.gardner@canonical.com> > > Signed-of-by: Andrew Hendry <andrew.hendry@gmail.com> > > Signed-off-by: David S. Miller <davem@davemloft.net> > Signed-off-by: Tim Gardner <tim.gardner@canonical.com> > --- > net/x25/x25_in.c | 10 +++++++--- > 1 files changed, 7 insertions(+), 3 deletions(-) > > diff --git a/net/x25/x25_in.c b/net/x25/x25_in.c > index 2614687..85eb3cb 100644 > --- a/net/x25/x25_in.c > +++ b/net/x25/x25_in.c > @@ -90,6 +90,7 @@ static int x25_state1_machine(struct sock *sk, struct sk_buff *skb, int frametyp > switch (frametype) { > case X25_CALL_ACCEPTED: { > struct x25_sock *x25 = x25_sk(sk); > + int len; > > x25_stop_timer(sk); > x25->condition = 0x00; > @@ -104,9 +105,12 @@ static int x25_state1_machine(struct sock *sk, struct sk_buff *skb, int frametyp > */ > skb_pull(skb, X25_STD_MIN_LEN); > skb_pull(skb, x25_addr_ntoa(skb->data, &source_addr, &dest_addr)); > - skb_pull(skb, > - x25_parse_facilities(skb, &x25->facilities, > - &x25->vc_facil_mask)); > + len = x25_parse_facilities(skb, &x25->facilities, > + &x25->vc_facil_mask); > + if (len <= 0) > + return 0; > + skb_pull(skb, len); > + > /* > * Copy any Call User Data. > */ > -- > 1.7.0.4 > Upstream seems to concur this will leak with -1, and that 0 is better here. Acked-by: Andy Whitcroft <apw@canonical.com> -apw
From 6ca583d619cacd4b0fe9cd183de2608ca8780b9d Mon Sep 17 00:00:00 2001 From: Tim Gardner <tim.gardner@canonical.com> Date: Fri, 28 Jan 2011 11:17:01 -0700 Subject: [PATCH] memory corruption in X.25 facilities parsing, CVE-2010-3873 BugLink: http://bugs.launchpad.net/bugs/709372 CVE-2010-3873 Partial backport from a6331d6f9a4298173b413cf99a40cc86a9d92c37 by Tim Gardner <tim.gardner@canonical.com> Signed-of-by: Andrew Hendry <andrew.hendry@gmail.com> Signed-off-by: David S. Miller <davem@davemloft.net> Signed-off-by: Tim Gardner <tim.gardner@canonical.com> --- net/x25/x25_in.c | 10 +++++++--- 1 files changed, 7 insertions(+), 3 deletions(-) diff --git a/net/x25/x25_in.c b/net/x25/x25_in.c index 2614687..85eb3cb 100644 --- a/net/x25/x25_in.c +++ b/net/x25/x25_in.c @@ -90,6 +90,7 @@ static int x25_state1_machine(struct sock *sk, struct sk_buff *skb, int frametyp switch (frametype) { case X25_CALL_ACCEPTED: { struct x25_sock *x25 = x25_sk(sk); + int len; x25_stop_timer(sk); x25->condition = 0x00; @@ -104,9 +105,12 @@ static int x25_state1_machine(struct sock *sk, struct sk_buff *skb, int frametyp */ skb_pull(skb, X25_STD_MIN_LEN); skb_pull(skb, x25_addr_ntoa(skb->data, &source_addr, &dest_addr)); - skb_pull(skb, - x25_parse_facilities(skb, &x25->facilities, - &x25->vc_facil_mask)); + len = x25_parse_facilities(skb, &x25->facilities, + &x25->vc_facil_mask); + if (len <= 0) + return 0; + skb_pull(skb, len); + /* * Copy any Call User Data. */ -- 1.7.0.4