netdev - Re: BUG in skb_pull with e1000e, PPTP, and L2TP

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:	Mon, 24 Oct 2011 16:59:14 -0500
From:	Bradley Peterson <despite@...il.com>
To:	Eric Dumazet <eric.dumazet@...il.com>
Cc:	netdev@...r.kernel.org, Jeff Kirsher <jeffrey.t.kirsher@...el.com>,
	Jesse Brandeburg <jesse.brandeburg@...el.com>,
	Bruce Allan <bruce.w.allan@...el.com>,
	Carolyn Wyborny <carolyn.wyborny@...el.com>,
	Don Skidmore <donald.c.skidmore@...el.com>,
	Greg Rose <gregory.v.rose@...el.com>,
	PJ Waskiewicz <peter.p.waskiewicz.jr@...el.com>,
	Alex Duyck <alexander.h.duyck@...el.com>,
	John Ronciak <john.ronciak@...el.com>,
	e1000-devel@...ts.sourceforge.net
Subject: Re: BUG in skb_pull with e1000e, PPTP, and L2TP

On Mon, Oct 17, 2011 at 10:59 PM, Eric Dumazet <eric.dumazet@...il.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@...il.com>
> Signed-off-by: Eric Dumazet <eric.dumazet@...il.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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html