netdev - Re: [PATCH net v4] xen-netback: fix fragment detection in checksum setup

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: <20131203140243.GF14293@zion.uk.xensource.com>
Date:	Tue, 3 Dec 2013 14:02:44 +0000
From:	Wei Liu <wei.liu2@...rix.com>
To:	Paul Durrant <paul.durrant@...rix.com>
CC:	<xen-devel@...ts.xen.org>, <netdev@...r.kernel.org>,
	Zoltan Kiss <zoltan.kiss@...rix.com>,
	Wei Liu <wei.liu2@...rix.com>,
	Ian Campbell <ian.campbell@...rix.com>,
	David Vrabel <david.vrabel@...rix.com>,
	David Miller <davem@...emloft.net>
Subject: Re: [PATCH net v4] xen-netback: fix fragment detection in checksum
 setup

On Mon, Dec 02, 2013 at 05:35:15PM +0000, Paul Durrant wrote:
> The code to detect fragments in checksum_setup() was missing for IPv4 and
> too eager for IPv6. (It transpires that Windows seems to send IPv6 packets
> with a fragment header even if they are not a fragment - i.e. offset is zero,
> and M bit is not set).
> This patch also incorporates a fix to callers of maybe_pull_tail() where
> skb->network_header was being erroneously added to the length argument.
> 
> Signed-off-by: Paul Durrant <paul.durrant@...rix.com>
> Signed-off-by: Zoltan Kiss <zoltan.kiss@...rix.com>
> Cc: Wei Liu <wei.liu2@...rix.com>
> Cc: Ian Campbell <ian.campbell@...rix.com>
> Cc: David Vrabel <david.vrabel@...rix.com>
> cc: David Miller <davem@...emloft.net>
> ---
> v4
> - Re-work maybe_pull_tail() to have a failure path and update mac and
>   network header pointers on success.
> - Re-work callers of maybe_pull_tail() to handle failures and allow for
>   movement of skb header pointers.
> - Incorporate fixes from patch "Fix pull size in checksum_setup_ip" since
>   it also modifies callers of maybe_pull_tail().
> - Remove error messages in checksum routines that could be triggered by frontends
> 
> v3
> - Use defined values for 'fragment offset' and 'more fragments'
> 
> v2
> - Added comments noting what fragment/offset masks mean
> 
>  drivers/net/xen-netback/netback.c |  215 ++++++++++++++++++++-----------------
>  include/linux/ipv6.h              |    1 +
>  include/net/ipv6.h                |    3 +-
>  3 files changed, 120 insertions(+), 99 deletions(-)
> 
> diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
> index 64f0e0d..e3d7216 100644
> --- a/drivers/net/xen-netback/netback.c
> +++ b/drivers/net/xen-netback/netback.c
> @@ -1149,49 +1149,62 @@ static int xenvif_set_skb_gso(struct xenvif *vif,
>  	return 0;
>  }
>  
> -static inline void maybe_pull_tail(struct sk_buff *skb, unsigned int len)
> +static inline bool maybe_pull_tail(struct sk_buff *skb, unsigned int len,
> +				   unsigned int max)
>  {
> -	if (skb_is_nonlinear(skb) && skb_headlen(skb) < len) {
> -		/* If we need to pullup then pullup to the max, so we
> -		 * won't need to do it again.
> -		 */
> -		int target = min_t(int, skb->len, MAX_TCP_HEADER);
> -		__pskb_pull_tail(skb, target - skb_headlen(skb));
> -	}
> +	if (skb_headlen(skb) >= len)
> +		return true;
> +
> +	/* If we need to pullup then pullup to the max, so we
> +	 * won't need to do it again.
> +	 */
> +	if (max > skb->len)
> +		max = skb->len;
> +
> +	__pskb_pull_tail(skb, max - skb_headlen(skb));
> +
> +	return skb_headlen(skb) >= len;
>  }
>  
> +/* This value should be large enough to cover a tagged ethernet header plus
> + * maximally sized IP and TCP or UDP headers.
> + */
> +#define MAX_IP_HDR_LEN 128
> +
>  static int checksum_setup_ip(struct xenvif *vif, struct sk_buff *skb,
>  			     int recalculate_partial_csum)
>  {
> -	struct iphdr *iph = (void *)skb->data;
> -	unsigned int header_size;
>  	unsigned int off;
> +	bool fragment;
>  	int err = -EPROTO;
>  
> -	off = sizeof(struct iphdr);
> +	fragment = false;
>  
> -	header_size = skb->network_header + off + MAX_IPOPTLEN;
> -	maybe_pull_tail(skb, header_size);
> +	if (!maybe_pull_tail(skb, sizeof(struct iphdr), MAX_IP_HDR_LEN))
> +		goto out;
> +

I think you need to correctly update err to reflect this failure.
Using -EPROTO will wrongly blame frontend while it is backend that's
failing to process the packet.

Wei.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html