[PATCH v2] ROSE: prevent heap corruption with bad facilities

[PATCH v2] ROSE: prevent heap corruption with bad facilities

[Dan Rosenberg]

When parsing the FAC_NATIONAL_DIGIS facilities field, it's possible for
a remote host to provide more digipeaters than expected, resulting in
heap corruption.  Check against ROSE_MAX_DIGIS to prevent overflows, and
abort facilities parsing on failure.

Additionally, when parsing the FAC_CCITT_DEST_NSAP and
FAC_CCITT_SRC_NSAP facilities fields, a remote host can provide a length
of less than 10, resulting in an underflow in a memcpy size, causing a
kernel panic due to massive heap corruption.  A length of greater than
20 results in a stack overflow of the callsign array.  Abort facilities
parsing on these invalid length values.

Signed-off-by: Dan Rosenberg <drosenberg@vsecurity.com>
Cc: stable@kernel.org
---
 net/rose/rose_subr.c |   18 ++++++++++++++++--
 1 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/net/rose/rose_subr.c b/net/rose/rose_subr.c
index 1734abb..6f5f0b5 100644
--- a/net/rose/rose_subr.c
+++ b/net/rose/rose_subr.c
@@ -290,10 +290,15 @@ static int rose_parse_national(unsigned char *p, struct rose_facilities_struct *
 				facilities->source_ndigis = 0;
 				facilities->dest_ndigis   = 0;
 				for (pt = p + 2, lg = 0 ; lg < l ; pt += AX25_ADDR_LEN, lg += AX25_ADDR_LEN) {
-					if (pt[6] & AX25_HBIT)
+					if (pt[6] & AX25_HBIT) {
+						if (facilities->dest_ndigis >= ROSE_MAX_DIGIS)
+							return -1;
 						memcpy(&facilities->dest_digis[facilities->dest_ndigis++], pt, AX25_ADDR_LEN);
-					else
+					} else {
+						if (facilities->source_ndigis >= ROSE_MAX_DIGIS)
+							return -1;
 						memcpy(&facilities->source_digis[facilities->source_ndigis++], pt, AX25_ADDR_LEN);
+					}
 				}
 			}
 			p   += l + 2;
