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]

Message-ID: <Y8gXmjlFPZdcoSzW@dcaratti.users.ipa.redhat.com>
Date:   Wed, 18 Jan 2023 17:00:26 +0100
From:   Davide Caratti <dcaratti@...hat.com>
To:     Jamal Hadi Salim <jhs@...atatu.com>
Cc:     Kyle Zeng <zengyhkyle@...il.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

all good points!

On Wed, Jan 18, 2023 at 08:06:12AM -0500, Jamal Hadi Salim wrote:

> The main issue is bigger than tcf_classify: It has to do with
> interpretation of tcf_result and the return codes.

Some classful qdiscs ignore the value of res.class and just lookup the
configured classes based on the value of res.classid. In this case, the qdisc
code validates tcf_results, and the resulting class is always a valid pointer
with correct layout and size.

With HTB, CBQ, DRR, HFSC, QFQ and ATM it's possible that the TC classifier
selects a traffic class for a given packet and writes that pointer in 'res.class'.
When/if this happens, the qdisc doesn't need to use res.classid and then lookup for
the traffic class: it assumes that res.class is already a good pointer to a
struct of the correct type (struct hfsc_class for HFSC, for instance).

> The main environmental rule that was at stake here is the return
> (verdict) code said to drop the packet. The validity of tcf_result in
> such a case is questionable and setting it to 0 was irrelevant.

I remember that the first matchall implementation forgot the assignment
of 'res' and this caused a problem similar to the reported one [1]. In my opinion
we should consider to eliminate 'res.class' and its usage in the above qdiscs,
if we find out that current TC filters just write res.classid (not sure of what
cls_bpf does, though) and constantly write 0 to res.class.

[1] https://lore.kernel.org/netdev/b930159de5531a4d216a1cd2c2ef03aa41f421f9.1505562794.git.dcaratti@redhat.com/

> The current return code is a "verdict" on what happened. Given that
> there is potential to misinterpret - as was seen here - a safer approach is to get the
> return code to be either an error/success code(eg ELOOP for the example being quoted) since
> that is a more common pattern and we store the "verdict code" in tcf_result (TC_ACT_SHOT).
> I was planning to send an RFC patch for that.

well, the implementation of "goto_chain" actually abuses tcf_result:
since it's going to pass the packet to another classifier, it
temporarily stores a handle to the next filter in the tcf_result -   
instead of passing it through the stack. That is not a problem, unless
a packet hits the max_reclassify_loop and a CBQ qdisc that dereferences
'res.class' even with a packet drop :)

> I am still not clear on the correlation that Zhengchao Shao was making between
> Davide's patch and this issue...

In my understanding there is no correlation. Oh, last small note:

> On Wed, Jan 18, 2023 at 6:06 AM Davide Caratti <dcaratti@...hat.com> wrote:
> >
> >
> > 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 think it is sufficient to avoid the crash, since the value of 'res.class'
should be a valid pointer (NULL most of the times?) after each call to 

tp->classify(skb, tp, res);

if the return value is different than TC_ACT_UNSPEC and TC_ACT_SHOT.
However, I still don't understand why cbq should charge classes when the
packet has been 'stolen' or 'trapped'...

-- 
davide