[net,v3] xen-netback: fix fragment detection in checksum setup

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).

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: David Vrabel <david.vrabel@citrix.com>
---
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 |   31 ++++++++++++++++++++++++++++---
 include/net/ipv6.h                |    3 ++-
 2 files changed, 30 insertions(+), 4 deletions(-)

From: Paul Durrant <paul.durrant@citrix.com>
Date: Fri, 29 Nov 2013 10:52:08 +0000

> @@ -1166,15 +1166,27 @@ static int checksum_setup_ip(struct xenvif *vif, struct sk_buff *skb,
>  	struct iphdr *iph = (void *)skb->data;
>  	unsigned int header_size;
>  	unsigned int off;
> +	bool fragment;
>  	int err = -EPROTO;
>  
> +	fragment = false;
> +
>  	off = sizeof(struct iphdr);
>  
>  	header_size = skb->network_header + off + MAX_IPOPTLEN;
>  	maybe_pull_tail(skb, header_size);
>  
> +	if (iph->frag_off & htons(IP_OFFSET | IP_MF))
> +		fragment = true;

This function has a serious problem.

maybe_pull_tail() can change skb->data, therefore this "iph" pointer
can become invalid, you're essentially dereferencing garbage if
maybe_pull_tail() actually does any work.

Secondly, do you really (even rate limited) want to span the system
log just because some ipv4 fragmented frames end up here?  That
doesn't make any sense to me.  Maybe bump a statistic or something
like that, but a log message triggerable by a remote entity?  No way.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

> -----Original Message-----
> From: David Miller [mailto:davem@davemloft.net]
> Sent: 30 November 2013 21:14
> To: Paul Durrant
> Cc: xen-devel@lists.xen.org; netdev@vger.kernel.org; Wei Liu; Ian Campbell;
> David Vrabel
> Subject: Re: [PATCH net v3] xen-netback: fix fragment detection in checksum
> setup
> 
> From: Paul Durrant <paul.durrant@citrix.com>
> Date: Fri, 29 Nov 2013 10:52:08 +0000
> 
> > @@ -1166,15 +1166,27 @@ static int checksum_setup_ip(struct xenvif *vif,
> struct sk_buff *skb,
> >  	struct iphdr *iph = (void *)skb->data;
> >  	unsigned int header_size;
> >  	unsigned int off;
> > +	bool fragment;
> >  	int err = -EPROTO;
> >
> > +	fragment = false;
> > +
> >  	off = sizeof(struct iphdr);
> >
> >  	header_size = skb->network_header + off + MAX_IPOPTLEN;
> >  	maybe_pull_tail(skb, header_size);
> >
> > +	if (iph->frag_off & htons(IP_OFFSET | IP_MF))
> > +		fragment = true;
> 
> This function has a serious problem.
> 
> maybe_pull_tail() can change skb->data, therefore this "iph" pointer
> can become invalid, you're essentially dereferencing garbage if
> maybe_pull_tail() actually does any work.

Ok. Clearly I'm misunderstanding what __pskb_pull_tail() does then. I was under the impression that it moved skb->tail on but left skb->data alone (which is what I want to do). I will have another look.

> 
> Secondly, do you really (even rate limited) want to span the system
> log just because some ipv4 fragmented frames end up here?  That
> doesn't make any sense to me.  Maybe bump a statistic or something
> like that, but a log message triggerable by a remote entity?  No way.

Ok.

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

On Mon, 2013-12-02 at 09:45 +0000, Paul Durrant wrote:
> > -----Original Message-----
> > From: David Miller [mailto:davem@davemloft.net]
> > Sent: 30 November 2013 21:14
> > To: Paul Durrant
> > Cc: xen-devel@lists.xen.org; netdev@vger.kernel.org; Wei Liu; Ian Campbell;
> > David Vrabel
> > Subject: Re: [PATCH net v3] xen-netback: fix fragment detection in checksum
> > setup
> > 
> > From: Paul Durrant <paul.durrant@citrix.com>
> > Date: Fri, 29 Nov 2013 10:52:08 +0000
> > 
> > > @@ -1166,15 +1166,27 @@ static int checksum_setup_ip(struct xenvif *vif,
> > struct sk_buff *skb,
> > >  	struct iphdr *iph = (void *)skb->data;
> > >  	unsigned int header_size;
> > >  	unsigned int off;
> > > +	bool fragment;
> > >  	int err = -EPROTO;
> > >
> > > +	fragment = false;
> > > +
> > >  	off = sizeof(struct iphdr);
> > >
> > >  	header_size = skb->network_header + off + MAX_IPOPTLEN;
> > >  	maybe_pull_tail(skb, header_size);
> > >
> > > +	if (iph->frag_off & htons(IP_OFFSET | IP_MF))
> > > +		fragment = true;
> > 
> > This function has a serious problem.
> > 
> > maybe_pull_tail() can change skb->data, therefore this "iph" pointer
> > can become invalid, you're essentially dereferencing garbage if
> > maybe_pull_tail() actually does any work.
> 
> Ok. Clearly I'm misunderstanding what __pskb_pull_tail() does then. I
> was under the impression that it moved skb->tail on but left skb->data
> alone (which is what I want to do). I will have another look.
[...]

A pull does not change the offset of skb->data but it may need to
expand, i.e. reallocate, the skb head area.  The kernel-doc states
clearly that this can happen.

Ben.