Re: [PATCH net v2] net: gso: validate transport header before computing segment length

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

Michael S. Tsirkin wrote:
> If a TSO packet is processed in the stack before a transport
> header was ever established, the TCP data offset is read through the
> unset header offset, far outside the packet.
> 
> This was demonstrated by syzkaller, back when the input path did not
> dissect such packets: a virtio net header supplying a malformed
> network header was enough.  Qdisc length estimation has carried a
> guard against exactly this since a0dce8752193 ("net: Skip GSO

please insert commit here and before 44378d02c5aa. I'm surprised
checkpatch did not complain.

> length estimation if transport header is not set"); the GSO length
> validators never got one.
> 
> We used to, but we no longer allow such packets in: since 44378d02c5aa
> ("net: always dissect GSO packets in __virtio_net_hdr_to_skb()") the
> virtio net input path dissects all GSO packets.
> 
> But as a defence in depth measure, let's also check before we read: if
> the TCP header is unset or outside the linear data, drop it.

is the packet indeed dropped?

> 
> Fixes: f43798c27684 ("tun: Allow GSO using virtio_net_hdr")
> Cc: Willem de Bruijn <willemb@google.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> changes from v1:
> - address Willem's comments, in particular, teach skb_gso_network_seglen
>   to return a negative error, and propagate through callers.
> 
>  include/linux/skbuff.h |  5 +++
>  net/core/gso.c         | 82 ++++++++++++++++++++++++++++++++++++------
>  2 files changed, 77 insertions(+), 10 deletions(-)
> 
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index a3ff380d43f0..aa5bc608de06 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -3041,6 +3041,11 @@ static inline bool skb_inner_network_header_was_set(const struct sk_buff *skb)
>  	return skb->inner_network_header > 0;
>  }
>  
> +static inline bool skb_inner_transport_header_was_set(const struct sk_buff *skb)
> +{
> +	return skb->inner_transport_header > 0;
> +}

It may be possible that skb_reset_inner_headers gets called when
skb->transport_header is ~0U. For safety, also check ~0U here?

