netdev - Re: [PATCH net 03/16] net/sched: act_csum: validate the control action inside init()

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]

Date:   Thu, 07 Mar 2019 14:56:47 +0100
From:   Davide Caratti <dcaratti@...hat.com>
To:     Cong Wang <xiyou.wangcong@...il.com>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Jamal Hadi Salim <jhs@...atatu.com>,
        Jiri Pirko <jiri@...nulli.us>,
        Vlad Buslov <vladbu@...lanox.com>,
        Paolo Abeni <pabeni@...hat.com>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>
Subject: Re: [PATCH net 03/16] net/sched: act_csum: validate the control
 action inside init()

On Fri, 2019-03-01 at 16:04 -0800, Cong Wang wrote:
> > On Fri, Mar 1, 2019 at 10:02 AM Davide Caratti <dcaratti@...hat.com> wrote:
> > 
> > if I well understand the question, you are worried about
> > tcf_action_goto_chain_exec(), that can dereference 'oldchain' while we are
> > overwriting the action. A call to tcf_chain_put_by_act(oldchain) would
> > decrease refcounts and eventually call kfree(oldchain).
> > 
> > But this would result in a use-after-free only in case the chain has only
> > refcount held by 1 action (the one we are overwriting), and 0 filters: is
> > this a condition where packets can go through this action's data plane?
> 
> Hmm? Isn't goto chain can be arbitrary? Packets can be routed
> from this action to any filter chain, so even if the target chain has 0
> filter this action still has traffic as long as itself is not on the same
> chain?

hi,

sorry for the delay: it took some time to verify experimentally if we
really need this or not. So, we want to ensure the control path doesn't do

    tcf_csum_init(..., ovr=1, ...) 
      tcf_chain_put_by_act(oldchain)
        tcf_chain_destroy(oldchain, ...)
	  kfree(oldchain);

while the traffic path of the action is doing

    res->goto_tp = rcu_dereference_bh(oldchain->filter_chain);

I iterated this script many times on a KASAN kernel,

# tc chain add dev crash0 chain 42 ingress protocol ip flower ip_proto udp action pass
# tc filter add dev crash0 ingress protocol ip flower ip_proto udp action csum udp goto chain 42 index 66
# tc chain del dev crash0 chain 42 ingress 
(start netperf traffic)
# tc action replace action csum udp pass index 66

and reproduced twice the following splat:

 ==================================================================
 BUG: KASAN: null-ptr-deref in tcf_action_exec+0x181/0x200
 Read of size 8 at addr 0000000000000000 by task netperf/5777

 CPU: 0 PID: 5777 Comm: netperf Not tainted 5.0.0-rc7.goto_chain_fix+ #551
 Hardware name: Red Hat KVM, BIOS 0.5.1 01/01/2011
 Call Trace:
 <IRQ>
  dump_stack+0xc7/0x15b
  ? show_regs_print_info+0x5/0x5
  ? _raw_read_lock_irq+0x40/0x40
  ? tcf_action_exec+0x181/0x200
  ? tcf_action_exec+0x181/0x200
  kasan_report+0x176/0x192
  ? tcf_action_exec+0x181/0x200
  tcf_action_exec+0x181/0x200
  ? find_dump_kind+0x170/0x170
  ? rcu_irq_exit+0x153/0x210
  fl_classify+0x81a/0x830

so, I think that the answer to your question:

On Wed, 2019-02-27 at 17:50 -0800, Cong Wang wrote:
> > > > +       if (oldchain)
> > > > +               tcf_chain_put_by_act(oldchain);
> > > 
> > > Do we need to respect RCU grace period here?

is a "yes, we do". 
Now I'm trying something similar to what's done in tcf_bpf_init(), to
release the bpf program on 'replace' operations:

365         if (res == ACT_P_CREATED) {
366                 tcf_idr_insert(tn, *act);
367         } else {
368                 /* make sure the program being replaced is no longer executing */
369                 synchronize_rcu();
370                 tcf_bpf_cfg_cleanup(&old);
371         }

do you think it's worth going in this direction?
thank you in advance!

-- 
davide