* [PATCH net] packet: in packet_snd start writing at link layer allocation @ 2018-05-11 17:24 Willem de Bruijn 2018-05-14 0:20 ` David Miller 0 siblings, 1 reply; 6+ messages in thread From: Willem de Bruijn @ 2018-05-11 17:24 UTC (permalink / raw) To: netdev; +Cc: davem, eric.dumazet, Willem de Bruijn From: Willem de Bruijn <willemb@google.com> Packet sockets allow construction of packets shorter than dev->hard_header_len to accommodate protocols with variable length link layer headers. These packets are padded to dev->hard_header_len, because some device drivers interpret that as a minimum packet size. packet_snd reserves dev->hard_header_len bytes on allocation. SOCK_DGRAM sockets call skb_push in dev_hard_header() to ensure that link layer headers are stored in the reserved range. SOCK_RAW sockets do the same in tpacket_snd, but not in packet_snd. Syzbot was able to send a zero byte packet to a device with massive 116B link layer header, causing padding to cross over into skb_shinfo. Fix this by writing from the start of the llheader reserved range also in the case of packet_snd/SOCK_RAW. Update skb_set_network_header to the new offset. This also corrects it for SOCK_DGRAM, where it incorrectly double counted reserve due to the skb_push in dev_hard_header. Fixes: 9ed988cd5915 ("packet: validate variable length ll headers") Reported-by: syzbot+71d74a5406d02057d559@syzkaller.appspotmail.com Signed-off-by: Willem de Bruijn <willemb@google.com> --- net/packet/af_packet.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c index 01f3515cada0d..e9422fe451793 100644 --- a/net/packet/af_packet.c +++ b/net/packet/af_packet.c @@ -2903,13 +2903,15 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len) if (skb == NULL) goto out_unlock; - skb_set_network_header(skb, reserve); + skb_reset_network_header(skb); err = -EINVAL; if (sock->type == SOCK_DGRAM) { offset = dev_hard_header(skb, dev, ntohs(proto), addr, NULL, len); if (unlikely(offset < 0)) goto out_free; + } else if (reserve) { + skb_push(skb, reserve); } /* Returns -EFAULT on error */ -- 2.17.0.441.gb46fe60e1d-goog ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net] packet: in packet_snd start writing at link layer allocation 2018-05-11 17:24 [PATCH net] packet: in packet_snd start writing at link layer allocation Willem de Bruijn @ 2018-05-14 0:20 ` David Miller 2018-05-24 15:07 ` Tariq Toukan 0 siblings, 1 reply; 6+ messages in thread From: David Miller @ 2018-05-14 0:20 UTC (permalink / raw) To: willemdebruijn.kernel; +Cc: netdev, eric.dumazet, willemb From: Willem de Bruijn <willemdebruijn.kernel@gmail.com> Date: Fri, 11 May 2018 13:24:25 -0400 > From: Willem de Bruijn <willemb@google.com> > > Packet sockets allow construction of packets shorter than > dev->hard_header_len to accommodate protocols with variable length > link layer headers. These packets are padded to dev->hard_header_len, > because some device drivers interpret that as a minimum packet size. > > packet_snd reserves dev->hard_header_len bytes on allocation. > SOCK_DGRAM sockets call skb_push in dev_hard_header() to ensure that > link layer headers are stored in the reserved range. SOCK_RAW sockets > do the same in tpacket_snd, but not in packet_snd. > > Syzbot was able to send a zero byte packet to a device with massive > 116B link layer header, causing padding to cross over into skb_shinfo. > Fix this by writing from the start of the llheader reserved range also > in the case of packet_snd/SOCK_RAW. > > Update skb_set_network_header to the new offset. This also corrects > it for SOCK_DGRAM, where it incorrectly double counted reserve due to > the skb_push in dev_hard_header. > > Fixes: 9ed988cd5915 ("packet: validate variable length ll headers") > Reported-by: syzbot+71d74a5406d02057d559@syzkaller.appspotmail.com > Signed-off-by: Willem de Bruijn <willemb@google.com> Applied and queued up for -stable, thanks Willem. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] packet: in packet_snd start writing at link layer allocation 2018-05-14 0:20 ` David Miller @ 2018-05-24 15:07 ` Tariq Toukan 2018-05-24 15:17 ` Willem de Bruijn 0 siblings, 1 reply; 6+ messages in thread From: Tariq Toukan @ 2018-05-24 15:07 UTC (permalink / raw) To: David Miller, willemdebruijn.kernel Cc: netdev, eric.dumazet, willemb, Maor Gottlieb On 14/05/2018 3:20 AM, David Miller wrote: > From: Willem de Bruijn <willemdebruijn.kernel@gmail.com> > Date: Fri, 11 May 2018 13:24:25 -0400 > >> From: Willem de Bruijn <willemb@google.com> >> >> Packet sockets allow construction of packets shorter than >> dev->hard_header_len to accommodate protocols with variable length >> link layer headers. These packets are padded to dev->hard_header_len, >> because some device drivers interpret that as a minimum packet size. >> >> packet_snd reserves dev->hard_header_len bytes on allocation. >> SOCK_DGRAM sockets call skb_push in dev_hard_header() to ensure that >> link layer headers are stored in the reserved range. SOCK_RAW sockets >> do the same in tpacket_snd, but not in packet_snd. >> >> Syzbot was able to send a zero byte packet to a device with massive >> 116B link layer header, causing padding to cross over into skb_shinfo. >> Fix this by writing from the start of the llheader reserved range also >> in the case of packet_snd/SOCK_RAW. >> >> Update skb_set_network_header to the new offset. This also corrects >> it for SOCK_DGRAM, where it incorrectly double counted reserve due to >> the skb_push in dev_hard_header. >> >> Fixes: 9ed988cd5915 ("packet: validate variable length ll headers") >> Reported-by: syzbot+71d74a5406d02057d559@syzkaller.appspotmail.com >> Signed-off-by: Willem de Bruijn <willemb@google.com> > > Applied and queued up for -stable, thanks Willem. > Hi, One of our regression tests started failing. Once this patch is reverted, test passes. The tests add flow steering rules in the receiver side and in the sender side it send the packet with some RAW socket applications. Then received side gets completion with error. Our verification team compared the packets between the stable and the broken version, in the broken version we have some extra bytes at the end of the packet. It looks like some bad push to the SKB, maybe the conditional reserved addition should be more strict? Any idea? Regards, Tariq ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] packet: in packet_snd start writing at link layer allocation 2018-05-24 15:07 ` Tariq Toukan @ 2018-05-24 15:17 ` Willem de Bruijn 2018-05-24 17:01 ` Willem de Bruijn 0 siblings, 1 reply; 6+ messages in thread From: Willem de Bruijn @ 2018-05-24 15:17 UTC (permalink / raw) To: Tariq Toukan Cc: David Miller, Network Development, Eric Dumazet, Willem de Bruijn, Maor Gottlieb On Thu, May 24, 2018 at 11:07 AM, Tariq Toukan <tariqt@mellanox.com> wrote: > > > On 14/05/2018 3:20 AM, David Miller wrote: >> >> From: Willem de Bruijn <willemdebruijn.kernel@gmail.com> >> Date: Fri, 11 May 2018 13:24:25 -0400 >> >>> From: Willem de Bruijn <willemb@google.com> >>> >>> Packet sockets allow construction of packets shorter than >>> dev->hard_header_len to accommodate protocols with variable length >>> link layer headers. These packets are padded to dev->hard_header_len, >>> because some device drivers interpret that as a minimum packet size. >>> >>> packet_snd reserves dev->hard_header_len bytes on allocation. >>> SOCK_DGRAM sockets call skb_push in dev_hard_header() to ensure that >>> link layer headers are stored in the reserved range. SOCK_RAW sockets >>> do the same in tpacket_snd, but not in packet_snd. >>> >>> Syzbot was able to send a zero byte packet to a device with massive >>> 116B link layer header, causing padding to cross over into skb_shinfo. >>> Fix this by writing from the start of the llheader reserved range also >>> in the case of packet_snd/SOCK_RAW. >>> >>> Update skb_set_network_header to the new offset. This also corrects >>> it for SOCK_DGRAM, where it incorrectly double counted reserve due to >>> the skb_push in dev_hard_header. >>> >>> Fixes: 9ed988cd5915 ("packet: validate variable length ll headers") >>> Reported-by: syzbot+71d74a5406d02057d559@syzkaller.appspotmail.com >>> Signed-off-by: Willem de Bruijn <willemb@google.com> >> >> >> Applied and queued up for -stable, thanks Willem. >> > > Hi, > > One of our regression tests started failing. Once this patch is reverted, > test passes. > > The tests add flow steering rules in the receiver side and in the sender > side it send the packet with some RAW socket applications. Then received > side gets completion with error. > > Our verification team compared the packets between the stable and the broken > version, in the broken version we have some extra bytes at the end of the > packet. > > It looks like some bad push to the SKB, maybe the conditional reserved > addition should be more strict? > > Any idea? Thanks for reporting, sorry for the breakage. I think I might. This skb_push moves back the start of skb->data in the same way that tpacket_snd does. But it does not reduce the length passed to skb_put, so this might double count hard_header_len. Let me construct a test. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] packet: in packet_snd start writing at link layer allocation 2018-05-24 15:17 ` Willem de Bruijn @ 2018-05-24 17:01 ` Willem de Bruijn 2018-05-24 22:13 ` Willem de Bruijn 0 siblings, 1 reply; 6+ messages in thread From: Willem de Bruijn @ 2018-05-24 17:01 UTC (permalink / raw) To: Tariq Toukan Cc: David Miller, Network Development, Eric Dumazet, Willem de Bruijn, Maor Gottlieb On Thu, May 24, 2018 at 11:17 AM, Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote: > On Thu, May 24, 2018 at 11:07 AM, Tariq Toukan <tariqt@mellanox.com> wrote: >> >> >> On 14/05/2018 3:20 AM, David Miller wrote: >>> >>> From: Willem de Bruijn <willemdebruijn.kernel@gmail.com> >>> Date: Fri, 11 May 2018 13:24:25 -0400 >>> >>>> From: Willem de Bruijn <willemb@google.com> >>>> >>>> Packet sockets allow construction of packets shorter than >>>> dev->hard_header_len to accommodate protocols with variable length >>>> link layer headers. These packets are padded to dev->hard_header_len, >>>> because some device drivers interpret that as a minimum packet size. >>>> >>>> packet_snd reserves dev->hard_header_len bytes on allocation. >>>> SOCK_DGRAM sockets call skb_push in dev_hard_header() to ensure that >>>> link layer headers are stored in the reserved range. SOCK_RAW sockets >>>> do the same in tpacket_snd, but not in packet_snd. >>>> >>>> Syzbot was able to send a zero byte packet to a device with massive >>>> 116B link layer header, causing padding to cross over into skb_shinfo. >>>> Fix this by writing from the start of the llheader reserved range also >>>> in the case of packet_snd/SOCK_RAW. >>>> >>>> Update skb_set_network_header to the new offset. This also corrects >>>> it for SOCK_DGRAM, where it incorrectly double counted reserve due to >>>> the skb_push in dev_hard_header. >>>> >>>> Fixes: 9ed988cd5915 ("packet: validate variable length ll headers") >>>> Reported-by: syzbot+71d74a5406d02057d559@syzkaller.appspotmail.com >>>> Signed-off-by: Willem de Bruijn <willemb@google.com> >>> >>> >>> Applied and queued up for -stable, thanks Willem. >>> >> >> Hi, >> >> One of our regression tests started failing. Once this patch is reverted, >> test passes. >> >> The tests add flow steering rules in the receiver side and in the sender >> side it send the packet with some RAW socket applications. Then received >> side gets completion with error. >> >> Our verification team compared the packets between the stable and the broken >> version, in the broken version we have some extra bytes at the end of the >> packet. >> >> It looks like some bad push to the SKB, maybe the conditional reserved >> addition should be more strict? >> >> Any idea? > > Thanks for reporting, sorry for the breakage. > > I think I might. This skb_push moves back the start of skb->data in the > same way that tpacket_snd does. But it does not reduce the length > passed to skb_put, so this might double count hard_header_len. > > Let me construct a test. Indeed. Still verifying, but this almost certainly has to be @@ -2911,7 +2912,7 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len) if (unlikely(offset < 0)) goto out_free; } else if (reserve) { - skb_push(skb, reserve); + skb_reserve(skb, -reserve); } to move the start of the packet without changing its length. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] packet: in packet_snd start writing at link layer allocation 2018-05-24 17:01 ` Willem de Bruijn @ 2018-05-24 22:13 ` Willem de Bruijn 0 siblings, 0 replies; 6+ messages in thread From: Willem de Bruijn @ 2018-05-24 22:13 UTC (permalink / raw) To: Tariq Toukan Cc: David Miller, Network Development, Eric Dumazet, Willem de Bruijn, Maor Gottlieb On Thu, May 24, 2018 at 1:01 PM, Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote: > On Thu, May 24, 2018 at 11:17 AM, Willem de Bruijn > <willemdebruijn.kernel@gmail.com> wrote: >> On Thu, May 24, 2018 at 11:07 AM, Tariq Toukan <tariqt@mellanox.com> wrote: >>> >>> >>> On 14/05/2018 3:20 AM, David Miller wrote: >>>> >>>> From: Willem de Bruijn <willemdebruijn.kernel@gmail.com> >>>> Date: Fri, 11 May 2018 13:24:25 -0400 >>>> >>>>> From: Willem de Bruijn <willemb@google.com> >>>>> >>>>> Packet sockets allow construction of packets shorter than >>>>> dev->hard_header_len to accommodate protocols with variable length >>>>> link layer headers. These packets are padded to dev->hard_header_len, >>>>> because some device drivers interpret that as a minimum packet size. >>>>> >>>>> packet_snd reserves dev->hard_header_len bytes on allocation. >>>>> SOCK_DGRAM sockets call skb_push in dev_hard_header() to ensure that >>>>> link layer headers are stored in the reserved range. SOCK_RAW sockets >>>>> do the same in tpacket_snd, but not in packet_snd. >>>>> >>>>> Syzbot was able to send a zero byte packet to a device with massive >>>>> 116B link layer header, causing padding to cross over into skb_shinfo. >>>>> Fix this by writing from the start of the llheader reserved range also >>>>> in the case of packet_snd/SOCK_RAW. >>>>> >>>>> Update skb_set_network_header to the new offset. This also corrects >>>>> it for SOCK_DGRAM, where it incorrectly double counted reserve due to >>>>> the skb_push in dev_hard_header. >>>>> >>>>> Fixes: 9ed988cd5915 ("packet: validate variable length ll headers") >>>>> Reported-by: syzbot+71d74a5406d02057d559@syzkaller.appspotmail.com >>>>> Signed-off-by: Willem de Bruijn <willemb@google.com> >>>> >>>> >>>> Applied and queued up for -stable, thanks Willem. >>>> >>> >>> Hi, >>> >>> One of our regression tests started failing. Once this patch is reverted, >>> test passes. >>> >>> The tests add flow steering rules in the receiver side and in the sender >>> side it send the packet with some RAW socket applications. Then received >>> side gets completion with error. >>> >>> Our verification team compared the packets between the stable and the broken >>> version, in the broken version we have some extra bytes at the end of the >>> packet. >>> >>> It looks like some bad push to the SKB, maybe the conditional reserved >>> addition should be more strict? >>> >>> Any idea? >> >> Thanks for reporting, sorry for the breakage. >> >> I think I might. This skb_push moves back the start of skb->data in the >> same way that tpacket_snd does. But it does not reduce the length >> passed to skb_put, so this might double count hard_header_len. >> >> Let me construct a test. > > Indeed. > > Still verifying, but this almost certainly has to be > > @@ -2911,7 +2912,7 @@ static int packet_snd(struct socket *sock, > struct msghdr *msg, size_t len) > if (unlikely(offset < 0)) > goto out_free; > } else if (reserve) { > - skb_push(skb, reserve); > + skb_reserve(skb, -reserve); > } > > to move the start of the packet without changing its length. I sent http://patchwork.ozlabs.org/patch/920126/ Again, thanks a lot for reporting this, Tariq. I'm working on some packet socket boundary condition tests for tools/testing/selftests/net, so that I cannot push such a mistake again. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-05-24 22:14 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-05-11 17:24 [PATCH net] packet: in packet_snd start writing at link layer allocation Willem de Bruijn 2018-05-14 0:20 ` David Miller 2018-05-24 15:07 ` Tariq Toukan 2018-05-24 15:17 ` Willem de Bruijn 2018-05-24 17:01 ` Willem de Bruijn 2018-05-24 22:13 ` Willem de Bruijn
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.