* [PATCH net 0/2] Fixes for header length truncation
@ 2017-02-07 20:57 Willem de Bruijn
2017-02-07 20:57 ` [PATCH net 1/2] net: introduce device min_header_len Willem de Bruijn
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Willem de Bruijn @ 2017-02-07 20:57 UTC (permalink / raw)
To: netdev; +Cc: davem, eric.dumazet, sowmini.varadhan, dvyukov, Willem de Bruijn
From: Willem de Bruijn <willemb@google.com>
Packets should not enter the stack with truncated link layer headers
and link layer headers should always be stored in the skb linear
segment.
Patch 1 ensures the first for PF_PACKET sockets
Patch 2 ensures the second for PF_PACKET GSO sockets without tx_ring
Willem de Bruijn (2):
net: introduce device min_header_len
packet: round up linear to header len
drivers/net/loopback.c | 1 +
include/linux/netdevice.h | 4 ++++
net/ethernet/eth.c | 1 +
net/packet/af_packet.c | 7 ++++---
4 files changed, 10 insertions(+), 3 deletions(-)
--
2.11.0.483.g087da7b7c-goog
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCH net 1/2] net: introduce device min_header_len 2017-02-07 20:57 [PATCH net 0/2] Fixes for header length truncation Willem de Bruijn @ 2017-02-07 20:57 ` Willem de Bruijn 2017-02-07 21:20 ` Eric Dumazet 2017-02-07 21:24 ` Sowmini Varadhan 2017-02-07 20:57 ` [PATCH net 2/2] packet: round up linear to header len Willem de Bruijn 2017-02-08 18:56 ` [PATCH net 0/2] Fixes for header length truncation David Miller 2 siblings, 2 replies; 11+ messages in thread From: Willem de Bruijn @ 2017-02-07 20:57 UTC (permalink / raw) To: netdev; +Cc: davem, eric.dumazet, sowmini.varadhan, dvyukov, Willem de Bruijn From: Willem de Bruijn <willemb@google.com> The stack must not pass packets to device drivers that are shorter than the minimum link layer header length. Previously, packet sockets would drop packets smaller than or equal to dev->hard_header_len, but this has false positives. Zero length payload is used over Ethernet. Other link layer protocols support variable length headers. Support for validation of these protocols removed the min length check for all protocols. Introduce an explicit dev->min_header_len parameter and drop all packets below this value. Initially, set it to non-zero only for Ethernet and loopback. Other protocols can follow in a patch to net-next. Fixes: 9ed988cd5915 ("packet: validate variable length ll headers") Reported-by: Sowmini Varadhan <sowmini.varadhan@oracle.com> Signed-off-by: Willem de Bruijn <willemb@google.com> --- drivers/net/loopback.c | 1 + include/linux/netdevice.h | 4 ++++ net/ethernet/eth.c | 1 + 3 files changed, 6 insertions(+) diff --git a/drivers/net/loopback.c b/drivers/net/loopback.c index 1e05b7c2d157..0844f8496413 100644 --- a/drivers/net/loopback.c +++ b/drivers/net/loopback.c @@ -164,6 +164,7 @@ static void loopback_setup(struct net_device *dev) { dev->mtu = 64 * 1024; dev->hard_header_len = ETH_HLEN; /* 14 */ + dev->min_header_len = ETH_HLEN; /* 14 */ dev->addr_len = ETH_ALEN; /* 6 */ dev->type = ARPHRD_LOOPBACK; /* 0x0001*/ dev->flags = IFF_LOOPBACK; diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h index 70ad0291d517..27914672602d 100644 --- a/include/linux/netdevice.h +++ b/include/linux/netdevice.h @@ -1511,6 +1511,7 @@ enum netdev_priv_flags { * @max_mtu: Interface Maximum MTU value * @type: Interface hardware type * @hard_header_len: Maximum hardware header length. + * @min_header_len: Minimum hardware header length * * @needed_headroom: Extra headroom the hardware may need, but not in all * cases can this be guaranteed @@ -1728,6 +1729,7 @@ struct net_device { unsigned int max_mtu; unsigned short type; unsigned short hard_header_len; + unsigned short min_header_len; unsigned short needed_headroom; unsigned short needed_tailroom; @@ -2694,6 +2696,8 @@ static inline bool dev_validate_header(const struct net_device *dev, { if (likely(len >= dev->hard_header_len)) return true; + if (len < dev->min_header_len) + return false; if (capable(CAP_SYS_RAWIO)) { memset(ll_header + len, 0, dev->hard_header_len - len); diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c index 8c5a479681ca..516c87e75de7 100644 --- a/net/ethernet/eth.c +++ b/net/ethernet/eth.c @@ -356,6 +356,7 @@ void ether_setup(struct net_device *dev) dev->header_ops = ð_header_ops; dev->type = ARPHRD_ETHER; dev->hard_header_len = ETH_HLEN; + dev->min_header_len = ETH_HLEN; dev->mtu = ETH_DATA_LEN; dev->min_mtu = ETH_MIN_MTU; dev->max_mtu = ETH_DATA_LEN; -- 2.11.0.483.g087da7b7c-goog ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH net 1/2] net: introduce device min_header_len 2017-02-07 20:57 ` [PATCH net 1/2] net: introduce device min_header_len Willem de Bruijn @ 2017-02-07 21:20 ` Eric Dumazet 2017-02-07 21:24 ` Sowmini Varadhan 1 sibling, 0 replies; 11+ messages in thread From: Eric Dumazet @ 2017-02-07 21:20 UTC (permalink / raw) To: Willem de Bruijn Cc: netdev, davem, sowmini.varadhan, dvyukov, Willem de Bruijn On Tue, 2017-02-07 at 15:57 -0500, Willem de Bruijn wrote: > From: Willem de Bruijn <willemb@google.com> > > The stack must not pass packets to device drivers that are shorter > than the minimum link layer header length. > > Previously, packet sockets would drop packets smaller than or equal > to dev->hard_header_len, but this has false positives. Zero length > payload is used over Ethernet. Other link layer protocols support > variable length headers. Support for validation of these protocols > removed the min length check for all protocols. > > Introduce an explicit dev->min_header_len parameter and drop all > packets below this value. Initially, set it to non-zero only for > Ethernet and loopback. Other protocols can follow in a patch to > net-next. > > Fixes: 9ed988cd5915 ("packet: validate variable length ll headers") > Reported-by: Sowmini Varadhan <sowmini.varadhan@oracle.com> > Signed-off-by: Willem de Bruijn <willemb@google.com> > --- Acked-by: Eric Dumazet <edumazet@google.com> ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net 1/2] net: introduce device min_header_len 2017-02-07 20:57 ` [PATCH net 1/2] net: introduce device min_header_len Willem de Bruijn 2017-02-07 21:20 ` Eric Dumazet @ 2017-02-07 21:24 ` Sowmini Varadhan 1 sibling, 0 replies; 11+ messages in thread From: Sowmini Varadhan @ 2017-02-07 21:24 UTC (permalink / raw) To: Willem de Bruijn; +Cc: netdev, davem, eric.dumazet, dvyukov, Willem de Bruijn On (02/07/17 15:57), Willem de Bruijn wrote: > > From: Willem de Bruijn <willemb@google.com> > > The stack must not pass packets to device drivers that are shorter > than the minimum link layer header length. Acked-by: Sowmini Varadhan <sowmini.varadhan@oracle.com> ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH net 2/2] packet: round up linear to header len 2017-02-07 20:57 [PATCH net 0/2] Fixes for header length truncation Willem de Bruijn 2017-02-07 20:57 ` [PATCH net 1/2] net: introduce device min_header_len Willem de Bruijn @ 2017-02-07 20:57 ` Willem de Bruijn 2017-02-07 21:23 ` Eric Dumazet 2017-02-08 15:34 ` Sowmini Varadhan 2017-02-08 18:56 ` [PATCH net 0/2] Fixes for header length truncation David Miller 2 siblings, 2 replies; 11+ messages in thread From: Willem de Bruijn @ 2017-02-07 20:57 UTC (permalink / raw) To: netdev; +Cc: davem, eric.dumazet, sowmini.varadhan, dvyukov, Willem de Bruijn From: Willem de Bruijn <willemb@google.com> Link layer protocols may unconditionally pull headers, as Ethernet does in eth_type_trans. Ensure that the entire link layer header always lies in the skb linear segment. tpacket_snd has such a check. Extend this to packet_snd. Variable length link layer headers complicate the computation somewhat. Here skb->len may be smaller than dev->hard_header_len. Round up the linear length to be at least as long as the smallest of the two. Reported-by: Dmitry Vyukov <dvyukov@google.com> Signed-off-by: Willem de Bruijn <willemb@google.com> --- net/packet/af_packet.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c index 3d555c79a7b5..d56ee46b11fc 100644 --- a/net/packet/af_packet.c +++ b/net/packet/af_packet.c @@ -2755,7 +2755,7 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len) struct virtio_net_hdr vnet_hdr = { 0 }; int offset = 0; struct packet_sock *po = pkt_sk(sk); - int hlen, tlen; + int hlen, tlen, linear; int extra_len = 0; /* @@ -2816,8 +2816,9 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len) err = -ENOBUFS; hlen = LL_RESERVED_SPACE(dev); tlen = dev->needed_tailroom; - skb = packet_alloc_skb(sk, hlen + tlen, hlen, len, - __virtio16_to_cpu(vio_le(), vnet_hdr.hdr_len), + linear = __virtio16_to_cpu(vio_le(), vnet_hdr.hdr_len); + linear = max(linear, min_t(int, len, dev->hard_header_len)); + skb = packet_alloc_skb(sk, hlen + tlen, hlen, len, linear, msg->msg_flags & MSG_DONTWAIT, &err); if (skb == NULL) goto out_unlock; -- 2.11.0.483.g087da7b7c-goog ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH net 2/2] packet: round up linear to header len 2017-02-07 20:57 ` [PATCH net 2/2] packet: round up linear to header len Willem de Bruijn @ 2017-02-07 21:23 ` Eric Dumazet 2017-02-08 15:34 ` Sowmini Varadhan 1 sibling, 0 replies; 11+ messages in thread From: Eric Dumazet @ 2017-02-07 21:23 UTC (permalink / raw) To: Willem de Bruijn Cc: netdev, davem, sowmini.varadhan, dvyukov, Willem de Bruijn On Tue, 2017-02-07 at 15:57 -0500, Willem de Bruijn wrote: > From: Willem de Bruijn <willemb@google.com> > > Link layer protocols may unconditionally pull headers, as Ethernet > does in eth_type_trans. Ensure that the entire link layer header > always lies in the skb linear segment. tpacket_snd has such a check. > Extend this to packet_snd. > > Variable length link layer headers complicate the computation > somewhat. Here skb->len may be smaller than dev->hard_header_len. > > Round up the linear length to be at least as long as the smallest of > the two. > > Reported-by: Dmitry Vyukov <dvyukov@google.com> > Signed-off-by: Willem de Bruijn <willemb@google.com> > --- Acked-by: Eric Dumazet <edumazet@google.com> ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net 2/2] packet: round up linear to header len 2017-02-07 20:57 ` [PATCH net 2/2] packet: round up linear to header len Willem de Bruijn 2017-02-07 21:23 ` Eric Dumazet @ 2017-02-08 15:34 ` Sowmini Varadhan 2017-02-08 16:37 ` Willem de Bruijn 1 sibling, 1 reply; 11+ messages in thread From: Sowmini Varadhan @ 2017-02-08 15:34 UTC (permalink / raw) To: Willem de Bruijn; +Cc: netdev, davem, eric.dumazet, dvyukov, Willem de Bruijn On (02/07/17 15:57), Willem de Bruijn wrote: > @@ -2816,8 +2816,9 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len) > err = -ENOBUFS; > hlen = LL_RESERVED_SPACE(dev); > tlen = dev->needed_tailroom; > - skb = packet_alloc_skb(sk, hlen + tlen, hlen, len, > - __virtio16_to_cpu(vio_le(), vnet_hdr.hdr_len), > + linear = __virtio16_to_cpu(vio_le(), vnet_hdr.hdr_len); > + linear = max(linear, min_t(int, len, dev->hard_header_len)); > + skb = packet_alloc_skb(sk, hlen + tlen, hlen, len, linear, > msg->msg_flags & MSG_DONTWAIT, &err); do we need a similar check in packet_sendsmg_spkt (even if it is deprecated, would be better to get it to align with packet_snd?) Also tpacket_fill_skb should ensure that copylen is set up like the code above? --Sowmini ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net 2/2] packet: round up linear to header len 2017-02-08 15:34 ` Sowmini Varadhan @ 2017-02-08 16:37 ` Willem de Bruijn 2017-02-08 16:58 ` Sowmini Varadhan 0 siblings, 1 reply; 11+ messages in thread From: Willem de Bruijn @ 2017-02-08 16:37 UTC (permalink / raw) To: Sowmini Varadhan Cc: Network Development, David Miller, Eric Dumazet, Dmitry Vyukov, Willem de Bruijn On Wed, Feb 8, 2017 at 7:34 AM, Sowmini Varadhan <sowmini.varadhan@oracle.com> wrote: > On (02/07/17 15:57), Willem de Bruijn wrote: >> @@ -2816,8 +2816,9 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len) >> err = -ENOBUFS; >> hlen = LL_RESERVED_SPACE(dev); >> tlen = dev->needed_tailroom; >> - skb = packet_alloc_skb(sk, hlen + tlen, hlen, len, >> - __virtio16_to_cpu(vio_le(), vnet_hdr.hdr_len), >> + linear = __virtio16_to_cpu(vio_le(), vnet_hdr.hdr_len); >> + linear = max(linear, min_t(int, len, dev->hard_header_len)); >> + skb = packet_alloc_skb(sk, hlen + tlen, hlen, len, linear, >> msg->msg_flags & MSG_DONTWAIT, &err); > > do we need a similar check in packet_sendsmg_spkt (even if it > is deprecated, would be better to get it to align with packet_snd?) That path does not need this check, because it does not support large packets and always allocates the entire packet as linear. > Also tpacket_fill_skb should ensure that copylen is set up like > the code above? Do you mean the difference that it unconditionally pulls hard_header_len, optionally with zero padding, whereas this new path can check against new min_header_len and thus allows packets shorter than hard_header_len? ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net 2/2] packet: round up linear to header len 2017-02-08 16:37 ` Willem de Bruijn @ 2017-02-08 16:58 ` Sowmini Varadhan 2017-02-08 17:27 ` Willem de Bruijn 0 siblings, 1 reply; 11+ messages in thread From: Sowmini Varadhan @ 2017-02-08 16:58 UTC (permalink / raw) To: Willem de Bruijn Cc: Network Development, David Miller, Eric Dumazet, Dmitry Vyukov, Willem de Bruijn On (02/08/17 08:37), Willem de Bruijn wrote: > Date: Wed, 8 Feb 2017 08:37:19 -0800 > From: Willem de Bruijn <willemdebruijn.kernel@gmail.com> > To: Sowmini Varadhan <sowmini.varadhan@oracle.com> > Cc: Network Development <netdev@vger.kernel.org>, David Miller > <davem@davemloft.net>, Eric Dumazet <eric.dumazet@gmail.com>, Dmitry > Vyukov <dvyukov@google.com>, Willem de Bruijn <willemb@google.com> > Subject: Re: [PATCH net 2/2] packet: round up linear to header len > > On Wed, Feb 8, 2017 at 7:34 AM, Sowmini Varadhan > <sowmini.varadhan@oracle.com> wrote: > > On (02/07/17 15:57), Willem de Bruijn wrote: > >> @@ -2816,8 +2816,9 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len) > >> err = -ENOBUFS; > >> hlen = LL_RESERVED_SPACE(dev); > >> tlen = dev->needed_tailroom; > >> - skb = packet_alloc_skb(sk, hlen + tlen, hlen, len, > >> - __virtio16_to_cpu(vio_le(), vnet_hdr.hdr_len), > >> + linear = __virtio16_to_cpu(vio_le(), vnet_hdr.hdr_len); > >> + linear = max(linear, min_t(int, len, dev->hard_header_len)); > >> + skb = packet_alloc_skb(sk, hlen + tlen, hlen, len, linear, > >> msg->msg_flags & MSG_DONTWAIT, &err); > > : > Do you mean the difference that it unconditionally pulls > hard_header_len, optionally with zero padding, whereas this new > path can check against new min_header_len and thus allows > packets shorter than hard_header_len? yes, maybe it doesnt matter, becaues hard_header_len >= min_header_len at all times --Sowmini ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net 2/2] packet: round up linear to header len 2017-02-08 16:58 ` Sowmini Varadhan @ 2017-02-08 17:27 ` Willem de Bruijn 0 siblings, 0 replies; 11+ messages in thread From: Willem de Bruijn @ 2017-02-08 17:27 UTC (permalink / raw) To: Sowmini Varadhan Cc: Network Development, David Miller, Eric Dumazet, Dmitry Vyukov, Willem de Bruijn On Wed, Feb 8, 2017 at 8:58 AM, Sowmini Varadhan <sowmini.varadhan@oracle.com> wrote: > On (02/08/17 08:37), Willem de Bruijn wrote: >> Date: Wed, 8 Feb 2017 08:37:19 -0800 >> From: Willem de Bruijn <willemdebruijn.kernel@gmail.com> >> To: Sowmini Varadhan <sowmini.varadhan@oracle.com> >> Cc: Network Development <netdev@vger.kernel.org>, David Miller >> <davem@davemloft.net>, Eric Dumazet <eric.dumazet@gmail.com>, Dmitry >> Vyukov <dvyukov@google.com>, Willem de Bruijn <willemb@google.com> >> Subject: Re: [PATCH net 2/2] packet: round up linear to header len >> >> On Wed, Feb 8, 2017 at 7:34 AM, Sowmini Varadhan >> <sowmini.varadhan@oracle.com> wrote: >> > On (02/07/17 15:57), Willem de Bruijn wrote: >> >> @@ -2816,8 +2816,9 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len) >> >> err = -ENOBUFS; >> >> hlen = LL_RESERVED_SPACE(dev); >> >> tlen = dev->needed_tailroom; >> >> - skb = packet_alloc_skb(sk, hlen + tlen, hlen, len, >> >> - __virtio16_to_cpu(vio_le(), vnet_hdr.hdr_len), >> >> + linear = __virtio16_to_cpu(vio_le(), vnet_hdr.hdr_len); >> >> + linear = max(linear, min_t(int, len, dev->hard_header_len)); >> >> + skb = packet_alloc_skb(sk, hlen + tlen, hlen, len, linear, >> >> msg->msg_flags & MSG_DONTWAIT, &err); >> > > : >> Do you mean the difference that it unconditionally pulls >> hard_header_len, optionally with zero padding, whereas this new >> path can check against new min_header_len and thus allows >> packets shorter than hard_header_len? > > yes, maybe it doesnt matter, becaues hard_header_len >= min_header_len > at all times The code is not subject to this bug, so I'd rather not touch it in this fix for stable. But you raise a good point. This logic is subtle and fragile. It will be good to deduplicate across packet_snd and tpacket_snd in a follow-up to net-next. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net 0/2] Fixes for header length truncation 2017-02-07 20:57 [PATCH net 0/2] Fixes for header length truncation Willem de Bruijn 2017-02-07 20:57 ` [PATCH net 1/2] net: introduce device min_header_len Willem de Bruijn 2017-02-07 20:57 ` [PATCH net 2/2] packet: round up linear to header len Willem de Bruijn @ 2017-02-08 18:56 ` David Miller 2 siblings, 0 replies; 11+ messages in thread From: David Miller @ 2017-02-08 18:56 UTC (permalink / raw) To: willemdebruijn.kernel Cc: netdev, eric.dumazet, sowmini.varadhan, dvyukov, willemb From: Willem de Bruijn <willemdebruijn.kernel@gmail.com> Date: Tue, 7 Feb 2017 15:57:19 -0500 > From: Willem de Bruijn <willemb@google.com> > > Packets should not enter the stack with truncated link layer headers > and link layer headers should always be stored in the skb linear > segment. > > Patch 1 ensures the first for PF_PACKET sockets > Patch 2 ensures the second for PF_PACKET GSO sockets without tx_ring Nice work, I'll apply this series and queue it up for -stable. Thanks! ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2017-02-08 18:57 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-02-07 20:57 [PATCH net 0/2] Fixes for header length truncation Willem de Bruijn 2017-02-07 20:57 ` [PATCH net 1/2] net: introduce device min_header_len Willem de Bruijn 2017-02-07 21:20 ` Eric Dumazet 2017-02-07 21:24 ` Sowmini Varadhan 2017-02-07 20:57 ` [PATCH net 2/2] packet: round up linear to header len Willem de Bruijn 2017-02-07 21:23 ` Eric Dumazet 2017-02-08 15:34 ` Sowmini Varadhan 2017-02-08 16:37 ` Willem de Bruijn 2017-02-08 16:58 ` Sowmini Varadhan 2017-02-08 17:27 ` Willem de Bruijn 2017-02-08 18:56 ` [PATCH net 0/2] Fixes for header length truncation David Miller
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.