>  static inline unsigned char *skb_inner_mac_header(const struct sk_buff *skb)
>  {
>  	return skb->head + skb->inner_mac_header;
> diff --git a/net/core/gso.c b/net/core/gso.c
> index e96ef6350064..874e4c8b1f1b 100644
> --- a/net/core/gso.c
> +++ b/net/core/gso.c
> @@ -131,6 +131,35 @@ struct sk_buff *__skb_gso_segment(struct sk_buff *skb,
>  }
>  EXPORT_SYMBOL(__skb_gso_segment);
>  
> +/*
> + * TSO processing (but not UDP/SCTP GSO, which uses fixed header
> + * sizes) reads the TCP header as specified by
> + * skb->transport_header/inner_transport_header.  Make sure it is
> + * there to read.
> + */
> +static bool skb_gso_transport_header_ok(const struct sk_buff *skb)
> +{
> +	const struct skb_shared_info *shinfo = skb_shinfo(skb);
> +	int thoff;
> +
> +	if (unlikely(!(shinfo->gso_type & (SKB_GSO_TCPV4 | SKB_GSO_TCPV6))))
> +		return true;

maybe skb_tso_transport_header_ok given this limitation
> +
> +	if (skb->encapsulation) {
> +		if (!skb_inner_transport_header_was_set(skb))
> +			return false;
> +
> +		thoff = skb_inner_transport_offset(skb);

skb_gso_transport_seglen reads both transport_offset and
inner_transport_offset, so if skb->encapsulation we probably should
not check only inner, but both?

> +	} else {
> +		if (!skb_transport_header_was_set(skb))
> +			return false;
> +
> +		thoff = skb_transport_offset(skb);
> +	}
> +
> +	return thoff >= 0 && thoff + sizeof(struct tcphdr) <= skb_headlen(skb);
> +}
> +
>  /**
>   * skb_gso_transport_seglen - Return length of individual segments of a gso packet
>   *
> @@ -140,12 +169,18 @@ EXPORT_SYMBOL(__skb_gso_segment);
>   * individual segments, including Layer4 headers (TCP/UDP).
>   *
>   * The MAC/L2 or network (IP, IPv6) headers are not accounted for.
> + *
> + * Return: the length, or a negative errno if the TCP header is not
> + * there to read.
>   */
> -static unsigned int skb_gso_transport_seglen(const struct sk_buff *skb)
> +static int skb_gso_transport_seglen(const struct sk_buff *skb)
>  {
>  	const struct skb_shared_info *shinfo = skb_shinfo(skb);
>  	unsigned int thlen = 0;
>  
> +	if (unlikely(!skb_gso_transport_header_ok(skb)))
> +		return -EINVAL;
> +
>  	if (skb->encapsulation) {
>  		thlen = skb_inner_transport_header(skb) -
>  			skb_transport_header(skb);
> @@ -175,13 +210,21 @@ static unsigned int skb_gso_transport_seglen(const struct sk_buff *skb)
>   * individual segments, including Layer3 (IP, IPv6) and L4 headers (TCP/UDP).
>   *
>   * The MAC/L2 header is not accounted for.
> + *
> + * Return: the length, or a negative errno if the TCP header is not
> + * there to read.
>   */
> -static unsigned int skb_gso_network_seglen(const struct sk_buff *skb)
> +static int skb_gso_network_seglen(const struct sk_buff *skb)
>  {
> -	unsigned int hdr_len = skb_transport_header(skb) -
> -			       skb_network_header(skb);
> +	unsigned int hdr_len;
> +	int tlen = skb_gso_transport_seglen(skb);

nit: reverse xmas tree

>  
> -	return hdr_len + skb_gso_transport_seglen(skb);
> +	if (tlen < 0)
> +		return tlen;
> +
> +	hdr_len = skb_transport_header(skb) - skb_network_header(skb);
> +
> +	return hdr_len + tlen;
>  }
>  
>  /**
> @@ -192,12 +235,21 @@ static unsigned int skb_gso_network_seglen(const struct sk_buff *skb)
>   * skb_gso_mac_seglen is used to determine the real size of the
>   * individual segments, including MAC/L2, Layer3 (IP, IPv6) and L4
>   * headers (TCP/UDP).
> + *
> + * Return: the length, or a negative errno if the TCP header is not
> + * there to read.
>   */
> -static unsigned int skb_gso_mac_seglen(const struct sk_buff *skb)
> +static int skb_gso_mac_seglen(const struct sk_buff *skb)
>  {
> -	unsigned int hdr_len = skb_transport_header(skb) - skb_mac_header(skb);
> +	unsigned int hdr_len;
> +	int tlen = skb_gso_transport_seglen(skb);
>  
> -	return hdr_len + skb_gso_transport_seglen(skb);
> +	if (tlen < 0)
> +		return tlen;
> +
> +	hdr_len = skb_transport_header(skb) - skb_mac_header(skb);
> +
> +	return hdr_len + tlen;
>  }
>  
>  /**
> @@ -253,7 +305,12 @@ static inline bool skb_gso_size_check(const struct sk_buff *skb,
>   */
>  bool skb_gso_validate_network_len(const struct sk_buff *skb, unsigned int mtu)
>  {
> -	return skb_gso_size_check(skb, skb_gso_network_seglen(skb), mtu);
> +	int seg_len = skb_gso_network_seglen(skb);
> +
> +	if (seg_len < 0)
> +		return false;
> +
> +	return skb_gso_size_check(skb, seg_len, mtu);
>  }
>  EXPORT_SYMBOL_GPL(skb_gso_validate_network_len);
>  
> @@ -268,7 +325,12 @@ EXPORT_SYMBOL_GPL(skb_gso_validate_network_len);
>   */
>  bool skb_gso_validate_mac_len(const struct sk_buff *skb, unsigned int len)
>  {
> -	return skb_gso_size_check(skb, skb_gso_mac_seglen(skb), len);
> +	int seg_len = skb_gso_mac_seglen(skb);
> +
> +	if (seg_len < 0)
> +		return false;
> +
> +	return skb_gso_size_check(skb, seg_len, len);
>  }
>  EXPORT_SYMBOL_GPL(skb_gso_validate_mac_len);
>  
> -- 
> MST
>