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

[walter harms <wharms@bfs.de>]

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)