src: rework batching logic to fix possible use of uninitialized pages

This patch reworks the batching logic in several aspects:

1) New batch pages are now always added into the batch page list in
   first place. Then, in the send path, if the last batch page is
   empty, it is removed from the batch list.

2) nft_batch_page_add() is only called if the current batch page is
   full. Therefore, it is guaranteed to find a valid netlink message
   in the batch page when moving the tail that didn't fit into a new
   batch page.

3) The batch paging is initialized and released from the nft_netlink()
   path.

4) No more global struct mnl_nlmsg_batch *batch that points to the
   current batch page. Instead, it is retrieved from the tail of the
   batch list, which indicates the current batch page.

This patch fixes a crash due to access of uninitialized memory area in
due to calling batch_page_add() with an empty batch in the send path,
and the memleak of the batch page contents. Reported in:

http://patchwork.ozlabs.org/patch/367085/
http://patchwork.ozlabs.org/patch/367774/

The patch is larger, but this saves the zeroing of the batch page area.

Reported-by: Yanchuan Nian <ycnian@gmail.com>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

3 files changed, 60 insertions, 49 deletions

diff --git a/src/main.c b/src/main.c
index 30ea2c6c..bd8feee5 100644
--- a/src/main.c
+++ b/src/main.c
@@ -166,13 +166,15 @@ static const struct input_descriptor indesc_cmdline = {
 static int nft_netlink(struct parser_state *state, struct list_head *msgs)
 {
 	struct netlink_ctx ctx;
-	struct cmd *cmd, *next;
+	struct cmd *cmd;
 	struct mnl_err *err, *tmp;
 	LIST_HEAD(err_list);
 	uint32_t batch_seqnum;
 	bool batch_supported = netlink_batch_supported();
 	int ret = 0;
 
+	mnl_batch_init();
+
 	batch_seqnum = mnl_batch_begin();
 	list_for_each_entry(cmd, &state->cmds, list) {
 		memset(&ctx, 0, sizeof(ctx));
@@ -182,16 +184,14 @@ static int nft_netlink(struct parser_state *state, struct list_head *msgs)
 		init_list_head(&ctx.list);
 		ret = do_command(&ctx, cmd);
 		if (ret < 0)
-			return ret;
+			goto out;
 	}
 	mnl_batch_end();
 
-	if (mnl_batch_ready())
-		ret = netlink_batch_send(&err_list);
-	else {
-		mnl_batch_reset();
+	if (!mnl_batch_ready())
 		goto out;
-	}
+
+	ret = netlink_batch_send(&err_list);
 
 	list_for_each_entry_safe(err, tmp, &err_list, head) {
 		list_for_each_entry(cmd, &state->cmds, list) {
@@ -208,16 +208,13 @@ static int nft_netlink(struct parser_state *state, struct list_head *msgs)
 		}
 	}
 out:
-	list_for_each_entry_safe(cmd, next, &state->cmds, list) {
-		list_del(&cmd->list);
-		cmd_free(cmd);
-	}
-
+	mnl_batch_reset();
 	return ret;
 }
 
 int nft_run(void *scanner, struct parser_state *state, struct list_head *msgs)
 {
+	struct cmd *cmd, *next;
 	int ret;
 
 	ret = nft_parse(scanner, state);
@@ -230,6 +227,11 @@ retry:
 		goto retry;
 	}
 
+	list_for_each_entry_safe(cmd, next, &state->cmds, list) {
+		list_del(&cmd->list);
+		cmd_free(cmd);
+	}
+
 	return ret;
 }
 
