netdev - Re: [patch net-next 3/3] fix skb_morph to preserve skb->sk and skb->destructor pointers

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: <1383660372.4291.134.camel@edumazet-glaptop2.roam.corp.google.com>
Date:	Tue, 05 Nov 2013 06:06:12 -0800
From:	Eric Dumazet <eric.dumazet@...il.com>
To:	Jiri Pirko <jiri@...nulli.us>
Cc:	netdev@...r.kernel.org, davem@...emloft.net, pablo@...filter.org,
	netfilter-devel@...r.kernel.org, yoshfuji@...ux-ipv6.org,
	kadlec@...ckhole.kfki.hu, kaber@...sh.net, mleitner@...hat.com,
	kuznet@....inr.ac.ru, jmorris@...ei.org, wensong@...ux-vs.org,
	horms@...ge.net.au, ja@....bg, edumazet@...gle.com,
	pshelar@...ira.com, jasowang@...hat.com,
	alexander.h.duyck@...el.com, coreteam@...filter.org, fw@...len.de
Subject: Re: [patch net-next 3/3] fix skb_morph to preserve skb->sk and
 skb->destructor pointers

On Tue, 2013-11-05 at 12:02 +0100, Jiri Pirko wrote:
> Currently __skb_clone sets skb->sk and skb->destructor to NULL. This is
> not right for skb_morph use case because skb->sk may be previously
> set (e. g. by xt_TPROXY).
> 
> Also, during skb_morph the destructor should not be called. It might be
> previously set, e. g. by xt_TPROXY to sock_edemux, and that would cause
> put sk while skb is still in flight.

truesize alert. You should add some documentation that skb_morph()
must not be used in transmit path.

Its not clear to be how this can happen.

skb_morph() being used only from ipv4 defrag (or ipv6 reassembly).

Maybe the problem could be fixed by doing this defrag _before_ setting
skb->sk ?

Also, I would prefer you find a way to put all this logic inside
skb_morph() instead of adding such complexity in this already complex
code, I fear the compiler will generate slower code with your patch on
fast path.

Maybe something as simple as following (untested) patch ?

Note that the truesize concern might need some care.

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 3735fad5616e..afabfd6ef341 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -793,18 +793,28 @@ static struct sk_buff *__skb_clone(struct sk_buff *n, struct sk_buff *skb)
 
 /**
  *	skb_morph	-	morph one skb into another
- *	@dst: the skb to receive the contents
+ *	@skb: the skb to receive the contents
  *	@src: the skb to supply the contents
  *
  *	This is identical to skb_clone except that the target skb is
- *	supplied by the user.
+ *	supplied by the user, and that we keep target skb destructor in place,
+ *	meaning this can not be used in transmit path, as skb->truesize might
+ *	change.
  *
  *	The target skb is returned upon exit.
  */
-struct sk_buff *skb_morph(struct sk_buff *dst, struct sk_buff *src)
+struct sk_buff *skb_morph(struct sk_buff *skb, struct sk_buff *src)
 {
-	skb_release_all(dst);
-	return __skb_clone(dst, src);
+	struct sock *save_sk = skb->sk;
+	void (*save_destructor)(struct sk_buff *) = skb->destructor;
+
+	skb->sk = NULL;
+	skb->destructor = NULL;
+	skb_release_all(skb);
+	__skb_clone(skb, src);
+	skb->sk = save_sk;
+	skb->destructor = save_destructor;
+	return skb;
 }
 EXPORT_SYMBOL_GPL(skb_morph);
 


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