| Message ID | 1318910393.2571.47.camel@edumazet-laptop |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Eric Dumazet <eric.dumazet@gmail.com> Date: Tue, 18 Oct 2011 05:59:53 +0200 > [PATCH v2] pptp: pptp_rcv_core() misses pskb_may_pull() call > > e1000e uses paged frags, so any layer incorrectly pulling bytes from skb > can trigger a BUG in skb_pull() > > [951.142737] [<ffffffff813d2f36>] skb_pull+0x15/0x17 > [951.142737] [<ffffffffa0286824>] pptp_rcv_core+0x126/0x19a [pptp] > [951.152725] [<ffffffff813d17c4>] sk_receive_skb+0x69/0x105 > [951.163558] [<ffffffffa0286993>] pptp_rcv+0xc8/0xdc [pptp] > [951.165092] [<ffffffffa02800a3>] gre_rcv+0x62/0x75 [gre] > [951.165092] [<ffffffff81410784>] ip_local_deliver_finish+0x150/0x1c1 > [951.177599] [<ffffffff81410634>] ? ip_local_deliver_finish+0x0/0x1c1 > [951.177599] [<ffffffff81410846>] NF_HOOK.clone.7+0x51/0x58 > [951.177599] [<ffffffff81410996>] ip_local_deliver+0x51/0x55 > [951.177599] [<ffffffff814105b9>] ip_rcv_finish+0x31a/0x33e > [951.177599] [<ffffffff8141029f>] ? ip_rcv_finish+0x0/0x33e > [951.204898] [<ffffffff81410846>] NF_HOOK.clone.7+0x51/0x58 > [951.214651] [<ffffffff81410bb5>] ip_rcv+0x21b/0x246 > > pptp_rcv_core() is a nice example of a function assuming everything it > needs is available in skb head. > > Reported-by: Bradley Peterson <despite@gmail.com> > Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com> I assume by the driver paths in the patch that you think this is 'net-next' material and not suitable for plain 'net', right? -- 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
Le mercredi 19 octobre 2011 à 03:31 -0400, David Miller a écrit : > From: Eric Dumazet <eric.dumazet@gmail.com> > Date: Tue, 18 Oct 2011 05:59:53 +0200 > > > [PATCH v2] pptp: pptp_rcv_core() misses pskb_may_pull() call > > > > e1000e uses paged frags, so any layer incorrectly pulling bytes from skb > > can trigger a BUG in skb_pull() > > > > [951.142737] [<ffffffff813d2f36>] skb_pull+0x15/0x17 > > [951.142737] [<ffffffffa0286824>] pptp_rcv_core+0x126/0x19a [pptp] > > [951.152725] [<ffffffff813d17c4>] sk_receive_skb+0x69/0x105 > > [951.163558] [<ffffffffa0286993>] pptp_rcv+0xc8/0xdc [pptp] > > [951.165092] [<ffffffffa02800a3>] gre_rcv+0x62/0x75 [gre] > > [951.165092] [<ffffffff81410784>] ip_local_deliver_finish+0x150/0x1c1 > > [951.177599] [<ffffffff81410634>] ? ip_local_deliver_finish+0x0/0x1c1 > > [951.177599] [<ffffffff81410846>] NF_HOOK.clone.7+0x51/0x58 > > [951.177599] [<ffffffff81410996>] ip_local_deliver+0x51/0x55 > > [951.177599] [<ffffffff814105b9>] ip_rcv_finish+0x31a/0x33e > > [951.177599] [<ffffffff8141029f>] ? ip_rcv_finish+0x0/0x33e > > [951.204898] [<ffffffff81410846>] NF_HOOK.clone.7+0x51/0x58 > > [951.214651] [<ffffffff81410bb5>] ip_rcv+0x21b/0x246 > > > > pptp_rcv_core() is a nice example of a function assuming everything it > > needs is available in skb head. > > > > Reported-by: Bradley Peterson <despite@gmail.com> > > Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com> > > I assume by the driver paths in the patch that you think this > is 'net-next' material and not suitable for plain 'net', right? I incorrectly thought this driver was at the same location in net & net-next, and I my net-next tree was more convenient to compile this. I can respin patch on net tree if you prefer. Thanks -- 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: Eric Dumazet <eric.dumazet@gmail.com> Date: Wed, 19 Oct 2011 09:47:15 +0200 > I incorrectly thought this driver was at the same location in net & > net-next, and I my net-next tree was more convenient to compile this. > > I can respin patch on net tree if you prefer. No need, I took care of adjusting the paths. Thanks! -- 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 Mon, Oct 17, 2011 at 10:59 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote: > Le mardi 18 octobre 2011 à 05:51 +0200, Eric Dumazet a écrit : >> Le mardi 18 octobre 2011 à 04:24 +0200, Eric Dumazet a écrit : >> >> > diff --git a/drivers/net/ppp/pptp.c b/drivers/net/ppp/pptp.c >> > index eae542a..d0197e3 100644 >> > --- a/drivers/net/ppp/pptp.c >> > +++ b/drivers/net/ppp/pptp.c >> > @@ -305,11 +305,16 @@ static int pptp_rcv_core(struct sock *sk, struct sk_buff *skb) >> > } >> > >> > header = (struct pptp_gre_header *)(skb->data); >> > + headersize = sizeof(*header); >> > >> > /* test if acknowledgement present */ >> > if (PPTP_GRE_IS_A(header->ver)) { >> > - __u32 ack = (PPTP_GRE_IS_S(header->flags)) ? >> > - header->ack : header->seq; /* ack in different place if S = 0 */ >> > + __u32 ack; >> > + >> > + if (!pskb_may_pull(skb, headersize)) >> > + goto drop; >> >> Oh well, this is buggy, I need to set header again, I'll send an updated >> patch >> > > [PATCH v2] pptp: pptp_rcv_core() misses pskb_may_pull() call > > e1000e uses paged frags, so any layer incorrectly pulling bytes from skb > can trigger a BUG in skb_pull() > > [951.142737] [<ffffffff813d2f36>] skb_pull+0x15/0x17 > [951.142737] [<ffffffffa0286824>] pptp_rcv_core+0x126/0x19a [pptp] > [951.152725] [<ffffffff813d17c4>] sk_receive_skb+0x69/0x105 > [951.163558] [<ffffffffa0286993>] pptp_rcv+0xc8/0xdc [pptp] > [951.165092] [<ffffffffa02800a3>] gre_rcv+0x62/0x75 [gre] > [951.165092] [<ffffffff81410784>] ip_local_deliver_finish+0x150/0x1c1 > [951.177599] [<ffffffff81410634>] ? ip_local_deliver_finish+0x0/0x1c1 > [951.177599] [<ffffffff81410846>] NF_HOOK.clone.7+0x51/0x58 > [951.177599] [<ffffffff81410996>] ip_local_deliver+0x51/0x55 > [951.177599] [<ffffffff814105b9>] ip_rcv_finish+0x31a/0x33e > [951.177599] [<ffffffff8141029f>] ? ip_rcv_finish+0x0/0x33e > [951.204898] [<ffffffff81410846>] NF_HOOK.clone.7+0x51/0x58 > [951.214651] [<ffffffff81410bb5>] ip_rcv+0x21b/0x246 > > pptp_rcv_core() is a nice example of a function assuming everything it > needs is available in skb head. > > Reported-by: Bradley Peterson <despite@gmail.com> > Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com> > --- > drivers/net/ppp/pptp.c | 20 ++++++++++++-------- > 1 file changed, 12 insertions(+), 8 deletions(-) > > diff --git a/drivers/net/ppp/pptp.c b/drivers/net/ppp/pptp.c > index eae542a..29730fd 100644 > --- a/drivers/net/ppp/pptp.c > +++ b/drivers/net/ppp/pptp.c > @@ -305,11 +305,18 @@ static int pptp_rcv_core(struct sock *sk, struct sk_buff *skb) > } > > header = (struct pptp_gre_header *)(skb->data); > + headersize = sizeof(*header); > > /* test if acknowledgement present */ > if (PPTP_GRE_IS_A(header->ver)) { > - __u32 ack = (PPTP_GRE_IS_S(header->flags)) ? > - header->ack : header->seq; /* ack in different place if S = 0 */ > + __u32 ack; > + > + if (!pskb_may_pull(skb, headersize)) > + goto drop; > + header = (struct pptp_gre_header *)(skb->data); > + > + /* ack in different place if S = 0 */ > + ack = PPTP_GRE_IS_S(header->flags) ? header->ack : header->seq; > > ack = ntohl(ack); > > @@ -318,21 +325,18 @@ static int pptp_rcv_core(struct sock *sk, struct sk_buff *skb) > /* also handle sequence number wrap-around */ > if (WRAPPED(ack, opt->ack_recv)) > opt->ack_recv = ack; > + } else { > + headersize -= sizeof(header->ack); > } > - > /* test if payload present */ > if (!PPTP_GRE_IS_S(header->flags)) > goto drop; > > - headersize = sizeof(*header); > payload_len = ntohs(header->payload_len); > seq = ntohl(header->seq); > > - /* no ack present? */ > - if (!PPTP_GRE_IS_A(header->ver)) > - headersize -= sizeof(header->ack); > /* check for incomplete packet (length smaller than expected) */ > - if (skb->len - headersize < payload_len) > + if (!pskb_may_pull(skb, headersize + payload_len)) > goto drop; > > payload = skb->data + headersize; > > > This patch has been working for me. 5 days uptime, with no crashes. Thanks for your help! Bradley Peterson -- 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/drivers/net/ppp/pptp.c b/drivers/net/ppp/pptp.c index eae542a..29730fd 100644 --- a/drivers/net/ppp/pptp.c +++ b/drivers/net/ppp/pptp.c @@ -305,11 +305,18 @@ static int pptp_rcv_core(struct sock *sk, struct sk_buff *skb) } header = (struct pptp_gre_header *)(skb->data); + headersize = sizeof(*header); /* test if acknowledgement present */ if (PPTP_GRE_IS_A(header->ver)) { - __u32 ack = (PPTP_GRE_IS_S(header->flags)) ? - header->ack : header->seq; /* ack in different place if S = 0 */ + __u32 ack; + + if (!pskb_may_pull(skb, headersize)) + goto drop; + header = (struct pptp_gre_header *)(skb->data); + + /* ack in different place if S = 0 */ + ack = PPTP_GRE_IS_S(header->flags) ? header->ack : header->seq; ack = ntohl(ack); @@ -318,21 +325,18 @@ static int pptp_rcv_core(struct sock *sk, struct sk_buff *skb) /* also handle sequence number wrap-around */ if (WRAPPED(ack, opt->ack_recv)) opt->ack_recv = ack; + } else { + headersize -= sizeof(header->ack); } - /* test if payload present */ if (!PPTP_GRE_IS_S(header->flags)) goto drop; - headersize = sizeof(*header); payload_len = ntohs(header->payload_len); seq = ntohl(header->seq); - /* no ack present? */ - if (!PPTP_GRE_IS_A(header->ver)) - headersize -= sizeof(header->ack); /* check for incomplete packet (length smaller than expected) */ - if (skb->len - headersize < payload_len) + if (!pskb_may_pull(skb, headersize + payload_len)) goto drop; payload = skb->data + headersize;