[PATCH nf-next 1/2] netfilter: nfnetlink_log: consolidate check for instance in nfulnl_recv_config()

[PATCH nf-next 1/2] netfilter: nfnetlink_log: consolidate check for instance in nfulnl_recv_config()

[Pablo Neira Ayuso]

This patch consolidates the check for valid logger instance once we have
passed the command handling:

The config message that we receive may contain the following info:

1) Command only: We always get a valid instance pointer if we just
   created it. In case that the instance is being destroyed or the
   command is unknown, we jump to exit path of nfulnl_recv_config().
   This patch doesn't modify this handling.

2) Config only: In this case, the instance must always exist since the
   user is asking for configuration updates. If the instance doesn't exist
   this returns -ENODEV.

3) No command and no configs are specified: This case is rare. The
   user is sending us a config message with neither commands nor
   config options. In this case, we have to check if the instance exists
   and bail out otherwise. Before this patch, it was possible to send a
   config message with no command and no config updates for an
   unexisting instance without triggering an error. So this is the only
   case that changes.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
@Ken-ichirou: Could you give a test to this patches? Thanks.

 net/netfilter/nfnetlink_log.c | 28 +++++-----------------------
 1 file changed, 5 insertions(+), 23 deletions(-)

diff --git a/net/netfilter/nfnetlink_log.c b/net/netfilter/nfnetlink_log.c
index f8d9bd8..2002d57 100644
--- a/net/netfilter/nfnetlink_log.c
+++ b/net/netfilter/nfnetlink_log.c
@@ -874,16 +874,15 @@ nfulnl_recv_config(struct sock *ctnl, struct sk_buff *skb,
 			ret = -ENOTSUPP;
 			break;
 		}
+	} else if (!inst) {
+		ret = -ENODEV;
+		goto out;
 	}
 
 	if (nfula[NFULA_CFG_MODE]) {
-		struct nfulnl_msg_config_mode *params;
-		params = nla_data(nfula[NFULA_CFG_MODE]);
+		struct nfulnl_msg_config_mode *params =
+			nla_data(nfula[NFULA_CFG_MODE]);
 
-		if (!inst) {
-			ret = -ENODEV;
-			goto out;
-		}
 		nfulnl_set_mode(inst, params->copy_mode,
 				ntohl(params->copy_range));
 	}
