[PATCH nf 0/4] netfilter: logging cleanups

[PATCH nf 0/4] netfilter: logging cleanups

[Florian Westphal]

A few patches to address LLM-reported issues.

While working on those LLM also reported that ARPHRD_SIT is dead
code, so:

1) Remove dead ARPHRD_SIT branches from MAC/HW header logging in
nf_log_syslog.c and nfnetlink_log.c. The paths are unreachable ever
since SIT tunnel hard_header_len was set to zero 6 years ago.

Then there are the originally reported issues:

2) Fix MAC header dump in nf_log_syslog to base the byte count on
skb_mac_header_len() clamped to dev->hard_header_len. Prevent logging
IP/IPv6 header bytes as part of the MAC address when the actual MAC header
is shorter than the device's hard header.

3) Extract dev_parse_header usage into a shared helper for nfnetlink_log and
nfnetlink_queue, clamping hw addresses to 8 bytes and validating the skb
mac header. Add BUILD_BUG_ON checks to assert identical container struct
layouts.

4) Use skb_mac_header_len when dumping mac headers in nfnetlink_log and
clamp to hard_header_len.

Skip mac header logging at POST_ROUTING where the header still belongs
to the ingress device.  Originally POST_ROUTING never had incoming
device handed to it, but that changed.   Make it stricter again.

Florian Westphal (4):
  netfilter: nf_log: remove dead ARPHRD_SIT handling from MAC/HW header
    logging
  netfilter: nf_log_syslog: fix MAC header dump to use real header
    length
  netfilter: nfnetlink_log: fix up dev_parse_header usage
  netfilter: nfnetlink_log: rework mac header logging

 net/netfilter/nf_log_syslog.c        | 24 ++-----
 net/netfilter/nfnetlink_log.c        | 90 ++++++++++++++++---------
 net/netfilter/nfnetlink_log_common.h | 99 ++++++++++++++++++++++++++++
 net/netfilter/nfnetlink_queue.c      | 18 ++---
 4 files changed, 167 insertions(+), 64 deletions(-)
 create mode 100644 net/netfilter/nfnetlink_log_common.h
-- 
2.56.0

[PATCH nf 1/4] netfilter: nf_log: remove dead ARPHRD_SIT handling from MAC/HW header logging

[Florian Westphal]

dump_mac_header() in nf_log_syslog.c and the HWHEADER dump in
nfnetlink_log.c special-case ARPHRD_SIT devices by backing the header
pointer up by ETH_HLEN before dumping dev->hard_header_len bytes, on
the assumption that a SIT tunnel's "hard header" covers the
underlying device's real Ethernet header plus the tunnel's IPv4
header.

That is no longer the case: ever sice commit 8ef9ba4d6666 ("IPv6: Set
SIT tunnel hard_header_len to zero"), dev->hard_header_len is 0 and
these branches are dead code.

No bug report in 6 years, so lets remove those branches.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 net/netfilter/nf_log_syslog.c | 23 +++--------------------
 net/netfilter/nfnetlink_log.c |  6 +-----
 2 files changed, 4 insertions(+), 25 deletions(-)

diff --git a/net/netfilter/nf_log_syslog.c b/net/netfilter/nf_log_syslog.c
index c3fd398ffcd7..3ff5a49a1c8d 100644
--- a/net/netfilter/nf_log_syslog.c
+++ b/net/netfilter/nf_log_syslog.c
@@ -820,26 +820,9 @@ static void dump_mac_header(struct nf_log_buf *m,
 		const unsigned char *p = skb_mac_header(skb);
 		unsigned int i;
 
-		if (dev->type == ARPHRD_SIT) {
-			p -= ETH_HLEN;
-
-			if (p < skb->head)
-				p = NULL;
-		}
-
-		if (p) {
-			nf_log_buf_add(m, "%02x", *p++);
-			for (i = 1; i < dev->hard_header_len; i++)
-				nf_log_buf_add(m, ":%02x", *p++);
-		}
-
-		if (dev->type == ARPHRD_SIT) {
-			const struct iphdr *iph =
-				(struct iphdr *)skb_mac_header(skb);
-
-			nf_log_buf_add(m, " TUNNEL=%pI4->%pI4", &iph->saddr,
-				       &iph->daddr);
-		}
+		nf_log_buf_add(m, "%02x", *p++);
+		for (i = 1; i < dev->hard_header_len; i++)
+			nf_log_buf_add(m, ":%02x", *p++);
 	}
 	nf_log_buf_add(m, " ");
 }
