netdev - Re: [Patch net] net_sched: refetch skb protocol for each filter

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 for Android: free password hash cracker in your pocket

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

Date:   Mon, 14 Jan 2019 10:23:15 +0100
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Cong Wang <xiyou.wangcong@...il.com>, netdev@...r.kernel.org
Cc:     Martin Olsson <martin.olsson+netdev@...torsecurity.com>,
        Jamal Hadi Salim <jhs@...atatu.com>,
        Jiri Pirko <jiri@...nulli.us>
Subject: Re: [Patch net] net_sched: refetch skb protocol for each filter

On 01/12/2019 03:55 AM, Cong Wang wrote:
> Martin reported a set of filters don't work after changing
> from reclassify to continue. Looking into the code, it
> looks like skb protocol is not always fetched for each
> iteration of the filters. But, as demonstrated by Martin,
> TC actions could modify skb->protocol, for example act_vlan,
> this means we have to refetch skb protocol in each iteration,
> rather than using the one we fetch in the beginning of the loop.
> 
> This bug is _not_ introduced by commit 3b3ae880266d
> ("net: sched: consolidate tc_classify{,_compat}"), technically,
> if act_vlan is the only action that modifies skb protocol, then
> it is commit c7e2b9689ef8 ("sched: introduce vlan action") which
> introduced this bug.
> 
> Reported-by: Martin Olsson <martin.olsson+netdev@...torsecurity.com>
> Cc: Jamal Hadi Salim <jhs@...atatu.com>
> Cc: Jiri Pirko <jiri@...nulli.us>
> Signed-off-by: Cong Wang <xiyou.wangcong@...il.com>
> ---
>  net/sched/cls_api.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
> index 8ce2a0507970..e2b5cb2eb34e 100644
> --- a/net/sched/cls_api.c
> +++ b/net/sched/cls_api.c
> @@ -1277,7 +1277,6 @@ EXPORT_SYMBOL(tcf_block_cb_unregister);
>  int tcf_classify(struct sk_buff *skb, const struct tcf_proto *tp,
>  		 struct tcf_result *res, bool compat_mode)
>  {
> -	__be16 protocol = tc_skb_protocol(skb);
>  #ifdef CONFIG_NET_CLS_ACT
>  	const int max_reclassify_loop = 4;
>  	const struct tcf_proto *orig_tp = tp;
> @@ -1287,6 +1286,7 @@ int tcf_classify(struct sk_buff *skb, const struct tcf_proto *tp,
>  reclassify:
>  #endif
>  	for (; tp; tp = rcu_dereference_bh(tp->next)) {
> +		__be16 protocol = tc_skb_protocol(skb);
>  		int err;
>  
>  		if (tp->protocol != protocol &&

Can't we do something like the below instead? Otherwise we'll needlessly refetch
protocol every time there is a mismatch in above tp->protocol check as well.

diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 8ce2a05..dc725a1 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -1305,6 +1305,11 @@ int tcf_classify(struct sk_buff *skb, const struct tcf_proto *tp,
 #endif
                if (err >= 0)
                        return err;
+
+               /* We also need to refetch protocol from here as e.g.
+                * act_vlan could have changed it.
+                */
+               protocol = tc_skb_protocol(skb);
        }

        return TC_ACT_UNSPEC; /* signal: continue lookup */

Thanks,
Daniel