netfilter: revalidate bridge ports

ebt_redirect_tg() dereferences br_port_get_rcu() return without a
NULL check, causing a kernel panic when the bridge port has been
removed between the original hook invocation and an NFQUEUE
reinject.

A mere NULL check isn't sufficient, however.  As sashiko review
points out userspace can not only remove the port from the bridge,
it could also place the device in a different virtual device, e.g.
macvlan.

If this happens, we must drop the packet, there is no way for us to
reinject it into the bridge path.

Switch to _upper API, we don't need the bridge port structure.
Also, this fix keeps another bug intact:

Both nfnetlink_log and nfnetlink_queue use CONFIG_BRIDGE_NETFILTER
too aggressive, which prevents certain logging features when queueing
in bridge family: NETFILTER_FAMILY_BRIDGE can be enabled while the old
CONFIG_BRIDGE_NETFILTER cruft is off.

Fixes tag is a common ancestor, this was always broken.

Fixes: f350a0a87374 ("bridge: use rx_handler_data pointer to store net_bridge_port pointer")
Reported-by: Ji'an Zhou <eilaimemedsnaimel@gmail.com>
Assisted-by: Claude:claude-sonnet-4-6
Signed-off-by: Florian Westphal <fw@strlen.de>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

diff --git a/net/bridge/netfilter/ebt_dnat.c b/net/bridge/netfilter/ebt_dnat.c
index 3fda71a8579d13920b3e738fc3fbcd8251dc468c..73f185cccd63dfcf00afbd5498c8230613de11ed 100644 (file)
--- a/net/bridge/netfilter/ebt_dnat.c
+++ b/net/bridge/netfilter/ebt_dnat.c
@@ -39,7 +39,9 @@ ebt_dnat_tg(struct sk_buff *skb, const struct xt_action_param *par)
                        dev = xt_in(par);
                        break;
                case NF_BR_PRE_ROUTING:
-                       dev = br_port_get_rcu(xt_in(par))->br->dev;
+                       dev = netdev_master_upper_dev_get_rcu(xt_in(par));
+                       if (!dev) /* bridge port removed? */
+                               return EBT_DROP;
                        break;
                default:
                        dev = NULL;

diff --git a/net/bridge/netfilter/ebt_redirect.c b/net/bridge/netfilter/ebt_redirect.c
index 307790562b492959d3e7a2332a5c8196ce901beb..83486cd4d564b1dabe26e705be32a39982006341 100644 (file)
--- a/net/bridge/netfilter/ebt_redirect.c
+++ b/net/bridge/netfilter/ebt_redirect.c
@@ -24,12 +24,18 @@ ebt_redirect_tg(struct sk_buff *skb, const struct xt_action_param *par)
        if (skb_ensure_writable(skb, 0))
                return EBT_DROP;
 
-       if (xt_hooknum(par) != NF_BR_BROUTING)
-               /* rcu_read_lock()ed by nf_hook_thresh */
-               ether_addr_copy(eth_hdr(skb)->h_dest,
-                               br_port_get_rcu(xt_in(par))->br->dev->dev_addr);
-       else
+       if (xt_hooknum(par) != NF_BR_BROUTING) {
+               const struct net_device *dev;
+
+               dev = netdev_master_upper_dev_get_rcu(xt_in(par));
+               if (!dev)
+                       return EBT_DROP;
+
+               ether_addr_copy(eth_hdr(skb)->h_dest, dev->dev_addr);
+       } else {
                ether_addr_copy(eth_hdr(skb)->h_dest, xt_in(par)->dev_addr);
+       }
+
        skb->pkt_type = PACKET_HOST;
        return info->target;
 }

diff --git a/net/netfilter/nfnetlink_log.c b/net/netfilter/nfnetlink_log.c
index 2439cbbd5b268e6de9fdd6b96f7abcbfc2b9d1fd..fa36575998616f63798b819cd84961493010aa48 100644 (file)
--- a/net/netfilter/nfnetlink_log.c
+++ b/net/netfilter/nfnetlink_log.c
@@ -451,6 +451,23 @@ nla_put_failure:
        return -1;
 }
 
+#if IS_ENABLED(CONFIG_BRIDGE_NETFILTER)
+static int nflog_put_master_ifindex(struct sk_buff *nlskb, int attr,
+                                   const struct net_device *dev)
+{
+       const struct net_device *upper;
+
+       if (dev && !netif_is_bridge_port(dev))
+               return 0;
+
+       upper = netdev_master_upper_dev_get_rcu((struct net_device *)dev);
+       if (upper && nla_put_be32(nlskb, attr, htonl(upper->ifindex)))
+               return -EMSGSIZE;
+
+       return 0;
+}
+#endif
+
 /* This is an inline function, we don't really care about a long
  * list of arguments */
 static inline int