@@ -333,6 +338,11 @@ static int rose_parse_ccitt(unsigned char *p, struct rose_facilities_struct *fac
 
 		case 0xC0:
 			l = p[1];
+
+			/* Prevent overflows*/
+			if (l < 10 || l > 20)
+				return -1;
+
 			if (*p == FAC_CCITT_DEST_NSAP) {
 				memcpy(&facilities->source_addr, p + 7, ROSE_ADDR_LEN);
 				memcpy(callsign, p + 12,   l - 10);
@@ -373,12 +383,16 @@ int rose_parse_facilities(unsigned char *p,
 			switch (*p) {
 			case FAC_NATIONAL:		/* National */
 				len = rose_parse_national(p + 1, facilities, facilities_len - 1);
+				if (len < 0)
+					return 0;
 				facilities_len -= len + 1;
 				p += len + 1;
 				break;
 
 			case FAC_CCITT:		/* CCITT */
 				len = rose_parse_ccitt(p + 1, facilities, facilities_len - 1);
+				if (len < 0)
+					return 0;
 				facilities_len -= len + 1;
 				p += len + 1;
 				break;

Re: [PATCH v2] ROSE: prevent heap corruption with bad facilities

[Ben Hutchings]

On Sun, 2011-03-20 at 02:43 -0400, Dan Rosenberg wrote:
> When parsing the FAC_NATIONAL_DIGIS facilities field, it's possible for
> a remote host to provide more digipeaters than expected, resulting in
> heap corruption.  Check against ROSE_MAX_DIGIS to prevent overflows, and
> abort facilities parsing on failure.
> 
> Additionally, when parsing the FAC_CCITT_DEST_NSAP and
> FAC_CCITT_SRC_NSAP facilities fields, a remote host can provide a length
> of less than 10, resulting in an underflow in a memcpy size, causing a
> kernel panic due to massive heap corruption.  A length of greater than
> 20 results in a stack overflow of the callsign array.  Abort facilities
> parsing on these invalid length values.
[...]

Look further and you'll see that the *entire* parsing code makes not the
slightest attempt to validate lengths.

This applies on top of your patch, and hopefully fixes the remaining
bugs.  It's compile-tested only.

Ben.

---
Subject: [PATCH] rose: Add length checks to CALL_REQUEST parsing

Define some constant offsets for CALL_REQUEST based on the description
at <http://www.techfest.com/networking/wan/x25plp.htm> and the
definition of ROSE as using 10-digit (5-byte) addresses.  Use them
consistently.  Validate all implicit and explicit facilities lengths.
Validate the address length byte rather than either trusting or
assuming its value.

Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
---
 include/net/rose.h       |    8 ++++-
 net/rose/af_rose.c       |    8 ++--
 net/rose/rose_loopback.c |   13 ++++++-
 net/rose/rose_route.c    |   20 +++++++----
 net/rose/rose_subr.c     |   91 +++++++++++++++++++++++++++++-----------------
 5 files changed, 93 insertions(+), 47 deletions(-)

diff --git a/include/net/rose.h b/include/net/rose.h
index 5ba9f02..555dd19 100644
--- a/include/net/rose.h
+++ b/include/net/rose.h
@@ -14,6 +14,12 @@
 
 #define	ROSE_MIN_LEN			3
 
+#define	ROSE_CALL_REQ_ADDR_LEN_OFF	3
+#define	ROSE_CALL_REQ_ADDR_LEN_VAL	0xAA	/* each address is 10 digits */
+#define	ROSE_CALL_REQ_DEST_ADDR_OFF	4
+#define	ROSE_CALL_REQ_SRC_ADDR_OFF	9
+#define	ROSE_CALL_REQ_FACILITIES_OFF	14
+
 #define	ROSE_GFI			0x10
 #define	ROSE_Q_BIT			0x80
 #define	ROSE_D_BIT			0x40
@@ -214,7 +220,7 @@ extern void rose_requeue_frames(struct sock *);
 extern int  rose_validate_nr(struct sock *, unsigned short);
 extern void rose_write_internal(struct sock *, int);
 extern int  rose_decode(struct sk_buff *, int *, int *, int *, int *, int *);
-extern int  rose_parse_facilities(unsigned char *, struct rose_facilities_struct *);
+extern int  rose_parse_facilities(unsigned char *, unsigned int, struct rose_facilities_struct *);
 extern void rose_disconnect(struct sock *, int, int, int);
 
 /* rose_timer.c */
diff --git a/net/rose/af_rose.c b/net/rose/af_rose.c
index 5ee0c62..a80aef6 100644
--- a/net/rose/af_rose.c
+++ b/net/rose/af_rose.c
@@ -978,7 +978,7 @@ int rose_rx_call_request(struct sk_buff *skb, struct net_device *dev, struct ros
 	struct sock *make;
 	struct rose_sock *make_rose;
 	struct rose_facilities_struct facilities;
-	int n, len;
+	int n;
 
 	skb->sk = NULL;		/* Initially we don't know who it's for */
 
@@ -987,9 +987,9 @@ int rose_rx_call_request(struct sk_buff *skb, struct net_device *dev, struct ros
 	 */
 	memset(&facilities, 0x00, sizeof(struct rose_facilities_struct));
 
-	len  = (((skb->data[3] >> 4) & 0x0F) + 1) >> 1;
-	len += (((skb->data[3] >> 0) & 0x0F) + 1) >> 1;
-	if (!rose_parse_facilities(skb->data + len + 4, &facilities)) {
+	if (!rose_parse_facilities(skb->data + ROSE_CALL_REQ_FACILITIES_OFF,
+				   skb->len - ROSE_CALL_REQ_FACILITIES_OFF,
+				   &facilities)) {
 		rose_transmit_clear_request(neigh, lci, ROSE_INVALID_FACILITY, 76);
 		return 0;
 	}
diff --git a/net/rose/rose_loopback.c b/net/rose/rose_loopback.c
index ae4a9d9..3444562 100644
--- a/net/rose/rose_loopback.c
+++ b/net/rose/rose_loopback.c
@@ -73,9 +73,20 @@ static void rose_loopback_timer(unsigned long param)
 	unsigned int lci_i, lci_o;
 
 	while ((skb = skb_dequeue(&loopback_queue)) != NULL) {
+		if (skb->len < ROSE_MIN_LEN) {
+			kfree_skb(skb);
+			continue;
+		}
 		lci_i     = ((skb->data[0] << 8) & 0xF00) + ((skb->data[1] << 0) & 0x0FF);
 		frametype = skb->data[2];
-		dest      = (rose_address *)(skb->data + 4);
+		if (frametype == ROSE_CALL_REQUEST &&
+		    (skb->len <= ROSE_CALL_REQ_FACILITIES_OFF ||
+		     skb->data[ROSE_CALL_REQ_ADDR_LEN_OFF] !=
+		     ROSE_CALL_REQ_ADDR_LEN_VAL)) {
+			kfree_skb(skb);
+			continue;
+		}
+		dest      = (rose_address *)(skb->data + ROSE_CALL_REQ_DEST_ADDR_OFF);
 		lci_o     = ROSE_DEFAULT_MAXVC + 1 - lci_i;
 
 		skb_reset_transport_header(skb);
diff --git a/net/rose/rose_route.c b/net/rose/rose_route.c
index 88a77e9..08dcd2f 100644
--- a/net/rose/rose_route.c
+++ b/net/rose/rose_route.c
@@ -861,7 +861,7 @@ int rose_route_frame(struct sk_buff *skb, ax25_cb *ax25)
 	unsigned int lci, new_lci;
 	unsigned char cause, diagnostic;
 	struct net_device *dev;
-	int len, res = 0;
+	int res = 0;
 	char buf[11];
 
 #if 0
@@ -869,10 +869,17 @@ int rose_route_frame(struct sk_buff *skb, ax25_cb *ax25)
 		return res;
 #endif
 
+	if (skb->len < ROSE_MIN_LEN)
+		return res;
 	frametype = skb->data[2];
 	lci = ((skb->data[0] << 8) & 0xF00) + ((skb->data[1] << 0) & 0x0FF);
-	src_addr  = (rose_address *)(skb->data + 9);
-	dest_addr = (rose_address *)(skb->data + 4);
+	if (frametype == ROSE_CALL_REQUEST &&
+	    (skb->len <= ROSE_CALL_REQ_FACILITIES_OFF ||
+	     skb->data[ROSE_CALL_REQ_ADDR_LEN_OFF] !=
+	     ROSE_CALL_REQ_ADDR_LEN_VAL))
+		return res;
+	src_addr  = (rose_address *)(skb->data + ROSE_CALL_REQ_SRC_ADDR_OFF);
+	dest_addr = (rose_address *)(skb->data + ROSE_CALL_REQ_DEST_ADDR_OFF);
 
 	spin_lock_bh(&rose_neigh_list_lock);
 	spin_lock_bh(&rose_route_list_lock);
@@ -1010,12 +1017,11 @@ int rose_route_frame(struct sk_buff *skb, ax25_cb *ax25)
 		goto out;
 	}
 
-	len  = (((skb->data[3] >> 4) & 0x0F) + 1) >> 1;
-	len += (((skb->data[3] >> 0) & 0x0F) + 1) >> 1;
-
 	memset(&facilities, 0x00, sizeof(struct rose_facilities_struct));
 
-	if (!rose_parse_facilities(skb->data + len + 4, &facilities)) {
+	if (!rose_parse_facilities(skb->data + ROSE_CALL_REQ_FACILITIES_OFF,
+				   skb->len - ROSE_CALL_REQ_FACILITIES_OFF,
+				   &facilities)) {
 		rose_transmit_clear_request(rose_neigh, lci, ROSE_INVALID_FACILITY, 76);
 		goto out;
 	}
diff --git a/net/rose/rose_subr.c b/net/rose/rose_subr.c
index 174d51c..f6c71ca 100644
--- a/net/rose/rose_subr.c
+++ b/net/rose/rose_subr.c
@@ -142,7 +142,7 @@ void rose_write_internal(struct sock *sk, int frametype)
 		*dptr++ = ROSE_GFI | lci1;
 		*dptr++ = lci2;
 		*dptr++ = frametype;
-		*dptr++ = 0xAA;
+		*dptr++ = ROSE_CALL_REQ_ADDR_LEN_VAL;
 		memcpy(dptr, &rose->dest_addr,  ROSE_ADDR_LEN);
 		dptr   += ROSE_ADDR_LEN;
 		memcpy(dptr, &rose->source_addr, ROSE_ADDR_LEN);
@@ -246,12 +246,16 @@ static int rose_parse_national(unsigned char *p, struct rose_facilities_struct *
 	do {
 		switch (*p & 0xC0) {
 		case 0x00:
+			if (len < 2)
+				return -1;
 			p   += 2;
 			n   += 2;
 			len -= 2;
 			break;
 
 		case 0x40:
+			if (len < 3)
+				return -1;
 			if (*p == FAC_NATIONAL_RAND)
 				facilities->rand = ((p[1] << 8) & 0xFF00) + ((p[2] << 0) & 0x00FF);
 			p   += 3;
@@ -260,32 +264,48 @@ static int rose_parse_national(unsigned char *p, struct rose_facilities_struct *
 			break;
 
 		case 0x80:
+			if (len < 4)
+				return -1;
 			p   += 4;
 			n   += 4;
 			len -= 4;
 			break;
 
 		case 0xC0:
+			if (len < 2)
+				return -1;
 			l = p[1];
+			if (len < 2 + l)
+				return -1;
 			if (*p == FAC_NATIONAL_DEST_DIGI) {
 				if (!fac_national_digis_received) {
+					if (l < AX25_ADDR_LEN)
+						return -1;
 					memcpy(&facilities->source_digis[0], p + 2, AX25_ADDR_LEN);
 					facilities->source_ndigis = 1;
 				}
 			}
 			else if (*p == FAC_NATIONAL_SRC_DIGI) {
 				if (!fac_national_digis_received) {
+					if (l < AX25_ADDR_LEN)
+						return -1;
 					memcpy(&facilities->dest_digis[0], p + 2, AX25_ADDR_LEN);
 					facilities->dest_ndigis = 1;
 				}
 			}
 			else if (*p == FAC_NATIONAL_FAIL_CALL) {
+				if (l < AX25_ADDR_LEN)
+					return -1;
 				memcpy(&facilities->fail_call, p + 2, AX25_ADDR_LEN);
 			}
 			else if (*p == FAC_NATIONAL_FAIL_ADD) {
+				if (l < 1 + ROSE_ADDR_LEN)
+					return -1;
 				memcpy(&facilities->fail_addr, p + 3, ROSE_ADDR_LEN);
 			}
 			else if (*p == FAC_NATIONAL_DIGIS) {
+				if (l % AX25_ADDR_LEN)
+					return -1;
 				fac_national_digis_received = 1;
 				facilities->source_ndigis = 0;
 				facilities->dest_ndigis   = 0;
@@ -319,24 +339,32 @@ static int rose_parse_ccitt(unsigned char *p, struct rose_facilities_struct *fac
 	do {
 		switch (*p & 0xC0) {
 		case 0x00:
+			if (len < 2)
+				return -1;
 			p   += 2;
 			n   += 2;
 			len -= 2;
 			break;
 
 		case 0x40:
+			if (len < 3)
+				return -1;
 			p   += 3;
 			n   += 3;
 			len -= 3;
 			break;
 
 		case 0x80:
+			if (len < 4)
+				return -1;
 			p   += 4;
 			n   += 4;
 			len -= 4;
 			break;
 
 		case 0xC0:
+			if (len < 2)
+				return -1;
 			l = p[1];
 
 			/* Prevent overflows*/
@@ -365,49 +393,44 @@ static int rose_parse_ccitt(unsigned char *p, struct rose_facilities_struct *fac
 	return n;
 }
 
-int rose_parse_facilities(unsigned char *p,
+int rose_parse_facilities(unsigned char *p, unsigned packet_len,
 	struct rose_facilities_struct *facilities)
 {
 	int facilities_len, len;
 
 	facilities_len = *p++;
 
-	if (facilities_len == 0)
+	if (facilities_len == 0 || (unsigned)facilities_len > packet_len)
 		return 0;
 
-	while (facilities_len > 0) {
-		if (*p == 0x00) {
-			facilities_len--;
-			p++;
-
-			switch (*p) {
-			case FAC_NATIONAL:		/* National */
-				len = rose_parse_national(p + 1, facilities, facilities_len - 1);
-				if (len < 0)
-					return 0;
-				facilities_len -= len + 1;
-				p += len + 1;
-				break;
-
-			case FAC_CCITT:		/* CCITT */
-				len = rose_parse_ccitt(p + 1, facilities, facilities_len - 1);
-				if (len < 0)
-					return 0;
-				facilities_len -= len + 1;
-				p += len + 1;
-				break;
-
-			default:
-				printk(KERN_DEBUG "ROSE: rose_parse_facilities - unknown facilities family %02X\n", *p);
-				facilities_len--;
-				p++;
-				break;
-			}
-		} else
-			break;	/* Error in facilities format */
+	while (facilities_len >= 3 && *p == 0x00) {
+		facilities_len--;
+		p++;
+
+		switch (*p) {
+		case FAC_NATIONAL:		/* National */
+			len = rose_parse_national(p + 1, facilities, facilities_len - 1);
+			break;
+
+		case FAC_CCITT:		/* CCITT */
+			len = rose_parse_ccitt(p + 1, facilities, facilities_len - 1);
+			break;
+
+		default:
+			printk(KERN_DEBUG "ROSE: rose_parse_facilities - unknown facilities family %02X\n", *p);
+			len = 1;
+			break;
+		}
+
+		if (len < 0)
+			return 0;
+		if (WARN_ON(len >= facilities_len))
+			return 0;
+		facilities_len -= len + 1;
+		p += len + 1;
 	}
 
-	return 1;
+	return facilities_len == 0;
 }
 
 static int rose_create_facilities(unsigned char *buffer, struct rose_sock *rose)
-- 
1.7.4.1

-- 
Ben Hutchings
Once a job is fouled up, anything done to improve it makes it worse.

Re: [PATCH v2] ROSE: prevent heap corruption with bad facilities

[David Miller]

From: Dan Rosenberg <drosenberg@vsecurity.com>
Date: Sun, 20 Mar 2011 02:43:43 -0400

> When parsing the FAC_NATIONAL_DIGIS facilities field, it's possible for
> a remote host to provide more digipeaters than expected, resulting in
> heap corruption.  Check against ROSE_MAX_DIGIS to prevent overflows, and
> abort facilities parsing on failure.
> 
> Additionally, when parsing the FAC_CCITT_DEST_NSAP and
> FAC_CCITT_SRC_NSAP facilities fields, a remote host can provide a length
> of less than 10, resulting in an underflow in a memcpy size, causing a
> kernel panic due to massive heap corruption.  A length of greater than
> 20 results in a stack overflow of the callsign array.  Abort facilities
> parsing on these invalid length values.
> 
> Signed-off-by: Dan Rosenberg <drosenberg@vsecurity.com>
> Cc: stable@kernel.org

Applied.

Re: [PATCH v2] ROSE: prevent heap corruption with bad facilities

[David Miller]

From: Ben Hutchings <ben@decadent.org.uk>
Date: Sun, 20 Mar 2011 16:48:05 +0000

> Subject: [PATCH] rose: Add length checks to CALL_REQUEST parsing
> 
> Define some constant offsets for CALL_REQUEST based on the description
> at <http://www.techfest.com/networking/wan/x25plp.htm> and the
> definition of ROSE as using 10-digit (5-byte) addresses.  Use them
> consistently.  Validate all implicit and explicit facilities lengths.
> Validate the address length byte rather than either trusting or
> assuming its value.
> 
> Signed-off-by: Ben Hutchings <ben@decadent.org.uk>

Applied.

Re: [PATCH v2] ROSE: prevent heap corruption with bad facilities

[Ben Hutchings]

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

On Sun, 2011-03-27 at 17:59 -0700, David Miller wrote:
> From: Ben Hutchings <ben@decadent.org.uk>
> Date: Sun, 20 Mar 2011 16:48:05 +0000
> 
> > Subject: [PATCH] rose: Add length checks to CALL_REQUEST parsing
> > 
> > Define some constant offsets for CALL_REQUEST based on the description
> > at <http://www.techfest.com/networking/wan/x25plp.htm> and the
> > definition of ROSE as using 10-digit (5-byte) addresses.  Use them
> > consistently.  Validate all implicit and explicit facilities lengths.
> > Validate the address length byte rather than either trusting or
> > assuming its value.
> > 
> > Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
> 
> Applied.

Ralf, I would really appreciate it if you could test this soon...

Ben.

-- 
Ben Hutchings
Once a job is fouled up, anything done to improve it makes it worse.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH v2] ROSE: prevent heap corruption with bad facilities

[Ralf Baechle]

On Tue, Mar 29, 2011 at 02:16:19AM +0100, Ben Hutchings wrote:

> On Sun, 2011-03-27 at 17:59 -0700, David Miller wrote:
> > From: Ben Hutchings <ben@decadent.org.uk>
> > Date: Sun, 20 Mar 2011 16:48:05 +0000
> > 
> > > Subject: [PATCH] rose: Add length checks to CALL_REQUEST parsing
> > > 
> > > Define some constant offsets for CALL_REQUEST based on the description
> > > at <http://www.techfest.com/networking/wan/x25plp.htm> and the
> > > definition of ROSE as using 10-digit (5-byte) addresses.  Use them
> > > consistently.  Validate all implicit and explicit facilities lengths.
> > > Validate the address length byte rather than either trusting or
> > > assuming its value.
> > > 
> > > Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
> > 
> > Applied.
> 
> Ralf, I would really appreciate it if you could test this soon...

Actual testing is a problem atm.  But I've reviewed the patche and it appears
ok.

Acked-by: Ralf Baechle <ralf@linux-mips.org>

  Ralf

Re: [PATCH v2] ROSE: prevent heap corruption with bad facilities

[Jiri Bohac]

On Sun, Mar 20, 2011 at 04:48:05PM +0000, Ben Hutchings wrote:
> @@ -365,49 +393,44 @@ static int rose_parse_ccitt(unsigned char *p, struct rose_facilities_struct *fac
>  	return n;
>  }
>  
> -int rose_parse_facilities(unsigned char *p,
> +int rose_parse_facilities(unsigned char *p, unsigned packet_len,
>  	struct rose_facilities_struct *facilities)
>  {
>  	int facilities_len, len;
>  
>  	facilities_len = *p++;
>  
> -	if (facilities_len == 0)
> +	if (facilities_len == 0 || (unsigned)facilities_len > packet_len)
>  		return 0;
>  
> -	while (facilities_len > 0) {
> -		if (*p == 0x00) {
> -			facilities_len--;
> -			p++;
> -
> -			switch (*p) {
> -			case FAC_NATIONAL:		/* National */
> -				len = rose_parse_national(p + 1, facilities, facilities_len - 1);
> -				if (len < 0)
> -					return 0;
> -				facilities_len -= len + 1;
> -				p += len + 1;
> -				break;
> -
> -			case FAC_CCITT:		/* CCITT */
> -				len = rose_parse_ccitt(p + 1, facilities, facilities_len - 1);
> -				if (len < 0)
> -					return 0;
> -				facilities_len -= len + 1;
> -				p += len + 1;
> -				break;
> -
> -			default:
> -				printk(KERN_DEBUG "ROSE: rose_parse_facilities - unknown facilities family %02X\n", *p);
> -				facilities_len--;
> -				p++;
> -				break;
> -			}
> -		} else
> -			break;	/* Error in facilities format */
> +	while (facilities_len >= 3 && *p == 0x00) {
> +		facilities_len--;
> +		p++;
> +
> +		switch (*p) {
> +		case FAC_NATIONAL:		/* National */
> +			len = rose_parse_national(p + 1, facilities, facilities_len - 1);
> +			break;
> +
> +		case FAC_CCITT:		/* CCITT */
> +			len = rose_parse_ccitt(p + 1, facilities, facilities_len - 1);
> +			break;
> +
> +		default:
> +			printk(KERN_DEBUG "ROSE: rose_parse_facilities - unknown facilities family %02X\n", *p);
> +			len = 1;
> +			break;
> +		}
> +
> +		if (len < 0)
> +			return 0;
> +		if (WARN_ON(len >= facilities_len))
> +			return 0;
> +		facilities_len -= len + 1;
> +		p += len + 1;
>  	}
>  
> -	return 1;
> +	return facilities_len == 0;
>  }


This last hunk does not look correct. In the default branch of
the switch, you set len = 1, which means 
	p += 2; facilities_len -= 2.

The original code does 
	facilities_len--; p++;
... and it looks correct. So, to get the old behaviour back:

diff --git a/net/rose/rose_subr.c b/net/rose/rose_subr.c
index f6c71ca..9777700 100644
--- a/net/rose/rose_subr.c
+++ b/net/rose/rose_subr.c
@@ -418,7 +418,7 @@ int rose_parse_facilities(unsigned char *p, unsigned packet_len,
 
 		default:
 			printk(KERN_DEBUG "ROSE: rose_parse_facilities - unknown facilities family %02X\n", *p);
-			len = 1;
+			len = 0;
 			break;
 		}

However, I wonder how much sense it makes to continue parsing the
facilities if an unknown facility family appears. We don't know
the length of its data, so we will interpret each 16 bytes a new
facilities header, hopefully soon bailing out on *p != 0x00.

In case of a long packet where every other byte is zero, the loop
will spam the kernel log with the printk ... which could probably
be classified as a security problem on its own. So how about the
following instead? I have no idea if this breaks some rose
specification, though. 


Signed-off-by: Jiri Bohac <jbohac@suse.cz>

diff --git a/net/rose/rose_subr.c b/net/rose/rose_subr.c
index f6c71ca..e687c7f 100644
--- a/net/rose/rose_subr.c
+++ b/net/rose/rose_subr.c
@@ -418,8 +418,7 @@ int rose_parse_facilities(unsigned char *p, unsigned packet_len,
 
 		default:
 			printk(KERN_DEBUG "ROSE: rose_parse_facilities - unknown facilities family %02X\n", *p);
-			len = 1;
-			break;
+			return 0;
 		}
 
 		if (len < 0)


-- 
Jiri Bohac <jbohac@suse.cz>
SUSE Labs, SUSE CZ

Re: [PATCH v2] ROSE: prevent heap corruption with bad facilities

[Jiri Bohac]

On Thu, Mar 31, 2011 at 08:02:25PM +0200, Jiri Bohac wrote:
> However, I wonder how much sense it makes to continue parsing the
> facilities if an unknown facility family appears. We don't know
> the length of its data, so we will interpret each 16 bytes a new

oops, typo:
s/16 bytes a new/16 bits as a new/

> facilities header, hopefully soon bailing out on *p != 0x00.
> 
> In case of a long packet where every other byte is zero, the loop
> will spam the kernel log with the printk ... which could probably
> be classified as a security problem on its own. So how about the
> following instead? I have no idea if this breaks some rose
> specification, though. 
-- 
Jiri Bohac <jbohac@suse.cz>
SUSE Labs, SUSE CZ

Re: [PATCH v2] ROSE: prevent heap corruption with bad facilities

[David Miller]

From: Jiri Bohac <jbohac@suse.cz>
Date: Thu, 31 Mar 2011 20:02:25 +0200

> So, to get the old behaviour back:

Jiri, please do not submit two patches in one email, it's beyond
confusing.

Instead, please submit a proper two-patch series.

Thanks.

Re: [PATCH v2] ROSE: prevent heap corruption with bad facilities

[Ben Hutchings]

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

On Thu, 2011-03-31 at 20:02 +0200, Jiri Bohac wrote:
[...]
> This last hunk does not look correct. In the default branch of
> the switch, you set len = 1, which means 
> 	p += 2; facilities_len -= 2.
> 
> The original code does 
> 	facilities_len--; p++;
> ... and it looks correct. So, to get the old behaviour back:
> 
> diff --git a/net/rose/rose_subr.c b/net/rose/rose_subr.c
> index f6c71ca..9777700 100644
> --- a/net/rose/rose_subr.c
> +++ b/net/rose/rose_subr.c
> @@ -418,7 +418,7 @@ int rose_parse_facilities(unsigned char *p, unsigned packet_len,
>  
>  		default:
>  			printk(KERN_DEBUG "ROSE: rose_parse_facilities - unknown facilities family %02X\n", *p);
> -			len = 1;
> +			len = 0;
>  			break;
>  		}

Yes, agreed.

> However, I wonder how much sense it makes to continue parsing the
> facilities if an unknown facility family appears. We don't know
> the length of its data, so we will interpret each 16 bytes a new
> facilities header, hopefully soon bailing out on *p != 0x00.
>
> In case of a long packet where every other byte is zero, the loop
> will spam the kernel log with the printk ... which could probably
> be classified as a security problem on its own. So how about the
> following instead? I have no idea if this breaks some rose
> specification, though. 
[...]

I don't know any more than you do; maybe Ralf knows or can find out.

Ben.

-- 
Ben Hutchings
Once a job is fouled up, anything done to improve it makes it worse.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH v2] ROSE: prevent heap corruption with bad facilities

[Jiri Bohac]

On Fri, Apr 01, 2011 at 09:41:48PM -0700, David Miller wrote:
> From: Jiri Bohac <jbohac@suse.cz>
> Date: Thu, 31 Mar 2011 20:02:25 +0200
> > So, to get the old behaviour back:
> 
> Jiri, please do not submit two patches in one email, it's beyond
> confusing.
> 
> Instead, please submit a proper two-patch series.

The two one line patches were two suggestions what to do. It's
either one or the other...

Sorry for the confusion.

-- 
Jiri Bohac <jbohac@suse.cz>
SUSE Labs, SUSE CZ