[PATCH net 0/2] Fixes for header length truncation

[PATCH net 0/2] Fixes for header length truncation

[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

[PATCH net 1/2] net: introduce device min_header_len

[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		= &eth_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

[PATCH net 2/2] packet: round up linear to header len

[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

Re: [PATCH net 1/2] net: introduce device min_header_len

[Eric Dumazet]

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>

Re: [PATCH net 2/2] packet: round up linear to header len

[Eric Dumazet]

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>

Re: [PATCH net 1/2] net: introduce device min_header_len

[Sowmini Varadhan]

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>

Re: [PATCH net 2/2] packet: round up linear to header len

[Sowmini Varadhan]

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

Re: [PATCH net 2/2] packet: round up linear to header len

[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?

Re: [PATCH net 2/2] packet: round up linear to header len

[Sowmini Varadhan]

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

Re: [PATCH net 2/2] packet: round up linear to header len

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

Re: [PATCH net 0/2] Fixes for header length truncation

[David Miller]

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!