@@ -505,8 +522,7 @@ __build_packet_message(struct nfnl_log_net *log,
                        /* rcu_read_lock()ed by nf_hook_thresh or
                         * nf_log_packet.
                         */
-                           nla_put_be32(inst->skb, NFULA_IFINDEX_INDEV,
-                                        htonl(br_port_get_rcu(indev)->br->dev->ifindex)))
+                           nflog_put_master_ifindex(inst->skb, NFULA_IFINDEX_INDEV, indev))
                                goto nla_put_failure;
                } else {
                        int physinif;
@@ -542,8 +558,7 @@ __build_packet_message(struct nfnl_log_net *log,
                        /* rcu_read_lock()ed by nf_hook_thresh or
                         * nf_log_packet.
                         */
-                           nla_put_be32(inst->skb, NFULA_IFINDEX_OUTDEV,
-                                        htonl(br_port_get_rcu(outdev)->br->dev->ifindex)))
+                           nflog_put_master_ifindex(inst->skb, NFULA_IFINDEX_OUTDEV, outdev))
                                goto nla_put_failure;
                } else {
                        struct net_device *physoutdev;

diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c
index 60ab88d45096e1cf88cd08d351f3019b6209e9a5..c5e29fec419bb43c04d50cfa80fa340c62ada221 100644 (file)
--- a/net/netfilter/nfnetlink_queue.c
+++ b/net/netfilter/nfnetlink_queue.c
@@ -440,10 +440,47 @@ static bool nf_ct_drop_unconfirmed(const struct nf_queue_entry *entry, bool *is_
        return false;
 }
 
+static bool nf_bridge_port_valid(const struct net_device *dev)
+{
+       if (!dev)
+               return true;
+
+       return netif_is_bridge_port(dev);
+}
+
+/* queued skbs leave rcu protection.  We bump device refcount so that
+ * the device cannot go away.  However, while packet was out the port
+ * could have been removed from the bridge.
+ *
+ * Ensure in+outdev are still part of a bridge at reinject time.
+ *
+ * The device rx_handler_data could even be pointing at data that is
+ * not a net_bridge_port structure.
+ */
+static bool nf_bridge_ports_valid(const struct nf_queue_entry *entry)
+{
+#if IS_ENABLED(CONFIG_BRIDGE_NETFILTER)
+       if (!nf_bridge_port_valid(entry->physin) ||
+           !nf_bridge_port_valid(entry->physout))
+               return false;
+#endif
+       if (entry->state.pf != PF_BRIDGE)
+               return true;
+
+       if (!nf_bridge_port_valid(entry->state.in) ||
+           !nf_bridge_port_valid(entry->state.out))
+               return false;
+
+       return true;
+}
+
 static void nfqnl_reinject(struct nf_queue_entry *entry, unsigned int verdict)
 {
        const struct nf_ct_hook *ct_hook;
 
+       if (!nf_bridge_ports_valid(entry))
+               verdict = NF_DROP;
+
        if (verdict == NF_ACCEPT ||
            verdict == NF_REPEAT ||
            verdict == NF_STOP) {
@@ -636,6 +673,23 @@ static int nf_queue_checksum_help(struct sk_buff *entskb)
        return skb_checksum_help(entskb);
 }
 
+#if IS_ENABLED(CONFIG_BRIDGE_NETFILTER)
+static int nfqnl_put_master_ifindex(struct sk_buff *nlskb, int attr,
+                                   const struct net_device *dev)
+{
+       const struct net_device *upper;
+
+       if (dev && !netif_is_bridge_port(dev))
+               return 0;
+
+       upper = netdev_master_upper_dev_get_rcu((struct net_device *)dev);
+       if (upper && nla_put_be32(nlskb, attr, htonl(upper->ifindex)))
+               return -EMSGSIZE;
+
+       return 0;
+}
+#endif
+
 static struct sk_buff *
 nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
                           struct nf_queue_entry *entry,
@@ -771,10 +825,7 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
                         * netfilter_bridge) */
                        if (nla_put_be32(skb, NFQA_IFINDEX_PHYSINDEV,
                                         htonl(indev->ifindex)) ||
-                       /* this is the bridge group "brX" */
-                       /* rcu_read_lock()ed by __nf_queue */
-                           nla_put_be32(skb, NFQA_IFINDEX_INDEV,
-                                        htonl(br_port_get_rcu(indev)->br->dev->ifindex)))
+                           nfqnl_put_master_ifindex(skb, NFQA_IFINDEX_INDEV, indev))
                                goto nla_put_failure;
                } else {
                        int physinif;
@@ -805,10 +856,7 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
                         * netfilter_bridge) */
                        if (nla_put_be32(skb, NFQA_IFINDEX_PHYSOUTDEV,
                                         htonl(outdev->ifindex)) ||
-                       /* this is the bridge group "brX" */
-                       /* rcu_read_lock()ed by __nf_queue */
-                           nla_put_be32(skb, NFQA_IFINDEX_OUTDEV,
-                                        htonl(br_port_get_rcu(outdev)->br->dev->ifindex)))
+                           nfqnl_put_master_ifindex(skb, NFQA_IFINDEX_OUTDEV, outdev))
                                goto nla_put_failure;
                } else {
                        int physoutif;