[PATCH net 0/3] validate variable length ll headers

[PATCH net 0/3] validate variable length ll headers

[Willem de Bruijn]

From: Willem de Bruijn <willemb@google.com>

Allow device-specific validation of link layer headers. Existing
checks drop all packets shorter than hard_header_len. For variable
length protocols, such packets can be valid.

patch 1 adds header_ops.validate and dev_validate_header
patch 2 replaces ll_header_truncated with dev_validate_header
patch 3 implements the protocol specific callback for AX25

Tested with a temporary eth_header_validate function. The AX25
code is compile-tested only at this point.

Willem de Bruijn (3):
  net: validate variable length ll headers
  packet: validate variable length ll headers
  ax25: add link layer header validation function

 include/linux/netdevice.h | 22 ++++++++++++++++++++--
 net/ax25/ax25_ip.c        | 15 +++++++++++++++
 net/packet/af_packet.c    | 38 +++++++++++++++++---------------------
 3 files changed, 52 insertions(+), 23 deletions(-)

-- 
2.7.0.rc3.207.g0ac5344

[PATCH net 1/3] net: validate variable length ll headers

[Willem de Bruijn]

From: Willem de Bruijn <willemb@google.com>

Netdevice parameter hard_header_len is variously interpreted both as
an upper and lower bound on link layer header length. The field is
used as upper bound when reserving room at allocation, as lower bound
when validating user input in PF_PACKET.

Clarify the definition to be maximum header length. For validation
of untrusted headers, add an optional validate member to header_ops.

Allow bypassing of validation by passing CAP_SYS_RAWIO, for instance
for deliberate testing of corrupt input. In this case, pad trailing
bytes, as some device drivers expect completely initialized headers.

See also http://comments.gmane.org/gmane.linux.network/401064

Signed-off-by: Willem de Bruijn <willemb@google.com>
---
 include/linux/netdevice.h | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 5440b7b..6d1d8f4 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -267,6 +267,7 @@ struct header_ops {
 	void	(*cache_update)(struct hh_cache *hh,
 				const struct net_device *dev,
 				const unsigned char *haddr);
+	bool	(*validate)(const char *ll_header, unsigned int len);
 };
 
 /* These flag bits are private to the generic network queueing
@@ -1420,8 +1421,7 @@ enum netdev_priv_flags {
  *	@dma:		DMA channel
  *	@mtu:		Interface MTU value
  *	@type:		Interface hardware type
- *	@hard_header_len: Hardware header length, which means that this is the
- *			  minimum size of a packet.
+ *	@hard_header_len: Maximum hardware header length.
  *
  *	@needed_headroom: Extra headroom the hardware may need, but not in all
  *			  cases can this be guaranteed
@@ -2627,6 +2627,24 @@ static inline int dev_parse_header(const struct sk_buff *skb,
 	return dev->header_ops->parse(skb, haddr);
 }
 
+/* ll_header must have at least hard_header_len allocated */
+static inline bool dev_validate_header(const struct net_device *dev,
+				       char *ll_header, int len)
+{
+	if (likely(len >= dev->hard_header_len))
+		return true;
+
+	if (capable(CAP_SYS_RAWIO)) {
+		memset(ll_header + len, 0, dev->hard_header_len - len);
+		return true;
+	}
+
+	if (dev->header_ops && dev->header_ops->validate)
+		return dev->header_ops->validate(ll_header, len);
+
+	return false;
+}
+
 typedef int gifconf_func_t(struct net_device * dev, char __user * bufptr, int len);
 int register_gifconf(unsigned int family, gifconf_func_t *gifconf);
 static inline int unregister_gifconf(unsigned int family)
-- 
2.7.0.rc3.207.g0ac5344

Re: [PATCH net 1/3] net: validate variable length ll headers

[walter harms]

