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@xxxxxxxxxx/ > > > > > > + 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.
