LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* x25: possible skb leak on bad facilities
@ 2011-01-31 13:08 Andy Whitcroft
  2011-02-01 11:55 ` Andrew Hendry
  2011-02-07  9:25 ` John Hughes
  0 siblings, 2 replies; 7+ messages in thread
From: Andy Whitcroft @ 2011-01-31 13:08 UTC (permalink / raw)
  To: Andrew Hendry, David S. Miller
  Cc: John Hughes, linux-x25, netdev, linux-kernel, Tim Gardner

Looking at the changes introduced in the commit below, we seem to
introduce an skb leak when a packet with bad facilities are present:

    commit a6331d6f9a4298173b413cf99a40cc86a9d92c37
    Author: andrew hendry <andrew.hendry@gmail.com>
    Date:   Wed Nov 3 12:54:53 2010 +0000

        memory corruption in X.25 facilities parsing

If I am understanding things correctly then we trigger a -1 return to
the main packet dispatch loop, this being non-zero implies that we have
requeued the skb and it should not be freed.  As it was not requeued,
I believe the skb is no longer referenced and then is leaked.

Perhaps someone better aquainted with this code could review my analysis
in the patch leader below.  If accurate I believe we need the patch below
to resolve this.  If it is not then I suspect a comment is required on
the -1 return.

Thoughts?

-apw

>From 5728c05fb669e8ee1e6d20fb7a71916362039411 Mon Sep 17 00:00:00 2001
From: Andy Whitcroft <apw@canonical.com>
Date: Mon, 31 Jan 2011 10:37:36 +0000
Subject: [PATCH] x25: drop packet on invalid facility headers

The commit below introduced additional checks for invalid facilities,
and a new return path when these were detected:

    commit a6331d6f9a4298173b413cf99a40cc86a9d92c37
    Author: andrew hendry <andrew.hendry@gmail.com>
    Date:   Wed Nov 3 12:54:53 2010 +0000

	memory corruption in X.25 facilities parsing

This new return path short circuits packet handling, the new return -1
below:

    static int x25_state1_machine(struct sock *sk, struct sk_buff *skb,
							    int frametype)
    {
    [...]
                        len = x25_parse_facilities(skb, &x25->facilities,
                                                &x25->dte_facilities,
                                                &x25->vc_facil_mask);
                        if (len > 0)
                                skb_pull(skb, len);
                        else
                                return -1;
    [...]

This return code is passed back up the chain (via x25_process_rx_frame)
and is interpreted as below by the caller:

    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;
    }

Here we interpret the non-zero status as indicating the skb has been
requeued and should be preserved.  As we have not actually done so it
will be leaked.

Fix this up by indicating that the packet should be dropped.

Signed-off-by: Andy Whitcroft <apw@canonical.com>
---
 net/x25/x25_in.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/x25/x25_in.c b/net/x25/x25_in.c