Am 04.03.2016 21:44, schrieb Willem de Bruijn:
> From: Willem de Bruijn <willemb@google.com>
> 
> Netdevice parameter hard_header_len is variously interpreted both as
> an upper and lower bound on link layer header length. The field is
> used as upper bound when reserving room at allocation, as lower bound
> when validating user input in PF_PACKET.
> 
> Clarify the definition to be maximum header length. For validation
> of untrusted headers, add an optional validate member to header_ops.
> 
> Allow bypassing of validation by passing CAP_SYS_RAWIO, for instance
> for deliberate testing of corrupt input. In this case, pad trailing
> bytes, as some device drivers expect completely initialized headers.
> 
> See also http://comments.gmane.org/gmane.linux.network/401064
> 
> Signed-off-by: Willem de Bruijn <willemb@google.com>
> ---
>  include/linux/netdevice.h | 22 ++++++++++++++++++++--
>  1 file changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 5440b7b..6d1d8f4 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -267,6 +267,7 @@ struct header_ops {
>  	void	(*cache_update)(struct hh_cache *hh,
>  				const struct net_device *dev,
>  				const unsigned char *haddr);
> +	bool	(*validate)(const char *ll_header, unsigned int len);
>  };
>  
>  /* These flag bits are private to the generic network queueing
> @@ -1420,8 +1421,7 @@ enum netdev_priv_flags {
>   *	@dma:		DMA channel
>   *	@mtu:		Interface MTU value
>   *	@type:		Interface hardware type
> - *	@hard_header_len: Hardware header length, which means that this is the
> - *			  minimum size of a packet.
> + *	@hard_header_len: Maximum hardware header length.
>   *
>   *	@needed_headroom: Extra headroom the hardware may need, but not in all
>   *			  cases can this be guaranteed
> @@ -2627,6 +2627,24 @@ static inline int dev_parse_header(const struct sk_buff *skb,
>  	return dev->header_ops->parse(skb, haddr);
>  }
>  
> +/* ll_header must have at least hard_header_len allocated */
> +static inline bool dev_validate_header(const struct net_device *dev,
> +				       char *ll_header, int len)
> +{
> +	if (likely(len >= dev->hard_header_len))
> +		return true;
> +
> +	if (capable(CAP_SYS_RAWIO)) {
> +		memset(ll_header + len, 0, dev->hard_header_len - len);
> +		return true;
> +	}
> +
> +	if (dev->header_ops && dev->header_ops->validate)
> +		return dev->header_ops->validate(ll_header, len);
> +
> +	return false;
> +}
> +


you could use

real_len=dev->hard_header_len-len;

if (real_len < 0)
...
if (capable(CAP_SYS_RAWIO))
	memset(ll_header + len, 0,real_len);
..

IMHO that makes the code more clear.

re,
 wh



>  typedef int gifconf_func_t(struct net_device * dev, char __user * bufptr, int len);
>  int register_gifconf(unsigned int family, gifconf_func_t *gifconf);
>  static inline int unregister_gifconf(unsigned int family)

[PATCH net 2/3] packet: validate variable length ll headers

[Willem de Bruijn]

From: Willem de Bruijn <willemb@google.com>

Replace link layer header validation check ll_header_truncate with
more generic dev_validate_header.

Validation based on hard_header_len incorrectly drops valid packets
in variable length protocols, such as AX25. dev_validate_header
calls header_ops.validate for such protocols to ensure correctness
below hard_header_len.

See also http://comments.gmane.org/gmane.linux.network/401064

Signed-off-by: Willem de Bruijn <willemb@google.com>
---
 net/packet/af_packet.c | 38 +++++++++++++++++---------------------
 1 file changed, 17 insertions(+), 21 deletions(-)

diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 992396a..488678a 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -1916,6 +1916,10 @@ retry:
 		goto retry;
 	}
 
+	if (!dev_validate_header(dev, skb->data, len)) {
+		err = -EINVAL;
+		goto out_unlock;
+	}
 	if (len > (dev->mtu + dev->hard_header_len + extra_len) &&
 	    !packet_extra_vlan_len_allowed(dev, skb)) {
 		err = -EMSGSIZE;
@@ -2326,18 +2330,6 @@ static void tpacket_destruct_skb(struct sk_buff *skb)
 	sock_wfree(skb);
 }
 
