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-next>] [day] [month] [year] [list]
Date:   Tue, 18 Oct 2016 22:18:24 +0200
From:   Daniel Borkmann <daniel@...earbox.net>
To:     davem@...emloft.net
Cc:     netdev@...r.kernel.org, jhs@...atatu.com,
        alexei.starovoitov@...il.com,
        Daniel Borkmann <daniel@...earbox.net>
Subject: [PATCH net] sched, cls: don't dump kernel addr into tc monitors on delete event

While trying out [1][2], I noticed that tc monitor doesn't show the
correct handle on delete:

  $ tc monitor
  qdisc clsact ffff: dev eno1 parent ffff:fff1
  filter dev eno1 ingress protocol all pref 49152 bpf handle 0x2a [...]
  deleted filter dev eno1 ingress protocol all pref 49152 bpf handle 0xf3be0c80

In fact, the shown handle points to a kernel address, in this case
0xffff8807f3be0c80, which points to a struct cls_bpf_prog from bpf
classifier.

The issue is not bpf specific though. tcf_fill_node() sets a fh as
tcm->tcm_handle, which gets overridden on != RTM_DELTFILTER events
only. For RTM_DELTFILTER events, we cannot call the classifier's
dump() handler, since notification is given after delete() handler
returned with success.

At latest when a classifier's dump() handler is called, tm->tcm_handle
is filled with an actual handle. They are currently classifier
internal, meaning a tcf_proto can handle multiple classifiers if
the implementation supports it, so it needs to be queried from the
callback.

For RTM_DELTFILTER, the fh value contains the address of the object
to dump. Commit 4e54c4816bfe ("[NET]: Add tc extensions infrastructure.")
added the logic to assign tcm->tcm_handle = fh. tcm_handle is 32bit
so for 64bit archs, it's stored truncated. Prior to that commit, it
was set to 0. Reintroduce this, so we at least don't leak the kernel
address or parts of it to unprivileged user space listeners.

Since user space cannot make any sense out of this 32bit part,
passing a random number would be just as good. Lets pass 0, since i)
this allows to add the feature at some point for net-next, and ii)
this is also consistent with notifications via tfilter_notify_chain()
when we delete the entire chain.

  [1] http://patchwork.ozlabs.org/patch/682828/
  [2] http://patchwork.ozlabs.org/patch/682829/

Fixes: 4e54c4816bfe ("[NET]: Add tc extensions infrastructure.")
Signed-off-by: Daniel Borkmann <daniel@...earbox.net>
---
 ( Commit is in -history tree. Jamal, please take a look if you have
   a chance, thanks. )

 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 2ee29a3..e11bdc5 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -400,12 +400,11 @@ static int tcf_fill_node(struct net *net, struct sk_buff *skb,
 	tcm->tcm__pad2 = 0;
 	tcm->tcm_ifindex = qdisc_dev(tp->q)->ifindex;
 	tcm->tcm_parent = tp->classid;
+	tcm->tcm_handle = 0;
 	tcm->tcm_info = TC_H_MAKE(tp->prio, tp->protocol);
 	if (nla_put_string(skb, TCA_KIND, tp->ops->kind))
 		goto nla_put_failure;
-	tcm->tcm_handle = fh;
 	if (RTM_DELTFILTER != event) {
-		tcm->tcm_handle = 0;
 		if (tp->ops->dump && tp->ops->dump(net, tp, fh, skb, tcm) < 0)
 			goto nla_put_failure;
 	}
-- 
1.9.3

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