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

[Topi Miettinen <toiwoton@gmail.com>]

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;