[PATCH net] ipv4: ip_gre: Handle skb_pull() failure in ipgre_xmit()

[PATCH net] ipv4: ip_gre: Handle skb_pull() failure in ipgre_xmit()

[Shigeru Yoshida]

In ipgre_xmit(), skb_pull() may fail even if pskb_inet_may_pull() returns
true. For example, applications can create a malformed packet that causes
this problem with PF_PACKET.

This patch fixes the problem by dropping skb and returning from the
function if skb_pull() fails.

Fixes: c54419321455 ("GRE: Refactor GRE tunneling code.")
Signed-off-by: Shigeru Yoshida <syoshida@redhat.com>
---
 net/ipv4/ip_gre.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
index 22a26d1d29a0..95efa97cb84b 100644
--- a/net/ipv4/ip_gre.c
+++ b/net/ipv4/ip_gre.c
@@ -643,7 +643,8 @@ static netdev_tx_t ipgre_xmit(struct sk_buff *skb,
 		/* Pull skb since ip_tunnel_xmit() needs skb->data pointing
 		 * to gre header.
 		 */
-		skb_pull(skb, tunnel->hlen + sizeof(struct iphdr));
+		if (!skb_pull(skb, tunnel->hlen + sizeof(struct iphdr)))
+			goto free_skb;
 		skb_reset_mac_header(skb);
 
 		if (skb->ip_summed == CHECKSUM_PARTIAL &&
-- 
2.41.0

Re: [PATCH net] ipv4: ip_gre: Handle skb_pull() failure in ipgre_xmit()

[Willem de Bruijn]

Shigeru Yoshida wrote:
> In ipgre_xmit(), skb_pull() may fail even if pskb_inet_may_pull() returns
> true. For example, applications can create a malformed packet that causes
> this problem with PF_PACKET.

It may fail because because pskb_inet_may_pull does not account for
tunnel->hlen.

Is that what you are referring to with malformed packet? Can you
eloborate a bit on in which way the packet has to be malformed to
reach this?

FYI: I had a quick look at the IPv6 equivalent code.
ip6gre_tunnel_xmit is sufficiently different. It makes sense that this
is an IPv4 only patch.

> This patch fixes the problem by dropping skb and returning from the
> function if skb_pull() fails.
> 
> Fixes: c54419321455 ("GRE: Refactor GRE tunneling code.")
> Signed-off-by: Shigeru Yoshida <syoshida@redhat.com>
> ---
>  net/ipv4/ip_gre.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
> index 22a26d1d29a0..95efa97cb84b 100644
> --- a/net/ipv4/ip_gre.c
> +++ b/net/ipv4/ip_gre.c
> @@ -643,7 +643,8 @@ static netdev_tx_t ipgre_xmit(struct sk_buff *skb,
>  		/* Pull skb since ip_tunnel_xmit() needs skb->data pointing
>  		 * to gre header.
>  		 */
> -		skb_pull(skb, tunnel->hlen + sizeof(struct iphdr));
> +		if (!skb_pull(skb, tunnel->hlen + sizeof(struct iphdr)))
> +			goto free_skb;
>  		skb_reset_mac_header(skb);
>  
>  		if (skb->ip_summed == CHECKSUM_PARTIAL &&
> -- 
> 2.41.0
> 

Re: [PATCH net] ipv4: ip_gre: Handle skb_pull() failure in ipgre_xmit()

[Shigeru Yoshida]

On Mon, 27 Nov 2023 10:55:01 -0500, Willem de Bruijn wrote:
> Shigeru Yoshida wrote:
>> In ipgre_xmit(), skb_pull() may fail even if pskb_inet_may_pull() returns
>> true. For example, applications can create a malformed packet that causes
>> this problem with PF_PACKET.
> 
> It may fail because because pskb_inet_may_pull does not account for
> tunnel->hlen.
> 
> Is that what you are referring to with malformed packet? Can you
> eloborate a bit on in which way the packet has to be malformed to
> reach this?

Thank you very much for your prompt feedback.

Actually, I found this problem by running syzkaller. Syzkaller
reported the following uninit-value issue (I think the root cause of
this issue is the same as the one Eric mentioned):

=====================================================
BUG: KMSAN: uninit-value in __gre_xmit net/ipv4/ip_gre.c:469 [inline]
BUG: KMSAN: uninit-value in ipgre_xmit+0xdf4/0xe70 net/ipv4/ip_gre.c:662
 __gre_xmit net/ipv4/ip_gre.c:469 [inline]
 ipgre_xmit+0xdf4/0xe70 net/ipv4/ip_gre.c:662
 __netdev_start_xmit include/linux/netdevice.h:4918 [inline]
 netdev_start_xmit include/linux/netdevice.h:4932 [inline]
 xmit_one net/core/dev.c:3543 [inline]
 dev_hard_start_xmit+0x24a/0xa10 net/core/dev.c:3559
 __dev_queue_xmit+0x32f6/0x50e0 net/core/dev.c:4344
 dev_queue_xmit include/linux/netdevice.h:3112 [inline]
 packet_xmit+0x8f/0x6b0 net/packet/af_packet.c:276
 packet_snd net/packet/af_packet.c:3087 [inline]
 packet_sendmsg+0x8c24/0x9aa0 net/packet/af_packet.c:3119
 sock_sendmsg_nosec net/socket.c:730 [inline]
 __sock_sendmsg net/socket.c:745 [inline]
 __sys_sendto+0x717/0xa00 net/socket.c:2194
 __do_sys_sendto net/socket.c:2206 [inline]
 __se_sys_sendto net/socket.c:2202 [inline]
 __x64_sys_sendto+0x130/0x200 net/socket.c:2202
 do_syscall_x64 arch/x86/entry/common.c:51 [inline]
 do_syscall_64+0x44/0x110 arch/x86/entry/common.c:82
 entry_SYSCALL_64_after_hwframe+0x63/0x6b

Uninit was stored to memory at:
 __gre_xmit net/ipv4/ip_gre.c:469 [inline]
 ipgre_xmit+0xded/0xe70 net/ipv4/ip_gre.c:662
 __netdev_start_xmit include/linux/netdevice.h:4918 [inline]
 netdev_start_xmit include/linux/netdevice.h:4932 [inline]
 xmit_one net/core/dev.c:3543 [inline]
 dev_hard_start_xmit+0x24a/0xa10 net/core/dev.c:3559
 __dev_queue_xmit+0x32f6/0x50e0 net/core/dev.c:4344
 dev_queue_xmit include/linux/netdevice.h:3112 [inline]
 packet_xmit+0x8f/0x6b0 net/packet/af_packet.c:276
 packet_snd net/packet/af_packet.c:3087 [inline]
 packet_sendmsg+0x8c24/0x9aa0 net/packet/af_packet.c:3119
 sock_sendmsg_nosec net/socket.c:730 [inline]
 __sock_sendmsg net/socket.c:745 [inline]
 __sys_sendto+0x717/0xa00 net/socket.c:2194
 __do_sys_sendto net/socket.c:2206 [inline]
 __se_sys_sendto net/socket.c:2202 [inline]
 __x64_sys_sendto+0x130/0x200 net/socket.c:2202
 do_syscall_x64 arch/x86/entry/common.c:51 [inline]
 do_syscall_64+0x44/0x110 arch/x86/entry/common.c:82
 entry_SYSCALL_64_after_hwframe+0x63/0x6b

Uninit was created at:
 slab_post_alloc_hook+0x103/0x9e0 mm/slab.h:768
 slab_alloc_node mm/slub.c:3478 [inline]
 kmem_cache_alloc_node+0x5f7/0xb50 mm/slub.c:3523
 kmalloc_reserve+0x13c/0x4a0 net/core/skbuff.c:560
 pskb_expand_head+0x20b/0x19a0 net/core/skbuff.c:2098
 __skb_cow include/linux/skbuff.h:3586 [inline]
 skb_cow_head include/linux/skbuff.h:3620 [inline]
 ipgre_xmit+0x73c/0xe70 net/ipv4/ip_gre.c:638
 __netdev_start_xmit include/linux/netdevice.h:4918 [inline]
 netdev_start_xmit include/linux/netdevice.h:4932 [inline]
 xmit_one net/core/dev.c:3543 [inline]
 dev_hard_start_xmit+0x24a/0xa10 net/core/dev.c:3559
 __dev_queue_xmit+0x32f6/0x50e0 net/core/dev.c:4344
 dev_queue_xmit include/linux/netdevice.h:3112 [inline]
 packet_xmit+0x8f/0x6b0 net/packet/af_packet.c:276
 packet_snd net/packet/af_packet.c:3087 [inline]
 packet_sendmsg+0x8c24/0x9aa0 net/packet/af_packet.c:3119
 sock_sendmsg_nosec net/socket.c:730 [inline]
 __sock_sendmsg net/socket.c:745 [inline]
 __sys_sendto+0x717/0xa00 net/socket.c:2194
 __do_sys_sendto net/socket.c:2206 [inline]
 __se_sys_sendto net/socket.c:2202 [inline]
 __x64_sys_sendto+0x130/0x200 net/socket.c:2202
 do_syscall_x64 arch/x86/entry/common.c:51 [inline]
 do_syscall_64+0x44/0x110 arch/x86/entry/common.c:82
 entry_SYSCALL_64_after_hwframe+0x63/0x6b

CPU: 1 PID: 11318 Comm: syz-executor.7 Not tainted 6.6.0-14500-g1c41041124bd #10
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.2-1.fc38 04/01/2014
=====================================================

The simplified version of the repro is shown below:

#include <linux/if_ether.h>
#include <sys/ioctl.h>
#include <netinet/ether.h>
#include <net/if.h>
#include <sys/socket.h>
#include <netinet/in.h>
#include <string.h>
#include <unistd.h>
#include <stdio.h>
#include <stdlib.h>
#include <linux/if_packet.h>

int main(void)
{
	int s, s1, s2, data = 0;
	struct ifreq ifr;
	struct sockaddr_ll addr = { 0 };
	unsigned char mac_addr[] = {0x1, 0x2, 0x3, 0x4, 0x5, 0x6};

	s = socket(AF_PACKET, SOCK_DGRAM, 0x300);
	s1 = socket(AF_PACKET, SOCK_RAW, 0x300);
	s2 = socket(AF_NETLINK, SOCK_RAW, 0);

	strcpy(ifr.ifr_name, "gre0");
	ioctl(s2, SIOCGIFINDEX, &ifr);

	addr.sll_family = AF_PACKET;
	addr.sll_ifindex = ifr.ifr_ifindex;
	addr.sll_protocol = htons(0);
	addr.sll_hatype = ARPHRD_ETHER;
	addr.sll_pkttype = PACKET_HOST;
	addr.sll_halen = ETH_ALEN;
	memcpy(addr.sll_addr, mac_addr, ETH_ALEN);

	sendto(s1, &data, 1, 0, (struct sockaddr *)&addr, sizeof(addr));

	return 0;
}

The repro sends a 1-byte packet that doesn't have the correct IP
header. I meant this as "malformed pachet", but that might be a bit
confusing, sorry.

I think the cause of the uninit-value access is that ipgre_xmit()
reallocates the skb with skb_cow_head() and copies only the 1-byte
data, so any IP header access through `tnl_params` can cause the
problem.

At first I tried to modify pskb_inet_may_pull() to detect this type of
packet, but I ended up doing this patch.

Any advice is welcome!

Thanks,
Shigeru

> 
> FYI: I had a quick look at the IPv6 equivalent code.
> ip6gre_tunnel_xmit is sufficiently different. It makes sense that this
> is an IPv4 only patch.
> 
>> This patch fixes the problem by dropping skb and returning from the
>> function if skb_pull() fails.
>> 
>> Fixes: c54419321455 ("GRE: Refactor GRE tunneling code.")
>> Signed-off-by: Shigeru Yoshida <syoshida@redhat.com>
>> ---
>>  net/ipv4/ip_gre.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>> 
>> diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
>> index 22a26d1d29a0..95efa97cb84b 100644
>> --- a/net/ipv4/ip_gre.c
>> +++ b/net/ipv4/ip_gre.c
>> @@ -643,7 +643,8 @@ static netdev_tx_t ipgre_xmit(struct sk_buff *skb,
>>  		/* Pull skb since ip_tunnel_xmit() needs skb->data pointing
>>  		 * to gre header.
>>  		 */
>> -		skb_pull(skb, tunnel->hlen + sizeof(struct iphdr));
>> +		if (!skb_pull(skb, tunnel->hlen + sizeof(struct iphdr)))
>> +			goto free_skb;
>>  		skb_reset_mac_header(skb);
>>  
>>  		if (skb->ip_summed == CHECKSUM_PARTIAL &&
>> -- 
>> 2.41.0
>> 
> 
> 

Re: [PATCH net] ipv4: ip_gre: Handle skb_pull() failure in ipgre_xmit()

[Eric Dumazet]

On Wed, Nov 29, 2023 at 2:51 AM Shigeru Yoshida <syoshida@redhat.com> wrote:
>
> On Mon, 27 Nov 2023 10:55:01 -0500, Willem de Bruijn wrote:
> > Shigeru Yoshida wrote:
> >> In ipgre_xmit(), skb_pull() may fail even if pskb_inet_may_pull() returns
> >> true. For example, applications can create a malformed packet that causes
> >> this problem with PF_PACKET.
> >
> > It may fail because because pskb_inet_may_pull does not account for
> > tunnel->hlen.
> >
> > Is that what you are referring to with malformed packet? Can you
> > eloborate a bit on in which way the packet has to be malformed to
> > reach this?
>
> Thank you very much for your prompt feedback.
>
> Actually, I found this problem by running syzkaller. Syzkaller
> reported the following uninit-value issue (I think the root cause of
> this issue is the same as the one Eric mentioned):

Yes, I also have a similar syzbot report (but no repro yet) I am
releasing it right now.

>
> =====================================================
> BUG: KMSAN: uninit-value in __gre_xmit net/ipv4/ip_gre.c:469 [inline]
> BUG: KMSAN: uninit-value in ipgre_xmit+0xdf4/0xe70 net/ipv4/ip_gre.c:662
>  __gre_xmit net/ipv4/ip_gre.c:469 [inline]
>



> The simplified version of the repro is shown below:
>
> #include <linux/if_ether.h>
> #include <sys/ioctl.h>
> #include <netinet/ether.h>
> #include <net/if.h>
> #include <sys/socket.h>
> #include <netinet/in.h>
> #include <string.h>
> #include <unistd.h>
> #include <stdio.h>
> #include <stdlib.h>
> #include <linux/if_packet.h>
>
> int main(void)
> {
>         int s, s1, s2, data = 0;
>         struct ifreq ifr;
>         struct sockaddr_ll addr = { 0 };
>         unsigned char mac_addr[] = {0x1, 0x2, 0x3, 0x4, 0x5, 0x6};
>
>         s = socket(AF_PACKET, SOCK_DGRAM, 0x300);
>         s1 = socket(AF_PACKET, SOCK_RAW, 0x300);
>         s2 = socket(AF_NETLINK, SOCK_RAW, 0);
>
>         strcpy(ifr.ifr_name, "gre0");
>         ioctl(s2, SIOCGIFINDEX, &ifr);
>
>         addr.sll_family = AF_PACKET;
>         addr.sll_ifindex = ifr.ifr_ifindex;
>         addr.sll_protocol = htons(0);
>         addr.sll_hatype = ARPHRD_ETHER;
>         addr.sll_pkttype = PACKET_HOST;
>         addr.sll_halen = ETH_ALEN;
>         memcpy(addr.sll_addr, mac_addr, ETH_ALEN);
>
>         sendto(s1, &data, 1, 0, (struct sockaddr *)&addr, sizeof(addr));
>
>         return 0;
> }
>
> The repro sends a 1-byte packet that doesn't have the correct IP
> header. I meant this as "malformed pachet", but that might be a bit
> confusing, sorry.
>
> I think the cause of the uninit-value access is that ipgre_xmit()
> reallocates the skb with skb_cow_head() and copies only the 1-byte
> data, so any IP header access through `tnl_params` can cause the
> problem.
>
> At first I tried to modify pskb_inet_may_pull() to detect this type of
> packet, but I ended up doing this patch.

Even after your patch, __skb_pull() could call BUG() and crash.

I would suggest using this fix instead.

diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
index 22a26d1d29a09d234f18ce3b0f329e5047c0c046..5169c3c72cffe49cef613e69889d139db867ff74
100644
--- a/net/ipv4/ip_gre.c
+++ b/net/ipv4/ip_gre.c
@@ -635,15 +635,18 @@ static netdev_tx_t ipgre_xmit(struct sk_buff *skb,
        }

        if (dev->header_ops) {
+               int pull_len = tunnel->hlen + sizeof(struct iphdr);
+
                if (skb_cow_head(skb, 0))
                        goto free_skb;

                tnl_params = (const struct iphdr *)skb->data;

-               /* Pull skb since ip_tunnel_xmit() needs skb->data pointing
-                * to gre header.
-                */
-               skb_pull(skb, tunnel->hlen + sizeof(struct iphdr));
+               if (!pskb_network_may_pull(skb, pull_len))
+                       goto free_skb;
+
+               /* ip_tunnel_xmit() needs skb->data pointing to gre header. */
+               skb_pull(skb, pull_len);
                skb_reset_mac_header(skb);

                if (skb->ip_summed == CHECKSUM_PARTIAL &&

Re: [PATCH net] ipv4: ip_gre: Handle skb_pull() failure in ipgre_xmit()

[Shigeru Yoshida]

On Wed, 29 Nov 2023 10:56:47 +0100, Eric Dumazet wrote:
> On Wed, Nov 29, 2023 at 2:51 AM Shigeru Yoshida <syoshida@redhat.com> wrote:
>>
>> On Mon, 27 Nov 2023 10:55:01 -0500, Willem de Bruijn wrote:
>> > Shigeru Yoshida wrote:
>> >> In ipgre_xmit(), skb_pull() may fail even if pskb_inet_may_pull() returns
>> >> true. For example, applications can create a malformed packet that causes
>> >> this problem with PF_PACKET.
>> >
>> > It may fail because because pskb_inet_may_pull does not account for
>> > tunnel->hlen.
>> >
>> > Is that what you are referring to with malformed packet? Can you
>> > eloborate a bit on in which way the packet has to be malformed to
>> > reach this?
>>
>> Thank you very much for your prompt feedback.
>>
>> Actually, I found this problem by running syzkaller. Syzkaller
>> reported the following uninit-value issue (I think the root cause of
>> this issue is the same as the one Eric mentioned):
> 
> Yes, I also have a similar syzbot report (but no repro yet) I am
> releasing it right now.
> 
>>
>> =====================================================
>> BUG: KMSAN: uninit-value in __gre_xmit net/ipv4/ip_gre.c:469 [inline]
>> BUG: KMSAN: uninit-value in ipgre_xmit+0xdf4/0xe70 net/ipv4/ip_gre.c:662
>>  __gre_xmit net/ipv4/ip_gre.c:469 [inline]
>>
> 
> 
> 
>> The simplified version of the repro is shown below:
>>
>> #include <linux/if_ether.h>
>> #include <sys/ioctl.h>
>> #include <netinet/ether.h>
>> #include <net/if.h>
>> #include <sys/socket.h>
>> #include <netinet/in.h>
>> #include <string.h>
>> #include <unistd.h>
>> #include <stdio.h>
>> #include <stdlib.h>
>> #include <linux/if_packet.h>
>>
>> int main(void)
>> {
>>         int s, s1, s2, data = 0;
>>         struct ifreq ifr;
>>         struct sockaddr_ll addr = { 0 };
>>         unsigned char mac_addr[] = {0x1, 0x2, 0x3, 0x4, 0x5, 0x6};
>>
>>         s = socket(AF_PACKET, SOCK_DGRAM, 0x300);
>>         s1 = socket(AF_PACKET, SOCK_RAW, 0x300);
>>         s2 = socket(AF_NETLINK, SOCK_RAW, 0);
>>
>>         strcpy(ifr.ifr_name, "gre0");
>>         ioctl(s2, SIOCGIFINDEX, &ifr);
>>
>>         addr.sll_family = AF_PACKET;
>>         addr.sll_ifindex = ifr.ifr_ifindex;
>>         addr.sll_protocol = htons(0);
>>         addr.sll_hatype = ARPHRD_ETHER;
>>         addr.sll_pkttype = PACKET_HOST;
>>         addr.sll_halen = ETH_ALEN;
>>         memcpy(addr.sll_addr, mac_addr, ETH_ALEN);
>>
>>         sendto(s1, &data, 1, 0, (struct sockaddr *)&addr, sizeof(addr));
>>
>>         return 0;
>> }
>>
>> The repro sends a 1-byte packet that doesn't have the correct IP
>> header. I meant this as "malformed pachet", but that might be a bit
>> confusing, sorry.
>>
>> I think the cause of the uninit-value access is that ipgre_xmit()
>> reallocates the skb with skb_cow_head() and copies only the 1-byte
>> data, so any IP header access through `tnl_params` can cause the
>> problem.
>>
>> At first I tried to modify pskb_inet_may_pull() to detect this type of
>> packet, but I ended up doing this patch.
> 
> Even after your patch, __skb_pull() could call BUG() and crash.
> 
> I would suggest using this fix instead.

Thank you for your comment.

Your patch ensures that skb_pull() can pull the required size, so it
looks good to me. Also, I have tested your suggested patch with the
repro and confirmed that it fixes the issue.

Thanks,
Shigeru

> 
> diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
> index 22a26d1d29a09d234f18ce3b0f329e5047c0c046..5169c3c72cffe49cef613e69889d139db867ff74
> 100644
> --- a/net/ipv4/ip_gre.c
> +++ b/net/ipv4/ip_gre.c
> @@ -635,15 +635,18 @@ static netdev_tx_t ipgre_xmit(struct sk_buff *skb,
>         }
> 
>         if (dev->header_ops) {
> +               int pull_len = tunnel->hlen + sizeof(struct iphdr);
> +
>                 if (skb_cow_head(skb, 0))
>                         goto free_skb;
> 
>                 tnl_params = (const struct iphdr *)skb->data;
> 
> -               /* Pull skb since ip_tunnel_xmit() needs skb->data pointing
> -                * to gre header.
> -                */
> -               skb_pull(skb, tunnel->hlen + sizeof(struct iphdr));
> +               if (!pskb_network_may_pull(skb, pull_len))
> +                       goto free_skb;
> +
> +               /* ip_tunnel_xmit() needs skb->data pointing to gre header. */
> +               skb_pull(skb, pull_len);
>                 skb_reset_mac_header(skb);
> 
>                 if (skb->ip_summed == CHECKSUM_PARTIAL &&
> 

Re: [PATCH net] ipv4: ip_gre: Handle skb_pull() failure in ipgre_xmit()

[Eric Dumazet]

On Thu, Nov 30, 2023 at 3:03 PM Shigeru Yoshida <syoshida@redhat.com> wrote:
>
> On Wed, 29 Nov 2023 10:56:47 +0100, Eric Dumazet wrote:
> > On Wed, Nov 29, 2023 at 2:51 AM Shigeru Yoshida <syoshida@redhat.com> wrote:
> >>
> >> On Mon, 27 Nov 2023 10:55:01 -0500, Willem de Bruijn wrote:
> >> > Shigeru Yoshida wrote:
> >> >> In ipgre_xmit(), skb_pull() may fail even if pskb_inet_may_pull() returns
> >> >> true. For example, applications can create a malformed packet that causes
> >> >> this problem with PF_PACKET.
> >> >
> >> > It may fail because because pskb_inet_may_pull does not account for
> >> > tunnel->hlen.
> >> >
> >> > Is that what you are referring to with malformed packet? Can you
> >> > eloborate a bit on in which way the packet has to be malformed to
> >> > reach this?
> >>
> >> Thank you very much for your prompt feedback.
> >>
> >> Actually, I found this problem by running syzkaller. Syzkaller
> >> reported the following uninit-value issue (I think the root cause of
> >> this issue is the same as the one Eric mentioned):
> >
> > Yes, I also have a similar syzbot report (but no repro yet) I am
> > releasing it right now.
> >
> >>
> >> =====================================================
> >> BUG: KMSAN: uninit-value in __gre_xmit net/ipv4/ip_gre.c:469 [inline]
> >> BUG: KMSAN: uninit-value in ipgre_xmit+0xdf4/0xe70 net/ipv4/ip_gre.c:662
> >>  __gre_xmit net/ipv4/ip_gre.c:469 [inline]
> >>
> >
> >
> >
> >> The simplified version of the repro is shown below:
> >>
> >> #include <linux/if_ether.h>
> >> #include <sys/ioctl.h>
> >> #include <netinet/ether.h>
> >> #include <net/if.h>
> >> #include <sys/socket.h>
> >> #include <netinet/in.h>
> >> #include <string.h>
> >> #include <unistd.h>
> >> #include <stdio.h>
> >> #include <stdlib.h>
> >> #include <linux/if_packet.h>
> >>
> >> int main(void)
> >> {
> >>         int s, s1, s2, data = 0;
> >>         struct ifreq ifr;
> >>         struct sockaddr_ll addr = { 0 };
> >>         unsigned char mac_addr[] = {0x1, 0x2, 0x3, 0x4, 0x5, 0x6};
> >>
> >>         s = socket(AF_PACKET, SOCK_DGRAM, 0x300);
> >>         s1 = socket(AF_PACKET, SOCK_RAW, 0x300);
> >>         s2 = socket(AF_NETLINK, SOCK_RAW, 0);
> >>
> >>         strcpy(ifr.ifr_name, "gre0");
> >>         ioctl(s2, SIOCGIFINDEX, &ifr);
> >>
> >>         addr.sll_family = AF_PACKET;
> >>         addr.sll_ifindex = ifr.ifr_ifindex;
> >>         addr.sll_protocol = htons(0);
> >>         addr.sll_hatype = ARPHRD_ETHER;
> >>         addr.sll_pkttype = PACKET_HOST;
> >>         addr.sll_halen = ETH_ALEN;
> >>         memcpy(addr.sll_addr, mac_addr, ETH_ALEN);
> >>
> >>         sendto(s1, &data, 1, 0, (struct sockaddr *)&addr, sizeof(addr));
> >>
> >>         return 0;
> >> }
> >>
> >> The repro sends a 1-byte packet that doesn't have the correct IP
> >> header. I meant this as "malformed pachet", but that might be a bit
> >> confusing, sorry.
> >>
> >> I think the cause of the uninit-value access is that ipgre_xmit()
> >> reallocates the skb with skb_cow_head() and copies only the 1-byte
> >> data, so any IP header access through `tnl_params` can cause the
> >> problem.
> >>
> >> At first I tried to modify pskb_inet_may_pull() to detect this type of
> >> packet, but I ended up doing this patch.
> >
> > Even after your patch, __skb_pull() could call BUG() and crash.
> >
> > I would suggest using this fix instead.
>
> Thank you for your comment.
>
> Your patch ensures that skb_pull() can pull the required size, so it
> looks good to me. Also, I have tested your suggested patch with the
> repro and confirmed that it fixes the issue.
>

This is great, please cook/send a V2 with this updated patch.

I will add a 'Reviewed-by: Eric Dumazet <edumazet@google.com>' then.

Thanks.

Re: [PATCH net] ipv4: ip_gre: Handle skb_pull() failure in ipgre_xmit()

[Shigeru Yoshida]

On Thu, 30 Nov 2023 15:05:38 +0100, Eric Dumazet wrote:
> On Thu, Nov 30, 2023 at 3:03 PM Shigeru Yoshida <syoshida@redhat.com> wrote:
>>
>> On Wed, 29 Nov 2023 10:56:47 +0100, Eric Dumazet wrote:
>> > On Wed, Nov 29, 2023 at 2:51 AM Shigeru Yoshida <syoshida@redhat.com> wrote:
>> >>
>> >> On Mon, 27 Nov 2023 10:55:01 -0500, Willem de Bruijn wrote:
>> >> > Shigeru Yoshida wrote:
>> >> >> In ipgre_xmit(), skb_pull() may fail even if pskb_inet_may_pull() returns
>> >> >> true. For example, applications can create a malformed packet that causes
>> >> >> this problem with PF_PACKET.
>> >> >
>> >> > It may fail because because pskb_inet_may_pull does not account for
>> >> > tunnel->hlen.
>> >> >
>> >> > Is that what you are referring to with malformed packet? Can you
>> >> > eloborate a bit on in which way the packet has to be malformed to
>> >> > reach this?
>> >>
>> >> Thank you very much for your prompt feedback.
>> >>
>> >> Actually, I found this problem by running syzkaller. Syzkaller
>> >> reported the following uninit-value issue (I think the root cause of
>> >> this issue is the same as the one Eric mentioned):
>> >
>> > Yes, I also have a similar syzbot report (but no repro yet) I am
>> > releasing it right now.
>> >
>> >>
>> >> =====================================================
>> >> BUG: KMSAN: uninit-value in __gre_xmit net/ipv4/ip_gre.c:469 [inline]
>> >> BUG: KMSAN: uninit-value in ipgre_xmit+0xdf4/0xe70 net/ipv4/ip_gre.c:662
>> >>  __gre_xmit net/ipv4/ip_gre.c:469 [inline]
>> >>
>> >
>> >
>> >
>> >> The simplified version of the repro is shown below:
>> >>
>> >> #include <linux/if_ether.h>
>> >> #include <sys/ioctl.h>
>> >> #include <netinet/ether.h>
>> >> #include <net/if.h>
>> >> #include <sys/socket.h>
>> >> #include <netinet/in.h>
>> >> #include <string.h>
>> >> #include <unistd.h>
>> >> #include <stdio.h>
>> >> #include <stdlib.h>
>> >> #include <linux/if_packet.h>
>> >>
>> >> int main(void)
>> >> {
>> >>         int s, s1, s2, data = 0;
>> >>         struct ifreq ifr;
>> >>         struct sockaddr_ll addr = { 0 };
>> >>         unsigned char mac_addr[] = {0x1, 0x2, 0x3, 0x4, 0x5, 0x6};
>> >>
>> >>         s = socket(AF_PACKET, SOCK_DGRAM, 0x300);
>> >>         s1 = socket(AF_PACKET, SOCK_RAW, 0x300);
>> >>         s2 = socket(AF_NETLINK, SOCK_RAW, 0);
>> >>
>> >>         strcpy(ifr.ifr_name, "gre0");
>> >>         ioctl(s2, SIOCGIFINDEX, &ifr);
>> >>
>> >>         addr.sll_family = AF_PACKET;
>> >>         addr.sll_ifindex = ifr.ifr_ifindex;
>> >>         addr.sll_protocol = htons(0);
>> >>         addr.sll_hatype = ARPHRD_ETHER;
>> >>         addr.sll_pkttype = PACKET_HOST;
>> >>         addr.sll_halen = ETH_ALEN;
>> >>         memcpy(addr.sll_addr, mac_addr, ETH_ALEN);
>> >>
>> >>         sendto(s1, &data, 1, 0, (struct sockaddr *)&addr, sizeof(addr));
>> >>
>> >>         return 0;
>> >> }
>> >>
>> >> The repro sends a 1-byte packet that doesn't have the correct IP
>> >> header. I meant this as "malformed pachet", but that might be a bit
>> >> confusing, sorry.
>> >>
>> >> I think the cause of the uninit-value access is that ipgre_xmit()
>> >> reallocates the skb with skb_cow_head() and copies only the 1-byte
>> >> data, so any IP header access through `tnl_params` can cause the
>> >> problem.
>> >>
>> >> At first I tried to modify pskb_inet_may_pull() to detect this type of
>> >> packet, but I ended up doing this patch.
>> >
>> > Even after your patch, __skb_pull() could call BUG() and crash.
>> >
>> > I would suggest using this fix instead.
>>
>> Thank you for your comment.
>>
>> Your patch ensures that skb_pull() can pull the required size, so it
>> looks good to me. Also, I have tested your suggested patch with the
>> repro and confirmed that it fixes the issue.
>>
> 
> This is great, please cook/send a V2 with this updated patch.
> 
> I will add a 'Reviewed-by: Eric Dumazet <edumazet@google.com>' then.

Thanks, Eric! I'll send a v2 patch soon.

Shigeru

> 
> Thanks.
> 

Re: [PATCH net] ipv4: ip_gre: Handle skb_pull() failure in ipgre_xmit()

[Eric Dumazet]

On Sun, Nov 26, 2023 at 4:17 PM Shigeru Yoshida <syoshida@redhat.com> wrote:
>
> In ipgre_xmit(), skb_pull() may fail even if pskb_inet_may_pull() returns
> true. For example, applications can create a malformed packet that causes
> this problem with PF_PACKET.
>
> This patch fixes the problem by dropping skb and returning from the
> function if skb_pull() fails.
>
> Fixes: c54419321455 ("GRE: Refactor GRE tunneling code.")
> Signed-off-by: Shigeru Yoshida <syoshida@redhat.com>
> ---
>  net/ipv4/ip_gre.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
> index 22a26d1d29a0..95efa97cb84b 100644
> --- a/net/ipv4/ip_gre.c
> +++ b/net/ipv4/ip_gre.c
> @@ -643,7 +643,8 @@ static netdev_tx_t ipgre_xmit(struct sk_buff *skb,
>                 /* Pull skb since ip_tunnel_xmit() needs skb->data pointing
>                  * to gre header.
>                  */
> -               skb_pull(skb, tunnel->hlen + sizeof(struct iphdr));
> +               if (!skb_pull(skb, tunnel->hlen + sizeof(struct iphdr)))
> +                       goto free_skb;
>                 skb_reset_mac_header(skb);
>
>                 if (skb->ip_summed == CHECKSUM_PARTIAL &&
> --


I have syszbot reports with an actual repro for this one.

I do not think your patch is correct, something should be fixed
earlier (before we hit this point)

Re: [PATCH net] ipv4: ip_gre: Handle skb_pull() failure in ipgre_xmit()

[Paolo Abeni]

On Tue, 2023-11-28 at 16:45 +0100, Eric Dumazet wrote:
> On Sun, Nov 26, 2023 at 4:17 PM Shigeru Yoshida <syoshida@redhat.com> wrote:
> > 
> > In ipgre_xmit(), skb_pull() may fail even if pskb_inet_may_pull() returns
> > true. For example, applications can create a malformed packet that causes
> > this problem with PF_PACKET.
> > 
> > This patch fixes the problem by dropping skb and returning from the
> > function if skb_pull() fails.
> > 
> > Fixes: c54419321455 ("GRE: Refactor GRE tunneling code.")
> > Signed-off-by: Shigeru Yoshida <syoshida@redhat.com>
> > ---
> >  net/ipv4/ip_gre.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
> > index 22a26d1d29a0..95efa97cb84b 100644
> > --- a/net/ipv4/ip_gre.c
> > +++ b/net/ipv4/ip_gre.c
> > @@ -643,7 +643,8 @@ static netdev_tx_t ipgre_xmit(struct sk_buff *skb,
> >                 /* Pull skb since ip_tunnel_xmit() needs skb->data pointing
> >                  * to gre header.
> >                  */
> > -               skb_pull(skb, tunnel->hlen + sizeof(struct iphdr));
> > +               if (!skb_pull(skb, tunnel->hlen + sizeof(struct iphdr)))
> > +                       goto free_skb;
> >                 skb_reset_mac_header(skb);
> > 
> >                 if (skb->ip_summed == CHECKSUM_PARTIAL &&
> > --
> 
> 
> I have syszbot reports with an actual repro for this one.

Could you please share them? I could not find easily the reports in
https://syzkaller.appspot.com/upstream

Thanks,

Paolo

Re: [PATCH net] ipv4: ip_gre: Handle skb_pull() failure in ipgre_xmit()

[Eric Dumazet]

On Tue, Nov 28, 2023 at 4:51 PM Paolo Abeni <pabeni@redhat.com> wrote:
>
> On Tue, 2023-11-28 at 16:45 +0100, Eric Dumazet wrote:
> > On Sun, Nov 26, 2023 at 4:17 PM Shigeru Yoshida <syoshida@redhat.com> wrote:
> > >
> > > In ipgre_xmit(), skb_pull() may fail even if pskb_inet_may_pull() returns
> > > true. For example, applications can create a malformed packet that causes
> > > this problem with PF_PACKET.
> > >
> > > This patch fixes the problem by dropping skb and returning from the
> > > function if skb_pull() fails.
> > >
> > > Fixes: c54419321455 ("GRE: Refactor GRE tunneling code.")
> > > Signed-off-by: Shigeru Yoshida <syoshida@redhat.com>
> > > ---
> > >  net/ipv4/ip_gre.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
> > > index 22a26d1d29a0..95efa97cb84b 100644
> > > --- a/net/ipv4/ip_gre.c
> > > +++ b/net/ipv4/ip_gre.c
> > > @@ -643,7 +643,8 @@ static netdev_tx_t ipgre_xmit(struct sk_buff *skb,
> > >                 /* Pull skb since ip_tunnel_xmit() needs skb->data pointing
> > >                  * to gre header.
> > >                  */
> > > -               skb_pull(skb, tunnel->hlen + sizeof(struct iphdr));
> > > +               if (!skb_pull(skb, tunnel->hlen + sizeof(struct iphdr)))
> > > +                       goto free_skb;
> > >                 skb_reset_mac_header(skb);
> > >
> > >                 if (skb->ip_summed == CHECKSUM_PARTIAL &&
> > > --
> >
> >
> > I have syszbot reports with an actual repro for this one.
>
> Could you please share them? I could not find easily the reports in
> https://syzkaller.appspot.com/upstream

Stack trace looks like the following:

skbuff: skb_under_panic: text:ffffffff845f50a0 len:920 put:20
head:ffff888171931000 data:ffff888171930ff8 tail:0x390 end:0x680
dev:gre4
------------[ cut here ]------------
kernel BUG at net/core/skbuff.c:120 !
invalid opcode: 0000 [#1] PREEMPT SMP KASAN
CPU: 0 PID: 12705 Comm: kworker/0:0 Not tainted
6.1.43-syzkaller-00022-g8f46c3493178 #0
Hardware name: Google Google Compute Engine/Google Compute Engine,
BIOS Google 10/09/2023
Workqueue: mld mld_ifc_work
RIP: 0010:skb_panic net/core/skbuff.c:120 [inline]
RIP: 0010:skb_under_panic+0x14c/0x150 net/core/skbuff.c:130
Code: 60 98 da 85 48 c7 c6 05 6b 2f 86 48 8b 55 c0 8b 4d d4 44 8b 45
d0 4c 8b 4d c8 53 41 55 41 54 41 57 e8 fc db f4 00 48 83 c4 20 <0f> 0b
66 90 55 48 89 e5 41 57 41 56 41 55 41 54 53 48 83 ec 18 41
RSP: 0018:ffffc9000551f0a0 EFLAGS: 00010286
RAX: 0000000000000087 RBX: ffff888162226000 RCX: 98ecdd4da3f28000
RDX: 0000000000000000 RSI: 0000000000000400 RDI: 0000000000000000
RBP: ffffc9000551f0e0 R08: ffffffff815a9ea5 R09: fffff52000aa3dad
R10: 0000000000000000 R11: dffffc0000000001 R12: 0000000000000390
R13: 0000000000000680 R14: dffffc0000000000 R15: ffff888171930ff8
FS: 0000000000000000(0000) GS:ffff8881f7000000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000001b2f421000 CR3: 000000010f5a3000 CR4: 00000000003506b0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
<TASK>
skb_push+0xf3/0x120 net/core/skbuff.c:2181
iptunnel_xmit+0x2d0/0x940 net/ipv4/ip_tunnel_core.c:67
ip_tunnel_xmit+0x218f/0x2ae0 net/ipv4/ip_tunnel.c:813
__gre_xmit net/ipv4/ip_gre.c:469 [inline]
ipgre_xmit+0x7ac/0xaa0 net/ipv4/ip_gre.c:661
__netdev_start_xmit include/linux/netdevice.h:4908 [inline]
netdev_start_xmit include/linux/netdevice.h:4922 [inline]
xmit_one net/core/dev.c:3602 [inline]
dev_hard_start_xmit+0x1de/0x630 net/core/dev.c:3618
__dev_queue_xmit+0x18c0/0x3700 net/core/dev.c:4268
dev_queue_xmit include/linux/netdevice.h:3076 [inline]
neigh_direct_output+0x17/0x20 net/core/neighbour.c:1592
neigh_output include/net/neighbour.h:552 [inline]
ip6_finish_output2+0x104a/0x1820 net/ipv6/ip6_output.c:134
__ip6_finish_output net/ipv6/ip6_output.c:195 [inline]
ip6_finish_output+0x5d9/0xb60 net/ipv6/ip6_output.c:206
NF_HOOK_COND include/linux/netfilter.h:294 [inline]
ip6_output+0x1f7/0x4d0 net/ipv6/ip6_output.c:227
dst_output include/net/dst.h:444 [inline]
NF_HOOK include/linux/netfilter.h:305 [inline]
mld_sendpack+0x803/0xe40 net/ipv6/mcast.c:1820
mld_send_cr net/ipv6/mcast.c:2121 [inline]
mld_ifc_work+0x7dc/0xba0 net/ipv6/mcast.c:2653
process_one_work+0x73d/0xcb0 kernel/workqueue.c:2299
worker_thread+0xa60/0x1260 kernel/workqueue.c:2446
kthread+0x26d/0x300 kernel/kthread.c:376
ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:306
</TASK>
Modules linked in:
---[ end trace 0000000000000000 ]---
RIP: 0010:skb_panic net/core/skbuff.c:120 [inline]
RIP: 0010:skb_under_panic+0x14c/0x150 net/core/skbuff.c:130
Code: 60 98 da 85 48 c7 c6 05 6b 2f 86 48 8b 55 c0 8b 4d d4 44 8b 45
d0 4c 8b 4d c8 53 41 55 41 54 41 57 e8 fc db f4 00 48 83 c4 20 <0f> 0b
66 90 55 48 89 e5 41 57 41 56 41 55 41 54 53 48 83 ec 18 41
RSP: 0018:ffffc9000551f0a0 EFLAGS: 00010286
RAX: 0000000000000087 RBX: ffff888162226000 RCX: 98ecdd4da3f28000
RDX: 0000000000000000 RSI: 0000000000000400 RDI: 0000000000000000
RBP: ffffc9000551f0e0 R08: ffffffff815a9ea5 R09: fffff52000aa3dad
R10: 0000000000000000 R11: dffffc0000000001 R12: 0000000000000390
R13: 0000000000000680 R14: dffffc0000000000 R15: ffff888171930ff8
FS: 0000000000000000(0000) GS:ffff8881f7000000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000001b2f421000 CR3: 000000010f5a3000 CR4: 00000000003506b0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400

Re: [PATCH net] ipv4: ip_gre: Handle skb_pull() failure in ipgre_xmit()

[Eric Dumazet]

On Tue, Nov 28, 2023 at 5:05 PM Eric Dumazet <edumazet@google.com> wrote:
>
> On Tue, Nov 28, 2023 at 4:51 PM Paolo Abeni <pabeni@redhat.com> wrote:
> >
> > On Tue, 2023-11-28 at 16:45 +0100, Eric Dumazet wrote:
> > > On Sun, Nov 26, 2023 at 4:17 PM Shigeru Yoshida <syoshida@redhat.com> wrote:
> > > >
> > > > In ipgre_xmit(), skb_pull() may fail even if pskb_inet_may_pull() returns
> > > > true. For example, applications can create a malformed packet that causes
> > > > this problem with PF_PACKET.
> > > >
> > > > This patch fixes the problem by dropping skb and returning from the
> > > > function if skb_pull() fails.
> > > >
> > > > Fixes: c54419321455 ("GRE: Refactor GRE tunneling code.")
> > > > Signed-off-by: Shigeru Yoshida <syoshida@redhat.com>
> > > > ---
> > > >  net/ipv4/ip_gre.c | 3 ++-
> > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
> > > > index 22a26d1d29a0..95efa97cb84b 100644
> > > > --- a/net/ipv4/ip_gre.c
> > > > +++ b/net/ipv4/ip_gre.c
> > > > @@ -643,7 +643,8 @@ static netdev_tx_t ipgre_xmit(struct sk_buff *skb,
> > > >                 /* Pull skb since ip_tunnel_xmit() needs skb->data pointing
> > > >                  * to gre header.
> > > >                  */
> > > > -               skb_pull(skb, tunnel->hlen + sizeof(struct iphdr));
> > > > +               if (!skb_pull(skb, tunnel->hlen + sizeof(struct iphdr)))
> > > > +                       goto free_skb;
> > > >                 skb_reset_mac_header(skb);
> > > >
> > > >                 if (skb->ip_summed == CHECKSUM_PARTIAL &&
> > > > --
> > >
> > >
> > > I have syszbot reports with an actual repro for this one.
> >
> > Could you please share them? I could not find easily the reports in
> > https://syzkaller.appspot.com/upstream
>
> Stack trace looks like the following:
>
> skbuff: skb_under_panic: text:ffffffff845f50a0 len:920 put:20
> head:ffff888171931000 data:ffff888171930ff8 tail:0x390 end:0x680
> dev:gre4
> ------------[ cut here ]------------
> kernel BUG at net/core/skbuff.c:120 !
> invalid opcode: 0000 [#1] PREEMPT SMP KASAN
> CPU: 0 PID: 12705 Comm: kworker/0:0 Not tainted
> 6.1.43-syzkaller-00022-g8f46c3493178 #0
> Hardware name: Google Google Compute Engine/Google Compute Engine,
> BIOS Google 10/09/2023
> Workqueue: mld mld_ifc_work
> RIP: 0010:skb_panic net/core/skbuff.c:120 [inline]
> RIP: 0010:skb_under_panic+0x14c/0x150 net/core/skbuff.c:130
> Code: 60 98 da 85 48 c7 c6 05 6b 2f 86 48 8b 55 c0 8b 4d d4 44 8b 45
> d0 4c 8b 4d c8 53 41 55 41 54 41 57 e8 fc db f4 00 48 83 c4 20 <0f> 0b
> 66 90 55 48 89 e5 41 57 41 56 41 55 41 54 53 48 83 ec 18 41
> RSP: 0018:ffffc9000551f0a0 EFLAGS: 00010286
> RAX: 0000000000000087 RBX: ffff888162226000 RCX: 98ecdd4da3f28000
> RDX: 0000000000000000 RSI: 0000000000000400 RDI: 0000000000000000
> RBP: ffffc9000551f0e0 R08: ffffffff815a9ea5 R09: fffff52000aa3dad
> R10: 0000000000000000 R11: dffffc0000000001 R12: 0000000000000390
> R13: 0000000000000680 R14: dffffc0000000000 R15: ffff888171930ff8
> FS: 0000000000000000(0000) GS:ffff8881f7000000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 0000001b2f421000 CR3: 000000010f5a3000 CR4: 00000000003506b0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> Call Trace:
> <TASK>
> skb_push+0xf3/0x120 net/core/skbuff.c:2181
> iptunnel_xmit+0x2d0/0x940 net/ipv4/ip_tunnel_core.c:67
> ip_tunnel_xmit+0x218f/0x2ae0 net/ipv4/ip_tunnel.c:813
> __gre_xmit net/ipv4/ip_gre.c:469 [inline]
> ipgre_xmit+0x7ac/0xaa0 net/ipv4/ip_gre.c:661
> __netdev_start_xmit include/linux/netdevice.h:4908 [inline]
> netdev_start_xmit include/linux/netdevice.h:4922 [inline]
> xmit_one net/core/dev.c:3602 [inline]
> dev_hard_start_xmit+0x1de/0x630 net/core/dev.c:3618
> __dev_queue_xmit+0x18c0/0x3700 net/core/dev.c:4268
> dev_queue_xmit include/linux/netdevice.h:3076 [inline]
> neigh_direct_output+0x17/0x20 net/core/neighbour.c:1592
> neigh_output include/net/neighbour.h:552 [inline]
> ip6_finish_output2+0x104a/0x1820 net/ipv6/ip6_output.c:134
> __ip6_finish_output net/ipv6/ip6_output.c:195 [inline]
> ip6_finish_output+0x5d9/0xb60 net/ipv6/ip6_output.c:206
> NF_HOOK_COND include/linux/netfilter.h:294 [inline]
> ip6_output+0x1f7/0x4d0 net/ipv6/ip6_output.c:227
> dst_output include/net/dst.h:444 [inline]
> NF_HOOK include/linux/netfilter.h:305 [inline]
> mld_sendpack+0x803/0xe40 net/ipv6/mcast.c:1820
> mld_send_cr net/ipv6/mcast.c:2121 [inline]
> mld_ifc_work+0x7dc/0xba0 net/ipv6/mcast.c:2653
> process_one_work+0x73d/0xcb0 kernel/workqueue.c:2299
> worker_thread+0xa60/0x1260 kernel/workqueue.c:2446
> kthread+0x26d/0x300 kernel/kthread.c:376
> ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:306
> </TASK>
> Modules linked in:
> ---[ end trace 0000000000000000 ]---
> RIP: 0010:skb_panic net/core/skbuff.c:120 [inline]
> RIP: 0010:skb_under_panic+0x14c/0x150 net/core/skbuff.c:130
> Code: 60 98 da 85 48 c7 c6 05 6b 2f 86 48 8b 55 c0 8b 4d d4 44 8b 45
> d0 4c 8b 4d c8 53 41 55 41 54 41 57 e8 fc db f4 00 48 83 c4 20 <0f> 0b
> 66 90 55 48 89 e5 41 57 41 56 41 55 41 54 53 48 83 ec 18 41
> RSP: 0018:ffffc9000551f0a0 EFLAGS: 00010286
> RAX: 0000000000000087 RBX: ffff888162226000 RCX: 98ecdd4da3f28000
> RDX: 0000000000000000 RSI: 0000000000000400 RDI: 0000000000000000
> RBP: ffffc9000551f0e0 R08: ffffffff815a9ea5 R09: fffff52000aa3dad
> R10: 0000000000000000 R11: dffffc0000000001 R12: 0000000000000390
> R13: 0000000000000680 R14: dffffc0000000000 R15: ffff888171930ff8
> FS: 0000000000000000(0000) GS:ffff8881f7000000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 0000001b2f421000 CR3: 000000010f5a3000 CR4: 00000000003506b0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400

It looks like the repro I had was fixed by "#syz fix: bonding: stop
the device in bond_setup_by_slave()"

(I am not sure, I have to double check)

# See https://goo.gl/kgGztJ for information about syzkaller reproducers.
#{"repeat":true,"procs":1,"slowdown":1,"sandbox":"","sandbox_arg":0,"close_fds":false}
socket$inet(0x2, 0x2, 0x0)
r0 = socket(0x200000000000011, 0x4000000000080002, 0x0)
r1 = socket$netlink(0x10, 0x3, 0x0)
r2 = socket$netlink(0x10, 0x3, 0x0)
r3 = socket(0x10, 0x803, 0x0)
sendmsg$NL80211_CMD_CRIT_PROTOCOL_START(r3, &(0x7f0000000580)={0x0,
0x0, &(0x7f0000000540)={0x0, 0x1c}}, 0x0)
getsockname$packet(r3, &(0x7f0000000600)={0x11, 0x0, <r4=>0x0, 0x1,
0x0, 0x6, @broadcast}, &(0x7f0000000080)=0x14)
sendmsg$nl_route(r2, &(0x7f0000000040)={0x0, 0x0,
&(0x7f0000000000)={&(0x7f0000000340)=ANY=[@ANYBLOB="3c0000001000850600002000fe612233ca000800",
@ANYRES32=r4, @ANYBLOB="2377f29e252155b21c0012000c000100626f6e64000000000c000200080001000134e7307075a7cc6d2dba6e4dce25f18968dd3d6f77199cd06d7a4cfcdc99dcfd5ec3f3e3d98be8a8bac2dcc414b58dda48b3ea35411d5b112c26f31b352982f55be446b3dd47e435954252213828ba98a1bc363278f8bd13ad746bb8edad619162f5d1892e9fa42e4fe2b60f5fe2bb963f08d6696820ade9cff2b2deb91ce5657168a90dc5230e33b8c26cd925c31366a2ae339f12ba8966be1439cec635b08c0a97490b133a5b7360b59347833fc95a7bf3dc9bc64741de1a6e83c9bdfdfd0baabec981099bb3dbd64a7e7979cfb7935affbcda49190b7ec9bc1e89d6ccedec20f91b571e6fc049ba82821b26ca4f85f4b03f70b176b43de915bec76e405bce49a4b46ec745b51f36282916b77d7f913a6afd6813df2c"],
0x3c}}, 0x0)
sendmsg$nl_route(r1, &(0x7f0000000240)={0x0, 0x0,
&(0x7f0000000180)={&(0x7f0000000780)=@newlink={0x58, 0x10, 0xffffff1f,
0x0, 0x0, {0x0, 0x0, 0x0, 0x0, 0x800}, [@IFLA_LINKINFO={0x28, 0x12,
0x0, 0x1, @gre={{0x8}, {0x1c, 0x2, 0x0, 0x1, [@IFLA_GRE_LOCAL={0x8,
0x6, @broadcast}, @IFLA_GRE_TOS={0x5, 0x9, 0x8}, @IFLA_GRE_OKEY={0x8,
0x5, 0x8}]}}}, @IFLA_MASTER={0x8, 0xa, r4}, @IFLA_GROUP={0x8, 0x1b,
0x8000}]}, 0x58}}, 0x4004000)
bind$packet(r0, &(0x7f00000000c0)={0x11, 0x0, r4, 0x1, 0x0, 0x6, @remote}, 0x14)
sendmsg$nl_route(r0, &(0x7f0000000300)={0x0, 0x0, &(0x7f00000002c0)={0x0}}, 0x0)

Re: [PATCH net] ipv4: ip_gre: Handle skb_pull() failure in ipgre_xmit()

[Eric Dumazet]

On Tue, Nov 28, 2023 at 5:13 PM Eric Dumazet <edumazet@google.com> wrote:
>
> On Tue, Nov 28, 2023 at 5:05 PM Eric Dumazet <edumazet@google.com> wrote:
> >
> > On Tue, Nov 28, 2023 at 4:51 PM Paolo Abeni <pabeni@redhat.com> wrote:
> > >
> > > On Tue, 2023-11-28 at 16:45 +0100, Eric Dumazet wrote:
> > > > On Sun, Nov 26, 2023 at 4:17 PM Shigeru Yoshida <syoshida@redhat.com> wrote:
> > > > >
> > > > > In ipgre_xmit(), skb_pull() may fail even if pskb_inet_may_pull() returns
> > > > > true. For example, applications can create a malformed packet that causes
> > > > > this problem with PF_PACKET.
> > > > >
> > > > > This patch fixes the problem by dropping skb and returning from the
> > > > > function if skb_pull() fails.
> > > > >
> > > > > Fixes: c54419321455 ("GRE: Refactor GRE tunneling code.")
> > > > > Signed-off-by: Shigeru Yoshida <syoshida@redhat.com>
> > > > > ---
> > > > >  net/ipv4/ip_gre.c | 3 ++-
> > > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
> > > > > index 22a26d1d29a0..95efa97cb84b 100644
> > > > > --- a/net/ipv4/ip_gre.c
> > > > > +++ b/net/ipv4/ip_gre.c
> > > > > @@ -643,7 +643,8 @@ static netdev_tx_t ipgre_xmit(struct sk_buff *skb,
> > > > >                 /* Pull skb since ip_tunnel_xmit() needs skb->data pointing
> > > > >                  * to gre header.
> > > > >                  */
> > > > > -               skb_pull(skb, tunnel->hlen + sizeof(struct iphdr));
> > > > > +               if (!skb_pull(skb, tunnel->hlen + sizeof(struct iphdr)))
> > > > > +                       goto free_skb;
> > > > >                 skb_reset_mac_header(skb);
> > > > >
> > > > >                 if (skb->ip_summed == CHECKSUM_PARTIAL &&
> > > > > --
> > > >
> > > >
> > > > I have syszbot reports with an actual repro for this one.
> > >
> > > Could you please share them? I could not find easily the reports in
> > > https://syzkaller.appspot.com/upstream
> >
> > Stack trace looks like the following:
> >
> > skbuff: skb_under_panic: text:ffffffff845f50a0 len:920 put:20
> > head:ffff888171931000 data:ffff888171930ff8 tail:0x390 end:0x680
> > dev:gre4
> > ------------[ cut here ]------------
> > kernel BUG at net/core/skbuff.c:120 !
> > invalid opcode: 0000 [#1] PREEMPT SMP KASAN
> > CPU: 0 PID: 12705 Comm: kworker/0:0 Not tainted
> > 6.1.43-syzkaller-00022-g8f46c3493178 #0
> > Hardware name: Google Google Compute Engine/Google Compute Engine,
> > BIOS Google 10/09/2023
> > Workqueue: mld mld_ifc_work
> > RIP: 0010:skb_panic net/core/skbuff.c:120 [inline]
> > RIP: 0010:skb_under_panic+0x14c/0x150 net/core/skbuff.c:130
> > Code: 60 98 da 85 48 c7 c6 05 6b 2f 86 48 8b 55 c0 8b 4d d4 44 8b 45
> > d0 4c 8b 4d c8 53 41 55 41 54 41 57 e8 fc db f4 00 48 83 c4 20 <0f> 0b
> > 66 90 55 48 89 e5 41 57 41 56 41 55 41 54 53 48 83 ec 18 41
> > RSP: 0018:ffffc9000551f0a0 EFLAGS: 00010286
> > RAX: 0000000000000087 RBX: ffff888162226000 RCX: 98ecdd4da3f28000
> > RDX: 0000000000000000 RSI: 0000000000000400 RDI: 0000000000000000
> > RBP: ffffc9000551f0e0 R08: ffffffff815a9ea5 R09: fffff52000aa3dad
> > R10: 0000000000000000 R11: dffffc0000000001 R12: 0000000000000390
> > R13: 0000000000000680 R14: dffffc0000000000 R15: ffff888171930ff8
> > FS: 0000000000000000(0000) GS:ffff8881f7000000(0000) knlGS:0000000000000000
> > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > CR2: 0000001b2f421000 CR3: 000000010f5a3000 CR4: 00000000003506b0
> > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> > Call Trace:
> > <TASK>
> > skb_push+0xf3/0x120 net/core/skbuff.c:2181
> > iptunnel_xmit+0x2d0/0x940 net/ipv4/ip_tunnel_core.c:67
> > ip_tunnel_xmit+0x218f/0x2ae0 net/ipv4/ip_tunnel.c:813
> > __gre_xmit net/ipv4/ip_gre.c:469 [inline]
> > ipgre_xmit+0x7ac/0xaa0 net/ipv4/ip_gre.c:661
> > __netdev_start_xmit include/linux/netdevice.h:4908 [inline]
> > netdev_start_xmit include/linux/netdevice.h:4922 [inline]
> > xmit_one net/core/dev.c:3602 [inline]
> > dev_hard_start_xmit+0x1de/0x630 net/core/dev.c:3618
> > __dev_queue_xmit+0x18c0/0x3700 net/core/dev.c:4268
> > dev_queue_xmit include/linux/netdevice.h:3076 [inline]
> > neigh_direct_output+0x17/0x20 net/core/neighbour.c:1592
> > neigh_output include/net/neighbour.h:552 [inline]
> > ip6_finish_output2+0x104a/0x1820 net/ipv6/ip6_output.c:134
> > __ip6_finish_output net/ipv6/ip6_output.c:195 [inline]
> > ip6_finish_output+0x5d9/0xb60 net/ipv6/ip6_output.c:206
> > NF_HOOK_COND include/linux/netfilter.h:294 [inline]
> > ip6_output+0x1f7/0x4d0 net/ipv6/ip6_output.c:227
> > dst_output include/net/dst.h:444 [inline]
> > NF_HOOK include/linux/netfilter.h:305 [inline]
> > mld_sendpack+0x803/0xe40 net/ipv6/mcast.c:1820
> > mld_send_cr net/ipv6/mcast.c:2121 [inline]
> > mld_ifc_work+0x7dc/0xba0 net/ipv6/mcast.c:2653
> > process_one_work+0x73d/0xcb0 kernel/workqueue.c:2299
> > worker_thread+0xa60/0x1260 kernel/workqueue.c:2446
> > kthread+0x26d/0x300 kernel/kthread.c:376
> > ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:306
> > </TASK>
> > Modules linked in:
> > ---[ end trace 0000000000000000 ]---
> > RIP: 0010:skb_panic net/core/skbuff.c:120 [inline]
> > RIP: 0010:skb_under_panic+0x14c/0x150 net/core/skbuff.c:130
> > Code: 60 98 da 85 48 c7 c6 05 6b 2f 86 48 8b 55 c0 8b 4d d4 44 8b 45
> > d0 4c 8b 4d c8 53 41 55 41 54 41 57 e8 fc db f4 00 48 83 c4 20 <0f> 0b
> > 66 90 55 48 89 e5 41 57 41 56 41 55 41 54 53 48 83 ec 18 41
> > RSP: 0018:ffffc9000551f0a0 EFLAGS: 00010286
> > RAX: 0000000000000087 RBX: ffff888162226000 RCX: 98ecdd4da3f28000
> > RDX: 0000000000000000 RSI: 0000000000000400 RDI: 0000000000000000
> > RBP: ffffc9000551f0e0 R08: ffffffff815a9ea5 R09: fffff52000aa3dad
> > R10: 0000000000000000 R11: dffffc0000000001 R12: 0000000000000390
> > R13: 0000000000000680 R14: dffffc0000000000 R15: ffff888171930ff8
> > FS: 0000000000000000(0000) GS:ffff8881f7000000(0000) knlGS:0000000000000000
> > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > CR2: 0000001b2f421000 CR3: 000000010f5a3000 CR4: 00000000003506b0
> > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
>
> It looks like the repro I had was fixed by "#syz fix: bonding: stop
> the device in bond_setup_by_slave()"
>
> (I am not sure, I have to double check)

This is the syzbot link :
https://syzkaller.appspot.com/bug?extid=802070ddd12342f07fce


>
> # See https://goo.gl/kgGztJ for information about syzkaller reproducers.
> #{"repeat":true,"procs":1,"slowdown":1,"sandbox":"","sandbox_arg":0,"close_fds":false}
> socket$inet(0x2, 0x2, 0x0)
> r0 = socket(0x200000000000011, 0x4000000000080002, 0x0)
> r1 = socket$netlink(0x10, 0x3, 0x0)
> r2 = socket$netlink(0x10, 0x3, 0x0)
> r3 = socket(0x10, 0x803, 0x0)
> sendmsg$NL80211_CMD_CRIT_PROTOCOL_START(r3, &(0x7f0000000580)={0x0,
> 0x0, &(0x7f0000000540)={0x0, 0x1c}}, 0x0)
> getsockname$packet(r3, &(0x7f0000000600)={0x11, 0x0, <r4=>0x0, 0x1,
> 0x0, 0x6, @broadcast}, &(0x7f0000000080)=0x14)
> sendmsg$nl_route(r2, &(0x7f0000000040)={0x0, 0x0,
> &(0x7f0000000000)={&(0x7f0000000340)=ANY=[@ANYBLOB="3c0000001000850600002000fe612233ca000800",
> @ANYRES32=r4, @ANYBLOB="2377f29e252155b21c0012000c000100626f6e64000000000c000200080001000134e7307075a7cc6d2dba6e4dce25f18968dd3d6f77199cd06d7a4cfcdc99dcfd5ec3f3e3d98be8a8bac2dcc414b58dda48b3ea35411d5b112c26f31b352982f55be446b3dd47e435954252213828ba98a1bc363278f8bd13ad746bb8edad619162f5d1892e9fa42e4fe2b60f5fe2bb963f08d6696820ade9cff2b2deb91ce5657168a90dc5230e33b8c26cd925c31366a2ae339f12ba8966be1439cec635b08c0a97490b133a5b7360b59347833fc95a7bf3dc9bc64741de1a6e83c9bdfdfd0baabec981099bb3dbd64a7e7979cfb7935affbcda49190b7ec9bc1e89d6ccedec20f91b571e6fc049ba82821b26ca4f85f4b03f70b176b43de915bec76e405bce49a4b46ec745b51f36282916b77d7f913a6afd6813df2c"],
> 0x3c}}, 0x0)
> sendmsg$nl_route(r1, &(0x7f0000000240)={0x0, 0x0,
> &(0x7f0000000180)={&(0x7f0000000780)=@newlink={0x58, 0x10, 0xffffff1f,
> 0x0, 0x0, {0x0, 0x0, 0x0, 0x0, 0x800}, [@IFLA_LINKINFO={0x28, 0x12,
> 0x0, 0x1, @gre={{0x8}, {0x1c, 0x2, 0x0, 0x1, [@IFLA_GRE_LOCAL={0x8,
> 0x6, @broadcast}, @IFLA_GRE_TOS={0x5, 0x9, 0x8}, @IFLA_GRE_OKEY={0x8,
> 0x5, 0x8}]}}}, @IFLA_MASTER={0x8, 0xa, r4}, @IFLA_GROUP={0x8, 0x1b,
> 0x8000}]}, 0x58}}, 0x4004000)
> bind$packet(r0, &(0x7f00000000c0)={0x11, 0x0, r4, 0x1, 0x0, 0x6, @remote}, 0x14)
> sendmsg$nl_route(r0, &(0x7f0000000300)={0x0, 0x0, &(0x7f00000002c0)={0x0}}, 0x0)