[CVE-2014-0131 3/3] skbuff: skb_segment: orphan frags before copying

[Stefan Bader]

On 13.06.2014 13:12, Luis Henriques wrote:
> From: "Michael S. Tsirkin" <mst at redhat.com>
> 
> skb_segment copies frags around, so we need
> to copy them carefully to avoid accessing
> user memory after reporting completion to userspace
> through a callback.
> 
> skb_segment doesn't normally happen on datapath:
> TSO needs to be disabled - so disabling zero copy
> in this case does not look like a big deal.
> 
> Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
> Acked-by: Herbert Xu <herbert at gondor.apana.org.au>
> Signed-off-by: David S. Miller <davem at davemloft.net>
> (back ported from commit 1fd819ecb90cc9b822cd84d3056ddba315d3340f)
> CVE-2014-0131
> BugLink: http://bugs.launchpad.net/bugs/1298119
> Signed-off-by: Luis Henriques <luis.henriques at canonical.com>
> ---
>  net/core/skbuff.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 66e03b1dec72..99a65b4e4adc 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -2739,6 +2739,9 @@ struct sk_buff *skb_segment(struct sk_buff *skb, netdev_features_t features)
>  						 skb_put(nskb, hsize), hsize);
>  
>  		while (pos < offset + len && i < nfrags) {
> +			if (unlikely(skb_orphan_frags(skb, GFP_ATOMIC)))
> +				goto err;
> +
>  			*frag = skb_shinfo(skb)->frags[i];
>  			__skb_frag_ref(frag);
>  			size = skb_frag_size(frag);
> 

I somehow cannot stop feeling slightly uneasy about this backport. Not that I
can claim to really understand it. I know Ben said (in his initial version)
something about skb_segment only supporting page-frags *or* a page-list.

Just looking at the new code it looks to be possible to move from the initial
skb to traversing the frag-list. And that seems to be possible with the code
in precise, too. Just that the iterator there is named fskb. And nskb is cloned
from fskb in that case (and fskb moves to the next element). Ok, on the other
hand, when fskb is not the same as the sk-frag-list the loop will not get
executed to the point of calling skb_orphan_frags...

                if (fskb != skb_shinfo(skb)->frag_list)
                        continue;

That seems not to be done anymore (in current code). So more hard to feel sure.
My understanding is that it should be required to call orphan frags for both,
the initial skb and those skbs that get used from the frag list. But again, I
might be totally misled there.

Generally I have some feeling that even if things are wrong, it will "only" fail
to orphan frags and that I suppose was not a oopsing kind of failure. So
things still working may be an unreliable indicator. :/


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 901 bytes
Desc: OpenPGP digital signature
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20140613/7e386a0f/attachment.sig>