Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:5 as permitted sender) client-ip=2620:137:e000::3:5;
MIME-Version: 1.0
References: <20231126151652.372783-1-syoshida@redhat.com> <6564bbd5580de_8a1ac29481@willemb.c.googlers.com.notmuch>
 <20231129.105046.2126277148145584341.syoshida@redhat.com>
In-Reply-To: <20231129.105046.2126277148145584341.syoshida@redhat.com>
From:   Eric Dumazet <edumazet@google.com>
Date:   Wed, 29 Nov 2023 10:56:47 +0100
Message-ID: <CANn89iLxEZAjomWEW4GFJds6kyd6Zf+ed9kx6eVsaQ57De6gMw@mail.gmail.com>
Subject: Re: [PATCH net] ipv4: ip_gre: Handle skb_pull() failure in ipgre_xmit()
To:     Shigeru Yoshida <syoshida@redhat.com>
Cc:     willemdebruijn.kernel@gmail.com, pabeni@redhat.com,
        davem@davemloft.net, dsahern@kernel.org, kuba@kernel.org,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Wed, Nov 29, 2023 at 2:51=E2=80=AFAM Shigeru Yoshida <syoshida@redhat.co=
m> 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() retu=
rns
> >> true. For example, applications can create a malformed packet that cau=
ses
> >> 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.

>
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D
> 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 =3D 0;
>         struct ifreq ifr;
>         struct sockaddr_ll addr =3D { 0 };
>         unsigned char mac_addr[] =3D {0x1, 0x2, 0x3, 0x4, 0x5, 0x6};
>
>         s =3D socket(AF_PACKET, SOCK_DGRAM, 0x300);
>         s1 =3D socket(AF_PACKET, SOCK_RAW, 0x300);
>         s2 =3D socket(AF_NETLINK, SOCK_RAW, 0);
>
>         strcpy(ifr.ifr_name, "gre0");
>         ioctl(s2, SIOCGIFINDEX, &ifr);
>
>         addr.sll_family =3D AF_PACKET;
>         addr.sll_ifindex =3D ifr.ifr_ifindex;
>         addr.sll_protocol =3D htons(0);
>         addr.sll_hatype =3D ARPHRD_ETHER;
>         addr.sll_pkttype =3D PACKET_HOST;
>         addr.sll_halen =3D 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..5169c3c72cffe49cef613e69889=
d139db867ff74
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 =3D tunnel->hlen + sizeof(struct iphdr);
+
                if (skb_cow_head(skb, 0))
                        goto free_skb;

                tnl_params =3D (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 =3D=3D CHECKSUM_PARTIAL &&