Patchwork Karmic CVE-2010-3873, memory corruption in X.25 facilities parsing (V2)

login
register
mail settings
Submitter Tim Gardner
Date Jan. 31, 2011, 5:17 p.m.
Message ID <4D46EEBF.3030501@canonical.com>
Download mbox | patch
Permalink /patch/81186/
State Accepted
Delegated to: Stefan Bader
Headers show

Comments

Tim Gardner - Jan. 31, 2011, 5:17 p.m.
Return 0 instead of -1 in order to avoid SKB leak.

rtg
Stefan Bader - Feb. 1, 2011, 2:18 p.m.
On 01/31/2011 06:17 PM, Tim Gardner wrote:
> Return 0 instead of -1 in order to avoid SKB leak.
> 
> rtg

ACK
Andy Whitcroft - Feb. 1, 2011, 2:39 p.m.
On Mon, Jan 31, 2011 at 10:17:51AM -0700, Tim Gardner wrote:
> Return 0 instead of -1 in order to avoid SKB leak.
> 
> rtg
> -- 
> Tim Gardner tim.gardner@canonical.com

> From 61e517424b490d5c1a50543c9606fdab758719ba Mon Sep 17 00:00:00 2001
> From: andrew hendry <andrew.hendry@gmail.com>
> Date: Wed, 3 Nov 2010 12:54:53 +0000
> Subject: [PATCH] memory corruption in X.25 facilities parsing, CVE-2010-3873
> 
> BugLink: http://bugs.launchpad.net/bugs/709372
> 
> CVE-2010-3873
> 
> Backported 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_facilities.c |    8 ++++----
>  net/x25/x25_in.c         |   11 +++++++----
>  2 files changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/net/x25/x25_facilities.c b/net/x25/x25_facilities.c
> index a21f664..e233de3 100644
> --- a/net/x25/x25_facilities.c
> +++ b/net/x25/x25_facilities.c
> @@ -126,15 +126,15 @@ int x25_parse_facilities(struct sk_buff *skb, struct x25_facilities *facilities,
>  		case X25_FAC_CLASS_D:
>  			switch (*p) {
>  			case X25_FAC_CALLING_AE:
> -				if (p[1] > X25_MAX_DTE_FACIL_LEN)
> -					break;
> +				if (p[1] > X25_MAX_DTE_FACIL_LEN || p[1] <= 1)
> +					return 0;
>  				dte_facs->calling_len = p[2];
>  				memcpy(dte_facs->calling_ae, &p[3], p[1] - 1);
>  				*vc_fac_mask |= X25_MASK_CALLING_AE;
>  				break;
>  			case X25_FAC_CALLED_AE:
> -				if (p[1] > X25_MAX_DTE_FACIL_LEN)
> -					break;
> +				if (p[1] > X25_MAX_DTE_FACIL_LEN || p[1] <= 1)
> +					return 0;
>  				dte_facs->called_len = p[2];
>  				memcpy(dte_facs->called_ae, &p[3], p[1] - 1);
>  				*vc_fac_mask |= X25_MASK_CALLED_AE;
> diff --git a/net/x25/x25_in.c b/net/x25/x25_in.c
> index 7d7c3ab..45d6c22 100644
> --- a/net/x25/x25_in.c
> +++ b/net/x25/x25_in.c
> @@ -93,6 +93,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;
> @@ -107,10 +108,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->dte_facilities,
> -						&x25->vc_facil_mask));
> +			len = x25_parse_facilities(skb, &x25->facilities,
> +				&x25->dte_facilities, &x25->vc_facil_mask);
> +			if (len <= 0)
> +				return 0;
> +			skb_pull(skb, len);
> +
>  			/*
>  			 *	Copy any Call User Data.
>  			 */

Looks good to me.

Acked-by: Andy Whitcroft <apw@canonical.com>

-apw

Patch

From 61e517424b490d5c1a50543c9606fdab758719ba Mon Sep 17 00:00:00 2001
From: andrew hendry <andrew.hendry@gmail.com>
Date: Wed, 3 Nov 2010 12:54:53 +0000
Subject: [PATCH] memory corruption in X.25 facilities parsing, CVE-2010-3873

BugLink: http://bugs.launchpad.net/bugs/709372

CVE-2010-3873

Backported 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_facilities.c |    8 ++++----
 net/x25/x25_in.c         |   11 +++++++----
 2 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/net/x25/x25_facilities.c b/net/x25/x25_facilities.c
index a21f664..e233de3 100644
--- a/net/x25/x25_facilities.c
+++ b/net/x25/x25_facilities.c
@@ -126,15 +126,15 @@  int x25_parse_facilities(struct sk_buff *skb, struct x25_facilities *facilities,
 		case X25_FAC_CLASS_D:
 			switch (*p) {
 			case X25_FAC_CALLING_AE:
-				if (p[1] > X25_MAX_DTE_FACIL_LEN)
-					break;
+				if (p[1] > X25_MAX_DTE_FACIL_LEN || p[1] <= 1)
+					return 0;
 				dte_facs->calling_len = p[2];
 				memcpy(dte_facs->calling_ae, &p[3], p[1] - 1);
 				*vc_fac_mask |= X25_MASK_CALLING_AE;
 				break;
 			case X25_FAC_CALLED_AE:
-				if (p[1] > X25_MAX_DTE_FACIL_LEN)
-					break;
+				if (p[1] > X25_MAX_DTE_FACIL_LEN || p[1] <= 1)
+					return 0;
 				dte_facs->called_len = p[2];
 				memcpy(dte_facs->called_ae, &p[3], p[1] - 1);
 				*vc_fac_mask |= X25_MASK_CALLED_AE;
diff --git a/net/x25/x25_in.c b/net/x25/x25_in.c
index 7d7c3ab..45d6c22 100644
--- a/net/x25/x25_in.c
+++ b/net/x25/x25_in.c
@@ -93,6 +93,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;
@@ -107,10 +108,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->dte_facilities,
-						&x25->vc_facil_mask));
+			len = x25_parse_facilities(skb, &x25->facilities,
+				&x25->dte_facilities, &x25->vc_facil_mask);
+			if (len <= 0)
+				return 0;
+			skb_pull(skb, len);
+
 			/*
 			 *	Copy any Call User Data.
 			 */
-- 
1.7.0.4