netdev - [PATCH net-next] ipv6: use ipv6_dup_options() from ip6_append

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]

Date:	Thu, 16 May 2013 17:27:32 -0700
From:	Eric Dumazet <eric.dumazet@...il.com>
To:	David Miller <davem@...emloft.net>
Cc:	Herbert Xu <herbert@...dor.apana.org.au>,
	netdev <netdev@...r.kernel.org>,
	Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>,
	Neal Cardwell <ncardwell@...gle.com>
Subject: [PATCH net-next] ipv6: use ipv6_dup_options() from ip6_append_data()

On Thu, 2013-05-16 at 15:23 -0700, Eric Dumazet wrote:
> Hi Herbert
> 
> Looking at the code added in commit 0178b695fd6b40a62a215cb
> ("ipv6: Copy cork options in ip6_append_data") it looks like we can have
> either a memleak or corruption (later in ip6_cork_release()) in case one
> of the sub-allocation (ip6_opt_dup()/ip6_rthdr_dup()) fails.
> 
> I would at least use a kzalloc() instead of kmalloc() in 
> 
> np->cork.opt = kmalloc(opt->tot_len, sk->sk_allocation);
> 
> Or maybe better, reuse the code in  ipv6_dup_options() so that we
> perform a single memory allocation ?

Something like following maybe ?

[PATCH net-next] ipv6: use ipv6_dup_options() from ip6_append_data()

commit 0178b695fd6b4 ("ipv6: Copy cork options in ip6_append_data")
added some code duplication and bad error recovery, leading to potential
crash.

Allow ipv6_dup_options() to be called with a NULL socket argument
so that we can reuse it.

Signed-off-by: Eric Dumazet <edumazet@...gle.com>
Cc: Herbert Xu <herbert@...dor.apana.org.au>
Cc: Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>
Cc: Neal Cardwell <ncardwell@...gle.com>
---
Only compile-tested, I would appreciate a review from Herbert and/or
Hideaki

 net/ipv6/exthdrs.c    |    6 +++++-
 net/ipv6/ip6_output.c |   38 ++++----------------------------------
 2 files changed, 9 insertions(+), 35 deletions(-)

diff --git a/net/ipv6/exthdrs.c b/net/ipv6/exthdrs.c
index 07a7d65..905ec23 100644
--- a/net/ipv6/exthdrs.c
+++ b/net/ipv6/exthdrs.c
@@ -721,7 +721,11 @@ ipv6_dup_options(struct sock *sk, struct ipv6_txoptions *opt)
 {
 	struct ipv6_txoptions *opt2;
 
-	opt2 = sock_kmalloc(sk, opt->tot_len, GFP_ATOMIC);
+	if (sk)
+		opt2 = sock_kmalloc(sk, opt->tot_len, GFP_ATOMIC);
+	else
+		opt2 = kmalloc(opt->tot_len, GFP_ATOMIC);
+
 	if (opt2) {
 		long dif = (char *)opt2 - (char *)opt;
 		memcpy(opt2, opt, opt->tot_len);
diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index d2eedf1..fd44b9c 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -1147,32 +1147,8 @@ int ip6_append_data(struct sock *sk, int getfrag(void *from, char *to,
 			if (WARN_ON(np->cork.opt))
 				return -EINVAL;
 
-			np->cork.opt = kmalloc(opt->tot_len, sk->sk_allocation);
-			if (unlikely(np->cork.opt == NULL))
-				return -ENOBUFS;
-
-			np->cork.opt->tot_len = opt->tot_len;
-			np->cork.opt->opt_flen = opt->opt_flen;
-			np->cork.opt->opt_nflen = opt->opt_nflen;
-
-			np->cork.opt->dst0opt = ip6_opt_dup(opt->dst0opt,
-							    sk->sk_allocation);
-			if (opt->dst0opt && !np->cork.opt->dst0opt)
-				return -ENOBUFS;
-
-			np->cork.opt->dst1opt = ip6_opt_dup(opt->dst1opt,
-							    sk->sk_allocation);
-			if (opt->dst1opt && !np->cork.opt->dst1opt)
-				return -ENOBUFS;
-
-			np->cork.opt->hopopt = ip6_opt_dup(opt->hopopt,
-							   sk->sk_allocation);
-			if (opt->hopopt && !np->cork.opt->hopopt)
-				return -ENOBUFS;
-
-			np->cork.opt->srcrt = ip6_rthdr_dup(opt->srcrt,
-							    sk->sk_allocation);
-			if (opt->srcrt && !np->cork.opt->srcrt)
+			np->cork.opt = ipv6_dup_options(NULL, opt);
+			if (unlikely(!np->cork.opt))
 				return -ENOBUFS;
 
 			/* need source address above miyazawa*/
@@ -1463,14 +1439,8 @@ EXPORT_SYMBOL_GPL(ip6_append_data);
 
 static void ip6_cork_release(struct inet_sock *inet, struct ipv6_pinfo *np)
 {
-	if (np->cork.opt) {
-		kfree(np->cork.opt->dst0opt);
-		kfree(np->cork.opt->dst1opt);
-		kfree(np->cork.opt->hopopt);
-		kfree(np->cork.opt->srcrt);
-		kfree(np->cork.opt);
-		np->cork.opt = NULL;
-	}
+	kfree(np->cork.opt);
+	np->cork.opt = NULL;
 
 	if (inet->cork.base.dst) {
 		dst_release(inet->cork.base.dst);


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