netdev - Re: [PATCH 2/3] net: is_skb_forwardable: validate length of GSO packet segments

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20180118234737.GA3502@localhost.localdomain>
Date:   Thu, 18 Jan 2018 21:47:38 -0200
From:   Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
To:     Daniel Axtens <dja@...ens.net>
Cc:     netdev@...r.kernel.org, Manish.Chopra@...ium.com,
        dev@...nvswitch.org
Subject: Re: [PATCH 2/3] net: is_skb_forwardable: validate length of GSO
 packet segments

On Tue, Jan 16, 2018 at 01:09:19PM +1100, Daniel Axtens wrote:
> is_skb_forwardable attempts to detect if a packet is too large to
> be sent to the destination device. However, this test does not
> consider GSO packets, and it is possible that a GSO packet, when
> resegmented, will be larger than the device can transmit.
> 
> Add detection for packets which will be too large by creating a
> skb_gso_validate_len() routine which is similar to
> skb_gso_validate_mtu(), but which considers L2 headers, and wire

Why not add a 3rd parameter to skb_gso_validate_mtu() instead, hlen?
After all it's the only difference between both.

And maybe rename _mtu if you think the term doesn't fit in the new
usecase or add a wrapper around it for each case.

As it is here, it gets very confusing on which function does what,
IMHO.

> it up in is_skb_forwardable().
> 
> Signed-off-by: Daniel Axtens <dja@...ens.net>
> ---
>  include/linux/skbuff.h |  1 +
>  net/core/dev.c         |  7 ++++---
>  net/core/skbuff.c      | 34 ++++++++++++++++++++++++++++++++++
>  3 files changed, 39 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index b8acafd73272..6a9e4b6f80a7 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -3287,6 +3287,7 @@ int skb_shift(struct sk_buff *tgt, struct sk_buff *skb, int shiftlen);
>  void skb_scrub_packet(struct sk_buff *skb, bool xnet);
>  unsigned int skb_gso_transport_seglen(const struct sk_buff *skb);
>  bool skb_gso_validate_mtu(const struct sk_buff *skb, unsigned int mtu);
> +bool skb_gso_validate_len(const struct sk_buff *skb, unsigned int len);
>  struct sk_buff *skb_segment(struct sk_buff *skb, netdev_features_t features);
>  struct sk_buff *skb_vlan_untag(struct sk_buff *skb);
>  int skb_ensure_writable(struct sk_buff *skb, int write_len);
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 94435cd09072..5af04be128c1 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -1841,11 +1841,12 @@ bool is_skb_forwardable(const struct net_device *dev, const struct sk_buff *skb)
>  	if (skb->len <= len)
>  		return true;
>  
> -	/* if TSO is enabled, we don't care about the length as the packet
> -	 * could be forwarded without being segmented before
> +	/*
> +	 * if TSO is enabled, we need to check the size of the
> +	 * segmented packets
>  	 */
>  	if (skb_is_gso(skb))
> -		return true;
> +		return skb_gso_validate_len(skb, len);
>  
>  	return false;
>  }
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 01e8285aea73..aefe049e4b0c 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -4945,6 +4945,40 @@ bool skb_gso_validate_mtu(const struct sk_buff *skb, unsigned int mtu)
>  }
>  EXPORT_SYMBOL_GPL(skb_gso_validate_mtu);
>  
> +/**
> + * skb_gso_validate_len - Will a split GSO skb fit in a given length?
> + *
> + * @skb: GSO skb
> + * @len: length to validate against
> + *
> + * skb_gso_validate_len validates if a given skb will fit a wanted
> + * length once split, including L2, L3 and L4 headers.
> + *
> + * Similar to skb_gso_validate_mtu, but inclusive of L2 headers.
> + */
> +bool skb_gso_validate_len(const struct sk_buff *skb, unsigned int len)
> +{
> +	const struct skb_shared_info *shinfo = skb_shinfo(skb);
> +	const struct sk_buff *iter;
> +	unsigned int hlen;
> +
> +	hlen = skb_gso_mac_seglen(skb);
> +
> +	if (shinfo->gso_size != GSO_BY_FRAGS)
> +		return hlen <= len;
> +
> +	/* Undo this so we can re-use header sizes */
> +	hlen -= GSO_BY_FRAGS;
> +
> +	skb_walk_frags(skb, iter) {
> +		if (hlen + skb_headlen(iter) > len)
> +			return false;
> +	}
> +
> +	return true;
> +}
> +EXPORT_SYMBOL_GPL(skb_gso_validate_len);
> +
>  static struct sk_buff *skb_reorder_vlan_header(struct sk_buff *skb)
>  {
>  	if (skb_cow(skb, skb_headroom(skb)) < 0) {
> -- 
> 2.14.1
>