[RFC PATCH v1 0/2] Fix non-TCP sockets restriction

[RFC PATCH v1 0/2] Fix non-TCP sockets restriction

[Mikhail Ivanov]

Hello!
This patch fixes incorrect restriction of non-TCP bind/connect actions.
Added tests cover all protocols which were incorrectly restricted. But I'm
not sure this coverage is necessary (considering a little complication of
network selftests), probably checking a single protocol (e.g. MPTCP) will
be enough. WDYT?

Closes: https://github.com/landlock-lsm/linux/issues/40

Mikhail Ivanov (2):
  landlock: Fix non-TCP sockets restriction
  selftests/landlock: Test non-TCP INET connection-based protocols

 security/landlock/net.c                     |   2 +-
 tools/testing/selftests/landlock/common.h   |   1 +
 tools/testing/selftests/landlock/config     |   5 +
 tools/testing/selftests/landlock/net_test.c | 212 ++++++++++++++++++--
 4 files changed, 199 insertions(+), 21 deletions(-)


base-commit: af3319b445a28d51bf936cf4fe350f9c8eda5a3a
-- 
2.34.1

[RFC PATCH v1 1/2] landlock: Fix non-TCP sockets restriction

[Mikhail Ivanov]

Do not check TCP access right if socket protocol is not IPPROTO_TCP.
LANDLOCK_ACCESS_NET_BIND_TCP and LANDLOCK_ACCESS_NET_CONNECT_TCP
should not restrict bind(2) and connect(2) for non-TCP protocols
(SCTP, MPTCP, SMC).

Closes: https://github.com/landlock-lsm/linux/issues/40
Fixes: fff69fb03dde ("landlock: Support network rules with TCP bind and connect")
Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
---
 security/landlock/net.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/security/landlock/net.c b/security/landlock/net.c
index bc3d943a7118..6f59dd98bb13 100644
--- a/security/landlock/net.c
+++ b/security/landlock/net.c
@@ -68,7 +68,7 @@ static int current_check_access_socket(struct socket *const sock,
 		return -EACCES;
 
 	/* Checks if it's a (potential) TCP socket. */
-	if (sock->type != SOCK_STREAM)
+	if (sock->type != SOCK_STREAM || sock->sk->sk_protocol != IPPROTO_TCP)
 		return 0;
 
 	/* Checks for minimal header length to safely read sa_family. */
-- 
2.34.1

[RFC PATCH v1 2/2] selftests/landlock: Test non-TCP INET connection-based protocols

[Mikhail Ivanov]

Extend protocol fixture with test suits for MPTCP, SCTP and SMC protocols.
Add all options required by this protocols in config.

Extend protocol_variant structure with protocol field (Cf. socket(2)).

Refactor is_restricted() helper and add few helpers to check struct
protocol_variant on specific protocols.

Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
---
 tools/testing/selftests/landlock/common.h   |   1 +
 tools/testing/selftests/landlock/config     |   5 +
 tools/testing/selftests/landlock/net_test.c | 212 ++++++++++++++++++--
 3 files changed, 198 insertions(+), 20 deletions(-)

diff --git a/tools/testing/selftests/landlock/common.h b/tools/testing/selftests/landlock/common.h
index 61056fa074bb..40a2def50b83 100644
--- a/tools/testing/selftests/landlock/common.h
+++ b/tools/testing/selftests/landlock/common.h
@@ -234,6 +234,7 @@ enforce_ruleset(struct __test_metadata *const _metadata, const int ruleset_fd)
 struct protocol_variant {
 	int domain;
 	int type;
+	int protocol;
 };
 
 struct service_fixture {
diff --git a/tools/testing/selftests/landlock/config b/tools/testing/selftests/landlock/config
index 29af19c4e9f9..73b01d7d0881 100644
--- a/tools/testing/selftests/landlock/config
+++ b/tools/testing/selftests/landlock/config
@@ -1,8 +1,12 @@
 CONFIG_CGROUPS=y
 CONFIG_CGROUP_SCHED=y
 CONFIG_INET=y
+CONFIG_INFINIBAND=y
+CONFIG_IP_SCTP=y
 CONFIG_IPV6=y
 CONFIG_KEYS=y
+CONFIG_MPTCP=y
+CONFIG_MPTCP_IPV6=y
 CONFIG_NET=y
 CONFIG_NET_NS=y
 CONFIG_OVERLAY_FS=y
@@ -10,6 +14,7 @@ CONFIG_PROC_FS=y
 CONFIG_SECURITY=y
 CONFIG_SECURITY_LANDLOCK=y
 CONFIG_SHMEM=y
+CONFIG_SMC=y
 CONFIG_SYSFS=y
 CONFIG_TMPFS=y
 CONFIG_TMPFS_XATTR=y
diff --git a/tools/testing/selftests/landlock/net_test.c b/tools/testing/selftests/landlock/net_test.c
index 4e0aeb53b225..dbe77d436281 100644
--- a/tools/testing/selftests/landlock/net_test.c
+++ b/tools/testing/selftests/landlock/net_test.c
@@ -36,6 +36,17 @@ enum sandbox_type {
 	TCP_SANDBOX,
 };
 
+/* Checks if IPPROTO_SMC is present for compatibility reasons. */
+#if !defined(__alpha__) && defined(IPPROTO_SMC)
+#define SMC_SUPPORTED 1
+#else
+#define SMC_SUPPORTED 0
+#endif
+
+#ifndef IPPROTO_SMC
+#define IPPROTO_SMC 256
+#endif
+
 static int set_service(struct service_fixture *const srv,
 		       const struct protocol_variant prot,
 		       const unsigned short index)
@@ -85,19 +96,37 @@ static void setup_loopback(struct __test_metadata *const _metadata)
 	clear_ambient_cap(_metadata, CAP_NET_ADMIN);
 }
 
+static bool prot_is_inet_stream(const struct protocol_variant *const prot)
+{
+	return (prot->domain == AF_INET || prot->domain == AF_INET6) &&
+	       prot->type == SOCK_STREAM;
+}
+
+static bool prot_is_tcp(const struct protocol_variant *const prot)
+{
+	return prot_is_inet_stream(prot) &&
+	       (prot->protocol == IPPROTO_TCP || prot->protocol == IPPROTO_IP);
+}
+
+static bool prot_is_sctp(const struct protocol_variant *const prot)
+{
+	return prot_is_inet_stream(prot) && prot->protocol == IPPROTO_SCTP;
+}
+
+static bool prot_is_smc(const struct protocol_variant *const prot)
+{
+	return prot_is_inet_stream(prot) && prot->protocol == IPPROTO_SMC;
+}
+
+static bool prot_is_unix_stream(const struct protocol_variant *const prot)
+{
+	return prot->domain == AF_UNIX && prot->type == SOCK_STREAM;
+}
+
 static bool is_restricted(const struct protocol_variant *const prot,
 			  const enum sandbox_type sandbox)
 {
-	switch (prot->domain) {
-	case AF_INET:
-	case AF_INET6:
-		switch (prot->type) {
-		case SOCK_STREAM:
-			return sandbox == TCP_SANDBOX;
-		}
-		break;
-	}
-	return false;
+	return prot_is_tcp(prot) && sandbox == TCP_SANDBOX;
 }
 
 static int socket_variant(const struct service_fixture *const srv)
@@ -105,7 +134,7 @@ static int socket_variant(const struct service_fixture *const srv)
 	int ret;
 
 	ret = socket(srv->protocol.domain, srv->protocol.type | SOCK_CLOEXEC,
-		     0);
+		     srv->protocol.protocol);
 	if (ret < 0)
 		return -errno;
 	return ret;
@@ -124,7 +153,7 @@ static socklen_t get_addrlen(const struct service_fixture *const srv,
 		return sizeof(srv->ipv4_addr);
 
 	case AF_INET6:
-		if (minimal)
+		if (minimal && !prot_is_sctp(&srv->protocol))
 			return SIN6_LEN_RFC2133;
 		return sizeof(srv->ipv6_addr);
 
@@ -271,6 +300,11 @@ FIXTURE_SETUP(protocol)
 		.type = SOCK_STREAM,
 	};
 
+#if !SMC_SUPPORTED
+	if (prot_is_smc(&variant->prot))
+		SKIP(return, "SMC protocol is not supported.");
+#endif
+
 	disable_caps(_metadata);
 
 	ASSERT_EQ(0, set_service(&self->srv0, variant->prot, 0));
@@ -299,6 +333,39 @@ FIXTURE_VARIANT_ADD(protocol, no_sandbox_with_ipv4_tcp) {
 	},
 };
 
