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. */