8/1/2024 1:36 PM, Günther Noack wrote:

On Wed, Jul 31, 2024 at 08:20:41PM +0300, Mikhail Ivanov wrote:

quoted

7/30/2024 11:24 AM, Günther Noack wrote:

quoted

On Sun, Jul 28, 2024 at 08:25:55AM +0800, Mikhail Ivanov wrote:

quoted

LANDLOCK_ACCESS_NET_BIND_TCP is useful to limit the scope of "bindable"
ports to forbid a malicious sandboxed process to impersonate a legitimate
server process. However, bind(2) might be used by (TCP) clients to set the
source port to a (legitimate) value. Controlling the ports that can be
used for listening would allow (TCP) clients to explicitly bind to ports
that are forbidden for listening.

Such control is implemented with a new LANDLOCK_ACCESS_NET_LISTEN_TCP
access right that restricts listening on undesired ports with listen(2).

Nit: I would turn around the first two commit message paragraphs and describe
your changes first, before explaining the problems in the bind(2) support.  I
was initially a bit confused that the description started talking about
LANDLOCK_ACCESS_NET_BIND_TCP.

General recommendations at:
https://www.kernel.org/doc/html/v6.10/process/submitting-patches.html#describe-your-changes

I consider the first paragraph as a problem statement for this patch.
According to linux recommendations problem should be established before
the description of changes. Do you think that the changes part should
stand before the problem anyway?

Up to you. To be fair, I'm sold on the approach in this patchset anyway :)

Nice :)

quoted

quoted

When we have the documentation wording finalized,
please send an update to the man pages as well,
for this and other documentation updates.

Should I send it after this patchset would be accepted?

Yes, that would be the normal process which we have been following so far.

(I don't like the process much either, because it decouples feature development
so far from documentation writing, but it's what we have for now.)

An example patch which does that for the network bind(2) and connect(2) features
(and where I would still like a review from Konstantin) is:
https://lore.kernel.org/all/20240723101917.90918-1-gnoack@google.com/ (local)

got it

quoted

quoted

Small remarks on what I've done here:

* I am avoiding the word "binding" when referring to the automatic assignment to
    an ephemeral port - IMHO, this is potentially confusing, since bind(2) is not
    explicitly called.
* I am also dropping the "It should be noted" / "Note that" phrase, which is
    frowned upon in man pages.

Didn't know that, thanks

Regarding "note that", see
https://lore.kernel.org/all/0aafcdd6-4ac7-8501-c607-9a24a98597d7@gmail.com/ (local)
https://lore.kernel.org/linux-man/20210729223535.qvyomfqvvahzmu5w@localhost.localdomain/ (local)
https://lore.kernel.org/linux-man/20230105225235.6cjtz6orjzxzvo6v@illithid/ (local)
(The "Kemper notectomy")

This came up in man page reviews, but we'll have an easier time keeping the
kernel and man page documentation in sync if we adhere to man page style
directly.  (The man page style is documented in man-pages(7) and contains some
groff-independent wording advice as well.)

Ok, such phrases should be really avoided in kernel as well.

quoted

quoted

If I understand correctly, these are cases where we use TCP on top of protocols
that are not IP (or have an additional layer in the middle, like TLS?).  This
can not be recognized through the socket family or type?

ULP can be used in the context of TCP protocols as an additional layer
(currently supported only by IP and MPTCP), so it cannot be recognized
with family or type. You can check this test [1] in which TCP IP socket
is created with ULP control hook.

[1] https://lore.kernel.org/all/20240728002602.3198398-8-ivanov.mikhail1@huawei-partners.com/ (local)

Thanks, this is helpful.

For reference, it seems that ULP were introduced in
https://lore.kernel.org/all/20170614183714.GA80310@davejwatson-mba.dhcp.thefacebook.com/ (local)

quoted

quoted

Do we have cases where we can run TCP on top of something else than plain IPv4
or IPv6, where the clone method exists?

Yeah, MPTCP protocol for example (see net/mptcp/subflow.c). ULP control
hook is supported only by IP and MPTCP, and in both cases
clone method is checked during listen(2) execution.

quoted

quoted

Aren't the socket type and family checks duplicated with existing logic that we
have for the connect(2) and bind(2) support?  Should it be deduplicated, or is
that too messy?

bind(2) and connect(2) hooks also support AF_UNSPEC family, so I think
such helper is gonna complicate code a little bit. Also it can
complicate switch in current_check_access_socket().

OK, sounds good. 👍

—Günther