netdev - Re: [PATCH net-next 2/5] net: sched: em_ipt: set the family based on the protocol when matching

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190626163353.6d5535cb@jimi>
Date:   Wed, 26 Jun 2019 16:33:53 +0300
From:   Eyal Birger <eyal.birger@...il.com>
To:     Nikolay Aleksandrov <nikolay@...ulusnetworks.com>
Cc:     netdev@...r.kernel.org, roopa@...ulusnetworks.com,
        pablo@...filter.org, xiyou.wangcong@...il.com, davem@...emloft.net,
        jiri@...nulli.us, jhs@...atatu.com
Subject: Re: [PATCH net-next 2/5] net: sched: em_ipt: set the family based
 on the protocol when matching

Hi Nikolay,
   
On Wed, 26 Jun 2019 14:58:52 +0300
Nikolay Aleksandrov <nikolay@...ulusnetworks.com> wrote:

> Set the family based on the protocol otherwise protocol-neutral
> matches will have wrong information (e.g. NFPROTO_UNSPEC). In
> preparation for using NFPROTO_UNSPEC xt matches.
> 
> Signed-off-by: Nikolay Aleksandrov <nikolay@...ulusnetworks.com>
> ---
>  net/sched/em_ipt.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/net/sched/em_ipt.c b/net/sched/em_ipt.c
> index 64dbafe4e94c..23965a071177 100644
> --- a/net/sched/em_ipt.c
> +++ b/net/sched/em_ipt.c
> @@ -189,10 +189,12 @@ static int em_ipt_match(struct sk_buff *skb,
> struct tcf_ematch *em, case htons(ETH_P_IP):
>  		if (!pskb_network_may_pull(skb, sizeof(struct
> iphdr))) return 0;
> +		state.pf = NFPROTO_IPV4;
>  		break;
>  	case htons(ETH_P_IPV6):
>  		if (!pskb_network_may_pull(skb, sizeof(struct
> ipv6hdr))) return 0;
> +		state.pf = NFPROTO_IPV6;
>  		break;
>  	default:
>  		return 0;
> @@ -203,7 +205,7 @@ static int em_ipt_match(struct sk_buff *skb,
> struct tcf_ematch *em, if (skb->skb_iif)
>  		indev = dev_get_by_index_rcu(em->net, skb->skb_iif);
>  
> -	nf_hook_state_init(&state, im->hook, im->match->family,
> +	nf_hook_state_init(&state, im->hook, state.pf,
>  			   indev ?: skb->dev, skb->dev, NULL,
> em->net, NULL); 
>  	acpar.match = im->match;

I think this change is incompatible with current behavior.

Consider the 'policy' match which matches the packet's xfrm state (sec_path)
with the provided user space parameters. The sec_path includes information
about the encapsulating packet's parameters whereas the current skb points to
the encapsulated packet, and the match is done on the encapsulating
packet's info.

So if you have an IPv6 packet encapsulated within an IPv4 packet, the match
parameters should be done using IPv4 parameters, not IPv6.

Maybe use the packet's family only if the match family is UNSPEC?

Eyal.