[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Y8fSmFD2dNtBpbwK@dcaratti.users.ipa.redhat.com>
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
Powered by blists - more mailing lists