[PATCH nf-next 0/2] netfilter: do not assume skb->sk is inet sk

[PATCH nf-next 0/2] netfilter: do not assume skb->sk is inet sk

[Florian Westphal]

LLM reported that packet socket could be handed off to an ip tunnel,
where the now tunneled skb retains skb->sk as packet socket.

There are spots where we assume skb->sk is inet socket.  Add helpers
for this and use them where needed.  Not reproducible (no crash / out
of bound access, nft_socket just provides a 'random' result instead of
'no match').

There are more skb->sk users but I could not find bad-access scenarios
for those, they fetch valid sk fields or are restricted to input hooks.

Florian Westphal (2):
  netfilter: add nf_skb_sk helper and use it
  netfilter: add nf_sk_to_full_sk helper and use it

 include/net/netfilter/nf_socket.h | 35 +++++++++++++++++++++++++++++++
 net/ipv4/netfilter.c              |  3 ++-
 net/ipv6/netfilter.c              |  3 ++-
 net/netfilter/nft_socket.c        | 16 +++++++-------
 net/netfilter/xt_socket.c         | 26 ++++++++++++-----------
 5 files changed, 62 insertions(+), 21 deletions(-)

-- 
2.55.0

[PATCH nf-next 1/2] netfilter: add nf_skb_sk helper and use it

[Florian Westphal]

nft and xt socket matching pass skb->sk to inet_sk_transparent().
On the LOCAL_OUT path an IP tunnel transmits while preserving the
original skb owner, so a non-INET socket (e.g. PF_PACKET) can reach
these code paths.  This is reachable for nft_socket.c which does allow
LOCAL_OUT. xt_socket only permits PREROUTING/LOCAL_IN.

nf_nat does this too, but there the inet_sk_transparent() call
is explicitly restricted to the INPUT hook.

For a packet socket the INET-specific offset lands inside the
packet statistics area.

Introduce nf_skb_sk() in nf_socket.h which returns skb->sk only when
it belongs to the given net namespace and passes sk_is_inet().

While at it, replace the `sk != skb->sk` refcount test with an
explicit refcounted flag that is set when the socket is obtained
via the lookup helper (the path that takes a reference).

LLM finding, no reproducer, no out-of-bounds memory access.

Assisted-by: LLM
Reported-by: Vega <vega@nebusec.ai>
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 include/net/netfilter/nf_socket.h | 20 ++++++++++++++++++++
 net/netfilter/nft_socket.c        | 16 +++++++++-------
 net/netfilter/xt_socket.c         | 26 ++++++++++++++------------
 3 files changed, 43 insertions(+), 19 deletions(-)

