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