[PATCH nf] netfilter: nft_socket: only do sk lookup when indev is available

[PATCH nf] netfilter: nft_socket: only do sk lookup when indev is available

[Florian Westphal]

nft_socket lacks .validate hooks to restrict its use to the prerouting
and input chains.

Adding such restriction now may break existing setups, also, if skb
has a socket attached to it, nft_socket will work fine.

Therefore, check if the incoming interface is available and NFT_BREAK
in case neither skb->sk nor input device are set.

Reported-by: Topi Miettinen <toiwoton@gmail.com>
Fixes: 554ced0a6e29 ("netfilter: nf_tables: add support for native socket matching")
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 net/netfilter/nft_socket.c | 41 +++++++++++++++++++++++++-------------
 1 file changed, 27 insertions(+), 14 deletions(-)

diff --git a/net/netfilter/nft_socket.c b/net/netfilter/nft_socket.c
index 6d9e8e0a3a7d..cbd1e4523ace 100644
--- a/net/netfilter/nft_socket.c
+++ b/net/netfilter/nft_socket.c
@@ -54,6 +54,32 @@ nft_sock_get_eval_cgroupv2(u32 *dest, struct sock *sk, const struct nft_pktinfo
 }
 #endif
 
+static struct sock *nft_socket_do_lookup(const struct nft_pktinfo *pkt)
+{
+	const struct net_device *indev = nft_in(pkt);
+	const struct sk_buff *skb = pkt->skb;
+	struct sock *sk = NULL;
+
+	if (!indev)
+		return NULL;
+
+	switch(nft_pf(pkt)) {
+	case NFPROTO_IPV4:
+		sk = nf_sk_lookup_slow_v4(nft_net(pkt), skb, indev);
+		break;
+#if IS_ENABLED(CONFIG_NF_TABLES_IPV6)
+	case NFPROTO_IPV6:
+		sk = nf_sk_lookup_slow_v6(nft_net(pkt), skb, indev);
+		break;
+#endif
+	default:
+		WARN_ON_ONCE(1);
+		break;
+	}
+
+	return sk;
+}
+
 static void nft_socket_eval(const struct nft_expr *expr,
 			    struct nft_regs *regs,
 			    const struct nft_pktinfo *pkt)
@@ -67,20 +93,7 @@ static void nft_socket_eval(const struct nft_expr *expr,
 		sk = NULL;
 
 	if (!sk)
-		switch(nft_pf(pkt)) {
-		case NFPROTO_IPV4:
-			sk = nf_sk_lookup_slow_v4(nft_net(pkt), skb, nft_in(pkt));
-			break;
-#if IS_ENABLED(CONFIG_NF_TABLES_IPV6)
-		case NFPROTO_IPV6:
-			sk = nf_sk_lookup_slow_v6(nft_net(pkt), skb, nft_in(pkt));
-			break;
-#endif
-		default:
-			WARN_ON_ONCE(1);
-			regs->verdict.code = NFT_BREAK;
-			return;
-		}
+		sk = nft_socket_do_lookup(pkt);
 
 	if (!sk) {
 		regs->verdict.code = NFT_BREAK;
-- 
2.35.1

Re: [PATCH nf] netfilter: nft_socket: only do sk lookup when indev is available

[Topi Miettinen]

On 27.4.2022 19.02, Florian Westphal wrote:
> nft_socket lacks .validate hooks to restrict its use to the prerouting
> and input chains.
> 
> Adding such restriction now may break existing setups, also, if skb
> has a socket attached to it, nft_socket will work fine.
> 
> Therefore, check if the incoming interface is available and NFT_BREAK
> in case neither skb->sk nor input device are set.
> 
> Reported-by: Topi Miettinen <toiwoton@gmail.com>
> Fixes: 554ced0a6e29 ("netfilter: nf_tables: add support for native socket matching")
> Signed-off-by: Florian Westphal <fw@strlen.de>

Thanks. After applying this patch, my test case which triggered a BUG in 
a few tries, doesn't do it anymore with 25 attempts.

Tested-by: Topi Miettinen <toiwoton@gmail.com>

> ---
>   net/netfilter/nft_socket.c | 41 +++++++++++++++++++++++++-------------
>   1 file changed, 27 insertions(+), 14 deletions(-)
> 
> diff --git a/net/netfilter/nft_socket.c b/net/netfilter/nft_socket.c
> index 6d9e8e0a3a7d..cbd1e4523ace 100644
> --- a/net/netfilter/nft_socket.c
> +++ b/net/netfilter/nft_socket.c
> @@ -54,6 +54,32 @@ nft_sock_get_eval_cgroupv2(u32 *dest, struct sock *sk, const struct nft_pktinfo
>   }
>   #endif
>   
> +static struct sock *nft_socket_do_lookup(const struct nft_pktinfo *pkt)
> +{
> +	const struct net_device *indev = nft_in(pkt);
> +	const struct sk_buff *skb = pkt->skb;
> +	struct sock *sk = NULL;
> +
> +	if (!indev)
> +		return NULL;
> +
> +	switch(nft_pf(pkt)) {
> +	case NFPROTO_IPV4:
> +		sk = nf_sk_lookup_slow_v4(nft_net(pkt), skb, indev);
> +		break;
> +#if IS_ENABLED(CONFIG_NF_TABLES_IPV6)
> +	case NFPROTO_IPV6:
> +		sk = nf_sk_lookup_slow_v6(nft_net(pkt), skb, indev);
> +		break;
> +#endif
> +	default:
> +		WARN_ON_ONCE(1);
> +		break;
> +	}
> +
> +	return sk;
> +}
> +
>   static void nft_socket_eval(const struct nft_expr *expr,
>   			    struct nft_regs *regs,
>   			    const struct nft_pktinfo *pkt)
> @@ -67,20 +93,7 @@ static void nft_socket_eval(const struct nft_expr *expr,
>   		sk = NULL;
>   
>   	if (!sk)
> -		switch(nft_pf(pkt)) {
> -		case NFPROTO_IPV4:
> -			sk = nf_sk_lookup_slow_v4(nft_net(pkt), skb, nft_in(pkt));
> -			break;
> -#if IS_ENABLED(CONFIG_NF_TABLES_IPV6)
> -		case NFPROTO_IPV6:
> -			sk = nf_sk_lookup_slow_v6(nft_net(pkt), skb, nft_in(pkt));
> -			break;
> -#endif
> -		default:
> -			WARN_ON_ONCE(1);
> -			regs->verdict.code = NFT_BREAK;
> -			return;
> -		}
> +		sk = nft_socket_do_lookup(pkt);
>   
>   	if (!sk) {
>   		regs->verdict.code = NFT_BREAK;