lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <CANn89i+u6tFJQKESV9DH-HypezVV7Ux+XhnyFGLd833PR9Qpyw@mail.gmail.com> Date: Thu, 30 Nov 2023 15:05:38 +0100 From: Eric Dumazet <edumazet@...gle.com> To: Shigeru Yoshida <syoshida@...hat.com> Cc: willemdebruijn.kernel@...il.com, pabeni@...hat.com, davem@...emloft.net, dsahern@...nel.org, kuba@...nel.org, netdev@...r.kernel.org, linux-kernel@...r.kernel.org Subject: Re: [PATCH net] ipv4: ip_gre: Handle skb_pull() failure in ipgre_xmit() On Thu, Nov 30, 2023 at 3:03 PM Shigeru Yoshida <syoshida@...hat.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@...hat.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@...gle.com>' then. Thanks.
Powered by blists - more mailing lists