Date: Fri, 17 May 2024 17:22:45 +0200
From: Mickaël Salaün <mic@...ikod.net>
To: Ivanov Mikhail <ivanov.mikhail1@...wei-partners.com>, 
	Eric Dumazet <edumazet@...gle.com>
Cc: willemdebruijn.kernel@...il.com, gnoack3000@...il.com, 
	linux-security-module@...r.kernel.org, netdev@...r.kernel.org, netfilter-devel@...r.kernel.org, 
	yusongping@...wei.com, artem.kuzin@...wei.com, konstantin.meskhidze@...wei.com, 
	Günther Noack <gnoack@...gle.com>
Subject: Re: [PATCH 1/2] landlock: Add hook on socket_listen()

On Mon, May 13, 2024 at 03:15:50PM +0300, Ivanov Mikhail wrote:
> 
> 
> 4/30/2024 4:36 PM, Mickaël Salaün wrote:
> > On Mon, Apr 08, 2024 at 05:47:46PM +0800, Ivanov Mikhail wrote:
> > > Make hook for socket_listen(). It will check that the socket protocol is
> > > TCP, and if the socket's local port number is 0 (which means,
> > > that listen(2) was called without any previous bind(2) call),
> > > then listen(2) call will be legitimate only if there is a rule for bind(2)
> > > allowing binding to port 0 (or if LANDLOCK_ACCESS_NET_BIND_TCP is not
> > > supported by the sandbox).
> > 
> > Thanks for this patch and sorry for the late full review.  The code is
> > good overall.
> > 
> > We should either consider this patch as a fix or add a new flag/access
> > right to Landlock syscalls for compatibility reason.  I think this
> > should be a fix.  Calling listen(2) without a previous call to bind(2)
> > is a corner case that we should properly handle.  The commit message
> > should make that explicit and highlight the goal of the patch: first
> > explain why, and then how.
> 
> Yeap, this is fix-patch. I have covered motivation and proposed solution
> in cover letter. Do you have any suggestions on how i can improve this?

You can start this commit message with the same description as in the
cover letter.

[...]

> > 
> > > +	if (!((1 << cur_sk_state) & (TCPF_CLOSE | TCPF_LISTEN)))
> > > +		return -EINVAL;
> > > +
> > > +	/* Sockets can listen only if ULP control hook has clone method. */
> > 
> > What is ULP?
> 
> ULP (Upper Layer Protocol) stands for protocols which are higher than
> transport protocol in OSI model. Linux has an infrastructure that
> allows TCP sockets to support logic of some ULP (e.g. TLS ULP). [1]

OK, you can extend the comment with this information, but no need for
the links.

> 
> There is a patch that prevents ULP sockets from listening
> if corresponding ULP implementation in linux doesn't have a clone
> method. [2]
> 
> Landlock shouldn't return EACCES for ULP sockets that cannot listen
> due to some ULP restrictions.

Looks good.

> 
> [1]
> https://lore.kernel.org/all/20170524162646.GA24128@davejwatson-mba.local/
> [2] https://lore.kernel.org/all/4b80c3d1dbe3d0ab072f80450c202d9bc88b4b03.1672740602.git.pabeni@redhat.com/
> 
> > 
> > > +	icsk = inet_csk(sk);
> > > +	if (icsk->icsk_ulp_ops && !icsk->icsk_ulp_ops->clone)
> > > +		return -EINVAL;
> > 
> > Can you please add tests covering all these error cases?
> 
> Yeap, i'll add a test for first check.
> 
> I have not found a way to trigger the second check from userspace.
> Since socket wasn't binded to any port, this means that it cannot
> be part of a TCP connection in any state, so it has to be in TCPF_CLOSE

If you're sure this cannot be triggered from user space, you can wrap
the test with WARN_ON_ONCE(), but we need to be careful.  I'd like to
get the point of view of kernel network expert though.

Eric, is this assumption correct?

> state. Nevertheless i think that this check is required:
> 
> * for consistency with inet stack (see __inet_listen_sk())
> 
> * i have not found any restrictions connected with sock locking
>   for TCP-like protocols, so listen(2) can be called after
>   sk->sk_prot->connect() method will change sock state in
>   __inet_stream_connect() (e.g. to TCP_SYN_SENT). In that case this
>   check may be required.
> 
> What do you think?

This looks good, but we need to explain this rationale in comments, with
explicit mention of network stack functions.

> Btw this hook on socket_listen() should be fixed in
> order to not check socket that is already in TCP_LISTEN state. Calling
> listen(2) only changes backlog value, so landlock shouldn't do anything
> in this case.
> 
> I'm not sure about ULP checking. I thought of adding test that creates
> espintcp ULP (net/xfrm/expintcp.c) socket and tries to listen on it.
> Since espintcp doesn't have clone method ULP check will be triggered.
> Problem is that kernel doesnt support this ULP module by default and it
> should be configured with CONFIG_XFRM_ESPINTCP option enabled. I think
> that selftests shouldn't depend on specific kernel configuration to be
> fully executed, so probably we should just skip this. What do you think?

Testing with espintcp makes sense for this clone case.  I hope it would
not require too much boilerplate code though.  We can and should add all
the required kernel option in tools/testing/selftests/landlock/config,
and we should not restrict tests to default kernel options, quite the
contrary if it makes sense.

[...]

> > 
> > > +	family = sock->sk->__sk_common.skc_family;
> > > +
> > > +	if (family == AF_INET || family == AF_INET6) {
> > 
> > This would make the code simpler:
> > 
> > if (family != AF_INET && family != AF_INET6)
> > 	return 0;
> 
> indeed, will be fixed.
> 
> > 
> > 
> > What would be the effect of listen() on an AF_UNSPEC socket?
> 
> AF_UNSPEC is a family type that only addresses can use.
> Socket itself can only be AF_INET or AF_INET6 in TCP.

Indeed, it is worth mentioning in a comment.

Powered by blists - more mailing lists