netdev - forwarded bridged packets enqueuing is broken

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID: <87pnh6lxch.fsf@tanaza.com>
Date:   Mon, 02 Dec 2019 13:34:54 +0100
From:   Marco Oliverio <marco.oliverio@...aza.com>
To:     netfilter-devel@...r.kernel.org
Cc:     Marco Oliverio <marco.oliverio@...aza.com>,
        Rocco Folino <notifications@...hub.com>,
        Florian Westphal <fw@...len.de>,
        netdev <netdev@...r.kernel.org>
Subject: forwarded bridged packets enqueuing is broken

Hi,

We cannot enqueue userspace bridged forwarded packets (neither in the
forward chain nor in the postrouting one):

nft add table bridge t
nft add chain bridge t forward {type filter hook forward priority 0\;}
nft add rule bridge t forward queue

packets from machines other than localhost aren't enqueued at all.

(this is also true for the postrouting chain).

We think the root of the problem is the check introduced by
b60a77386b1d4868f72f6353d35dabe5fbe981f2 (net: make skb_dst_force
return true when dst is refcounted):

modified   net/netfilter/nf_queue.c
@@ -174,6 +174,11 @@ static int __nf_queue(struct sk_buff *skb, const struct nf_hook_state *state,
 		goto err;
 	}

+	if (!skb_dst_force(skb) && state->hook != NF_INET_PRE_ROUTING) {
+		status = -ENETDOWN;
+		goto err;
+	}
+

AFAIU forwarded bridge packets have a null dst entry in the first
place, as they don't enter the ip stack, so skb_dst_force() returns
false. The very same commit suggested to check skb_dst() before
skb_dst_force(), doing that indeed fix the issue for us:

modified   net/netfilter/nf_queue.c
@@ -174,7 +174,7 @@ static int __nf_queue(struct sk_buff *skb, const struct nf_hook_state *state,
 		goto err;
 	}

-	if (!skb_dst_force(skb) && state->hook != NF_INET_PRE_ROUTING) {
+	if (skb_dst(skb) && !skb_dst_force(skb)) {
 		status = -ENETDOWN;
 		goto err;
 	}

This assumes that we shouldn't enqueue the packet if skb_dst_force()
sets not-NULL skb->dst to NULL, but it is safe to do that if skb->dst
was NULL in the first place. It should also cover che PRE_ROUTING hook
case. Is this assumption correct? Are there any side effects we're
missing?

If it is correct and it helps we can send a patch on top of the
netfilter tree.

Greetins
Marco