netdev - Re: [PATCHv1 net-next] xen-netback: remove unconditional pull_skb

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: <1415037337.1411.9.camel@citrix.com>
Date:	Mon, 3 Nov 2014 17:55:37 +0000
From:	Ian Campbell <Ian.Campbell@...rix.com>
To:	David Vrabel <david.vrabel@...rix.com>
CC:	<netdev@...r.kernel.org>, <xen-devel@...ts.xenproject.org>,
	Wei Liu <wei.liu2@...rix.com>,
	Malcolm Crossley <malcolm.crossley@...rix.com>
Subject: Re: [PATCHv1 net-next] xen-netback: remove unconditional
 pull_skb_tail in guest Tx path

On Mon, 2014-11-03 at 17:46 +0000, David Vrabel wrote:
> On 03/11/14 17:39, Ian Campbell wrote:
> > On Mon, 2014-11-03 at 17:23 +0000, David Vrabel wrote:
> >> From: Malcolm Crossley <malcolm.crossley@...rix.com>
> >>
> >> Unconditionally pulling 128 bytes into the linear buffer is not
> >> required. Netback has already grant copied up-to 128 bytes from the
> >> first slot of a packet into the linear buffer. The first slot normally
> >> contain all the IPv4/IPv6 and TCP/UDP headers.
> > 
> > What about when it doesn't? It sounds as if we now won't pull up, which
> > would be bad.
> 
> The network stack will always pull any headers it needs to inspect (the
> frag may be a userspace page which has the same security issues as a
> frag with a foreign page).

I don't believe it will, unless something changed since I last looked.
The kernel assumes that it has been sensible enough to put the headers
in the linear area, since it is the one which generates them in most
cases. In other cases its up to the relevant driver to make sure this is
true.

> e.g., see skb_checksum_setup() called slightly later on in netback.

This however is what will make things safe for us (note that this is
only used by xen-net* in practice), it is this which should be mentioned
in the commit message I think.

> > To avoid the pull up the code would need to grant copy up-to 128 bytes
> > from as many slots as needed, not only the first.
> > 
> > Also, if the grant copy has already placed 128 bytes in the linear area,
> > why is the pull up touching anything in the first place? Shouldn't it be
> > a nop in that case?
> 
> The grant copy only copies from the first frag which may be less than
> 128 bytes in length.
> 
> David


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