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

From: Shigeru Yoshida
Date: Tue Nov 28 2023 - 20:58:39 EST

Next message: Stephen Rothwell: "linux-next: build warning after merge of the pwm tree"
Previous message: Jacky Huang: "Re: [PATCH v2 3/4] arm64: dts: nuvoton: Add pinctrl support for ma35d1"
In reply to: Willem de Bruijn: "Re: [PATCH net] ipv4: ip_gre: Handle skb_pull() failure in ipgre_xmit()"
Next in thread: Eric Dumazet: "Re: [PATCH net] ipv4: ip_gre: Handle skb_pull() failure in ipgre_xmit()"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxxxxxx>
>> ---
>> 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
>>
>
>

Next message: Stephen Rothwell: "linux-next: build warning after merge of the pwm tree"
Previous message: Jacky Huang: "Re: [PATCH v2 3/4] arm64: dts: nuvoton: Add pinctrl support for ma35d1"
In reply to: Willem de Bruijn: "Re: [PATCH net] ipv4: ip_gre: Handle skb_pull() failure in ipgre_xmit()"
Next in thread: Eric Dumazet: "Re: [PATCH net] ipv4: ip_gre: Handle skb_pull() failure in ipgre_xmit()"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]