index f729f02..213b93a 100644
--- a/net/x25/x25_in.c
+++ b/net/x25/x25_in.c
@@ -120,7 +120,7 @@ static int x25_state1_machine(struct sock *sk, struct sk_buff *skb, int frametyp
 			if (len > 0)
 				skb_pull(skb, len);
 			else
-				return -1;
+				return 0;
 			/*
 			 *	Copy any Call User Data.
 			 */
-- 
1.7.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: x25: possible skb leak on bad facilities
  2011-01-31 13:08 x25: possible skb leak on bad facilities Andy Whitcroft
@ 2011-02-01 11:55 ` Andrew Hendry
  2011-02-07  4:28   ` David Miller
  2011-02-07  9:25 ` John Hughes
  1 sibling, 1 reply; 7+ messages in thread
From: Andrew Hendry @ 2011-02-01 11:55 UTC (permalink / raw)
  To: Andy Whitcroft
  Cc: David S. Miller, John Hughes, linux-x25, netdev, linux-kernel,
	Tim Gardner

There are two callers, when I was crashing it I don't remember it
using the backlog path.
x25_process_rx_frame is called from both x25_backlog_rcv and also
x25_receive_data (via x25_lapb_receive_frame)

But reviewing that second path now it looks like it will also leak, -1
would make it skip the kfree_skb there as well.
So patch looks good to me, when I have some time I'll run it through
the environment I had setup originally to confirm.

On Tue, Feb 1, 2011 at 12:08 AM, Andy Whitcroft <apw@canonical.com> wrote:
> Looking at the changes introduced in the commit below, we seem to
> introduce an skb leak when a packet with bad facilities are present:
>
>    commit a6331d6f9a4298173b413cf99a40cc86a9d92c37
>    Author: andrew hendry <andrew.hendry@gmail.com>
>    Date:   Wed Nov 3 12:54:53 2010 +0000
>
>        memory corruption in X.25 facilities parsing
>
> If I am understanding things correctly then we trigger a -1 return to
> the main packet dispatch loop, this being non-zero implies that we have
> requeued the skb and it should not be freed.  As it was not requeued,
> I believe the skb is no longer referenced and then is leaked.
>
> Perhaps someone better aquainted with this code could review my analysis
> in the patch leader below.  If accurate I believe we need the patch below
> to resolve this.  If it is not then I suspect a comment is required on
> the -1 return.
>
> Thoughts?
>
> -apw
>
> From 5728c05fb669e8ee1e6d20fb7a71916362039411 Mon Sep 17 00:00:00 2001
> From: Andy Whitcroft <apw@canonical.com>
> Date: Mon, 31 Jan 2011 10:37:36 +0000
> Subject: [PATCH] x25: drop packet on invalid facility headers
>
> The commit below introduced additional checks for invalid facilities,
> and a new return path when these were detected:
>
>    commit a6331d6f9a4298173b413cf99a40cc86a9d92c37
>    Author: andrew hendry <andrew.hendry@gmail.com>
>    Date:   Wed Nov 3 12:54:53 2010 +0000
>
>        memory corruption in X.25 facilities parsing
>
> This new return path short circuits packet handling, the new return -1
> below:
>
>    static int x25_state1_machine(struct sock *sk, struct sk_buff *skb,
>                                                            int frametype)
>    {
>    [...]
>                        len = x25_parse_facilities(skb, &x25->facilities,
>                                                &x25->dte_facilities,
>                                                &x25->vc_facil_mask);
>                        if (len > 0)
>                                skb_pull(skb, len);
>                        else
>                                return -1;
>    [...]
>
> This return code is passed back up the chain (via x25_process_rx_frame)
> and is interpreted as below by the caller:
>
>    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;
>    }
>
> Here we interpret the non-zero status as indicating the skb has been
> requeued and should be preserved.  As we have not actually done so it
> will be leaked.
>
> Fix this up by indicating that the packet should be dropped.
>
> Signed-off-by: Andy Whitcroft <apw@canonical.com>
> ---
>  net/x25/x25_in.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/net/x25/x25_in.c b/net/x25/x25_in.c
> index f729f02..213b93a 100644
> --- a/net/x25/x25_in.c
> +++ b/net/x25/x25_in.c
> @@ -120,7 +120,7 @@ static int x25_state1_machine(struct sock *sk, struct sk_buff *skb, int frametyp
>                        if (len > 0)
>                                skb_pull(skb, len);
>                        else
> -                               return -1;
> +                               return 0;
>                        /*
>                         *      Copy any Call User Data.
>                         */
> --
> 1.7.1
>
>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: x25: possible skb leak on bad facilities
  2011-02-01 11:55 ` Andrew Hendry
@ 2011-02-07  4:28   ` David Miller
  2011-02-07  6:29     ` Andrew Hendry
  0 siblings, 1 reply; 7+ messages in thread
From: David Miller @ 2011-02-07  4:28 UTC (permalink / raw)
  To: andrew.hendry; +Cc: apw, john, linux-x25, netdev, linux-kernel, tim.gardner

From: Andrew Hendry <andrew.hendry@gmail.com>
Date: Tue, 1 Feb 2011 22:55:13 +1100

> There are two callers, when I was crashing it I don't remember it
> using the backlog path.
> x25_process_rx_frame is called from both x25_backlog_rcv and also
> x25_receive_data (via x25_lapb_receive_frame)
> 
> But reviewing that second path now it looks like it will also leak, -1
> would make it skip the kfree_skb there as well.
> So patch looks good to me, when I have some time I'll run it through
> the environment I had setup originally to confirm.

Andrew, have you had a chance to do this yet?

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: x25: possible skb leak on bad facilities
  2011-02-07  4:28   ` David Miller
@ 2011-02-07  6:29     ` Andrew Hendry
  2011-02-07 10:08       ` Andrew Hendry
  0 siblings, 1 reply; 7+ messages in thread
From: Andrew Hendry @ 2011-02-07  6:29 UTC (permalink / raw)
  To: David Miller; +Cc: apw, john, linux-x25, netdev, linux-kernel, tim.gardner

The issue is a bit more complex than Andy's patch, I think I have a full fix.
Burning it in on test system now, if thats OK ill post patch in a few hours.


On Mon, Feb 7, 2011 at 3:28 PM, David Miller <davem@davemloft.net> wrote:
> From: Andrew Hendry <andrew.hendry@gmail.com>
> Date: Tue, 1 Feb 2011 22:55:13 +1100
>
>> There are two callers, when I was crashing it I don't remember it
>> using the backlog path.
>> x25_process_rx_frame is called from both x25_backlog_rcv and also
>> x25_receive_data (via x25_lapb_receive_frame)
>>
>> But reviewing that second path now it looks like it will also leak, -1
>> would make it skip the kfree_skb there as well.
>> So patch looks good to me, when I have some time I'll run it through
>> the environment I had setup originally to confirm.
>
> Andrew, have you had a chance to do this yet?
>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: x25: possible skb leak on bad facilities
  2011-01-31 13:08 x25: possible skb leak on bad facilities Andy Whitcroft
  2011-02-01 11:55 ` Andrew Hendry
@ 2011-02-07  9:25 ` John Hughes
  1 sibling, 0 replies; 7+ messages in thread
From: John Hughes @ 2011-02-07  9:25 UTC (permalink / raw)
  To: Andy Whitcroft
  Cc: Andrew Hendry, David S. Miller, linux-x25, netdev, linux-kernel,
	Tim Gardner

On 31/01/11 14:08, Andy Whitcroft wrote:
> Looking at the changes introduced in the commit below, we seem to
> introduce an skb leak when a packet with bad facilities are present:
>
>      commit a6331d6f9a4298173b413cf99a40cc86a9d92c37
>      Author: andrew hendry<andrew.hendry@gmail.com>
>      Date:   Wed Nov 3 12:54:53 2010 +0000
>
>          memory corruption in X.25 facilities parsing
>
> If I am understanding things correctly then we trigger a -1 return to
> the main packet dispatch loop, this being non-zero implies that we have
> requeued the skb and it should not be freed.  As it was not requeued,
> I believe the skb is no longer referenced and then is leaked.
>
> Perhaps someone better aquainted with this code could review my analysis
> in the patch leader below.  If accurate I believe we need the patch below
> to resolve this.  If it is not then I suspect a comment is required on
> the -1 return.
>
> Thoughts?
>    
Sadly, after nearly 30 years (1982-2010) we've just closed our last X.25 
line so I can no longer test this.

Sorry.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: x25: possible skb leak on bad facilities
  2011-02-07  6:29     ` Andrew Hendry
@ 2011-02-07 10:08       ` Andrew Hendry
  2011-02-07 21:42         ` David Miller
  0 siblings, 1 reply; 7+ messages in thread
From: Andrew Hendry @ 2011-02-07 10:08 UTC (permalink / raw)
  To: David Miller; +Cc: apw, john, linux-x25, netdev, linux-kernel, tim.gardner


Originally x25_parse_facilities returned
-1 for an error
 0 meaning 0 length facilities
>0 the length of the facilities parsed.

5ef41308f94dc introduced more error checking in x25_parse_facilities
however used 0 to indicate bad parsing
a6331d6f9a429 followed this further for DTE facilities, again using 0 for bad parsing.

The meaning of 0 got confused in the callers.
If the facilities are messed up we can't determine where the data starts.
So patch makes all parsing errors return -1 and ensures callers close and don't use the skb further.

Reported-by: Andy Whitcroft <apw@canonical.com>
Signed-off-by: Andrew Hendry <andrew.hendry@gmail.com>


---
 net/x25/x25_facilities.c |   28 +++++++++++++++++++---------
 net/x25/x25_in.c         |   14 +++++++++++---
 2 files changed, 30 insertions(+), 12 deletions(-)

diff --git a/net/x25/x25_facilities.c b/net/x25/x25_facilities.c
index 55187c8..4062075 100644
--- a/net/x25/x25_facilities.c
+++ b/net/x25/x25_facilities.c
@@ -27,9 +27,19 @@
 #include <net/sock.h>
 #include <net/x25.h>
 
-/*
- * Parse a set of facilities into the facilities structures. Unrecognised
- *	facilities are written to the debug log file.
+/**
+ * x25_parse_facilities - Parse facilities from skb into the facilities structs
+ *
+ * @skb: sk_buff to parse
+ * @facilities: Regular facilites, updated as facilities are found
+ * @dte_facs: ITU DTE facilities, updated as DTE facilities are found
+ * @vc_fac_mask: mask is updated with all facilities found
+ *
+ * Return codes:
+ *  -1 - Parsing error, caller should drop call and clean up
+ *   0 - Parse OK, this skb has no facilities
+ *  >0 - Parse OK, returns the length of the facilities header
+ *
  */
 int x25_parse_facilities(struct sk_buff *skb, struct x25_facilities *facilities,
 		struct x25_dte_facilities *dte_facs, unsigned long *vc_fac_mask)
@@ -62,7 +72,7 @@ int x25_parse_facilities(struct sk_buff *skb, struct x25_facilities *facilities,
 		switch (*p & X25_FAC_CLASS_MASK) {
 		case X25_FAC_CLASS_A:
 			if (len < 2)
-				return 0;
+				return -1;
 			switch (*p) {
 			case X25_FAC_REVERSE:
 				if((p[1] & 0x81) == 0x81) {
@@ -107,7 +117,7 @@ int x25_parse_facilities(struct sk_buff *skb, struct x25_facilities *facilities,
 			break;
 		case X25_FAC_CLASS_B:
 			if (len < 3)
-				return 0;
+				return -1;
 			switch (*p) {
 			case X25_FAC_PACKET_SIZE:
 				facilities->pacsize_in  = p[1];
@@ -130,7 +140,7 @@ int x25_parse_facilities(struct sk_buff *skb, struct x25_facilities *facilities,
 			break;
 		case X25_FAC_CLASS_C:
 			if (len < 4)
-				return 0;
+				return -1;
 			printk(KERN_DEBUG "X.25: unknown facility %02X, "
 			       "values %02X, %02X, %02X\n",
 			       p[0], p[1], p[2], p[3]);
@@ -139,18 +149,18 @@ int x25_parse_facilities(struct sk_buff *skb, struct x25_facilities *facilities,
 			break;
 		case X25_FAC_CLASS_D:
 			if (len < p[1] + 2)
-				return 0;
+				return -1;
 			switch (*p) {
 			case X25_FAC_CALLING_AE:
 				if (p[1] > X25_MAX_DTE_FACIL_LEN || p[1] <= 1)
-					return 0;
+					return -1;
 				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 || p[1] <= 1)
-					return 0;
+					return -1;
 				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 f729f02..15de65f 100644
--- a/net/x25/x25_in.c
+++ b/net/x25/x25_in.c
@@ -91,10 +91,10 @@ static int x25_state1_machine(struct sock *sk, struct sk_buff *skb, int frametyp
 {
 	struct x25_address source_addr, dest_addr;
 	int len;
+	struct x25_sock *x25 = x25_sk(sk);
 
 	switch (frametype) {
 		case X25_CALL_ACCEPTED: {
-			struct x25_sock *x25 = x25_sk(sk);
 
 			x25_stop_timer(sk);
 			x25->condition = 0x00;
@@ -113,14 +113,16 @@ static int x25_state1_machine(struct sock *sk, struct sk_buff *skb, int frametyp
 						&dest_addr);
 			if (len > 0)
 				skb_pull(skb, len);
+			else if (len < 0)
+				goto out_clear;
 
 			len = x25_parse_facilities(skb, &x25->facilities,
 						&x25->dte_facilities,
 						&x25->vc_facil_mask);
 			if (len > 0)
 				skb_pull(skb, len);
-			else
-				return -1;
+			else if (len < 0)
+				goto out_clear;
 			/*
 			 *	Copy any Call User Data.
 			 */
@@ -144,6 +146,12 @@ static int x25_state1_machine(struct sock *sk, struct sk_buff *skb, int frametyp
 	}
 
 	return 0;
+
+out_clear:
+	x25_write_internal(sk, X25_CLEAR_REQUEST);
+	x25->state = X25_STATE_2;
+	x25_start_t23timer(sk);
+	return 0;
 }
 
 /*
-- 
1.7.1



On Mon, 2011-02-07 at 17:29 +1100, Andrew Hendry wrote:
> The issue is a bit more complex than Andy's patch, I think I have a full fix.
> Burning it in on test system now, if thats OK ill post patch in a few hours.
> 
> 
> On Mon, Feb 7, 2011 at 3:28 PM, David Miller <davem@davemloft.net> wrote:
> > From: Andrew Hendry <andrew.hendry@gmail.com>
> > Date: Tue, 1 Feb 2011 22:55:13 +1100
> >
> >> There are two callers, when I was crashing it I don't remember it
> >> using the backlog path.
> >> x25_process_rx_frame is called from both x25_backlog_rcv and also
> >> x25_receive_data (via x25_lapb_receive_frame)
> >>
> >> But reviewing that second path now it looks like it will also leak, -1
> >> would make it skip the kfree_skb there as well.
> >> So patch looks good to me, when I have some time I'll run it through
> >> the environment I had setup originally to confirm.
> >
> > Andrew, have you had a chance to do this yet?
> >



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: x25: possible skb leak on bad facilities
  2011-02-07 10:08       ` Andrew Hendry
@ 2011-02-07 21:42         ` David Miller
  0 siblings, 0 replies; 7+ messages in thread
From: David Miller @ 2011-02-07 21:42 UTC (permalink / raw)
  To: andrew.hendry; +Cc: apw, john, linux-x25, netdev, linux-kernel, tim.gardner

From: Andrew Hendry <andrew.hendry@gmail.com>
Date: Mon, 07 Feb 2011 21:08:15 +1100

> 
> Originally x25_parse_facilities returned
> -1 for an error
>  0 meaning 0 length facilities
>>0 the length of the facilities parsed.
> 
> 5ef41308f94dc introduced more error checking in x25_parse_facilities
> however used 0 to indicate bad parsing
> a6331d6f9a429 followed this further for DTE facilities, again using 0 for bad parsing.
> 
> The meaning of 0 got confused in the callers.
> If the facilities are messed up we can't determine where the data starts.
> So patch makes all parsing errors return -1 and ensures callers close and don't use the skb further.
> 
> Reported-by: Andy Whitcroft <apw@canonical.com>
> Signed-off-by: Andrew Hendry <andrew.hendry@gmail.com>

Please reference the commit header line text when referring to SHA1
IDs, because when backporting to other GIT trees the SHA1 IDs might be
different.

I took care of this when applying your patch, thanks.


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2011-02-07 21:42 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-01-31 13:08 x25: possible skb leak on bad facilities Andy Whitcroft
2011-02-01 11:55 ` Andrew Hendry
2011-02-07  4:28   ` David Miller
2011-02-07  6:29     ` Andrew Hendry
2011-02-07 10:08       ` Andrew Hendry
2011-02-07 21:42         ` David Miller
2011-02-07  9:25 ` John Hughes

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).