netdev - Re: [PATCH stable-3.2 stable-3.12] net: fix checksum check in skb_copy_and_csum_datagram

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: <20151228153836.GA28529@unicorn.suse.cz>
Date:	Mon, 28 Dec 2015 16:38:36 +0100
From:	Michal Kubecek <mkubecek@...e.cz>
To:	Sabrina Dubroca <sd@...asysnail.net>
Cc:	Eric Dumazet <eric.dumazet@...il.com>, stable@...r.kernel.org,
	Jiri Slaby <jslaby@...e.cz>,
	Ben Hutchings <ben@...adent.org.uk>, netdev@...r.kernel.org
Subject: Re: [PATCH stable-3.2 stable-3.12] net: fix checksum check in
 skb_copy_and_csum_datagram_iovec()

On Mon, Dec 28, 2015 at 03:29:42PM +0100, Sabrina Dubroca wrote:
> 2015-12-28, 15:01:57 +0100, Michal Kubecek wrote:
> > Recent fix "net: add length argument to
> > skb_copy_and_csum_datagram_iovec" added to some pre-3.19 stable
> > branches, namely
> > 
> >   stable-3.2.y: commit 127500d724f8
> >   stable-3.12.y: commit 3e1ac3aafbd0
> > 
> > doesn't handle truncated reads correctly. If read length is shorter than
> > incoming datagram (but non-zero) and first segment of target iovec is
> > sufficient for read length, skb_copy_and_csum_datagram() is used to copy
> > checksum the data while copying it. For truncated reads this means only
> > the copied part is checksummed (rather than the whole datagram) so that
> > the check almost always fails.
> 
> I just ran into this issue too, sorry I didn't notice it earlier :(
> 
> > Add checksum of the remaining part so that the proper checksum of the
> > whole datagram is computed and checked. Special care must be taken if
> > the copied length is odd.
> > 
> > For zero read length, we don't have to copy anything but we still should
> > check the checksum so that a peek doesn't return with a datagram which
> > is invalid and wouldn't be returned by an actual read.
> > 
> > Signed-off-by: Michal Kubecek <mkubecek@...e.cz>
> > ---
> >  net/core/datagram.c | 26 +++++++++++++++++++++-----
> >  1 file changed, 21 insertions(+), 5 deletions(-)
> > 
> > diff --git a/net/core/datagram.c b/net/core/datagram.c
> > index f22f120771ef..af4bf368257c 100644
> > --- a/net/core/datagram.c
> > +++ b/net/core/datagram.c
> > @@ -809,13 +809,14 @@ int skb_copy_and_csum_datagram_iovec(struct sk_buff *skb,
> >  				     int hlen, struct iovec *iov, int len)
> >  {
> >  	__wsum csum;
> > -	int chunk = skb->len - hlen;
> > +	int full_chunk = skb->len - hlen;
> > +	int chunk = min_t(int, full_chunk, len);
> >  
> > -	if (chunk > len)
> > -		chunk = len;
> > -
> > -	if (!chunk)
> > +	if (!chunk) {
> > +		if (__skb_checksum_complete(skb))
> > +			goto csum_error;
> >  		return 0;
> > +	}
> >  
> >  	/* Skip filled elements.
> >  	 * Pretty silly, look at memcpy_toiovec, though 8)
> > @@ -833,6 +834,21 @@ int skb_copy_and_csum_datagram_iovec(struct sk_buff *skb,
> >  		if (skb_copy_and_csum_datagram(skb, hlen, iov->iov_base,
> >  					       chunk, &csum))
> >  			goto fault;
> > +		if (full_chunk > chunk) {
> > +			if (chunk % 2) {
> > +				__be16 odd = 0;
> > +
> > +				if (skb_copy_bits(skb, hlen + chunk,
> > +						  (char *)&odd + 1, 1))
> > +					goto fault;
> > +				csum = add32_with_carry(odd, csum);
> > +				csum = skb_checksum(skb, hlen + chunk + 1,
> > +						    full_chunk - chunk - 1,
> > +						    csum);
> > +			} else
> > +				csum = skb_checksum(skb, hlen + chunk,
> > +						    full_chunk - chunk, csum);
> > +		}
> >  		if (csum_fold(csum))
> >  			goto csum_error;
> >  		if (unlikely(skb->ip_summed == CHECKSUM_COMPLETE))
> > -- 
> > 2.6.4
> 
> 
> This adds quite a bit of complexity.

I'm not really happy about it either. :-( Most of the complexity comes
from the corner case when one 16-bit word is divided between the copied
part and the rest - but I can't see a nicer way to handle it.

There is another option: in the case of truncated read, we could simply
take the first branch where copying is separated from checksumming. This
would be less efficient, obviously, but I must admit I have no idea how
much.

> I am considering a revert of my
> buggy patch, and use what Eric Dumazet suggested instead:
> 
> https://patchwork.ozlabs.org/patch/543562/
> 
> What do you think?

I believe that would work. I have a little bad feeling about such
solution as it would keep the function broken and just rely on not
hitting it in the case when it matters. But it worked that way for quite
some time so it's probably safe.

                                                          Michal Kubecek

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