| Message ID | 20110131130826.GC16804@shadowen.org |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
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 > > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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? > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. */
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(-)