linux-kernel - Re: kernel BUG at net/core/skbuff.c:LINE! (2)

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20180119171900.GO1422@alphalink.fr>
Date:   Fri, 19 Jan 2018 18:19:00 +0100
From:   Guillaume Nault <g.nault@...halink.fr>
To:     Xin Long <lucien.xin@...il.com>
Cc:     syzbot 
        <syzbot+ed0838d0fa4c4f2b528e20286e6dc63effc7c14d@...kaller.appspotmail.com>,
        davem <davem@...emloft.net>,
        Eric Dumazet <eric.dumazet@...il.com>,
        kuznet <kuznet@....inr.ac.ru>,
        LKML <linux-kernel@...r.kernel.org>, linux-sctp@...r.kernel.org,
        network dev <netdev@...r.kernel.org>,
        Neil Horman <nhorman@...driver.com>,
        syzkaller-bugs@...glegroups.com,
        Vlad Yasevich <vyasevich@...il.com>,
        Américo Wang <xiyou.wangcong@...il.com>,
        yoshfuji <yoshfuji@...ux-ipv6.org>
Subject: Re: kernel BUG at net/core/skbuff.c:LINE! (2)

On Tue, Jan 16, 2018 at 04:21:40PM +0800, Xin Long wrote:
> ipv4 tunnels don't  really set dev->hard_header_len properly,
> we may should fix it in pppoe by using needed_headroom,
> as what it doesn't in arp_create.
> 
I'm a bit in doubt about which device needs to be fixed. Should ip_gre
set ->hard_header_len? Or should pppoe take ->needed_headroom into
account in skb_reserve()? I'd favor the later option too, but I haven't
figured out the semantic of these fields precisely enough to justify
this choice.

> @@ -860,16 +861,16 @@ static int pppoe_sendmsg(struct socket *sock,
> struct msghdr *m,
>         if (total_len > (dev->mtu + dev->hard_header_len))
>                 goto end;
> 
> +       rlen = LL_RESERVED_SPACE(dev) + dev->needed_tailroom;
> 
> -       skb = sock_wmalloc(sk, total_len + dev->hard_header_len + 32,
> -                          0, GFP_KERNEL);
> +       skb = sock_wmalloc(sk, total_len + rlen + 32, 0, GFP_KERNEL);
>         if (!skb) {
>                 error = -ENOMEM;
>                 goto end;
>         }
> 
>         /* Reserve space for headers. */
> -       skb_reserve(skb, dev->hard_header_len);
> +       skb_reserve(skb, rlen);
Any reason why you include dev->needed_tailroom in skb_reserve()?
BTW, we also have to fix __pppoe_xmit.

What about this patch?


---- >8 ----
diff --git a/drivers/net/ppp/pppoe.c b/drivers/net/ppp/pppoe.c
index 4e1da1645b15..42518af53332 100644
--- a/drivers/net/ppp/pppoe.c
+++ b/drivers/net/ppp/pppoe.c
@@ -842,6 +842,7 @@ static int pppoe_sendmsg(struct socket *sock, struct msghdr *m,
 	struct pppoe_hdr *ph;
 	struct net_device *dev;
 	char *start;
+	int hlen;
 
 	lock_sock(sk);
 	if (sock_flag(sk, SOCK_DEAD) || !(sk->sk_state & PPPOX_CONNECTED)) {
@@ -860,16 +861,16 @@ static int pppoe_sendmsg(struct socket *sock, struct msghdr *m,
 	if (total_len > (dev->mtu + dev->hard_header_len))
 		goto end;
 
-
-	skb = sock_wmalloc(sk, total_len + dev->hard_header_len + 32,
-			   0, GFP_KERNEL);
+	hlen = LL_RESERVED_SPACE(dev);
+	skb = sock_wmalloc(sk, hlen + sizeof(struct pppoe_hdr) + total_len +
+			   dev->needed_tailroom, 0, GFP_KERNEL);
 	if (!skb) {
 		error = -ENOMEM;
 		goto end;
 	}
 
 	/* Reserve space for headers. */
-	skb_reserve(skb, dev->hard_header_len);
+	skb_reserve(skb, hlen);
 	skb_reset_network_header(skb);
 
 	skb->dev = dev;
@@ -930,7 +931,7 @@ static int __pppoe_xmit(struct sock *sk, struct sk_buff *skb)
 	/* Copy the data if there is no space for the header or if it's
 	 * read-only.
 	 */
-	if (skb_cow_head(skb, sizeof(*ph) + dev->hard_header_len))
+	if (skb_cow_head(skb, LL_RESERVED_SPACE(dev) + sizeof(*ph)))
 		goto abort;
 
 	__skb_push(skb, sizeof(*ph));