+/* clang-format off */
+FIXTURE_VARIANT_ADD(protocol, no_sandbox_with_ipv4_mptcp) {
+	/* clang-format on */
+	.sandbox = NO_SANDBOX,
+	.prot = {
+		.domain = AF_INET,
+		.type = SOCK_STREAM,
+		.protocol = IPPROTO_MPTCP,
+	},
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(protocol, no_sandbox_with_ipv4_sctp) {
+	/* clang-format on */
+	.sandbox = NO_SANDBOX,
+	.prot = {
+		.domain = AF_INET,
+		.type = SOCK_STREAM,
+		.protocol = IPPROTO_SCTP,
+	},
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(protocol, no_sandbox_with_ipv4_smc) {
+	/* clang-format on */
+	.sandbox = NO_SANDBOX,
+	.prot = {
+		.domain = AF_INET,
+		.type = SOCK_STREAM,
+		.protocol = IPPROTO_SMC,
+	},
+};
+
 /* clang-format off */
 FIXTURE_VARIANT_ADD(protocol, no_sandbox_with_ipv6_tcp) {
 	/* clang-format on */
@@ -309,6 +376,39 @@ FIXTURE_VARIANT_ADD(protocol, no_sandbox_with_ipv6_tcp) {
 	},
 };
 
+/* clang-format off */
+FIXTURE_VARIANT_ADD(protocol, no_sandbox_with_ipv6_mptcp) {
+	/* clang-format on */
+	.sandbox = NO_SANDBOX,
+	.prot = {
+		.domain = AF_INET6,
+		.type = SOCK_STREAM,
+		.protocol = IPPROTO_MPTCP,
+	},
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(protocol, no_sandbox_with_ipv6_sctp) {
+	/* clang-format on */
+	.sandbox = NO_SANDBOX,
+	.prot = {
+		.domain = AF_INET6,
+		.type = SOCK_STREAM,
+		.protocol = IPPROTO_SCTP,
+	},
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(protocol, no_sandbox_with_ipv6_smc) {
+	/* clang-format on */
+	.sandbox = NO_SANDBOX,
+	.prot = {
+		.domain = AF_INET6,
+		.type = SOCK_STREAM,
+		.protocol = IPPROTO_SMC,
+	},
+};
+
 /* clang-format off */
 FIXTURE_VARIANT_ADD(protocol, no_sandbox_with_ipv4_udp) {
 	/* clang-format on */
@@ -359,6 +459,39 @@ FIXTURE_VARIANT_ADD(protocol, tcp_sandbox_with_ipv4_tcp) {
 	},
 };
 
+/* clang-format off */
+FIXTURE_VARIANT_ADD(protocol, tcp_sandbox_with_ipv4_mptcp) {
+	/* clang-format on */
+	.sandbox = TCP_SANDBOX,
+	.prot = {
+		.domain = AF_INET,
+		.type = SOCK_STREAM,
+		.protocol = IPPROTO_MPTCP,
+	},
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(protocol, tcp_sandbox_with_ipv4_sctp) {
+	/* clang-format on */
+	.sandbox = TCP_SANDBOX,
+	.prot = {
+		.domain = AF_INET,
+		.type = SOCK_STREAM,
+		.protocol = IPPROTO_SCTP,
+	},
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(protocol, tcp_sandbox_with_ipv4_smc) {
+	/* clang-format on */
+	.sandbox = TCP_SANDBOX,
+	.prot = {
+		.domain = AF_INET,
+		.type = SOCK_STREAM,
+		.protocol = IPPROTO_SMC,
+	},
+};
+
 /* clang-format off */
 FIXTURE_VARIANT_ADD(protocol, tcp_sandbox_with_ipv6_tcp) {
 	/* clang-format on */
@@ -369,6 +502,39 @@ FIXTURE_VARIANT_ADD(protocol, tcp_sandbox_with_ipv6_tcp) {
 	},
 };
 
+/* clang-format off */
+FIXTURE_VARIANT_ADD(protocol, tcp_sandbox_with_ipv6_mptcp) {
+	/* clang-format on */
+	.sandbox = TCP_SANDBOX,
+	.prot = {
+		.domain = AF_INET6,
+		.type = SOCK_STREAM,
+		.protocol = IPPROTO_MPTCP,
+	},
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(protocol, tcp_sandbox_with_ipv6_sctp) {
+	/* clang-format on */
+	.sandbox = TCP_SANDBOX,
+	.prot = {
+		.domain = AF_INET6,
+		.type = SOCK_STREAM,
+		.protocol = IPPROTO_SCTP,
+	},
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(protocol, tcp_sandbox_with_ipv6_smc) {
+	/* clang-format on */
+	.sandbox = TCP_SANDBOX,
+	.prot = {
+		.domain = AF_INET6,
+		.type = SOCK_STREAM,
+		.protocol = IPPROTO_SMC,
+	},
+};
+
 /* clang-format off */
 FIXTURE_VARIANT_ADD(protocol, tcp_sandbox_with_ipv4_udp) {
 	/* clang-format on */
@@ -663,7 +829,7 @@ TEST_F(protocol, bind_unspec)
 
 	/* Allowed bind on AF_UNSPEC/INADDR_ANY. */
 	ret = bind_variant(bind_fd, &self->unspec_any0);
-	if (variant->prot.domain == AF_INET) {
+	if (variant->prot.domain == AF_INET && !prot_is_sctp(&variant->prot)) {
 		EXPECT_EQ(0, ret)
 		{
 			TH_LOG("Failed to bind to unspec/any socket: %s",
@@ -689,7 +855,7 @@ TEST_F(protocol, bind_unspec)
 
 	/* Denied bind on AF_UNSPEC/INADDR_ANY. */
 	ret = bind_variant(bind_fd, &self->unspec_any0);
-	if (variant->prot.domain == AF_INET) {
+	if (variant->prot.domain == AF_INET && !prot_is_sctp(&variant->prot)) {
 		if (is_restricted(&variant->prot, variant->sandbox)) {
 			EXPECT_EQ(-EACCES, ret);
 		} else {
@@ -727,6 +893,10 @@ TEST_F(protocol, connect_unspec)
 	int bind_fd, client_fd, status;
 	pid_t child;
 
+	if (prot_is_smc(&variant->prot))
+		SKIP(return, "SMC does not properly handles disconnect "
+			     "in the case of fallback to TCP");
+
 	/* Specific connection tests. */
 	bind_fd = socket_variant(&self->srv0);
 	ASSERT_LE(0, bind_fd);
@@ -769,17 +939,18 @@ TEST_F(protocol, connect_unspec)
 
 		/* Disconnects already connected socket, or set peer. */
 		ret = connect_variant(connect_fd, &self->unspec_any0);
-		if (self->srv0.protocol.domain == AF_UNIX &&
-		    self->srv0.protocol.type == SOCK_STREAM) {
+		if (prot_is_unix_stream(&variant->prot)) {
 			EXPECT_EQ(-EINVAL, ret);
+		} else if (prot_is_sctp(&variant->prot)) {
+			EXPECT_EQ(-EOPNOTSUPP, ret);
 		} else {
 			EXPECT_EQ(0, ret);
 		}
 
 		/* Tries to reconnect, or set peer. */
 		ret = connect_variant(connect_fd, &self->srv0);
-		if (self->srv0.protocol.domain == AF_UNIX &&
-		    self->srv0.protocol.type == SOCK_STREAM) {
+		if (prot_is_unix_stream(&variant->prot) ||
+		    prot_is_sctp(&variant->prot)) {
 			EXPECT_EQ(-EISCONN, ret);
 		} else {
 			EXPECT_EQ(0, ret);
@@ -796,9 +967,10 @@ TEST_F(protocol, connect_unspec)
 		}
 
 		ret = connect_variant(connect_fd, &self->unspec_any0);
-		if (self->srv0.protocol.domain == AF_UNIX &&
-		    self->srv0.protocol.type == SOCK_STREAM) {
+		if (prot_is_unix_stream(&variant->prot)) {
 			EXPECT_EQ(-EINVAL, ret);
+		} else if (prot_is_sctp(&variant->prot)) {
+			EXPECT_EQ(-EOPNOTSUPP, ret);
 		} else {
 			/* Always allowed to disconnect. */
 			EXPECT_EQ(0, ret);
-- 
2.34.1

Re: [RFC PATCH v1 1/2] landlock: Fix non-TCP sockets restriction

[Günther Noack]

On Thu, Oct 03, 2024 at 10:39:31PM +0800, Mikhail Ivanov wrote:
> Do not check TCP access right if socket protocol is not IPPROTO_TCP.
> LANDLOCK_ACCESS_NET_BIND_TCP and LANDLOCK_ACCESS_NET_CONNECT_TCP
> should not restrict bind(2) and connect(2) for non-TCP protocols
> (SCTP, MPTCP, SMC).
> 
> Closes: https://github.com/landlock-lsm/linux/issues/40
> Fixes: fff69fb03dde ("landlock: Support network rules with TCP bind and connect")
> Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
> ---
>  security/landlock/net.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/security/landlock/net.c b/security/landlock/net.c
> index bc3d943a7118..6f59dd98bb13 100644
> --- a/security/landlock/net.c
> +++ b/security/landlock/net.c
> @@ -68,7 +68,7 @@ static int current_check_access_socket(struct socket *const sock,
>  		return -EACCES;
>  
>  	/* Checks if it's a (potential) TCP socket. */
> -	if (sock->type != SOCK_STREAM)
> +	if (sock->type != SOCK_STREAM || sock->sk->sk_protocol != IPPROTO_TCP)
>  		return 0;
>  
>  	/* Checks for minimal header length to safely read sa_family. */
> -- 
> 2.34.1
> 

Thank you! Good catch!

Reviewed-by: Günther Noack <gnoack@google.com>

Re: [RFC PATCH v1 2/2] selftests/landlock: Test non-TCP INET connection-based protocols

[Günther Noack]

On Thu, Oct 03, 2024 at 10:39:32PM +0800, Mikhail Ivanov wrote:
> Extend protocol fixture with test suits for MPTCP, SCTP and SMC protocols.
> Add all options required by this protocols in config.
> 
> Extend protocol_variant structure with protocol field (Cf. socket(2)).
> 
> Refactor is_restricted() helper and add few helpers to check struct
> protocol_variant on specific protocols.
> 
> Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
> ---
>  tools/testing/selftests/landlock/common.h   |   1 +
>  tools/testing/selftests/landlock/config     |   5 +
>  tools/testing/selftests/landlock/net_test.c | 212 ++++++++++++++++++--
>  3 files changed, 198 insertions(+), 20 deletions(-)
> 
> diff --git a/tools/testing/selftests/landlock/common.h b/tools/testing/selftests/landlock/common.h
> index 61056fa074bb..40a2def50b83 100644
> --- a/tools/testing/selftests/landlock/common.h
> +++ b/tools/testing/selftests/landlock/common.h
> @@ -234,6 +234,7 @@ enforce_ruleset(struct __test_metadata *const _metadata, const int ruleset_fd)
>  struct protocol_variant {
>  	int domain;
>  	int type;
> +	int protocol;
>  };
>  
>  struct service_fixture {
> diff --git a/tools/testing/selftests/landlock/config b/tools/testing/selftests/landlock/config
> index 29af19c4e9f9..73b01d7d0881 100644
> --- a/tools/testing/selftests/landlock/config
> +++ b/tools/testing/selftests/landlock/config
> @@ -1,8 +1,12 @@
>  CONFIG_CGROUPS=y
>  CONFIG_CGROUP_SCHED=y
>  CONFIG_INET=y
> +CONFIG_INFINIBAND=y
> +CONFIG_IP_SCTP=y
>  CONFIG_IPV6=y
>  CONFIG_KEYS=y
> +CONFIG_MPTCP=y
> +CONFIG_MPTCP_IPV6=y
>  CONFIG_NET=y
>  CONFIG_NET_NS=y
>  CONFIG_OVERLAY_FS=y
> @@ -10,6 +14,7 @@ CONFIG_PROC_FS=y
>  CONFIG_SECURITY=y
>  CONFIG_SECURITY_LANDLOCK=y
>  CONFIG_SHMEM=y
> +CONFIG_SMC=y
>  CONFIG_SYSFS=y
>  CONFIG_TMPFS=y
>  CONFIG_TMPFS_XATTR=y
> diff --git a/tools/testing/selftests/landlock/net_test.c b/tools/testing/selftests/landlock/net_test.c
> index 4e0aeb53b225..dbe77d436281 100644
> --- a/tools/testing/selftests/landlock/net_test.c
> +++ b/tools/testing/selftests/landlock/net_test.c
> @@ -36,6 +36,17 @@ enum sandbox_type {
>  	TCP_SANDBOX,
>  };
>  
> +/* Checks if IPPROTO_SMC is present for compatibility reasons. */
> +#if !defined(__alpha__) && defined(IPPROTO_SMC)
> +#define SMC_SUPPORTED 1
> +#else
> +#define SMC_SUPPORTED 0
> +#endif
> +
> +#ifndef IPPROTO_SMC
> +#define IPPROTO_SMC 256
> +#endif
> +
>  static int set_service(struct service_fixture *const srv,
>  		       const struct protocol_variant prot,
>  		       const unsigned short index)
> @@ -85,19 +96,37 @@ static void setup_loopback(struct __test_metadata *const _metadata)
>  	clear_ambient_cap(_metadata, CAP_NET_ADMIN);
>  }
>  
> +static bool prot_is_inet_stream(const struct protocol_variant *const prot)
> +{
> +	return (prot->domain == AF_INET || prot->domain == AF_INET6) &&
> +	       prot->type == SOCK_STREAM;
> +}
> +
> +static bool prot_is_tcp(const struct protocol_variant *const prot)
> +{
> +	return prot_is_inet_stream(prot) &&
> +	       (prot->protocol == IPPROTO_TCP || prot->protocol == IPPROTO_IP);
> +}
> +
> +static bool prot_is_sctp(const struct protocol_variant *const prot)
> +{
> +	return prot_is_inet_stream(prot) && prot->protocol == IPPROTO_SCTP;
> +}
> +
> +static bool prot_is_smc(const struct protocol_variant *const prot)
> +{
> +	return prot_is_inet_stream(prot) && prot->protocol == IPPROTO_SMC;
> +}
> +
> +static bool prot_is_unix_stream(const struct protocol_variant *const prot)
> +{
> +	return prot->domain == AF_UNIX && prot->type == SOCK_STREAM;
> +}
> +
>  static bool is_restricted(const struct protocol_variant *const prot,
>  			  const enum sandbox_type sandbox)
>  {
> -	switch (prot->domain) {
> -	case AF_INET:
> -	case AF_INET6:
> -		switch (prot->type) {
> -		case SOCK_STREAM:
> -			return sandbox == TCP_SANDBOX;
> -		}
> -		break;
> -	}
> -	return false;
> +	return prot_is_tcp(prot) && sandbox == TCP_SANDBOX;
>  }
>  
>  static int socket_variant(const struct service_fixture *const srv)
> @@ -105,7 +134,7 @@ static int socket_variant(const struct service_fixture *const srv)
>  	int ret;
>  
>  	ret = socket(srv->protocol.domain, srv->protocol.type | SOCK_CLOEXEC,
> -		     0);
> +		     srv->protocol.protocol);
>  	if (ret < 0)
>  		return -errno;
>  	return ret;
> @@ -124,7 +153,7 @@ static socklen_t get_addrlen(const struct service_fixture *const srv,
>  		return sizeof(srv->ipv4_addr);
>  
>  	case AF_INET6:
> -		if (minimal)
> +		if (minimal && !prot_is_sctp(&srv->protocol))
>  			return SIN6_LEN_RFC2133;
>  		return sizeof(srv->ipv6_addr);
>  
> @@ -271,6 +300,11 @@ FIXTURE_SETUP(protocol)
>  		.type = SOCK_STREAM,
>  	};
>  
> +#if !SMC_SUPPORTED
> +	if (prot_is_smc(&variant->prot))
> +		SKIP(return, "SMC protocol is not supported.");
> +#endif
> +
>  	disable_caps(_metadata);
>  
>  	ASSERT_EQ(0, set_service(&self->srv0, variant->prot, 0));
> @@ -299,6 +333,39 @@ FIXTURE_VARIANT_ADD(protocol, no_sandbox_with_ipv4_tcp) {
>  	},
>  };
>  
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(protocol, no_sandbox_with_ipv4_mptcp) {
> +	/* clang-format on */
> +	.sandbox = NO_SANDBOX,
> +	.prot = {
> +		.domain = AF_INET,
> +		.type = SOCK_STREAM,
> +		.protocol = IPPROTO_MPTCP,
> +	},
> +};
> +
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(protocol, no_sandbox_with_ipv4_sctp) {
> +	/* clang-format on */
> +	.sandbox = NO_SANDBOX,
> +	.prot = {
> +		.domain = AF_INET,
> +		.type = SOCK_STREAM,
> +		.protocol = IPPROTO_SCTP,
> +	},
> +};
> +
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(protocol, no_sandbox_with_ipv4_smc) {
> +	/* clang-format on */
> +	.sandbox = NO_SANDBOX,
> +	.prot = {
> +		.domain = AF_INET,
> +		.type = SOCK_STREAM,
> +		.protocol = IPPROTO_SMC,
> +	},
> +};
> +
>  /* clang-format off */
>  FIXTURE_VARIANT_ADD(protocol, no_sandbox_with_ipv6_tcp) {
>  	/* clang-format on */
> @@ -309,6 +376,39 @@ FIXTURE_VARIANT_ADD(protocol, no_sandbox_with_ipv6_tcp) {
>  	},
>  };
>  
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(protocol, no_sandbox_with_ipv6_mptcp) {
> +	/* clang-format on */
> +	.sandbox = NO_SANDBOX,
> +	.prot = {
> +		.domain = AF_INET6,
> +		.type = SOCK_STREAM,
> +		.protocol = IPPROTO_MPTCP,
> +	},
> +};
> +
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(protocol, no_sandbox_with_ipv6_sctp) {
> +	/* clang-format on */
> +	.sandbox = NO_SANDBOX,
> +	.prot = {
> +		.domain = AF_INET6,
> +		.type = SOCK_STREAM,
> +		.protocol = IPPROTO_SCTP,
> +	},
> +};
> +
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(protocol, no_sandbox_with_ipv6_smc) {
> +	/* clang-format on */
> +	.sandbox = NO_SANDBOX,
> +	.prot = {
> +		.domain = AF_INET6,
> +		.type = SOCK_STREAM,
> +		.protocol = IPPROTO_SMC,
> +	},
> +};
> +
>  /* clang-format off */
>  FIXTURE_VARIANT_ADD(protocol, no_sandbox_with_ipv4_udp) {
>  	/* clang-format on */
> @@ -359,6 +459,39 @@ FIXTURE_VARIANT_ADD(protocol, tcp_sandbox_with_ipv4_tcp) {
>  	},
>  };
>  
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(protocol, tcp_sandbox_with_ipv4_mptcp) {
> +	/* clang-format on */
> +	.sandbox = TCP_SANDBOX,
> +	.prot = {
> +		.domain = AF_INET,
> +		.type = SOCK_STREAM,
> +		.protocol = IPPROTO_MPTCP,
> +	},
> +};
> +
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(protocol, tcp_sandbox_with_ipv4_sctp) {
> +	/* clang-format on */
> +	.sandbox = TCP_SANDBOX,
> +	.prot = {
> +		.domain = AF_INET,
> +		.type = SOCK_STREAM,
> +		.protocol = IPPROTO_SCTP,
> +	},
> +};
> +
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(protocol, tcp_sandbox_with_ipv4_smc) {
> +	/* clang-format on */
> +	.sandbox = TCP_SANDBOX,
> +	.prot = {
> +		.domain = AF_INET,
> +		.type = SOCK_STREAM,
> +		.protocol = IPPROTO_SMC,
> +	},
> +};
> +
>  /* clang-format off */
>  FIXTURE_VARIANT_ADD(protocol, tcp_sandbox_with_ipv6_tcp) {
>  	/* clang-format on */
> @@ -369,6 +502,39 @@ FIXTURE_VARIANT_ADD(protocol, tcp_sandbox_with_ipv6_tcp) {
>  	},
>  };
>  
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(protocol, tcp_sandbox_with_ipv6_mptcp) {
> +	/* clang-format on */
> +	.sandbox = TCP_SANDBOX,
> +	.prot = {
> +		.domain = AF_INET6,
> +		.type = SOCK_STREAM,
> +		.protocol = IPPROTO_MPTCP,
> +	},
> +};
> +
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(protocol, tcp_sandbox_with_ipv6_sctp) {
> +	/* clang-format on */
> +	.sandbox = TCP_SANDBOX,
> +	.prot = {
> +		.domain = AF_INET6,
> +		.type = SOCK_STREAM,
> +		.protocol = IPPROTO_SCTP,
> +	},
> +};
> +
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(protocol, tcp_sandbox_with_ipv6_smc) {
> +	/* clang-format on */
> +	.sandbox = TCP_SANDBOX,
> +	.prot = {
> +		.domain = AF_INET6,
> +		.type = SOCK_STREAM,
> +		.protocol = IPPROTO_SMC,
> +	},
> +};
> +
>  /* clang-format off */
>  FIXTURE_VARIANT_ADD(protocol, tcp_sandbox_with_ipv4_udp) {
>  	/* clang-format on */
> @@ -663,7 +829,7 @@ TEST_F(protocol, bind_unspec)
>  
>  	/* Allowed bind on AF_UNSPEC/INADDR_ANY. */
>  	ret = bind_variant(bind_fd, &self->unspec_any0);
> -	if (variant->prot.domain == AF_INET) {
> +	if (variant->prot.domain == AF_INET && !prot_is_sctp(&variant->prot)) {
>  		EXPECT_EQ(0, ret)
>  		{
>  			TH_LOG("Failed to bind to unspec/any socket: %s",
> @@ -689,7 +855,7 @@ TEST_F(protocol, bind_unspec)
>  
>  	/* Denied bind on AF_UNSPEC/INADDR_ANY. */
>  	ret = bind_variant(bind_fd, &self->unspec_any0);
> -	if (variant->prot.domain == AF_INET) {
> +	if (variant->prot.domain == AF_INET && !prot_is_sctp(&variant->prot)) {
>  		if (is_restricted(&variant->prot, variant->sandbox)) {
>  			EXPECT_EQ(-EACCES, ret);
>  		} else {
> @@ -727,6 +893,10 @@ TEST_F(protocol, connect_unspec)
>  	int bind_fd, client_fd, status;
>  	pid_t child;
>  
> +	if (prot_is_smc(&variant->prot))
> +		SKIP(return, "SMC does not properly handles disconnect "
> +			     "in the case of fallback to TCP");
> +
>  	/* Specific connection tests. */
>  	bind_fd = socket_variant(&self->srv0);
>  	ASSERT_LE(0, bind_fd);
> @@ -769,17 +939,18 @@ TEST_F(protocol, connect_unspec)
>  
>  		/* Disconnects already connected socket, or set peer. */
>  		ret = connect_variant(connect_fd, &self->unspec_any0);
> -		if (self->srv0.protocol.domain == AF_UNIX &&
> -		    self->srv0.protocol.type == SOCK_STREAM) {
> +		if (prot_is_unix_stream(&variant->prot)) {
>  			EXPECT_EQ(-EINVAL, ret);
> +		} else if (prot_is_sctp(&variant->prot)) {
> +			EXPECT_EQ(-EOPNOTSUPP, ret);
>  		} else {
>  			EXPECT_EQ(0, ret);
>  		}
>  
>  		/* Tries to reconnect, or set peer. */
>  		ret = connect_variant(connect_fd, &self->srv0);
> -		if (self->srv0.protocol.domain == AF_UNIX &&
> -		    self->srv0.protocol.type == SOCK_STREAM) {
> +		if (prot_is_unix_stream(&variant->prot) ||
> +		    prot_is_sctp(&variant->prot)) {
>  			EXPECT_EQ(-EISCONN, ret);
>  		} else {
>  			EXPECT_EQ(0, ret);
> @@ -796,9 +967,10 @@ TEST_F(protocol, connect_unspec)
>  		}
>  
>  		ret = connect_variant(connect_fd, &self->unspec_any0);
> -		if (self->srv0.protocol.domain == AF_UNIX &&
> -		    self->srv0.protocol.type == SOCK_STREAM) {
> +		if (prot_is_unix_stream(&variant->prot)) {
>  			EXPECT_EQ(-EINVAL, ret);
> +		} else if (prot_is_sctp(&variant->prot)) {
> +			EXPECT_EQ(-EOPNOTSUPP, ret);
>  		} else {
>  			/* Always allowed to disconnect. */
>  			EXPECT_EQ(0, ret);
> -- 
> 2.34.1
> 

Looks good.

Reviewed-by: Günther Noack <gnoack@google.com>

Re: [RFC PATCH v1 1/2] landlock: Fix non-TCP sockets restriction

[Mickaël Salaün]

Please also add Matthieu in Cc for the network patch series.

On Thu, Oct 03, 2024 at 10:39:31PM +0800, Mikhail Ivanov wrote:
> Do not check TCP access right if socket protocol is not IPPROTO_TCP.
> LANDLOCK_ACCESS_NET_BIND_TCP and LANDLOCK_ACCESS_NET_CONNECT_TCP
> should not restrict bind(2) and connect(2) for non-TCP protocols
> (SCTP, MPTCP, SMC).
> 
> Closes: https://github.com/landlock-lsm/linux/issues/40
> Fixes: fff69fb03dde ("landlock: Support network rules with TCP bind and connect")
> Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
> ---
>  security/landlock/net.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/security/landlock/net.c b/security/landlock/net.c
> index bc3d943a7118..6f59dd98bb13 100644
> --- a/security/landlock/net.c
> +++ b/security/landlock/net.c
> @@ -68,7 +68,7 @@ static int current_check_access_socket(struct socket *const sock,
>  		return -EACCES;
>  
>  	/* Checks if it's a (potential) TCP socket. */

We can extend this comment to explain that we don't use sk_is_tcp()
because we need to handle the AF_UNSPEC case.

> -	if (sock->type != SOCK_STREAM)
> +	if (sock->type != SOCK_STREAM || sock->sk->sk_protocol != IPPROTO_TCP)

I think we should check sock->sk->sk_type instead of sock->type (even if
it should be the same).  To make it simpler, we should only use sk in
current_check_access_socket():
struct sock *sk = sock->sk;

Could you please also do s/__sk_common\.skc_/sk_/g ?

>  		return 0;
>  
>  	/* Checks for minimal header length to safely read sa_family. */
> -- 
> 2.34.1
> 
> 

Re: [RFC PATCH v1 2/2] selftests/landlock: Test non-TCP INET connection-based protocols

[Mickaël Salaün]

On Thu, Oct 03, 2024 at 10:39:32PM +0800, Mikhail Ivanov wrote:
> Extend protocol fixture with test suits for MPTCP, SCTP and SMC protocols.
> Add all options required by this protocols in config.

Great coverage!  It's nice to check against SCTP and MPTCP, but as you
were wondering, I think you can remove the SMC protocol to simplify
tests. MPTCP seems to work similarly as TCP wrt AF_UNSPEC, so it might
be worth keeping it, and we might want to control these protocols too
one day.

> 
> Extend protocol_variant structure with protocol field (Cf. socket(2)).
> 
> Refactor is_restricted() helper and add few helpers to check struct
> protocol_variant on specific protocols.

> 
> Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
> ---
>  tools/testing/selftests/landlock/common.h   |   1 +
>  tools/testing/selftests/landlock/config     |   5 +
>  tools/testing/selftests/landlock/net_test.c | 212 ++++++++++++++++++--
>  3 files changed, 198 insertions(+), 20 deletions(-)
> 
> diff --git a/tools/testing/selftests/landlock/common.h b/tools/testing/selftests/landlock/common.h
> index 61056fa074bb..40a2def50b83 100644
> --- a/tools/testing/selftests/landlock/common.h
> +++ b/tools/testing/selftests/landlock/common.h
> @@ -234,6 +234,7 @@ enforce_ruleset(struct __test_metadata *const _metadata, const int ruleset_fd)
>  struct protocol_variant {
>  	int domain;
>  	int type;
> +	int protocol;
>  };
>  
>  struct service_fixture {
> diff --git a/tools/testing/selftests/landlock/config b/tools/testing/selftests/landlock/config
> index 29af19c4e9f9..73b01d7d0881 100644
> --- a/tools/testing/selftests/landlock/config
> +++ b/tools/testing/selftests/landlock/config
> @@ -1,8 +1,12 @@
>  CONFIG_CGROUPS=y
>  CONFIG_CGROUP_SCHED=y
>  CONFIG_INET=y
> +CONFIG_INFINIBAND=y

Without SMC this infiniband should not be required.

> +CONFIG_IP_SCTP=y
>  CONFIG_IPV6=y
>  CONFIG_KEYS=y
> +CONFIG_MPTCP=y
> +CONFIG_MPTCP_IPV6=y
>  CONFIG_NET=y
>  CONFIG_NET_NS=y
>  CONFIG_OVERLAY_FS=y
> @@ -10,6 +14,7 @@ CONFIG_PROC_FS=y
>  CONFIG_SECURITY=y
>  CONFIG_SECURITY_LANDLOCK=y
>  CONFIG_SHMEM=y
> +CONFIG_SMC=y
>  CONFIG_SYSFS=y
>  CONFIG_TMPFS=y
>  CONFIG_TMPFS_XATTR=y
> diff --git a/tools/testing/selftests/landlock/net_test.c b/tools/testing/selftests/landlock/net_test.c
> index 4e0aeb53b225..dbe77d436281 100644
> --- a/tools/testing/selftests/landlock/net_test.c
> +++ b/tools/testing/selftests/landlock/net_test.c
> @@ -36,6 +36,17 @@ enum sandbox_type {
>  	TCP_SANDBOX,
>  };
>  
> +/* Checks if IPPROTO_SMC is present for compatibility reasons. */
> +#if !defined(__alpha__) && defined(IPPROTO_SMC)
> +#define SMC_SUPPORTED 1
> +#else
> +#define SMC_SUPPORTED 0
> +#endif
> +
> +#ifndef IPPROTO_SMC
> +#define IPPROTO_SMC 256
> +#endif
> +
>  static int set_service(struct service_fixture *const srv,
>  		       const struct protocol_variant prot,
>  		       const unsigned short index)
> @@ -85,19 +96,37 @@ static void setup_loopback(struct __test_metadata *const _metadata)
>  	clear_ambient_cap(_metadata, CAP_NET_ADMIN);
>  }
>  
> +static bool prot_is_inet_stream(const struct protocol_variant *const prot)
> +{
> +	return (prot->domain == AF_INET || prot->domain == AF_INET6) &&
> +	       prot->type == SOCK_STREAM;
> +}
> +
> +static bool prot_is_tcp(const struct protocol_variant *const prot)
> +{
> +	return prot_is_inet_stream(prot) &&
> +	       (prot->protocol == IPPROTO_TCP || prot->protocol == IPPROTO_IP);

Why do we need to check against IPPROTO_IP?

> +}
> +
> +static bool prot_is_sctp(const struct protocol_variant *const prot)
> +{
> +	return prot_is_inet_stream(prot) && prot->protocol == IPPROTO_SCTP;
> +}
> +
> +static bool prot_is_smc(const struct protocol_variant *const prot)
> +{
> +	return prot_is_inet_stream(prot) && prot->protocol == IPPROTO_SMC;
> +}
> +
> +static bool prot_is_unix_stream(const struct protocol_variant *const prot)
> +{
> +	return prot->domain == AF_UNIX && prot->type == SOCK_STREAM;
> +}
> +
>  static bool is_restricted(const struct protocol_variant *const prot,
>  			  const enum sandbox_type sandbox)
>  {
> -	switch (prot->domain) {
> -	case AF_INET:
> -	case AF_INET6:
> -		switch (prot->type) {
> -		case SOCK_STREAM:
> -			return sandbox == TCP_SANDBOX;
> -		}
> -		break;
> -	}
> -	return false;
> +	return prot_is_tcp(prot) && sandbox == TCP_SANDBOX;
>  }
>  
>  static int socket_variant(const struct service_fixture *const srv)
> @@ -105,7 +134,7 @@ static int socket_variant(const struct service_fixture *const srv)
>  	int ret;
>  
>  	ret = socket(srv->protocol.domain, srv->protocol.type | SOCK_CLOEXEC,
> -		     0);
> +		     srv->protocol.protocol);
>  	if (ret < 0)
>  		return -errno;
>  	return ret;
> @@ -124,7 +153,7 @@ static socklen_t get_addrlen(const struct service_fixture *const srv,
>  		return sizeof(srv->ipv4_addr);
>  
>  	case AF_INET6:
> -		if (minimal)
> +		if (minimal && !prot_is_sctp(&srv->protocol))

Why SCTP requires this exception?

>  			return SIN6_LEN_RFC2133;
>  		return sizeof(srv->ipv6_addr);
>  
> @@ -271,6 +300,11 @@ FIXTURE_SETUP(protocol)
>  		.type = SOCK_STREAM,
>  	};
>  
> +#if !SMC_SUPPORTED
> +	if (prot_is_smc(&variant->prot))
> +		SKIP(return, "SMC protocol is not supported.");
> +#endif
> +
>  	disable_caps(_metadata);
>  
>  	ASSERT_EQ(0, set_service(&self->srv0, variant->prot, 0));
> @@ -299,6 +333,39 @@ FIXTURE_VARIANT_ADD(protocol, no_sandbox_with_ipv4_tcp) {
>  	},
>  };
>  
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(protocol, no_sandbox_with_ipv4_mptcp) {
> +	/* clang-format on */
> +	.sandbox = NO_SANDBOX,
> +	.prot = {
> +		.domain = AF_INET,
> +		.type = SOCK_STREAM,
> +		.protocol = IPPROTO_MPTCP,
> +	},
> +};
> +
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(protocol, no_sandbox_with_ipv4_sctp) {
> +	/* clang-format on */
> +	.sandbox = NO_SANDBOX,
> +	.prot = {
> +		.domain = AF_INET,
> +		.type = SOCK_STREAM,
> +		.protocol = IPPROTO_SCTP,
> +	},
> +};
> +
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(protocol, no_sandbox_with_ipv4_smc) {
> +	/* clang-format on */
> +	.sandbox = NO_SANDBOX,
> +	.prot = {
> +		.domain = AF_INET,
> +		.type = SOCK_STREAM,
> +		.protocol = IPPROTO_SMC,
> +	},
> +};
> +
>  /* clang-format off */
>  FIXTURE_VARIANT_ADD(protocol, no_sandbox_with_ipv6_tcp) {
>  	/* clang-format on */
> @@ -309,6 +376,39 @@ FIXTURE_VARIANT_ADD(protocol, no_sandbox_with_ipv6_tcp) {
>  	},
>  };
>  
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(protocol, no_sandbox_with_ipv6_mptcp) {
> +	/* clang-format on */
> +	.sandbox = NO_SANDBOX,
> +	.prot = {
> +		.domain = AF_INET6,
> +		.type = SOCK_STREAM,
> +		.protocol = IPPROTO_MPTCP,
> +	},
> +};
> +
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(protocol, no_sandbox_with_ipv6_sctp) {
> +	/* clang-format on */
> +	.sandbox = NO_SANDBOX,
> +	.prot = {
> +		.domain = AF_INET6,
> +		.type = SOCK_STREAM,
> +		.protocol = IPPROTO_SCTP,
> +	},
> +};
> +
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(protocol, no_sandbox_with_ipv6_smc) {
> +	/* clang-format on */
> +	.sandbox = NO_SANDBOX,
> +	.prot = {
> +		.domain = AF_INET6,
> +		.type = SOCK_STREAM,
> +		.protocol = IPPROTO_SMC,
> +	},
> +};
> +
>  /* clang-format off */
>  FIXTURE_VARIANT_ADD(protocol, no_sandbox_with_ipv4_udp) {
>  	/* clang-format on */
> @@ -359,6 +459,39 @@ FIXTURE_VARIANT_ADD(protocol, tcp_sandbox_with_ipv4_tcp) {
>  	},
>  };
>  
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(protocol, tcp_sandbox_with_ipv4_mptcp) {
> +	/* clang-format on */
> +	.sandbox = TCP_SANDBOX,
> +	.prot = {
> +		.domain = AF_INET,
> +		.type = SOCK_STREAM,
> +		.protocol = IPPROTO_MPTCP,
> +	},
> +};
> +
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(protocol, tcp_sandbox_with_ipv4_sctp) {
> +	/* clang-format on */
> +	.sandbox = TCP_SANDBOX,
> +	.prot = {
> +		.domain = AF_INET,
> +		.type = SOCK_STREAM,
> +		.protocol = IPPROTO_SCTP,
> +	},
> +};
> +
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(protocol, tcp_sandbox_with_ipv4_smc) {
> +	/* clang-format on */
> +	.sandbox = TCP_SANDBOX,
> +	.prot = {
> +		.domain = AF_INET,
> +		.type = SOCK_STREAM,
> +		.protocol = IPPROTO_SMC,
> +	},
> +};
> +
>  /* clang-format off */
>  FIXTURE_VARIANT_ADD(protocol, tcp_sandbox_with_ipv6_tcp) {
>  	/* clang-format on */
> @@ -369,6 +502,39 @@ FIXTURE_VARIANT_ADD(protocol, tcp_sandbox_with_ipv6_tcp) {
>  	},
>  };
>  
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(protocol, tcp_sandbox_with_ipv6_mptcp) {
> +	/* clang-format on */
> +	.sandbox = TCP_SANDBOX,
> +	.prot = {
> +		.domain = AF_INET6,
> +		.type = SOCK_STREAM,
> +		.protocol = IPPROTO_MPTCP,
> +	},
> +};
> +
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(protocol, tcp_sandbox_with_ipv6_sctp) {
> +	/* clang-format on */
> +	.sandbox = TCP_SANDBOX,
> +	.prot = {
> +		.domain = AF_INET6,
> +		.type = SOCK_STREAM,
> +		.protocol = IPPROTO_SCTP,
> +	},
> +};
> +
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(protocol, tcp_sandbox_with_ipv6_smc) {
> +	/* clang-format on */
> +	.sandbox = TCP_SANDBOX,
> +	.prot = {
> +		.domain = AF_INET6,
> +		.type = SOCK_STREAM,
> +		.protocol = IPPROTO_SMC,
> +	},
> +};
> +
>  /* clang-format off */
>  FIXTURE_VARIANT_ADD(protocol, tcp_sandbox_with_ipv4_udp) {
>  	/* clang-format on */
> @@ -663,7 +829,7 @@ TEST_F(protocol, bind_unspec)
>  
>  	/* Allowed bind on AF_UNSPEC/INADDR_ANY. */
>  	ret = bind_variant(bind_fd, &self->unspec_any0);
> -	if (variant->prot.domain == AF_INET) {
> +	if (variant->prot.domain == AF_INET && !prot_is_sctp(&variant->prot)) {
>  		EXPECT_EQ(0, ret)
>  		{
>  			TH_LOG("Failed to bind to unspec/any socket: %s",
> @@ -689,7 +855,7 @@ TEST_F(protocol, bind_unspec)
>  
>  	/* Denied bind on AF_UNSPEC/INADDR_ANY. */
>  	ret = bind_variant(bind_fd, &self->unspec_any0);
> -	if (variant->prot.domain == AF_INET) {
> +	if (variant->prot.domain == AF_INET && !prot_is_sctp(&variant->prot)) {

It looks like we need the same exception for the next bind_variant()
call.

>  		if (is_restricted(&variant->prot, variant->sandbox)) {
>  			EXPECT_EQ(-EACCES, ret);
>  		} else {
> @@ -727,6 +893,10 @@ TEST_F(protocol, connect_unspec)
>  	int bind_fd, client_fd, status;
>  	pid_t child;
>  
> +	if (prot_is_smc(&variant->prot))
> +		SKIP(return, "SMC does not properly handles disconnect "
> +			     "in the case of fallback to TCP");
> +
>  	/* Specific connection tests. */
>  	bind_fd = socket_variant(&self->srv0);
>  	ASSERT_LE(0, bind_fd);
> @@ -769,17 +939,18 @@ TEST_F(protocol, connect_unspec)
>  
>  		/* Disconnects already connected socket, or set peer. */
>  		ret = connect_variant(connect_fd, &self->unspec_any0);
> -		if (self->srv0.protocol.domain == AF_UNIX &&
> -		    self->srv0.protocol.type == SOCK_STREAM) {
> +		if (prot_is_unix_stream(&variant->prot)) {
>  			EXPECT_EQ(-EINVAL, ret);
> +		} else if (prot_is_sctp(&variant->prot)) {
> +			EXPECT_EQ(-EOPNOTSUPP, ret);
>  		} else {
>  			EXPECT_EQ(0, ret);
>  		}
>  
>  		/* Tries to reconnect, or set peer. */
>  		ret = connect_variant(connect_fd, &self->srv0);
> -		if (self->srv0.protocol.domain == AF_UNIX &&
> -		    self->srv0.protocol.type == SOCK_STREAM) {
> +		if (prot_is_unix_stream(&variant->prot) ||
> +		    prot_is_sctp(&variant->prot)) {
>  			EXPECT_EQ(-EISCONN, ret);
>  		} else {
>  			EXPECT_EQ(0, ret);
> @@ -796,9 +967,10 @@ TEST_F(protocol, connect_unspec)
>  		}
>  
>  		ret = connect_variant(connect_fd, &self->unspec_any0);
> -		if (self->srv0.protocol.domain == AF_UNIX &&
> -		    self->srv0.protocol.type == SOCK_STREAM) {
> +		if (prot_is_unix_stream(&variant->prot)) {
>  			EXPECT_EQ(-EINVAL, ret);
> +		} else if (prot_is_sctp(&variant->prot)) {
> +			EXPECT_EQ(-EOPNOTSUPP, ret);
>  		} else {
>  			/* Always allowed to disconnect. */
>  			EXPECT_EQ(0, ret);
> -- 
> 2.34.1
> 
> 

Re: [RFC PATCH v1 2/2] selftests/landlock: Test non-TCP INET connection-based protocols

[Mikhail Ivanov]

On 10/3/2024 8:45 PM, Mickaël Salaün wrote:
> On Thu, Oct 03, 2024 at 10:39:32PM +0800, Mikhail Ivanov wrote:
>> Extend protocol fixture with test suits for MPTCP, SCTP and SMC protocols.
>> Add all options required by this protocols in config.
> 
> Great coverage!  It's nice to check against SCTP and MPTCP, but as you
> were wondering, I think you can remove the SMC protocol to simplify
> tests. MPTCP seems to work similarly as TCP wrt AF_UNSPEC, so it might
> be worth keeping it, and we might want to control these protocols too
> one day.

Thanks! I'll remove SMC then.

> 
>>
>> Extend protocol_variant structure with protocol field (Cf. socket(2)).
>>
>> Refactor is_restricted() helper and add few helpers to check struct
>> protocol_variant on specific protocols.
> 
>>
>> Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
>> ---
>>   tools/testing/selftests/landlock/common.h   |   1 +
>>   tools/testing/selftests/landlock/config     |   5 +
>>   tools/testing/selftests/landlock/net_test.c | 212 ++++++++++++++++++--
>>   3 files changed, 198 insertions(+), 20 deletions(-)
>>
>> diff --git a/tools/testing/selftests/landlock/common.h b/tools/testing/selftests/landlock/common.h
>> index 61056fa074bb..40a2def50b83 100644
>> --- a/tools/testing/selftests/landlock/common.h
>> +++ b/tools/testing/selftests/landlock/common.h
>> @@ -234,6 +234,7 @@ enforce_ruleset(struct __test_metadata *const _metadata, const int ruleset_fd)
>>   struct protocol_variant {
>>   	int domain;
>>   	int type;
>> +	int protocol;
>>   };
>>   
>>   struct service_fixture {
>> diff --git a/tools/testing/selftests/landlock/config b/tools/testing/selftests/landlock/config
>> index 29af19c4e9f9..73b01d7d0881 100644
>> --- a/tools/testing/selftests/landlock/config
>> +++ b/tools/testing/selftests/landlock/config
>> @@ -1,8 +1,12 @@
>>   CONFIG_CGROUPS=y
>>   CONFIG_CGROUP_SCHED=y
>>   CONFIG_INET=y
>> +CONFIG_INFINIBAND=y
> 
> Without SMC this infiniband should not be required.

yeap

> 
>> +CONFIG_IP_SCTP=y
>>   CONFIG_IPV6=y
>>   CONFIG_KEYS=y
>> +CONFIG_MPTCP=y
>> +CONFIG_MPTCP_IPV6=y
>>   CONFIG_NET=y
>>   CONFIG_NET_NS=y
>>   CONFIG_OVERLAY_FS=y
>> @@ -10,6 +14,7 @@ CONFIG_PROC_FS=y
>>   CONFIG_SECURITY=y
>>   CONFIG_SECURITY_LANDLOCK=y
>>   CONFIG_SHMEM=y
>> +CONFIG_SMC=y
>>   CONFIG_SYSFS=y
>>   CONFIG_TMPFS=y
>>   CONFIG_TMPFS_XATTR=y
>> diff --git a/tools/testing/selftests/landlock/net_test.c b/tools/testing/selftests/landlock/net_test.c
>> index 4e0aeb53b225..dbe77d436281 100644
>> --- a/tools/testing/selftests/landlock/net_test.c
>> +++ b/tools/testing/selftests/landlock/net_test.c
>> @@ -36,6 +36,17 @@ enum sandbox_type {
>>   	TCP_SANDBOX,
>>   };
>>   
>> +/* Checks if IPPROTO_SMC is present for compatibility reasons. */
>> +#if !defined(__alpha__) && defined(IPPROTO_SMC)
>> +#define SMC_SUPPORTED 1
>> +#else
>> +#define SMC_SUPPORTED 0
>> +#endif
>> +
>> +#ifndef IPPROTO_SMC
>> +#define IPPROTO_SMC 256
>> +#endif
>> +
>>   static int set_service(struct service_fixture *const srv,
>>   		       const struct protocol_variant prot,
>>   		       const unsigned short index)
>> @@ -85,19 +96,37 @@ static void setup_loopback(struct __test_metadata *const _metadata)
>>   	clear_ambient_cap(_metadata, CAP_NET_ADMIN);
>>   }
>>   
>> +static bool prot_is_inet_stream(const struct protocol_variant *const prot)
>> +{
>> +	return (prot->domain == AF_INET || prot->domain == AF_INET6) &&
>> +	       prot->type == SOCK_STREAM;
>> +}
>> +
>> +static bool prot_is_tcp(const struct protocol_variant *const prot)
>> +{
>> +	return prot_is_inet_stream(prot) &&
>> +	       (prot->protocol == IPPROTO_TCP || prot->protocol == IPPROTO_IP);
> 
> Why do we need to check against IPPROTO_IP?

IPPROTO_IP = 0 and can be used as an alias for IPPROTO_TCP (=6) in
socket(2) (also for IPPROTO_UDP(=17), Cf. inet_create).

Since we create TCP sockets in a common way here (with protocol = 0),
checking against IPPROTO_TCP is not necessary, but I decided to leave it
for completeness.

> 
>> +}
>> +
>> +static bool prot_is_sctp(const struct protocol_variant *const prot)
>> +{
>> +	return prot_is_inet_stream(prot) && prot->protocol == IPPROTO_SCTP;
>> +}
>> +
>> +static bool prot_is_smc(const struct protocol_variant *const prot)
>> +{
>> +	return prot_is_inet_stream(prot) && prot->protocol == IPPROTO_SMC;
>> +}
>> +
>> +static bool prot_is_unix_stream(const struct protocol_variant *const prot)
>> +{
>> +	return prot->domain == AF_UNIX && prot->type == SOCK_STREAM;
>> +}
>> +
>>   static bool is_restricted(const struct protocol_variant *const prot,
>>   			  const enum sandbox_type sandbox)
>>   {
>> -	switch (prot->domain) {
>> -	case AF_INET:
>> -	case AF_INET6:
>> -		switch (prot->type) {
>> -		case SOCK_STREAM:
>> -			return sandbox == TCP_SANDBOX;
>> -		}
>> -		break;
>> -	}
>> -	return false;
>> +	return prot_is_tcp(prot) && sandbox == TCP_SANDBOX;
>>   }
>>   
>>   static int socket_variant(const struct service_fixture *const srv)
>> @@ -105,7 +134,7 @@ static int socket_variant(const struct service_fixture *const srv)
>>   	int ret;
>>   
>>   	ret = socket(srv->protocol.domain, srv->protocol.type | SOCK_CLOEXEC,
>> -		     0);
>> +		     srv->protocol.protocol);
>>   	if (ret < 0)
>>   		return -errno;
>>   	return ret;
>> @@ -124,7 +153,7 @@ static socklen_t get_addrlen(const struct service_fixture *const srv,
>>   		return sizeof(srv->ipv4_addr);
>>   
>>   	case AF_INET6:
>> -		if (minimal)
>> +		if (minimal && !prot_is_sctp(&srv->protocol))
> 
> Why SCTP requires this exception?

SCTP implementation (possibly incorrectly) checks that address length is
at least sizeof(struct sockaddr_in6) (Cf. sctp_sockaddr_af() for bind(2)
and in sctp_connect() for connect(2)).

> 
>>   			return SIN6_LEN_RFC2133;
>>   		return sizeof(srv->ipv6_addr);
>>   
>> @@ -271,6 +300,11 @@ FIXTURE_SETUP(protocol)
>>   		.type = SOCK_STREAM,
>>   	};
>>   
>> +#if !SMC_SUPPORTED
>> +	if (prot_is_smc(&variant->prot))
>> +		SKIP(return, "SMC protocol is not supported.");
>> +#endif
>> +
>>   	disable_caps(_metadata);
>>   
>>   	ASSERT_EQ(0, set_service(&self->srv0, variant->prot, 0));
>> @@ -299,6 +333,39 @@ FIXTURE_VARIANT_ADD(protocol, no_sandbox_with_ipv4_tcp) {
>>   	},
>>   };
>>   
>> +/* clang-format off */
>> +FIXTURE_VARIANT_ADD(protocol, no_sandbox_with_ipv4_mptcp) {
>> +	/* clang-format on */
>> +	.sandbox = NO_SANDBOX,
>> +	.prot = {
>> +		.domain = AF_INET,
>> +		.type = SOCK_STREAM,
>> +		.protocol = IPPROTO_MPTCP,
>> +	},
>> +};
>> +
>> +/* clang-format off */
>> +FIXTURE_VARIANT_ADD(protocol, no_sandbox_with_ipv4_sctp) {
>> +	/* clang-format on */
>> +	.sandbox = NO_SANDBOX,
>> +	.prot = {
>> +		.domain = AF_INET,
>> +		.type = SOCK_STREAM,
>> +		.protocol = IPPROTO_SCTP,
>> +	},
>> +};
>> +
>> +/* clang-format off */
>> +FIXTURE_VARIANT_ADD(protocol, no_sandbox_with_ipv4_smc) {
>> +	/* clang-format on */
>> +	.sandbox = NO_SANDBOX,
>> +	.prot = {
>> +		.domain = AF_INET,
>> +		.type = SOCK_STREAM,
>> +		.protocol = IPPROTO_SMC,
>> +	},
>> +};
>> +
>>   /* clang-format off */
>>   FIXTURE_VARIANT_ADD(protocol, no_sandbox_with_ipv6_tcp) {
>>   	/* clang-format on */
>> @@ -309,6 +376,39 @@ FIXTURE_VARIANT_ADD(protocol, no_sandbox_with_ipv6_tcp) {
>>   	},
>>   };
>>   
>> +/* clang-format off */
>> +FIXTURE_VARIANT_ADD(protocol, no_sandbox_with_ipv6_mptcp) {
>> +	/* clang-format on */
>> +	.sandbox = NO_SANDBOX,
>> +	.prot = {
>> +		.domain = AF_INET6,
>> +		.type = SOCK_STREAM,
>> +		.protocol = IPPROTO_MPTCP,
>> +	},
>> +};
>> +
>> +/* clang-format off */
>> +FIXTURE_VARIANT_ADD(protocol, no_sandbox_with_ipv6_sctp) {
>> +	/* clang-format on */
>> +	.sandbox = NO_SANDBOX,
>> +	.prot = {
>> +		.domain = AF_INET6,
>> +		.type = SOCK_STREAM,
>> +		.protocol = IPPROTO_SCTP,
>> +	},
>> +};
>> +
>> +/* clang-format off */
>> +FIXTURE_VARIANT_ADD(protocol, no_sandbox_with_ipv6_smc) {
>> +	/* clang-format on */
>> +	.sandbox = NO_SANDBOX,
>> +	.prot = {
>> +		.domain = AF_INET6,
>> +		.type = SOCK_STREAM,
>> +		.protocol = IPPROTO_SMC,
>> +	},
>> +};
>> +
>>   /* clang-format off */
>>   FIXTURE_VARIANT_ADD(protocol, no_sandbox_with_ipv4_udp) {
>>   	/* clang-format on */
>> @@ -359,6 +459,39 @@ FIXTURE_VARIANT_ADD(protocol, tcp_sandbox_with_ipv4_tcp) {
>>   	},
>>   };
>>   
>> +/* clang-format off */
>> +FIXTURE_VARIANT_ADD(protocol, tcp_sandbox_with_ipv4_mptcp) {
>> +	/* clang-format on */
>> +	.sandbox = TCP_SANDBOX,
>> +	.prot = {
>> +		.domain = AF_INET,
>> +		.type = SOCK_STREAM,
>> +		.protocol = IPPROTO_MPTCP,
>> +	},
>> +};
>> +
>> +/* clang-format off */
>> +FIXTURE_VARIANT_ADD(protocol, tcp_sandbox_with_ipv4_sctp) {
>> +	/* clang-format on */
>> +	.sandbox = TCP_SANDBOX,
>> +	.prot = {
>> +		.domain = AF_INET,
>> +		.type = SOCK_STREAM,
>> +		.protocol = IPPROTO_SCTP,
>> +	},
>> +};
>> +
>> +/* clang-format off */
>> +FIXTURE_VARIANT_ADD(protocol, tcp_sandbox_with_ipv4_smc) {
>> +	/* clang-format on */
>> +	.sandbox = TCP_SANDBOX,
>> +	.prot = {
>> +		.domain = AF_INET,
>> +		.type = SOCK_STREAM,
>> +		.protocol = IPPROTO_SMC,
>> +	},
>> +};
>> +
>>   /* clang-format off */
>>   FIXTURE_VARIANT_ADD(protocol, tcp_sandbox_with_ipv6_tcp) {
>>   	/* clang-format on */
>> @@ -369,6 +502,39 @@ FIXTURE_VARIANT_ADD(protocol, tcp_sandbox_with_ipv6_tcp) {
>>   	},
>>   };
>>   
>> +/* clang-format off */
>> +FIXTURE_VARIANT_ADD(protocol, tcp_sandbox_with_ipv6_mptcp) {
>> +	/* clang-format on */
>> +	.sandbox = TCP_SANDBOX,
>> +	.prot = {
>> +		.domain = AF_INET6,
>> +		.type = SOCK_STREAM,
>> +		.protocol = IPPROTO_MPTCP,
>> +	},
>> +};
>> +
>> +/* clang-format off */
>> +FIXTURE_VARIANT_ADD(protocol, tcp_sandbox_with_ipv6_sctp) {
>> +	/* clang-format on */
>> +	.sandbox = TCP_SANDBOX,
>> +	.prot = {
>> +		.domain = AF_INET6,
>> +		.type = SOCK_STREAM,
>> +		.protocol = IPPROTO_SCTP,
>> +	},
>> +};
>> +
>> +/* clang-format off */
>> +FIXTURE_VARIANT_ADD(protocol, tcp_sandbox_with_ipv6_smc) {
>> +	/* clang-format on */
>> +	.sandbox = TCP_SANDBOX,
>> +	.prot = {
>> +		.domain = AF_INET6,
>> +		.type = SOCK_STREAM,
>> +		.protocol = IPPROTO_SMC,
>> +	},
>> +};
>> +
>>   /* clang-format off */
>>   FIXTURE_VARIANT_ADD(protocol, tcp_sandbox_with_ipv4_udp) {
>>   	/* clang-format on */
>> @@ -663,7 +829,7 @@ TEST_F(protocol, bind_unspec)
>>   
>>   	/* Allowed bind on AF_UNSPEC/INADDR_ANY. */
>>   	ret = bind_variant(bind_fd, &self->unspec_any0);
>> -	if (variant->prot.domain == AF_INET) {
>> +	if (variant->prot.domain == AF_INET && !prot_is_sctp(&variant->prot)) {
>>   		EXPECT_EQ(0, ret)
>>   		{
>>   			TH_LOG("Failed to bind to unspec/any socket: %s",
>> @@ -689,7 +855,7 @@ TEST_F(protocol, bind_unspec)
>>   
>>   	/* Denied bind on AF_UNSPEC/INADDR_ANY. */
>>   	ret = bind_variant(bind_fd, &self->unspec_any0);
>> -	if (variant->prot.domain == AF_INET) {
>> +	if (variant->prot.domain == AF_INET && !prot_is_sctp(&variant->prot)) {
> 
> It looks like we need the same exception for the next bind_variant()
> call.

I ran these tests with active selinux (and few other LSMs) (selinux is 
set by default for x86_64) and it seems that this check was passed
correctly due to SCTP errno inconsistency in selinux_socket_bind().

With selinux_socket_bind() disabled, bind_variant() returns -EINVAL as
it should (Cf. sctp_do_bind).

Such inconsistency happens because sksec->sclass security field can be
initialized with SECCLASS_SOCKET (Cf. socket_type_to_security_class)
in SCTP case, and selinux_socket_bind() provides following check:

	/* Note that SCTP services expect -EINVAL, others -EAFNOSUPPORT. */
	if (sksec->sclass == SECCLASS_SCTP_SOCKET)
		return -EINVAL;
	return -EAFNOSUPPORT;

I'll possibly send a fix for this to selinux.

> 
>>   		if (is_restricted(&variant->prot, variant->sandbox)) {
>>   			EXPECT_EQ(-EACCES, ret);
>>   		} else {
>> @@ -727,6 +893,10 @@ TEST_F(protocol, connect_unspec)
>>   	int bind_fd, client_fd, status;
>>   	pid_t child;
>>   
>> +	if (prot_is_smc(&variant->prot))
>> +		SKIP(return, "SMC does not properly handles disconnect "
>> +			     "in the case of fallback to TCP");
>> +
>>   	/* Specific connection tests. */
>>   	bind_fd = socket_variant(&self->srv0);
>>   	ASSERT_LE(0, bind_fd);
>> @@ -769,17 +939,18 @@ TEST_F(protocol, connect_unspec)
>>   
>>   		/* Disconnects already connected socket, or set peer. */
>>   		ret = connect_variant(connect_fd, &self->unspec_any0);
>> -		if (self->srv0.protocol.domain == AF_UNIX &&
>> -		    self->srv0.protocol.type == SOCK_STREAM) {
>> +		if (prot_is_unix_stream(&variant->prot)) {
>>   			EXPECT_EQ(-EINVAL, ret);
>> +		} else if (prot_is_sctp(&variant->prot)) {
>> +			EXPECT_EQ(-EOPNOTSUPP, ret);
>>   		} else {
>>   			EXPECT_EQ(0, ret);
>>   		}
>>   
>>   		/* Tries to reconnect, or set peer. */
>>   		ret = connect_variant(connect_fd, &self->srv0);
>> -		if (self->srv0.protocol.domain == AF_UNIX &&
>> -		    self->srv0.protocol.type == SOCK_STREAM) {
>> +		if (prot_is_unix_stream(&variant->prot) ||
>> +		    prot_is_sctp(&variant->prot)) {
>>   			EXPECT_EQ(-EISCONN, ret);
>>   		} else {
>>   			EXPECT_EQ(0, ret);
>> @@ -796,9 +967,10 @@ TEST_F(protocol, connect_unspec)
>>   		}
>>   
>>   		ret = connect_variant(connect_fd, &self->unspec_any0);
>> -		if (self->srv0.protocol.domain == AF_UNIX &&
>> -		    self->srv0.protocol.type == SOCK_STREAM) {
>> +		if (prot_is_unix_stream(&variant->prot)) {
>>   			EXPECT_EQ(-EINVAL, ret);
>> +		} else if (prot_is_sctp(&variant->prot)) {
>> +			EXPECT_EQ(-EOPNOTSUPP, ret);
>>   		} else {
>>   			/* Always allowed to disconnect. */
>>   			EXPECT_EQ(0, ret);
>> -- 
>> 2.34.1
>>
>>

Re: [RFC PATCH v1 1/2] landlock: Fix non-TCP sockets restriction

[Mikhail Ivanov]

On 10/3/2024 8:45 PM, Mickaël Salaün wrote:
> Please also add Matthieu in Cc for the network patch series.
> 
> On Thu, Oct 03, 2024 at 10:39:31PM +0800, Mikhail Ivanov wrote:
>> Do not check TCP access right if socket protocol is not IPPROTO_TCP.
>> LANDLOCK_ACCESS_NET_BIND_TCP and LANDLOCK_ACCESS_NET_CONNECT_TCP
>> should not restrict bind(2) and connect(2) for non-TCP protocols
>> (SCTP, MPTCP, SMC).
>>
>> Closes: https://github.com/landlock-lsm/linux/issues/40
>> Fixes: fff69fb03dde ("landlock: Support network rules with TCP bind and connect")
>> Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
>> ---
>>   security/landlock/net.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/security/landlock/net.c b/security/landlock/net.c
>> index bc3d943a7118..6f59dd98bb13 100644
>> --- a/security/landlock/net.c
>> +++ b/security/landlock/net.c
>> @@ -68,7 +68,7 @@ static int current_check_access_socket(struct socket *const sock,
>>   		return -EACCES;
>>   
>>   	/* Checks if it's a (potential) TCP socket. */
> 
> We can extend this comment to explain that we don't use sk_is_tcp()
> because we need to handle the AF_UNSPEC case.

Indeed, I'll do this.

> 
>> -	if (sock->type != SOCK_STREAM)
>> +	if (sock->type != SOCK_STREAM || sock->sk->sk_protocol != IPPROTO_TCP)
> 
> I think we should check sock->sk->sk_type instead of sock->type (even if
> it should be the same).  To make it simpler, we should only use sk in
> current_check_access_socket():
> struct sock *sk = sock->sk;

Agreed.

> 
> Could you please also do s/__sk_common\.skc_/sk_/g ?

Ofc

Btw, there is probably incorrect read of skc_family in this function
[1]. I'll add READ_ONCE for sk->sk_family.

[1] https://lore.kernel.org/all/20240202095404.183274-1-edumazet@google.com/

> 
>>   		return 0;
>>   
>>   	/* Checks for minimal header length to safely read sa_family. */
>> -- 
>> 2.34.1
>>
>>

Re: [RFC PATCH v1 1/2] landlock: Fix non-TCP sockets restriction

[Mikhail Ivanov]

On 10/3/2024 8:45 PM, Mickaël Salaün wrote:
> Please also add Matthieu in Cc for the network patch series.

Ok

Re: [RFC PATCH v1 1/2] landlock: Fix non-TCP sockets restriction

[Mickaël Salaün]

On Fri, Oct 04, 2024 at 12:30:02AM +0300, Mikhail Ivanov wrote:
> On 10/3/2024 8:45 PM, Mickaël Salaün wrote:
> > Please also add Matthieu in Cc for the network patch series.
> > 
> > On Thu, Oct 03, 2024 at 10:39:31PM +0800, Mikhail Ivanov wrote:
> > > Do not check TCP access right if socket protocol is not IPPROTO_TCP.
> > > LANDLOCK_ACCESS_NET_BIND_TCP and LANDLOCK_ACCESS_NET_CONNECT_TCP
> > > should not restrict bind(2) and connect(2) for non-TCP protocols
> > > (SCTP, MPTCP, SMC).
> > > 
> > > Closes: https://github.com/landlock-lsm/linux/issues/40
> > > Fixes: fff69fb03dde ("landlock: Support network rules with TCP bind and connect")
> > > Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
> > > ---
> > >   security/landlock/net.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/security/landlock/net.c b/security/landlock/net.c
> > > index bc3d943a7118..6f59dd98bb13 100644
> > > --- a/security/landlock/net.c
> > > +++ b/security/landlock/net.c
> > > @@ -68,7 +68,7 @@ static int current_check_access_socket(struct socket *const sock,
> > >   		return -EACCES;
> > >   	/* Checks if it's a (potential) TCP socket. */
> > 
> > We can extend this comment to explain that we don't use sk_is_tcp()
> > because we need to handle the AF_UNSPEC case.
> 
> Indeed, I'll do this.
> 
> > 
> > > -	if (sock->type != SOCK_STREAM)
> > > +	if (sock->type != SOCK_STREAM || sock->sk->sk_protocol != IPPROTO_TCP)
> > 
> > I think we should check sock->sk->sk_type instead of sock->type (even if
> > it should be the same).  To make it simpler, we should only use sk in
> > current_check_access_socket():
> > struct sock *sk = sock->sk;
> 
> Agreed.
> 
> > 
> > Could you please also do s/__sk_common\.skc_/sk_/g ?
> 
> Ofc
> 
> Btw, there is probably incorrect read of skc_family in this function
> [1]. I'll add READ_ONCE for sk->sk_family.
> 
> [1] https://lore.kernel.org/all/20240202095404.183274-1-edumazet@google.com/

I think it should not be a bug with the current code (IPv6 -> IPV4, and
socket vs. sock) but we should indeed use READ_ONCE() (and add this link
to the commit message).

> 
> > 
> > >   		return 0;
> > >   	/* Checks for minimal header length to safely read sa_family. */
> > > -- 
> > > 2.34.1
> > > 
> > > 
> 

Re: [RFC PATCH v1 2/2] selftests/landlock: Test non-TCP INET connection-based protocols

[Mickaël Salaün]

Eric, Vlad, Neil, and David, there might be a bug in the SCTP
implementation:

Paul, Alexey, there is a bug in the SELinux hooks for SCTP:

On Fri, Oct 04, 2024 at 12:22:42AM +0300, Mikhail Ivanov wrote:
> 
> 
> On 10/3/2024 8:45 PM, Mickaël Salaün wrote:
> > On Thu, Oct 03, 2024 at 10:39:32PM +0800, Mikhail Ivanov wrote:
> > > Extend protocol fixture with test suits for MPTCP, SCTP and SMC protocols.
> > > Add all options required by this protocols in config.
> > 
> > Great coverage!  It's nice to check against SCTP and MPTCP, but as you
> > were wondering, I think you can remove the SMC protocol to simplify
> > tests. MPTCP seems to work similarly as TCP wrt AF_UNSPEC, so it might
> > be worth keeping it, and we might want to control these protocols too
> > one day.
> 
> Thanks! I'll remove SMC then.
> 
> > 
> > > 
> > > Extend protocol_variant structure with protocol field (Cf. socket(2)).
> > > 
> > > Refactor is_restricted() helper and add few helpers to check struct
> > > protocol_variant on specific protocols.
> > 
> > > 
> > > Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
> > > ---
> > >   tools/testing/selftests/landlock/common.h   |   1 +
> > >   tools/testing/selftests/landlock/config     |   5 +
> > >   tools/testing/selftests/landlock/net_test.c | 212 ++++++++++++++++++--
> > >   3 files changed, 198 insertions(+), 20 deletions(-)
> > > 
> > > diff --git a/tools/testing/selftests/landlock/common.h b/tools/testing/selftests/landlock/common.h
> > > index 61056fa074bb..40a2def50b83 100644
> > > --- a/tools/testing/selftests/landlock/common.h
> > > +++ b/tools/testing/selftests/landlock/common.h
> > > @@ -234,6 +234,7 @@ enforce_ruleset(struct __test_metadata *const _metadata, const int ruleset_fd)
> > >   struct protocol_variant {
> > >   	int domain;
> > >   	int type;
> > > +	int protocol;
> > >   };
> > >   struct service_fixture {
> > > diff --git a/tools/testing/selftests/landlock/config b/tools/testing/selftests/landlock/config
> > > index 29af19c4e9f9..73b01d7d0881 100644
> > > --- a/tools/testing/selftests/landlock/config
> > > +++ b/tools/testing/selftests/landlock/config
> > > @@ -1,8 +1,12 @@
> > >   CONFIG_CGROUPS=y
> > >   CONFIG_CGROUP_SCHED=y
> > >   CONFIG_INET=y
> > > +CONFIG_INFINIBAND=y
> > 
> > Without SMC this infiniband should not be required.
> 
> yeap
> 
> > 
> > > +CONFIG_IP_SCTP=y
> > >   CONFIG_IPV6=y
> > >   CONFIG_KEYS=y
> > > +CONFIG_MPTCP=y
> > > +CONFIG_MPTCP_IPV6=y
> > >   CONFIG_NET=y
> > >   CONFIG_NET_NS=y
> > >   CONFIG_OVERLAY_FS=y
> > > @@ -10,6 +14,7 @@ CONFIG_PROC_FS=y
> > >   CONFIG_SECURITY=y
> > >   CONFIG_SECURITY_LANDLOCK=y
> > >   CONFIG_SHMEM=y
> > > +CONFIG_SMC=y
> > >   CONFIG_SYSFS=y
> > >   CONFIG_TMPFS=y
> > >   CONFIG_TMPFS_XATTR=y
> > > diff --git a/tools/testing/selftests/landlock/net_test.c b/tools/testing/selftests/landlock/net_test.c
> > > index 4e0aeb53b225..dbe77d436281 100644
> > > --- a/tools/testing/selftests/landlock/net_test.c
> > > +++ b/tools/testing/selftests/landlock/net_test.c
> > > @@ -36,6 +36,17 @@ enum sandbox_type {
> > >   	TCP_SANDBOX,
> > >   };
> > > +/* Checks if IPPROTO_SMC is present for compatibility reasons. */
> > > +#if !defined(__alpha__) && defined(IPPROTO_SMC)
> > > +#define SMC_SUPPORTED 1
> > > +#else
> > > +#define SMC_SUPPORTED 0
> > > +#endif
> > > +
> > > +#ifndef IPPROTO_SMC
> > > +#define IPPROTO_SMC 256
> > > +#endif
> > > +
> > >   static int set_service(struct service_fixture *const srv,
> > >   		       const struct protocol_variant prot,
> > >   		       const unsigned short index)
> > > @@ -85,19 +96,37 @@ static void setup_loopback(struct __test_metadata *const _metadata)
> > >   	clear_ambient_cap(_metadata, CAP_NET_ADMIN);
> > >   }
> > > +static bool prot_is_inet_stream(const struct protocol_variant *const prot)
> > > +{
> > > +	return (prot->domain == AF_INET || prot->domain == AF_INET6) &&
> > > +	       prot->type == SOCK_STREAM;
> > > +}
> > > +
> > > +static bool prot_is_tcp(const struct protocol_variant *const prot)
> > > +{
> > > +	return prot_is_inet_stream(prot) &&
> > > +	       (prot->protocol == IPPROTO_TCP || prot->protocol == IPPROTO_IP);
> > 
> > Why do we need to check against IPPROTO_IP?
> 
> IPPROTO_IP = 0 and can be used as an alias for IPPROTO_TCP (=6) in
> socket(2) (also for IPPROTO_UDP(=17), Cf. inet_create).
> 
> Since we create TCP sockets in a common way here (with protocol = 0),
> checking against IPPROTO_TCP is not necessary, but I decided to leave it
> for completeness.

Sound good, but we should then also add variants with IPPROTO_TCP for
sandboxed and not-sandboxed tests:

/* clang-format off */
FIXTURE_VARIANT_ADD(protocol, no_sandbox_with_ipv4_tcp1) {
	/* clang-format on */
	.sandbox = NO_SANDBOX,
	.prot = {
		.domain = AF_INET,
		.type = SOCK_STREAM,
		/* IPPROTO_IP == 0 */
		.protocol = IPPROTO_IP,
	},
};


/* clang-format off */
FIXTURE_VARIANT_ADD(protocol, no_sandbox_with_ipv4_tcp2) {
	/* clang-format on */
	.sandbox = NO_SANDBOX,
	.prot = {
		.domain = AF_INET,
		.type = SOCK_STREAM,
		.protocol = IPPROTO_TCP,
	},
};

> 
> > 
> > > +}
> > > +
> > > +static bool prot_is_sctp(const struct protocol_variant *const prot)
> > > +{
> > > +	return prot_is_inet_stream(prot) && prot->protocol == IPPROTO_SCTP;
> > > +}
> > > +
> > > +static bool prot_is_smc(const struct protocol_variant *const prot)
> > > +{
> > > +	return prot_is_inet_stream(prot) && prot->protocol == IPPROTO_SMC;
> > > +}
> > > +
> > > +static bool prot_is_unix_stream(const struct protocol_variant *const prot)
> > > +{
> > > +	return prot->domain == AF_UNIX && prot->type == SOCK_STREAM;
> > > +}
> > > +
> > >   static bool is_restricted(const struct protocol_variant *const prot,
> > >   			  const enum sandbox_type sandbox)
> > >   {
> > > -	switch (prot->domain) {
> > > -	case AF_INET:
> > > -	case AF_INET6:
> > > -		switch (prot->type) {
> > > -		case SOCK_STREAM:
> > > -			return sandbox == TCP_SANDBOX;
> > > -		}
> > > -		break;
> > > -	}
> > > -	return false;
> > > +	return prot_is_tcp(prot) && sandbox == TCP_SANDBOX;
> > >   }
> > >   static int socket_variant(const struct service_fixture *const srv)
> > > @@ -105,7 +134,7 @@ static int socket_variant(const struct service_fixture *const srv)
> > >   	int ret;
> > >   	ret = socket(srv->protocol.domain, srv->protocol.type | SOCK_CLOEXEC,
> > > -		     0);
> > > +		     srv->protocol.protocol);
> > >   	if (ret < 0)
> > >   		return -errno;
> > >   	return ret;
> > > @@ -124,7 +153,7 @@ static socklen_t get_addrlen(const struct service_fixture *const srv,
> > >   		return sizeof(srv->ipv4_addr);
> > >   	case AF_INET6:
> > > -		if (minimal)
> > > +		if (minimal && !prot_is_sctp(&srv->protocol))
> > 
> > Why SCTP requires this exception?
> 
> SCTP implementation (possibly incorrectly) checks that address length is
> at least sizeof(struct sockaddr_in6) (Cf. sctp_sockaddr_af() for bind(2)
> and in sctp_connect() for connect(2)).

sctp_sockaddr_af() checks for len < SIN6_LEN_RFC2133, but also for
len < af->sockaddr_len, which refers to sctp_af_inet6.sockaddr_len =
sizeof(struct sockaddr_in6).

I think this is a bug in the SCTP implementation and it would be a fix
of 81e98370293a ("sctp: sctp_sockaddr_af must check minimal addr length
for AF_INET6"), which fixes all versions of Linux.

> 
> > 
> > >   			return SIN6_LEN_RFC2133;
> > >   		return sizeof(srv->ipv6_addr);
> > > @@ -271,6 +300,11 @@ FIXTURE_SETUP(protocol)
> > >   		.type = SOCK_STREAM,
> > >   	};
> > > +#if !SMC_SUPPORTED
> > > +	if (prot_is_smc(&variant->prot))
> > > +		SKIP(return, "SMC protocol is not supported.");
> > > +#endif
> > > +
> > >   	disable_caps(_metadata);
> > >   	ASSERT_EQ(0, set_service(&self->srv0, variant->prot, 0));
> > > @@ -299,6 +333,39 @@ FIXTURE_VARIANT_ADD(protocol, no_sandbox_with_ipv4_tcp) {
> > >   	},
> > >   };
> > > +/* clang-format off */
> > > +FIXTURE_VARIANT_ADD(protocol, no_sandbox_with_ipv4_mptcp) {
> > > +	/* clang-format on */
> > > +	.sandbox = NO_SANDBOX,
> > > +	.prot = {
> > > +		.domain = AF_INET,
> > > +		.type = SOCK_STREAM,
> > > +		.protocol = IPPROTO_MPTCP,
> > > +	},
> > > +};
> > > +
> > > +/* clang-format off */
> > > +FIXTURE_VARIANT_ADD(protocol, no_sandbox_with_ipv4_sctp) {
> > > +	/* clang-format on */
> > > +	.sandbox = NO_SANDBOX,
> > > +	.prot = {
> > > +		.domain = AF_INET,
> > > +		.type = SOCK_STREAM,
> > > +		.protocol = IPPROTO_SCTP,
> > > +	},
> > > +};
> > > +
> > > +/* clang-format off */
> > > +FIXTURE_VARIANT_ADD(protocol, no_sandbox_with_ipv4_smc) {
> > > +	/* clang-format on */
> > > +	.sandbox = NO_SANDBOX,
> > > +	.prot = {
> > > +		.domain = AF_INET,
> > > +		.type = SOCK_STREAM,
> > > +		.protocol = IPPROTO_SMC,
> > > +	},
> > > +};
> > > +
> > >   /* clang-format off */
> > >   FIXTURE_VARIANT_ADD(protocol, no_sandbox_with_ipv6_tcp) {
> > >   	/* clang-format on */
> > > @@ -309,6 +376,39 @@ FIXTURE_VARIANT_ADD(protocol, no_sandbox_with_ipv6_tcp) {
> > >   	},
> > >   };
> > > +/* clang-format off */
> > > +FIXTURE_VARIANT_ADD(protocol, no_sandbox_with_ipv6_mptcp) {
> > > +	/* clang-format on */
> > > +	.sandbox = NO_SANDBOX,
> > > +	.prot = {
> > > +		.domain = AF_INET6,
> > > +		.type = SOCK_STREAM,
> > > +		.protocol = IPPROTO_MPTCP,
> > > +	},
> > > +};
> > > +
> > > +/* clang-format off */
> > > +FIXTURE_VARIANT_ADD(protocol, no_sandbox_with_ipv6_sctp) {
> > > +	/* clang-format on */
> > > +	.sandbox = NO_SANDBOX,
> > > +	.prot = {
> > > +		.domain = AF_INET6,
> > > +		.type = SOCK_STREAM,
> > > +		.protocol = IPPROTO_SCTP,
> > > +	},
> > > +};
> > > +
> > > +/* clang-format off */
> > > +FIXTURE_VARIANT_ADD(protocol, no_sandbox_with_ipv6_smc) {
> > > +	/* clang-format on */
> > > +	.sandbox = NO_SANDBOX,
> > > +	.prot = {
> > > +		.domain = AF_INET6,
> > > +		.type = SOCK_STREAM,
> > > +		.protocol = IPPROTO_SMC,
> > > +	},
> > > +};
> > > +
> > >   /* clang-format off */
> > >   FIXTURE_VARIANT_ADD(protocol, no_sandbox_with_ipv4_udp) {
> > >   	/* clang-format on */
> > > @@ -359,6 +459,39 @@ FIXTURE_VARIANT_ADD(protocol, tcp_sandbox_with_ipv4_tcp) {
> > >   	},
> > >   };
> > > +/* clang-format off */
> > > +FIXTURE_VARIANT_ADD(protocol, tcp_sandbox_with_ipv4_mptcp) {
> > > +	/* clang-format on */
> > > +	.sandbox = TCP_SANDBOX,
> > > +	.prot = {
> > > +		.domain = AF_INET,
> > > +		.type = SOCK_STREAM,
> > > +		.protocol = IPPROTO_MPTCP,
> > > +	},
> > > +};
> > > +
> > > +/* clang-format off */
> > > +FIXTURE_VARIANT_ADD(protocol, tcp_sandbox_with_ipv4_sctp) {
> > > +	/* clang-format on */
> > > +	.sandbox = TCP_SANDBOX,
> > > +	.prot = {
> > > +		.domain = AF_INET,
> > > +		.type = SOCK_STREAM,
> > > +		.protocol = IPPROTO_SCTP,
> > > +	},
> > > +};
> > > +
> > > +/* clang-format off */
> > > +FIXTURE_VARIANT_ADD(protocol, tcp_sandbox_with_ipv4_smc) {
> > > +	/* clang-format on */
> > > +	.sandbox = TCP_SANDBOX,
> > > +	.prot = {
> > > +		.domain = AF_INET,
> > > +		.type = SOCK_STREAM,
> > > +		.protocol = IPPROTO_SMC,
> > > +	},
> > > +};
> > > +
> > >   /* clang-format off */
> > >   FIXTURE_VARIANT_ADD(protocol, tcp_sandbox_with_ipv6_tcp) {
> > >   	/* clang-format on */
> > > @@ -369,6 +502,39 @@ FIXTURE_VARIANT_ADD(protocol, tcp_sandbox_with_ipv6_tcp) {
> > >   	},
> > >   };
> > > +/* clang-format off */
> > > +FIXTURE_VARIANT_ADD(protocol, tcp_sandbox_with_ipv6_mptcp) {
> > > +	/* clang-format on */
> > > +	.sandbox = TCP_SANDBOX,
> > > +	.prot = {
> > > +		.domain = AF_INET6,
> > > +		.type = SOCK_STREAM,
> > > +		.protocol = IPPROTO_MPTCP,
> > > +	},
> > > +};
> > > +
> > > +/* clang-format off */
> > > +FIXTURE_VARIANT_ADD(protocol, tcp_sandbox_with_ipv6_sctp) {
> > > +	/* clang-format on */
> > > +	.sandbox = TCP_SANDBOX,
> > > +	.prot = {
> > > +		.domain = AF_INET6,
> > > +		.type = SOCK_STREAM,
> > > +		.protocol = IPPROTO_SCTP,
> > > +	},
> > > +};
> > > +
> > > +/* clang-format off */
> > > +FIXTURE_VARIANT_ADD(protocol, tcp_sandbox_with_ipv6_smc) {
> > > +	/* clang-format on */
> > > +	.sandbox = TCP_SANDBOX,
> > > +	.prot = {
> > > +		.domain = AF_INET6,
> > > +		.type = SOCK_STREAM,
> > > +		.protocol = IPPROTO_SMC,
> > > +	},
> > > +};
> > > +
> > >   /* clang-format off */
> > >   FIXTURE_VARIANT_ADD(protocol, tcp_sandbox_with_ipv4_udp) {
> > >   	/* clang-format on */
> > > @@ -663,7 +829,7 @@ TEST_F(protocol, bind_unspec)
> > >   	/* Allowed bind on AF_UNSPEC/INADDR_ANY. */
> > >   	ret = bind_variant(bind_fd, &self->unspec_any0);
> > > -	if (variant->prot.domain == AF_INET) {
> > > +	if (variant->prot.domain == AF_INET && !prot_is_sctp(&variant->prot)) {
> > >   		EXPECT_EQ(0, ret)
> > >   		{
> > >   			TH_LOG("Failed to bind to unspec/any socket: %s",
> > > @@ -689,7 +855,7 @@ TEST_F(protocol, bind_unspec)
> > >   	/* Denied bind on AF_UNSPEC/INADDR_ANY. */
> > >   	ret = bind_variant(bind_fd, &self->unspec_any0);
> > > -	if (variant->prot.domain == AF_INET) {
> > > +	if (variant->prot.domain == AF_INET && !prot_is_sctp(&variant->prot)) {
> > 
> > It looks like we need the same exception for the next bind_variant()
> > call.
> 
> I ran these tests with active selinux (and few other LSMs) (selinux is set
> by default for x86_64) and it seems that this check was passed
> correctly due to SCTP errno inconsistency in selinux_socket_bind().
> 
> With selinux_socket_bind() disabled, bind_variant() returns -EINVAL as
> it should (Cf. sctp_do_bind).
> 
> Such inconsistency happens because sksec->sclass security field can be
> initialized with SECCLASS_SOCKET (Cf. socket_type_to_security_class)
> in SCTP case, and selinux_socket_bind() provides following check:
> 
> 	/* Note that SCTP services expect -EINVAL, others -EAFNOSUPPORT. */
> 	if (sksec->sclass == SECCLASS_SCTP_SOCKET)
> 		return -EINVAL;
> 	return -EAFNOSUPPORT;
> 
> I'll possibly send a fix for this to selinux.

Yes please, and it would be handy to split this patch with the first
providing MPTCP coverage and the second SCTP coverage.  This way I'll
quickly merge the MPTCP tests and wait for the SCTP fixes.

The SELinux issue might have been introduced with commit 0f8db8cc73df
("selinux: add AF_UNSPEC and INADDR_ANY checks to
selinux_socket_bind()").

> 
> > 
> > >   		if (is_restricted(&variant->prot, variant->sandbox)) {
> > >   			EXPECT_EQ(-EACCES, ret);
> > >   		} else {
> > > @@ -727,6 +893,10 @@ TEST_F(protocol, connect_unspec)
> > >   	int bind_fd, client_fd, status;
> > >   	pid_t child;
> > > +	if (prot_is_smc(&variant->prot))
> > > +		SKIP(return, "SMC does not properly handles disconnect "
> > > +			     "in the case of fallback to TCP");
> > > +
> > >   	/* Specific connection tests. */
> > >   	bind_fd = socket_variant(&self->srv0);
> > >   	ASSERT_LE(0, bind_fd);
> > > @@ -769,17 +939,18 @@ TEST_F(protocol, connect_unspec)
> > >   		/* Disconnects already connected socket, or set peer. */
> > >   		ret = connect_variant(connect_fd, &self->unspec_any0);
> > > -		if (self->srv0.protocol.domain == AF_UNIX &&
> > > -		    self->srv0.protocol.type == SOCK_STREAM) {
> > > +		if (prot_is_unix_stream(&variant->prot)) {
> > >   			EXPECT_EQ(-EINVAL, ret);
> > > +		} else if (prot_is_sctp(&variant->prot)) {
> > > +			EXPECT_EQ(-EOPNOTSUPP, ret);
> > >   		} else {
> > >   			EXPECT_EQ(0, ret);
> > >   		}
> > >   		/* Tries to reconnect, or set peer. */
> > >   		ret = connect_variant(connect_fd, &self->srv0);
> > > -		if (self->srv0.protocol.domain == AF_UNIX &&
> > > -		    self->srv0.protocol.type == SOCK_STREAM) {
> > > +		if (prot_is_unix_stream(&variant->prot) ||
> > > +		    prot_is_sctp(&variant->prot)) {
> > >   			EXPECT_EQ(-EISCONN, ret);
> > >   		} else {
> > >   			EXPECT_EQ(0, ret);
> > > @@ -796,9 +967,10 @@ TEST_F(protocol, connect_unspec)
> > >   		}
> > >   		ret = connect_variant(connect_fd, &self->unspec_any0);
> > > -		if (self->srv0.protocol.domain == AF_UNIX &&
> > > -		    self->srv0.protocol.type == SOCK_STREAM) {
> > > +		if (prot_is_unix_stream(&variant->prot)) {
> > >   			EXPECT_EQ(-EINVAL, ret);
> > > +		} else if (prot_is_sctp(&variant->prot)) {
> > > +			EXPECT_EQ(-EOPNOTSUPP, ret);
> > >   		} else {
> > >   			/* Always allowed to disconnect. */
> > >   			EXPECT_EQ(0, ret);
> > > -- 
> > > 2.34.1
> > > 
> > > 
> 

Re: [RFC PATCH v1 1/2] landlock: Fix non-TCP sockets restriction

[Mikhail Ivanov]

On 10/4/2024 1:13 PM, Mickaël Salaün wrote:
> On Fri, Oct 04, 2024 at 12:30:02AM +0300, Mikhail Ivanov wrote:
>> On 10/3/2024 8:45 PM, Mickaël Salaün wrote:
>>> Please also add Matthieu in Cc for the network patch series.
>>>
>>> On Thu, Oct 03, 2024 at 10:39:31PM +0800, Mikhail Ivanov wrote:
>>>> Do not check TCP access right if socket protocol is not IPPROTO_TCP.
>>>> LANDLOCK_ACCESS_NET_BIND_TCP and LANDLOCK_ACCESS_NET_CONNECT_TCP
>>>> should not restrict bind(2) and connect(2) for non-TCP protocols
>>>> (SCTP, MPTCP, SMC).
>>>>
>>>> Closes: https://github.com/landlock-lsm/linux/issues/40
>>>> Fixes: fff69fb03dde ("landlock: Support network rules with TCP bind and connect")
>>>> Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
>>>> ---
>>>>    security/landlock/net.c | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/security/landlock/net.c b/security/landlock/net.c
>>>> index bc3d943a7118..6f59dd98bb13 100644
>>>> --- a/security/landlock/net.c
>>>> +++ b/security/landlock/net.c
>>>> @@ -68,7 +68,7 @@ static int current_check_access_socket(struct socket *const sock,
>>>>    		return -EACCES;
>>>>    	/* Checks if it's a (potential) TCP socket. */
>>>
>>> We can extend this comment to explain that we don't use sk_is_tcp()
>>> because we need to handle the AF_UNSPEC case.
>>
>> Indeed, I'll do this.

I've noticed that we still should check sk->sk_family = AF_INET{,6}
here (so sk_is_tcp() is suitable). AF_UNSPEC can be only related to
addresses and we should not provide any checks (for address) if socket
is unrestrictable (i.e. it's not TCP). It's not useful and might lead to
error incosistency for non-TCP sockets.

Btw, I suppose we can improve error consistency by bringing more checks
from INET/TCP stack. For example it may be useful to return EISCONN
instead of EACCES while connect(2) is called on a connected socket.

This should be done really carefully and only for some useful cases.
Anyway it's not related to the current patch (since it's not a bug).

>>
>>>
>>>> -	if (sock->type != SOCK_STREAM)
>>>> +	if (sock->type != SOCK_STREAM || sock->sk->sk_protocol != IPPROTO_TCP)
>>>
>>> I think we should check sock->sk->sk_type instead of sock->type (even if
>>> it should be the same).  To make it simpler, we should only use sk in
>>> current_check_access_socket():
>>> struct sock *sk = sock->sk;
>>
>> Agreed.
>>
>>>
>>> Could you please also do s/__sk_common\.skc_/sk_/g ?
>>
>> Ofc
>>
>> Btw, there is probably incorrect read of skc_family in this function
>> [1]. I'll add READ_ONCE for sk->sk_family.
>>
>> [1] https://lore.kernel.org/all/20240202095404.183274-1-edumazet@google.com/
> 
> I think it should not be a bug with the current code (IPv6 -> IPV4, and
> socket vs. sock) but we should indeed use READ_ONCE() (and add this link
> to the commit message).

ok

> 
>>
>>>
>>>>    		return 0;
>>>>    	/* Checks for minimal header length to safely read sa_family. */
>>>> -- 
>>>> 2.34.1
>>>>
>>>>
>>

Re: [RFC PATCH v1 1/2] landlock: Fix non-TCP sockets restriction

[Mickaël Salaün]

On Fri, Oct 04, 2024 at 09:16:56PM +0300, Mikhail Ivanov wrote:
> On 10/4/2024 1:13 PM, Mickaël Salaün wrote:
> > On Fri, Oct 04, 2024 at 12:30:02AM +0300, Mikhail Ivanov wrote:
> > > On 10/3/2024 8:45 PM, Mickaël Salaün wrote:
> > > > Please also add Matthieu in Cc for the network patch series.
> > > > 
> > > > On Thu, Oct 03, 2024 at 10:39:31PM +0800, Mikhail Ivanov wrote:
> > > > > Do not check TCP access right if socket protocol is not IPPROTO_TCP.
> > > > > LANDLOCK_ACCESS_NET_BIND_TCP and LANDLOCK_ACCESS_NET_CONNECT_TCP
> > > > > should not restrict bind(2) and connect(2) for non-TCP protocols
> > > > > (SCTP, MPTCP, SMC).
> > > > > 
> > > > > Closes: https://github.com/landlock-lsm/linux/issues/40
> > > > > Fixes: fff69fb03dde ("landlock: Support network rules with TCP bind and connect")
> > > > > Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
> > > > > ---
> > > > >    security/landlock/net.c | 2 +-
> > > > >    1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/security/landlock/net.c b/security/landlock/net.c
> > > > > index bc3d943a7118..6f59dd98bb13 100644
> > > > > --- a/security/landlock/net.c
> > > > > +++ b/security/landlock/net.c
> > > > > @@ -68,7 +68,7 @@ static int current_check_access_socket(struct socket *const sock,
> > > > >    		return -EACCES;
> > > > >    	/* Checks if it's a (potential) TCP socket. */
> > > > 
> > > > We can extend this comment to explain that we don't use sk_is_tcp()
> > > > because we need to handle the AF_UNSPEC case.
> > > 
> > > Indeed, I'll do this.
> 
> I've noticed that we still should check sk->sk_family = AF_INET{,6}
> here (so sk_is_tcp() is suitable). AF_UNSPEC can be only related to
> addresses and we should not provide any checks (for address) if socket
> is unrestrictable (i.e. it's not TCP). It's not useful and might lead to
> error incosistency for non-TCP sockets.

Good catch, let's use sk_is_tcp().

> 
> Btw, I suppose we can improve error consistency by bringing more checks
> from INET/TCP stack. For example it may be useful to return EISCONN
> instead of EACCES while connect(2) is called on a connected socket.

Yes, that would be nice (with the related tests).

> 
> This should be done really carefully and only for some useful cases.
> Anyway it's not related to the current patch (since it's not a bug).

Sure.

The following patch series could probably be extended for all LSM to
benefit from these fixes:
https://lore.kernel.org/all/20240327120036.233641-1-mic@digikod.net/

Mikhail, according to your SCTP tests with SELinux, it looks like this
patch series should be updated, but that should be simple.

Paul, what is the status of this LSM patch series?  Could Mikhail
integrate this LSM patch (with the SCTP fix) as part of the current
Landlock patch series?  This would help fixing the Landlock tests (which
check SCTP error consistency) when run with SELinux.

> 
> > > 
> > > > 
> > > > > -	if (sock->type != SOCK_STREAM)
> > > > > +	if (sock->type != SOCK_STREAM || sock->sk->sk_protocol != IPPROTO_TCP)
> > > > 
> > > > I think we should check sock->sk->sk_type instead of sock->type (even if
> > > > it should be the same).  To make it simpler, we should only use sk in
> > > > current_check_access_socket():
> > > > struct sock *sk = sock->sk;
> > > 
> > > Agreed.
> > > 
> > > > 
> > > > Could you please also do s/__sk_common\.skc_/sk_/g ?
> > > 
> > > Ofc
> > > 
> > > Btw, there is probably incorrect read of skc_family in this function
> > > [1]. I'll add READ_ONCE for sk->sk_family.
> > > 
> > > [1] https://lore.kernel.org/all/20240202095404.183274-1-edumazet@google.com/
> > 
> > I think it should not be a bug with the current code (IPv6 -> IPV4, and
> > socket vs. sock) but we should indeed use READ_ONCE() (and add this link
> > to the commit message).
> 
> ok
> 
> > 
> > > 
> > > > 
> > > > >    		return 0;
> > > > >    	/* Checks for minimal header length to safely read sa_family. */
> > > > > -- 
> > > > > 2.34.1
> > > > > 
> > > > > 
> > > 
> 

Re: [RFC PATCH v1 1/2] landlock: Fix non-TCP sockets restriction

[Mickaël Salaün]

On Sat, Oct 05, 2024 at 05:49:59PM +0200, Mickaël Salaün wrote:
> On Fri, Oct 04, 2024 at 09:16:56PM +0300, Mikhail Ivanov wrote:
> > On 10/4/2024 1:13 PM, Mickaël Salaün wrote:
> > > On Fri, Oct 04, 2024 at 12:30:02AM +0300, Mikhail Ivanov wrote:
> > > > On 10/3/2024 8:45 PM, Mickaël Salaün wrote:
> > > > > Please also add Matthieu in Cc for the network patch series.
> > > > > 
> > > > > On Thu, Oct 03, 2024 at 10:39:31PM +0800, Mikhail Ivanov wrote:
> > > > > > Do not check TCP access right if socket protocol is not IPPROTO_TCP.
> > > > > > LANDLOCK_ACCESS_NET_BIND_TCP and LANDLOCK_ACCESS_NET_CONNECT_TCP
> > > > > > should not restrict bind(2) and connect(2) for non-TCP protocols
> > > > > > (SCTP, MPTCP, SMC).
> > > > > > 
> > > > > > Closes: https://github.com/landlock-lsm/linux/issues/40
> > > > > > Fixes: fff69fb03dde ("landlock: Support network rules with TCP bind and connect")
> > > > > > Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
> > > > > > ---
> > > > > >    security/landlock/net.c | 2 +-
> > > > > >    1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > 
> > > > > > diff --git a/security/landlock/net.c b/security/landlock/net.c
> > > > > > index bc3d943a7118..6f59dd98bb13 100644
> > > > > > --- a/security/landlock/net.c
> > > > > > +++ b/security/landlock/net.c
> > > > > > @@ -68,7 +68,7 @@ static int current_check_access_socket(struct socket *const sock,
> > > > > >    		return -EACCES;
> > > > > >    	/* Checks if it's a (potential) TCP socket. */
> > > > > 
> > > > > We can extend this comment to explain that we don't use sk_is_tcp()
> > > > > because we need to handle the AF_UNSPEC case.
> > > > 
> > > > Indeed, I'll do this.
> > 
> > I've noticed that we still should check sk->sk_family = AF_INET{,6}
> > here (so sk_is_tcp() is suitable). AF_UNSPEC can be only related to
> > addresses and we should not provide any checks (for address) if socket
> > is unrestrictable (i.e. it's not TCP). It's not useful and might lead to
> > error incosistency for non-TCP sockets.
> 
> Good catch, let's use sk_is_tcp().
> 
> > 
> > Btw, I suppose we can improve error consistency by bringing more checks
> > from INET/TCP stack. For example it may be useful to return EISCONN
> > instead of EACCES while connect(2) is called on a connected socket.
> 
> Yes, that would be nice (with the related tests).
> 
> > 
> > This should be done really carefully and only for some useful cases.
> > Anyway it's not related to the current patch (since it's not a bug).
> 
> Sure.
> 
> The following patch series could probably be extended for all LSM to
> benefit from these fixes:
> https://lore.kernel.org/all/20240327120036.233641-1-mic@digikod.net/
> 
> Mikhail, according to your SCTP tests with SELinux, it looks like this
> patch series should be updated, but that should be simple.
> 
> Paul, what is the status of this LSM patch series?  Could Mikhail
> integrate this LSM patch (with the SCTP fix) as part of the current
> Landlock patch series?  This would help fixing the Landlock tests (which
> check SCTP error consistency) when run with SELinux.

Well, this whole LSM network check should probably be part of another
patch series.  For now, let's just fix SELinux (and check that other
LSMs don't return wrong error codes).

Re: [RFC PATCH v1 1/2] landlock: Fix non-TCP sockets restriction

[Mikhail Ivanov]

On 10/5/2024 6:49 PM, Mickaël Salaün wrote:
> On Fri, Oct 04, 2024 at 09:16:56PM +0300, Mikhail Ivanov wrote:
>> On 10/4/2024 1:13 PM, Mickaël Salaün wrote:
>>> On Fri, Oct 04, 2024 at 12:30:02AM +0300, Mikhail Ivanov wrote:
>>>> On 10/3/2024 8:45 PM, Mickaël Salaün wrote:
>>>>> Please also add Matthieu in Cc for the network patch series.
>>>>>
>>>>> On Thu, Oct 03, 2024 at 10:39:31PM +0800, Mikhail Ivanov wrote:
>>>>>> Do not check TCP access right if socket protocol is not IPPROTO_TCP.
>>>>>> LANDLOCK_ACCESS_NET_BIND_TCP and LANDLOCK_ACCESS_NET_CONNECT_TCP
>>>>>> should not restrict bind(2) and connect(2) for non-TCP protocols
>>>>>> (SCTP, MPTCP, SMC).
>>>>>>
>>>>>> Closes: https://github.com/landlock-lsm/linux/issues/40
>>>>>> Fixes: fff69fb03dde ("landlock: Support network rules with TCP bind and connect")
>>>>>> Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
>>>>>> ---
>>>>>>     security/landlock/net.c | 2 +-
>>>>>>     1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/security/landlock/net.c b/security/landlock/net.c
>>>>>> index bc3d943a7118..6f59dd98bb13 100644
>>>>>> --- a/security/landlock/net.c
>>>>>> +++ b/security/landlock/net.c
>>>>>> @@ -68,7 +68,7 @@ static int current_check_access_socket(struct socket *const sock,
>>>>>>     		return -EACCES;
>>>>>>     	/* Checks if it's a (potential) TCP socket. */
>>>>>
>>>>> We can extend this comment to explain that we don't use sk_is_tcp()
>>>>> because we need to handle the AF_UNSPEC case.
>>>>
>>>> Indeed, I'll do this.
>>
>> I've noticed that we still should check sk->sk_family = AF_INET{,6}
>> here (so sk_is_tcp() is suitable). AF_UNSPEC can be only related to
>> addresses and we should not provide any checks (for address) if socket
>> is unrestrictable (i.e. it's not TCP). It's not useful and might lead to
>> error incosistency for non-TCP sockets.
> 
> Good catch, let's use sk_is_tcp().
> 
>>
>> Btw, I suppose we can improve error consistency by bringing more checks
>> from INET/TCP stack. For example it may be useful to return EISCONN
>> instead of EACCES while connect(2) is called on a connected socket.
> 
> Yes, that would be nice (with the related tests).
> 
>>
>> This should be done really carefully and only for some useful cases.
>> Anyway it's not related to the current patch (since it's not a bug).
> 
> Sure.

I have a little question to clarify before sending a next version. Are
we condisering order of network checks for error consistency?

For example, in the current_check_access_socket() we have following
order of checks for ipv4 connect(2) action:
(1) addrlen < sizeof(struct sockaddr_in) -> return -EINVAL
(2) sa_family != sk_family -> return -EINVAL

The ipv4 stack has a check for sock->state before (1) and (2), which can
return -EISCONN if the socket is already connected.

This results in the possiblity of two following scenarios:

Landlock enabled:
1. socket(ipv4) -> OK
2. connect(ipv4 address) -> OK
3. connect(ipv6 address) -> -EINVAL (sa_family != sk_family)

Landlock disabled:
1. socket(ipv4) -> OK
2. connect(ipv4 address) -> OK
3. connect(ipv6 address) -> -EISCONN (socket is already connected)

I have always considered the order of network checks as part of error
consistency, and I'd like to make sure that we're on the same page
before extending current patch with error inconsistency fixes.

> 
> The following patch series could probably be extended for all LSM to
> benefit from these fixes:
> https://lore.kernel.org/all/20240327120036.233641-1-mic@digikod.net/
> 
> Mikhail, according to your SCTP tests with SELinux, it looks like this
> patch series should be updated, but that should be simple.
> 
> Paul, what is the status of this LSM patch series?  Could Mikhail
> integrate this LSM patch (with the SCTP fix) as part of the current
> Landlock patch series?  This would help fixing the Landlock tests (which
> check SCTP error consistency) when run with SELinux.
> 
>>
>>>>
>>>>>
>>>>>> -	if (sock->type != SOCK_STREAM)
>>>>>> +	if (sock->type != SOCK_STREAM || sock->sk->sk_protocol != IPPROTO_TCP)
>>>>>
>>>>> I think we should check sock->sk->sk_type instead of sock->type (even if
>>>>> it should be the same).  To make it simpler, we should only use sk in
>>>>> current_check_access_socket():
>>>>> struct sock *sk = sock->sk;
>>>>
>>>> Agreed.
>>>>
>>>>>
>>>>> Could you please also do s/__sk_common\.skc_/sk_/g ?
>>>>
>>>> Ofc
>>>>
>>>> Btw, there is probably incorrect read of skc_family in this function
>>>> [1]. I'll add READ_ONCE for sk->sk_family.
>>>>
>>>> [1] https://lore.kernel.org/all/20240202095404.183274-1-edumazet@google.com/
>>>
>>> I think it should not be a bug with the current code (IPv6 -> IPV4, and
>>> socket vs. sock) but we should indeed use READ_ONCE() (and add this link
>>> to the commit message).
>>
>> ok
>>
>>>
>>>>
>>>>>
>>>>>>     		return 0;
>>>>>>     	/* Checks for minimal header length to safely read sa_family. */
>>>>>> -- 
>>>>>> 2.34.1
>>>>>>
>>>>>>
>>>>
>>

Re: [RFC PATCH v1 1/2] landlock: Fix non-TCP sockets restriction

[Mikhail Ivanov]

On 10/7/2024 2:06 PM, Mikhail Ivanov wrote:
> On 10/5/2024 6:49 PM, Mickaël Salaün wrote:
>> On Fri, Oct 04, 2024 at 09:16:56PM +0300, Mikhail Ivanov wrote:
>>> On 10/4/2024 1:13 PM, Mickaël Salaün wrote:
>>>> On Fri, Oct 04, 2024 at 12:30:02AM +0300, Mikhail Ivanov wrote:
>>>>> On 10/3/2024 8:45 PM, Mickaël Salaün wrote:
>>>>>> Please also add Matthieu in Cc for the network patch series.
>>>>>>
>>>>>> On Thu, Oct 03, 2024 at 10:39:31PM +0800, Mikhail Ivanov wrote:
>>>>>>> Do not check TCP access right if socket protocol is not IPPROTO_TCP.
>>>>>>> LANDLOCK_ACCESS_NET_BIND_TCP and LANDLOCK_ACCESS_NET_CONNECT_TCP
>>>>>>> should not restrict bind(2) and connect(2) for non-TCP protocols
>>>>>>> (SCTP, MPTCP, SMC).
>>>>>>>
>>>>>>> Closes: https://github.com/landlock-lsm/linux/issues/40
>>>>>>> Fixes: fff69fb03dde ("landlock: Support network rules with TCP 
>>>>>>> bind and connect")
>>>>>>> Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
>>>>>>> ---
>>>>>>>     security/landlock/net.c | 2 +-
>>>>>>>     1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/security/landlock/net.c b/security/landlock/net.c
>>>>>>> index bc3d943a7118..6f59dd98bb13 100644
>>>>>>> --- a/security/landlock/net.c
>>>>>>> +++ b/security/landlock/net.c
>>>>>>> @@ -68,7 +68,7 @@ static int current_check_access_socket(struct 
>>>>>>> socket *const sock,
>>>>>>>             return -EACCES;
>>>>>>>         /* Checks if it's a (potential) TCP socket. */
>>>>>>
>>>>>> We can extend this comment to explain that we don't use sk_is_tcp()
>>>>>> because we need to handle the AF_UNSPEC case.
>>>>>
>>>>> Indeed, I'll do this.
>>>
>>> I've noticed that we still should check sk->sk_family = AF_INET{,6}
>>> here (so sk_is_tcp() is suitable). AF_UNSPEC can be only related to
>>> addresses and we should not provide any checks (for address) if socket
>>> is unrestrictable (i.e. it's not TCP). It's not useful and might lead to
>>> error incosistency for non-TCP sockets.
>>
>> Good catch, let's use sk_is_tcp().
>>
>>>
>>> Btw, I suppose we can improve error consistency by bringing more checks
>>> from INET/TCP stack. For example it may be useful to return EISCONN
>>> instead of EACCES while connect(2) is called on a connected socket.
>>
>> Yes, that would be nice (with the related tests).
>>
>>>
>>> This should be done really carefully and only for some useful cases.
>>> Anyway it's not related to the current patch (since it's not a bug).
>>
>> Sure.
> 
> I have a little question to clarify before sending a next version. Are
> we condisering order of network checks for error consistency?
> 
> For example, in the current_check_access_socket() we have following
> order of checks for ipv4 connect(2) action:
> (1) addrlen < sizeof(struct sockaddr_in) -> return -EINVAL
> (2) sa_family != sk_family -> return -EINVAL
> 
> The ipv4 stack has a check for sock->state before (1) and (2), which can
> return -EISCONN if the socket is already connected.
> 
> This results in the possiblity of two following scenarios:
> 
> Landlock enabled:
> 1. socket(ipv4) -> OK
> 2. connect(ipv4 address) -> OK
> 3. connect(ipv6 address) -> -EINVAL (sa_family != sk_family)
> 
> Landlock disabled:
> 1. socket(ipv4) -> OK
> 2. connect(ipv4 address) -> OK
> 3. connect(ipv6 address) -> -EISCONN (socket is already connected)
> 
> I have always considered the order of network checks as part of error
> consistency, and I'd like to make sure that we're on the same page
> before extending current patch with error inconsistency fixes.

BTW, a similar inconsistency in the error order was also found in
selinux hooks. Accounting [1], I wonder if validating socket state
in security hooks for bind/connect actions has been considered before.

[1] https://lore.kernel.org/all/20231228113917.62089-1-mic@digikod.net/

Re: [RFC PATCH v1 1/2] landlock: Fix non-TCP sockets restriction

[Mickaël Salaün]

On Mon, Oct 07, 2024 at 02:58:43PM +0300, Mikhail Ivanov wrote:
> On 10/7/2024 2:06 PM, Mikhail Ivanov wrote:
> > On 10/5/2024 6:49 PM, Mickaël Salaün wrote:
> > > On Fri, Oct 04, 2024 at 09:16:56PM +0300, Mikhail Ivanov wrote:
> > > > On 10/4/2024 1:13 PM, Mickaël Salaün wrote:
> > > > > On Fri, Oct 04, 2024 at 12:30:02AM +0300, Mikhail Ivanov wrote:
> > > > > > On 10/3/2024 8:45 PM, Mickaël Salaün wrote:
> > > > > > > Please also add Matthieu in Cc for the network patch series.
> > > > > > > 
> > > > > > > On Thu, Oct 03, 2024 at 10:39:31PM +0800, Mikhail Ivanov wrote:
> > > > > > > > Do not check TCP access right if socket protocol is not IPPROTO_TCP.
> > > > > > > > LANDLOCK_ACCESS_NET_BIND_TCP and LANDLOCK_ACCESS_NET_CONNECT_TCP
> > > > > > > > should not restrict bind(2) and connect(2) for non-TCP protocols
> > > > > > > > (SCTP, MPTCP, SMC).
> > > > > > > > 
> > > > > > > > Closes: https://github.com/landlock-lsm/linux/issues/40
> > > > > > > > Fixes: fff69fb03dde ("landlock: Support network
> > > > > > > > rules with TCP bind and connect")
> > > > > > > > Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
> > > > > > > > ---
> > > > > > > >     security/landlock/net.c | 2 +-
> > > > > > > >     1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > > > 
> > > > > > > > diff --git a/security/landlock/net.c b/security/landlock/net.c
> > > > > > > > index bc3d943a7118..6f59dd98bb13 100644
> > > > > > > > --- a/security/landlock/net.c
> > > > > > > > +++ b/security/landlock/net.c
> > > > > > > > @@ -68,7 +68,7 @@ static int
> > > > > > > > current_check_access_socket(struct socket *const
> > > > > > > > sock,
> > > > > > > >             return -EACCES;
> > > > > > > >         /* Checks if it's a (potential) TCP socket. */
> > > > > > > 
> > > > > > > We can extend this comment to explain that we don't use sk_is_tcp()
> > > > > > > because we need to handle the AF_UNSPEC case.
> > > > > > 
> > > > > > Indeed, I'll do this.
> > > > 
> > > > I've noticed that we still should check sk->sk_family = AF_INET{,6}
> > > > here (so sk_is_tcp() is suitable). AF_UNSPEC can be only related to
> > > > addresses and we should not provide any checks (for address) if socket
> > > > is unrestrictable (i.e. it's not TCP). It's not useful and might lead to
> > > > error incosistency for non-TCP sockets.
> > > 
> > > Good catch, let's use sk_is_tcp().
> > > 
> > > > 
> > > > Btw, I suppose we can improve error consistency by bringing more checks
> > > > from INET/TCP stack. For example it may be useful to return EISCONN
> > > > instead of EACCES while connect(2) is called on a connected socket.
> > > 
> > > Yes, that would be nice (with the related tests).
> > > 
> > > > 
> > > > This should be done really carefully and only for some useful cases.
> > > > Anyway it's not related to the current patch (since it's not a bug).
> > > 
> > > Sure.
> > 
> > I have a little question to clarify before sending a next version. Are
> > we condisering order of network checks for error consistency?
> > 
> > For example, in the current_check_access_socket() we have following
> > order of checks for ipv4 connect(2) action:
> > (1) addrlen < sizeof(struct sockaddr_in) -> return -EINVAL
> > (2) sa_family != sk_family -> return -EINVAL
> > 
> > The ipv4 stack has a check for sock->state before (1) and (2), which can
> > return -EISCONN if the socket is already connected.
> > 
> > This results in the possiblity of two following scenarios:
> > 
> > Landlock enabled:
> > 1. socket(ipv4) -> OK
> > 2. connect(ipv4 address) -> OK
> > 3. connect(ipv6 address) -> -EINVAL (sa_family != sk_family)
> > 
> > Landlock disabled:
> > 1. socket(ipv4) -> OK
> > 2. connect(ipv4 address) -> OK
> > 3. connect(ipv6 address) -> -EISCONN (socket is already connected)
> > 
> > I have always considered the order of network checks as part of error
> > consistency, and I'd like to make sure that we're on the same page
> > before extending current patch with error inconsistency fixes.

Yes, we should try to stick to the same error ordering, and this should
be covered by tests.

> 
> BTW, a similar inconsistency in the error order was also found in
> selinux hooks. Accounting [1], I wonder if validating socket state
> in security hooks for bind/connect actions has been considered before.
> 
> [1] https://lore.kernel.org/all/20231228113917.62089-1-mic@digikod.net/

I think Landlock has a better test coverage than any other
(access-control) LSM, which is why we find these inconsistencies.
The LSM hooks should be better integrated into the network stack to
benefit from all the inconsistency checks.  On the other end, one
benefit of being call earlier is that an LSM can stop invalid requests
(I don't think it's worth it though).

However, before trying to change the hook call sites, we should first
make sure the side effects are OK for every LSMs:
https://lore.kernel.org/all/20240327120036.233641-1-mic@digikod.net/
...which also include testing (which is what we do for Landlock).

Any though from the network folks?