netdev - Re: [PATCH] net: fix two possible memory leaks in netif_set_xps

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] [day] [month] [year] [list]

Message-ID: <55144CBF.2060102@redhat.com>
Date:	Thu, 26 Mar 2015 11:15:27 -0700
From:	Alexander Duyck <alexander.h.duyck@...hat.com>
To:	roy.qing.li@...il.com, netdev@...r.kernel.org
Subject: Re: [PATCH] net: fix two possible memory leaks in netif_set_xps_queue

On 03/26/2015 12:40 AM, roy.qing.li@...il.com wrote:
> From: Li RongQing <roy.qing.li@...il.com>
>
> new_dev_maps->cpu_map[cpu] maybe be assigned to the memory from
> expand_xps_map() in the first loop, so should be freed
>
> when free dev_maps, make sure to free new_dev_maps->cpu_map[cpu]
> firstly
>
> Fixes: 537c00de1c [net: Add functions netif_reset_xps_queue and netif_set_xps_queue]
> Cc: Alexander Duyck <alexander.h.duyck@...hat.com>
> Signed-off-by: Li RongQing <roy.qing.li@...il.com>
> ---
>   net/core/dev.c | 11 ++++++++++-
>   1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 5d43e01..aa6459f 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -2025,9 +2025,11 @@ int netif_set_xps_queue(struct net_device *dev, const struct cpumask *mask,
>   		} else if (dev_maps) {
>   			/* fill in the new device map from the old device map */
>   			map = xmap_dereference(dev_maps->cpu_map[cpu]);
> +			new_map = xmap_dereference(new_dev_maps->cpu_map[cpu]);
>   			RCU_INIT_POINTER(new_dev_maps->cpu_map[cpu], map);
> +			if (new_map && map != new_map)
> +				kfree_rcu(new_map, rcu);
>   		}
> -
>   	}
>   

This would cause memory corruption as you are freeing objects that are 
still active in dev->xps_maps, they cannot be freed until the section 
clearly called out as "Cleanup old maps" below it.  That is where this 
memory is freed using the exact same logic you seemed to copy from below.

>   	rcu_assign_pointer(dev->xps_maps, new_dev_maps);
> @@ -2068,6 +2070,13 @@ out_no_new_maps:
>   	/* free map if not active */
>   	if (!active) {
>   		RCU_INIT_POINTER(dev->xps_maps, NULL);
> +
> +		for_each_possible_cpu(cpu) {
> +			map = xmap_dereference(dev_maps->cpu_map[cpu]);
> +			if (map)
> +				kfree_rcu(map, rcu);
> +		}
> +
>   		kfree_rcu(dev_maps, rcu);
>   	}
>   

This is unnecessary.  If there are no active queue sets left then we 
already removed all of the memory in remove_xps_queue.

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