[PATCH 1/2] ipc: freebsd: avoid leaking memory in kernel_get_device()

[PATCH 1/2] ipc: freebsd: avoid leaking memory in kernel_get_device()

[kevans]

From: Kyle Evans <kevans@FreeBSD.org>

Primarily, front-load validation of an allowed-ip entry to before we
allocate `aip`, so that we don't need to free() it if we end up skipping
this entry.  Assert that `aip` is NULL after we exit the loop, as we
should have transfered ownership to the `peer` or freed it in all paths
through the allowed-ip loop.

FreeBSD-Coverity:	1500405
Signed-off-by: Kyle Evans <kevans@FreeBSD.org>
---
 src/ipc-freebsd.h | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/src/ipc-freebsd.h b/src/ipc-freebsd.h
index b5be15b..78064e9 100644
--- a/src/ipc-freebsd.h
+++ b/src/ipc-freebsd.h
@@ -8,6 +8,8 @@
 #include <sys/sockio.h>
 #include <dev/wg/if_wg.h>
 
+#include <assert.h>
+
 #define IPC_SUPPORTS_KERNEL_INTERFACE
 
 static int get_dgram_socket(void)
@@ -118,7 +120,7 @@ static int kernel_get_device(struct wgdevice **device, const char *ifname)
 		goto skip_peers;
 	for (i = 0; i < peer_count; ++i) {
 		struct wgpeer *peer;
-		struct wgallowedip *aip;
+		struct wgallowedip *aip = NULL;
 		const nvlist_t *const *nvl_aips;
 		size_t aip_count, j;
 
@@ -169,11 +171,13 @@ static int kernel_get_device(struct wgdevice **device, const char *ifname)
 		if (!aip_count || !nvl_aips)
 			goto skip_allowed_ips;
 		for (j = 0; j < aip_count; ++j) {
+			if (!nvlist_exists_number(nvl_aips[j], "cidr"))
+				continue;
+			if (!nvlist_exists_binary(nvl_aips[j], "ipv4") && !nvlist_exists_binary(nvl_aips[j], "ipv6"))
+				continue;
 			aip = calloc(1, sizeof(*aip));
 			if (!aip)
 				goto err_allowed_ips;
-			if (!nvlist_exists_number(nvl_aips[j], "cidr"))
-				continue;
 			number = nvlist_get_number(nvl_aips[j], "cidr");
 			if (nvlist_exists_binary(nvl_aips[j], "ipv4")) {
 				binary = nvlist_get_binary(nvl_aips[j], "ipv4", &size);
@@ -184,7 +188,8 @@ static int kernel_get_device(struct wgdevice **device, const char *ifname)
 				aip->family = AF_INET;
 				aip->cidr = number;
 				memcpy(&aip->ip4, binary, sizeof(aip->ip4));
-			} else if (nvlist_exists_binary(nvl_aips[j], "ipv6")) {
+			} else {
+				assert(nvlist_exists_binary(nvl_aips[j], "ipv6"));
 				binary = nvlist_get_binary(nvl_aips[j], "ipv6", &size);
 				if (!binary || number > 128) {
 					ret = EINVAL;
@@ -193,14 +198,14 @@ static int kernel_get_device(struct wgdevice **device, const char *ifname)
 				aip->family = AF_INET6;
 				aip->cidr = number;
 				memcpy(&aip->ip6, binary, sizeof(aip->ip6));
-			} else
-				continue;
+			}
 
 			if (!peer->first_allowedip)
 				peer->first_allowedip = aip;
 			else
 				peer->last_allowedip->next_allowedip = aip;
 			peer->last_allowedip = aip;
+			aip = NULL;
 			continue;
 
 		err_allowed_ips:
@@ -209,6 +214,12 @@ static int kernel_get_device(struct wgdevice **device, const char *ifname)
 			free(aip);
 			goto err_peer;
 		}
+
+		/*
+		 * Nothing leaked, hopefully -- ownership transferred or aip
+		 * freed.
+		 */
+		assert(aip == NULL);
 	skip_allowed_ips:
 		if (!dev->first_peer)
 			dev->first_peer = peer;
-- 
2.36.1

[PATCH 2/2] ipc: freebsd: NULL out some freed memory in kernel_set_device()

[kevans]

From: Kyle Evans <kevans@FreeBSD.org>

The `err` path in kernel_set_device() will attempt to free() allocated
nvl_peers, but these two cases meant we could end up attempting a use
after free or a double free, as we rely on nvlist_destroy(NULL) being
a NOP as well as free(NULL).

FreeBSD-Coverity:	1500421
Signed-off-by: Kyle Evans <kevans@FreeBSD.org>
---
 src/ipc-freebsd.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/ipc-freebsd.h b/src/ipc-freebsd.h
index 78064e9..f9d3550 100644
--- a/src/ipc-freebsd.h
+++ b/src/ipc-freebsd.h
@@ -333,6 +333,7 @@ static int kernel_set_device(struct wgdevice *dev)
 			nvlist_destroy(nvl_aips[j]);
 		free(nvl_aips);
 		nvlist_destroy(nvl_peers[i]);
+		nvl_peers[i] = NULL;
 		goto err;
 	}
 	if (i) {
@@ -340,9 +341,11 @@ static int kernel_set_device(struct wgdevice *dev)
 		for (i = 0; i < peer_count; ++i)
 			nvlist_destroy(nvl_peers[i]);
 		free(nvl_peers);
+		nvl_peers = NULL;
 	}
 	wgd.wgd_data = nvlist_pack(nvl_device, &wgd.wgd_size);
 	nvlist_destroy(nvl_device);
+	nvl_device = NULL;
 	if (!wgd.wgd_data)
 		goto err;
 	s = get_dgram_socket();
-- 
2.36.1