[PATCH net-next v2 0/5] bridge: Validate and clean up IPv6 neighbour suppression

[PATCH net-next v2 0/5] bridge: Validate and clean up IPv6 neighbour suppression

[Danielle Ratson]

The bridge implements IPv6 neighbour suppression by snooping Neighbour
Solicitation and Neighbour Advertisement messages, but it previously only
checked the ICMPv6 type and code before acting on them. This leaves it
open to acting on malformed or spoofed packets that any RFC 4861 compliant
node should reject, and the option parsing in br_nd_send() open-codes a
loop that has historically been a source of bugs.

This series hardens and cleans up that path:

Add ndisc_check_ns_na(), a standalone NS/NA validator modeled after
ipv6_mc_check_mld(), implementing the RFC 4861 section 7.1.1 / 7.1.2
mandatory receive checks (hop limit, checksum, code, length, target and
option validation). Wire the bridge into it so NS/NA messages are
validated to the same standard MLD already enjoys.

Replace the manual ND option parsing loop in br_nd_send() with
ndisc_parse_options() and ndisc_opt_addr_data(), and linearize the skb
once it has been validated as an NS/NA message so that this and any future
ND message handling operate on a linear buffer. The first patch is a small
preparatory cleanup that drops the now-unnecessary skb_header_pointer()
fallback from br_is_nd_neigh_msg().

No functional change is intended for well-formed packets.

Patchset overview:
Patch #1: drop the skb_header_pointer() fallback.
Patches #2-#3: add ndisc_check_ns_na() and validate NS/NA with it.
Patch #4: linearize once the ND message type is validated.
Patch #5: parse options via ndisc_parse_options().

v2:
	* Patches #2,#5:
		* Use EXPORT_SYMBOL_GPL() instead of EXPORT_SYMBOL() to export
		  ndisc_check_ns_na().
	* Patch #4:
		* Add a comment noting that br_is_nd_neigh_msg() also linearizes
		  the skb.

Danielle Ratson (5):
  bridge: Use direct pointer in br_is_nd_neigh_msg()
  ipv6: ndisc: Add ndisc_check_ns_na() validation helper
  bridge: Validate NS/NA messages using ndisc_check_ns_na()
  bridge: Linearize skb once the ND message type is validated
  bridge: Use ndisc_parse_options() to parse ND options in br_nd_send()

 include/net/ndisc.h          |   2 +
 net/bridge/br_arp_nd_proxy.c |  57 ++++++-----
 net/bridge/br_device.c       |   4 +-
 net/bridge/br_input.c        |   4 +-
 net/bridge/br_private.h      |   2 +-
 net/ipv6/Makefile            |   2 +-
 net/ipv6/ndisc.c             |   1 +
 net/ipv6/ndisc_snoop.c       | 190 +++++++++++++++++++++++++++++++++++
 8 files changed, 227 insertions(+), 35 deletions(-)
 create mode 100644 net/ipv6/ndisc_snoop.c

-- 
2.54.0

[PATCH net-next v2 1/5] bridge: Use direct pointer in br_is_nd_neigh_msg()

[Danielle Ratson]

Both callers of br_is_nd_neigh_msg() already call pskb_may_pull() to
ensure sizeof(struct ipv6hdr) + sizeof(struct nd_msg) bytes are in the
linear area before invoking this function. The skb_header_pointer()
call and its fallback buffer are therefore unnecessary.

Replace skb_header_pointer() with a direct cast to ipv6_hdr(skb) + 1
and drop the now-unused 'msg' parameter and its corresponding stack
buffer from all callers.

Reviewed-by: Petr Machata <petrm@nvidia.com>
Acked-by: Nikolay Aleksandrov <razor@blackwall.org>
Signed-off-by: Danielle Ratson <danieller@nvidia.com>
---
 net/bridge/br_arp_nd_proxy.c | 9 ++-------
 net/bridge/br_device.c       | 4 ++--
 net/bridge/br_input.c        | 4 ++--
 net/bridge/br_private.h      | 2 +-
 4 files changed, 7 insertions(+), 12 deletions(-)