@@ -891,41 +890,24 @@ nfulnl_recv_config(struct sock *ctnl, struct sk_buff *skb,
 	if (nfula[NFULA_CFG_TIMEOUT]) {
 		__be32 timeout = nla_get_be32(nfula[NFULA_CFG_TIMEOUT]);
 
-		if (!inst) {
-			ret = -ENODEV;
-			goto out;
-		}
 		nfulnl_set_timeout(inst, ntohl(timeout));
 	}
 
 	if (nfula[NFULA_CFG_NLBUFSIZ]) {
 		__be32 nlbufsiz = nla_get_be32(nfula[NFULA_CFG_NLBUFSIZ]);
 
-		if (!inst) {
-			ret = -ENODEV;
-			goto out;
-		}
 		nfulnl_set_nlbufsiz(inst, ntohl(nlbufsiz));
 	}
 
 	if (nfula[NFULA_CFG_QTHRESH]) {
 		__be32 qthresh = nla_get_be32(nfula[NFULA_CFG_QTHRESH]);
 
-		if (!inst) {
-			ret = -ENODEV;
-			goto out;
-		}
 		nfulnl_set_qthresh(inst, ntohl(qthresh));
 	}
 
 	if (nfula[NFULA_CFG_FLAGS]) {
 		u16 flags = ntohs(nla_get_be16(nfula[NFULA_CFG_FLAGS]));
 
-		if (!inst) {
-			ret = -ENODEV;
-			goto out;
-		}
-
 		if (flags & NFULNL_CFG_F_CONNTRACK &&
 		    !rcu_access_pointer(nfnl_ct_hook)) {
 #ifdef CONFIG_MODULES
-- 
2.1.4

[PATCH nf-next 2/2] netfilter: nfnetlink_log: validate dependencies to avoid breaking atomicity

[Pablo Neira Ayuso]

Check that dependencies are fulfilled before updating the logger
instance, otherwise we can leave things in intermediate state on errors
in nfulnl_recv_config().

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nfnetlink_log.c | 43 ++++++++++++++++++++++++-------------------
 1 file changed, 24 insertions(+), 19 deletions(-)

diff --git a/net/netfilter/nfnetlink_log.c b/net/netfilter/nfnetlink_log.c
index 2002d57..a5b9680 100644
--- a/net/netfilter/nfnetlink_log.c
+++ b/net/netfilter/nfnetlink_log.c
@@ -825,6 +825,7 @@ nfulnl_recv_config(struct sock *ctnl, struct sk_buff *skb,
 	struct net *net = sock_net(ctnl);
 	struct nfnl_log_net *log = nfnl_log_pernet(net);
 	int ret = 0;
+	u16 flags;
 
 	if (nfula[NFULA_CFG_CMD]) {
 		u_int8_t pf = nfmsg->nfgen_family;
@@ -846,6 +847,28 @@ nfulnl_recv_config(struct sock *ctnl, struct sk_buff *skb,
 		goto out_put;
 	}
 
+	/* Check if we support these flags in first place, dependencies should
+	 * be there too not to break atomicity.
+	 */
+	if (nfula[NFULA_CFG_FLAGS]) {
+		flags = ntohs(nla_get_be16(nfula[NFULA_CFG_FLAGS]));
+
+		if ((flags & NFULNL_CFG_F_CONNTRACK) &&
+		    !rcu_access_pointer(nfnl_ct_hook)) {
+#ifdef CONFIG_MODULES
+			nfnl_unlock(NFNL_SUBSYS_ULOG);
+			request_module("ip_conntrack_netlink");
+			nfnl_lock(NFNL_SUBSYS_ULOG);
+			if (rcu_access_pointer(nfnl_ct_hook)) {
+				ret = -EAGAIN;
+				goto out_put;
+			}
+#endif
+			ret = -EOPNOTSUPP;
+			goto out_put;
+		}
+	}
+
 	if (cmd != NULL) {
 		switch (cmd->command) {
 		case NFULNL_CFG_CMD_BIND:
@@ -905,26 +928,8 @@ nfulnl_recv_config(struct sock *ctnl, struct sk_buff *skb,
 		nfulnl_set_qthresh(inst, ntohl(qthresh));
 	}
 
-	if (nfula[NFULA_CFG_FLAGS]) {
-		u16 flags = ntohs(nla_get_be16(nfula[NFULA_CFG_FLAGS]));
-
-		if (flags & NFULNL_CFG_F_CONNTRACK &&
-		    !rcu_access_pointer(nfnl_ct_hook)) {
-#ifdef CONFIG_MODULES
-			nfnl_unlock(NFNL_SUBSYS_ULOG);
-			request_module("ip_conntrack_netlink");
-			nfnl_lock(NFNL_SUBSYS_ULOG);
-			if (rcu_access_pointer(nfnl_ct_hook)) {
-				ret = -EAGAIN;
-				goto out;
-			}
-#endif
-			ret = -EOPNOTSUPP;
-			goto out;
-		}
-
+	if (nfula[NFULA_CFG_FLAGS])
 		nfulnl_set_flags(inst, flags);
-	}
 
 out_put:
 	instance_put(inst);
-- 
2.1.4

Re: [PATCH nf-next 2/2] netfilter: nfnetlink_log: validate dependencies to avoid breaking atomicity

[Ken-ichirou MATSUZAWA]

[-- Attachment #1: Type: text/plain, Size: 864 bytes --]

 Thank you for your usual support.
 
On Tue, Oct 13, 2015 at 12:47:47PM +0200, Pablo Neira Ayuso wrote:
> @Ken-ichirou: Could you give a test to this patches? Thanks.

I've tested these with appended and is fine, of course.
Before applying patches:

    # ./a.out 123
    a.out: empty_cfg.c:56: main: Assertion `mnl_cb_run(buf, ret, 0, portid, ((void *)0), ((void *)0)) == -1' failed.
    Aborted

On Tue, Oct 13, 2015 at 12:47:48PM +0200, Pablo Neira Ayuso wrote:
> +#ifdef CONFIG_MODULES
> +			nfnl_unlock(NFNL_SUBSYS_ULOG);
> +			request_module("ip_conntrack_netlink");
> +			nfnl_lock(NFNL_SUBSYS_ULOG);
> +			if (rcu_access_pointer(nfnl_ct_hook)) {
> +				ret = -EAGAIN;
> +				goto out_put;
> +			}
> +#endif
> +			ret = -EOPNOTSUPP;
> +			goto out_put;

It's off the subject, but this fixes module unload. It was not
put instance at my first patch, Thanks!

[-- Attachment #2: empty_cfg.c --]
[-- Type: text/x-csrc, Size: 1326 bytes --]

/*
 * This example is placed in the public domain.
 * cc empty_cfg.c -lmnl -lnetfilter_log
 */

#include <stdio.h>
#include <stdlib.h>
#include <arpa/inet.h>
#include <errno.h>
#include <assert.h>

#include <linux/netfilter/nfnetlink_log.h>

#include <libmnl/libmnl.h>
#include <libnetfilter_log/libnetfilter_log.h>

int main(int argc, char *argv[])
{
	struct mnl_socket *nl;
	char buf[MNL_SOCKET_BUFFER_SIZE];
	struct nlmsghdr *nlh;
	int ret;
	unsigned int portid, qnum;

	if (argc != 2) {
		printf("Usage: %s [queue_num]\n", argv[0]);
		exit(EXIT_FAILURE);
	}
	qnum = atoi(argv[1]);

	nl = mnl_socket_open(NETLINK_NETFILTER);
	if (nl == NULL) {
		perror("mnl_socket_open");
		exit(EXIT_FAILURE);
	}

	if (mnl_socket_bind(nl, 0, MNL_SOCKET_AUTOPID) < 0) {
		perror("mnl_socket_bind");
		exit(EXIT_FAILURE);
	}
	portid = mnl_socket_get_portid(nl);

	nlh = nflog_nlmsg_put_header(buf, NFULNL_MSG_CONFIG, AF_UNSPEC, qnum);
	nlh->nlmsg_flags |= NLM_F_ACK;
	if (mnl_socket_sendto(nl, nlh, nlh->nlmsg_len) < 0) {
		perror("mnl_socket_sendto");
		exit(EXIT_FAILURE);
	}

	ret = mnl_socket_recvfrom(nl, buf, sizeof(buf));
	if (ret == -1) {
		perror("mnl_socket_recvfrom");
		exit(EXIT_FAILURE);
	}

	assert(mnl_cb_run(buf, ret, 0, portid, NULL, NULL) == MNL_CB_ERROR);
	assert(errno == ENODEV);

	mnl_socket_close(nl);

	return 0;
}