-static bool ll_header_truncated(const struct net_device *dev, int len)
-{
-	/* net device doesn't like empty head */
-	if (unlikely(len < dev->hard_header_len)) {
-		net_warn_ratelimited("%s: packet size is too short (%d < %d)\n",
-				     current->comm, len, dev->hard_header_len);
-		return true;
-	}
-
-	return false;
-}
-
 static void tpacket_set_protocol(const struct net_device *dev,
 				 struct sk_buff *skb)
 {
@@ -2420,14 +2412,14 @@ static int tpacket_fill_skb(struct packet_sock *po, struct sk_buff *skb,
 		if (unlikely(err < 0))
 			return -EINVAL;
 	} else if (dev->hard_header_len) {
-		if (ll_header_truncated(dev, tp_len))
-			return -EINVAL;
+		int hhlen = min_t(int, dev->hard_header_len, tp_len);
 
 		skb_push(skb, dev->hard_header_len);
-		err = skb_store_bits(skb, 0, data,
-				dev->hard_header_len);
+		err = skb_store_bits(skb, 0, data, hhlen);
 		if (unlikely(err))
 			return err;
+		if (!dev_validate_header(dev, skb->data, hhlen))
+			return -EINVAL;
 		if (!skb->protocol)
 			tpacket_set_protocol(dev, skb);
 
@@ -2758,14 +2750,12 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len)
 
 	skb_set_network_header(skb, reserve);
 
-	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 (ll_header_truncated(dev, len))
+		if (unlikely(offset < 0)) {
+			err = -EINVAL;
 			goto out_free;
+		}
 	}
 
 	/* Returns -EFAULT on error */
@@ -2773,6 +2763,12 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len)
 	if (err)
 		goto out_free;
 