diff --git a/net/netfilter/nfnetlink_log.c b/net/netfilter/nfnetlink_log.c
index d923f2cb1398..0b590c117f4c 100644
--- a/net/netfilter/nfnetlink_log.c
+++ b/net/netfilter/nfnetlink_log.c
@@ -610,11 +610,7 @@ __build_packet_message(struct nfnl_log_net *log,
 
 		hwhdrp = skb_mac_header(skb);
 
-		if (skb->dev->type == ARPHRD_SIT)
-			hwhdrp -= ETH_HLEN;
-
-		if (hwhdrp >= skb->head &&
-		    nla_put(inst->skb, NFULA_HWHEADER,
+		if (nla_put(inst->skb, NFULA_HWHEADER,
 			    skb->dev->hard_header_len, hwhdrp))
 			goto nla_put_failure;
 	}
-- 
2.56.0

[PATCH nf 2/4] netfilter: nf_log_syslog: fix MAC header dump to use real header length

[Florian Westphal]

old code unconditionally iterated dev->hard_header_len bytes, assuming
the SKB's MAC header is always exactly that long.

If the actual mac header in the packet is smaller, the loop reads past
the MAC and logs ip/ipv6 header as if it was part of the address.

Use skb_mac_header_len() as the base length, then clamp the result to
dev->hard_header_len so we never read beyond what the
device actually uses for its hard header, even if mac header was
mangled.

LLM finding, no bug was ever reported about this.

Fixes: 7eb9282cd0ef ("netfilter: ipt_LOG/ip6t_LOG: add option to print decoded MAC header")
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 net/netfilter/nf_log_syslog.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/net/netfilter/nf_log_syslog.c b/net/netfilter/nf_log_syslog.c
index 3ff5a49a1c8d..332fe47fd109 100644
--- a/net/netfilter/nf_log_syslog.c
+++ b/net/netfilter/nf_log_syslog.c
@@ -815,14 +815,19 @@ static void dump_mac_header(struct nf_log_buf *m,
 
 fallback:
 	nf_log_buf_add(m, "MAC=");
-	if (dev->hard_header_len && skb_mac_header_was_set(skb) &&
-	    skb_mac_header_len(skb) != 0) {
+	if (dev->hard_header_len && skb_mac_header_was_set(skb)) {
+		unsigned int mac_len = skb_mac_header_len(skb);
 		const unsigned char *p = skb_mac_header(skb);
 		unsigned int i;
 
-		nf_log_buf_add(m, "%02x", *p++);
-		for (i = 1; i < dev->hard_header_len; i++)
-			nf_log_buf_add(m, ":%02x", *p++);
+		if (mac_len > dev->hard_header_len)
+			mac_len = dev->hard_header_len;
+
+		if (mac_len) {
+			nf_log_buf_add(m, "%02x", *p++);
+			for (i = 1; i < mac_len; i++)
+				nf_log_buf_add(m, ":%02x", *p++);
+		}
 	}
 	nf_log_buf_add(m, " ");
 }
-- 
2.56.0

[PATCH nf 3/4] netfilter: nfnetlink_log: fix up dev_parse_header usage

[Florian Westphal]

dev_parse_header() grabs the source hw address from the skb and stores
it in the given buffer.

e.g.: eth_header_parse() is simply:

int eth_header_parse(const struct sk_buff *skb,
		     const struct net_device *dev,
		     unsigned char *haddr)
{
	memcpy(haddr, eth_hdr(skb->h_source, ETH_ALEN);
	return ETH_ALEN;
}

Problem is that the compound structure used by the UABI in nfnetlink_log
and nfnetlink_queue has a fixed 8-byte size.
Enough for ETH_ALEN, but not necessarily enough for other address types.

Move this to a helper and allow sharing it between the copypasted users.

The struct layout of the two container structures have same layout:
add BUILD_BUG_ON checks to assert this.

Due to the nature of the dev_parse_header() helpers:

1. skb MUST have a valid mac header, else we read garbage
2. users MUST NOT use this when ingress device and skb->dev
have diverged (we can't know we would decode something
meaningful anymore).
3. Because container structure only has space for 8 octets,
   we have to clamp the result.

Do all of this in the common helper.

This is an LLM finding, no bug was ever reported about this.

Fixes: 0597f2680d66 ("[NETFILTER]: Add new "nfnetlink_log" userspace packet logging facility")
Fixes: 7af4cc3fa158 ("[NETFILTER]: Add "nfnetlink_queue" netfilter queue handler over nfnetlink")
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 net/netfilter/nfnetlink_log.c        | 18 ++---
 net/netfilter/nfnetlink_log_common.h | 99 ++++++++++++++++++++++++++++
 net/netfilter/nfnetlink_queue.c      | 18 ++---
 3 files changed, 107 insertions(+), 28 deletions(-)
 create mode 100644 net/netfilter/nfnetlink_log_common.h

diff --git a/net/netfilter/nfnetlink_log.c b/net/netfilter/nfnetlink_log.c
index 0b590c117f4c..29530b753928 100644
--- a/net/netfilter/nfnetlink_log.c
+++ b/net/netfilter/nfnetlink_log.c
@@ -47,6 +47,8 @@
 #include <net/netfilter/nf_conntrack.h>
 #endif
 
+#include "nfnetlink_log_common.h"
+
 #define NFULNL_COPY_DISABLED	0xff
 #define NFULNL_NLBUFSIZ_DEFAULT	NLMSG_GOODSIZE
 #define NFULNL_TIMEOUT_DEFAULT 	100	/* every second */
@@ -587,20 +589,8 @@ __build_packet_message(struct nfnl_log_net *log,
 	    nla_put_be32(inst->skb, NFULA_MARK, htonl(skb->mark)))
 		goto nla_put_failure;
 
-	if (indev && skb->dev &&
-	    skb_mac_header_was_set(skb) &&
-	    skb_mac_header_len(skb) != 0) {
-		struct nfulnl_msg_packet_hw phw;
-		int len;
-
-		memset(&phw, 0, sizeof(phw));
-		len = dev_parse_header(skb, phw.hw_addr);
-		if (len > 0) {
-			phw.hw_addrlen = htons(len);
-			if (nla_put(inst->skb, NFULA_HWADDR, sizeof(phw), &phw))
-				goto nla_put_failure;
-		}
-	}
+	if (nfnetlink_put_hwaddr(inst->skb, skb, indev, NFULA_HWADDR))
+		goto nla_put_failure;
 
 	if (indev && skb_mac_header_was_set(skb)) {
 		if (nla_put_be16(inst->skb, NFULA_HWTYPE, htons(skb->dev->type)) ||
diff --git a/net/netfilter/nfnetlink_log_common.h b/net/netfilter/nfnetlink_log_common.h
new file mode 100644
index 000000000000..755a3364c8a0
--- /dev/null
+++ b/net/netfilter/nfnetlink_log_common.h
@@ -0,0 +1,99 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _NF_NETLINK_LOG_COMMON_H
+#define _NF_NETLINK_LOG_COMMON_H
+
+#include <linux/netfilter/nfnetlink_queue.h>
+#include <linux/netfilter/nfnetlink_log.h>
+
+/* nfnetlink_log and nfnetlink_queue suffer from copypaste; avoid adding
+ * more and place shared code here.
+ */
+static __always_inline void assert_nfqnl_nfunl_hdrs(void)
+{
+	struct nfulnl_msg_packet_hw phwu;
+	struct nfqnl_msg_packet_hw phwq;
+
+	BUILD_BUG_ON(sizeof(phwu) != sizeof(phwq));
+
+#define X(memb) offsetof(struct nfulnl_msg_packet_hw, memb) != \
+		offsetof(struct nfqnl_msg_packet_hw, memb)
+
+	BUILD_BUG_ON(X(hw_addrlen));
+	BUILD_BUG_ON(X(hw_addr));
+#undef X
+#define X(memb) sizeof(phwu.memb) != sizeof(phwq.memb)
+	BUILD_BUG_ON(X(hw_addrlen));
+#undef X
+}
+
+/**
+ * nfnetlink_put_hwaddr - Serialize the L2 hardware address into a netlink attribute.
+ * @nlskb: netlink message to append the attribute to.
+ * @skb: the packet whose hardware address is being serialized.
+ * @indev: the input device; must equal @skb->dev.
+ * @attr: netlink attribute type identifier.
+ *
+ * Extracts the MAC header from @skb using dev_parse_header() and appends
+ * it as a netlink attribute of type @attr to @nlskb.
+ *
+ * UAPI NOTE: The attribute is serialized in a legacy container structure
+ * (nfnl_msg_packet_hw) rather than a plain nla_put(hwaddr, len). The
+ * hw_addrlen member in that structure is redundant -- userspace already
+ * knows the size from the netlink attribute length -- but is retained
+ * for ABI compatibility. nfnetlink_queue duplicates this layout; both
+ * structs are layout-identical, so this one helper serves both.
+ *
+ * The maximum address length is 8 octets (MAX_ADDR_LEN / ETH_ALEN upper
+ * bound), even though netlink itself supports longer addresses. This is a
+ * limitation of the compound-structure ABI, not of netlink.
+ *
+ * This helper checks:
+ *  - @skb->dev is non-NULL.
+ *  - @indev is equal @skb->dev; dev_parse_header() dereferences
+ *    dev->header_ops on skb->dev, so a mismatched device (e.g. egress
+ *    device on an egress path) would parse the wrong L2 header.
+ *  - L2 header is present.
+ *
+ * Return: -EMSGSIZE if the tailroom of the skb is insufficient to store
+ * the attribute header and payload.
+ */
+static inline int
+nfnetlink_put_hwaddr(struct sk_buff *nlskb,
+		     const struct sk_buff *skb,
+		     const struct net_device *indev,
+		     u16 attr)
+{
+	char hw_addr[MAX_ADDR_LEN];
+	unsigned int mac_len;
+	int len;
+
+	if (!skb->dev ||
+	    indev != skb->dev ||
+	    !skb_mac_header_was_set(skb))
+		return 0;
+
+	mac_len = skb_mac_header_len(skb);
+	if (mac_len == 0)
+		return 0;
+
+	len = dev_parse_header(skb, hw_addr);
+	if (len > 0) {
+		struct nfulnl_msg_packet_hw phw;
+		const int max_hwlen = (int)sizeof(phw.hw_addr);
+
+		assert_nfqnl_nfunl_hdrs();
+
+		/* UAPI limitation, must clamp. */
+		if (len > max_hwlen)
+			len = max_hwlen;
+
+		memset(&phw, 0, sizeof(phw));
+		memcpy(phw.hw_addr, hw_addr, len);
+		phw.hw_addrlen = htons(len);
+
+		return nla_put(nlskb, attr, sizeof(phw), &phw);
+	}
+
+	return 0;
+}
+#endif
diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c
index 90d887546f7d..7c0fb4777852 100644
--- a/net/netfilter/nfnetlink_queue.c
+++ b/net/netfilter/nfnetlink_queue.c
@@ -56,6 +56,8 @@
 #include <net/netfilter/nf_conntrack.h>
 #endif
 
+#include "nfnetlink_log_common.h"
+
 #define NFQNL_QMAX_DEFAULT 1024
 #define NFQNL_HASH_MIN     8
 #define NFQNL_HASH_MAX     32768
@@ -906,20 +908,8 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
 	    nla_put_be32(skb, NFQA_PRIORITY, htonl(entskb->priority)))
 		goto nla_put_failure;
 
-	if (indev && entskb->dev &&
-	    skb_mac_header_was_set(entskb) &&
-	    skb_mac_header_len(entskb) != 0) {
-		struct nfqnl_msg_packet_hw phw;
-		int len;
-
-		memset(&phw, 0, sizeof(phw));
-		len = dev_parse_header(entskb, phw.hw_addr);
-		if (len) {
-			phw.hw_addrlen = htons(len);
-			if (nla_put(skb, NFQA_HWADDR, sizeof(phw), &phw))
-				goto nla_put_failure;
-		}
-	}
+	if (nfnetlink_put_hwaddr(skb, entskb, indev, NFQA_HWADDR))
+		goto nla_put_failure;
 
 	if (nfqnl_put_bridge(entry, skb) < 0)
 		goto nla_put_failure;
-- 
2.56.0

[PATCH nf 4/4] netfilter: nfnetlink_log: rework mac header logging

[Florian Westphal]

For unknown reasons this uses dev->hard_header_len.

LLM says that hard_header_len says how long a link layer header can get,
not how many bytes this packet holds. Use skb_mac_header_len instead and
cap if needed.

AFAICS this should have dumped skb_mac_header_len bytes instead, with one
single attribute (plus maybe skb->protocol).

Keep it close to the original version: if hard_header_len is larger than
the mac header size of the packet, then clamp to that value.

While at it, also move this so that we don't call this for POST_ROUTING
hook; mac header is still that of the incoming device, not the egress one.

I suspect that this is almost exclusively used with plain ethernet,
where hard_header_len and skb_mac_header_len() are 14 bytes which
would explain absence of bug reports.

Fixes: 72961ecf84d6 ("netfilter: nfnetlink_log: send complete hardware header")
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 net/netfilter/nfnetlink_log.c | 68 +++++++++++++++++++++++++++--------
 1 file changed, 54 insertions(+), 14 deletions(-)

diff --git a/net/netfilter/nfnetlink_log.c b/net/netfilter/nfnetlink_log.c
index 29530b753928..e6877ac36d26 100644
--- a/net/netfilter/nfnetlink_log.c
+++ b/net/netfilter/nfnetlink_log.c
@@ -458,6 +458,56 @@ static int nfulnl_put_bridge(struct nfulnl_instance *inst, const struct sk_buff
 	return -1;
 }
 
+/**
+ * nfulnl_put_hwheader - Append link-layer header info to the netlink skb.
+ * @nlskb:      Target netlink message to append attributes to.
+ * @skb:        Original packet skb whose link-layer header is described.
+ *
+ * Emits the NFULA_HWTYPE and NFULA_HWLEN netlink attributes into @nlskb
+ * based on the link-layer header of @skb.
+ *
+ * The device's hard_header_len describes the maximum size a link-layer
+ * header can grow to, not the actual size of this packet's header.
+ * The actual length is taken from skb_mac_header_len() and capped at
+ * hard_header_len if needed.
+ *
+ * The NFULA_HWLEN attribute is retained for backward compatibility.
+ * Userspace should derive the length from the NFULA_HWHEADER attribute
+ * size instead.
+ *
+ * Caller must check skb_mac_header_was_set(skb) before using this
+ * function.
+ *
+ * Return: -EMSGSIZE if the tailroom of the skb is insufficient to store
+ * the attribute header and payload.
+ */
+static int nfulnl_put_hwheader(struct sk_buff *nlskb, const struct sk_buff *skb)
+{
+	unsigned int hwlen = READ_ONCE(skb->dev->hard_header_len);
+	const unsigned char *hwhdrp = skb_mac_header(skb);
+	int len = skb_mac_header_len(skb);
+	u16 hwtype = skb->dev->type;
+
+	if (nla_put_be16(nlskb, NFULA_HWTYPE, htons(hwtype)))
+		return -EMSGSIZE;
+
+	if (len > hwlen)
+		len = hwlen;
+
+	/* This attribute should not exist, userspace should
+	 * use NFULA_HWHEADER attribute size. Removing
+	 * this breaks existing users which unfortunately
+	 * rely on NFULA_HWLEN presence.
+	 */
+	if (nla_put_be16(nlskb, NFULA_HWLEN, htons(len)))
+		return -EMSGSIZE;
+
+	if (len == 0)
+		return 0;
+
+	return nla_put(nlskb, NFULA_HWHEADER, len, hwhdrp);
+}
+
 #if IS_ENABLED(CONFIG_BRIDGE_NETFILTER)
 static int nflog_put_master_ifindex(struct sk_buff *nlskb, int attr,
 				    const struct net_device *dev)
@@ -494,7 +544,6 @@ __build_packet_message(struct nfnl_log_net *log,
 	struct nlmsghdr *nlh;
 	sk_buff_data_t old_tail = inst->skb->tail;
 	struct sock *sk;
-	const unsigned char *hwhdrp;
 
 	nlh = nfnl_msg_put(inst->skb, 0, 0,
 			   nfnl_msg_type(NFNL_SUBSYS_ULOG, NFULNL_MSG_PACKET),
@@ -592,25 +641,16 @@ __build_packet_message(struct nfnl_log_net *log,
 	if (nfnetlink_put_hwaddr(inst->skb, skb, indev, NFULA_HWADDR))
 		goto nla_put_failure;
 
-	if (indev && skb_mac_header_was_set(skb)) {
-		if (nla_put_be16(inst->skb, NFULA_HWTYPE, htons(skb->dev->type)) ||
-		    nla_put_be16(inst->skb, NFULA_HWLEN,
-				 htons(skb->dev->hard_header_len)))
-			goto nla_put_failure;
-
-		hwhdrp = skb_mac_header(skb);
-
-		if (nla_put(inst->skb, NFULA_HWHEADER,
-			    skb->dev->hard_header_len, hwhdrp))
-			goto nla_put_failure;
-	}
-
 	if (hooknum <= NF_INET_FORWARD) {
 		struct timespec64 kts = ktime_to_timespec64(skb_tstamp_cond(skb, true));
 		struct nfulnl_msg_packet_timestamp ts;
 		ts.sec = cpu_to_be64(kts.tv_sec);
 		ts.usec = cpu_to_be64(kts.tv_nsec / NSEC_PER_USEC);
 
+		if (indev && skb->dev && skb_mac_header_was_set(skb) &&
+		    nfulnl_put_hwheader(inst->skb, skb))
+			goto nla_put_failure;
+
 		if (nla_put(inst->skb, NFULA_TIMESTAMP, sizeof(ts), &ts))
 			goto nla_put_failure;
 	}
-- 
2.56.0