diff --git a/include/net/netfilter/nf_socket.h b/include/net/netfilter/nf_socket.h
index f9d7bee9bd4e..39047c73b642 100644
--- a/include/net/netfilter/nf_socket.h
+++ b/include/net/netfilter/nf_socket.h
@@ -10,4 +10,24 @@ struct sock *nf_sk_lookup_slow_v4(struct net *net, const struct sk_buff *skb,
 struct sock *nf_sk_lookup_slow_v6(struct net *net, const struct sk_buff *skb,
 				  const struct net_device *indev);
 
+/**
+ * nf_skb_sk - Return the inet socket associated with an skb
+ * @skb: The skb to fetch sk from.
+ * @net: The network namespace the socket must belong to.
+ *
+ * On the output path, skb->sk may be non-INET when the skb is passing
+ * through an IP tunnel.
+ *
+ * Return:
+ * *sk* if it is an INET socket that belongs to @net, %NULL otherwise.
+ */
+static inline struct sock *nf_skb_sk(const struct sk_buff *skb, const struct net *net)
+{
+	struct sock *sk = skb->sk;
+
+	if (sk && net_eq(net, sock_net(sk)) && sk_is_inet(sk))
+		return sk;
+
+	return NULL;
+}
 #endif
diff --git a/net/netfilter/nft_socket.c b/net/netfilter/nft_socket.c
index 52d892e04261..ea6c2f3dc3ff 100644
--- a/net/netfilter/nft_socket.c
+++ b/net/netfilter/nft_socket.c
@@ -111,15 +111,17 @@ static void nft_socket_eval(const struct nft_expr *expr,
 			    const struct nft_pktinfo *pkt)
 {
 	const struct nft_socket *priv = nft_expr_priv(expr);
-	struct sk_buff *skb = pkt->skb;
-	struct sock *sk = skb->sk;
 	u32 *dest = &regs->data[priv->dreg];
+	struct sk_buff *skb = pkt->skb;
+	bool refcounted = false;
+	struct sock *sk;
 
-	if (sk && !net_eq(nft_net(pkt), sock_net(sk)))
-		sk = NULL;
-
-	if (!sk)
+	sk = nf_skb_sk(skb, nft_net(pkt));
+	if (!sk) {
 		sk = nft_socket_do_lookup(pkt);
+		if (sk)
+			refcounted = true;
+	}
 
 	if (!sk) {
 		regs->verdict.code = NFT_BREAK;
@@ -159,7 +161,7 @@ static void nft_socket_eval(const struct nft_expr *expr,
 	}
 
 out_put_sk:
-	if (sk != skb->sk)
+	if (refcounted)
 		sock_gen_put(sk);
 }
 
diff --git a/net/netfilter/xt_socket.c b/net/netfilter/xt_socket.c
index 811e53bee408..cd3fc333ccf1 100644
--- a/net/netfilter/xt_socket.c
+++ b/net/netfilter/xt_socket.c
@@ -49,14 +49,15 @@ static bool
 socket_match(const struct sk_buff *skb, struct xt_action_param *par,
 	     const struct xt_socket_mtinfo1 *info)
 {
+	struct sock *sk = nf_skb_sk(skb, xt_net(par));
 	struct sk_buff *pskb = (struct sk_buff *)skb;
-	struct sock *sk = skb->sk;
+	bool refcounted = false;
 
-	if (sk && !net_eq(xt_net(par), sock_net(sk)))
-		sk = NULL;
-
-	if (!sk)
+	if (!sk) {
 		sk = nf_sk_lookup_slow_v4(xt_net(par), skb, xt_in(par));
+		if (sk)
+			refcounted = true;
+	}
 
 	if (sk) {
 		bool wildcard;
@@ -79,7 +80,7 @@ socket_match(const struct sk_buff *skb, struct xt_action_param *par,
 		    transparent && sk_fullsock(sk))
 			pskb->mark = READ_ONCE(sk->sk_mark);
 
-		if (sk != skb->sk)
+		if (refcounted)
 			sock_gen_put(sk);
 
 		if (wildcard || !transparent)
@@ -110,14 +111,15 @@ static bool
 socket_mt6_v1_v2_v3(const struct sk_buff *skb, struct xt_action_param *par)
 {
 	const struct xt_socket_mtinfo1 *info = (struct xt_socket_mtinfo1 *) par->matchinfo;
+	struct sock *sk = nf_skb_sk(skb, xt_net(par));
 	struct sk_buff *pskb = (struct sk_buff *)skb;
-	struct sock *sk = skb->sk;
+	bool refcounted = false;
 
-	if (sk && !net_eq(xt_net(par), sock_net(sk)))
-		sk = NULL;
-
-	if (!sk)
+	if (!sk) {
 		sk = nf_sk_lookup_slow_v6(xt_net(par), skb, xt_in(par));
+		if (sk)
+			refcounted = true;
+	}
 
 	if (sk) {
 		bool wildcard;
@@ -140,7 +142,7 @@ socket_mt6_v1_v2_v3(const struct sk_buff *skb, struct xt_action_param *par)
 		    transparent && sk_fullsock(sk))
 			pskb->mark = READ_ONCE(sk->sk_mark);
 
-		if (sk != skb->sk)
+		if (refcounted)
 			sock_gen_put(sk);
 
 		if (wildcard || !transparent)
-- 
2.55.0

[PATCH nf-next 2/2] netfilter: add nf_sk_to_full_sk helper and use it

[Florian Westphal]

Some callers of these helpers pass skb->sk as sk_partial argument, but
for LOCAL_OUT path an IP tunnel transmits while preserving the original
skb owner, so a non-INET socket (e.g. PF_PACKET) can reach these.

Like previous patch, add a helper to also check for this case.
Related to LLM finding in the earlier patch when querying the agent about
other possible scenarios.

Assisted-by: LLM
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 include/net/netfilter/nf_socket.h | 15 +++++++++++++++
 net/ipv4/netfilter.c              |  3 ++-
 net/ipv6/netfilter.c              |  3 ++-
 3 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/include/net/netfilter/nf_socket.h b/include/net/netfilter/nf_socket.h
index 39047c73b642..f7da3dc150a9 100644
--- a/include/net/netfilter/nf_socket.h
+++ b/include/net/netfilter/nf_socket.h
@@ -3,6 +3,7 @@
 #define _NF_SOCK_H_
 
 #include <net/sock.h>
+#include <net/inet_sock.h>
 
 struct sock *nf_sk_lookup_slow_v4(struct net *net, const struct sk_buff *skb,
 				  const struct net_device *indev);
@@ -30,4 +31,18 @@ static inline struct sock *nf_skb_sk(const struct sk_buff *skb, const struct net
 
 	return NULL;
 }
+
+/**
+ * nf_sk_to_full_sk - Careful access to a full socket
+ * @sk: pointer to a socket
+ *
+ * %sk_to_full_sk for use when sk might not be an inet socket.
+ */
+static inline struct sock *nf_sk_to_full_sk(struct sock *sk)
+{
+	if (sk && sk_is_inet(sk))
+		return sk_to_full_sk(sk);
+
+	return NULL;
+}
 #endif
diff --git a/net/ipv4/netfilter.c b/net/ipv4/netfilter.c
index ce9e1bfa4259..76e1eb40b8ce 100644
--- a/net/ipv4/netfilter.c
+++ b/net/ipv4/netfilter.c
@@ -17,6 +17,7 @@
 #include <net/xfrm.h>
 #include <net/ip.h>
 #include <net/netfilter/nf_queue.h>
+#include <net/netfilter/nf_socket.h>
 
 /* route_me_harder function, used by iptable_nat, iptable_mangle + ip_queue */
 int ip_route_me_harder(struct net *net, struct sock *sk, struct sk_buff *skb, unsigned int addr_type)
@@ -30,7 +31,7 @@ int ip_route_me_harder(struct net *net, struct sock *sk, struct sk_buff *skb, un
 	struct flow_keys flkeys;
 	unsigned int hh_len;
 
-	sk = sk_to_full_sk(sk);
+	sk = nf_sk_to_full_sk(sk);
 	flags = sk ? inet_sk_flowi_flags(sk) : 0;
 
 	if (addr_type == RTN_UNSPEC)
diff --git a/net/ipv6/netfilter.c b/net/ipv6/netfilter.c
index a7025ec87035..70e47ccdf9ae 100644
--- a/net/ipv6/netfilter.c
+++ b/net/ipv6/netfilter.c
@@ -17,14 +17,15 @@
 #include <net/ip6_route.h>
 #include <net/xfrm.h>
 #include <net/netfilter/nf_queue.h>
+#include <net/netfilter/nf_socket.h>
 #include <net/netfilter/nf_conntrack_bridge.h>
 #include <net/netfilter/ipv6/nf_defrag_ipv6.h>
 #include "../bridge/br_private.h"
 
 int ip6_route_me_harder(struct net *net, struct sock *sk_partial, struct sk_buff *skb)
 {
+	struct sock *sk = nf_sk_to_full_sk(sk_partial);
 	const struct ipv6hdr *iph = ipv6_hdr(skb);
-	struct sock *sk = sk_to_full_sk(sk_partial);
 	struct net_device *dev = skb_dst_dev(skb);
 	struct flow_keys flkeys;
 	unsigned int hh_len;
-- 
2.55.0