netdev - [PATCH ethtool v2 12/13] ethtool: fix leak of memory after realloc

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: <20221208011122.2343363-13-jesse.brandeburg@intel.com>
Date:   Wed,  7 Dec 2022 17:11:21 -0800
From:   Jesse Brandeburg <jesse.brandeburg@...el.com>
To:     mkubecek@...e.cz
Cc:     netdev@...r.kernel.org,
        Jesse Brandeburg <jesse.brandeburg@...el.com>
Subject: [PATCH ethtool v2 12/13] ethtool: fix leak of memory after realloc

cppcheck finds:
netlink/msgbuff.c:63:2: error: Memory leak: nbuff [memleak]
 return 0;
 ^

This is a pretty common problem with realloc() and just requires handling
the return code correctly which makes us refactor to reuse the structure
free/reinit code that already exists in msgbuf_done().

This fixes the code flow by doing the right thing if realloc() succeeds and
if it fails then being sure to free the original memory and replicate the
steps the original code took.

Signed-off-by: Jesse Brandeburg <jesse.brandeburg@...el.com>
---
 netlink/msgbuff.c | 39 ++++++++++++++++++---------------------
 1 file changed, 18 insertions(+), 21 deletions(-)

diff --git a/netlink/msgbuff.c b/netlink/msgbuff.c
index 216f5b946236..a599cab06014 100644
--- a/netlink/msgbuff.c
+++ b/netlink/msgbuff.c
@@ -15,6 +15,20 @@
 
 #define MAX_MSG_SIZE (4 << 20)		/* 4 MB */
 
+/**
+ * msg_done() - destroy a message buffer
+ * @msgbuff: message buffer
+ *
+ * Free the buffer and reset size and remaining size.
+ */
+void msgbuff_done(struct nl_msg_buff *msgbuff)
+{
+	free(msgbuff->buff);
+	msgbuff->buff = NULL;
+	msgbuff->size = 0;
+	msgbuff->left = 0;
+}
+
 /**
  * msgbuff_realloc() - reallocate buffer if needed
  * @msgbuff:  message buffer
@@ -43,19 +57,16 @@ int msgbuff_realloc(struct nl_msg_buff *msgbuff, unsigned int new_size)
 	if (new_size > MAX_MSG_SIZE)
 		return -EMSGSIZE;
 	nbuff = realloc(msgbuff->buff, new_size);
-	if (!nbuff) {
-		msgbuff->buff = NULL;
-		msgbuff->size = 0;
-		msgbuff->left = 0;
-		return -ENOMEM;
-	}
-	if (nbuff != msgbuff->buff) {
+	if (nbuff) {
 		if (new_size > old_size)
 			memset(nbuff + old_size, '\0', new_size - old_size);
 		msgbuff->nlhdr = (struct nlmsghdr *)(nbuff + nlhdr_off);
 		msgbuff->genlhdr = (struct genlmsghdr *)(nbuff + genlhdr_off);
 		msgbuff->payload = nbuff + payload_off;
 		msgbuff->buff = nbuff;
+	} else {
+		msgbuff_done(msgbuff);
+		return -ENOMEM;
 	}
 	msgbuff->size = new_size;
 	msgbuff->left += (new_size - old_size);
@@ -240,17 +251,3 @@ void msgbuff_init(struct nl_msg_buff *msgbuff)
 {
 	memset(msgbuff, '\0', sizeof(*msgbuff));
 }
-
-/**
- * msg_done() - destroy a message buffer
- * @msgbuff: message buffer
- *
- * Free the buffer and reset size and remaining size.
- */
-void msgbuff_done(struct nl_msg_buff *msgbuff)
-{
-	free(msgbuff->buff);
-	msgbuff->buff = NULL;
-	msgbuff->size = 0;
-	msgbuff->left = 0;
-}
-- 
2.31.1