netdev - Re: [PATCH net] sched, cls: don't dump kernel addr into tc monitors on delete event

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: <c02ce4f1-ba99-f54c-d35d-ca81cc771c8a@mojatatu.com>
Date:   Tue, 18 Oct 2016 17:21:53 -0400
From:   Jamal Hadi Salim <jhs@...atatu.com>
To:     Daniel Borkmann <daniel@...earbox.net>, davem@...emloft.net
Cc:     netdev@...r.kernel.org, alexei.starovoitov@...il.com
Subject: Re: [PATCH net] sched, cls: don't dump kernel addr into tc monitors
 on delete event

On 16-10-18 04:18 PM, Daniel Borkmann wrote:
> 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;
>  	}
>


I was sitting on this patch I was going to send ;->
Does this resolve it?

cheers,
jamal



View attachment "patch-u32-handle-fix" of type "text/plain" (518 bytes)