netdev - Re: [RFC net-next 1/1] net: sched: protect against loops in TC filter hooks

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:   Sun, 26 May 2019 10:50:09 -0400
From:   Jamal Hadi Salim <jhs@...atatu.com>
To:     Daniel Borkmann <daniel@...earbox.net>,
        John Hurley <john.hurley@...ronome.com>,
        netdev@...r.kernel.org, jiri@...lanox.com, davem@...emloft.net,
        xiyou.wangcong@...il.com
Cc:     simon.horman@...ronome.com, jakub.kicinski@...ronome.com,
        oss-drivers@...ronome.com, alexei.starovoitov@...il.com
Subject: Re: [RFC net-next 1/1] net: sched: protect against loops in TC filter
 hooks

On 2019-05-24 2:32 p.m., Daniel Borkmann wrote:
> On 05/24/2019 06:05 PM, John Hurley wrote:
>> TC hooks allow the application of filters and actions to packets at both
>> ingress and egress of the network stack. It is possible, with poor
>> configuration, that this can produce loops whereby an ingress hook calls
>> a mirred egress action that has an egress hook that redirects back to
>> the first ingress etc. The TC core classifier protects against loops when
>> doing reclassifies but, as yet, there is no protection against a packet
>> looping between multiple hooks. This can lead to stack overflow panics.
>>
>> Add a per cpu counter that tracks recursion of packets through TC hooks.
>> The packet will be dropped if a recursive limit is passed and the counter
>> reset for the next packet.
> 
> NAK. This is quite a hack in the middle of the fast path. Such redirection
> usually has a rescheduling point, like in cls_bpf case. If this is not the
> case for mirred action as I read your commit message above, then fix mirred
> instead if it's such broken.
> 

Actually a per-cpu approach will *never* work. This has to be per-skb
(since the skb is looping).
Take a look at the (new) skb metadata encoding approach from Florian;
you should be able to encode the counters in the skb.

cheers,
jamal