Re: [PATCH nft 3/3] src: rework batching logic to fix possible use of uninitialized pages

[Yanchuan Nian <ycnian@gmail.com>]

On Tue, Jul 15, 2014 at 05:47:18PM +0200, Pablo Neira Ayuso wrote:
> 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 save the zeroing of the batch page area.

Tested, it works find. Just a little question: is it better to release
all batches but the current after each message sending?

> 
> Reported-by: Yanchuan Nian <ycnian@gmail.com>
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> ---
>  src/main.c    |   14 +++++-----
>  src/mnl.c     |   82 ++++++++++++++++++++++++++++++++-------------------------
>  src/netlink.c |    1 -
>  3 files changed, 54 insertions(+), 43 deletions(-)
> 
> diff --git a/src/main.c b/src/main.c
> index 30ea2c6..552a35d 100644
> --- a/src/main.c
> +++ b/src/main.c
> @@ -173,6 +173,8 @@ static int nft_netlink(struct parser_state *state, struct list_head *msgs)
>  	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,6 +208,8 @@ static int nft_netlink(struct parser_state *state, struct list_head *msgs)
>  		}
>  	}
>  out:
> +	mnl_batch_reset();
> +
>  	list_for_each_entry_safe(cmd, next, &state->cmds, list) {
>  		list_del(&cmd->list);
>  		cmd_free(cmd);
> diff --git a/src/mnl.c b/src/mnl.c
> index e4f0339..42b11f5 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 afad5a4..0176cb4 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)
> -- 
> 1.7.10.4