netdev - Re: Question: Patch:("net: sched: cbq: dont intepret cls results when asked to drop") may be not bug for branch LTS 5.10

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:   Wed, 18 Jan 2023 12:06:00 +0100
From:   Davide Caratti <dcaratti@...hat.com>
To:     Kyle Zeng <zengyhkyle@...il.com>
Cc:     Jamal Hadi Salim <jhs@...atatu.com>,
        shaozhengchao <shaozhengchao@...wei.com>,
        David Miller <davem@...emloft.net>,
        Sasha Levin <sashal@...nel.org>,
        Cong Wang <xiyou.wangcong@...il.com>,
        Jiri Pirko <jiri@...nulli.us>,
        Jakub Kicinski <kuba@...nel.org>,
        Eric Dumazet <edumazet@...gle.com>,
        Paolo Abeni <pabeni@...hat.com>,
        Marcelo Ricardo Leitner <marcelo.leitner@...il.com>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>
Subject: Re: Question: Patch:("net: sched: cbq: dont intepret cls results
 when asked to drop") may be not bug for branch LTS 5.10

hello,

On Tue, Jan 17, 2023 at 05:10:58PM -0700, Kyle Zeng wrote:
> Hi Zhengchao,
> 
> I'm the finder of the vulnerability. In my initial report, there was a
> more detailed explanation of why this bug happened. But it got left
> out in the commit message.
> So, I'll explain it here and see whether people want to patch the
> actual root cause of the crash.
> 
> The underlying bug that this patch was trying to address is actually
> in `__tcf_classify`. Notice that `struct tcf_result` is actually a
> union type, so whenever the kernel sets res.goto_tp, it also sets
> res.class.

>From what I see/remember, 'res' (struct tcf_result) is unassigned
unless the packet is matched by a classifier (i.e. it does not return
TC_ACT_UNSPEC).

When this match happens (__tcf_classify returns non-negative) and the
control action says TC_ACT_GOTO_CHAIN, res->goto_tp is written.
Like you say, 'res.class' is written as well because it's a union.

> And this can happen inside `tcf_action_goto_chain_exec`. In
> other words, `tcf_action_goto_chain_exec` will set res.class. Notice
> that goto_chain can point back to itself, which causes an infinite
> loop. To avoid the infinite loop situation, `__tcf_classify` checks
> how many times the loop has been executed
> (https://elixir.bootlin.com/linux/v6.1/source/net/sched/cls_api.c#L1586),
> if it is more than a specific number, it will mark the result as
> TC_ACT_SHOT and then return:
> 
> if (unlikely(limit++ >= max_reclassify_loop)) {
>     ...
>     return TC_ACT_SHOT;
> }

maybe there is an easier reproducer, something made of 2 TC actions.
The first one goes to a valid chain, and then the second one (executed from
within the chain) drops the packet. I think that unpatched CBQ scheduler 
will find 'res.class' with a value also there.
 
> However, when it returns in the infinite loop handler, it forgets to
> clear whatever is in the `res` variable, which still holds pointers in
> `goto_tp`. As a result, cbq_classify will think it is a valid
> `res.class` and causes type confusion.
> 
> My initial proposed patch was to memset `res` before `return
> TC_ACT_SHOT` in `__tcf_classify`, but it didn't get merged. But I
> guess the merged patch is more generic.

The merged patch looks good to me; however, I wonder if it's sufficient.
If I well read the code, there is still the possibility of hitting the
same problem on a patched kernel when TC_ACT_TRAP / TC_ACT_STOLEN is
returned after a 'goto chain' when the qdisc is CBQ.

I like Jamal's idea of sharing the reproducer :)

thanks!
-- 
davide