netdev - Re: [Xen-devel] [PATCH net v5] 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]

Date:	Thu, 05 Dec 2013 18:00:38 +0800
From:	annie li <annie.li@...cle.com>
To:	Paul Durrant <Paul.Durrant@...rix.com>
CC:	Wei Liu <wei.liu2@...rix.com>,
	Ian Campbell <Ian.Campbell@...rix.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"xen-devel@...ts.xen.org" <xen-devel@...ts.xen.org>,
	David Vrabel <david.vrabel@...rix.com>,
	Zoltan Kiss <zoltan.kiss@...rix.com>,
	David Miller <davem@...emloft.net>
Subject: Re: [Xen-devel] [PATCH net v5] xen-netback: fix fragment detection
 in checksum setup


On 2013/12/5 17:08, Paul Durrant wrote:
>> -----Original Message-----
>> From: annie li [mailto:annie.li@...cle.com]
>> Sent: 05 December 2013 06:28
>> To: Paul Durrant
>> Cc: xen-devel@...ts.xen.org; netdev@...r.kernel.org; Zoltan Kiss; Wei Liu;
>> Ian Campbell; David Vrabel; David Miller
>> Subject: Re: [PATCH net v5] xen-netback: fix fragment detection in checksum
>> setup
>>
>>
>> On 2013/12/4 1:39, Paul Durrant wrote:
>>
>>> +/* This value should be large enough to cover a tagged ethernet header
>> plus
>>> + * an IPv6 header, all options, and a maximal TCP or UDP header.
>>> + */
>>> +#define MAX_IPV6_HDR_LEN 256
>>> +
>>> +#define OPT_HDR(type, skb, off) \
>>> +	(type *)(skb_network_header(skb) + (off))
>>> +
>>>    static int checksum_setup_ipv6(struct xenvif *vif, struct sk_buff *skb,
>>>    			       int recalculate_partial_csum)
>>>    {
>>> -	int err = -EPROTO;
>>> -	struct ipv6hdr *ipv6h = (void *)skb->data;
>>> +	int err;
>>>    	u8 nexthdr;
>>> -	unsigned int header_size;
>>>    	unsigned int off;
>>> +	unsigned int len;
>>>    	bool fragment;
>>>    	bool done;
>>>
>>> +	fragment = false;
>>>    	done = false;
>>>
>>>    	off = sizeof(struct ipv6hdr);
>>>
>>> -	header_size = skb->network_header + off;
>>> -	maybe_pull_tail(skb, header_size);
>>> +	err = maybe_pull_tail(skb, off, MAX_IPV6_HDR_LEN);
>>> +	if (err < 0)
>>> +		goto out;
>>>
>>> -	nexthdr = ipv6h->nexthdr;
>>> +	nexthdr = ipv6_hdr(skb)->nexthdr;
>>>
>>> -	while ((off <= sizeof(struct ipv6hdr) + ntohs(ipv6h->payload_len))
>> &&
>>> -	       !done) {
>>> +	len = sizeof(struct ipv6hdr) + ntohs(ipv6_hdr(skb)->payload_len);
>>> +	while (off <= len && !done) {
>>>    		switch (nexthdr) {
>>>    		case IPPROTO_DSTOPTS:
>>>    		case IPPROTO_HOPOPTS:
>>>    		case IPPROTO_ROUTING: {
>>> -			struct ipv6_opt_hdr *hp = (void *)(skb->data + off);
>>> +			struct ipv6_opt_hdr *hp;
>>>
>>> -			header_size = skb->network_header +
>>> -				off +
>>> -				sizeof(struct ipv6_opt_hdr);
>>> -			maybe_pull_tail(skb, header_size);
>>> +			err = maybe_pull_tail(skb,
>>> +					      off +
>>> +					      sizeof(struct ipv6_opt_hdr),
>>> +					      MAX_IPV6_HDR_LEN);
>>> +			if (err < 0)
>>> +				goto out;
>>>
>>> +			hp = OPT_HDR(struct ipv6_opt_hdr, skb, off);
>>>    			nexthdr = hp->nexthdr;
>>>    			off += ipv6_optlen(hp);
>>>    			break;
>>>    		}
>>>    		case IPPROTO_AH: {
>>> -			struct ip_auth_hdr *hp = (void *)(skb->data + off);
>>> +			struct ip_auth_hdr *hp;
>>> +
>>> +			err = maybe_pull_tail(skb,
>>> +					      off +
>>> +					      sizeof(struct ip_auth_hdr),
>>> +					      MAX_IPV6_HDR_LEN);
>>> +			if (err < 0)
>>> +				goto out;
>>> +
>>> +			hp = OPT_HDR(struct ip_auth_hdr, skb, off);
>>> +			nexthdr = hp->nexthdr;
>>> +			off += ipv6_authlen(hp);
>>> +			break;
>>> +		}
>>> +		case IPPROTO_FRAGMENT: {
>>> +			struct frag_hdr *hp;
>>>
>>> -			header_size = skb->network_header +
>>> -				off +
>>> -				sizeof(struct ip_auth_hdr);
>>> -			maybe_pull_tail(skb, header_size);
>>> +			err = maybe_pull_tail(skb,
>>> +					      off +
>>> +					      sizeof(struct frag_hdr),
>>> +					      MAX_IPV6_HDR_LEN);
>>> +			if (err < 0)
>>> +				goto out;
>>> +
>>> +			hp = OPT_HDR(struct frag_hdr, skb, off);
>>> +
>>> +			if (hp->frag_off & htons(IP6_OFFSET | IP6_MF))
>>> +				fragment = true;
>>>
>>>    			nexthdr = hp->nexthdr;
>>> -			off += (hp->hdrlen+2)<<2;
>>> +			off += sizeof(struct frag_hdr);
>>>    			break;
>>>    		}
>>> -		case IPPROTO_FRAGMENT:
>>> -			fragment = true;
>>> -			/* fall through */
>> About IPv6 extension headers, should "Encapsulating Security Payload" be
>> processed too?(See rfc2460 and rfc2406) Is there any concern to ignore
>> it here?
>>
> It's deliberately ignored. If we have an ESP header then the TCP/UDP header and payload will be encrypted so you can't do checksum offload, hence this function should return an error.
>

Got it. But from current code, if ESP extension header exists, switch 
(nexthdr) will go into default case directly, and done is set with true, 
then the code quits switch and while loop. After that, it will go into 
next switch and then the default case since the nexthdr is ipv6 ESP 
extension header, not TCP/UDP header. Then netdev_err probably prints 
out improper log.

Thanks
Annie
--
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