net: Relax xmalloc-ing (and fix NULL deref)

Submitted by Pavel Emelianov on June 16, 2017, 3:19 p.m.

Details

Message ID d3e425a3-a6d7-1cef-1866-c26c4f1a3c00@virtuozzo.com
State Accepted
Series "net: Relax xmalloc-ing (and fix NULL deref)"
Headers show

Commit Message

Pavel Emelianov June 16, 2017, 3:19 p.m.
There's potential NULL-derefernece in dump_netns_con() -- two xmalloc
results are not checked. However, since there's a huge set of these
xmallocs, I propose to relax the whole thing with one big xmalloc and
xptr_pull() helper.

Signed-off-by: Pavel Emelyanov <xemul@virtuozzo.com>
---
 criu/net.c | 55 +++++++++++++++++++++----------------------------------
 1 file changed, 21 insertions(+), 34 deletions(-)

Patch hide | download patch | download mbox

diff --git a/criu/net.c b/criu/net.c
index 6849cd2..90aa947 100644
--- a/criu/net.c
+++ b/criu/net.c
@@ -1564,6 +1564,7 @@  static inline int dump_iptables(struct cr_imgset *fds)
 
 static int dump_netns_conf(struct ns_id *ns, struct cr_imgset *fds)
 {
+	void *buf, *o_buf;
 	int ret = -1;
 	int i;
 	NetnsEntry netns = NETNS_ENTRY__INIT;
@@ -1580,8 +1581,16 @@  static int dump_netns_conf(struct ns_id *ns, struct cr_imgset *fds)
 	list_for_each_entry(p, &ns->net.ids, node)
 		i++;
 
-	netns.nsids = xmalloc(sizeof(NetnsId *) * i);
-	ids = xmalloc(sizeof(NetnsId) * i);
+	o_buf = buf = xmalloc(
+			i * (sizeof(NetnsId*) + sizeof(NetnsId)) +
+			size4 * (sizeof(SysctlEntry*) + sizeof(SysctlEntry)) * 2 +
+			size6 * (sizeof(SysctlEntry*) + sizeof(SysctlEntry)) * 2
+		     );
+	if (!buf)
+		goto out;
+
+	netns.nsids = xptr_pull_s(&buf, i * sizeof(NetnsId*));
+	ids = xptr_pull_s(&buf, i * sizeof(NetnsId));
 	i = 0;
 	list_for_each_entry(p, &ns->net.ids, node) {
 		netns_id__init(&ids[i]);
@@ -1594,18 +1603,10 @@  static int dump_netns_conf(struct ns_id *ns, struct cr_imgset *fds)
 
 	netns.n_def_conf4 = size4;
 	netns.n_all_conf4 = size4;
-	netns.def_conf4 = xmalloc(sizeof(SysctlEntry *) * size4);
-	if (!netns.def_conf4)
-		goto err_free;
-	netns.all_conf4 = xmalloc(sizeof(SysctlEntry *) * size4);
-	if (!netns.all_conf4)
-		goto err_free;
-	def_confs4 = xmalloc(sizeof(SysctlEntry) * size4);
-	if (!def_confs4)
-		goto err_free;
-	all_confs4 = xmalloc(sizeof(SysctlEntry) * size4);
-	if (!all_confs4)
-		goto err_free;
+	netns.def_conf4 = xptr_pull_s(&buf, size4 * sizeof(SysctlEntry*));
+	netns.all_conf4 = xptr_pull_s(&buf, size4 * sizeof(SysctlEntry*));
+	def_confs4 = xptr_pull_s(&buf, size4 * sizeof(SysctlEntry));
+	all_confs4 = xptr_pull_s(&buf, size4 * sizeof(SysctlEntry));
 
 	for (i = 0; i < size4; i++) {
 		sysctl_entry__init(&def_confs4[i]);
@@ -1618,18 +1619,10 @@  static int dump_netns_conf(struct ns_id *ns, struct cr_imgset *fds)
 
 	netns.n_def_conf6 = size6;
 	netns.n_all_conf6 = size6;
-	netns.def_conf6 = xmalloc(sizeof(SysctlEntry *) * size6);
-	if (!netns.def_conf6)
-		goto err_free;
-	netns.all_conf6 = xmalloc(sizeof(SysctlEntry *) * size6);
-	if (!netns.all_conf6)
-		goto err_free;
-	def_confs6 = xmalloc(sizeof(SysctlEntry) * size6);
-	if (!def_confs6)
-		goto err_free;
-	all_confs6 = xmalloc(sizeof(SysctlEntry) * size6);
-	if (!all_confs6)
-		goto err_free;
+	netns.def_conf6 = xptr_pull_s(&buf, size6 * sizeof(SysctlEntry*));
+	netns.all_conf6 = xptr_pull_s(&buf, size6 * sizeof(SysctlEntry*));
+	def_confs6 = xptr_pull_s(&buf, size6 * sizeof(SysctlEntry));
+	all_confs6 = xptr_pull_s(&buf, size6 * sizeof(SysctlEntry));
 
 	for (i = 0; i < size6; i++) {
 		sysctl_entry__init(&def_confs6[i]);
@@ -1663,14 +1656,8 @@  static int dump_netns_conf(struct ns_id *ns, struct cr_imgset *fds)
 
 	ret = pb_write_one(img_from_set(fds, CR_FD_NETNS), &netns, PB_NETNS);
 err_free:
-	xfree(netns.def_conf4);
-	xfree(netns.all_conf4);
-	xfree(def_confs4);
-	xfree(all_confs4);
-	xfree(netns.def_conf6);
-	xfree(netns.all_conf6);
-	xfree(def_confs6);
-	xfree(all_confs6);
+	xfree(o_buf);
+out:
 	return ret;
 }
 

Comments

Andrey Vagin June 21, 2017, 9:22 p.m.
Applied, thanks!

On Fri, Jun 16, 2017 at 06:19:04PM +0300, Pavel Emelyanov wrote:
> There's potential NULL-derefernece in dump_netns_con() -- two xmalloc
> results are not checked. However, since there's a huge set of these
> xmallocs, I propose to relax the whole thing with one big xmalloc and
> xptr_pull() helper.
> 
> Signed-off-by: Pavel Emelyanov <xemul@virtuozzo.com>
> ---
>  criu/net.c | 55 +++++++++++++++++++++----------------------------------
>  1 file changed, 21 insertions(+), 34 deletions(-)
> 
> diff --git a/criu/net.c b/criu/net.c
> index 6849cd2..90aa947 100644
> --- a/criu/net.c
> +++ b/criu/net.c
> @@ -1564,6 +1564,7 @@ static inline int dump_iptables(struct cr_imgset *fds)
>  
>  static int dump_netns_conf(struct ns_id *ns, struct cr_imgset *fds)
>  {
> +	void *buf, *o_buf;
>  	int ret = -1;
>  	int i;
>  	NetnsEntry netns = NETNS_ENTRY__INIT;
> @@ -1580,8 +1581,16 @@ static int dump_netns_conf(struct ns_id *ns, struct cr_imgset *fds)
>  	list_for_each_entry(p, &ns->net.ids, node)
>  		i++;
>  
> -	netns.nsids = xmalloc(sizeof(NetnsId *) * i);
> -	ids = xmalloc(sizeof(NetnsId) * i);
> +	o_buf = buf = xmalloc(
> +			i * (sizeof(NetnsId*) + sizeof(NetnsId)) +
> +			size4 * (sizeof(SysctlEntry*) + sizeof(SysctlEntry)) * 2 +
> +			size6 * (sizeof(SysctlEntry*) + sizeof(SysctlEntry)) * 2
> +		     );
> +	if (!buf)
> +		goto out;
> +
> +	netns.nsids = xptr_pull_s(&buf, i * sizeof(NetnsId*));
> +	ids = xptr_pull_s(&buf, i * sizeof(NetnsId));
>  	i = 0;
>  	list_for_each_entry(p, &ns->net.ids, node) {
>  		netns_id__init(&ids[i]);
> @@ -1594,18 +1603,10 @@ static int dump_netns_conf(struct ns_id *ns, struct cr_imgset *fds)
>  
>  	netns.n_def_conf4 = size4;
>  	netns.n_all_conf4 = size4;
> -	netns.def_conf4 = xmalloc(sizeof(SysctlEntry *) * size4);
> -	if (!netns.def_conf4)
> -		goto err_free;
> -	netns.all_conf4 = xmalloc(sizeof(SysctlEntry *) * size4);
> -	if (!netns.all_conf4)
> -		goto err_free;
> -	def_confs4 = xmalloc(sizeof(SysctlEntry) * size4);
> -	if (!def_confs4)
> -		goto err_free;
> -	all_confs4 = xmalloc(sizeof(SysctlEntry) * size4);
> -	if (!all_confs4)
> -		goto err_free;
> +	netns.def_conf4 = xptr_pull_s(&buf, size4 * sizeof(SysctlEntry*));
> +	netns.all_conf4 = xptr_pull_s(&buf, size4 * sizeof(SysctlEntry*));
> +	def_confs4 = xptr_pull_s(&buf, size4 * sizeof(SysctlEntry));
> +	all_confs4 = xptr_pull_s(&buf, size4 * sizeof(SysctlEntry));
>  
>  	for (i = 0; i < size4; i++) {
>  		sysctl_entry__init(&def_confs4[i]);
> @@ -1618,18 +1619,10 @@ static int dump_netns_conf(struct ns_id *ns, struct cr_imgset *fds)
>  
>  	netns.n_def_conf6 = size6;
>  	netns.n_all_conf6 = size6;
> -	netns.def_conf6 = xmalloc(sizeof(SysctlEntry *) * size6);
> -	if (!netns.def_conf6)
> -		goto err_free;
> -	netns.all_conf6 = xmalloc(sizeof(SysctlEntry *) * size6);
> -	if (!netns.all_conf6)
> -		goto err_free;
> -	def_confs6 = xmalloc(sizeof(SysctlEntry) * size6);
> -	if (!def_confs6)
> -		goto err_free;
> -	all_confs6 = xmalloc(sizeof(SysctlEntry) * size6);
> -	if (!all_confs6)
> -		goto err_free;
> +	netns.def_conf6 = xptr_pull_s(&buf, size6 * sizeof(SysctlEntry*));
> +	netns.all_conf6 = xptr_pull_s(&buf, size6 * sizeof(SysctlEntry*));
> +	def_confs6 = xptr_pull_s(&buf, size6 * sizeof(SysctlEntry));
> +	all_confs6 = xptr_pull_s(&buf, size6 * sizeof(SysctlEntry));
>  
>  	for (i = 0; i < size6; i++) {
>  		sysctl_entry__init(&def_confs6[i]);
> @@ -1663,14 +1656,8 @@ static int dump_netns_conf(struct ns_id *ns, struct cr_imgset *fds)
>  
>  	ret = pb_write_one(img_from_set(fds, CR_FD_NETNS), &netns, PB_NETNS);
>  err_free:
> -	xfree(netns.def_conf4);
> -	xfree(netns.all_conf4);
> -	xfree(def_confs4);
> -	xfree(all_confs4);
> -	xfree(netns.def_conf6);
> -	xfree(netns.all_conf6);
> -	xfree(def_confs6);
> -	xfree(all_confs6);
> +	xfree(o_buf);
> +out:
>  	return ret;
>  }
>  
> -- 
> 2.1.4
> _______________________________________________
> CRIU mailing list
> CRIU@openvz.org
> https://lists.openvz.org/mailman/listinfo/criu