diff --git a/net/bridge/br_arp_nd_proxy.c b/net/bridge/br_arp_nd_proxy.c
index 23eb6931a2b4..db08c3272001 100644
--- a/net/bridge/br_arp_nd_proxy.c
+++ b/net/bridge/br_arp_nd_proxy.c
@@ -234,14 +234,9 @@ void br_do_proxy_suppress_arp(struct sk_buff *skb, struct net_bridge *br,
 #endif
 
 #if IS_ENABLED(CONFIG_IPV6)
-struct nd_msg *br_is_nd_neigh_msg(const struct sk_buff *skb, struct nd_msg *msg)
+struct nd_msg *br_is_nd_neigh_msg(const struct sk_buff *skb)
 {
-	struct nd_msg *m;
-
-	m = skb_header_pointer(skb, skb_network_offset(skb) +
-			       sizeof(struct ipv6hdr), sizeof(*msg), msg);
-	if (!m)
-		return NULL;
+	struct nd_msg *m = (struct nd_msg *)(ipv6_hdr(skb) + 1);
 
 	if (m->icmph.icmp6_code != 0 ||
 	    (m->icmph.icmp6_type != NDISC_NEIGHBOUR_SOLICITATION &&
diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
index e7f343ab22d3..ff55dab73632 100644
--- a/net/bridge/br_device.c
+++ b/net/bridge/br_device.c
@@ -80,9 +80,9 @@ netdev_tx_t br_dev_xmit(struct sk_buff *skb, struct net_device *dev)
 		   pskb_may_pull(skb, sizeof(struct ipv6hdr) +
 				 sizeof(struct nd_msg)) &&
 		   ipv6_hdr(skb)->nexthdr == IPPROTO_ICMPV6) {
-			struct nd_msg *msg, _msg;
+			struct nd_msg *msg;
 
-			msg = br_is_nd_neigh_msg(skb, &_msg);
+			msg = br_is_nd_neigh_msg(skb);
 			if (msg)
 				br_do_suppress_nd(skb, br, vid, NULL, msg);
 	}
diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index ddb8f002a40e..d87a5f9fa92b 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c
@@ -176,9 +176,9 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
 		   pskb_may_pull(skb, sizeof(struct ipv6hdr) +
 				 sizeof(struct nd_msg)) &&
 		   ipv6_hdr(skb)->nexthdr == IPPROTO_ICMPV6) {
-			struct nd_msg *msg, _msg;
+			struct nd_msg *msg;
 
-			msg = br_is_nd_neigh_msg(skb, &_msg);
+			msg = br_is_nd_neigh_msg(skb);
 			if (msg)
 				br_do_suppress_nd(skb, br, vid, p, msg);
 	}
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 4cf8b1ab9047..e31439cb4420 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -2367,7 +2367,7 @@ void br_do_proxy_suppress_arp(struct sk_buff *skb, struct net_bridge *br,
 			      u16 vid, struct net_bridge_port *p);
 void br_do_suppress_nd(struct sk_buff *skb, struct net_bridge *br,
 		       u16 vid, struct net_bridge_port *p, struct nd_msg *msg);
-struct nd_msg *br_is_nd_neigh_msg(const struct sk_buff *skb, struct nd_msg *m);
+struct nd_msg *br_is_nd_neigh_msg(const struct sk_buff *skb);
 bool br_is_neigh_suppress_enabled(const struct net_bridge_port *p, u16 vid);
 bool br_is_neigh_forward_grat_enabled(const struct net_bridge_port *p, u16 vid);
 #endif
-- 
2.54.0

[PATCH net-next v2 2/5] ipv6: ndisc: Add ndisc_check_ns_na() validation helper

[Danielle Ratson]

Add ndisc_check_ns_na(), a standalone NS/NA packet validator modeled
after ipv6_mc_check_mld(). It performs the RFC 4861 section 7.1.1
(Neighbor Solicitation) and 7.1.2 (Neighbor Advertisement) mandatory
checks that are relevant for software operating at the bridge level,
where packets bypass the normal IPv6 stack path:

 - Hop Limit must be 255 (packet was not forwarded by a router)
 - ICMPv6 checksum is valid
 - ICMP Code is 0
 - ICMP length is at least 24 octets (sizeof(struct nd_msg))
 - Target Address must not be a multicast address
 - All included options have a length that is greater than zero
 - NS/DAD: destination must be a solicited-node multicast address
 - NS/DAD: no Source Link-Layer Address option when source is unspecified
 - NA: Solicited flag must be 0 when IP Destination is multicast

On success the function sets the skb transport header and returns 0,
matching the convention of ipv6_mc_check_mld().

Reviewed-by: Petr Machata <petrm@nvidia.com>
Acked-by: Nikolay Aleksandrov <razor@blackwall.org>
Signed-off-by: Danielle Ratson <danieller@nvidia.com>
---

Notes:
    v2:
    	* Use EXPORT_SYMBOL_GPL() instead of EXPORT_SYMBOL() to export
    	  ndisc_check_ns_na().

 include/net/ndisc.h    |   2 +
 net/ipv6/Makefile      |   2 +-
 net/ipv6/ndisc_snoop.c | 190 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 193 insertions(+), 1 deletion(-)
 create mode 100644 net/ipv6/ndisc_snoop.c

diff --git a/include/net/ndisc.h b/include/net/ndisc.h
index 3da1a6f8d3f9..9e5379ad2d8e 100644
--- a/include/net/ndisc.h
+++ b/include/net/ndisc.h
@@ -430,6 +430,8 @@ void ndisc_update(const struct net_device *dev, struct neighbour *neigh,
 		  const u8 *lladdr, u8 new, u32 flags, u8 icmp6_type,
 		  struct ndisc_options *ndopts);
 
+int ndisc_check_ns_na(struct sk_buff *skb);
+
 /*
  *	IGMP
  */
diff --git a/net/ipv6/Makefile b/net/ipv6/Makefile
index 5b0cd6488021..cf5e01f83ce3 100644
--- a/net/ipv6/Makefile
+++ b/net/ipv6/Makefile
@@ -51,7 +51,7 @@ obj-$(subst m,y,$(CONFIG_IPV6)) += inet6_hashtables.o
 
 ifneq ($(CONFIG_IPV6),)
 obj-$(CONFIG_NET_UDP_TUNNEL) += ip6_udp_tunnel.o
-obj-y += mcast_snoop.o
+obj-y += mcast_snoop.o ndisc_snoop.o
 obj-$(CONFIG_TCP_AO) += tcp_ao.o
 endif
 
diff --git a/net/ipv6/ndisc_snoop.c b/net/ipv6/ndisc_snoop.c
new file mode 100644
index 000000000000..fa86528d5cfe
--- /dev/null
+++ b/net/ipv6/ndisc_snoop.c
@@ -0,0 +1,190 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <linux/skbuff.h>
+#include <net/addrconf.h>
+#include <net/ip6_checksum.h>
+#include <net/ipv6.h>
+#include <net/ndisc.h>
+
+static int ndisc_check_ip6hdr(struct sk_buff *skb)
+{
+	const struct ipv6hdr *ip6h;
+	unsigned int offset, len;
+
+	offset = skb_network_offset(skb) + sizeof(*ip6h);
+	if (!pskb_may_pull(skb, offset))
+		return -EINVAL;
+
+	ip6h = ipv6_hdr(skb);
+
+	if (ip6h->version != 6)
+		return -EINVAL;
+
+	if (ip6h->nexthdr != IPPROTO_ICMPV6)
+		return -ENOMSG;
+
+	/* RFC 4861 7.1.1 / 7.1.2: must not have been forwarded by a router */
+	if (ip6h->hop_limit != 255)
+		return -EINVAL;
+
+	len = offset + ntohs(ip6h->payload_len);
+	if (skb->len < len || len <= offset)
+		return -EINVAL;
+
+	skb_set_transport_header(skb, offset);
+
+	return 0;
+}
+
+static __sum16 ndisc_validate_checksum(struct sk_buff *skb)
+{
+	return skb_checksum_validate(skb, IPPROTO_ICMPV6, ip6_compute_pseudo);
+}
+
+static int ndisc_check_icmpv6(struct sk_buff *skb)
+{
+	unsigned int len = skb_transport_offset(skb) + sizeof(struct icmp6hdr);
+	unsigned int transport_len = ipv6_transport_len(skb);
+	struct sk_buff *skb_chk;
+	struct icmp6hdr *hdr;
+
+	if (!pskb_may_pull(skb, len))
+		return -EINVAL;
+
+	/* RFC 4861 7.1.1 / 7.1.2: the ICMPv6 checksum must be valid */
+	skb_chk = skb_checksum_trimmed(skb, transport_len,
+				       ndisc_validate_checksum);
+	if (!skb_chk)
+		return -EINVAL;
+
+	if (skb_chk != skb)
+		kfree_skb(skb_chk);
+
+	/* RFC 4861 7.1.1 / 7.1.2: Code must be 0 */
+	hdr = (struct icmp6hdr *)skb_transport_header(skb);
+	if (hdr->icmp6_code != 0)
+		return -EINVAL;
+
+	return 0;
+}
+
+static int ndisc_check_options(struct sk_buff *skb, unsigned int opts_len,
+			       bool reject_slla)
+{
+	unsigned int offset = skb_transport_offset(skb) + sizeof(struct nd_msg);
+	struct nd_opt_hdr *opt, _opt;
+
+	while (opts_len > 0) {
+		if (opts_len < sizeof(*opt))
+			return -EINVAL;
+
+		opt = skb_header_pointer(skb, offset, sizeof(_opt), &_opt);
+		if (!opt)
+			return -EINVAL;
+
+		/* RFC 4861 7.1.1 / 7.1.2: all option lengths must be > 0 */
+		if (!opt->nd_opt_len)
+			return -EINVAL;
+
+		/* RFC 4861 7.1.1: DAD NS must not contain a source link-layer
+		 * address option
+		 */
+		if (reject_slla && opt->nd_opt_type == ND_OPT_SOURCE_LL_ADDR)
+			return -EINVAL;
+
+		if (opt->nd_opt_len * 8 > opts_len)
+			return -EINVAL;
+
+		offset += opt->nd_opt_len * 8;
+		opts_len -= opt->nd_opt_len * 8;
+	}
+
+	return 0;
+}
+
+static int ndisc_check_nd_msg(struct sk_buff *skb)
+{
+	unsigned int len = skb_transport_offset(skb) + sizeof(struct nd_msg);
+	unsigned int transport_len = ipv6_transport_len(skb);
+	bool reject_slla = false;
+	const struct nd_msg *msg;
+
+	if (!pskb_may_pull(skb, len))
+		return -EINVAL;
+
+	/* RFC 4861 7.1.1 / 7.1.2: ICMP length is at least sizeof(nd_msg) */
+	if (transport_len < sizeof(struct nd_msg))
+		return -EINVAL;
+
+	msg = (struct nd_msg *)skb_transport_header(skb);
+
+	/* RFC 4861 7.1.1 / 7.1.2: Target Address must not be a
+	 * multicast address
+	 */
+	if (ipv6_addr_is_multicast(&msg->target))
+		return -EINVAL;
+
+	switch (msg->icmph.icmp6_type) {
+	case NDISC_NEIGHBOUR_SOLICITATION:
+		if (ipv6_addr_any(&ipv6_hdr(skb)->saddr)) {
+			/* RFC 4861 7.1.1: DAD NS destination must be a
+			 * solicited-node multicast address
+			 */
+			if (!ipv6_addr_is_solict_mult(&ipv6_hdr(skb)->daddr))
+				return -EINVAL;
+			/* RFC 4861 7.1.1: DAD NS must not contain a source
+			 * link-layer address option
+			 */
+			reject_slla = true;
+		}
+		break;
+	case NDISC_NEIGHBOUR_ADVERTISEMENT:
+		/* RFC 4861 7.1.2: Solicited flag must be 0 for
+		 * multicast destinations
+		 */
+		if (ipv6_addr_is_multicast(&ipv6_hdr(skb)->daddr) &&
+		    msg->icmph.icmp6_solicited)
+			return -EINVAL;
+		break;
+	default:
+		return -ENODATA;
+	}
+
+	return ndisc_check_options(skb, transport_len - sizeof(struct nd_msg),
+				   reject_slla);
+}
+
+/**
+ * ndisc_check_ns_na - validate an NS/NA packet and set its transport header
+ * @skb: the skb to validate
+ *
+ * Validates an IPv6 packet for compliance with RFC 4861 sections 7.1.1
+ * (Neighbor Solicitation) and 7.1.2 (Neighbor Advertisement). If valid,
+ * sets the skb transport header.
+ *
+ * Caller needs to set the skb network header.
+ *
+ * Return:
+ * * 0        - valid NS/NA; the skb transport header has been set.
+ * * -EINVAL  - a broken packet was detected, i.e. it violates some
+ *              internet standard.
+ * * -ENOMSG  - IP header validation succeeded but it is not an ICMPv6
+ *              packet.
+ * * -ENODATA - IP+ICMPv6 header validation succeeded but it is not a
+ *              Neighbor Solicitation or Neighbor Advertisement.
+ */
+int ndisc_check_ns_na(struct sk_buff *skb)
+{
+	int ret;
+
+	ret = ndisc_check_ip6hdr(skb);
+	if (ret < 0)
+		return ret;
+
+	ret = ndisc_check_icmpv6(skb);
+	if (ret < 0)
+		return ret;
+
+	return ndisc_check_nd_msg(skb);
+}
+EXPORT_SYMBOL_GPL(ndisc_check_ns_na);
-- 
2.54.0

[PATCH net-next v2 3/5] bridge: Validate NS/NA messages using ndisc_check_ns_na()

[Danielle Ratson]

The bridge performs neighbor suppression by snooping NS/NA messages, but
previously only checked the ICMPv6 type and code. This leaves it open to
acting on malformed or spoofed packets that any RFC-compliant node should
reject.

Wire br_is_nd_neigh_msg() into the new ndisc_check_ns_na() helper, which
enforces the full RFC 4861 section 7.1.1/7.1.2 receive validation:
hop limit of 255, valid checksum, correct code, and type-specific rules
(NS target not multicast; NA solicited flag clear for multicast
destinations).

MLD messages are already validated by ipv6_mc_check_mld() before the
bridge acts on them; this brings NS/NA to the same standard.

As a side effect, the skb parameter of br_is_nd_neigh_msg() changes from
const to non-const, since ndisc_check_ns_na() may reallocate the skb head
via pskb_may_pull() and sets the transport header. The returned pointer is
now derived from skb_transport_header() rather than a direct cast.

Reviewed-by: Petr Machata <petrm@nvidia.com>
Acked-by: Nikolay Aleksandrov <razor@blackwall.org>
Signed-off-by: Danielle Ratson <danieller@nvidia.com>
---
 net/bridge/br_arp_nd_proxy.c | 11 ++++-------
 net/bridge/br_private.h      |  2 +-
 2 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/net/bridge/br_arp_nd_proxy.c b/net/bridge/br_arp_nd_proxy.c
index db08c3272001..445c930ed59b 100644
--- a/net/bridge/br_arp_nd_proxy.c
+++ b/net/bridge/br_arp_nd_proxy.c
@@ -19,6 +19,7 @@
 #include <net/addrconf.h>
 #if IS_ENABLED(CONFIG_IPV6)
 #include <net/ip6_checksum.h>
+#include <net/ndisc.h>
 #endif
 
 #include "br_private.h"
@@ -234,16 +235,12 @@ void br_do_proxy_suppress_arp(struct sk_buff *skb, struct net_bridge *br,
 #endif
 
 #if IS_ENABLED(CONFIG_IPV6)
-struct nd_msg *br_is_nd_neigh_msg(const struct sk_buff *skb)
+struct nd_msg *br_is_nd_neigh_msg(struct sk_buff *skb)
 {
-	struct nd_msg *m = (struct nd_msg *)(ipv6_hdr(skb) + 1);
-
-	if (m->icmph.icmp6_code != 0 ||
-	    (m->icmph.icmp6_type != NDISC_NEIGHBOUR_SOLICITATION &&
-	     m->icmph.icmp6_type != NDISC_NEIGHBOUR_ADVERTISEMENT))
+	if (ndisc_check_ns_na(skb))
 		return NULL;
 
-	return m;
+	return (struct nd_msg *)skb_transport_header(skb);
 }
 
 static void br_nd_send(struct net_bridge *br, struct net_bridge_port *p,
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index e31439cb4420..d337b1cfb980 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -2367,7 +2367,7 @@ void br_do_proxy_suppress_arp(struct sk_buff *skb, struct net_bridge *br,
 			      u16 vid, struct net_bridge_port *p);
 void br_do_suppress_nd(struct sk_buff *skb, struct net_bridge *br,
 		       u16 vid, struct net_bridge_port *p, struct nd_msg *msg);
-struct nd_msg *br_is_nd_neigh_msg(const struct sk_buff *skb);
+struct nd_msg *br_is_nd_neigh_msg(struct sk_buff *skb);
 bool br_is_neigh_suppress_enabled(const struct net_bridge_port *p, u16 vid);
 bool br_is_neigh_forward_grat_enabled(const struct net_bridge_port *p, u16 vid);
 #endif
-- 
2.54.0

[PATCH net-next v2 4/5] bridge: Linearize skb once the ND message type is validated

[Danielle Ratson]

br_nd_send() parses ND options from ns->opt[] and therefore needs the skb
to be linear. Commit a01aee7cafc5 ("bridge: br_nd_send: linearize skb
before parsing ND options") ensured that by linearizing inside
br_nd_send() itself.

Move the linearization up into br_is_nd_neigh_msg(), right after
ndisc_check_ns_na() has validated the message as an NS/NA. This makes a
linear buffer a property of every recognized ND message, so that this and
any future ND message handling operate on a linear skb and cannot
reintroduce that class of bug by forgetting to linearize.

Since the skb is now linear by the time br_nd_send() runs, drop the
linearization there and derive ns from the transport header set by
ndisc_check_ns_na(), instead of recomputing it from the network header.

If linearization fails under memory pressure, br_is_nd_neigh_msg() returns
NULL and the packet falls back to normal forwarding rather than being
suppressed.

Reviewed-by: Petr Machata <petrm@nvidia.com>
Signed-off-by: Danielle Ratson <danieller@nvidia.com>
---

Notes:
    v2:
    	* Add a comment noting that br_is_nd_neigh_msg() also linearizes the
    	  skb.

 net/bridge/br_arp_nd_proxy.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/net/bridge/br_arp_nd_proxy.c b/net/bridge/br_arp_nd_proxy.c
index 445c930ed59b..6b6de0eff38c 100644
--- a/net/bridge/br_arp_nd_proxy.c
+++ b/net/bridge/br_arp_nd_proxy.c
@@ -235,11 +235,17 @@ void br_do_proxy_suppress_arp(struct sk_buff *skb, struct net_bridge *br,
 #endif
 
 #if IS_ENABLED(CONFIG_IPV6)
+/* Validate skb as an NS/NA and linearize it for br_nd_send()'s ND
+ * option parsing; returns the nd_msg, or NULL on failure.
+ */
 struct nd_msg *br_is_nd_neigh_msg(struct sk_buff *skb)
 {
 	if (ndisc_check_ns_na(skb))
 		return NULL;
 
+	if (skb_linearize(skb))
+		return NULL;
+
 	return (struct nd_msg *)skb_transport_header(skb);
 }
 
@@ -259,7 +265,7 @@ static void br_nd_send(struct net_bridge *br, struct net_bridge_port *p,
 	bool dad;
 	u16 pvid;
 
-	if (!dev || skb_linearize(request))
+	if (!dev)
 		return;
 
 	len = LL_RESERVED_SPACE(dev) + sizeof(struct ipv6hdr) +
@@ -276,8 +282,7 @@ static void br_nd_send(struct net_bridge *br, struct net_bridge_port *p,
 	skb_set_mac_header(reply, 0);
 
 	daddr = eth_hdr(request)->h_source;
-	ns = (struct nd_msg *)(skb_network_header(request) +
-			       sizeof(struct ipv6hdr));
+	ns = (struct nd_msg *)skb_transport_header(request);
 
 	/* Do we need option processing ? */
 	ns_olen = request->len - (skb_network_offset(request) +
-- 
2.54.0

[PATCH net-next v2 5/5] bridge: Use ndisc_parse_options() to parse ND options in br_nd_send()

[Danielle Ratson]

Replace the manual ND option parsing loop in br_nd_send() with
ndisc_parse_options(), which provides proper validation and avoids the
class of bugs that were fixed by commit 53fc685243bd ("bridge: Avoid
infinite loop when suppressing NS messages with invalid options") and
commit 850837965af1 ("bridge: br_nd_send: validate ND option lengths").

Use ndisc_opt_addr_data() to extract the source link-layer address
from the parsed options, which correctly validates the option length
for the underlying device type.

Export ndisc_parse_options() so that it can be resolved from the bridge
when it is built as a module (CONFIG_BRIDGE=m); otherwise modpost fails
with an undefined symbol.

Reviewed-by: Petr Machata <petrm@nvidia.com>
Acked-by: Nikolay Aleksandrov <razor@blackwall.org>
Signed-off-by: Danielle Ratson <danieller@nvidia.com>
---

Notes:
    v2:
    	* Use EXPORT_SYMBOL_GPL() instead of EXPORT_SYMBOL() to export
    	  ndisc_parse_options().

 net/bridge/br_arp_nd_proxy.c | 32 +++++++++++++++++---------------
 net/ipv6/ndisc.c             |  1 +
 2 files changed, 18 insertions(+), 15 deletions(-)

diff --git a/net/bridge/br_arp_nd_proxy.c b/net/bridge/br_arp_nd_proxy.c
index 6b6de0eff38c..b6e5a86b6a92 100644
--- a/net/bridge/br_arp_nd_proxy.c
+++ b/net/bridge/br_arp_nd_proxy.c
@@ -255,15 +255,16 @@ static void br_nd_send(struct net_bridge *br, struct net_bridge_port *p,
 {
 	struct net_device *dev = request->dev;
 	struct net_bridge_vlan_group *vg;
+	struct ndisc_options ndopts;
 	struct nd_msg *na, *ns;
 	struct sk_buff *reply;
 	struct ipv6hdr *pip6;
 	int na_olen = 8; /* opt hdr + ETH_ALEN for target */
 	int ns_olen;
-	int i, len;
 	u8 *daddr;
 	bool dad;
 	u16 pvid;
+	int len;
 
 	if (!dev)
 		return;
@@ -284,20 +285,21 @@ static void br_nd_send(struct net_bridge *br, struct net_bridge_port *p,
 	daddr = eth_hdr(request)->h_source;
 	ns = (struct nd_msg *)skb_transport_header(request);
 
-	/* Do we need option processing ? */
-	ns_olen = request->len - (skb_network_offset(request) +
-				  sizeof(struct ipv6hdr)) - sizeof(*ns);
-	for (i = 0; i < ns_olen - 1; i += (ns->opt[i + 1] << 3)) {
-		if (!ns->opt[i + 1] || i + (ns->opt[i + 1] << 3) > ns_olen) {
-			kfree_skb(reply);
-			return;
-		}
-		if (ns->opt[i] == ND_OPT_SOURCE_LL_ADDR) {
-			if ((ns->opt[i + 1] << 3) >=
-			    sizeof(struct nd_opt_hdr) + ETH_ALEN)
-				daddr = ns->opt + i + sizeof(struct nd_opt_hdr);
-			break;
-		}
+	/* Derive the option length from the IPv6 payload length so that any
+	 * trailing L2 padding in the skb is not parsed as ND options.
+	 */
+	ns_olen = ntohs(ipv6_hdr(request)->payload_len) - sizeof(*ns);
+	if (!ndisc_parse_options(dev, ns->opt, ns_olen, &ndopts)) {
+		kfree_skb(reply);
+		return;
+	}
+
+	if (ndopts.nd_opts_src_lladdr) {
+		u8 *lladdr;
+
+		lladdr = ndisc_opt_addr_data(ndopts.nd_opts_src_lladdr, dev);
+		if (lladdr)
+			daddr = lladdr;
 	}
 
 	dad = ipv6_addr_any(&ipv6_hdr(request)->saddr);
diff --git a/net/ipv6/ndisc.c b/net/ipv6/ndisc.c
index fe36b3f51285..2ceb655c4229 100644
--- a/net/ipv6/ndisc.c
+++ b/net/ipv6/ndisc.c
@@ -283,6 +283,7 @@ struct ndisc_options *ndisc_parse_options(const struct net_device *dev,
 	}
 	return ndopts;
 }
+EXPORT_SYMBOL_GPL(ndisc_parse_options);
 
 int ndisc_mc_map(const struct in6_addr *addr, char *buf, struct net_device *dev, int dir)
 {
-- 
2.54.0

Re: [PATCH net-next v2 4/5] bridge: Linearize skb once the ND message type is validated

[Nikolay Aleksandrov]

On Mon, Aug 03, 2026 at 02:25:04PM +0300, Danielle Ratson wrote:
> br_nd_send() parses ND options from ns->opt[] and therefore needs the skb
> to be linear. Commit a01aee7cafc5 ("bridge: br_nd_send: linearize skb
> before parsing ND options") ensured that by linearizing inside
> br_nd_send() itself.
> 
> Move the linearization up into br_is_nd_neigh_msg(), right after
> ndisc_check_ns_na() has validated the message as an NS/NA. This makes a
> linear buffer a property of every recognized ND message, so that this and
> any future ND message handling operate on a linear skb and cannot
> reintroduce that class of bug by forgetting to linearize.
> 
> Since the skb is now linear by the time br_nd_send() runs, drop the
> linearization there and derive ns from the transport header set by
> ndisc_check_ns_na(), instead of recomputing it from the network header.
> 
> If linearization fails under memory pressure, br_is_nd_neigh_msg() returns
> NULL and the packet falls back to normal forwarding rather than being
> suppressed.
> 
> Reviewed-by: Petr Machata <petrm@nvidia.com>
> Signed-off-by: Danielle Ratson <danieller@nvidia.com>
> ---
> 
> Notes:
>     v2:
>     	* Add a comment noting that br_is_nd_neigh_msg() also linearizes the
>     	  skb.
> 
>  net/bridge/br_arp_nd_proxy.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/net/bridge/br_arp_nd_proxy.c b/net/bridge/br_arp_nd_proxy.c
> index 445c930ed59b..6b6de0eff38c 100644
> --- a/net/bridge/br_arp_nd_proxy.c
> +++ b/net/bridge/br_arp_nd_proxy.c
> @@ -235,11 +235,17 @@ void br_do_proxy_suppress_arp(struct sk_buff *skb, struct net_bridge *br,
>  #endif
>  
>  #if IS_ENABLED(CONFIG_IPV6)
> +/* Validate skb as an NS/NA and linearize it for br_nd_send()'s ND
> + * option parsing; returns the nd_msg, or NULL on failure.
> + */
>  struct nd_msg *br_is_nd_neigh_msg(struct sk_buff *skb)
>  {
>  	if (ndisc_check_ns_na(skb))
>  		return NULL;
>  
> +	if (skb_linearize(skb))
> +		return NULL;
> +
>  	return (struct nd_msg *)skb_transport_header(skb);
>  }
>  
> @@ -259,7 +265,7 @@ static void br_nd_send(struct net_bridge *br, struct net_bridge_port *p,
>  	bool dad;
>  	u16 pvid;
>  
> -	if (!dev || skb_linearize(request))
> +	if (!dev)
>  		return;
>  
>  	len = LL_RESERVED_SPACE(dev) + sizeof(struct ipv6hdr) +
> @@ -276,8 +282,7 @@ static void br_nd_send(struct net_bridge *br, struct net_bridge_port *p,
>  	skb_set_mac_header(reply, 0);
>  
>  	daddr = eth_hdr(request)->h_source;
> -	ns = (struct nd_msg *)(skb_network_header(request) +
> -			       sizeof(struct ipv6hdr));
> +	ns = (struct nd_msg *)skb_transport_header(request);
>  
>  	/* Do we need option processing ? */
>  	ns_olen = request->len - (skb_network_offset(request) +
> -- 
> 2.54.0
> 

Thanks,
Acked-by: Nikolay Aleksandrov <razor@blackwall.org>

RE: [PATCH net-next v2 1/5] bridge: Use direct pointer in br_is_nd_neigh_msg()

[Danielle Ratson]

> -----Original Message-----
> From: Danielle Ratson <danieller@nvidia.com>
> Sent: Monday, 3 August 2026 14:25
> To: netdev@vger.kernel.org
> Cc: dsahern@kernel.org; Ido Schimmel <idosch@nvidia.com>;
> davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
> pabeni@redhat.com; horms@kernel.org; razor@blackwall.org; ja@ssi.bg; Petr
> Machata <petrm@nvidia.com>; fw@strlen.de; kuniyu@google.com;
> bridge@lists.linux.dev; linux-kernel@vger.kernel.org; Danielle Ratson
> <danieller@nvidia.com>
> Subject: [PATCH net-next v2 1/5] bridge: Use direct pointer in
> br_is_nd_neigh_msg()
> 
> Both callers of br_is_nd_neigh_msg() already call pskb_may_pull() to ensure
> sizeof(struct ipv6hdr) + sizeof(struct nd_msg) bytes are in the linear area
> before invoking this function. The skb_header_pointer() call and its fallback
> buffer are therefore unnecessary.
> 
> Replace skb_header_pointer() with a direct cast to ipv6_hdr(skb) + 1 and drop
> the now-unused 'msg' parameter and its corresponding stack buffer from all
> callers.
> 
> Reviewed-by: Petr Machata <petrm@nvidia.com>
> Acked-by: Nikolay Aleksandrov <razor@blackwall.org>
> Signed-off-by: Danielle Ratson <danieller@nvidia.com>
> ---

Sashiko wrote:

"
> diff --git a/net/bridge/br_arp_nd_proxy.c b/net/bridge/br_arp_nd_proxy.c
> index 23eb6931a2b4a..db08c3272001f 100644
> --- a/net/bridge/br_arp_nd_proxy.c
> +++ b/net/bridge/br_arp_nd_proxy.c

This is a pre-existing issue, but can br_do_proxy_suppress_arp() also suffer
from an out-of-bounds read when skb_network_offset(skb) > 0?
In br_do_proxy_suppress_arp(), pskb_may_pull() is used to validate
arp_hdr_len(dev) relative to skb->data:
        if ((dev->flags & IFF_NOARP) ||
            !pskb_may_pull(skb, arp_hdr_len(dev)))
                return;
        parp = arp_hdr(skb);

Since arp_hdr(skb) resolves to skb_network_header(skb), accessing the arp
payload reads beyond the validated linear area if the network offset is
greater than 0. This could happen if a bpf program at traffic control egress
uses bpf_skb_adjust_room to shift the network header relative to skb->data
before bridge processing.
"

Pre-existing issue, not in the scope.

"
> @@ -234,14 +234,9 @@ void br_do_proxy_suppress_arp(struct sk_buff *skb, struct net_bridge *br,
>  #endif
>  
>  #if IS_ENABLED(CONFIG_IPV6)
> -struct nd_msg *br_is_nd_neigh_msg(const struct sk_buff *skb, struct nd_msg *msg)
> +struct nd_msg *br_is_nd_neigh_msg(const struct sk_buff *skb)
>  {
> -	struct nd_msg *m;
> -
> -	m = skb_header_pointer(skb, skb_network_offset(skb) +
> -			       sizeof(struct ipv6hdr), sizeof(*msg), msg);
> -	if (!m)
> -		return NULL;
> +	struct nd_msg *m = (struct nd_msg *)(ipv6_hdr(skb) + 1);

Does this cast introduce an out-of-bounds read if skb_network_offset(skb) > 0?
The callers use pskb_may_pull() to validate the length relative to skb->data.
However, ipv6_hdr(skb) resolves to skb_network_header(skb), which is
skb->data + skb_network_offset(skb).
If a packet is sent through the bridge device where skb_network_offset(skb)
is greater than 0 (such as when a bpf program at traffic control egress
expands the mac header), the bridge's unconditional pull of the ethernet
header leaves skb->data before the network header.
In this situation, the direct pointer cast evaluates to a memory address that
requires:
skb_network_offset(skb) + sizeof(struct ipv6hdr) + sizeof(struct nd_msg)
linear bytes, but pskb_may_pull() only validated sizeof(struct ipv6hdr) +
sizeof(struct nd_msg) relative to skb->data.
”

Patch #3 supersedes the cast, no bug in the assembled series.

RE: [PATCH net-next v2 2/5] ipv6: ndisc: Add ndisc_check_ns_na() validation helper

[Danielle Ratson]

> -----Original Message-----
> From: Danielle Ratson <danieller@nvidia.com>
> Sent: Monday, 3 August 2026 14:25
> To: netdev@vger.kernel.org
> Cc: dsahern@kernel.org; Ido Schimmel <idosch@nvidia.com>;
> davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
> pabeni@redhat.com; horms@kernel.org; razor@blackwall.org; ja@ssi.bg; Petr
> Machata <petrm@nvidia.com>; fw@strlen.de; kuniyu@google.com;
> bridge@lists.linux.dev; linux-kernel@vger.kernel.org; Danielle Ratson
> <danieller@nvidia.com>
> Subject: [PATCH net-next v2 2/5] ipv6: ndisc: Add ndisc_check_ns_na()
> validation helper
> 
> Add ndisc_check_ns_na(), a standalone NS/NA packet validator modeled after
> ipv6_mc_check_mld(). It performs the RFC 4861 section 7.1.1 (Neighbor
> Solicitation) and 7.1.2 (Neighbor Advertisement) mandatory checks that are
> relevant for software operating at the bridge level, where packets bypass the
> normal IPv6 stack path:
> 
>  - Hop Limit must be 255 (packet was not forwarded by a router)
>  - ICMPv6 checksum is valid
>  - ICMP Code is 0
>  - ICMP length is at least 24 octets (sizeof(struct nd_msg))
>  - Target Address must not be a multicast address
>  - All included options have a length that is greater than zero
>  - NS/DAD: destination must be a solicited-node multicast address
>  - NS/DAD: no Source Link-Layer Address option when source is unspecified
>  - NA: Solicited flag must be 0 when IP Destination is multicast
> 
> On success the function sets the skb transport header and returns 0, matching
> the convention of ipv6_mc_check_mld().
> 
> Reviewed-by: Petr Machata <petrm@nvidia.com>
> Acked-by: Nikolay Aleksandrov <razor@blackwall.org>
> Signed-off-by: Danielle Ratson <danieller@nvidia.com>
> ---

Sashiko wrote:

" 
--- Patch [2]: [PATCH net-next v2 2/5] ipv6: ndisc: Add ndisc_check_ns_na() validation helper ---
commit 4031e051060238d9c59321eb3171c00634174182
Author: Danielle Ratson <danieller@nvidia.com>
ipv6: ndisc: Add ndisc_check_ns_na() validation helper
This commit adds ndisc_check_ns_na(), a standalone validator for IPv6
Neighbor Solicitation and Neighbor Advertisement packets. It performs
mandatory RFC 4861 checks for software operating at the bridge level.
> diff --git a/net/ipv6/ndisc_snoop.c b/net/ipv6/ndisc_snoop.c
> new file mode 100644
> index 0000000000000..fa86528d5cfee
> --- /dev/null
> +++ b/net/ipv6/ndisc_snoop.c
> @@ -0,0 +1,190 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include <linux/skbuff.h>
> +#include <net/addrconf.h>
> +#include <net/ip6_checksum.h>
> +#include <net/ipv6.h>
> +#include <net/ndisc.h>
> +
> +static int ndisc_check_ip6hdr(struct sk_buff *skb)
> +{
> +	const struct ipv6hdr *ip6h;
> +	unsigned int offset, len;
> +
> +	offset = skb_network_offset(skb) + sizeof(*ip6h);
> +	if (!pskb_may_pull(skb, offset))
> +		return -EINVAL;
> +
> +	ip6h = ipv6_hdr(skb);
> +
> +	if (ip6h->version != 6)
> +		return -EINVAL;
> +
> +	if (ip6h->nexthdr != IPPROTO_ICMPV6)
> +		return -ENOMSG;

Does this strict check allow attackers to bypass bridge-level ND security
mechanisms using IPv6 extension headers?
If a spoofed Neighbor Solicitation or Advertisement packet is sent with a
valid IPv6 extension header (like Destination Options), this check will 
return -ENOMSG. A bridge would then likely treat the packet as a non-ND
message and forward it normally, bypassing security mechanisms like
ND snooping or anti-spoofing.
The target host's IPv6 stack will traverse the extension headers and
successfully process the malicious ND payload. Should this use
ipv6_skip_exthdr() to properly traverse to the ICMPv6 header instead?
"

The callers only reach this after checking ipv6_hdr(skb)->nexthdr == IPPROTO_ICMPV6, so a packet with an extension header (nexthdr != ICMPV6) never gets here in the first place.
And if such a packet were ever skipped, it's just not suppressed and floods normally (the default without this feature), so there's no bypass. 

RE: [PATCH net-next v2 5/5] bridge: Use ndisc_parse_options() to parse ND options in br_nd_send()

[Danielle Ratson]

> -----Original Message-----
> From: Danielle Ratson <danieller@nvidia.com>
> Sent: Monday, 3 August 2026 14:25
> To: netdev@vger.kernel.org
> Cc: dsahern@kernel.org; Ido Schimmel <idosch@nvidia.com>;
> davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
> pabeni@redhat.com; horms@kernel.org; razor@blackwall.org; ja@ssi.bg; Petr
> Machata <petrm@nvidia.com>; fw@strlen.de; kuniyu@google.com;
> bridge@lists.linux.dev; linux-kernel@vger.kernel.org; Danielle Ratson
> <danieller@nvidia.com>
> Subject: [PATCH net-next v2 5/5] bridge: Use ndisc_parse_options() to parse
> ND options in br_nd_send()
> 
> Replace the manual ND option parsing loop in br_nd_send() with
> ndisc_parse_options(), which provides proper validation and avoids the class
> of bugs that were fixed by commit 53fc685243bd ("bridge: Avoid infinite loop
> when suppressing NS messages with invalid options") and commit
> 850837965af1 ("bridge: br_nd_send: validate ND option lengths").
> 
> Use ndisc_opt_addr_data() to extract the source link-layer address from the
> parsed options, which correctly validates the option length for the underlying
> device type.
> 
> Export ndisc_parse_options() so that it can be resolved from the bridge when
> it is built as a module (CONFIG_BRIDGE=m); otherwise modpost fails with an
> undefined symbol.
> 
> Reviewed-by: Petr Machata <petrm@nvidia.com>
> Acked-by: Nikolay Aleksandrov <razor@blackwall.org>
> Signed-off-by: Danielle Ratson <danieller@nvidia.com>
> ---

Sashiko wrote: 

"
--- Patch [5]: [PATCH net-next v2 5/5] bridge: Use ndisc_parse_options() to parse ND options in br_nd_send() ---
commit a5e722187a4312d62366c8f19772265d9169d89e
Author: Danielle Ratson <danieller@nvidia.com>
bridge: Use ndisc_parse_options() to parse ND options in br_nd_send()
This commit replaces the manual ND option parsing loop in br_nd_send()
with ndisc_parse_options() to provide proper validation. It also exports
ndisc_parse_options() from the IPv6 module so it can be called from the
bridge subsystem.
> diff --git a/net/bridge/br_arp_nd_proxy.c b/net/bridge/br_arp_nd_proxy.c
> --- a/net/bridge/br_arp_nd_proxy.c
> +++ b/net/bridge/br_arp_nd_proxy.c
[ ... ]
> @@ -284,20 +285,21 @@ static void br_nd_send(struct net_bridge *br, struct net_bridge_port *p,
>  	daddr = eth_hdr(request)->h_source;
>  	ns = (struct nd_msg *)skb_transport_header(request);
>  
> -	/* Do we need option processing ? */
> -	ns_olen = request->len - (skb_network_offset(request) +
> -				  sizeof(struct ipv6hdr)) - sizeof(*ns);
> -	for (i = 0; i < ns_olen - 1; i += (ns->opt[i + 1] << 3)) {
> -		if (!ns->opt[i + 1] || i + (ns->opt[i + 1] << 3) > ns_olen) {
> -			kfree_skb(reply);
> -			return;
> -		}
> -		if (ns->opt[i] == ND_OPT_SOURCE_LL_ADDR) {
> -			if ((ns->opt[i + 1] << 3) >=
> -			    sizeof(struct nd_opt_hdr) + ETH_ALEN)
> -				daddr = ns->opt + i + sizeof(struct nd_opt_hdr);
> -			break;
> -		}
> +	/* Derive the option length from the IPv6 payload length so that any
> +	 * trailing L2 padding in the skb is not parsed as ND options.
> +	 */
> +	ns_olen = ntohs(ipv6_hdr(request)->payload_len) - sizeof(*ns);
> +	if (!ndisc_parse_options(dev, ns->opt, ns_olen, &ndopts)) {

Will this cause a link-time build failure when CONFIG_BRIDGE=y and
CONFIG_IPV6=m?
The commit message mentions exporting ndisc_parse_options() allows it to be
resolved when the bridge is built as a module (CONFIG_BRIDGE=m). 
However, since ndisc_parse_options() is defined in net/ipv6/ndisc.c, it is
compiled into the ipv6.ko module when CONFIG_IPV6=m. If the bridge subsystem
is built in (CONFIG_BRIDGE=y), vmlinux won't be able to resolve the symbol
at link time.
"

CONFIG_IPV6 is built-in only. It's a bool, not a tristate. So it is not a possible scenario.