netdev - Re: [PATCH v2] net: sched: Add support for packet bursting.

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:   Tue, 29 Jun 2021 16:35:11 +0200
From:   Toke Høiland-Jørgensen <toke@...hat.com>
To:     Niclas Hedam <nhed@....dk>
Cc:     "stephen@...workplumber.org" <stephen@...workplumber.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Dave Taht <dave.taht@...il.com>
Subject: Re: [PATCH v2] net: sched: Add support for packet bursting.

Niclas Hedam <nhed@....dk> writes:

> Thanks for the valuable thoughts, Toke.
>
> The patch started with me being tasked to try and mitigate timing
> attacks caused by network latencies. I scouted over the current
> network stack and didn't find anything that fully matched my use-case.
> While I now understand that you can actually leverage the slots
> functionality for this, I would still opt for a new interface and
> implementation.

So what is the actual use case for this? If this is something you're
actually planning to deploy this in production, I'm not sure netem is
the best place for it...

> I have not done any CPU benchmarks on the slots system, so I'm not
> approaching this from the practical performance side per se.
>
> Instead, I argue for seperation with reference to the Seperation of
> Concern design principle. The slots functionality is not
> built/designed to cater security guarantees, and my patch is not built
> to cater duty cycles, etc.

Separation of concerns is all well and good, but you're still adding
this to an existing qdisc (and one mostly used for emulating networks,
at that), in a way that will silently disable most of the other
functionality. I.e., if the 'bursting' field is set, 'rate' and
'latency' will just silently stop working because you're skipping the
code path that uses those.

So you'll need to reject invalid combinations at configure time, which
AFAICT is any other feature combined with bursting. Also, please add an
unlikely() around the check for the bursting parameter to hint the
compiler that this is not the most commonly used feature.

> If we opt to merge these two functionalities or discard mine, we have
> to implement some guarantee that the slots functionality won't become
> significantly slower or complex, which in my opinion is less
> maintainable than two similar systems. Also, this patch is very
> limited in lines of code, so maintaining it is pretty trivial.

Maintenance is not just about lines of code, it is also about
combination of features (e.g., dealing with things like "my rate limiter
stopped working after I turned on this 'security' feature"). At the very
least you'll need to clearly document interactions (and refuse invalid
combinations as mentioned above).

-Toke