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]

Message-Id: <52A75645020000780010C01B@nat28.tlf.novell.com>
Date:	Tue, 10 Dec 2013 16:58:29 +0000
From:	"Jan Beulich" <JBeulich@...e.com>
To:	"Paul Durrant" <Paul.Durrant@...rix.com>
Cc:	"David Vrabel" <david.vrabel@...rix.com>,
	"Ian Campbell" <Ian.Campbell@...rix.com>,
	"Wei Liu" <wei.liu2@...rix.com>,
	"Zoltan Kiss" <zoltan.kiss@...rix.com>,
	"David Miller" <davem@...emloft.net>,
	"xen-devel" <xen-devel@...ts.xenproject.org>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: RE: [Xen-devel] [PATCH net v5] xen-netback: fix fragment
 detection in checksum setup

>>> On 10.12.13 at 17:24, Paul Durrant <Paul.Durrant@...rix.com> wrote:
>>  -----Original Message-----
>> From: Jan Beulich [mailto:JBeulich@...e.com]
>> Sent: 10 December 2013 16:12
>> To: Paul Durrant
>> Cc: David Vrabel; Ian Campbell; Wei Liu; Zoltan Kiss; David Miller; 
> xen-devel;
>> netdev@...r.kernel.org 
>> Subject: Re: [Xen-devel] [PATCH net v5] xen-netback: fix fragment detection
>> in checksum setup
>> 
>> >>> On 03.12.13 at 18:39, Paul Durrant <paul.durrant@...rix.com> wrote:
>> >  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;
>> > -	int err = -EPROTO;
>> > +	bool fragment;
>> > +	int err;
>> > +
>> > +	fragment = false;
>> > +
>> > +	err = maybe_pull_tail(skb,
>> > +			      sizeof(struct iphdr),
>> > +			      MAX_IP_HDR_LEN);
>> > +	if (err < 0)
>> > +		goto out;
>> >
>> > -	off = sizeof(struct iphdr);
>> > +	if (ip_hdr(skb)->frag_off & htons(IP_OFFSET | IP_MF))
>> > +		fragment = true;
>> 
>> You don't seem to be using "fragment" anywhere.
>> 
>> >
>> > -	header_size = skb->network_header + off + MAX_IPOPTLEN;
>> > -	maybe_pull_tail(skb, header_size);
>> > +	off = ip_hdrlen(skb);
>> >
>> > -	off = iph->ihl * 4;
>> > +	err = -EPROTO;
>> >
>> > -	switch (iph->protocol) {
>> > +	switch (ip_hdr(skb)->protocol) {
>> >  	case IPPROTO_TCP:
>> >  		if (!skb_partial_csum_set(skb, off,
>> >  					  offsetof(struct tcphdr, check)))
>> >  			goto out;
>> >
>> >  		if (recalculate_partial_csum) {
>> > -			struct tcphdr *tcph = tcp_hdr(skb);
>> > -
>> > -			header_size = skb->network_header +
>> > -				off +
>> > -				sizeof(struct tcphdr);
>> > -			maybe_pull_tail(skb, header_size);
>> > -
>> > -			tcph->check = ~csum_tcpudp_magic(iph->saddr, iph-
>> >daddr,
>> > -							 skb->len - off,
>> > -							 IPPROTO_TCP, 0);
>> > +			err = maybe_pull_tail(skb,
>> > +					      off + sizeof(struct tcphdr),
>> > +					      MAX_IP_HDR_LEN);
>> 
>> Is it really necessary/worthwhile to specify MAX_IP_HDR_LEN
>> here? Other than in the IPv6 case you're not risking to need
>> another pull if you simply used off + sizeof(struct tcphdr) instead.
>> 
> 
> Yes, I guess that's true but if we decide to pull up at all then is it 
> harmful to pull more than we absolutely need?

_If_ we manage to pull anything here, it means we weren't able to
pull up to the max anyway, so it seems a little odd to try again.

Another question: Don't the skb_partial_csum_set() calls require
the respective pulls to have happened already?

Jan

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