[PATCH ethtool 0/2] netlink: data lifetime error fixes

[PATCH ethtool 0/2] netlink: data lifetime error fixes

[Michal Kubecek]

Fixes of two data lifetime bugs found by testing with valgrind: one use
after free, one memory leak.

Michal Kubecek (2):
  netlink: fix use after free in netlink_run_handler()
  netlink: fix leaked instances of struct nl_socket

 netlink/netlink.c | 21 +++++++++++++++------
 netlink/nlsock.c  |  3 +++
 2 files changed, 18 insertions(+), 6 deletions(-)

-- 
2.29.2

[PATCH ethtool 1/2] netlink: fix use after free in netlink_run_handler()

[Michal Kubecek]

Valgrind detected use after free in netlink_run_handler(): some members of
struct nl_context are accessed after the netlink context is freed by
netlink_done(). Use local variables to store the two flags and check them
instead.

Fixes: 6c19c0d559c8 ("netlink: use genetlink ops information to decide about fallback")
Signed-off-by: Michal Kubecek <mkubecek@suse.cz>
---
 netlink/netlink.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/netlink/netlink.c b/netlink/netlink.c
index f655f6ea25b7..aaaabdd3048e 100644
--- a/netlink/netlink.c
+++ b/netlink/netlink.c
@@ -457,6 +457,7 @@ void netlink_run_handler(struct cmd_context *ctx, nl_func_t nlfunc,
 			 bool no_fallback)
 {
 	bool wildcard = ctx->devname && !strcmp(ctx->devname, WILDCARD_DEVNAME);
+	bool wildcard_unsupported, ioctl_fallback;
 	struct nl_context *nlctx;
 	const char *reason;
 	int ret;
@@ -478,14 +479,17 @@ void netlink_run_handler(struct cmd_context *ctx, nl_func_t nlfunc,
 	nlctx = ctx->nlctx;
 
 	ret = nlfunc(ctx);
+	wildcard_unsupported = nlctx->wildcard_unsupported;
+	ioctl_fallback = nlctx->ioctl_fallback;
 	netlink_done(ctx);
-	if (no_fallback || ret != -EOPNOTSUPP || !nlctx->ioctl_fallback) {
-		if (nlctx->wildcard_unsupported)
+
+	if (no_fallback || ret != -EOPNOTSUPP || !ioctl_fallback) {
+		if (wildcard_unsupported)
 			fprintf(stderr, "%s\n",
 				"subcommand does not support wildcard dump");
 		exit(ret >= 0 ? ret : 1);
 	}
-	if (nlctx->wildcard_unsupported)
+	if (wildcard_unsupported)
 		reason = "subcommand does not support wildcard dump";
 	else
 		reason = "kernel netlink support for subcommand missing";
-- 
2.29.2

[PATCH ethtool 2/2] netlink: fix leaked instances of struct nl_socket

[Michal Kubecek]

Valgrind detected memory leaks caused by missing cleanup of netlink
context's ethnl_socket, ethnl2_socket and rtnl_socket. Also, contrary to
its description, nlsock_done() does not free struct nl_socket itself.
Fix nlsock_done() to free the structure and use it to dispose of sockets
pointed to by struct nl_context members.

Fixes: 50efb3cdd2bb ("netlink: netlink socket wrapper and helpers")
Fixes: 87307c30724d ("netlink: initialize ethtool netlink socket")
Fixes: 7f3585b22a4b ("netlink: add handler for permaddr (-P)")
Signed-off-by: Michal Kubecek <mkubecek@suse.cz>
---
 netlink/netlink.c | 11 ++++++++---
 netlink/nlsock.c  |  3 +++
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/netlink/netlink.c b/netlink/netlink.c
index aaaabdd3048e..ffe06339f099 100644
--- a/netlink/netlink.c
+++ b/netlink/netlink.c
@@ -435,11 +435,16 @@ out_free:
 
 static void netlink_done(struct cmd_context *ctx)
 {
-	if (!ctx->nlctx)
+	struct nl_context *nlctx = ctx->nlctx;
+
+	if (!nlctx)
 		return;
 
-	free(ctx->nlctx->ops_info);
-	free(ctx->nlctx);
+	nlsock_done(nlctx->ethnl_socket);
+	nlsock_done(nlctx->ethnl2_socket);
+	nlsock_done(nlctx->rtnl_socket);
+	free(nlctx->ops_info);
+	free(nlctx);
 	ctx->nlctx = NULL;
 	cleanup_all_strings();
 }
diff --git a/netlink/nlsock.c b/netlink/nlsock.c
index ef31d8c33b29..0ec2738d81d2 100644
--- a/netlink/nlsock.c
+++ b/netlink/nlsock.c
@@ -395,8 +395,11 @@ out_msgbuff:
  */
 void nlsock_done(struct nl_socket *nlsk)
 {
+	if (!nlsk)
+		return;
 	if (nlsk->sk)
 		mnl_socket_close(nlsk->sk);
 	msgbuff_done(&nlsk->msgbuff);
 	memset(nlsk, '\0', sizeof(*nlsk));
+	free(nlsk);
 }
-- 
2.29.2