netdev - matchall race (was: Re: [PATCH net-next] net/sched: cls_flower: verify root pointer before dereferncing it)

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] [day] [month] [year] [list]

Message-ID: <583807CB.4000503@iogearbox.net>
Date:   Fri, 25 Nov 2016 10:43:39 +0100
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Cong Wang <xiyou.wangcong@...il.com>
CC:     David Miller <davem@...emloft.net>, Jiri Pirko <jiri@...nulli.us>,
        Roi Dayan <roid@...lanox.com>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>,
        Jiri Pirko <jiri@...lanox.com>,
        Or Gerlitz <ogerlitz@...lanox.com>,
        Cong Wang <cwang@...pensource.com>,
        John Fastabend <john.fastabend@...il.com>,
        Alexei Starovoitov <alexei.starovoitov@...il.com>
Subject: matchall race (was: Re: [PATCH net-next] net/sched: cls_flower: verify
 root pointer before dereferncing it)

[ Making this a different thread, since unrelated to the other one. ]

On 11/25/2016 07:23 AM, Cong Wang wrote:
> On Thu, Nov 24, 2016 at 4:17 PM, Daniel Borkmann <daniel@...earbox.net> wrote:
[...]
>>
>> (Btw, matchall is still broken besides this fix. mall_delete() sets the
>>   RCU_INIT_POINTER(head->filter, NULL), so that a mall_delete() plus
>>   mall_destroy() combo doesn't free head->filter twice, but doing that is
>>   racy with mall_classify() where head->filter is dereferenced unchecked.
>>   Requires additional fix.)
>
> This seems due to matchall only has one filter per tp. But you don't need
> to worry since readers never read a freed pointer, right?

The race would be that CPU0 is in tc_ctl_tfilter() with RTM_DELTFILTER and
tcm_handle = 1 request. matchall has only single handle and id of it is 1
(or a prior user-specified one). So in tc_ctl_tfilter() ->get() will find it,
returns pointer as fh. Then we call ->delete(). mall_delete() sets head->filter
to NULL with RCU_INIT_POINTER() (head->filter is not even an RCU pointer btw),
real filter destruction comes via call_rcu(). If we got here, that tp is still
visible and not unlinked from tp chain. So in CPU1 tc_classify() executes
mall_classify(), we fetch the head (tp->root) and the corresponding head->filter
from there, and dereference it next. Probably best would have been to just
return -EOPNOTSUPP for ->delete() like in cls_cgroup case. It probably makes
sense to just defer real destruction to mall_destroy() instead of mall_delete()
if one wants to avoid extra !f test in mall_classify().