| Message ID | 20110128203527.20780F89F8@sepang.rtg.net |
|---|---|
| State | Accepted |
| Delegated to: | Stefan Bader |
| Headers | show |
The change to the state1_machine function (applies to all patches) could have been slightly more like upstream which is if (len > 0) skb_pull(skb, len); else return -1; Though in the end it is the same and hopefully we neither need to backport the change that modified that code before. On 01/28/2011 09:35 PM, 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> Acked-by: Stefan Bader <stefan.bader@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. > */
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
applied and pushed
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(-)