diff --git a/src/mnl.c b/src/mnl.c
index e4f03397..42b11f5b 100644
--- a/src/mnl.c
+++ b/src/mnl.c
@@ -82,8 +82,6 @@ nft_mnl_talk(struct mnl_socket *nf_sock, const void *data, unsigned int len,
  */
 #define BATCH_PAGE_SIZE getpagesize() * 32
 
-static struct mnl_nlmsg_batch *batch;
-
 static struct mnl_nlmsg_batch *mnl_batch_alloc(void)
 {
 	static char *buf;
@@ -93,11 +91,6 @@ static struct mnl_nlmsg_batch *mnl_batch_alloc(void)
 	return mnl_nlmsg_batch_start(buf, BATCH_PAGE_SIZE);
 }
 
-void mnl_batch_init(void)
-{
-	batch = mnl_batch_alloc();
-}
-
 static LIST_HEAD(batch_page_list);
 static int batch_num_pages;
 
@@ -106,35 +99,53 @@ struct batch_page {
 	struct mnl_nlmsg_batch *batch;
 };
 
+void mnl_batch_init(void)
+{
+	struct batch_page *batch_page;
+
+	batch_page = xmalloc(sizeof(struct batch_page));
+	batch_page->batch = mnl_batch_alloc();
+	batch_num_pages++;
+	list_add_tail(&batch_page->head, &batch_page_list);
+}
+
+static struct batch_page *nft_batch_page_current(void)
+{
+	return list_entry(batch_page_list.prev, struct batch_page, head);
+}
+
 static void *nft_nlmsg_batch_current(void)
 {
-	return mnl_nlmsg_batch_current(batch);
+	return mnl_nlmsg_batch_current(nft_batch_page_current()->batch);
 }
 
-static void mnl_batch_page_add(void)
+static void nft_batch_page_add(void)
 {
-	struct batch_page *batch_page;
 	struct nlmsghdr *last_nlh;
 
 	/* Get the last message not fitting in the batch */
 	last_nlh = nft_nlmsg_batch_current();
-
-	batch_page = xmalloc(sizeof(struct batch_page));
-	batch_page->batch = batch;
-	list_add_tail(&batch_page->head, &batch_page_list);
-	batch_num_pages++;
-	batch = mnl_batch_alloc();
-
+	/* Add new batch page */
+	mnl_batch_init();
 	/* Copy the last message not fitting to the new batch page */
 	memcpy(nft_nlmsg_batch_current(), last_nlh, last_nlh->nlmsg_len);
 	/* No overflow may happen as this is a new empty batch page */
-	mnl_nlmsg_batch_next(batch);
+	mnl_nlmsg_batch_next(nft_batch_page_current()->batch);
+}
+
+static void nft_batch_page_release(struct batch_page *batch_page)
+{
+	list_del(&batch_page->head);
+	xfree(mnl_nlmsg_batch_head(batch_page->batch));
+	mnl_nlmsg_batch_stop(batch_page->batch);
+	xfree(batch_page);
+	batch_num_pages--;
 }
 
 static void nft_batch_continue(void)
 {
-	if (!mnl_nlmsg_batch_next(batch))
-		mnl_batch_page_add();
+	if (!mnl_nlmsg_batch_next(nft_batch_page_current()->batch))
+		nft_batch_page_add();
 }
 
 static uint32_t mnl_batch_put(int type)
@@ -171,12 +182,16 @@ bool mnl_batch_ready(void)
 	/* Check if the batch only contains the initial and trailing batch
 	 * messages. In that case, the batch is empty.
 	 */
-	return mnl_nlmsg_batch_size(batch) != (NLMSG_HDRLEN+sizeof(struct nfgenmsg)) * 2;
+	return mnl_nlmsg_batch_size(nft_batch_page_current()->batch) !=
+	       (NLMSG_HDRLEN+sizeof(struct nfgenmsg)) * 2;
 }
 
 void mnl_batch_reset(void)
 {
-	mnl_nlmsg_batch_reset(batch);
+	struct batch_page *batch_page, *next;
+
+	list_for_each_entry_safe(batch_page, next, &batch_page_list, head)
+		nft_batch_page_release(batch_page);
 }
 
 static void mnl_err_list_node_add(struct list_head *err_list, int error,
@@ -226,12 +241,12 @@ static ssize_t mnl_nft_socket_sendmsg(const struct mnl_socket *nl)
 		.msg_iov	= iov,
 		.msg_iovlen	= batch_num_pages,
 	};
-	struct batch_page *batch_page, *next;
+	struct batch_page *batch_page;
 	int i = 0;
 
 	mnl_set_sndbuffer(nl);
 
-	list_for_each_entry_safe(batch_page, next, &batch_page_list, head) {
+	list_for_each_entry(batch_page, &batch_page_list, head) {
 		iov[i].iov_base = mnl_nlmsg_batch_head(batch_page->batch);
 		iov[i].iov_len = mnl_nlmsg_batch_size(batch_page->batch);
 		i++;
@@ -243,10 +258,6 @@ static ssize_t mnl_nft_socket_sendmsg(const struct mnl_socket *nl)
 					  sizeof(struct nfgenmsg));
 		}
 #endif
-		list_del(&batch_page->head);
-		xfree(batch_page->batch);
-		xfree(batch_page);
-		batch_num_pages--;
 	}
 
 	return sendmsg(mnl_socket_get_fd(nl), &msg, 0);
@@ -262,12 +273,13 @@ int mnl_batch_talk(struct mnl_socket *nl, struct list_head *err_list)
 		.tv_usec	= 0
 	};
 
-	if (!mnl_nlmsg_batch_is_empty(batch))
-		mnl_batch_page_add();
+	/* Remove last page from the batch if it's empty */
+	if (mnl_nlmsg_batch_is_empty(nft_batch_page_current()->batch))
+		nft_batch_page_release(nft_batch_page_current());
 
 	ret = mnl_nft_socket_sendmsg(nl);
 	if (ret == -1)
-		goto err;
+		return -1;
 
 	FD_ZERO(&readfds);
 	FD_SET(fd, &readfds);
@@ -275,14 +287,14 @@ int mnl_batch_talk(struct mnl_socket *nl, struct list_head *err_list)
 	/* receive and digest all the acknowledgments from the kernel. */
 	ret = select(fd+1, &readfds, NULL, NULL, &tv);
 	if (ret == -1)
-		goto err;
+		return -1;
 
 	while (ret > 0 && FD_ISSET(fd, &readfds)) {
 		struct nlmsghdr *nlh = (struct nlmsghdr *)rcv_buf;
 
 		ret = mnl_socket_recvfrom(nl, rcv_buf, sizeof(rcv_buf));
 		if (ret == -1)
-			goto err;
+			return -1;
 
 		ret = mnl_cb_run(rcv_buf, ret, 0, portid, NULL, NULL);
 		/* Continue on error, make sure we get all acknowledgments */
@@ -291,13 +303,11 @@ int mnl_batch_talk(struct mnl_socket *nl, struct list_head *err_list)
 
 		ret = select(fd+1, &readfds, NULL, NULL, &tv);
 		if (ret == -1)
-			goto err;
+			return -1;
 
 		FD_ZERO(&readfds);
 		FD_SET(fd, &readfds);
 	}
-err:
-	mnl_nlmsg_batch_reset(batch);
 	return ret;
 }
 
diff --git a/src/netlink.c b/src/netlink.c
index 05fae103..e1492152 100644
--- a/src/netlink.c
+++ b/src/netlink.c
@@ -59,7 +59,6 @@ static void __init netlink_open_sock(void)
 {
 	nf_sock = nfsock_open();
 	fcntl(mnl_socket_get_fd(nf_sock), F_SETFL, O_NONBLOCK);
-	mnl_batch_init();
 }
 
 static void __exit netlink_close_sock(void)