Message-ID: <CADW8OBvNcMCogJsMJkVXw70PL3oGU9s1a16DOK+xqdnCfgQzvg@mail.gmail.com>
Date:   Tue, 17 Jan 2023 17:10:58 -0700
From:   Kyle Zeng <zengyhkyle@...il.com>
To:     Jamal Hadi Salim <jhs@...atatu.com>
Cc:     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>,
        Davide Caratti <dcaratti@...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

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. 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;
}

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.

BTW, I'm not sure whether it is a bug or it is intended in the merged
patch for this bug: moving `return NULL` statement in `cbq_classify`
results in a behavior change that is not documented anywhere:
previously, packets that return TC_ACT_QUEUED, TC_ACT_STOLEN, and
TC_ACT_TRAP will eventually return NULL, but now they will be passed
into `cbq_reclassify`. Is this expected?

Best,
Kyle Zeng

On Tue, Jan 17, 2023 at 12:07 PM Jamal Hadi Salim <jhs@...atatu.com> wrote:
>
> +Cc netdev
>
> On Tue, Jan 17, 2023 at 10:06 AM Jamal Hadi Salim <jhs@...atatu.com> wrote:
> >
> > Trimmed Cc (+Davide).
> >
> > I am not sure i followed what you are saying because i dont see the
> > relationship between the
> > two commits. Did that patch(9410c9409d3e) cause a problem?
> > How do you reproduce the issue that is caused by this patch that you are seeing?
> >
> > One of the challenges we have built over time is consumers of classification and
> > action execution may not be fully conserving the semantics of the return code.
> > The return code is a "verdict" on what happened; a safer approach is to get the
> > return code to be either an error/success code. But that seems to be a
> > separate issue.
> >
> > cheers,
> > jamal
> >
> > On Mon, Jan 16, 2023 at 3:28 AM shaozhengchao <shaozhengchao@...wei.com> wrote:
> > >
> > > When I analyzed the following LTS 5.10 patch, I had a small question:
> > > https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?h=linux-5.10.y&id=b2c917e510e5ddbc7896329c87d20036c8b82952
> > >
> > > As described in this patch, res is obtained through the tcf_classify()
> > > interface. If result is TC_ACT_SHOT, res may be an abnormal value.
> > > Accessing class in res will cause abnormal access.
> > >
> > > For LTS version 5.10, if tcf_classify() is to return a positive value,
> > > the classify hook function to the filter must be called, and the hook
> > > function returns a positive number. Observe the classify function of
> > > each filter. Generally, res is initialized in four scenarios.
> > > 1. res is assigned a value by res in the private member of each filter.
> > > Generally, kzalloc is used to assign initial values to res of various
> > > filters. Therefore, class in res is initialized to 0. Then use the
> > > tcf_bind_filter() interface to assign values to members in res.
> > > Therefore, value of class is assigned. For example, cls_basic.
> > > 2. The classify function of the filter directly assigns a value to the
> > > class of res, for example, cls_cgroup.
> > > 3. The filter classify function references tp and assigns a value to
> > > res, for example, cls_u32.
> > > 4. The change function of the filter references fh and assigns a value
> > > to class in res, for example, cls_rsvp.
> > >
> > > This Mainline problem is caused by commit:3aa260559455 (" net/sched:
> > > store the last executed chain also for clsact egress") and
> > > commit:9410c9409d3e ("net: sched: Introduce ingress classification
> > > function"). I don't know if my analysis is correct, please help correct,
> > > thank you very much.
> > >
> > > Zhengchao Shao

Powered by blists - more mailing lists