[PATCH nf] netfilter: nfnetlink_queue: unclone skb before writing NFQA_L2HDR

[PATCH nf] netfilter: nfnetlink_queue: unclone skb before writing NFQA_L2HDR

[Tamaki Yanagawa]

nfqa_parse_bridge() copies the NFQA_L2HDR attribute over the mac header
of the queued skb in place. Bridge flooding queues clones that share
their data with the copies sent to the other ports, so the rewritten
header also shows up on frames that never went through the queue, and
is changed while those may already be in transmission. Unclone the skb
before the copy, as nfqnl_mangle() does for NFQA_PAYLOAD via
skb_ensure_writable().

Fixes: 8d45ff22f1b4 ("netfilter: bridge: nf queue verdict to use NFQA_VLAN and NFQA_L2HDR")
Signed-off-by: Tamaki Yanagawa <ty@000ty.net>
Assisted-by: Claude:claude-opus-5-5
---
 net/netfilter/nfnetlink_queue.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c
index 90d887546f..146211fd03 100644
--- a/net/netfilter/nfnetlink_queue.c
+++ b/net/netfilter/nfnetlink_queue.c
@@ -1819,10 +1819,15 @@ static int nfqa_parse_bridge(struct nf_queue_entry *entry,
 
 		if (mac_header_len != nla_len(nfqa[NFQA_L2HDR]))
 			return -EINVAL;
-		else if (mac_header_len > 0)
+
+		if (mac_header_len > 0) {
+			if (skb_unclone(entry->skb, GFP_ATOMIC))
+				return -ENOMEM;
+
 			memcpy(skb_mac_header(entry->skb),
 			       nla_data(nfqa[NFQA_L2HDR]),
 			       mac_header_len);
+		}
 	}
 
 	return 0;

base-commit: 734bef548a96e8774d6eef5dfa2026c6edc4e80b
-- 
2.52.0

Re: [PATCH nf] netfilter: nfnetlink_queue: unclone skb before writing NFQA_L2HDR

[Florian Westphal]

Tamaki Yanagawa <ty@000ty.net> wrote:
> nfqa_parse_bridge() copies the NFQA_L2HDR attribute over the mac header
> of the queued skb in place. Bridge flooding queues clones that share
> their data with the copies sent to the other ports, so the rewritten
> header also shows up on frames that never went through the queue, and
> is changed while those may already be in transmission. Unclone the skb
> before the copy, as nfqnl_mangle() does for NFQA_PAYLOAD via
> skb_ensure_writable().

Can you explain in changelog why full unclone is needed rather than
skb_ensure_writable()?

Re: [PATCH nf] netfilter: nfnetlink_queue: unclone skb before writing NFQA_L2HDR

[Tamaki Yanagawa]

Florian Westphal <fw@strlen.de> wrote:
> Can you explain in changelog why full unclone is needed rather than
> skb_ensure_writable()?

It isn't. At the bridge hooks skb->data points to the network header, so
the mac header is in the headroom, and skb_clone_writable() already
checks headroom against hdr_len. skb_ensure_writable(skb, 0) is enough
and also avoids copying clones that own their header area. I'll send a
v2 using it and saying so in the changelog. I re-ran the bridge flood
test with it: the frame on the other port keeps its original header.

Thanks,
Tamaki