+	if (sock->type == SOCK_RAW &&
+	    !dev_validate_header(dev, skb->data, len)) {
+		err = -EINVAL;
+		goto out_free;
+	}
+
 	sock_tx_timestamp(sk, &skb_shinfo(skb)->tx_flags);
 
 	if (!gso_type && (len > dev->mtu + reserve + extra_len) &&
-- 
2.7.0.rc3.207.g0ac5344

Re: [PATCH net 2/3] packet: validate variable length ll headers

[Willem de Bruijn]

On Fri, Mar 4, 2016 at 3:44 PM, Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
> From: Willem de Bruijn <willemb@google.com>
>
> Replace link layer header validation check ll_header_truncate with
> more generic dev_validate_header.
>
> Validation based on hard_header_len incorrectly drops valid packets
> in variable length protocols, such as AX25. dev_validate_header
> calls header_ops.validate for such protocols to ensure correctness
> below hard_header_len.
>
> See also http://comments.gmane.org/gmane.linux.network/401064

Forgot to add

  Fixes 9c7077622dd9 ("packet: make packet_snd fail on len smaller
than l2 header")

This patch, if submitted to net, will have non-trivial merge conflicts
with net-next due to

  1d036d25e560 ("packet: tpacket_snd gso and checksum offload")

I maintain a patchset on top of net-next. Can send that to supersede
this one to resolve it (at the cost of only fixing the bug in the later
kernel version).

> Signed-off-by: Willem de Bruijn <willemb@google.com>
> ---
>  net/packet/af_packet.c | 38 +++++++++++++++++---------------------
>  1 file changed, 17 insertions(+), 21 deletions(-)
>
> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index 992396a..488678a 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -1916,6 +1916,10 @@ retry:
>                 goto retry;
>         }
>
> +       if (!dev_validate_header(dev, skb->data, len)) {
> +               err = -EINVAL;
> +               goto out_unlock;
> +       }
>         if (len > (dev->mtu + dev->hard_header_len + extra_len) &&
>             !packet_extra_vlan_len_allowed(dev, skb)) {
>                 err = -EMSGSIZE;
> @@ -2326,18 +2330,6 @@ static void tpacket_destruct_skb(struct sk_buff *skb)
>         sock_wfree(skb);
>  }
>
> -static bool ll_header_truncated(const struct net_device *dev, int len)
> -{
> -       /* net device doesn't like empty head */
> -       if (unlikely(len < dev->hard_header_len)) {
> -               net_warn_ratelimited("%s: packet size is too short (%d < %d)\n",
> -                                    current->comm, len, dev->hard_header_len);
> -               return true;
> -       }
> -
> -       return false;
> -}
> -
>  static void tpacket_set_protocol(const struct net_device *dev,
>                                  struct sk_buff *skb)
>  {
> @@ -2420,14 +2412,14 @@ static int tpacket_fill_skb(struct packet_sock *po, struct sk_buff *skb,
>                 if (unlikely(err < 0))
>                         return -EINVAL;
>         } else if (dev->hard_header_len) {
> -               if (ll_header_truncated(dev, tp_len))
> -                       return -EINVAL;
> +               int hhlen = min_t(int, dev->hard_header_len, tp_len);
>
>                 skb_push(skb, dev->hard_header_len);
> -               err = skb_store_bits(skb, 0, data,
> -                               dev->hard_header_len);
> +               err = skb_store_bits(skb, 0, data, hhlen);
>                 if (unlikely(err))
>                         return err;
> +               if (!dev_validate_header(dev, skb->data, hhlen))
> +                       return -EINVAL;
>                 if (!skb->protocol)
>                         tpacket_set_protocol(dev, skb);
>
> @@ -2758,14 +2750,12 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len)
>
>         skb_set_network_header(skb, reserve);
>
> -       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 (ll_header_truncated(dev, len))
> +               if (unlikely(offset < 0)) {
> +                       err = -EINVAL;
>                         goto out_free;
> +               }
>         }
>
>         /* Returns -EFAULT on error */
> @@ -2773,6 +2763,12 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len)
>         if (err)
>                 goto out_free;
>
> +       if (sock->type == SOCK_RAW &&
> +           !dev_validate_header(dev, skb->data, len)) {
> +               err = -EINVAL;
> +               goto out_free;
> +       }
> +
>         sock_tx_timestamp(sk, &skb_shinfo(skb)->tx_flags);
>
>         if (!gso_type && (len > dev->mtu + reserve + extra_len) &&
> --
> 2.7.0.rc3.207.g0ac5344
>

[PATCH net 3/3] ax25: add link layer header validation function

[Willem de Bruijn]

From: Willem de Bruijn <willemb@google.com>

As variable length protocol, AX25 fails link layer header validation
tests based on a minimum length. header_ops.validate allows protocols
to validate headers that are shorter than hard_header_len. Implement
this callback for AX25.

Compile tested only.

See also http://comments.gmane.org/gmane.linux.network/401064

Signed-off-by: Willem de Bruijn <willemb@google.com>
---
 net/ax25/ax25_ip.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/net/ax25/ax25_ip.c b/net/ax25/ax25_ip.c
index b563a3f..77bc20d 100644
--- a/net/ax25/ax25_ip.c
+++ b/net/ax25/ax25_ip.c
@@ -228,8 +228,23 @@ netdev_tx_t ax25_ip_xmit(struct sk_buff *skb)
 }
 #endif
 
+static bool ax25_validate_header(const char *header, unsigned int len)
+{
+	ax25_digi digi;
+
+	if (!len)
+		return false;
+
+	if (header[0] != 0)
+		return true;
+
+	return ax25_addr_parse(header + 1, len - 1, NULL, NULL, &digi, NULL,
+			       NULL);
+}
+
 const struct header_ops ax25_header_ops = {
 	.create = ax25_hard_header,
+	.validate = ax25_validate_header,
 };
 
 EXPORT_SYMBOL(ax25_header_ops);
-- 
2.7.0.rc3.207.g0ac5344

Re: [PATCH net 0/3] validate variable length ll headers

[David Miller]

From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Date: Fri,  4 Mar 2016 15:44:14 -0500

> From: Willem de Bruijn <willemb@google.com>
> 
> Allow device-specific validation of link layer headers. Existing
> checks drop all packets shorter than hard_header_len. For variable
> length protocols, such packets can be valid.
> 
> patch 1 adds header_ops.validate and dev_validate_header
> patch 2 replaces ll_header_truncated with dev_validate_header
> patch 3 implements the protocol specific callback for AX25
> 
> Tested with a temporary eth_header_validate function. The AX25
> code is compile-tested only at this point.

I'm not going to be able to send another pull request to Linus
before -final, so please respin this against net-next and I'll
queue it up for -stable.

You can add the missing Fixes: tags as well when you do this.

Thanks.