[PATCH 0/3] netfilter: nf_log: nlmsg size fixes

[PATCH 0/3] netfilter: nf_log: nlmsg size fixes

[Florian Westphal]

The following three patches attempt to cure the reported
nlmsg skb trailroom exhaustion bug.

First two patches add missing size accounting, third patch
relaxes the impact of out-of-tailroom by tossing the skb
and giving a WARN_ONCE trace with size information, so we can
investigate further if needed.

Florian Westphal (2):
      netfilter: nf_log: account for size of NLMSG_DONE attribute in skb size
      netfilter: nfnetlink_log: fix maximum packet length logged to userspace

Houcheng Lin (1):
      netfilter: nf_log: release skbuff on nlmsg put failure

 net/netfilter/nfnetlink_log.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

[PATCH 1/3] netfilter: nf_log: account for size of NLMSG_DONE attribute

[Florian Westphal]

We currently neither account for the nlattr size, nor do we consider
the size of the trailing NLMSG_DONE when allocating nlmsg skb.

This can result in nflog to stop working, as __nfulnl_send() re-tries
sending forever if it failed to append NLMSG_DONE (which will never
work if buffer is not large enough).

Reported-by: Houcheng Lin <houcheng@gmail.com>
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 net/netfilter/nfnetlink_log.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/netfilter/nfnetlink_log.c b/net/netfilter/nfnetlink_log.c
index b1e3a05..8117fba 100644
--- a/net/netfilter/nfnetlink_log.c
+++ b/net/netfilter/nfnetlink_log.c
@@ -649,7 +649,8 @@ nfulnl_log_packet(struct net *net,
 		+ nla_total_size(sizeof(u_int32_t))	/* gid */
 		+ nla_total_size(plen)			/* prefix */
 		+ nla_total_size(sizeof(struct nfulnl_msg_packet_hw))
-		+ nla_total_size(sizeof(struct nfulnl_msg_packet_timestamp));
+		+ nla_total_size(sizeof(struct nfulnl_msg_packet_timestamp))
+		+ nla_total_size(sizeof(struct nfgenmsg));	/* NLMSG_DONE */
 
 	if (in && skb_mac_header_was_set(skb)) {
 		size +=   nla_total_size(skb->dev->hard_header_len)
@@ -692,8 +693,7 @@ nfulnl_log_packet(struct net *net,
 		goto unlock_and_release;
 	}
 
-	if (inst->skb &&
-	    size > skb_tailroom(inst->skb) - sizeof(struct nfgenmsg)) {
+	if (inst->skb && size > skb_tailroom(inst->skb)) {
 		/* either the queue len is too high or we don't have
 		 * enough room in the skb left. flush to userspace. */
 		__nfulnl_flush(inst);
-- 
2.0.4

Re: [PATCH 1/3] netfilter: nf_log: account for size of NLMSG_DONE attribute

[Pablo Neira Ayuso]

On Thu, Oct 23, 2014 at 10:36:06AM +0200, Florian Westphal wrote:
> We currently neither account for the nlattr size, nor do we consider
> the size of the trailing NLMSG_DONE when allocating nlmsg skb.
> 
> This can result in nflog to stop working, as __nfulnl_send() re-tries
> sending forever if it failed to append NLMSG_DONE (which will never
> work if buffer is not large enough).

Applied, thanks.

Re: [PATCH 1/3] netfilter: nf_log: account for size of NLMSG_DONE attribute

[Pablo Neira Ayuso]

On Fri, Oct 24, 2014 at 02:30:27PM +0200, Pablo Neira Ayuso wrote:
> On Thu, Oct 23, 2014 at 10:36:06AM +0200, Florian Westphal wrote:
> > We currently neither account for the nlattr size, nor do we consider
> > the size of the trailing NLMSG_DONE when allocating nlmsg skb.
> > 
> > This can result in nflog to stop working, as __nfulnl_send() re-tries
> > sending forever if it failed to append NLMSG_DONE (which will never
> > work if buffer is not large enough).
> 
> Applied, thanks.

BTW, I'm going to pass this to -stable, it applies cleanly starting
2.6.32 and upwards.

[PATCH 2/3] netfilter: nfnetlink_log: fix maximum packet length logged to userspace

[Florian Westphal]

don't try to queue payloads > 0xffff - NLA_HDRLEN, it does not work.
The nla length includes the size of the nla struct, so anything larger
results in u16 integer overflow.

This patch is similar to
9cefbbc9c8f9abe (netfilter: nfnetlink_queue: cleanup copy_range usage).

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 net/netfilter/nfnetlink_log.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/net/netfilter/nfnetlink_log.c b/net/netfilter/nfnetlink_log.c
index 8117fba..2d02eac3 100644
--- a/net/netfilter/nfnetlink_log.c
+++ b/net/netfilter/nfnetlink_log.c
@@ -43,7 +43,8 @@
 #define NFULNL_NLBUFSIZ_DEFAULT	NLMSG_GOODSIZE
 #define NFULNL_TIMEOUT_DEFAULT 	100	/* every second */
 #define NFULNL_QTHRESH_DEFAULT 	100	/* 100 packets */
-#define NFULNL_COPY_RANGE_MAX	0xFFFF	/* max packet size is limited by 16-bit struct nfattr nfa_len field */
+/* max packet size is limited by 16-bit struct nfattr nfa_len field */
+#define NFULNL_COPY_RANGE_MAX	(0xFFFF - NLA_HDRLEN)
 
 #define PRINTR(x, args...)	do { if (net_ratelimit()) \
 				     printk(x, ## args); } while (0);
@@ -252,6 +253,8 @@ nfulnl_set_mode(struct nfulnl_instance *inst, u_int8_t mode,
 
 	case NFULNL_COPY_PACKET:
 		inst->copy_mode = mode;
+		if (range == 0)
+			range = NFULNL_COPY_RANGE_MAX;
 		inst->copy_range = min_t(unsigned int,
 					 range, NFULNL_COPY_RANGE_MAX);
 		break;
@@ -679,8 +682,7 @@ nfulnl_log_packet(struct net *net,
 		break;
 
 	case NFULNL_COPY_PACKET:
-		if (inst->copy_range == 0
-		    || inst->copy_range > skb->len)
+		if (inst->copy_range > skb->len)
 			data_len = skb->len;
 		else
 			data_len = inst->copy_range;
-- 
2.0.4

Re: [PATCH 2/3] netfilter: nfnetlink_log: fix maximum packet length logged to userspace

[Pablo Neira Ayuso]

On Thu, Oct 23, 2014 at 10:36:07AM +0200, Florian Westphal wrote:
> don't try to queue payloads > 0xffff - NLA_HDRLEN, it does not work.
> The nla length includes the size of the nla struct, so anything larger
> results in u16 integer overflow.
> 
> This patch is similar to
> 9cefbbc9c8f9abe (netfilter: nfnetlink_queue: cleanup copy_range usage).

Indeed, if we find problem in nfqueue, we should also keep in mind
that we should revisit nflog too. Those two codebases are very similar
(I suspect one forked from another at some point, with ad-hoc
modifications to each case).

Applied, thanks a lot for taking the time to look into this.

[PATCH 3/3] netfilter: nf_log: release skbuff on nlmsg put failure

[Florian Westphal]

From: Houcheng Lin <houcheng@gmail.com>

The kernel should reserve enough room in the skb so that the DONE
message can always be appended.  However, in case of e.g. new attribute
erronously not being size-accounted for, __nfulnl_send() will still
try to put next nlmsg into this full skbuf, causing the skb to be stuck
forever and blocking delivery of further messages.

Fix issue by releasing skb immediately after nlmsg_put error and
WARN() so we can track down the cause of such size mismatch.

[ fw@strlen.de: add tailroom/len info to WARN ]

Signed-off-by: Houcheng Lin <houcheng@gmail.com>
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 net/netfilter/nfnetlink_log.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/net/netfilter/nfnetlink_log.c b/net/netfilter/nfnetlink_log.c
index 2d02eac3..5f1be5b 100644
--- a/net/netfilter/nfnetlink_log.c
+++ b/net/netfilter/nfnetlink_log.c
@@ -346,26 +346,25 @@ nfulnl_alloc_skb(struct net *net, u32 peer_portid, unsigned int inst_size,
 	return skb;
 }
 
-static int
+static void
 __nfulnl_send(struct nfulnl_instance *inst)
 {
-	int status = -1;
-
 	if (inst->qlen > 1) {
 		struct nlmsghdr *nlh = nlmsg_put(inst->skb, 0, 0,
 						 NLMSG_DONE,
 						 sizeof(struct nfgenmsg),
 						 0);
-		if (!nlh)
+		if (WARN_ONCE(!nlh, "bad nlskb size: %u, tailroom %d\n",
+			      inst->skb->len, skb_tailroom(inst->skb))) {
+			kfree_skb(inst->skb);
 			goto out;
+		}
 	}
-	status = nfnetlink_unicast(inst->skb, inst->net, inst->peer_portid,
-				   MSG_DONTWAIT);
-
+	nfnetlink_unicast(inst->skb, inst->net, inst->peer_portid,
+			  MSG_DONTWAIT);
+out:
 	inst->qlen = 0;
 	inst->skb = NULL;
-out:
-	return status;
 }
 
 static void
-- 
2.0.4

Re: [PATCH 3/3] netfilter: nf_log: release skbuff on nlmsg put failure

[Pablo Neira Ayuso]

On Thu, Oct 23, 2014 at 10:36:08AM +0200, Florian Westphal wrote:
> From: Houcheng Lin <houcheng@gmail.com>
> 
> The kernel should reserve enough room in the skb so that the DONE
> message can always be appended.  However, in case of e.g. new attribute
> erronously not being size-accounted for, __nfulnl_send() will still
> try to put next nlmsg into this full skbuf, causing the skb to be stuck
> forever and blocking delivery of further messages.
> 
> Fix issue by releasing skb immediately after nlmsg_put error and
> WARN() so we can track down the cause of such size mismatch.

Good idea.

> [ fw@strlen.de: add tailroom/len info to WARN ]

Applied, thanks.