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

[Shigeru Yoshida <syoshida@redhat.com>]

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.
>