[RFC PATCH v2 00/12] Socket type control for Landlock

[RFC PATCH v2 00/12] Socket type control for Landlock

[Mikhail Ivanov]

Hello! This is v2 RFC patch dedicated to socket protocols restriction.

It is based on the landlock's mic-next branch on top of v6.9 kernel
version.

Description
===========
Patchset implements new type of Landlock rule, that restricts socket
protocols used in the sandboxed process. This restriction does not affect
socket actions such as bind(2) or send(2), only those actions that result
in a socket with unwanted protocol (e.g. creating socket with socket(2)).

Such restriction would be useful to ensure that a sandboxed process uses
only necessary protocols. For example sandboxed TCP server may want to
permit only TCP sockets and deny any others. See [1] for more cases.

The rules store information about the socket family and type. Thus, any
protocol that can be defined by a family-type pair can be restricted by
Landlock.

struct landlock_socket_attr {
	__u64 allowed_access;
	int family; // same as domain in socket(2)
	int type; // see socket(2)
}

Patchset currently implements rule only for socket creation, but
other necessary rules will also be impemented. [2]

[1] https://lore.kernel.org/all/ZJvy2SViorgc+cZI@google.com/
[2] https://lore.kernel.org/all/b8a2045a-e7e8-d141-7c01-bf47874c7930@digikod.net/

Code coverage
=============
Code coverage(gcov) report with the launch of all the landlock selftests:
* security/landlock:
lines......: 93.3% (795 of 852 lines)
functions..: 95.5% (106 of 111 functions)

* security/landlock/socket.c:
lines......: 100.0% (33 of 33 lines)
functions..: 100.0% (5 of 5 functions)

General changes
===============
 * Rebases on mic-next (landlock-6.10-rc1).
 * Refactors code and commits.
 * Renames `family` into `domain` in landlock_socket_attr.
 * Changes ABI version from 5 to 6.
 * Reverts landlock_key.data type from u64 to uinptr_t.
 * Adds mini.socket_overflow, mini.socket_invalid_type tests.

Previous versions
=================
v1: https://lore.kernel.org/all/20240408093927.1759381-1-ivanov.mikhail1@huawei-partners.com/

Mikhail Ivanov (12):
  landlock: Support socket access-control
  landlock: Add hook on socket creation
  selftests/landlock: Add protocol.create to socket tests
  selftests/landlock: Add protocol.socket_access_rights to socket tests
  selftests/landlock: Add protocol.rule_with_unknown_access to socket
    tests
  selftests/landlock: Add protocol.rule_with_unhandled_access to socket
    tests
  selftests/landlock: Add protocol.inval to socket tests
  selftests/landlock: Add tcp_layers.ruleset_overlap to socket tests
  selftests/landlock: Add mini.ruleset_with_unknown_access to socket
    tests
  selftests/landlock: Add mini.socket_overflow to socket tests
  selftests/landlock: Add mini.socket_invalid_type to socket tests
  samples/landlock: Support socket protocol restrictions

 include/uapi/linux/landlock.h                 |  53 +-
 samples/landlock/sandboxer.c                  | 141 ++++-
 security/landlock/Makefile                    |   2 +-
 security/landlock/limits.h                    |   5 +
 security/landlock/ruleset.c                   |  37 +-
 security/landlock/ruleset.h                   |  41 +-
 security/landlock/setup.c                     |   2 +
 security/landlock/socket.c                    | 130 ++++
 security/landlock/socket.h                    |  19 +
 security/landlock/syscalls.c                  |  66 +-
 tools/testing/selftests/landlock/base_test.c  |   2 +-
 tools/testing/selftests/landlock/common.h     |   1 +
 tools/testing/selftests/landlock/config       |   1 +
 .../testing/selftests/landlock/socket_test.c  | 581 ++++++++++++++++++
 14 files changed, 1056 insertions(+), 25 deletions(-)
 create mode 100644 security/landlock/socket.c
 create mode 100644 security/landlock/socket.h
 create mode 100644 tools/testing/selftests/landlock/socket_test.c

-- 
2.34.1

[RFC PATCH v2 01/12] landlock: Support socket access-control

[Mikhail Ivanov]

* Add new landlock rule type that corresponds to the restriction of
  socket protocols. This is represented as an landlock_socket_attr
  structure. Protocol allowed by landlock must be described by
  a family-type pair (see socket(2)).

* Support socket rule storage in landlock ruleset.

* Add flag LANDLOCK_ACCESS_SOCKET_CREATE that will provide the
  ability to control socket creation.

* Add socket.c file that will contain socket rules management and hooks.
  Implement helper pack_socket_key() to convert 32-bit family and type
  values into uintptr_t. This is possible due to the fact that these
  values are limited to AF_MAX (=46), SOCK_MAX (=11) constants. Assumption
  is checked in build-time by the helper.

* Support socket rules in landlock syscalls. Change ABI version to 6.

Closes: https://github.com/landlock-lsm/linux/issues/6
Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
---

Changes since v1:
* Reverts landlock_key.data type from u64 to uinptr_t.
* Adds helper to pack domain and type values into uintptr_t.
* Denies inserting socket rule with invalid family and type.
* Renames 'domain' to 'family' in landlock_socket_attr.
* Updates ABI version to 6 since ioctl patches changed it to 5.
* Formats code with clang-format.
* Minor fixes.
---
 include/uapi/linux/landlock.h                | 53 +++++++++++++++-
 security/landlock/Makefile                   |  2 +-
 security/landlock/limits.h                   |  5 ++
 security/landlock/ruleset.c                  | 37 ++++++++++-
 security/landlock/ruleset.h                  | 41 +++++++++++-
 security/landlock/socket.c                   | 60 ++++++++++++++++++
 security/landlock/socket.h                   | 17 +++++
 security/landlock/syscalls.c                 | 66 ++++++++++++++++++--
 tools/testing/selftests/landlock/base_test.c |  2 +-
 9 files changed, 272 insertions(+), 11 deletions(-)
 create mode 100644 security/landlock/socket.c
 create mode 100644 security/landlock/socket.h

diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
index 68625e728f43..a25ba5983dfb 100644
--- a/include/uapi/linux/landlock.h
+++ b/include/uapi/linux/landlock.h
@@ -37,6 +37,13 @@ struct landlock_ruleset_attr {
 	 * rule explicitly allow them.
 	 */
 	__u64 handled_access_net;
+
+	/**
+	 * @handled_access_socket: Bitmask of actions (cf. `Socket flags`_)
+	 * that is handled by this ruleset and should then be forbidden if no
+	 * rule explicitly allow them.
+	 */
+	__u64 handled_access_socket;
 };
 
 /*
@@ -65,6 +72,11 @@ enum landlock_rule_type {
 	 * landlock_net_port_attr .
 	 */
 	LANDLOCK_RULE_NET_PORT,
+	/**
+	 * @LANDLOCK_RULE_SOCKET: Type of a &struct
+	 * landlock_socket_attr .
+	 */
+	LANDLOCK_RULE_SOCKET,
 };
 
 /**
@@ -115,6 +127,28 @@ struct landlock_net_port_attr {
 	__u64 port;
 };
 
+/**
+ * struct landlock_socket_attr - Socket definition
+ *
+ * Argument of sys_landlock_add_rule().
+ */
+struct landlock_socket_attr {
+	/**
+	 * @allowed_access: Bitmask of allowed access for a socket
+	 * (cf. `Socket flags`_).
+	 */
+	__u64 allowed_access;
+	/**
+	 * @family: Protocol family used for communication
+	 * (same as domain in socket(2)).
+	 */
+	int family;
+	/**
+	 * @type: Socket type (see socket(2)).
+	 */
+	int type;
+};
+
 /**
  * DOC: fs_access
  *
@@ -251,7 +285,7 @@ struct landlock_net_port_attr {
  * DOC: net_access
  *
  * Network flags
- * ~~~~~~~~~~~~~~~~
+ * ~~~~~~~~~~~~~
  *
  * These flags enable to restrict a sandboxed process to a set of network
  * actions. This is supported since the Landlock ABI version 4.
@@ -266,4 +300,21 @@ struct landlock_net_port_attr {
 #define LANDLOCK_ACCESS_NET_BIND_TCP			(1ULL << 0)
 #define LANDLOCK_ACCESS_NET_CONNECT_TCP			(1ULL << 1)
 /* clang-format on */
+
+/**
+ * DOC: socket_access
+ *
+ * Socket flags
+ * ~~~~~~~~~~~~
+ *
+ * These flags enable to restrict a sanboxed process to a set of socket
+ * protocols. This is supported since the Landlock ABI version 6.
+ *
+ * The following access rights apply only to sockets:
+ *
+ * - %LANDLOCK_ACCESS_SOCKET_CREATE: Create a socket.
+ */
+/* clang-format off */
+#define LANDLOCK_ACCESS_SOCKET_CREATE			(1ULL << 0)
+/* clang-format on */
 #endif /* _UAPI_LINUX_LANDLOCK_H */
diff --git a/security/landlock/Makefile b/security/landlock/Makefile
index b4538b7cf7d2..ff1dd98f6a1b 100644
--- a/security/landlock/Makefile
+++ b/security/landlock/Makefile
@@ -1,6 +1,6 @@
 obj-$(CONFIG_SECURITY_LANDLOCK) := landlock.o
 
 landlock-y := setup.o syscalls.o object.o ruleset.o \
-	cred.o task.o fs.o
+	cred.o task.o fs.o socket.o
 
 landlock-$(CONFIG_INET) += net.o
diff --git a/security/landlock/limits.h b/security/landlock/limits.h
index 20fdb5ff3514..448b4d596783 100644
--- a/security/landlock/limits.h
+++ b/security/landlock/limits.h
@@ -28,6 +28,11 @@
 #define LANDLOCK_NUM_ACCESS_NET		__const_hweight64(LANDLOCK_MASK_ACCESS_NET)
 #define LANDLOCK_SHIFT_ACCESS_NET	LANDLOCK_NUM_ACCESS_FS
 
+#define LANDLOCK_LAST_ACCESS_SOCKET	    LANDLOCK_ACCESS_SOCKET_CREATE
+#define LANDLOCK_MASK_ACCESS_SOCKET	    ((LANDLOCK_LAST_ACCESS_SOCKET << 1) - 1)
+#define LANDLOCK_NUM_ACCESS_SOCKET		__const_hweight64(LANDLOCK_MASK_ACCESS_SOCKET)
+#define LANDLOCK_SHIFT_ACCESS_SOCKET	LANDLOCK_NUM_ACCESS_SOCKET
+
 /* clang-format on */
 
 #endif /* _SECURITY_LANDLOCK_LIMITS_H */
diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
index e0a5fbf9201a..c782f7cd313d 100644
--- a/security/landlock/ruleset.c
+++ b/security/landlock/ruleset.c
@@ -40,6 +40,7 @@ static struct landlock_ruleset *create_ruleset(const u32 num_layers)
 #if IS_ENABLED(CONFIG_INET)
 	new_ruleset->root_net_port = RB_ROOT;
 #endif /* IS_ENABLED(CONFIG_INET) */
+	new_ruleset->root_socket = RB_ROOT;
 
 	new_ruleset->num_layers = num_layers;
 	/*
@@ -52,12 +53,13 @@ static struct landlock_ruleset *create_ruleset(const u32 num_layers)
 
 struct landlock_ruleset *
 landlock_create_ruleset(const access_mask_t fs_access_mask,
-			const access_mask_t net_access_mask)
+			const access_mask_t net_access_mask,
+			const access_mask_t socket_access_mask)
 {
 	struct landlock_ruleset *new_ruleset;
 
 	/* Informs about useless ruleset. */
-	if (!fs_access_mask && !net_access_mask)
+	if (!fs_access_mask && !net_access_mask && !socket_access_mask)
 		return ERR_PTR(-ENOMSG);
 	new_ruleset = create_ruleset(1);
 	if (IS_ERR(new_ruleset))
@@ -66,6 +68,9 @@ landlock_create_ruleset(const access_mask_t fs_access_mask,
 		landlock_add_fs_access_mask(new_ruleset, fs_access_mask, 0);
 	if (net_access_mask)
 		landlock_add_net_access_mask(new_ruleset, net_access_mask, 0);
+	if (socket_access_mask)
+		landlock_add_socket_access_mask(new_ruleset, socket_access_mask,
+						0);
 	return new_ruleset;
 }
 
@@ -89,6 +94,9 @@ static bool is_object_pointer(const enum landlock_key_type key_type)
 		return false;
 #endif /* IS_ENABLED(CONFIG_INET) */
 
+	case LANDLOCK_KEY_SOCKET:
+		return false;
+
 	default:
 		WARN_ON_ONCE(1);
 		return false;
@@ -146,6 +154,9 @@ static struct rb_root *get_root(struct landlock_ruleset *const ruleset,
 		return &ruleset->root_net_port;
 #endif /* IS_ENABLED(CONFIG_INET) */
 
+	case LANDLOCK_KEY_SOCKET:
+		return &ruleset->root_socket;
+
 	default:
 		WARN_ON_ONCE(1);
 		return ERR_PTR(-EINVAL);
@@ -175,7 +186,9 @@ static void build_check_ruleset(void)
 	BUILD_BUG_ON(ruleset.num_layers < LANDLOCK_MAX_NUM_LAYERS);
 	BUILD_BUG_ON(access_masks <
 		     ((LANDLOCK_MASK_ACCESS_FS << LANDLOCK_SHIFT_ACCESS_FS) |
-		      (LANDLOCK_MASK_ACCESS_NET << LANDLOCK_SHIFT_ACCESS_NET)));
+		      (LANDLOCK_MASK_ACCESS_NET << LANDLOCK_SHIFT_ACCESS_NET) |
+		      (LANDLOCK_MASK_ACCESS_SOCKET
+		       << LANDLOCK_SHIFT_ACCESS_SOCKET)));
 }
 
 /**
@@ -399,6 +412,11 @@ static int merge_ruleset(struct landlock_ruleset *const dst,
 		goto out_unlock;
 #endif /* IS_ENABLED(CONFIG_INET) */
 
+	/* Merges the @src socket tree. */
+	err = merge_tree(dst, src, LANDLOCK_KEY_SOCKET);
+	if (err)
+		goto out_unlock;
+
 out_unlock:
 	mutex_unlock(&src->lock);
 	mutex_unlock(&dst->lock);
@@ -462,6 +480,11 @@ static int inherit_ruleset(struct landlock_ruleset *const parent,
 		goto out_unlock;
 #endif /* IS_ENABLED(CONFIG_INET) */
 
+	/* Copies the @parent socket tree. */
+	err = inherit_tree(parent, child, LANDLOCK_KEY_SOCKET);
+	if (err)
+		goto out_unlock;
+
 	if (WARN_ON_ONCE(child->num_layers <= parent->num_layers)) {
 		err = -EINVAL;
 		goto out_unlock;
@@ -498,6 +521,10 @@ static void free_ruleset(struct landlock_ruleset *const ruleset)
 		free_rule(freeme, LANDLOCK_KEY_NET_PORT);
 #endif /* IS_ENABLED(CONFIG_INET) */
 
+	rbtree_postorder_for_each_entry_safe(freeme, next,
+					     &ruleset->root_socket, node)
+		free_rule(freeme, LANDLOCK_KEY_SOCKET);
+
 	put_hierarchy(ruleset->hierarchy);
 	kfree(ruleset);
 }
@@ -708,6 +735,10 @@ landlock_init_layer_masks(const struct landlock_ruleset *const domain,
 		break;
 #endif /* IS_ENABLED(CONFIG_INET) */
 
+	case LANDLOCK_KEY_SOCKET:
+		get_access_mask = landlock_get_socket_access_mask;
+		num_access = LANDLOCK_NUM_ACCESS_SOCKET;
+		break;
 	default:
 		WARN_ON_ONCE(1);
 		return 0;
diff --git a/security/landlock/ruleset.h b/security/landlock/ruleset.h
index c7f1526784fd..a9773efd529b 100644
--- a/security/landlock/ruleset.h
+++ b/security/landlock/ruleset.h
@@ -92,6 +92,12 @@ enum landlock_key_type {
 	 * node keys.
 	 */
 	LANDLOCK_KEY_NET_PORT,
+
+	/**
+	 * @LANDLOCK_KEY_SOCKET: Type of &landlock_ruleset.root_socket's
+	 * node keys.
+	 */
+	LANDLOCK_KEY_SOCKET,
 };
 
 /**
@@ -177,6 +183,15 @@ struct landlock_ruleset {
 	struct rb_root root_net_port;
 #endif /* IS_ENABLED(CONFIG_INET) */
 
+	/**
+	 * @root_socket: Root of a red-black tree containing &struct
+	 * landlock_rule nodes with socket type, described by (family, type)
+	 * pair (see socket(2)). Once a ruleset is tied to a
+	 * process (i.e. as a domain), this tree is immutable until @usage
+	 * reaches zero.
+	 */
+	struct rb_root root_socket;
+
 	/**
 	 * @hierarchy: Enables hierarchy identification even when a parent
 	 * domain vanishes.  This is needed for the ptrace protection.
@@ -233,7 +248,8 @@ struct landlock_ruleset {
 
 struct landlock_ruleset *
 landlock_create_ruleset(const access_mask_t access_mask_fs,
-			const access_mask_t access_mask_net);
+			const access_mask_t access_mask_net,
+			const access_mask_t access_mask_socket);
 
 void landlock_put_ruleset(struct landlock_ruleset *const ruleset);
 void landlock_put_ruleset_deferred(struct landlock_ruleset *const ruleset);
@@ -282,6 +298,20 @@ landlock_add_net_access_mask(struct landlock_ruleset *const ruleset,
 		(net_mask << LANDLOCK_SHIFT_ACCESS_NET);
 }
 
+static inline void
+landlock_add_socket_access_mask(struct landlock_ruleset *const ruleset,
+				const access_mask_t socket_access_mask,
+				const u16 layer_level)
+{
+	access_mask_t socket_mask = socket_access_mask &
+				    LANDLOCK_MASK_ACCESS_SOCKET;
+
+	/* Should already be checked in sys_landlock_create_ruleset(). */
+	WARN_ON_ONCE(socket_access_mask != socket_mask);
+	ruleset->access_masks[layer_level] |=
+		(socket_mask << LANDLOCK_SHIFT_ACCESS_SOCKET);
+}
+
 static inline access_mask_t
 landlock_get_raw_fs_access_mask(const struct landlock_ruleset *const ruleset,
 				const u16 layer_level)
@@ -309,6 +339,15 @@ landlock_get_net_access_mask(const struct landlock_ruleset *const ruleset,
 	       LANDLOCK_MASK_ACCESS_NET;
 }
 
+static inline access_mask_t
+landlock_get_socket_access_mask(const struct landlock_ruleset *const ruleset,
+				const u16 layer_level)
+{
+	return (ruleset->access_masks[layer_level] >>
+		LANDLOCK_SHIFT_ACCESS_SOCKET) &
+	       LANDLOCK_MASK_ACCESS_SOCKET;
+}
+
 bool landlock_unmask_layers(const struct landlock_rule *const rule,
 			    const access_mask_t access_request,
 			    layer_mask_t (*const layer_masks)[],
diff --git a/security/landlock/socket.c b/security/landlock/socket.c
new file mode 100644
index 000000000000..1249a4a36503
--- /dev/null
+++ b/security/landlock/socket.c
@@ -0,0 +1,60 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Landlock LSM - Socket management and hooks
+ *
+ * Copyright © 2024 Huawei Tech. Co., Ltd.
+ */
+
+#include <linux/net.h>
+#include <linux/socket.h>
+#include <linux/stddef.h>
+
+#include "limits.h"
+#include "ruleset.h"
+#include "socket.h"
+
+static uintptr_t pack_socket_key(const int family, const int type)
+{
+	union {
+		struct {
+			unsigned short family, type;
+		} __packed data;
+		uintptr_t packed;
+	} socket_key;
+
+	/* Checks that all supported socket families and types can be stored in socket_key. */
+	BUILD_BUG_ON(AF_MAX > (typeof(socket_key.data.family))~0);
+	BUILD_BUG_ON(SOCK_MAX > (typeof(socket_key.data.type))~0);
+
+	/* Checks that socket_key can be stored in landlock_key. */
+	BUILD_BUG_ON(sizeof(socket_key.data) > sizeof(socket_key.packed));
+	BUILD_BUG_ON(sizeof(socket_key.packed) >
+		     sizeof_field(union landlock_key, data));
+
+	socket_key.data.family = (unsigned short)family;
+	socket_key.data.type = (unsigned short)type;
+
+	return socket_key.packed;
+}
+
+int landlock_append_socket_rule(struct landlock_ruleset *const ruleset,
+				const int family, const int type,
+				access_mask_t access_rights)
+{
+	int err;
+
+	const struct landlock_id id = {
+		.key.data = pack_socket_key(family, type),
+		.type = LANDLOCK_KEY_SOCKET,
+	};
+
+	/* Transforms relative access rights to absolute ones. */
+	access_rights |= LANDLOCK_MASK_ACCESS_SOCKET &
+			 ~landlock_get_socket_access_mask(ruleset, 0);
+
+	mutex_lock(&ruleset->lock);
+	err = landlock_insert_rule(ruleset, id, access_rights);
+	mutex_unlock(&ruleset->lock);
+
+	return err;
+}
diff --git a/security/landlock/socket.h b/security/landlock/socket.h
new file mode 100644
index 000000000000..8519357f1c39
--- /dev/null
+++ b/security/landlock/socket.h
@@ -0,0 +1,17 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Landlock LSM - Socket management and hooks
+ *
+ * Copyright © 2024 Huawei Tech. Co., Ltd.
+ */
+
+#ifndef _SECURITY_LANDLOCK_SOCKET_H
+#define _SECURITY_LANDLOCK_SOCKET_H
+
+#include "ruleset.h"
+
+int landlock_append_socket_rule(struct landlock_ruleset *const ruleset,
+				const int family, const int type,
+				access_mask_t access_rights);
+
+#endif /* _SECURITY_LANDLOCK_SOCKET_H */
diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c
index 03b470f5a85a..30c771f5e74f 100644
--- a/security/landlock/syscalls.c
+++ b/security/landlock/syscalls.c
@@ -24,12 +24,14 @@
 #include <linux/syscalls.h>
 #include <linux/types.h>
 #include <linux/uaccess.h>
+#include <linux/net.h>
 #include <uapi/linux/landlock.h>
 
 #include "cred.h"
 #include "fs.h"
 #include "limits.h"
 #include "net.h"
+#include "socket.h"
 #include "ruleset.h"
 #include "setup.h"
 
@@ -88,7 +90,8 @@ static void build_check_abi(void)
 	struct landlock_ruleset_attr ruleset_attr;
 	struct landlock_path_beneath_attr path_beneath_attr;
 	struct landlock_net_port_attr net_port_attr;
-	size_t ruleset_size, path_beneath_size, net_port_size;
+	struct landlock_socket_attr socket_attr;
+	size_t ruleset_size, path_beneath_size, net_port_size, socket_size;
 
 	/*
 	 * For each user space ABI structures, first checks that there is no
@@ -97,8 +100,9 @@ static void build_check_abi(void)
 	 */
 	ruleset_size = sizeof(ruleset_attr.handled_access_fs);
 	ruleset_size += sizeof(ruleset_attr.handled_access_net);
+	ruleset_size += sizeof(ruleset_attr.handled_access_socket);
 	BUILD_BUG_ON(sizeof(ruleset_attr) != ruleset_size);
-	BUILD_BUG_ON(sizeof(ruleset_attr) != 16);
+	BUILD_BUG_ON(sizeof(ruleset_attr) != 24);
 
 	path_beneath_size = sizeof(path_beneath_attr.allowed_access);
 	path_beneath_size += sizeof(path_beneath_attr.parent_fd);
@@ -109,6 +113,12 @@ static void build_check_abi(void)
 	net_port_size += sizeof(net_port_attr.port);
 	BUILD_BUG_ON(sizeof(net_port_attr) != net_port_size);
 	BUILD_BUG_ON(sizeof(net_port_attr) != 16);
+
+	socket_size = sizeof(socket_attr.allowed_access);
+	socket_size += sizeof(socket_attr.family);
+	socket_size += sizeof(socket_attr.type);
+	BUILD_BUG_ON(sizeof(socket_attr) != socket_size);
+	BUILD_BUG_ON(sizeof(socket_attr) != 16);
 }
 
 /* Ruleset handling */
@@ -149,7 +159,7 @@ static const struct file_operations ruleset_fops = {
 	.write = fop_dummy_write,
 };
 
-#define LANDLOCK_ABI_VERSION 5
+#define LANDLOCK_ABI_VERSION 6
 
 /**
  * sys_landlock_create_ruleset - Create a new ruleset
@@ -213,9 +223,15 @@ SYSCALL_DEFINE3(landlock_create_ruleset,
 	    LANDLOCK_MASK_ACCESS_NET)
 		return -EINVAL;
 
+	/* Checks socket content (and 32-bits cast). */
+	if ((ruleset_attr.handled_access_socket |
+	     LANDLOCK_MASK_ACCESS_SOCKET) != LANDLOCK_MASK_ACCESS_SOCKET)
+		return -EINVAL;
+
 	/* Checks arguments and transforms to kernel struct. */
 	ruleset = landlock_create_ruleset(ruleset_attr.handled_access_fs,
-					  ruleset_attr.handled_access_net);
+					  ruleset_attr.handled_access_net,
+					  ruleset_attr.handled_access_socket);
 	if (IS_ERR(ruleset))
 		return PTR_ERR(ruleset);
 
@@ -371,6 +387,45 @@ static int add_rule_net_port(struct landlock_ruleset *ruleset,
 					net_port_attr.allowed_access);
 }
 
+static int add_rule_socket(struct landlock_ruleset *ruleset,
+			   const void __user *const rule_attr)
+{
+	struct landlock_socket_attr socket_attr;
+	int family, type;
+	int res;
+	access_mask_t mask;
+
+	/* Copies raw user space buffer. */
+	res = copy_from_user(&socket_attr, rule_attr, sizeof(socket_attr));
+	if (res)
+		return -EFAULT;
+
+	/*
+	 * Informs about useless rule: empty allowed_access (i.e. deny rules)
+	 * are ignored by socket actions.
+	 */
+	if (!socket_attr.allowed_access)
+		return -ENOMSG;
+
+	/* Checks that allowed_access matches the @ruleset constraints. */
+	mask = landlock_get_socket_access_mask(ruleset, 0);
+	if ((socket_attr.allowed_access | mask) != mask)
+		return -EINVAL;
+
+	family = socket_attr.family;
+	type = socket_attr.type;
+
+	/* Denies inserting a rule with unsupported socket family and type. */
+	if (family < 0 || family >= AF_MAX)
+		return -EINVAL;
+	if (type < 0 || type >= SOCK_MAX)
+		return -EINVAL;
+
+	/* Imports the new rule. */
+	return landlock_append_socket_rule(ruleset, family, type,
+					   socket_attr.allowed_access);
+}
+
 /**
  * sys_landlock_add_rule - Add a new rule to a ruleset
  *
@@ -429,6 +484,9 @@ SYSCALL_DEFINE4(landlock_add_rule, const int, ruleset_fd,
 	case LANDLOCK_RULE_NET_PORT:
 		err = add_rule_net_port(ruleset, rule_attr);
 		break;
+	case LANDLOCK_RULE_SOCKET:
+		err = add_rule_socket(ruleset, rule_attr);
+		break;
 	default:
 		err = -EINVAL;
 		break;
diff --git a/tools/testing/selftests/landlock/base_test.c b/tools/testing/selftests/landlock/base_test.c
index 3c1e9f35b531..52b00472a487 100644
--- a/tools/testing/selftests/landlock/base_test.c
+++ b/tools/testing/selftests/landlock/base_test.c
@@ -75,7 +75,7 @@ TEST(abi_version)
 	const struct landlock_ruleset_attr ruleset_attr = {
 		.handled_access_fs = LANDLOCK_ACCESS_FS_READ_FILE,
 	};
-	ASSERT_EQ(5, landlock_create_ruleset(NULL, 0,
+	ASSERT_EQ(6, landlock_create_ruleset(NULL, 0,
 					     LANDLOCK_CREATE_RULESET_VERSION));
 
 	ASSERT_EQ(-1, landlock_create_ruleset(&ruleset_attr, 0,
-- 
2.34.1

[RFC PATCH v2 02/12] landlock: Add hook on socket creation

[Mikhail Ivanov]

Add hook to security_socket_post_create(), which checks whether the socket
type and family are allowed by domain. Hook is called after initializing
the socket in the network stack to not wrongfully return EACCES for a
family-type pair, which is considered invalid by the protocol.

Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
---

Changes since v1:
* Use lsm hook arguments instead of struct socket fields as family-type
  values.
* Packs socket family and type using helper.
* Fixes commit message.
* Formats with clang-format.
---
 security/landlock/setup.c  |  2 ++
 security/landlock/socket.c | 70 ++++++++++++++++++++++++++++++++++++++
 security/landlock/socket.h |  2 ++
 3 files changed, 74 insertions(+)

diff --git a/security/landlock/setup.c b/security/landlock/setup.c
index 28519a45b11f..fd4e7e8f3cb2 100644
--- a/security/landlock/setup.c
+++ b/security/landlock/setup.c
@@ -14,6 +14,7 @@
 #include "cred.h"
 #include "fs.h"
 #include "net.h"
+#include "socket.h"
 #include "setup.h"
 #include "task.h"
 
@@ -37,6 +38,7 @@ static int __init landlock_init(void)
 	landlock_add_task_hooks();
 	landlock_add_fs_hooks();
 	landlock_add_net_hooks();
+	landlock_add_socket_hooks();
 	landlock_initialized = true;
 	pr_info("Up and running.\n");
 	return 0;
diff --git a/security/landlock/socket.c b/security/landlock/socket.c
index 1249a4a36503..b2775473b3dc 100644
--- a/security/landlock/socket.c
+++ b/security/landlock/socket.c
@@ -8,7 +8,9 @@
 #include <linux/net.h>
 #include <linux/socket.h>
 #include <linux/stddef.h>
+#include <net/ipv6.h>
 
+#include "cred.h"
 #include "limits.h"
 #include "ruleset.h"
 #include "socket.h"
@@ -58,3 +60,71 @@ int landlock_append_socket_rule(struct landlock_ruleset *const ruleset,
 
 	return err;
 }
+
+static access_mask_t
+get_raw_handled_socket_accesses(const struct landlock_ruleset *const domain)
+{
+	access_mask_t access_dom = 0;
+	size_t layer_level;
+
+	for (layer_level = 0; layer_level < domain->num_layers; layer_level++)
+		access_dom |=
+			landlock_get_socket_access_mask(domain, layer_level);
+	return access_dom;
+}
+
+static const struct landlock_ruleset *get_current_socket_domain(void)
+{
+	const struct landlock_ruleset *const dom =
+		landlock_get_current_domain();
+
+	if (!dom || !get_raw_handled_socket_accesses(dom))
+		return NULL;
+
+	return dom;
+}
+
+static int current_check_access_socket(struct socket *const sock, int family,
+				       int type,
+				       const access_mask_t access_request)
+{
+	layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_SOCKET] = {};
+	const struct landlock_rule *rule;
+	access_mask_t handled_access;
+	struct landlock_id id = {
+		.type = LANDLOCK_KEY_SOCKET,
+	};
+	const struct landlock_ruleset *const dom = get_current_socket_domain();
+
+	if (!dom)
+		return 0;
+	if (WARN_ON_ONCE(dom->num_layers < 1))
+		return -EACCES;
+
+	id.key.data = pack_socket_key(family, type);
+
+	rule = landlock_find_rule(dom, id);
+	handled_access = landlock_init_layer_masks(
+		dom, access_request, &layer_masks, LANDLOCK_KEY_SOCKET);
+	if (landlock_unmask_layers(rule, handled_access, &layer_masks,
+				   ARRAY_SIZE(layer_masks)))
+		return 0;
+	return -EACCES;
+}
+
+static int hook_socket_create(struct socket *const sock, int family, int type,
+			      int protocol, int kern)
+{
+	return current_check_access_socket(sock, family, type,
+					   LANDLOCK_ACCESS_SOCKET_CREATE);
+}
+
+static struct security_hook_list landlock_hooks[] __ro_after_init = {
+	LSM_HOOK_INIT(socket_post_create, hook_socket_create),
+};
+
+__init void landlock_add_socket_hooks(void)
+{
+	security_add_hooks(landlock_hooks, ARRAY_SIZE(landlock_hooks),
+			   &landlock_lsmid);
+}
diff --git a/security/landlock/socket.h b/security/landlock/socket.h
index 8519357f1c39..5c36eae9732f 100644
--- a/security/landlock/socket.h
+++ b/security/landlock/socket.h
@@ -10,6 +10,8 @@
 
 #include "ruleset.h"
 
+__init void landlock_add_socket_hooks(void);
+
 int landlock_append_socket_rule(struct landlock_ruleset *const ruleset,
 				const int family, const int type,
 				access_mask_t access_rights);
-- 
2.34.1

[RFC PATCH v2 03/12] selftests/landlock: Add protocol.create to socket tests

[Mikhail Ivanov]

Initiate socket_test.c selftests. Add protocol fixture for tests
with changeable family-type values. Only most common variants of
protocols (like ipv4-tcp,ipv6-udp, unix) were added.
Add simple socket access right checking test.

Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
---

Changes since v1:
* Replaces test_socket_create() and socket_variant() helpers
  with test_socket().
* Renames domain to family in protocol fixture.
* Remove AF_UNSPEC fixture entry and add unspec_srv0 fixture field to
  check AF_UNSPEC socket creation case.
* Formats code with clang-format.
* Refactors commit message.
---
 .../testing/selftests/landlock/socket_test.c  | 181 ++++++++++++++++++
 1 file changed, 181 insertions(+)
 create mode 100644 tools/testing/selftests/landlock/socket_test.c

diff --git a/tools/testing/selftests/landlock/socket_test.c b/tools/testing/selftests/landlock/socket_test.c
new file mode 100644
index 000000000000..4c51f89ed578
--- /dev/null
+++ b/tools/testing/selftests/landlock/socket_test.c
@@ -0,0 +1,181 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Landlock tests - Socket
+ *
+ * Copyright © 2024 Huawei Tech. Co., Ltd.
+ * Copyright © 2024 Microsoft Corporation
+ */
+
+#define _GNU_SOURCE
+
+#include <errno.h>
+#include <linux/landlock.h>
+#include <sched.h>
+#include <string.h>
+#include <sys/prctl.h>
+#include <sys/socket.h>
+
+#include "common.h"
+
+/* clang-format off */
+
+#define ACCESS_LAST LANDLOCK_ACCESS_SOCKET_CREATE
+
+#define ACCESS_ALL ( \
+	LANDLOCK_ACCESS_SOCKET_CREATE)
+
+/* clang-format on */
+
+struct protocol_variant {
+	int family;
+	int type;
+};
+
+struct service_fixture {
+	struct protocol_variant protocol;
+};
+
+static void setup_namespace(struct __test_metadata *const _metadata)
+{
+	set_cap(_metadata, CAP_SYS_ADMIN);
+	ASSERT_EQ(0, unshare(CLONE_NEWNET));
+	clear_cap(_metadata, CAP_SYS_ADMIN);
+}
+
+static int test_socket(const struct service_fixture *const srv)
+{
+	int fd;
+
+	fd = socket(srv->protocol.family, srv->protocol.type | SOCK_CLOEXEC, 0);
+	if (fd < 0)
+		return errno;
+	/*
+	 * Mixing error codes from close(2) and socket(2) should not lead to any
+	 * (access type) confusion for this test.
+	 */
+	if (close(fd) != 0)
+		return errno;
+	return 0;
+}
+
+FIXTURE(protocol)
+{
+	struct service_fixture srv0, unspec_srv0;
+};
+
+FIXTURE_VARIANT(protocol)
+{
+	const struct protocol_variant protocol;
+};
+
+FIXTURE_SETUP(protocol)
+{
+	const struct protocol_variant prot_unspec = {
+		.family = AF_UNSPEC,
+		.type = SOCK_STREAM,
+	};
+
+	disable_caps(_metadata);
+	self->srv0.protocol = variant->protocol;
+	self->unspec_srv0.protocol = prot_unspec;
+	setup_namespace(_metadata);
+};
+
+FIXTURE_TEARDOWN(protocol)
+{
+}
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(protocol, unix_stream) {
+	/* clang-format on */
+	.protocol = {
+		.family = AF_UNIX,
+		.type = SOCK_STREAM,
+	},
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(protocol, unix_dgram) {
+	/* clang-format on */
+	.protocol = {
+		.family = AF_UNIX,
+		.type = SOCK_DGRAM,
+	},
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(protocol, ipv4_tcp) {
+	/* clang-format on */
+	.protocol = {
+		.family = AF_INET,
+		.type = SOCK_STREAM,
+	},
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(protocol, ipv4_udp) {
+	/* clang-format on */
+	.protocol = {
+		.family = AF_INET,
+		.type = SOCK_DGRAM,
+	},
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(protocol, ipv6_tcp) {
+	/* clang-format on */
+	.protocol = {
+		.family = AF_INET6,
+		.type = SOCK_STREAM,
+	},
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(protocol, ipv6_udp) {
+	/* clang-format on */
+	.protocol = {
+		.family = AF_INET6,
+		.type = SOCK_DGRAM,
+	},
+};
+
+TEST_F(protocol, create)
+{
+	const struct landlock_ruleset_attr ruleset_attr = {
+		.handled_access_socket = LANDLOCK_ACCESS_SOCKET_CREATE,
+	};
+	const struct landlock_socket_attr create_socket_attr = {
+		.allowed_access = LANDLOCK_ACCESS_SOCKET_CREATE,
+		.family = self->srv0.protocol.family,
+		.type = self->srv0.protocol.type,
+	};
+
+	int ruleset_fd;
+
+	/* Allowed create */
+	ruleset_fd =
+		landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
+	ASSERT_LE(0, ruleset_fd);
+
+	ASSERT_EQ(0, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_SOCKET,
+				       &create_socket_attr, 0));
+
+	enforce_ruleset(_metadata, ruleset_fd);
+	EXPECT_EQ(0, close(ruleset_fd));
+
+	ASSERT_EQ(0, test_socket(&self->srv0));
+	ASSERT_EQ(EAFNOSUPPORT, test_socket(&self->unspec_srv0));
+
+	/* Denied create */
+	ruleset_fd =
+		landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
+	ASSERT_LE(0, ruleset_fd);
+
+	enforce_ruleset(_metadata, ruleset_fd);
+	EXPECT_EQ(0, close(ruleset_fd));
+
+	ASSERT_EQ(EACCES, test_socket(&self->srv0));
+	ASSERT_EQ(EAFNOSUPPORT, test_socket(&self->unspec_srv0));
+}
+
+TEST_HARNESS_MAIN
-- 
2.34.1

[RFC PATCH v2 04/12] selftests/landlock: Add protocol.socket_access_rights to socket tests

[Mikhail Ivanov]

Add test that checks possibility of adding rule with every possible
access right.

Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
---

Changes since v1:
* Formats code with clang-format.
* Refactors commit message.
---
 .../testing/selftests/landlock/socket_test.c  | 28 +++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/tools/testing/selftests/landlock/socket_test.c b/tools/testing/selftests/landlock/socket_test.c
index 4c51f89ed578..eb5d62263460 100644
--- a/tools/testing/selftests/landlock/socket_test.c
+++ b/tools/testing/selftests/landlock/socket_test.c
@@ -178,4 +178,32 @@ TEST_F(protocol, create)
 	ASSERT_EQ(EAFNOSUPPORT, test_socket(&self->unspec_srv0));
 }
 
+TEST_F(protocol, socket_access_rights)
+{
+	const struct landlock_ruleset_attr ruleset_attr = {
+		.handled_access_socket = ACCESS_ALL,
+	};
+	struct landlock_socket_attr protocol = {
+		.family = self->srv0.protocol.family,
+		.type = self->srv0.protocol.type,
+	};
+	int ruleset_fd;
+	__u64 access;
+
+	ruleset_fd =
+		landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
+	ASSERT_LE(0, ruleset_fd);
+
+	for (access = 1; access <= ACCESS_LAST; access <<= 1) {
+		protocol.allowed_access = access;
+		EXPECT_EQ(0, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_SOCKET,
+					       &protocol, 0))
+		{
+			TH_LOG("Failed to add rule with access 0x%llx: %s",
+			       access, strerror(errno));
+		}
+	}
+	EXPECT_EQ(0, close(ruleset_fd));
+}
+
 TEST_HARNESS_MAIN
-- 
2.34.1

[RFC PATCH v2 05/12] selftests/landlock: Add protocol.rule_with_unknown_access to socket tests

[Mikhail Ivanov]

Add test that validates behavior of landlock after rule with
unknown access is added.

Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
---

Changes since v1:
* Refactors commit messsage.
---
 .../testing/selftests/landlock/socket_test.c  | 26 +++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/tools/testing/selftests/landlock/socket_test.c b/tools/testing/selftests/landlock/socket_test.c
index eb5d62263460..57d5927906b8 100644
--- a/tools/testing/selftests/landlock/socket_test.c
+++ b/tools/testing/selftests/landlock/socket_test.c
@@ -206,4 +206,30 @@ TEST_F(protocol, socket_access_rights)
 	EXPECT_EQ(0, close(ruleset_fd));
 }
 
+TEST_F(protocol, rule_with_unknown_access)
+{
+	const struct landlock_ruleset_attr ruleset_attr = {
+		.handled_access_socket = ACCESS_ALL,
+	};
+	struct landlock_socket_attr protocol = {
+		.family = self->srv0.protocol.family,
+		.type = self->srv0.protocol.type,
+	};
+	int ruleset_fd;
+	__u64 access;
+
+	ruleset_fd =
+		landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
+	ASSERT_LE(0, ruleset_fd);
+
+	for (access = 1ULL << 63; access != ACCESS_LAST; access >>= 1) {
+		protocol.allowed_access = access;
+		EXPECT_EQ(-1,
+			  landlock_add_rule(ruleset_fd, LANDLOCK_RULE_SOCKET,
+					    &protocol, 0));
+		EXPECT_EQ(EINVAL, errno);
+	}
+	EXPECT_EQ(0, close(ruleset_fd));
+}
+
 TEST_HARNESS_MAIN
-- 
2.34.1

[RFC PATCH v2 06/12] selftests/landlock: Add protocol.rule_with_unhandled_access to socket tests

[Mikhail Ivanov]

Add test that validates behavior of landlock after rule with
unhandled access is added.

Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
---

Changes since v1:
* Refactors commit message.
---
 .../testing/selftests/landlock/socket_test.c  | 33 +++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/tools/testing/selftests/landlock/socket_test.c b/tools/testing/selftests/landlock/socket_test.c
index 57d5927906b8..31af47de1937 100644
--- a/tools/testing/selftests/landlock/socket_test.c
+++ b/tools/testing/selftests/landlock/socket_test.c
@@ -232,4 +232,37 @@ TEST_F(protocol, rule_with_unknown_access)
 	EXPECT_EQ(0, close(ruleset_fd));
 }
 
+TEST_F(protocol, rule_with_unhandled_access)
+{
+	struct landlock_ruleset_attr ruleset_attr = {
+		.handled_access_socket = LANDLOCK_ACCESS_SOCKET_CREATE,
+	};
+	struct landlock_socket_attr protocol = {
+		.family = self->srv0.protocol.family,
+		.type = self->srv0.protocol.type,
+	};
+	int ruleset_fd;
+	__u64 access;
+
+	ruleset_fd =
+		landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
+	ASSERT_LE(0, ruleset_fd);
+
+	for (access = 1; access > 0; access <<= 1) {
+		int err;
+
+		protocol.allowed_access = access;
+		err = landlock_add_rule(ruleset_fd, LANDLOCK_RULE_SOCKET,
+					&protocol, 0);
+		if (access == ruleset_attr.handled_access_socket) {
+			EXPECT_EQ(0, err);
+		} else {
+			EXPECT_EQ(-1, err);
+			EXPECT_EQ(EINVAL, errno);
+		}
+	}
+
+	EXPECT_EQ(0, close(ruleset_fd));
+}
+
 TEST_HARNESS_MAIN
-- 
2.34.1

[RFC PATCH v2 07/12] selftests/landlock: Add protocol.inval to socket tests

[Mikhail Ivanov]

Add test that validates behavior of landlock with fully
access restriction.

Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
---

Changes since v1:
* Refactors commit message.
---
 .../testing/selftests/landlock/socket_test.c  | 34 +++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/tools/testing/selftests/landlock/socket_test.c b/tools/testing/selftests/landlock/socket_test.c
index 31af47de1937..751596c381fe 100644
--- a/tools/testing/selftests/landlock/socket_test.c
+++ b/tools/testing/selftests/landlock/socket_test.c
@@ -265,4 +265,38 @@ TEST_F(protocol, rule_with_unhandled_access)
 	EXPECT_EQ(0, close(ruleset_fd));
 }
 
+TEST_F(protocol, inval)
+{
+	const struct landlock_ruleset_attr ruleset_attr = {
+		.handled_access_socket = LANDLOCK_ACCESS_SOCKET_CREATE
+	};
+
+	struct landlock_socket_attr protocol = {
+		.allowed_access = LANDLOCK_ACCESS_SOCKET_CREATE,
+		.family = self->srv0.protocol.family,
+		.type = self->srv0.protocol.type,
+	};
+
+	struct landlock_socket_attr protocol_denied = {
+		.allowed_access = 0,
+		.family = self->srv0.protocol.family,
+		.type = self->srv0.protocol.type,
+	};
+
+	int ruleset_fd;
+
+	ruleset_fd =
+		landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
+	ASSERT_LE(0, ruleset_fd);
+
+	/* Checks zero access value. */
+	EXPECT_EQ(-1, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_SOCKET,
+					&protocol_denied, 0));
+	EXPECT_EQ(ENOMSG, errno);
+
+	/* Adds with legitimate values. */
+	ASSERT_EQ(0, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_SOCKET,
+				       &protocol, 0));
+}
+
 TEST_HARNESS_MAIN
-- 
2.34.1

[RFC PATCH v2 08/12] selftests/landlock: Add tcp_layers.ruleset_overlap to socket tests

[Mikhail Ivanov]

* Add tcp_layers fixture for tests that check multiple layer
  configuration scenarios.

* Add test that validates multiple layer behavior with overlapped
  restrictions.

Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
---

Changes since v1:
* Replaces test_socket_create() with test_socket().
* Formats code with clang-format.
* Refactors commit message.
* Minor fixes.
---
 .../testing/selftests/landlock/socket_test.c  | 109 ++++++++++++++++++
 1 file changed, 109 insertions(+)

diff --git a/tools/testing/selftests/landlock/socket_test.c b/tools/testing/selftests/landlock/socket_test.c
index 751596c381fe..52edc1a8ac21 100644
--- a/tools/testing/selftests/landlock/socket_test.c
+++ b/tools/testing/selftests/landlock/socket_test.c
@@ -299,4 +299,113 @@ TEST_F(protocol, inval)
 				       &protocol, 0));
 }
 
+FIXTURE(tcp_layers)
+{
+	struct service_fixture srv0;
+};
+
+FIXTURE_VARIANT(tcp_layers)
+{
+	const size_t num_layers;
+};
+
+FIXTURE_SETUP(tcp_layers)
+{
+	const struct protocol_variant prot = {
+		.family = AF_INET,
+		.type = SOCK_STREAM,
+	};
+
+	disable_caps(_metadata);
+	self->srv0.protocol = prot;
+	setup_namespace(_metadata);
+};
+
+FIXTURE_TEARDOWN(tcp_layers)
+{
+}
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(tcp_layers, no_sandbox_with_ipv4) {
+	/* clang-format on */
+	.num_layers = 0,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(tcp_layers, one_sandbox_with_ipv4) {
+	/* clang-format on */
+	.num_layers = 1,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(tcp_layers, two_sandboxes_with_ipv4) {
+	/* clang-format on */
+	.num_layers = 2,
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(tcp_layers, three_sandboxes_with_ipv4) {
+	/* clang-format on */
+	.num_layers = 3,
+};
+
+TEST_F(tcp_layers, ruleset_overlap)
+{
+	const struct landlock_ruleset_attr ruleset_attr = {
+		.handled_access_socket = LANDLOCK_ACCESS_SOCKET_CREATE,
+	};
+	const struct landlock_socket_attr tcp_create = {
+		.allowed_access = LANDLOCK_ACCESS_SOCKET_CREATE,
+		.family = self->srv0.protocol.family,
+		.type = self->srv0.protocol.type,
+	};
+
+	if (variant->num_layers >= 1) {
+		int ruleset_fd;
+
+		ruleset_fd = landlock_create_ruleset(&ruleset_attr,
+						     sizeof(ruleset_attr), 0);
+		ASSERT_LE(0, ruleset_fd);
+
+		/* Allows create. */
+		ASSERT_EQ(0, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_SOCKET,
+					       &tcp_create, 0));
+		enforce_ruleset(_metadata, ruleset_fd);
+		EXPECT_EQ(0, close(ruleset_fd));
+	}
+
+	if (variant->num_layers >= 2) {
+		int ruleset_fd;
+
+		/* Creates another ruleset layer with denied create. */
+		ruleset_fd = landlock_create_ruleset(&ruleset_attr,
+						     sizeof(ruleset_attr), 0);
+		ASSERT_LE(0, ruleset_fd);
+
+		enforce_ruleset(_metadata, ruleset_fd);
+		EXPECT_EQ(0, close(ruleset_fd));
+	}
+
+	if (variant->num_layers >= 3) {
+		int ruleset_fd;
+
+		/* Creates another ruleset layer. */
+		ruleset_fd = landlock_create_ruleset(&ruleset_attr,
+						     sizeof(ruleset_attr), 0);
+		ASSERT_LE(0, ruleset_fd);
+
+		/* Try to allow create second time. */
+		ASSERT_EQ(0, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_SOCKET,
+					       &tcp_create, 0));
+		enforce_ruleset(_metadata, ruleset_fd);
+		EXPECT_EQ(0, close(ruleset_fd));
+	}
+
+	if (variant->num_layers < 2) {
+		ASSERT_EQ(0, test_socket(&self->srv0));
+	} else {
+		ASSERT_EQ(EACCES, test_socket(&self->srv0));
+	}
+}
+
 TEST_HARNESS_MAIN
-- 
2.34.1

[RFC PATCH v2 09/12] selftests/landlock: Add mini.ruleset_with_unknown_access to socket tests

[Mikhail Ivanov]

* Add fixture mini for tests that check specific cases.

* Add test that validates behavior of landlock after ruleset with
  unknown access is created.

Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
---

Changes since v1:
* Refactors commit message.
---
 .../testing/selftests/landlock/socket_test.c  | 31 +++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/tools/testing/selftests/landlock/socket_test.c b/tools/testing/selftests/landlock/socket_test.c
index 52edc1a8ac21..c81f02ffef6c 100644
--- a/tools/testing/selftests/landlock/socket_test.c
+++ b/tools/testing/selftests/landlock/socket_test.c
@@ -408,4 +408,35 @@ TEST_F(tcp_layers, ruleset_overlap)
 	}
 }
 
+/* clang-format off */
+FIXTURE(mini) {};
+/* clang-format on */
+
+FIXTURE_SETUP(mini)
+{
+	disable_caps(_metadata);
+
+	setup_namespace(_metadata);
+};
+
+FIXTURE_TEARDOWN(mini)
+{
+}
+
+TEST_F(mini, ruleset_with_unknown_access)
+{
+	__u64 access_mask;
+
+	for (access_mask = 1ULL << 63; access_mask != ACCESS_LAST;
+	     access_mask >>= 1) {
+		const struct landlock_ruleset_attr ruleset_attr = {
+			.handled_access_socket = access_mask,
+		};
+
+		EXPECT_EQ(-1, landlock_create_ruleset(&ruleset_attr,
+						      sizeof(ruleset_attr), 0));
+		EXPECT_EQ(EINVAL, errno);
+	}
+}
+
 TEST_HARNESS_MAIN
-- 
2.34.1

[RFC PATCH v2 10/12] selftests/landlock: Add mini.socket_overflow to socket tests

[Mikhail Ivanov]

* Add test validating that adding a rule for sockets that do not match
  the ranges (0 <= domain < AF_MAX), (0 <= type < SOCK_MAX)
  is prohibited. This test also checks that Landlock supports maximum
  possible domain, type values.

* Add CONFIG_MCTP to selftests config to check the socket with maximum
  family (AF_MCTP).

* Add CAP_NET_RAW capability to landlock selftests, which is required
  to create PACKET sockets (maximum type value).

Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
---
 tools/testing/selftests/landlock/common.h     |  1 +
 tools/testing/selftests/landlock/config       |  1 +
 .../testing/selftests/landlock/socket_test.c  | 93 +++++++++++++++++++
 3 files changed, 95 insertions(+)

diff --git a/tools/testing/selftests/landlock/common.h b/tools/testing/selftests/landlock/common.h
index 7e2b431b9f90..28df49fa22d5 100644
--- a/tools/testing/selftests/landlock/common.h
+++ b/tools/testing/selftests/landlock/common.h
@@ -66,6 +66,7 @@ static void _init_caps(struct __test_metadata *const _metadata, bool drop_all)
 		CAP_NET_BIND_SERVICE,
 		CAP_SYS_ADMIN,
 		CAP_SYS_CHROOT,
+		CAP_NET_RAW,
 		/* clang-format on */
 	};
 	const unsigned int noroot = SECBIT_NOROOT | SECBIT_NOROOT_LOCKED;
diff --git a/tools/testing/selftests/landlock/config b/tools/testing/selftests/landlock/config
index 0086efaa7b68..2820c481aefe 100644
--- a/tools/testing/selftests/landlock/config
+++ b/tools/testing/selftests/landlock/config
@@ -12,3 +12,4 @@ CONFIG_SHMEM=y
 CONFIG_SYSFS=y
 CONFIG_TMPFS=y
 CONFIG_TMPFS_XATTR=y
+CONFIG_MCTP=y
\ No newline at end of file
diff --git a/tools/testing/selftests/landlock/socket_test.c b/tools/testing/selftests/landlock/socket_test.c
index c81f02ffef6c..80c904380075 100644
--- a/tools/testing/selftests/landlock/socket_test.c
+++ b/tools/testing/selftests/landlock/socket_test.c
@@ -9,6 +9,7 @@
 #define _GNU_SOURCE
 
 #include <errno.h>
+#include <linux/net.h>
 #include <linux/landlock.h>
 #include <sched.h>
 #include <string.h>
@@ -439,4 +440,96 @@ TEST_F(mini, ruleset_with_unknown_access)
 	}
 }
 
+TEST_F(mini, socket_overflow)
+{
+	const struct landlock_ruleset_attr ruleset_attr = {
+		.handled_access_socket = LANDLOCK_ACCESS_SOCKET_CREATE,
+	};
+	/*
+	 * Assuming that AF_MCTP == AF_MAX - 1 uses MCTP as protocol
+	 * with maximum family value. Appropriate сheck for this is given below.
+	 */
+	const struct landlock_socket_attr create_socket_max_family = {
+		.allowed_access = LANDLOCK_ACCESS_SOCKET_CREATE,
+		.family = AF_MCTP,
+		.type = SOCK_DGRAM,
+	};
+	/*
+	 * Assuming that SOCK_PACKET == SOCK_MAX - 1 uses PACKET socket as
+	 * socket with maximum type value. Since SOCK_MAX cannot be accessed
+	 * from selftests, this assumption is not verified.
+	 */
+	const struct landlock_socket_attr create_socket_max_type = {
+		.allowed_access = LANDLOCK_ACCESS_SOCKET_CREATE,
+		.family = AF_PACKET,
+		.type = SOCK_PACKET,
+	};
+	struct landlock_socket_attr create_socket_overflow = {
+		.allowed_access = LANDLOCK_ACCESS_SOCKET_CREATE,
+	};
+	const struct protocol_variant protocol_max_family = {
+		.family = create_socket_max_family.family,
+		.type = create_socket_max_family.type,
+	};
+	const struct protocol_variant protocol_max_type = {
+		.family = create_socket_max_type.family,
+		.type = create_socket_max_type.type,
+	};
+	const struct protocol_variant ipv4_tcp = {
+		.family = AF_INET,
+		.type = SOCK_STREAM,
+	};
+	struct service_fixture srv_max_allowed_family, srv_max_allowed_type,
+		srv_denied;
+	int ruleset_fd;
+
+	/* Checks protocol_max_family correctness. */
+	ASSERT_EQ(AF_MCTP + 1, AF_MAX);
+
+	srv_max_allowed_family.protocol = protocol_max_family;
+	srv_max_allowed_type.protocol = protocol_max_type;
+	srv_denied.protocol = ipv4_tcp;
+
+	ruleset_fd =
+		landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
+	ASSERT_LE(0, ruleset_fd);
+
+	ASSERT_EQ(0, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_SOCKET,
+				       &create_socket_max_family, 0));
+	ASSERT_EQ(0, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_SOCKET,
+				       &create_socket_max_type, 0));
+
+	/* Checks the overflow variants for family, type values. */
+#define CHECK_RULE_OVERFLOW(family_val, type_val)                             \
+	do {                                                                  \
+		create_socket_overflow.family = family_val;                   \
+		create_socket_overflow.type = type_val;                       \
+		EXPECT_EQ(-1,                                                 \
+			  landlock_add_rule(ruleset_fd, LANDLOCK_RULE_SOCKET, \
+					    &create_socket_overflow, 0));     \
+		EXPECT_EQ(EINVAL, errno);                                     \
+	} while (0)
+
+	CHECK_RULE_OVERFLOW(AF_MAX, SOCK_STREAM);
+	CHECK_RULE_OVERFLOW(AF_INET, (SOCK_PACKET + 1));
+	CHECK_RULE_OVERFLOW(AF_MAX, (SOCK_PACKET + 1));
+	CHECK_RULE_OVERFLOW(-1, SOCK_STREAM);
+	CHECK_RULE_OVERFLOW(AF_INET, -1);
+	CHECK_RULE_OVERFLOW(-1, -1);
+	CHECK_RULE_OVERFLOW(INT16_MAX + 1, INT16_MAX + 1);
+
+#undef CHECK_RULE_OVERFLOW
+
+	enforce_ruleset(_metadata, ruleset_fd);
+
+	EXPECT_EQ(0, test_socket(&srv_max_allowed_family));
+
+	/* PACKET sockets can be used only with CAP_NET_RAW. */
+	set_cap(_metadata, CAP_NET_RAW);
+	EXPECT_EQ(0, test_socket(&srv_max_allowed_type));
+	clear_cap(_metadata, CAP_NET_RAW);
+
+	EXPECT_EQ(EACCES, test_socket(&srv_denied));
+}
+
 TEST_HARNESS_MAIN
-- 
2.34.1

[RFC PATCH v2 11/12] selftests/landlock: Add mini.socket_invalid_type to socket tests

[Mikhail Ivanov]

Add test validating that landlock doesn't wrongfully return EACCES for
socket with invalid type (e.g. UNIX socket with PACKET type).

Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
---
 .../testing/selftests/landlock/socket_test.c  | 46 +++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/tools/testing/selftests/landlock/socket_test.c b/tools/testing/selftests/landlock/socket_test.c
index 80c904380075..b062001a778e 100644
--- a/tools/testing/selftests/landlock/socket_test.c
+++ b/tools/testing/selftests/landlock/socket_test.c
@@ -532,4 +532,50 @@ TEST_F(mini, socket_overflow)
 	EXPECT_EQ(EACCES, test_socket(&srv_denied));
 }
 
+TEST_F(mini, socket_invalid_type)
+{
+	const struct landlock_ruleset_attr ruleset_attr = {
+		.handled_access_socket = LANDLOCK_ACCESS_SOCKET_CREATE,
+	};
+	/*
+	 * SOCK_PACKET is invalid type for UNIX socket
+	 * (see net/unix/af_unix.c:unix_create()).
+	 */
+	const struct landlock_socket_attr create_unix_invalid = {
+		.allowed_access = LANDLOCK_ACCESS_SOCKET_CREATE,
+		.family = AF_UNIX,
+		.type = SOCK_PACKET,
+	};
+	const struct protocol_variant protocol_invalid = {
+		.family = create_unix_invalid.family,
+		.type = create_unix_invalid.type,
+	};
+	struct service_fixture srv_unix_invalid;
+	int ruleset_fd;
+
+	srv_unix_invalid.protocol = protocol_invalid;
+
+	/* Allowed created */
+	ruleset_fd =
+		landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
+	ASSERT_LE(0, ruleset_fd);
+
+	ASSERT_EQ(0, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_SOCKET,
+				       &create_unix_invalid, 0));
+	enforce_ruleset(_metadata, ruleset_fd);
+	EXPECT_EQ(0, close(ruleset_fd));
+
+	EXPECT_EQ(ESOCKTNOSUPPORT, test_socket(&srv_unix_invalid));
+
+	/* Denied create */
+	ruleset_fd =
+		landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
+	ASSERT_LE(0, ruleset_fd);
+
+	enforce_ruleset(_metadata, ruleset_fd);
+	EXPECT_EQ(0, close(ruleset_fd));
+
+	EXPECT_EQ(ESOCKTNOSUPPORT, test_socket(&srv_unix_invalid));
+}
+
 TEST_HARNESS_MAIN
-- 
2.34.1

[RFC PATCH v2 12/12] samples/landlock: Support socket protocol restrictions

[Mikhail Ivanov]

* Add socket protocol control support in sandboxer demo. It's possible
  to allow a sandboxer to create sockets with specified family and type
  values. This is controlled with the new LL_SOCKET_CREATE environment
  variable. Single token in this variable looks like this:
  'FAMILY.TYPE', where FAMILY corresponds to one of the possible socket
  family name and TYPE to the possible socket type name (see socket(2)).
  Add ENV_TOKEN_INTERNAL_DELIMITER.

* Add parse_socket_protocol() method to parse socket family and type
  strings to the appropriate constants. Add CHECK_FAMILY() and
  CHECK_TYPE() macroses to prevent copypaste in method.

* Change LANDLOCK_ABI_LAST to 6.

Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
---

Changes since v1:
* Refactors get_socket_protocol(). Rename it to parse_socket_protocol().
* Changes LANDLOCK_ABI_LAST to 6 since ioctl patchlist updated it to 5.
* Refactors commit message.
* Formats with clang-format.
* Minor changes.
---
 samples/landlock/sandboxer.c | 141 +++++++++++++++++++++++++++++++----
 1 file changed, 127 insertions(+), 14 deletions(-)

diff --git a/samples/landlock/sandboxer.c b/samples/landlock/sandboxer.c
index e8223c3e781a..b6afe011e563 100644
--- a/samples/landlock/sandboxer.c
+++ b/samples/landlock/sandboxer.c
@@ -14,6 +14,7 @@
 #include <fcntl.h>
 #include <linux/landlock.h>
 #include <linux/prctl.h>
+#include <linux/socket.h>
 #include <stddef.h>
 #include <stdio.h>
 #include <stdlib.h>
@@ -55,8 +56,11 @@ static inline int landlock_restrict_self(const int ruleset_fd,
 #define ENV_FS_RW_NAME "LL_FS_RW"
 #define ENV_TCP_BIND_NAME "LL_TCP_BIND"
 #define ENV_TCP_CONNECT_NAME "LL_TCP_CONNECT"
+#define ENV_SOCKET_CREATE_NAME "LL_SOCKET_CREATE"
 #define ENV_DELIMITER ":"
 
+#define ENV_TOKEN_INTERNAL_DELIMITER "."
+
 static int parse_path(char *env_path, const char ***const path_list)
 {
 	int i, num_paths = 0;
@@ -86,6 +90,42 @@ static int parse_path(char *env_path, const char ***const path_list)
 
 /* clang-format on */
 
+static int parse_socket_protocol(char *strfamily, char *strtype,
+				 struct landlock_socket_attr *protocol)
+{
+	protocol->family = -1;
+	protocol->type = -1;
+
+#define CHECK_FAMILY(family_variant)                           \
+	do {                                                   \
+		if (strcmp(strfamily, #family_variant) == 0) { \
+			protocol->family = family_variant;     \
+		}                                              \
+	} while (0)
+
+#define CHECK_TYPE(type_variant)                           \
+	do {                                               \
+		if (strcmp(strtype, #type_variant) == 0) { \
+			protocol->type = type_variant;     \
+		}                                          \
+	} while (0)
+
+	CHECK_FAMILY(AF_UNIX);
+	CHECK_FAMILY(AF_INET);
+	CHECK_FAMILY(AF_INET6);
+
+	CHECK_TYPE(SOCK_STREAM);
+	CHECK_TYPE(SOCK_DGRAM);
+
+#undef CHECK_FAMILY
+#undef CHECK_TYPE
+
+	/* Unknown protocol or type. */
+	if (protocol->family == -1 || protocol->type == -1)
+		return 1;
+	return 0;
+}
+
 static int populate_ruleset_fs(const char *const env_var, const int ruleset_fd,
 			       const __u64 allowed_access)
 {
@@ -184,6 +224,61 @@ static int populate_ruleset_net(const char *const env_var, const int ruleset_fd,
 	return ret;
 }
 
+static int populate_ruleset_socket(const char *const env_var,
+				   const int ruleset_fd,
+				   const __u64 allowed_access)
+{
+	int ret = 1;
+	char *env_protocol_name, *strprotocol, *strfamily, *strtype;
+	struct landlock_socket_attr protocol = {
+		.allowed_access = allowed_access,
+		.family = 0,
+		.type = 0,
+	};
+
+	env_protocol_name = getenv(env_var);
+	if (!env_protocol_name)
+		return 0;
+	env_protocol_name = strdup(env_protocol_name);
+	unsetenv(env_var);
+
+	while ((strprotocol = strsep(&env_protocol_name, ENV_DELIMITER))) {
+		strfamily = strsep(&strprotocol, ENV_TOKEN_INTERNAL_DELIMITER);
+		strtype = strsep(&strprotocol, ENV_TOKEN_INTERNAL_DELIMITER);
+
+		if (!strtype) {
+			fprintf(stderr,
+				"Failed to extract socket protocol with "
+				"unspecified type value\n");
+			goto out_free_name;
+		}
+
+		if (parse_socket_protocol(strfamily, strtype, &protocol)) {
+			fprintf(stderr,
+				"Failed to extract socket protocol with "
+				"domain: \"%s\", type: \"%s\"\n"
+				"Sandboxer currently supports AF_UNIX, AF_INET, AF_INET6 "
+				"families and SOCK_STREAM, SOCK_DGRAM types\n",
+				strfamily, strtype);
+			goto out_free_name;
+		}
+
+		if (landlock_add_rule(ruleset_fd, LANDLOCK_RULE_SOCKET,
+				      &protocol, 0)) {
+			fprintf(stderr,
+				"Failed to update the ruleset with "
+				"family \"%s\" and type \"%s\": %s\n",
+				strfamily, strtype, strerror(errno));
+			goto out_free_name;
+		}
+	}
+	ret = 0;
+
+out_free_name:
+	free(env_protocol_name);
+	return ret;
+}
+
 /* clang-format off */
 
 #define ACCESS_FS_ROUGHLY_READ ( \
@@ -208,14 +303,14 @@ static int populate_ruleset_net(const char *const env_var, const int ruleset_fd,
 
 /* clang-format on */
 
-#define LANDLOCK_ABI_LAST 5
+#define LANDLOCK_ABI_LAST 6
 
 int main(const int argc, char *const argv[], char *const *const envp)
 {
 	const char *cmd_path;
 	char *const *cmd_argv;
 	int ruleset_fd, abi;
-	char *env_port_name;
+	char *env_optional_name;
 	__u64 access_fs_ro = ACCESS_FS_ROUGHLY_READ,
 	      access_fs_rw = ACCESS_FS_ROUGHLY_READ | ACCESS_FS_ROUGHLY_WRITE;
 
@@ -223,18 +318,19 @@ int main(const int argc, char *const argv[], char *const *const envp)
 		.handled_access_fs = access_fs_rw,
 		.handled_access_net = LANDLOCK_ACCESS_NET_BIND_TCP |
 				      LANDLOCK_ACCESS_NET_CONNECT_TCP,
+		.handled_access_socket = LANDLOCK_ACCESS_SOCKET_CREATE,
 	};
 
 	if (argc < 2) {
 		fprintf(stderr,
-			"usage: %s=\"...\" %s=\"...\" %s=\"...\" %s=\"...\"%s "
+			"usage: %s=\"...\" %s=\"...\" %s=\"...\" %s=\"...\" %s=\"...\"%s "
 			"<cmd> [args]...\n\n",
 			ENV_FS_RO_NAME, ENV_FS_RW_NAME, ENV_TCP_BIND_NAME,
-			ENV_TCP_CONNECT_NAME, argv[0]);
+			ENV_TCP_CONNECT_NAME, ENV_SOCKET_CREATE_NAME, argv[0]);
 		fprintf(stderr,
 			"Execute a command in a restricted environment.\n\n");
 		fprintf(stderr,
-			"Environment variables containing paths and ports "
+			"Environment variables containing paths, ports and protocols "
 			"each separated by a colon:\n");
 		fprintf(stderr,
 			"* %s: list of paths allowed to be used in a read-only way.\n",
@@ -243,7 +339,7 @@ int main(const int argc, char *const argv[], char *const *const envp)
 			"* %s: list of paths allowed to be used in a read-write way.\n\n",
 			ENV_FS_RW_NAME);
 		fprintf(stderr,
-			"Environment variables containing ports are optional "
+			"Environment variables containing ports or protocols are optional "
 			"and could be skipped.\n");
 		fprintf(stderr,
 			"* %s: list of ports allowed to bind (server).\n",
@@ -251,22 +347,25 @@ int main(const int argc, char *const argv[], char *const *const envp)
 		fprintf(stderr,
 			"* %s: list of ports allowed to connect (client).\n",
 			ENV_TCP_CONNECT_NAME);
+		fprintf(stderr,
+			"* %s: list of socket protocols allowed to be created.\n",
+			ENV_SOCKET_CREATE_NAME);
 		fprintf(stderr,
 			"\nexample:\n"
 			"%s=\"${PATH}:/lib:/usr:/proc:/etc:/dev/urandom\" "
 			"%s=\"/dev/null:/dev/full:/dev/zero:/dev/pts:/tmp\" "
 			"%s=\"9418\" "
 			"%s=\"80:443\" "
+			"%s=\"AF_INET6.SOCK_DGRAM:AF_UNIX.SOCK_STREAM\" "
 			"%s bash -i\n\n",
 			ENV_FS_RO_NAME, ENV_FS_RW_NAME, ENV_TCP_BIND_NAME,
-			ENV_TCP_CONNECT_NAME, argv[0]);
+			ENV_TCP_CONNECT_NAME, ENV_SOCKET_CREATE_NAME, argv[0]);
 		fprintf(stderr,
 			"This sandboxer can use Landlock features "
 			"up to ABI version %d.\n",
 			LANDLOCK_ABI_LAST);
 		return 1;
 	}
-
 	abi = landlock_create_ruleset(NULL, 0, LANDLOCK_CREATE_RULESET_VERSION);
 	if (abi < 0) {
 		const int err = errno;
@@ -326,7 +425,11 @@ int main(const int argc, char *const argv[], char *const *const envp)
 	case 4:
 		/* Removes LANDLOCK_ACCESS_FS_IOCTL_DEV for ABI < 5 */
 		ruleset_attr.handled_access_fs &= ~LANDLOCK_ACCESS_FS_IOCTL_DEV;
-
+		__attribute__((fallthrough));
+	case 5:
+		/* Removes socket support for ABI < 6 */
+		ruleset_attr.handled_access_socket &=
+			~LANDLOCK_ACCESS_SOCKET_CREATE;
 		fprintf(stderr,
 			"Hint: You should update the running kernel "
 			"to leverage Landlock features "
@@ -346,18 +449,23 @@ int main(const int argc, char *const argv[], char *const *const envp)
 	access_fs_rw &= ruleset_attr.handled_access_fs;
 
 	/* Removes bind access attribute if not supported by a user. */
-	env_port_name = getenv(ENV_TCP_BIND_NAME);
-	if (!env_port_name) {
+	env_optional_name = getenv(ENV_TCP_BIND_NAME);
+	if (!env_optional_name) {
 		ruleset_attr.handled_access_net &=
 			~LANDLOCK_ACCESS_NET_BIND_TCP;
 	}
 	/* Removes connect access attribute if not supported by a user. */
-	env_port_name = getenv(ENV_TCP_CONNECT_NAME);
-	if (!env_port_name) {
+	env_optional_name = getenv(ENV_TCP_CONNECT_NAME);
+	if (!env_optional_name) {
 		ruleset_attr.handled_access_net &=
 			~LANDLOCK_ACCESS_NET_CONNECT_TCP;
 	}
-
+	/* Removes socket create access attribute if not supported by a user. */
+	env_optional_name = getenv(ENV_SOCKET_CREATE_NAME);
+	if (!env_optional_name) {
+		ruleset_attr.handled_access_socket &=
+			~LANDLOCK_ACCESS_SOCKET_CREATE;
+	}
 	ruleset_fd =
 		landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
 	if (ruleset_fd < 0) {
@@ -381,6 +489,11 @@ int main(const int argc, char *const argv[], char *const *const envp)
 		goto err_close_ruleset;
 	}
 
+	if (populate_ruleset_socket(ENV_SOCKET_CREATE_NAME, ruleset_fd,
+				    LANDLOCK_ACCESS_SOCKET_CREATE)) {
+		goto err_close_ruleset;
+	}
+
 	if (prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0)) {
 		perror("Failed to restrict privileges");
 		goto err_close_ruleset;
-- 
2.34.1

Re: [RFC PATCH v2 02/12] landlock: Add hook on socket creation

[Günther Noack]

Hello Mikhail!

Thanks for sending another revision of this patch set!

On Fri, May 24, 2024 at 05:30:05PM +0800, Mikhail Ivanov wrote:
> Add hook to security_socket_post_create(), which checks whether the socket
> type and family are allowed by domain. Hook is called after initializing
> the socket in the network stack to not wrongfully return EACCES for a
> family-type pair, which is considered invalid by the protocol.
> 
> Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>

## Some observations that *do not* need to be addressed in this commit, IMHO:

get_raw_handled_socket_accesses, get_current_socket_domain and
current_check_access_socket are based on the similarly-named functions from
net.c (and fs.c), and it makes sense to stay consistent with these.

There are some possible refactorings that could maybe be applied to that code,
but given that the same ones would apply to net.c as well, it's probably best to
address these separately.

  * Should get_raw_handled_socket_accesses be inlined?
  * Does the WARN_ON_ONCE(dom->num_layers < 1) check have the right return code?
  * Can we refactor out commonalities (probably not worth it right now though)?


## The only actionable feedback that I have that is specific to this commit is:

In the past, we have introduced new (non-test) Landlock functionality in a
single commit -- that way, we have no "loose ends" in the code between these two
commits, and that simplifies it for people who want to patch your feature onto
other kernel trees.  (e.g. I think we should maybe merge commit 01/12 and 02/12
into a single commit.)  WDYT?

—Günther

Re: [RFC PATCH v2 01/12] landlock: Support socket access-control

[Günther Noack]

On Fri, May 24, 2024 at 05:30:04PM +0800, Mikhail Ivanov wrote:
> * Add new landlock rule type that corresponds to the restriction of
>   socket protocols. This is represented as an landlock_socket_attr
>   structure. Protocol allowed by landlock must be described by
>   a family-type pair (see socket(2)).
> 
> * Support socket rule storage in landlock ruleset.
> 
> * Add flag LANDLOCK_ACCESS_SOCKET_CREATE that will provide the
>   ability to control socket creation.
> 
> * Add socket.c file that will contain socket rules management and hooks.
>   Implement helper pack_socket_key() to convert 32-bit family and type
>   values into uintptr_t. This is possible due to the fact that these
>   values are limited to AF_MAX (=46), SOCK_MAX (=11) constants. Assumption
>   is checked in build-time by the helper.
> 
> * Support socket rules in landlock syscalls. Change ABI version to 6.
> 
> Closes: https://github.com/landlock-lsm/linux/issues/6
> Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
> ---
> 
> Changes since v1:
> * Reverts landlock_key.data type from u64 to uinptr_t.
> * Adds helper to pack domain and type values into uintptr_t.
> * Denies inserting socket rule with invalid family and type.
> * Renames 'domain' to 'family' in landlock_socket_attr.
> * Updates ABI version to 6 since ioctl patches changed it to 5.
> * Formats code with clang-format.
> * Minor fixes.
> ---
>  include/uapi/linux/landlock.h                | 53 +++++++++++++++-
>  security/landlock/Makefile                   |  2 +-
>  security/landlock/limits.h                   |  5 ++
>  security/landlock/ruleset.c                  | 37 ++++++++++-
>  security/landlock/ruleset.h                  | 41 +++++++++++-
>  security/landlock/socket.c                   | 60 ++++++++++++++++++
>  security/landlock/socket.h                   | 17 +++++
>  security/landlock/syscalls.c                 | 66 ++++++++++++++++++--
>  tools/testing/selftests/landlock/base_test.c |  2 +-
>  9 files changed, 272 insertions(+), 11 deletions(-)
>  create mode 100644 security/landlock/socket.c
>  create mode 100644 security/landlock/socket.h
> 
> diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
> index 68625e728f43..a25ba5983dfb 100644
> --- a/include/uapi/linux/landlock.h
> +++ b/include/uapi/linux/landlock.h
> @@ -37,6 +37,13 @@ struct landlock_ruleset_attr {
>  	 * rule explicitly allow them.
>  	 */
>  	__u64 handled_access_net;
> +
> +	/**
> +	 * @handled_access_socket: Bitmask of actions (cf. `Socket flags`_)
> +	 * that is handled by this ruleset and should then be forbidden if no
> +	 * rule explicitly allow them.
> +	 */
> +	__u64 handled_access_socket;
>  };
>  
>  /*
> @@ -65,6 +72,11 @@ enum landlock_rule_type {
>  	 * landlock_net_port_attr .
>  	 */
>  	LANDLOCK_RULE_NET_PORT,
> +	/**
> +	 * @LANDLOCK_RULE_SOCKET: Type of a &struct
> +	 * landlock_socket_attr .
> +	 */
> +	LANDLOCK_RULE_SOCKET,
>  };
>  
>  /**
> @@ -115,6 +127,28 @@ struct landlock_net_port_attr {
>  	__u64 port;
>  };
>  
> +/**
> + * struct landlock_socket_attr - Socket definition
> + *
> + * Argument of sys_landlock_add_rule().
> + */
> +struct landlock_socket_attr {
> +	/**
> +	 * @allowed_access: Bitmask of allowed access for a socket
> +	 * (cf. `Socket flags`_).
> +	 */
> +	__u64 allowed_access;
> +	/**
> +	 * @family: Protocol family used for communication
> +	 * (same as domain in socket(2)).
> +	 */
> +	int family;
> +	/**
> +	 * @type: Socket type (see socket(2)).
> +	 */
> +	int type;
> +};

Regarding the naming of struct landlock_socket_attr and the associated
LANDLOCK_RULE_SOCKET enum:

For the two existing rule types LANDLOCK_RULE_PATH_BENEATH (struct
landlock_path_beneath_attr) and LANDLOCK_RULE_NET_PORT (struct
landlock_net_port_attr), the names of the rule types are describing the
*properties* by which we are filtering (path *beneath*, *network port*), rather
than just the kind of object that we are filtering on.

Should the new enum and struct maybe be called differently as well to match that
convention?  Maybe LANDLOCK_RULE_SOCKET_FAMILY_TYPE and struct
landlock_socket_family_type_attr?

Are there *other* properties apart from family and type, by which you are
thinking of restricting the use of sockets in the future?


> @@ -266,4 +300,21 @@ struct landlock_net_port_attr {
>  #define LANDLOCK_ACCESS_NET_BIND_TCP			(1ULL << 0)
>  #define LANDLOCK_ACCESS_NET_CONNECT_TCP			(1ULL << 1)
>  /* clang-format on */
> +
> +/**
> + * DOC: socket_access
> + *
> + * Socket flags
> + * ~~~~~~~~~~~~
> + *
> + * These flags enable to restrict a sanboxed process to a set of socket
> + * protocols. This is supported since the Landlock ABI version 6.

(Some phrasing remarks)

  * typo in "sanboxed"
  * Optional grammar nit: you can drop the "the" in front of "Landlock ABI
    version 6" (or alternatively use the phrasing as it was used in the FS
    restriction docs)
  * Grammar nit: The use of "enable to" sounds weird in my ears (but I am not a
    native speaker either).  I think it could just be dropped here ("These flags
    restrict a sandboxed process..." or "These flags control the use of...").
    I realize that the wording was used in other places already, so it's just an
    optional remark.

(More about the content)

The Landlock documentation states the general approach up front:

  A Landlock rule describes an *action* on an *object* which the process intends
  to perform.

(In your case, the object is a socket, and the action is the socket's creation.
The Landlock rules describe predicates on objects to restrict the set of actions
through the access_mask_t.)

The implementation is perfectly in line with that, but it would help to phrase
the documentation also in terms of that framework.  That means, what we are
restricting are *actions*, not protocols.

To make a more constructive suggestion:

  "These flags restrict actions on sockets for a sandboxed process (e.g. socket
  creation)."

Does it also need the following addition?

  "Sockets opened before sandboxing are not subject to these restrictions."


> + *
> + * The following access rights apply only to sockets:
                    ^^^^^^^^^^^^^^^^^^^
Probably better to use singular for now: "access right applies".

> + *
> + * - %LANDLOCK_ACCESS_SOCKET_CREATE: Create a socket.

Can we be more specific here what operations are affected by this?  It is rather
obvious that this affects socket(2), but does this also affect accept(2) and
connect(2)?

A scenario that I could imagine being useful is to sandbox a TCP server like
this:

  * create a socket, bind(2) and listen(2)
  * sandbox yourself so that no new sockets can be created with socket(2)
  * go into the main loop and start accept(2)ing new connections

Is this an approach that would work with this patch set?

(It might make a neat sample tool as well, if something like this works :))


Regarding the list of socket access rights with only one item in it:

I am still unsure what other socket actions are in scope in the future; it would
probably help to phrase the documentation in those terms.  (listen(2), bind(2),
connect(2), shutdown(2)?  On the other hand, bind(2) and connect(2) for TCP are
already restrictable differently.))

> + */
> +/* clang-format off */
> +#define LANDLOCK_ACCESS_SOCKET_CREATE			(1ULL << 0)
> +/* clang-format on */
>  #endif /* _UAPI_LINUX_LANDLOCK_H */
> diff --git a/security/landlock/Makefile b/security/landlock/Makefile
> index b4538b7cf7d2..ff1dd98f6a1b 100644
> --- a/security/landlock/Makefile
> +++ b/security/landlock/Makefile
> @@ -1,6 +1,6 @@
>  obj-$(CONFIG_SECURITY_LANDLOCK) := landlock.o
>  
>  landlock-y := setup.o syscalls.o object.o ruleset.o \
> -	cred.o task.o fs.o
> +	cred.o task.o fs.o socket.o
>  
>  landlock-$(CONFIG_INET) += net.o
> diff --git a/security/landlock/limits.h b/security/landlock/limits.h
> index 20fdb5ff3514..448b4d596783 100644
> --- a/security/landlock/limits.h
> +++ b/security/landlock/limits.h
> @@ -28,6 +28,11 @@
>  #define LANDLOCK_NUM_ACCESS_NET		__const_hweight64(LANDLOCK_MASK_ACCESS_NET)
>  #define LANDLOCK_SHIFT_ACCESS_NET	LANDLOCK_NUM_ACCESS_FS
>  
> +#define LANDLOCK_LAST_ACCESS_SOCKET	    LANDLOCK_ACCESS_SOCKET_CREATE
> +#define LANDLOCK_MASK_ACCESS_SOCKET	    ((LANDLOCK_LAST_ACCESS_SOCKET << 1) - 1)
> +#define LANDLOCK_NUM_ACCESS_SOCKET		__const_hweight64(LANDLOCK_MASK_ACCESS_SOCKET)
> +#define LANDLOCK_SHIFT_ACCESS_SOCKET	LANDLOCK_NUM_ACCESS_SOCKET
> +
>  /* clang-format on */
>  
>  #endif /* _SECURITY_LANDLOCK_LIMITS_H */
> diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
> index e0a5fbf9201a..c782f7cd313d 100644
> --- a/security/landlock/ruleset.c
> +++ b/security/landlock/ruleset.c
> @@ -40,6 +40,7 @@ static struct landlock_ruleset *create_ruleset(const u32 num_layers)
>  #if IS_ENABLED(CONFIG_INET)
>  	new_ruleset->root_net_port = RB_ROOT;
>  #endif /* IS_ENABLED(CONFIG_INET) */
> +	new_ruleset->root_socket = RB_ROOT;
>  
>  	new_ruleset->num_layers = num_layers;
>  	/*
> @@ -52,12 +53,13 @@ static struct landlock_ruleset *create_ruleset(const u32 num_layers)
>  
>  struct landlock_ruleset *
>  landlock_create_ruleset(const access_mask_t fs_access_mask,
> -			const access_mask_t net_access_mask)
> +			const access_mask_t net_access_mask,
> +			const access_mask_t socket_access_mask)
>  {
>  	struct landlock_ruleset *new_ruleset;
>  
>  	/* Informs about useless ruleset. */
> -	if (!fs_access_mask && !net_access_mask)
> +	if (!fs_access_mask && !net_access_mask && !socket_access_mask)
>  		return ERR_PTR(-ENOMSG);
>  	new_ruleset = create_ruleset(1);
>  	if (IS_ERR(new_ruleset))
> @@ -66,6 +68,9 @@ landlock_create_ruleset(const access_mask_t fs_access_mask,
>  		landlock_add_fs_access_mask(new_ruleset, fs_access_mask, 0);
>  	if (net_access_mask)
>  		landlock_add_net_access_mask(new_ruleset, net_access_mask, 0);
> +	if (socket_access_mask)
> +		landlock_add_socket_access_mask(new_ruleset, socket_access_mask,
> +						0);
>  	return new_ruleset;
>  }
>  
> @@ -89,6 +94,9 @@ static bool is_object_pointer(const enum landlock_key_type key_type)
>  		return false;
>  #endif /* IS_ENABLED(CONFIG_INET) */
>  
> +	case LANDLOCK_KEY_SOCKET:
> +		return false;
> +
>  	default:
>  		WARN_ON_ONCE(1);
>  		return false;
> @@ -146,6 +154,9 @@ static struct rb_root *get_root(struct landlock_ruleset *const ruleset,
>  		return &ruleset->root_net_port;
>  #endif /* IS_ENABLED(CONFIG_INET) */
>  
> +	case LANDLOCK_KEY_SOCKET:
> +		return &ruleset->root_socket;
> +
>  	default:
>  		WARN_ON_ONCE(1);
>  		return ERR_PTR(-EINVAL);
> @@ -175,7 +186,9 @@ static void build_check_ruleset(void)
>  	BUILD_BUG_ON(ruleset.num_layers < LANDLOCK_MAX_NUM_LAYERS);
>  	BUILD_BUG_ON(access_masks <
>  		     ((LANDLOCK_MASK_ACCESS_FS << LANDLOCK_SHIFT_ACCESS_FS) |
> -		      (LANDLOCK_MASK_ACCESS_NET << LANDLOCK_SHIFT_ACCESS_NET)));
> +		      (LANDLOCK_MASK_ACCESS_NET << LANDLOCK_SHIFT_ACCESS_NET) |
> +		      (LANDLOCK_MASK_ACCESS_SOCKET
> +		       << LANDLOCK_SHIFT_ACCESS_SOCKET)));
>  }
>  
>  /**
> @@ -399,6 +412,11 @@ static int merge_ruleset(struct landlock_ruleset *const dst,
>  		goto out_unlock;
>  #endif /* IS_ENABLED(CONFIG_INET) */
>  
> +	/* Merges the @src socket tree. */
> +	err = merge_tree(dst, src, LANDLOCK_KEY_SOCKET);
> +	if (err)
> +		goto out_unlock;
> +
>  out_unlock:
>  	mutex_unlock(&src->lock);
>  	mutex_unlock(&dst->lock);
> @@ -462,6 +480,11 @@ static int inherit_ruleset(struct landlock_ruleset *const parent,
>  		goto out_unlock;
>  #endif /* IS_ENABLED(CONFIG_INET) */
>  
> +	/* Copies the @parent socket tree. */
> +	err = inherit_tree(parent, child, LANDLOCK_KEY_SOCKET);
> +	if (err)
> +		goto out_unlock;
> +
>  	if (WARN_ON_ONCE(child->num_layers <= parent->num_layers)) {
>  		err = -EINVAL;
>  		goto out_unlock;
> @@ -498,6 +521,10 @@ static void free_ruleset(struct landlock_ruleset *const ruleset)
>  		free_rule(freeme, LANDLOCK_KEY_NET_PORT);
>  #endif /* IS_ENABLED(CONFIG_INET) */
>  
> +	rbtree_postorder_for_each_entry_safe(freeme, next,
> +					     &ruleset->root_socket, node)
> +		free_rule(freeme, LANDLOCK_KEY_SOCKET);
> +
>  	put_hierarchy(ruleset->hierarchy);
>  	kfree(ruleset);
>  }
> @@ -708,6 +735,10 @@ landlock_init_layer_masks(const struct landlock_ruleset *const domain,
>  		break;
>  #endif /* IS_ENABLED(CONFIG_INET) */
>  
> +	case LANDLOCK_KEY_SOCKET:
> +		get_access_mask = landlock_get_socket_access_mask;
> +		num_access = LANDLOCK_NUM_ACCESS_SOCKET;
> +		break;
>  	default:
>  		WARN_ON_ONCE(1);
>  		return 0;
> diff --git a/security/landlock/ruleset.h b/security/landlock/ruleset.h
> index c7f1526784fd..a9773efd529b 100644
> --- a/security/landlock/ruleset.h
> +++ b/security/landlock/ruleset.h
> @@ -92,6 +92,12 @@ enum landlock_key_type {
>  	 * node keys.
>  	 */
>  	LANDLOCK_KEY_NET_PORT,
> +
> +	/**
> +	 * @LANDLOCK_KEY_SOCKET: Type of &landlock_ruleset.root_socket's
> +	 * node keys.
> +	 */
> +	LANDLOCK_KEY_SOCKET,
>  };
>  
>  /**
> @@ -177,6 +183,15 @@ struct landlock_ruleset {
>  	struct rb_root root_net_port;
>  #endif /* IS_ENABLED(CONFIG_INET) */
>  
> +	/**
> +	 * @root_socket: Root of a red-black tree containing &struct
> +	 * landlock_rule nodes with socket type, described by (family, type)
> +	 * pair (see socket(2)). Once a ruleset is tied to a
> +	 * process (i.e. as a domain), this tree is immutable until @usage
> +	 * reaches zero.
> +	 */
> +	struct rb_root root_socket;
> +
>  	/**
>  	 * @hierarchy: Enables hierarchy identification even when a parent
>  	 * domain vanishes.  This is needed for the ptrace protection.
> @@ -233,7 +248,8 @@ struct landlock_ruleset {
>  
>  struct landlock_ruleset *
>  landlock_create_ruleset(const access_mask_t access_mask_fs,
> -			const access_mask_t access_mask_net);
> +			const access_mask_t access_mask_net,
> +			const access_mask_t access_mask_socket);
>  
>  void landlock_put_ruleset(struct landlock_ruleset *const ruleset);
>  void landlock_put_ruleset_deferred(struct landlock_ruleset *const ruleset);
> @@ -282,6 +298,20 @@ landlock_add_net_access_mask(struct landlock_ruleset *const ruleset,
>  		(net_mask << LANDLOCK_SHIFT_ACCESS_NET);
>  }
>  
> +static inline void
> +landlock_add_socket_access_mask(struct landlock_ruleset *const ruleset,
> +				const access_mask_t socket_access_mask,
> +				const u16 layer_level)
> +{
> +	access_mask_t socket_mask = socket_access_mask &
> +				    LANDLOCK_MASK_ACCESS_SOCKET;
> +
> +	/* Should already be checked in sys_landlock_create_ruleset(). */
> +	WARN_ON_ONCE(socket_access_mask != socket_mask);
> +	ruleset->access_masks[layer_level] |=
> +		(socket_mask << LANDLOCK_SHIFT_ACCESS_SOCKET);
> +}
> +
>  static inline access_mask_t
>  landlock_get_raw_fs_access_mask(const struct landlock_ruleset *const ruleset,
>  				const u16 layer_level)
> @@ -309,6 +339,15 @@ landlock_get_net_access_mask(const struct landlock_ruleset *const ruleset,
>  	       LANDLOCK_MASK_ACCESS_NET;
>  }
>  
> +static inline access_mask_t
> +landlock_get_socket_access_mask(const struct landlock_ruleset *const ruleset,
> +				const u16 layer_level)
> +{
> +	return (ruleset->access_masks[layer_level] >>
> +		LANDLOCK_SHIFT_ACCESS_SOCKET) &
> +	       LANDLOCK_MASK_ACCESS_SOCKET;
> +}
> +
>  bool landlock_unmask_layers(const struct landlock_rule *const rule,
>  			    const access_mask_t access_request,
>  			    layer_mask_t (*const layer_masks)[],
> diff --git a/security/landlock/socket.c b/security/landlock/socket.c
> new file mode 100644
> index 000000000000..1249a4a36503
> --- /dev/null
> +++ b/security/landlock/socket.c
> @@ -0,0 +1,60 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Landlock LSM - Socket management and hooks
> + *
> + * Copyright © 2024 Huawei Tech. Co., Ltd.
> + */
> +
> +#include <linux/net.h>
> +#include <linux/socket.h>
> +#include <linux/stddef.h>
> +
> +#include "limits.h"
> +#include "ruleset.h"
> +#include "socket.h"
> +
> +static uintptr_t pack_socket_key(const int family, const int type)
> +{
> +	union {
> +		struct {
> +			unsigned short family, type;
> +		} __packed data;
> +		uintptr_t packed;
> +	} socket_key;
> +
> +	/* Checks that all supported socket families and types can be stored in socket_key. */
> +	BUILD_BUG_ON(AF_MAX > (typeof(socket_key.data.family))~0);
> +	BUILD_BUG_ON(SOCK_MAX > (typeof(socket_key.data.type))~0);

Off-by-one nit: AF_MAX and SOCK_MAX are one higher than the last permitted value,
so technically it would be ok if they are one higher than (unsigned short)~0.

> +
> +	/* Checks that socket_key can be stored in landlock_key. */
> +	BUILD_BUG_ON(sizeof(socket_key.data) > sizeof(socket_key.packed));
> +	BUILD_BUG_ON(sizeof(socket_key.packed) >
> +		     sizeof_field(union landlock_key, data));
> +
> +	socket_key.data.family = (unsigned short)family;
> +	socket_key.data.type = (unsigned short)type;
> +
> +	return socket_key.packed;

Can socket_key.packed end up containing uninitialized memory here?

> +}

I see that this function traces back to Mickaël's comment in
https://lore.kernel.org/all/20240412.phoh7laim7Th@digikod.net/

In my understanding, the motivation was to keep the key size in check.
But that does not mean that we need to turn it into a uintptr_t?

Would it not have been possible to extend the union landlock_key in ruleset.h
with a

  struct {
    unsigned short family, type;
  }

and then do the AF_MAX, SOCK_MAX build-time checks on that?
It seems like that might be more in line with what we already have?

> +
> +int landlock_append_socket_rule(struct landlock_ruleset *const ruleset,
> +				const int family, const int type,
> +				access_mask_t access_rights)
> +{
> +	int err;
> +
> +	const struct landlock_id id = {
> +		.key.data = pack_socket_key(family, type),
> +		.type = LANDLOCK_KEY_SOCKET,
> +	};
> +
> +	/* Transforms relative access rights to absolute ones. */
> +	access_rights |= LANDLOCK_MASK_ACCESS_SOCKET &
> +			 ~landlock_get_socket_access_mask(ruleset, 0);
> +
> +	mutex_lock(&ruleset->lock);
> +	err = landlock_insert_rule(ruleset, id, access_rights);
> +	mutex_unlock(&ruleset->lock);
> +
> +	return err;
> +}
> diff --git a/security/landlock/socket.h b/security/landlock/socket.h
> new file mode 100644
> index 000000000000..8519357f1c39
> --- /dev/null
> +++ b/security/landlock/socket.h
> @@ -0,0 +1,17 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Landlock LSM - Socket management and hooks
> + *
> + * Copyright © 2024 Huawei Tech. Co., Ltd.
> + */
> +
> +#ifndef _SECURITY_LANDLOCK_SOCKET_H
> +#define _SECURITY_LANDLOCK_SOCKET_H
> +
> +#include "ruleset.h"
> +
> +int landlock_append_socket_rule(struct landlock_ruleset *const ruleset,
> +				const int family, const int type,
> +				access_mask_t access_rights);
> +
> +#endif /* _SECURITY_LANDLOCK_SOCKET_H */
> diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c
> index 03b470f5a85a..30c771f5e74f 100644
> --- a/security/landlock/syscalls.c
> +++ b/security/landlock/syscalls.c
> @@ -24,12 +24,14 @@
>  #include <linux/syscalls.h>
>  #include <linux/types.h>
>  #include <linux/uaccess.h>
> +#include <linux/net.h>
>  #include <uapi/linux/landlock.h>
>  
>  #include "cred.h"
>  #include "fs.h"
>  #include "limits.h"
>  #include "net.h"
> +#include "socket.h"
>  #include "ruleset.h"
>  #include "setup.h"
>  
> @@ -88,7 +90,8 @@ static void build_check_abi(void)
>  	struct landlock_ruleset_attr ruleset_attr;
>  	struct landlock_path_beneath_attr path_beneath_attr;
>  	struct landlock_net_port_attr net_port_attr;
> -	size_t ruleset_size, path_beneath_size, net_port_size;
> +	struct landlock_socket_attr socket_attr;
> +	size_t ruleset_size, path_beneath_size, net_port_size, socket_size;
>  
>  	/*
>  	 * For each user space ABI structures, first checks that there is no
> @@ -97,8 +100,9 @@ static void build_check_abi(void)
>  	 */
>  	ruleset_size = sizeof(ruleset_attr.handled_access_fs);
>  	ruleset_size += sizeof(ruleset_attr.handled_access_net);
> +	ruleset_size += sizeof(ruleset_attr.handled_access_socket);
>  	BUILD_BUG_ON(sizeof(ruleset_attr) != ruleset_size);
> -	BUILD_BUG_ON(sizeof(ruleset_attr) != 16);
> +	BUILD_BUG_ON(sizeof(ruleset_attr) != 24);
>  
>  	path_beneath_size = sizeof(path_beneath_attr.allowed_access);
>  	path_beneath_size += sizeof(path_beneath_attr.parent_fd);
> @@ -109,6 +113,12 @@ static void build_check_abi(void)
>  	net_port_size += sizeof(net_port_attr.port);
>  	BUILD_BUG_ON(sizeof(net_port_attr) != net_port_size);
>  	BUILD_BUG_ON(sizeof(net_port_attr) != 16);
> +
> +	socket_size = sizeof(socket_attr.allowed_access);
> +	socket_size += sizeof(socket_attr.family);
> +	socket_size += sizeof(socket_attr.type);
> +	BUILD_BUG_ON(sizeof(socket_attr) != socket_size);
> +	BUILD_BUG_ON(sizeof(socket_attr) != 16);
>  }
>  
>  /* Ruleset handling */
> @@ -149,7 +159,7 @@ static const struct file_operations ruleset_fops = {
>  	.write = fop_dummy_write,
>  };
>  
> -#define LANDLOCK_ABI_VERSION 5
> +#define LANDLOCK_ABI_VERSION 6
>  
>  /**
>   * sys_landlock_create_ruleset - Create a new ruleset
> @@ -213,9 +223,15 @@ SYSCALL_DEFINE3(landlock_create_ruleset,
>  	    LANDLOCK_MASK_ACCESS_NET)
>  		return -EINVAL;
>  
> +	/* Checks socket content (and 32-bits cast). */
> +	if ((ruleset_attr.handled_access_socket |
> +	     LANDLOCK_MASK_ACCESS_SOCKET) != LANDLOCK_MASK_ACCESS_SOCKET)
> +		return -EINVAL;
> +
>  	/* Checks arguments and transforms to kernel struct. */
>  	ruleset = landlock_create_ruleset(ruleset_attr.handled_access_fs,
> -					  ruleset_attr.handled_access_net);
> +					  ruleset_attr.handled_access_net,
> +					  ruleset_attr.handled_access_socket);
>  	if (IS_ERR(ruleset))
>  		return PTR_ERR(ruleset);
>  
> @@ -371,6 +387,45 @@ static int add_rule_net_port(struct landlock_ruleset *ruleset,
>  					net_port_attr.allowed_access);
>  }
>  
> +static int add_rule_socket(struct landlock_ruleset *ruleset,
> +			   const void __user *const rule_attr)
> +{
> +	struct landlock_socket_attr socket_attr;
> +	int family, type;
> +	int res;
> +	access_mask_t mask;
> +
> +	/* Copies raw user space buffer. */
> +	res = copy_from_user(&socket_attr, rule_attr, sizeof(socket_attr));
> +	if (res)
> +		return -EFAULT;
> +
> +	/*
> +	 * Informs about useless rule: empty allowed_access (i.e. deny rules)
> +	 * are ignored by socket actions.
> +	 */
> +	if (!socket_attr.allowed_access)
> +		return -ENOMSG;
> +
> +	/* Checks that allowed_access matches the @ruleset constraints. */
> +	mask = landlock_get_socket_access_mask(ruleset, 0);
> +	if ((socket_attr.allowed_access | mask) != mask)
> +		return -EINVAL;
> +
> +	family = socket_attr.family;
> +	type = socket_attr.type;
> +
> +	/* Denies inserting a rule with unsupported socket family and type. */
> +	if (family < 0 || family >= AF_MAX)
> +		return -EINVAL;
> +	if (type < 0 || type >= SOCK_MAX)
> +		return -EINVAL;

enum sock_type (include/linux/net.h) has "holes": values 7, 8 and 9 are not
defined in the header.  Should we check more specifically for the supported
values here?  (Is there already a helper function for that?)


> +	/* Imports the new rule. */
> +	return landlock_append_socket_rule(ruleset, family, type,
> +					   socket_attr.allowed_access);
> +}
> +
>  /**
>   * sys_landlock_add_rule - Add a new rule to a ruleset
>   *
> @@ -429,6 +484,9 @@ SYSCALL_DEFINE4(landlock_add_rule, const int, ruleset_fd,
>  	case LANDLOCK_RULE_NET_PORT:
>  		err = add_rule_net_port(ruleset, rule_attr);
>  		break;
> +	case LANDLOCK_RULE_SOCKET:
> +		err = add_rule_socket(ruleset, rule_attr);
> +		break;
>  	default:
>  		err = -EINVAL;
>  		break;
> diff --git a/tools/testing/selftests/landlock/base_test.c b/tools/testing/selftests/landlock/base_test.c
> index 3c1e9f35b531..52b00472a487 100644
> --- a/tools/testing/selftests/landlock/base_test.c
> +++ b/tools/testing/selftests/landlock/base_test.c
> @@ -75,7 +75,7 @@ TEST(abi_version)
>  	const struct landlock_ruleset_attr ruleset_attr = {
>  		.handled_access_fs = LANDLOCK_ACCESS_FS_READ_FILE,
>  	};
> -	ASSERT_EQ(5, landlock_create_ruleset(NULL, 0,
> +	ASSERT_EQ(6, landlock_create_ruleset(NULL, 0,
>  					     LANDLOCK_CREATE_RULESET_VERSION));
>  
>  	ASSERT_EQ(-1, landlock_create_ruleset(&ruleset_attr, 0,
> -- 
> 2.34.1
> 

—Günther

Re: [RFC PATCH v2 03/12] selftests/landlock: Add protocol.create to socket tests

[Günther Noack]

On Fri, May 24, 2024 at 05:30:06PM +0800, Mikhail Ivanov wrote:
> Initiate socket_test.c selftests. Add protocol fixture for tests
> with changeable family-type values. Only most common variants of
> protocols (like ipv4-tcp,ipv6-udp, unix) were added.
> Add simple socket access right checking test.
> 
> Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
> ---
> 
> Changes since v1:
> * Replaces test_socket_create() and socket_variant() helpers
>   with test_socket().
> * Renames domain to family in protocol fixture.
> * Remove AF_UNSPEC fixture entry and add unspec_srv0 fixture field to
>   check AF_UNSPEC socket creation case.
> * Formats code with clang-format.
> * Refactors commit message.
> ---
>  .../testing/selftests/landlock/socket_test.c  | 181 ++++++++++++++++++
>  1 file changed, 181 insertions(+)
>  create mode 100644 tools/testing/selftests/landlock/socket_test.c
> 
> diff --git a/tools/testing/selftests/landlock/socket_test.c b/tools/testing/selftests/landlock/socket_test.c
> new file mode 100644
> index 000000000000..4c51f89ed578
> --- /dev/null
> +++ b/tools/testing/selftests/landlock/socket_test.c
> @@ -0,0 +1,181 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Landlock tests - Socket
> + *
> + * Copyright © 2024 Huawei Tech. Co., Ltd.
> + * Copyright © 2024 Microsoft Corporation

It looked to me like these patches came from Huawei?
Was this left by accident?


> + */
> +
> +#define _GNU_SOURCE
> +
> +#include <errno.h>
> +#include <linux/landlock.h>
> +#include <sched.h>
> +#include <string.h>
> +#include <sys/prctl.h>
> +#include <sys/socket.h>
> +
> +#include "common.h"
> +
> +/* clang-format off */
> +
> +#define ACCESS_LAST LANDLOCK_ACCESS_SOCKET_CREATE
> +
> +#define ACCESS_ALL ( \
> +	LANDLOCK_ACCESS_SOCKET_CREATE)
> +
> +/* clang-format on */

It does not look like clang-format would really mess up this format in a bad
way.  Maybe we can remove the "clang-format off" section here and just write the
"#define"s on one line?

ACCESS_ALL is unused in this commit.
Should it be introduced in a subsequent commit instead?


> +static int test_socket(const struct service_fixture *const srv)
> +{
> +	int fd;
> +
> +	fd = socket(srv->protocol.family, srv->protocol.type | SOCK_CLOEXEC, 0);
> +	if (fd < 0)
> +		return errno;
> +	/*
> +	 * Mixing error codes from close(2) and socket(2) should not lead to any
> +	 * (access type) confusion for this test.
> +	 */
> +	if (close(fd) != 0)
> +		return errno;
> +	return 0;
> +}

I personally find that it helps me remember if these test helpers have the same
signature as the syscall that they are exercising.  (But I don't feel very
strongly about it.  Just a suggestion.)


> [...]
>
> +TEST_F(protocol, create)
> +{
> +	const struct landlock_ruleset_attr ruleset_attr = {
> +		.handled_access_socket = LANDLOCK_ACCESS_SOCKET_CREATE,
> +	};
> +	const struct landlock_socket_attr create_socket_attr = {
> +		.allowed_access = LANDLOCK_ACCESS_SOCKET_CREATE,
> +		.family = self->srv0.protocol.family,
> +		.type = self->srv0.protocol.type,
> +	};
> +
> +	int ruleset_fd;
> +
> +	/* Allowed create */
> +	ruleset_fd =
> +		landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
> +	ASSERT_LE(0, ruleset_fd);
> +
> +	ASSERT_EQ(0, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_SOCKET,
> +				       &create_socket_attr, 0));
> +
> +	enforce_ruleset(_metadata, ruleset_fd);
> +	EXPECT_EQ(0, close(ruleset_fd));
> +
> +	ASSERT_EQ(0, test_socket(&self->srv0));
> +	ASSERT_EQ(EAFNOSUPPORT, test_socket(&self->unspec_srv0));
> +
> +	/* Denied create */
> +	ruleset_fd =
> +		landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
> +	ASSERT_LE(0, ruleset_fd);
> +
> +	enforce_ruleset(_metadata, ruleset_fd);
> +	EXPECT_EQ(0, close(ruleset_fd));
> +
> +	ASSERT_EQ(EACCES, test_socket(&self->srv0));
> +	ASSERT_EQ(EAFNOSUPPORT, test_socket(&self->unspec_srv0));

Should we exhaustively try out the other combinations (other than selv->srv0)
here?  I assume socket() should always fail for these?

(If you are alredy doing this in another commit that I have not looked at yet,
please ignore this comment.)

—Günther

Re: [RFC PATCH v2 04/12] selftests/landlock: Add protocol.socket_access_rights to socket tests

[Günther Noack]

Hello!

I see that this test is adapted from the network_access_rights test in
net_test.c, and some of the subsequent are similarly copied from there.  It
makes it hard to criticize the code, because being a little bit consistent is
probably a good thing.  Have you found any opportunities to extract
commonalities into common.h?

On Fri, May 24, 2024 at 05:30:07PM +0800, Mikhail Ivanov wrote:
> Add test that checks possibility of adding rule with every possible
> access right.
> 
> Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
> ---
> 
> Changes since v1:
> * Formats code with clang-format.
> * Refactors commit message.
> ---
>  .../testing/selftests/landlock/socket_test.c  | 28 +++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/tools/testing/selftests/landlock/socket_test.c b/tools/testing/selftests/landlock/socket_test.c
> index 4c51f89ed578..eb5d62263460 100644
> --- a/tools/testing/selftests/landlock/socket_test.c
> +++ b/tools/testing/selftests/landlock/socket_test.c
> @@ -178,4 +178,32 @@ TEST_F(protocol, create)
>  	ASSERT_EQ(EAFNOSUPPORT, test_socket(&self->unspec_srv0));
>  }
>  
> +TEST_F(protocol, socket_access_rights)
> +{
> +	const struct landlock_ruleset_attr ruleset_attr = {
> +		.handled_access_socket = ACCESS_ALL,
> +	};
> +	struct landlock_socket_attr protocol = {
> +		.family = self->srv0.protocol.family,
> +		.type = self->srv0.protocol.type,
> +	};
> +	int ruleset_fd;
> +	__u64 access;
> +
> +	ruleset_fd =
> +		landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
> +	ASSERT_LE(0, ruleset_fd);
> +
> +	for (access = 1; access <= ACCESS_LAST; access <<= 1) {
> +		protocol.allowed_access = access;
> +		EXPECT_EQ(0, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_SOCKET,
> +					       &protocol, 0))
> +		{
> +			TH_LOG("Failed to add rule with access 0x%llx: %s",
> +			       access, strerror(errno));
> +		}
> +	}
> +	EXPECT_EQ(0, close(ruleset_fd));

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

P.S. We are inconsistent with our use of EXPECT/ASSERT for test teardown.  The
fs_test.c uses ASSERT_EQ in these places whereas net_test.c and your new tests
use EXPECT_EQ.

It admittedly does not make much of a difference for close(), so should be OK.
Some other selftests are even ignoring the result for close().  If we want to
make it consistent in the Landlock tests again, we can also do it in an
independent sweep.

I filed a small cleanup task as a reminder:
https://github.com/landlock-lsm/linux/issues/31

—Günther

Re: [RFC PATCH v2 08/12] selftests/landlock: Add tcp_layers.ruleset_overlap to socket tests

[Günther Noack]

On Fri, May 24, 2024 at 05:30:11PM +0800, Mikhail Ivanov wrote:
> * Add tcp_layers fixture for tests that check multiple layer
>   configuration scenarios.
> 
> * Add test that validates multiple layer behavior with overlapped
>   restrictions.
> 
> Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
> ---
> 
> Changes since v1:
> * Replaces test_socket_create() with test_socket().
> * Formats code with clang-format.
> * Refactors commit message.
> * Minor fixes.
> ---
>  .../testing/selftests/landlock/socket_test.c  | 109 ++++++++++++++++++
>  1 file changed, 109 insertions(+)
> 
> diff --git a/tools/testing/selftests/landlock/socket_test.c b/tools/testing/selftests/landlock/socket_test.c
> index 751596c381fe..52edc1a8ac21 100644
> --- a/tools/testing/selftests/landlock/socket_test.c
> +++ b/tools/testing/selftests/landlock/socket_test.c
> @@ -299,4 +299,113 @@ TEST_F(protocol, inval)
>  				       &protocol, 0));
>  }
>  
> +FIXTURE(tcp_layers)
> +{
> +	struct service_fixture srv0;
> +};
> +
> +FIXTURE_VARIANT(tcp_layers)
> +{
> +	const size_t num_layers;
> +};
> +
> +FIXTURE_SETUP(tcp_layers)
> +{
> +	const struct protocol_variant prot = {
> +		.family = AF_INET,
> +		.type = SOCK_STREAM,
> +	};
> +
> +	disable_caps(_metadata);
> +	self->srv0.protocol = prot;
> +	setup_namespace(_metadata);
> +};
> +
> +FIXTURE_TEARDOWN(tcp_layers)
> +{
> +}
> +
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(tcp_layers, no_sandbox_with_ipv4) {
> +	/* clang-format on */
> +	.num_layers = 0,
> +};
> +
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(tcp_layers, one_sandbox_with_ipv4) {
> +	/* clang-format on */
> +	.num_layers = 1,
> +};
> +
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(tcp_layers, two_sandboxes_with_ipv4) {
> +	/* clang-format on */
> +	.num_layers = 2,
> +};
> +
> +/* clang-format off */
> +FIXTURE_VARIANT_ADD(tcp_layers, three_sandboxes_with_ipv4) {
> +	/* clang-format on */
> +	.num_layers = 3,
> +};
> +
> +TEST_F(tcp_layers, ruleset_overlap)
> +{
> +	const struct landlock_ruleset_attr ruleset_attr = {
> +		.handled_access_socket = LANDLOCK_ACCESS_SOCKET_CREATE,
> +	};
> +	const struct landlock_socket_attr tcp_create = {
> +		.allowed_access = LANDLOCK_ACCESS_SOCKET_CREATE,
> +		.family = self->srv0.protocol.family,
> +		.type = self->srv0.protocol.type,
> +	};
> +
> +	if (variant->num_layers >= 1) {
> +		int ruleset_fd;
> +
> +		ruleset_fd = landlock_create_ruleset(&ruleset_attr,
> +						     sizeof(ruleset_attr), 0);
> +		ASSERT_LE(0, ruleset_fd);
> +
> +		/* Allows create. */
> +		ASSERT_EQ(0, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_SOCKET,
> +					       &tcp_create, 0));
> +		enforce_ruleset(_metadata, ruleset_fd);
> +		EXPECT_EQ(0, close(ruleset_fd));
> +	}
> +
> +	if (variant->num_layers >= 2) {
> +		int ruleset_fd;
> +
> +		/* Creates another ruleset layer with denied create. */
> +		ruleset_fd = landlock_create_ruleset(&ruleset_attr,
> +						     sizeof(ruleset_attr), 0);
> +		ASSERT_LE(0, ruleset_fd);
> +
> +		enforce_ruleset(_metadata, ruleset_fd);
> +		EXPECT_EQ(0, close(ruleset_fd));
> +	}
> +
> +	if (variant->num_layers >= 3) {
> +		int ruleset_fd;
> +
> +		/* Creates another ruleset layer. */
> +		ruleset_fd = landlock_create_ruleset(&ruleset_attr,
> +						     sizeof(ruleset_attr), 0);
> +		ASSERT_LE(0, ruleset_fd);
> +
> +		/* Try to allow create second time. */
> +		ASSERT_EQ(0, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_SOCKET,
> +					       &tcp_create, 0));
> +		enforce_ruleset(_metadata, ruleset_fd);
> +		EXPECT_EQ(0, close(ruleset_fd));
> +	}
> +
> +	if (variant->num_layers < 2) {
> +		ASSERT_EQ(0, test_socket(&self->srv0));
> +	} else {
> +		ASSERT_EQ(EACCES, test_socket(&self->srv0));
> +	}
> +}

Wouldn't this be simpler if you did multiple checks in one test, in a sequence?

  * Expect that socket() works
  * Enforce ruleset 1 with a rule
  * Expect that socket() works
  * Enforce ruleset 2 without a rule
  * Expect that socket() fails
  * Enforce ruleset 3
  * Expect that socket() still fails

Then it would test the same and you would not need the fixture.
If you extracted these if bodies above into helper functions,
I think it would also read reasonably well.

—Günther

Re: [RFC PATCH v2 05/12] selftests/landlock: Add protocol.rule_with_unknown_access to socket tests

[Günther Noack]

On Fri, May 24, 2024 at 05:30:08PM +0800, Mikhail Ivanov wrote:
> Add test that validates behavior of landlock after rule with
> unknown access is added.
> 
> Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
> ---
> 
> Changes since v1:
> * Refactors commit messsage.
> ---
>  .../testing/selftests/landlock/socket_test.c  | 26 +++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/tools/testing/selftests/landlock/socket_test.c b/tools/testing/selftests/landlock/socket_test.c
> index eb5d62263460..57d5927906b8 100644
> --- a/tools/testing/selftests/landlock/socket_test.c
> +++ b/tools/testing/selftests/landlock/socket_test.c
> @@ -206,4 +206,30 @@ TEST_F(protocol, socket_access_rights)
>  	EXPECT_EQ(0, close(ruleset_fd));
>  }
>  
> +TEST_F(protocol, rule_with_unknown_access)
> +{
> +	const struct landlock_ruleset_attr ruleset_attr = {
> +		.handled_access_socket = ACCESS_ALL,
> +	};
> +	struct landlock_socket_attr protocol = {
> +		.family = self->srv0.protocol.family,
> +		.type = self->srv0.protocol.type,
> +	};
> +	int ruleset_fd;
> +	__u64 access;
> +
> +	ruleset_fd =
> +		landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
> +	ASSERT_LE(0, ruleset_fd);
> +
> +	for (access = 1ULL << 63; access != ACCESS_LAST; access >>= 1) {
> +		protocol.allowed_access = access;
> +		EXPECT_EQ(-1,
> +			  landlock_add_rule(ruleset_fd, LANDLOCK_RULE_SOCKET,
> +					    &protocol, 0));
> +		EXPECT_EQ(EINVAL, errno);
> +	}
> +	EXPECT_EQ(0, close(ruleset_fd));
> +}
> +
>  TEST_HARNESS_MAIN
> -- 
> 2.34.1
> 

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

Re: [RFC PATCH v2 06/12] selftests/landlock: Add protocol.rule_with_unhandled_access to socket tests

[Günther Noack]

On Fri, May 24, 2024 at 05:30:09PM +0800, Mikhail Ivanov wrote:
> Add test that validates behavior of landlock after rule with
> unhandled access is added.
> 
> Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
> ---
> 
> Changes since v1:
> * Refactors commit message.
> ---
>  .../testing/selftests/landlock/socket_test.c  | 33 +++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/tools/testing/selftests/landlock/socket_test.c b/tools/testing/selftests/landlock/socket_test.c
> index 57d5927906b8..31af47de1937 100644
> --- a/tools/testing/selftests/landlock/socket_test.c
> +++ b/tools/testing/selftests/landlock/socket_test.c
> @@ -232,4 +232,37 @@ TEST_F(protocol, rule_with_unknown_access)
>  	EXPECT_EQ(0, close(ruleset_fd));
>  }
>  
> +TEST_F(protocol, rule_with_unhandled_access)
> +{
> +	struct landlock_ruleset_attr ruleset_attr = {
> +		.handled_access_socket = LANDLOCK_ACCESS_SOCKET_CREATE,
> +	};
> +	struct landlock_socket_attr protocol = {
> +		.family = self->srv0.protocol.family,
> +		.type = self->srv0.protocol.type,
> +	};
> +	int ruleset_fd;
> +	__u64 access;
> +
> +	ruleset_fd =
> +		landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
> +	ASSERT_LE(0, ruleset_fd);
> +
> +	for (access = 1; access > 0; access <<= 1) {
> +		int err;
> +
> +		protocol.allowed_access = access;
> +		err = landlock_add_rule(ruleset_fd, LANDLOCK_RULE_SOCKET,
> +					&protocol, 0);
> +		if (access == ruleset_attr.handled_access_socket) {
> +			EXPECT_EQ(0, err);
> +		} else {
> +			EXPECT_EQ(-1, err);
> +			EXPECT_EQ(EINVAL, errno);
> +		}
> +	}
> +
> +	EXPECT_EQ(0, close(ruleset_fd));
> +}
> +
>  TEST_HARNESS_MAIN
> -- 
> 2.34.1
>

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

Like the commit before, this is copied from net_test.c and I don't want to
bikeshed around on code style which was discussed before.

Trying to factor out commonalities might also introduce additional layers of
indirection that would obscure what is happening.  I think it should be fine
like that despite it having some duplication.

—Günther

Re: [RFC PATCH v2 07/12] selftests/landlock: Add protocol.inval to socket tests

[Günther Noack]

On Fri, May 24, 2024 at 05:30:10PM +0800, Mikhail Ivanov wrote:
> Add test that validates behavior of landlock with fully
> access restriction.
> 
> Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
> ---
> 
> Changes since v1:
> * Refactors commit message.
> ---
>  .../testing/selftests/landlock/socket_test.c  | 34 +++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/tools/testing/selftests/landlock/socket_test.c b/tools/testing/selftests/landlock/socket_test.c
> index 31af47de1937..751596c381fe 100644
> --- a/tools/testing/selftests/landlock/socket_test.c
> +++ b/tools/testing/selftests/landlock/socket_test.c
> @@ -265,4 +265,38 @@ TEST_F(protocol, rule_with_unhandled_access)
>  	EXPECT_EQ(0, close(ruleset_fd));
>  }
>  
> +TEST_F(protocol, inval)
> +{
> +	const struct landlock_ruleset_attr ruleset_attr = {
> +		.handled_access_socket = LANDLOCK_ACCESS_SOCKET_CREATE
> +	};
> +
> +	struct landlock_socket_attr protocol = {
> +		.allowed_access = LANDLOCK_ACCESS_SOCKET_CREATE,
> +		.family = self->srv0.protocol.family,
> +		.type = self->srv0.protocol.type,
> +	};
> +
> +	struct landlock_socket_attr protocol_denied = {
> +		.allowed_access = 0,
> +		.family = self->srv0.protocol.family,
> +		.type = self->srv0.protocol.type,
> +	};
> +
> +	int ruleset_fd;
> +
> +	ruleset_fd =
> +		landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
> +	ASSERT_LE(0, ruleset_fd);
> +
> +	/* Checks zero access value. */
> +	EXPECT_EQ(-1, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_SOCKET,
> +					&protocol_denied, 0));
> +	EXPECT_EQ(ENOMSG, errno);
> +
> +	/* Adds with legitimate values. */
> +	ASSERT_EQ(0, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_SOCKET,
> +				       &protocol, 0));
> +}
> +
>  TEST_HARNESS_MAIN
> -- 
> 2.34.1
> 

Code is based on TEST_F(mini, inval) from net_test.c.  I see that you removed
the check for unhandled allowed_access, because there is already a separate
TEST_F(mini, rule_with_unhandled_access) for that.

That is true for the "legitimate value" case as well, though...?  We already
have a test for that too.  Should that also get removed?

Should we then rename the "inval" test to "rule_with_zero_access", so that the
naming is consistent with the "rule_with_unhandled_access" test?

—Günther

Re: [RFC PATCH v2 01/12] landlock: Support socket access-control

[Mikhail Ivanov]

5/27/2024 12:57 PM, Günther Noack wrote:
> On Fri, May 24, 2024 at 05:30:04PM +0800, Mikhail Ivanov wrote:
>> * Add new landlock rule type that corresponds to the restriction of
>>    socket protocols. This is represented as an landlock_socket_attr
>>    structure. Protocol allowed by landlock must be described by
>>    a family-type pair (see socket(2)).
>>
>> * Support socket rule storage in landlock ruleset.
>>
>> * Add flag LANDLOCK_ACCESS_SOCKET_CREATE that will provide the
>>    ability to control socket creation.
>>
>> * Add socket.c file that will contain socket rules management and hooks.
>>    Implement helper pack_socket_key() to convert 32-bit family and type
>>    values into uintptr_t. This is possible due to the fact that these
>>    values are limited to AF_MAX (=46), SOCK_MAX (=11) constants. Assumption
>>    is checked in build-time by the helper.
>>
>> * Support socket rules in landlock syscalls. Change ABI version to 6.
>>
>> Closes: https://github.com/landlock-lsm/linux/issues/6
>> Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
>> ---
>>
>> Changes since v1:
>> * Reverts landlock_key.data type from u64 to uinptr_t.
>> * Adds helper to pack domain and type values into uintptr_t.
>> * Denies inserting socket rule with invalid family and type.
>> * Renames 'domain' to 'family' in landlock_socket_attr.
>> * Updates ABI version to 6 since ioctl patches changed it to 5.
>> * Formats code with clang-format.
>> * Minor fixes.
>> ---
>>   include/uapi/linux/landlock.h                | 53 +++++++++++++++-
>>   security/landlock/Makefile                   |  2 +-
>>   security/landlock/limits.h                   |  5 ++
>>   security/landlock/ruleset.c                  | 37 ++++++++++-
>>   security/landlock/ruleset.h                  | 41 +++++++++++-
>>   security/landlock/socket.c                   | 60 ++++++++++++++++++
>>   security/landlock/socket.h                   | 17 +++++
>>   security/landlock/syscalls.c                 | 66 ++++++++++++++++++--
>>   tools/testing/selftests/landlock/base_test.c |  2 +-
>>   9 files changed, 272 insertions(+), 11 deletions(-)
>>   create mode 100644 security/landlock/socket.c
>>   create mode 100644 security/landlock/socket.h
>>
>> diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
>> index 68625e728f43..a25ba5983dfb 100644
>> --- a/include/uapi/linux/landlock.h
>> +++ b/include/uapi/linux/landlock.h
>> @@ -37,6 +37,13 @@ struct landlock_ruleset_attr {
>>   	 * rule explicitly allow them.
>>   	 */
>>   	__u64 handled_access_net;
>> +
>> +	/**
>> +	 * @handled_access_socket: Bitmask of actions (cf. `Socket flags`_)
>> +	 * that is handled by this ruleset and should then be forbidden if no
>> +	 * rule explicitly allow them.
>> +	 */
>> +	__u64 handled_access_socket;
>>   };
>>   
>>   /*
>> @@ -65,6 +72,11 @@ enum landlock_rule_type {
>>   	 * landlock_net_port_attr .
>>   	 */
>>   	LANDLOCK_RULE_NET_PORT,
>> +	/**
>> +	 * @LANDLOCK_RULE_SOCKET: Type of a &struct
>> +	 * landlock_socket_attr .
>> +	 */
>> +	LANDLOCK_RULE_SOCKET,
>>   };
>>   
>>   /**
>> @@ -115,6 +127,28 @@ struct landlock_net_port_attr {
>>   	__u64 port;
>>   };
>>   
>> +/**
>> + * struct landlock_socket_attr - Socket definition
>> + *
>> + * Argument of sys_landlock_add_rule().
>> + */
>> +struct landlock_socket_attr {
>> +	/**
>> +	 * @allowed_access: Bitmask of allowed access for a socket
>> +	 * (cf. `Socket flags`_).
>> +	 */
>> +	__u64 allowed_access;
>> +	/**
>> +	 * @family: Protocol family used for communication
>> +	 * (same as domain in socket(2)).
>> +	 */
>> +	int family;
>> +	/**
>> +	 * @type: Socket type (see socket(2)).
>> +	 */
>> +	int type;
>> +};
> 
> Regarding the naming of struct landlock_socket_attr and the associated
> LANDLOCK_RULE_SOCKET enum:
> 
> For the two existing rule types LANDLOCK_RULE_PATH_BENEATH (struct
> landlock_path_beneath_attr) and LANDLOCK_RULE_NET_PORT (struct
> landlock_net_port_attr), the names of the rule types are describing the
> *properties* by which we are filtering (path *beneath*, *network port*), rather
> than just the kind of object that we are filtering on.
> 
> Should the new enum and struct maybe be called differently as well to match that
> convention?  Maybe LANDLOCK_RULE_SOCKET_FAMILY_TYPE and struct
> landlock_socket_family_type_attr?
> 
> Are there *other* properties apart from family and type, by which you are
> thinking of restricting the use of sockets in the future?

There was a thought about adding `protocol` (socket(2)) restriction,
but Mickaël noted that it would be useless [1]. Therefore, no other
properties are planned until someone has good use cases.

I agree that current naming can be associated with socket objects. But i
don't think using family-type words for naming of this rule would be
convenient for users. In comparison with net port and path beneath
family-type pair doesn't represent a single semantic unit, so it would
be a little harder to read the code.

Perhaps LANDLOCK_RULE_SOCKET_PROTO (struct landlock_socket_proto_attr)
would be more suitable here? Although socket(2) has `protocol` argument
to specify the socket protocol in some cases (e.g. RAW sockets), in most
cases family-type pair defines protocol itself. Since the purpose of
this patchlist is to restrict protocols used in a sandboxed process, I
think that in the presence of well-written documentation, such naming
may be appropriate here. WDYT?

[1] 
https://lore.kernel.org/all/a6318388-e28a-e96f-b1ae-51948c13de4d@digikod.net/

> 
> 
>> @@ -266,4 +300,21 @@ struct landlock_net_port_attr {
>>   #define LANDLOCK_ACCESS_NET_BIND_TCP			(1ULL << 0)
>>   #define LANDLOCK_ACCESS_NET_CONNECT_TCP			(1ULL << 1)
>>   /* clang-format on */
>> +
>> +/**
>> + * DOC: socket_access
>> + *
>> + * Socket flags
>> + * ~~~~~~~~~~~~
>> + *
>> + * These flags enable to restrict a sanboxed process to a set of socket
>> + * protocols. This is supported since the Landlock ABI version 6.
> 
> (Some phrasing remarks)
> 
>    * typo in "sanboxed"
>    * Optional grammar nit: you can drop the "the" in front of "Landlock ABI
>      version 6" (or alternatively use the phrasing as it was used in the FS
>      restriction docs)
>    * Grammar nit: The use of "enable to" sounds weird in my ears (but I am not a
>      native speaker either).  I think it could just be dropped here ("These flags
>      restrict a sandboxed process..." or "These flags control the use of...").
>      I realize that the wording was used in other places already, so it's just an
>      optional remark.

Thanks! I'll fix this according to your advices.

> 
> (More about the content)
> 
> The Landlock documentation states the general approach up front:
> 
>    A Landlock rule describes an *action* on an *object* which the process intends
>    to perform.
> 
> (In your case, the object is a socket, and the action is the socket's creation.
> The Landlock rules describe predicates on objects to restrict the set of actions
> through the access_mask_t.)
> 
> The implementation is perfectly in line with that, but it would help to phrase
> the documentation also in terms of that framework.  That means, what we are
> restricting are *actions*, not protocols.
> 
> To make a more constructive suggestion:
> 
>    "These flags restrict actions on sockets for a sandboxed process (e.g. socket
>    creation)."

I think this has too general meaning (e.g. bind(2) is also an action on
socket). Probably this one would be more suitable:

   "These flags restrict actions of adding sockets in a sandboxed
   process (e.g. socket creation, passing socket FDs to/from the
   process)."

> 
> Does it also need the following addition?
> 
>    "Sockets opened before sandboxing are not subject to these restrictions."

Yeah, it makes sense. I think it would be great to somehow make this
note common to all types of rules though.

> 
> 
>> + *
>> + * The following access rights apply only to sockets:
>                      ^^^^^^^^^^^^^^^^^^^
> Probably better to use singular for now: "access right applies".

agreed, thanks

> 
>> + *
>> + * - %LANDLOCK_ACCESS_SOCKET_CREATE: Create a socket.
> 
> Can we be more specific here what operations are affected by this?  It is rather
> obvious that this affects socket(2), but does this also affect accept(2) and
> connect(2)?
> 
> A scenario that I could imagine being useful is to sandbox a TCP server like
> this:
> 
>    * create a socket, bind(2) and listen(2)
>    * sandbox yourself so that no new sockets can be created with socket(2)
>    * go into the main loop and start accept(2)ing new connections
> 
> Is this an approach that would work with this patch set?

Yes, such scenario is possible. This rule should apply to all socket
creation requests in the user space (socket(2), socketpair(2), io_uring
request). Perhaps it's necessary to clarify here that only user space
sockets are restricted?

Btw, current implementation doesn't check that the socket creation
request doesn't come from the kernel space. Will be fixed.

> 
> (It might make a neat sample tool as well, if something like this works :))
> 
> 
> Regarding the list of socket access rights with only one item in it:
> 
> I am still unsure what other socket actions are in scope in the future; it would
> probably help to phrase the documentation in those terms.  (listen(2), bind(2),
> connect(2), shutdown(2)?  On the other hand, bind(2) and connect(2) for TCP are
> already restrictable differently.))

I think it would be useful to restrict sending and receiving socket
FDs via unix domain sockets (see SCM_RIGHTS in unix(7)).

> 
>> + */
>> +/* clang-format off */
>> +#define LANDLOCK_ACCESS_SOCKET_CREATE			(1ULL << 0)
>> +/* clang-format on */
>>   #endif /* _UAPI_LINUX_LANDLOCK_H */
>> diff --git a/security/landlock/Makefile b/security/landlock/Makefile
>> index b4538b7cf7d2..ff1dd98f6a1b 100644
>> --- a/security/landlock/Makefile
>> +++ b/security/landlock/Makefile
>> @@ -1,6 +1,6 @@
>>   obj-$(CONFIG_SECURITY_LANDLOCK) := landlock.o
>>   
>>   landlock-y := setup.o syscalls.o object.o ruleset.o \
>> -	cred.o task.o fs.o
>> +	cred.o task.o fs.o socket.o
>>   
>>   landlock-$(CONFIG_INET) += net.o
>> diff --git a/security/landlock/limits.h b/security/landlock/limits.h
>> index 20fdb5ff3514..448b4d596783 100644
>> --- a/security/landlock/limits.h
>> +++ b/security/landlock/limits.h
>> @@ -28,6 +28,11 @@
>>   #define LANDLOCK_NUM_ACCESS_NET		__const_hweight64(LANDLOCK_MASK_ACCESS_NET)
>>   #define LANDLOCK_SHIFT_ACCESS_NET	LANDLOCK_NUM_ACCESS_FS
>>   
>> +#define LANDLOCK_LAST_ACCESS_SOCKET	    LANDLOCK_ACCESS_SOCKET_CREATE
>> +#define LANDLOCK_MASK_ACCESS_SOCKET	    ((LANDLOCK_LAST_ACCESS_SOCKET << 1) - 1)
>> +#define LANDLOCK_NUM_ACCESS_SOCKET		__const_hweight64(LANDLOCK_MASK_ACCESS_SOCKET)
>> +#define LANDLOCK_SHIFT_ACCESS_SOCKET	LANDLOCK_NUM_ACCESS_SOCKET
>> +
>>   /* clang-format on */
>>   
>>   #endif /* _SECURITY_LANDLOCK_LIMITS_H */
>> diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
>> index e0a5fbf9201a..c782f7cd313d 100644
>> --- a/security/landlock/ruleset.c
>> +++ b/security/landlock/ruleset.c
>> @@ -40,6 +40,7 @@ static struct landlock_ruleset *create_ruleset(const u32 num_layers)
>>   #if IS_ENABLED(CONFIG_INET)
>>   	new_ruleset->root_net_port = RB_ROOT;
>>   #endif /* IS_ENABLED(CONFIG_INET) */
>> +	new_ruleset->root_socket = RB_ROOT;
>>   
>>   	new_ruleset->num_layers = num_layers;
>>   	/*
>> @@ -52,12 +53,13 @@ static struct landlock_ruleset *create_ruleset(const u32 num_layers)
>>   
>>   struct landlock_ruleset *
>>   landlock_create_ruleset(const access_mask_t fs_access_mask,
>> -			const access_mask_t net_access_mask)
>> +			const access_mask_t net_access_mask,
>> +			const access_mask_t socket_access_mask)
>>   {
>>   	struct landlock_ruleset *new_ruleset;
>>   
>>   	/* Informs about useless ruleset. */
>> -	if (!fs_access_mask && !net_access_mask)
>> +	if (!fs_access_mask && !net_access_mask && !socket_access_mask)
>>   		return ERR_PTR(-ENOMSG);
>>   	new_ruleset = create_ruleset(1);
>>   	if (IS_ERR(new_ruleset))
>> @@ -66,6 +68,9 @@ landlock_create_ruleset(const access_mask_t fs_access_mask,
>>   		landlock_add_fs_access_mask(new_ruleset, fs_access_mask, 0);
>>   	if (net_access_mask)
>>   		landlock_add_net_access_mask(new_ruleset, net_access_mask, 0);
>> +	if (socket_access_mask)
>> +		landlock_add_socket_access_mask(new_ruleset, socket_access_mask,
>> +						0);
>>   	return new_ruleset;
>>   }
>>   
>> @@ -89,6 +94,9 @@ static bool is_object_pointer(const enum landlock_key_type key_type)
>>   		return false;
>>   #endif /* IS_ENABLED(CONFIG_INET) */
>>   
>> +	case LANDLOCK_KEY_SOCKET:
>> +		return false;
>> +
>>   	default:
>>   		WARN_ON_ONCE(1);
>>   		return false;
>> @@ -146,6 +154,9 @@ static struct rb_root *get_root(struct landlock_ruleset *const ruleset,
>>   		return &ruleset->root_net_port;
>>   #endif /* IS_ENABLED(CONFIG_INET) */
>>   
>> +	case LANDLOCK_KEY_SOCKET:
>> +		return &ruleset->root_socket;
>> +
>>   	default:
>>   		WARN_ON_ONCE(1);
>>   		return ERR_PTR(-EINVAL);
>> @@ -175,7 +186,9 @@ static void build_check_ruleset(void)
>>   	BUILD_BUG_ON(ruleset.num_layers < LANDLOCK_MAX_NUM_LAYERS);
>>   	BUILD_BUG_ON(access_masks <
>>   		     ((LANDLOCK_MASK_ACCESS_FS << LANDLOCK_SHIFT_ACCESS_FS) |
>> -		      (LANDLOCK_MASK_ACCESS_NET << LANDLOCK_SHIFT_ACCESS_NET)));
>> +		      (LANDLOCK_MASK_ACCESS_NET << LANDLOCK_SHIFT_ACCESS_NET) |
>> +		      (LANDLOCK_MASK_ACCESS_SOCKET
>> +		       << LANDLOCK_SHIFT_ACCESS_SOCKET)));
>>   }
>>   
>>   /**
>> @@ -399,6 +412,11 @@ static int merge_ruleset(struct landlock_ruleset *const dst,
>>   		goto out_unlock;
>>   #endif /* IS_ENABLED(CONFIG_INET) */
>>   
>> +	/* Merges the @src socket tree. */
>> +	err = merge_tree(dst, src, LANDLOCK_KEY_SOCKET);
>> +	if (err)
>> +		goto out_unlock;
>> +
>>   out_unlock:
>>   	mutex_unlock(&src->lock);
>>   	mutex_unlock(&dst->lock);
>> @@ -462,6 +480,11 @@ static int inherit_ruleset(struct landlock_ruleset *const parent,
>>   		goto out_unlock;
>>   #endif /* IS_ENABLED(CONFIG_INET) */
>>   
>> +	/* Copies the @parent socket tree. */
>> +	err = inherit_tree(parent, child, LANDLOCK_KEY_SOCKET);
>> +	if (err)
>> +		goto out_unlock;
>> +
>>   	if (WARN_ON_ONCE(child->num_layers <= parent->num_layers)) {
>>   		err = -EINVAL;
>>   		goto out_unlock;
>> @@ -498,6 +521,10 @@ static void free_ruleset(struct landlock_ruleset *const ruleset)
>>   		free_rule(freeme, LANDLOCK_KEY_NET_PORT);
>>   #endif /* IS_ENABLED(CONFIG_INET) */
>>   
>> +	rbtree_postorder_for_each_entry_safe(freeme, next,
>> +					     &ruleset->root_socket, node)
>> +		free_rule(freeme, LANDLOCK_KEY_SOCKET);
>> +
>>   	put_hierarchy(ruleset->hierarchy);
>>   	kfree(ruleset);
>>   }
>> @@ -708,6 +735,10 @@ landlock_init_layer_masks(const struct landlock_ruleset *const domain,
>>   		break;
>>   #endif /* IS_ENABLED(CONFIG_INET) */
>>   
>> +	case LANDLOCK_KEY_SOCKET:
>> +		get_access_mask = landlock_get_socket_access_mask;
>> +		num_access = LANDLOCK_NUM_ACCESS_SOCKET;
>> +		break;
>>   	default:
>>   		WARN_ON_ONCE(1);
>>   		return 0;
>> diff --git a/security/landlock/ruleset.h b/security/landlock/ruleset.h
>> index c7f1526784fd..a9773efd529b 100644
>> --- a/security/landlock/ruleset.h
>> +++ b/security/landlock/ruleset.h
>> @@ -92,6 +92,12 @@ enum landlock_key_type {
>>   	 * node keys.
>>   	 */
>>   	LANDLOCK_KEY_NET_PORT,
>> +
>> +	/**
>> +	 * @LANDLOCK_KEY_SOCKET: Type of &landlock_ruleset.root_socket's
>> +	 * node keys.
>> +	 */
>> +	LANDLOCK_KEY_SOCKET,
>>   };
>>   
>>   /**
>> @@ -177,6 +183,15 @@ struct landlock_ruleset {
>>   	struct rb_root root_net_port;
>>   #endif /* IS_ENABLED(CONFIG_INET) */
>>   
>> +	/**
>> +	 * @root_socket: Root of a red-black tree containing &struct
>> +	 * landlock_rule nodes with socket type, described by (family, type)
>> +	 * pair (see socket(2)). Once a ruleset is tied to a
>> +	 * process (i.e. as a domain), this tree is immutable until @usage
>> +	 * reaches zero.
>> +	 */
>> +	struct rb_root root_socket;
>> +
>>   	/**
>>   	 * @hierarchy: Enables hierarchy identification even when a parent
>>   	 * domain vanishes.  This is needed for the ptrace protection.
>> @@ -233,7 +248,8 @@ struct landlock_ruleset {
>>   
>>   struct landlock_ruleset *
>>   landlock_create_ruleset(const access_mask_t access_mask_fs,
>> -			const access_mask_t access_mask_net);
>> +			const access_mask_t access_mask_net,
>> +			const access_mask_t access_mask_socket);
>>   
>>   void landlock_put_ruleset(struct landlock_ruleset *const ruleset);
>>   void landlock_put_ruleset_deferred(struct landlock_ruleset *const ruleset);
>> @@ -282,6 +298,20 @@ landlock_add_net_access_mask(struct landlock_ruleset *const ruleset,
>>   		(net_mask << LANDLOCK_SHIFT_ACCESS_NET);
>>   }
>>   
>> +static inline void
>> +landlock_add_socket_access_mask(struct landlock_ruleset *const ruleset,
>> +				const access_mask_t socket_access_mask,
>> +				const u16 layer_level)
>> +{
>> +	access_mask_t socket_mask = socket_access_mask &
>> +				    LANDLOCK_MASK_ACCESS_SOCKET;
>> +
>> +	/* Should already be checked in sys_landlock_create_ruleset(). */
>> +	WARN_ON_ONCE(socket_access_mask != socket_mask);
>> +	ruleset->access_masks[layer_level] |=
>> +		(socket_mask << LANDLOCK_SHIFT_ACCESS_SOCKET);
>> +}
>> +
>>   static inline access_mask_t
>>   landlock_get_raw_fs_access_mask(const struct landlock_ruleset *const ruleset,
>>   				const u16 layer_level)
>> @@ -309,6 +339,15 @@ landlock_get_net_access_mask(const struct landlock_ruleset *const ruleset,
>>   	       LANDLOCK_MASK_ACCESS_NET;
>>   }
>>   
>> +static inline access_mask_t
>> +landlock_get_socket_access_mask(const struct landlock_ruleset *const ruleset,
>> +				const u16 layer_level)
>> +{
>> +	return (ruleset->access_masks[layer_level] >>
>> +		LANDLOCK_SHIFT_ACCESS_SOCKET) &
>> +	       LANDLOCK_MASK_ACCESS_SOCKET;
>> +}
>> +
>>   bool landlock_unmask_layers(const struct landlock_rule *const rule,
>>   			    const access_mask_t access_request,
>>   			    layer_mask_t (*const layer_masks)[],
>> diff --git a/security/landlock/socket.c b/security/landlock/socket.c
>> new file mode 100644
>> index 000000000000..1249a4a36503
>> --- /dev/null
>> +++ b/security/landlock/socket.c
>> @@ -0,0 +1,60 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Landlock LSM - Socket management and hooks
>> + *
>> + * Copyright © 2024 Huawei Tech. Co., Ltd.
>> + */
>> +
>> +#include <linux/net.h>
>> +#include <linux/socket.h>
>> +#include <linux/stddef.h>
>> +
>> +#include "limits.h"
>> +#include "ruleset.h"
>> +#include "socket.h"
>> +
>> +static uintptr_t pack_socket_key(const int family, const int type)
>> +{
>> +	union {
>> +		struct {
>> +			unsigned short family, type;
>> +		} __packed data;
>> +		uintptr_t packed;
>> +	} socket_key;
>> +
>> +	/* Checks that all supported socket families and types can be stored in socket_key. */
>> +	BUILD_BUG_ON(AF_MAX > (typeof(socket_key.data.family))~0);
>> +	BUILD_BUG_ON(SOCK_MAX > (typeof(socket_key.data.type))~0);
> 
> Off-by-one nit: AF_MAX and SOCK_MAX are one higher than the last permitted value,
> so technically it would be ok if they are one higher than (unsigned short)~0.
> 
>> +
>> +	/* Checks that socket_key can be stored in landlock_key. */
>> +	BUILD_BUG_ON(sizeof(socket_key.data) > sizeof(socket_key.packed));
>> +	BUILD_BUG_ON(sizeof(socket_key.packed) >
>> +		     sizeof_field(union landlock_key, data));
>> +
>> +	socket_key.data.family = (unsigned short)family;
>> +	socket_key.data.type = (unsigned short)type;
>> +
>> +	return socket_key.packed;
> 
> Can socket_key.packed end up containing uninitialized memory here?


Indeed, there is UB here. socket_key.packed type should be changed to
unsigned int. Thank you!

> 
>> +}
> 
> I see that this function traces back to Mickaël's comment in
> https://lore.kernel.org/all/20240412.phoh7laim7Th@digikod.net/
> 
> In my understanding, the motivation was to keep the key size in check.
> But that does not mean that we need to turn it into a uintptr_t?
> 
> Would it not have been possible to extend the union landlock_key in ruleset.h
> with a
> 
>    struct {
>      unsigned short family, type;
>    }
> 
> and then do the AF_MAX, SOCK_MAX build-time checks on that?
> It seems like that might be more in line with what we already have?

I don't think that complicating general entity with such a specific
representation would be a good solution here. `landlock_key` shouldn't
contain any semantic information about the key content.

> 
>> +
>> +int landlock_append_socket_rule(struct landlock_ruleset *const ruleset,
>> +				const int family, const int type,
>> +				access_mask_t access_rights)
>> +{
>> +	int err;
>> +
>> +	const struct landlock_id id = {
>> +		.key.data = pack_socket_key(family, type),
>> +		.type = LANDLOCK_KEY_SOCKET,
>> +	};
>> +
>> +	/* Transforms relative access rights to absolute ones. */
>> +	access_rights |= LANDLOCK_MASK_ACCESS_SOCKET &
>> +			 ~landlock_get_socket_access_mask(ruleset, 0);
>> +
>> +	mutex_lock(&ruleset->lock);
>> +	err = landlock_insert_rule(ruleset, id, access_rights);
>> +	mutex_unlock(&ruleset->lock);
>> +
>> +	return err;
>> +}
>> diff --git a/security/landlock/socket.h b/security/landlock/socket.h
>> new file mode 100644
>> index 000000000000..8519357f1c39
>> --- /dev/null
>> +++ b/security/landlock/socket.h
>> @@ -0,0 +1,17 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * Landlock LSM - Socket management and hooks
>> + *
>> + * Copyright © 2024 Huawei Tech. Co., Ltd.
>> + */
>> +
>> +#ifndef _SECURITY_LANDLOCK_SOCKET_H
>> +#define _SECURITY_LANDLOCK_SOCKET_H
>> +
>> +#include "ruleset.h"
>> +
>> +int landlock_append_socket_rule(struct landlock_ruleset *const ruleset,
>> +				const int family, const int type,
>> +				access_mask_t access_rights);
>> +
>> +#endif /* _SECURITY_LANDLOCK_SOCKET_H */
>> diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c
>> index 03b470f5a85a..30c771f5e74f 100644
>> --- a/security/landlock/syscalls.c
>> +++ b/security/landlock/syscalls.c
>> @@ -24,12 +24,14 @@
>>   #include <linux/syscalls.h>
>>   #include <linux/types.h>
>>   #include <linux/uaccess.h>
>> +#include <linux/net.h>
>>   #include <uapi/linux/landlock.h>
>>   
>>   #include "cred.h"
>>   #include "fs.h"
>>   #include "limits.h"
>>   #include "net.h"
>> +#include "socket.h"
>>   #include "ruleset.h"
>>   #include "setup.h"
>>   
>> @@ -88,7 +90,8 @@ static void build_check_abi(void)
>>   	struct landlock_ruleset_attr ruleset_attr;
>>   	struct landlock_path_beneath_attr path_beneath_attr;
>>   	struct landlock_net_port_attr net_port_attr;
>> -	size_t ruleset_size, path_beneath_size, net_port_size;
>> +	struct landlock_socket_attr socket_attr;
>> +	size_t ruleset_size, path_beneath_size, net_port_size, socket_size;
>>   
>>   	/*
>>   	 * For each user space ABI structures, first checks that there is no
>> @@ -97,8 +100,9 @@ static void build_check_abi(void)
>>   	 */
>>   	ruleset_size = sizeof(ruleset_attr.handled_access_fs);
>>   	ruleset_size += sizeof(ruleset_attr.handled_access_net);
>> +	ruleset_size += sizeof(ruleset_attr.handled_access_socket);
>>   	BUILD_BUG_ON(sizeof(ruleset_attr) != ruleset_size);
>> -	BUILD_BUG_ON(sizeof(ruleset_attr) != 16);
>> +	BUILD_BUG_ON(sizeof(ruleset_attr) != 24);
>>   
>>   	path_beneath_size = sizeof(path_beneath_attr.allowed_access);
>>   	path_beneath_size += sizeof(path_beneath_attr.parent_fd);
>> @@ -109,6 +113,12 @@ static void build_check_abi(void)
>>   	net_port_size += sizeof(net_port_attr.port);
>>   	BUILD_BUG_ON(sizeof(net_port_attr) != net_port_size);
>>   	BUILD_BUG_ON(sizeof(net_port_attr) != 16);
>> +
>> +	socket_size = sizeof(socket_attr.allowed_access);
>> +	socket_size += sizeof(socket_attr.family);
>> +	socket_size += sizeof(socket_attr.type);
>> +	BUILD_BUG_ON(sizeof(socket_attr) != socket_size);
>> +	BUILD_BUG_ON(sizeof(socket_attr) != 16);
>>   }
>>   
>>   /* Ruleset handling */
>> @@ -149,7 +159,7 @@ static const struct file_operations ruleset_fops = {
>>   	.write = fop_dummy_write,
>>   };
>>   
>> -#define LANDLOCK_ABI_VERSION 5
>> +#define LANDLOCK_ABI_VERSION 6
>>   
>>   /**
>>    * sys_landlock_create_ruleset - Create a new ruleset
>> @@ -213,9 +223,15 @@ SYSCALL_DEFINE3(landlock_create_ruleset,
>>   	    LANDLOCK_MASK_ACCESS_NET)
>>   		return -EINVAL;
>>   
>> +	/* Checks socket content (and 32-bits cast). */
>> +	if ((ruleset_attr.handled_access_socket |
>> +	     LANDLOCK_MASK_ACCESS_SOCKET) != LANDLOCK_MASK_ACCESS_SOCKET)
>> +		return -EINVAL;
>> +
>>   	/* Checks arguments and transforms to kernel struct. */
>>   	ruleset = landlock_create_ruleset(ruleset_attr.handled_access_fs,
>> -					  ruleset_attr.handled_access_net);
>> +					  ruleset_attr.handled_access_net,
>> +					  ruleset_attr.handled_access_socket);
>>   	if (IS_ERR(ruleset))
>>   		return PTR_ERR(ruleset);
>>   
>> @@ -371,6 +387,45 @@ static int add_rule_net_port(struct landlock_ruleset *ruleset,
>>   					net_port_attr.allowed_access);
>>   }
>>   
>> +static int add_rule_socket(struct landlock_ruleset *ruleset,
>> +			   const void __user *const rule_attr)
>> +{
>> +	struct landlock_socket_attr socket_attr;
>> +	int family, type;
>> +	int res;
>> +	access_mask_t mask;
>> +
>> +	/* Copies raw user space buffer. */
>> +	res = copy_from_user(&socket_attr, rule_attr, sizeof(socket_attr));
>> +	if (res)
>> +		return -EFAULT;
>> +
>> +	/*
>> +	 * Informs about useless rule: empty allowed_access (i.e. deny rules)
>> +	 * are ignored by socket actions.
>> +	 */
>> +	if (!socket_attr.allowed_access)
>> +		return -ENOMSG;
>> +
>> +	/* Checks that allowed_access matches the @ruleset constraints. */
>> +	mask = landlock_get_socket_access_mask(ruleset, 0);
>> +	if ((socket_attr.allowed_access | mask) != mask)
>> +		return -EINVAL;
>> +
>> +	family = socket_attr.family;
>> +	type = socket_attr.type;
>> +
>> +	/* Denies inserting a rule with unsupported socket family and type. */
>> +	if (family < 0 || family >= AF_MAX)
>> +		return -EINVAL;
>> +	if (type < 0 || type >= SOCK_MAX)
>> +		return -EINVAL;
> 
> enum sock_type (include/linux/net.h) has "holes": values 7, 8 and 9 are not
> defined in the header.  Should we check more specifically for the supported
> values here?  (Is there already a helper function for that?)

I think that a more detailed check of the family-type values may have a
good effect here, since the rules will contain real codes of families
and types.

I haven't found any helper to check the supported socket type value.
Performing a check inside landlock can lead to several minor problems,
which theoretically should not lead to any costs.

* There are would be a dependency with constants of enum sock_types. But
   we are unlikely to see new types of sockets in the next few years, so
   it wouldn't be a problem to maintain such check.

* enum sock_types can be redefined (see ARCH_HAS_SOCKET_TYPES in net.h),
   but i haven't found anyone to actually change the constants of socket
   types. It would be wrong to have a different landlock behavior for
   arch that redefines sock_types for some purposes, so probably this
   should also be maintained.

WDYT?

> 
> 
>> +	/* Imports the new rule. */
>> +	return landlock_append_socket_rule(ruleset, family, type,
>> +					   socket_attr.allowed_access);
>> +}
>> +
>>   /**
>>    * sys_landlock_add_rule - Add a new rule to a ruleset
>>    *
>> @@ -429,6 +484,9 @@ SYSCALL_DEFINE4(landlock_add_rule, const int, ruleset_fd,
>>   	case LANDLOCK_RULE_NET_PORT:
>>   		err = add_rule_net_port(ruleset, rule_attr);
>>   		break;
>> +	case LANDLOCK_RULE_SOCKET:
>> +		err = add_rule_socket(ruleset, rule_attr);
>> +		break;
>>   	default:
>>   		err = -EINVAL;
>>   		break;
>> diff --git a/tools/testing/selftests/landlock/base_test.c b/tools/testing/selftests/landlock/base_test.c
>> index 3c1e9f35b531..52b00472a487 100644
>> --- a/tools/testing/selftests/landlock/base_test.c
>> +++ b/tools/testing/selftests/landlock/base_test.c
>> @@ -75,7 +75,7 @@ TEST(abi_version)
>>   	const struct landlock_ruleset_attr ruleset_attr = {
>>   		.handled_access_fs = LANDLOCK_ACCESS_FS_READ_FILE,
>>   	};
>> -	ASSERT_EQ(5, landlock_create_ruleset(NULL, 0,
>> +	ASSERT_EQ(6, landlock_create_ruleset(NULL, 0,
>>   					     LANDLOCK_CREATE_RULESET_VERSION));
>>   
>>   	ASSERT_EQ(-1, landlock_create_ruleset(&ruleset_attr, 0,
>> -- 
>> 2.34.1
>>
> 
> —Günther

Re: [RFC PATCH v2 02/12] landlock: Add hook on socket creation

[Mikhail Ivanov]

5/27/2024 11:48 AM, Günther Noack wrote:
> Hello Mikhail!
> 
> Thanks for sending another revision of this patch set!
> 
> On Fri, May 24, 2024 at 05:30:05PM +0800, Mikhail Ivanov wrote:
>> Add hook to security_socket_post_create(), which checks whether the socket
>> type and family are allowed by domain. Hook is called after initializing
>> the socket in the network stack to not wrongfully return EACCES for a
>> family-type pair, which is considered invalid by the protocol.
>>
>> Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
> 
> ## Some observations that *do not* need to be addressed in this commit, IMHO:
> 
> get_raw_handled_socket_accesses, get_current_socket_domain and
> current_check_access_socket are based on the similarly-named functions from
> net.c (and fs.c), and it makes sense to stay consistent with these.
> 
> There are some possible refactorings that could maybe be applied to that code,
> but given that the same ones would apply to net.c as well, it's probably best to
> address these separately.
> 
>    * Should get_raw_handled_socket_accesses be inlined
It's a fairly simple and compact function, so compiler should inline it
without any problems. Mickaël was against optional inlines [1].

[1] 
https://lore.kernel.org/linux-security-module/5c6c99f7-4218-1f79-477e-5d943c9809fd@digikod.net/

>    * Does the WARN_ON_ONCE(dom->num_layers < 1) check have the right return code?

Looks like a rudimental check. `dom` is always NULL when `num_layers`< 1
(see get_*_domain functions).

>    * Can we refactor out commonalities (probably not worth it right now though)?

I had a few ideas about refactoring commonalities, as currently landlock
has several repetitive patterns in the code. But solution requires a
good design and a separate patch. Probably it's worth opening an issue
on github. WDYT?

> 
> 
> ## The only actionable feedback that I have that is specific to this commit is:
> 
> In the past, we have introduced new (non-test) Landlock functionality in a
> single commit -- that way, we have no "loose ends" in the code between these two
> commits, and that simplifies it for people who want to patch your feature onto
> other kernel trees.  (e.g. I think we should maybe merge commit 01/12 and 02/12
> into a single commit.)  WDYT?

Yeah, this two should be merged and tests commits as well. I just wanted
to do this in one of the latest patch versions to simplify code review.

> 
> —Günther

Re: [RFC PATCH v2 03/12] selftests/landlock: Add protocol.create to socket tests

[Mikhail Ivanov]

5/27/2024 6:27 PM, Günther Noack wrote:
> On Fri, May 24, 2024 at 05:30:06PM +0800, Mikhail Ivanov wrote:
>> Initiate socket_test.c selftests. Add protocol fixture for tests
>> with changeable family-type values. Only most common variants of
>> protocols (like ipv4-tcp,ipv6-udp, unix) were added.
>> Add simple socket access right checking test.
>>
>> Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
>> ---
>>
>> Changes since v1:
>> * Replaces test_socket_create() and socket_variant() helpers
>>    with test_socket().
>> * Renames domain to family in protocol fixture.
>> * Remove AF_UNSPEC fixture entry and add unspec_srv0 fixture field to
>>    check AF_UNSPEC socket creation case.
>> * Formats code with clang-format.
>> * Refactors commit message.
>> ---
>>   .../testing/selftests/landlock/socket_test.c  | 181 ++++++++++++++++++
>>   1 file changed, 181 insertions(+)
>>   create mode 100644 tools/testing/selftests/landlock/socket_test.c
>>
>> diff --git a/tools/testing/selftests/landlock/socket_test.c b/tools/testing/selftests/landlock/socket_test.c
>> new file mode 100644
>> index 000000000000..4c51f89ed578
>> --- /dev/null
>> +++ b/tools/testing/selftests/landlock/socket_test.c
>> @@ -0,0 +1,181 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Landlock tests - Socket
>> + *
>> + * Copyright © 2024 Huawei Tech. Co., Ltd.
>> + * Copyright © 2024 Microsoft Corporation
> 
> It looked to me like these patches came from Huawei?
> Was this left by accident?

Yeah, second line should be removed. Thanks!

> 
> 
>> + */
>> +
>> +#define _GNU_SOURCE
>> +
>> +#include <errno.h>
>> +#include <linux/landlock.h>
>> +#include <sched.h>
>> +#include <string.h>
>> +#include <sys/prctl.h>
>> +#include <sys/socket.h>
>> +
>> +#include "common.h"
>> +
>> +/* clang-format off */
>> +
>> +#define ACCESS_LAST LANDLOCK_ACCESS_SOCKET_CREATE
>> +
>> +#define ACCESS_ALL ( \
>> +	LANDLOCK_ACCESS_SOCKET_CREATE)
>> +
>> +/* clang-format on */
> 
> It does not look like clang-format would really mess up this format in a bad
> way.  Maybe we can remove the "clang-format off" section here and just write the
> "#define"s on one line?

You're right, I'll fix it

> 
> ACCESS_ALL is unused in this commit.
> Should it be introduced in a subsequent commit instead?

Indeed, thanks

> 
> 
>> +static int test_socket(const struct service_fixture *const srv)
>> +{
>> +	int fd;
>> +
>> +	fd = socket(srv->protocol.family, srv->protocol.type | SOCK_CLOEXEC, 0);
>> +	if (fd < 0)
>> +		return errno;
>> +	/*
>> +	 * Mixing error codes from close(2) and socket(2) should not lead to any
>> +	 * (access type) confusion for this test.
>> +	 */
>> +	if (close(fd) != 0)
>> +		return errno;
>> +	return 0;
>> +}
> 
> I personally find that it helps me remember if these test helpers have the same
> signature as the syscall that they are exercising.  (But I don't feel very
> strongly about it.  Just a suggestion.)

You're right, in this case test_socket() would be more clear.
I'll fix it.

> 
> 
>> [...]
>>
>> +TEST_F(protocol, create)
>> +{
>> +	const struct landlock_ruleset_attr ruleset_attr = {
>> +		.handled_access_socket = LANDLOCK_ACCESS_SOCKET_CREATE,
>> +	};
>> +	const struct landlock_socket_attr create_socket_attr = {
>> +		.allowed_access = LANDLOCK_ACCESS_SOCKET_CREATE,
>> +		.family = self->srv0.protocol.family,
>> +		.type = self->srv0.protocol.type,
>> +	};
>> +
>> +	int ruleset_fd;
>> +
>> +	/* Allowed create */
>> +	ruleset_fd =
>> +		landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
>> +	ASSERT_LE(0, ruleset_fd);
>> +
>> +	ASSERT_EQ(0, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_SOCKET,
>> +				       &create_socket_attr, 0));
>> +
>> +	enforce_ruleset(_metadata, ruleset_fd);
>> +	EXPECT_EQ(0, close(ruleset_fd));
>> +
>> +	ASSERT_EQ(0, test_socket(&self->srv0));
>> +	ASSERT_EQ(EAFNOSUPPORT, test_socket(&self->unspec_srv0));
>> +
>> +	/* Denied create */
>> +	ruleset_fd =
>> +		landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
>> +	ASSERT_LE(0, ruleset_fd);
>> +
>> +	enforce_ruleset(_metadata, ruleset_fd);
>> +	EXPECT_EQ(0, close(ruleset_fd));
>> +
>> +	ASSERT_EQ(EACCES, test_socket(&self->srv0));
>> +	ASSERT_EQ(EAFNOSUPPORT, test_socket(&self->unspec_srv0));
> 
> Should we exhaustively try out the other combinations (other than selv->srv0)
> here?  I assume socket() should always fail for these?

Do you mean testing all supported protocols? AFAICS this will require
adding ~80 FIXTURE_VARIANTs, but it won't be an issue if you think that
it can be useful.

> 
> (If you are alredy doing this in another commit that I have not looked at yet,
> please ignore this comment.)
> 
> —Günther

Re: [RFC PATCH v2 04/12] selftests/landlock: Add protocol.socket_access_rights to socket tests

[Mikhail Ivanov]

5/27/2024 11:52 PM, Günther Noack wrote:
> Hello!
> 
> I see that this test is adapted from the network_access_rights test in
> net_test.c, and some of the subsequent are similarly copied from there.  It
> makes it hard to criticize the code, because being a little bit consistent is
> probably a good thing.  Have you found any opportunities to extract
> commonalities into common.h?

I think that all common tests should be extracted to common.h or maybe
some new header. *_test.c could maintain a fixture for these tests for
some rule-specific logic. Such refactoring should be in separate patch
though.

> 	
> On Fri, May 24, 2024 at 05:30:07PM +0800, Mikhail Ivanov wrote:
>> Add test that checks possibility of adding rule with every possible
>> access right.
>>
>> Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
>> ---
>>
>> Changes since v1:
>> * Formats code with clang-format.
>> * Refactors commit message.
>> ---
>>   .../testing/selftests/landlock/socket_test.c  | 28 +++++++++++++++++++
>>   1 file changed, 28 insertions(+)
>>
>> diff --git a/tools/testing/selftests/landlock/socket_test.c b/tools/testing/selftests/landlock/socket_test.c
>> index 4c51f89ed578..eb5d62263460 100644
>> --- a/tools/testing/selftests/landlock/socket_test.c
>> +++ b/tools/testing/selftests/landlock/socket_test.c
>> @@ -178,4 +178,32 @@ TEST_F(protocol, create)
>>   	ASSERT_EQ(EAFNOSUPPORT, test_socket(&self->unspec_srv0));
>>   }
>>   
>> +TEST_F(protocol, socket_access_rights)
>> +{
>> +	const struct landlock_ruleset_attr ruleset_attr = {
>> +		.handled_access_socket = ACCESS_ALL,
>> +	};
>> +	struct landlock_socket_attr protocol = {
>> +		.family = self->srv0.protocol.family,
>> +		.type = self->srv0.protocol.type,
>> +	};
>> +	int ruleset_fd;
>> +	__u64 access;
>> +
>> +	ruleset_fd =
>> +		landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
>> +	ASSERT_LE(0, ruleset_fd);
>> +
>> +	for (access = 1; access <= ACCESS_LAST; access <<= 1) {
>> +		protocol.allowed_access = access;
>> +		EXPECT_EQ(0, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_SOCKET,
>> +					       &protocol, 0))
>> +		{
>> +			TH_LOG("Failed to add rule with access 0x%llx: %s",
>> +			       access, strerror(errno));
>> +		}
>> +	}
>> +	EXPECT_EQ(0, close(ruleset_fd));
> 
> Reviewed-by: Günther Noack <gnoack@google.com>
> 
> P.S. We are inconsistent with our use of EXPECT/ASSERT for test teardown.  The
> fs_test.c uses ASSERT_EQ in these places whereas net_test.c and your new tests
> use EXPECT_EQ.
> 
> It admittedly does not make much of a difference for close(), so should be OK.
> Some other selftests are even ignoring the result for close().  If we want to
> make it consistent in the Landlock tests again, we can also do it in an
> independent sweep.
> 
> I filed a small cleanup task as a reminder:
> https://github.com/landlock-lsm/linux/issues/31
> 
> —Günther

Re: [RFC PATCH v2 08/12] selftests/landlock: Add tcp_layers.ruleset_overlap to socket tests

[Mikhail Ivanov]

5/28/2024 12:09 AM, Günther Noack wrote:
> On Fri, May 24, 2024 at 05:30:11PM +0800, Mikhail Ivanov wrote:
>> * Add tcp_layers fixture for tests that check multiple layer
>>    configuration scenarios.
>>
>> * Add test that validates multiple layer behavior with overlapped
>>    restrictions.
>>
>> Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
>> ---
>>
>> Changes since v1:
>> * Replaces test_socket_create() with test_socket().
>> * Formats code with clang-format.
>> * Refactors commit message.
>> * Minor fixes.
>> ---
>>   .../testing/selftests/landlock/socket_test.c  | 109 ++++++++++++++++++
>>   1 file changed, 109 insertions(+)
>>
>> diff --git a/tools/testing/selftests/landlock/socket_test.c b/tools/testing/selftests/landlock/socket_test.c
>> index 751596c381fe..52edc1a8ac21 100644
>> --- a/tools/testing/selftests/landlock/socket_test.c
>> +++ b/tools/testing/selftests/landlock/socket_test.c
>> @@ -299,4 +299,113 @@ TEST_F(protocol, inval)
>>   				       &protocol, 0));
>>   }
>>   
>> +FIXTURE(tcp_layers)
>> +{
>> +	struct service_fixture srv0;
>> +};
>> +
>> +FIXTURE_VARIANT(tcp_layers)
>> +{
>> +	const size_t num_layers;
>> +};
>> +
>> +FIXTURE_SETUP(tcp_layers)
>> +{
>> +	const struct protocol_variant prot = {
>> +		.family = AF_INET,
>> +		.type = SOCK_STREAM,
>> +	};
>> +
>> +	disable_caps(_metadata);
>> +	self->srv0.protocol = prot;
>> +	setup_namespace(_metadata);
>> +};
>> +
>> +FIXTURE_TEARDOWN(tcp_layers)
>> +{
>> +}
>> +
>> +/* clang-format off */
>> +FIXTURE_VARIANT_ADD(tcp_layers, no_sandbox_with_ipv4) {
>> +	/* clang-format on */
>> +	.num_layers = 0,
>> +};
>> +
>> +/* clang-format off */
>> +FIXTURE_VARIANT_ADD(tcp_layers, one_sandbox_with_ipv4) {
>> +	/* clang-format on */
>> +	.num_layers = 1,
>> +};
>> +
>> +/* clang-format off */
>> +FIXTURE_VARIANT_ADD(tcp_layers, two_sandboxes_with_ipv4) {
>> +	/* clang-format on */
>> +	.num_layers = 2,
>> +};
>> +
>> +/* clang-format off */
>> +FIXTURE_VARIANT_ADD(tcp_layers, three_sandboxes_with_ipv4) {
>> +	/* clang-format on */
>> +	.num_layers = 3,
>> +};
>> +
>> +TEST_F(tcp_layers, ruleset_overlap)
>> +{
>> +	const struct landlock_ruleset_attr ruleset_attr = {
>> +		.handled_access_socket = LANDLOCK_ACCESS_SOCKET_CREATE,
>> +	};
>> +	const struct landlock_socket_attr tcp_create = {
>> +		.allowed_access = LANDLOCK_ACCESS_SOCKET_CREATE,
>> +		.family = self->srv0.protocol.family,
>> +		.type = self->srv0.protocol.type,
>> +	};
>> +
>> +	if (variant->num_layers >= 1) {
>> +		int ruleset_fd;
>> +
>> +		ruleset_fd = landlock_create_ruleset(&ruleset_attr,
>> +						     sizeof(ruleset_attr), 0);
>> +		ASSERT_LE(0, ruleset_fd);
>> +
>> +		/* Allows create. */
>> +		ASSERT_EQ(0, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_SOCKET,
>> +					       &tcp_create, 0));
>> +		enforce_ruleset(_metadata, ruleset_fd);
>> +		EXPECT_EQ(0, close(ruleset_fd));
>> +	}
>> +
>> +	if (variant->num_layers >= 2) {
>> +		int ruleset_fd;
>> +
>> +		/* Creates another ruleset layer with denied create. */
>> +		ruleset_fd = landlock_create_ruleset(&ruleset_attr,
>> +						     sizeof(ruleset_attr), 0);
>> +		ASSERT_LE(0, ruleset_fd);
>> +
>> +		enforce_ruleset(_metadata, ruleset_fd);
>> +		EXPECT_EQ(0, close(ruleset_fd));
>> +	}
>> +
>> +	if (variant->num_layers >= 3) {
>> +		int ruleset_fd;
>> +
>> +		/* Creates another ruleset layer. */
>> +		ruleset_fd = landlock_create_ruleset(&ruleset_attr,
>> +						     sizeof(ruleset_attr), 0);
>> +		ASSERT_LE(0, ruleset_fd);
>> +
>> +		/* Try to allow create second time. */
>> +		ASSERT_EQ(0, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_SOCKET,
>> +					       &tcp_create, 0));
>> +		enforce_ruleset(_metadata, ruleset_fd);
>> +		EXPECT_EQ(0, close(ruleset_fd));
>> +	}
>> +
>> +	if (variant->num_layers < 2) {
>> +		ASSERT_EQ(0, test_socket(&self->srv0));
>> +	} else {
>> +		ASSERT_EQ(EACCES, test_socket(&self->srv0));
>> +	}
>> +}
> 
> Wouldn't this be simpler if you did multiple checks in one test, in a sequence?
> 
>    * Expect that socket() works
>    * Enforce ruleset 1 with a rule
>    * Expect that socket() works
>    * Enforce ruleset 2 without a rule
>    * Expect that socket() fails
>    * Enforce ruleset 3
>    * Expect that socket() still fails
> 
> Then it would test the same and you would not need the fixture.
> If you extracted these if bodies above into helper functions,
> I think it would also read reasonably well.

I adapted this test from net_test.c and wanted it to remain similar to
the original. But I agree that such simplification is rational, probably
it's worth a little inconsistency.

Perhaps this test should be made common, like the tests that were
discussed earlier [1].

[1] 
https://lore.kernel.org/all/f4b5e2b9-e960-fd08-fdf4-328bb475e2ef@huawei-partners.com/

> 
> —Günther

Re: [RFC PATCH v2 07/12] selftests/landlock: Add protocol.inval to socket tests

[Mikhail Ivanov]

5/28/2024 12:27 AM, Günther Noack wrote:
> On Fri, May 24, 2024 at 05:30:10PM +0800, Mikhail Ivanov wrote:
>> Add test that validates behavior of landlock with fully
>> access restriction.
>>
>> Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
>> ---
>>
>> Changes since v1:
>> * Refactors commit message.
>> ---
>>   .../testing/selftests/landlock/socket_test.c  | 34 +++++++++++++++++++
>>   1 file changed, 34 insertions(+)
>>
>> diff --git a/tools/testing/selftests/landlock/socket_test.c b/tools/testing/selftests/landlock/socket_test.c
>> index 31af47de1937..751596c381fe 100644
>> --- a/tools/testing/selftests/landlock/socket_test.c
>> +++ b/tools/testing/selftests/landlock/socket_test.c
>> @@ -265,4 +265,38 @@ TEST_F(protocol, rule_with_unhandled_access)
>>   	EXPECT_EQ(0, close(ruleset_fd));
>>   }
>>   
>> +TEST_F(protocol, inval)
>> +{
>> +	const struct landlock_ruleset_attr ruleset_attr = {
>> +		.handled_access_socket = LANDLOCK_ACCESS_SOCKET_CREATE
>> +	};
>> +
>> +	struct landlock_socket_attr protocol = {
>> +		.allowed_access = LANDLOCK_ACCESS_SOCKET_CREATE,
>> +		.family = self->srv0.protocol.family,
>> +		.type = self->srv0.protocol.type,
>> +	};
>> +
>> +	struct landlock_socket_attr protocol_denied = {
>> +		.allowed_access = 0,
>> +		.family = self->srv0.protocol.family,
>> +		.type = self->srv0.protocol.type,
>> +	};
>> +
>> +	int ruleset_fd;
>> +
>> +	ruleset_fd =
>> +		landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
>> +	ASSERT_LE(0, ruleset_fd);
>> +
>> +	/* Checks zero access value. */
>> +	EXPECT_EQ(-1, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_SOCKET,
>> +					&protocol_denied, 0));
>> +	EXPECT_EQ(ENOMSG, errno);
>> +
>> +	/* Adds with legitimate values. */
>> +	ASSERT_EQ(0, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_SOCKET,
>> +				       &protocol, 0));
>> +}
>> +
>>   TEST_HARNESS_MAIN
>> -- 
>> 2.34.1
>>
> 
> Code is based on TEST_F(mini, inval) from net_test.c.  I see that you removed
> the check for unhandled allowed_access, because there is already a separate
> TEST_F(mini, rule_with_unhandled_access) for that.
> 
> That is true for the "legitimate value" case as well, though...?  We already
> have a test for that too.  Should that also get removed?

I thought that "legitimate value" case is needed to check that adding
a zero-access rule doesn't affect landlock behavior when adding correct
rules. Do you think it's not worth it?

> 
> Should we then rename the "inval" test to "rule_with_zero_access", so that the
> naming is consistent with the "rule_with_unhandled_access" test?

Definitely, thanks!

> 
> —Günther

Re: [RFC PATCH v2 00/12] Socket type control for Landlock

[Günther Noack]

On Fri, May 24, 2024 at 05:30:03PM +0800, Mikhail Ivanov wrote:
> Hello! This is v2 RFC patch dedicated to socket protocols restriction.
> 
> It is based on the landlock's mic-next branch on top of v6.9 kernel
> version.

Hello Mikhail!

I patched in your patchset and tried to use the feature with a small
demo tool, but I ran into what I think is a bug -- do you happen to
know what this might be?

I used 6.10-rc1 as a base and patched your patches on top.

The code is a small tool called "nonet", which does the following:

  - Disable socket creation with a Landlock ruleset with the following
    attributes:
  
    struct landlock_ruleset_attr attr = {
      .handled_access_socket = LANDLOCK_ACCESS_SOCKET_CREATE,
    };

  - open("/dev/null", O_WRONLY)

Expected result:

  - open() should work

Observed result:

  - open() fails with EACCES.

I traced this with perf, and found that the open() gets rejected from
Landlock's hook_file_open, whereas hook_socket_create does not get
invoked.  This is surprising to me -- Enabling a policy for socket
creation should not influence the outcome of opening files!

Tracing commands:

  sudo perf probe hook_socket_create '$params'
  sudo perf probe 'hook_file_open%return $retval'
  sudo perf record -e 'probe:*' -g -- ./nonet
  sudo perf report
 
You can find the tool in my landlock-examples repo in the nonet_bug branch:
https://github.com/gnoack/landlock-examples/blob/nonet_bug/nonet.c

Landlock is enabled like this:
https://github.com/gnoack/landlock-examples/blob/nonet_bug/sandbox_socket.c

Do you have a hunch what might be going on?

Thanks,
–Günther

Re: [RFC PATCH v2 01/12] landlock: Support socket access-control

[Günther Noack]

Hello!

On Thu, May 30, 2024 at 03:05:56PM +0300, Mikhail Ivanov wrote:
> 5/27/2024 12:57 PM, Günther Noack wrote:
> > On Fri, May 24, 2024 at 05:30:04PM +0800, Mikhail Ivanov wrote:
> > > +/**
> > > + * struct landlock_socket_attr - Socket definition
> > > + *
> > > + * Argument of sys_landlock_add_rule().
> > > + */
> > > +struct landlock_socket_attr {
> > > +	/**
> > > +	 * @allowed_access: Bitmask of allowed access for a socket
> > > +	 * (cf. `Socket flags`_).
> > > +	 */
> > > +	__u64 allowed_access;
> > > +	/**
> > > +	 * @family: Protocol family used for communication
> > > +	 * (same as domain in socket(2)).
> > > +	 */
> > > +	int family;
> > > +	/**
> > > +	 * @type: Socket type (see socket(2)).
> > > +	 */
> > > +	int type;
> > > +};
> > 
> > Regarding the naming of struct landlock_socket_attr and the associated
> > LANDLOCK_RULE_SOCKET enum:
> > 
> > For the two existing rule types LANDLOCK_RULE_PATH_BENEATH (struct
> > landlock_path_beneath_attr) and LANDLOCK_RULE_NET_PORT (struct
> > landlock_net_port_attr), the names of the rule types are describing the
> > *properties* by which we are filtering (path *beneath*, *network port*), rather
> > than just the kind of object that we are filtering on.
> > 
> > Should the new enum and struct maybe be called differently as well to match that
> > convention?  Maybe LANDLOCK_RULE_SOCKET_FAMILY_TYPE and struct
> > landlock_socket_family_type_attr?
> > 
> > Are there *other* properties apart from family and type, by which you are
> > thinking of restricting the use of sockets in the future?
> 
> There was a thought about adding `protocol` (socket(2)) restriction,
> but Mickaël noted that it would be useless [1]. Therefore, no other
> properties are planned until someone has good use cases.
> 
> I agree that current naming can be associated with socket objects. But i
> don't think using family-type words for naming of this rule would be
> convenient for users. In comparison with net port and path beneath
> family-type pair doesn't represent a single semantic unit, so it would
> be a little harder to read the code.
> 
> Perhaps LANDLOCK_RULE_SOCKET_PROTO (struct landlock_socket_proto_attr)
> would be more suitable here? Although socket(2) has `protocol` argument
> to specify the socket protocol in some cases (e.g. RAW sockets), in most
> cases family-type pair defines protocol itself. Since the purpose of
> this patchlist is to restrict protocols used in a sandboxed process, I
> think that in the presence of well-written documentation, such naming
> may be appropriate here. WDYT?
> 
> [1]
> https://lore.kernel.org/all/a6318388-e28a-e96f-b1ae-51948c13de4d@digikod.net/

It is difficult, I also can't come up with a much better name.  In doubt, we
could stick with what you already have, I think.

LANDLOCK_RULE_SOCKET_PROTO alludes to "protocol" and even though that is the
general term, it can be confused with the third argument to socket(2), which is
also called "protocol" and is rarely used.

Mickaël, do you have any opinions on the naming of this?


> > (More about the content)
> > 
> > The Landlock documentation states the general approach up front:
> > 
> >    A Landlock rule describes an *action* on an *object* which the process intends
> >    to perform.
> > 
> > (In your case, the object is a socket, and the action is the socket's creation.
> > The Landlock rules describe predicates on objects to restrict the set of actions
> > through the access_mask_t.)
> > 
> > The implementation is perfectly in line with that, but it would help to phrase
> > the documentation also in terms of that framework.  That means, what we are
> > restricting are *actions*, not protocols.
> > 
> > To make a more constructive suggestion:
> > 
> >    "These flags restrict actions on sockets for a sandboxed process (e.g. socket
> >    creation)."
> 
> I think this has too general meaning (e.g. bind(2) is also an action on
> socket). Probably this one would be more suitable:
> 
>   "These flags restrict actions of adding sockets in a sandboxed
>   process (e.g. socket creation, passing socket FDs to/from the
>   process)."

Sounds good.  (Although I would not give "passing socket FDs to/from the
process" as an example, as long as it's not supported yet.)


> > > + * - %LANDLOCK_ACCESS_SOCKET_CREATE: Create a socket.
> > 
> > Can we be more specific here what operations are affected by this?  It is rather
> > obvious that this affects socket(2), but does this also affect accept(2) and
> > connect(2)?
> > 
> > A scenario that I could imagine being useful is to sandbox a TCP server like
> > this:
> > 
> >    * create a socket, bind(2) and listen(2)
> >    * sandbox yourself so that no new sockets can be created with socket(2)
> >    * go into the main loop and start accept(2)ing new connections
> > 
> > Is this an approach that would work with this patch set?
> 
> Yes, such scenario is possible. This rule should apply to all socket
> creation requests in the user space (socket(2), socketpair(2), io_uring
> request). Perhaps it's necessary to clarify here that only user space
> sockets are restricted?
> 
> Btw, current implementation doesn't check that the socket creation
> request doesn't come from the kernel space. Will be fixed.

Two brief side discussions:

* What are the scenarios where that creation request comes from kernel space?
  If this is used under the hood for network-backed file systems like NFS, can
  this result in surprising interactions when the program tries to access the
  file system?

* To be clear, I think it would be useful to support the scenario above, where
  accept() continues to work. - It would make it easy to create sandboxed server
  processes and they could still accept connections, but do no other networking.

But to bring it back to my original remark, and to unblock progress:

I think for this patch set (focused on userspace-requested socket creation), it
would be enough to clarify in the documentation which operations are affected by
the LANDLOCK_ACCESS_SOCKET_CREATE right.


> > (It might make a neat sample tool as well, if something like this works :))
> > 
> > 
> > Regarding the list of socket access rights with only one item in it:
> > 
> > I am still unsure what other socket actions are in scope in the future; it would
> > probably help to phrase the documentation in those terms.  (listen(2), bind(2),
> > connect(2), shutdown(2)?  On the other hand, bind(2) and connect(2) for TCP are
> > already restrictable differently.))
> 
> I think it would be useful to restrict sending and receiving socket
> FDs via unix domain sockets (see SCM_RIGHTS in unix(7)).

That seems like a reasonable idea.  Would you like to file an issue on the
Landlock bugtracker about it?

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


> > > +	/* Checks that all supported socket families and types can be stored in socket_key. */
> > > +	BUILD_BUG_ON(AF_MAX > (typeof(socket_key.data.family))~0);
> > > +	BUILD_BUG_ON(SOCK_MAX > (typeof(socket_key.data.type))~0);
> > 
> > Off-by-one nit: AF_MAX and SOCK_MAX are one higher than the last permitted value,
> > so technically it would be ok if they are one higher than (unsigned short)~0.

(Did you see this remark?)


> > I see that this function traces back to Mickaël's comment in
> > https://lore.kernel.org/all/20240412.phoh7laim7Th@digikod.net/
> > 
> > In my understanding, the motivation was to keep the key size in check.
> > But that does not mean that we need to turn it into a uintptr_t?
> > 
> > Would it not have been possible to extend the union landlock_key in ruleset.h
> > with a
> > 
> >    struct {
> >      unsigned short family, type;
> >    }
> > 
> > and then do the AF_MAX, SOCK_MAX build-time checks on that?
> > It seems like that might be more in line with what we already have?
> 
> I don't think that complicating general entity with such a specific
> representation would be a good solution here. `landlock_key` shouldn't
> contain any semantic information about the key content.

Hm, OK.  I think that is debatable, but these are all things that are
implementation details and can be changed later if needed.  Sounds good to me if
we fix the undefined behaviour in the key calculation.


> > > +	/* Denies inserting a rule with unsupported socket family and type. */
                                        ^^^^^^^^^^^^^^^^^^^^^^^^^

Is the wording "unsupported socket family" misleading here?

(a) It is technically a "protocol family" and a "socket type", according to
    socket(2). (BTW, the exact delineation between a "protocol family" and an
    "address family" is not clear to me.)

(b) "unsupported" in the context of protocol families may mean that the kernel
    does not know how to speak that protocol, which is slightly different than
    saying that it's outside of the [0, AF_MAX) range.  If we wanted to check
    for the protocol family being "supported", we should also probably return
    -EAFNOSUPPORT, similar to what we already return when adding a "port" rule
    with the wrong protocol [1]?

    [1] https://docs.kernel.org/userspace-api/landlock.html#extending-a-ruleset

I suspect that -EINVAL is slightly more correct here, because this is not about
the protocols that the kernel supports, but only about the range.  If we wanted
to return errors about the protocol that the kernel supports, I realized that
we'd probably also have to check whether the *combination* of family and type
makes sense.  In my understanding, the equivalent errors for type and protocol,
ESOCKTNOSUPPORT and EPROTONOSUPPORT, only get returned based on whether they
make sense together with the other values.


> > > +	if (family < 0 || family >= AF_MAX)
> > > +		return -EINVAL;
> > > +	if (type < 0 || type >= SOCK_MAX)
> > > +		return -EINVAL;
> > 
> > enum sock_type (include/linux/net.h) has "holes": values 7, 8 and 9 are not
> > defined in the header.  Should we check more specifically for the supported
> > values here?  (Is there already a helper function for that?)
> 
> I think that a more detailed check of the family-type values may have a
> good effect here, since the rules will contain real codes of families
> and types.
> 
> I haven't found any helper to check the supported socket type value.
> Performing a check inside landlock can lead to several minor problems,
> which theoretically should not lead to any costs.
> 
> * There are would be a dependency with constants of enum sock_types. But
>   we are unlikely to see new types of sockets in the next few years, so
>   it wouldn't be a problem to maintain such check.
> 
> * enum sock_types can be redefined (see ARCH_HAS_SOCKET_TYPES in net.h),
>   but i haven't found anyone to actually change the constants of socket
>   types. It would be wrong to have a different landlock behavior for
>   arch that redefines sock_types for some purposes, so probably this
>   should also be maintained.
> 
> WDYT?

Thinking about it again, from a Landlock safety perspective, I believe it is
safe to keep the checks as they are and to check for the two values to be in the
ranges [0, AF_MAX) and [0, SOCK_MAX).

Even if we permit the rule to be added for an invalid socket type, there does
not seem to be any harm in that, as these sockets can't be created anyway.
Also, given the semantics of these errors in socket(2), where also the
*combinations* of the values are checked, it seems overly complicated to check
all these combinations.  I think it would be fine to keep as is, I was mostly
wondering whether you had done any deeper analysis?

It might be worth spelling out in the struct documentation that the values which
fulfil 0 <= family < AF_MAX and 0 <= type < SOCK_MAX are considered valid.  Does
that sound reasonable?

P.S., it seems that the security/apparmor/Makefile is turning the "#define"s
into C code with lookup tables, but it seems that this is only used for
human-readable audit-logging, not for validating the policies.

—Günther

Re: [RFC PATCH v2 02/12] landlock: Add hook on socket creation

[Günther Noack]

Hello!

On Thu, May 30, 2024 at 03:20:21PM +0300, Mikhail Ivanov wrote:
> 5/27/2024 11:48 AM, Günther Noack wrote:
> > On Fri, May 24, 2024 at 05:30:05PM +0800, Mikhail Ivanov wrote:
> > > Add hook to security_socket_post_create(), which checks whether the socket
> > > type and family are allowed by domain. Hook is called after initializing
> > > the socket in the network stack to not wrongfully return EACCES for a
> > > family-type pair, which is considered invalid by the protocol.
> > > 
> > > Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
> > 
> > ## Some observations that *do not* need to be addressed in this commit, IMHO:
> > 
> > get_raw_handled_socket_accesses, get_current_socket_domain and
> > current_check_access_socket are based on the similarly-named functions from
> > net.c (and fs.c), and it makes sense to stay consistent with these.
> > 
> > There are some possible refactorings that could maybe be applied to that code,
> > but given that the same ones would apply to net.c as well, it's probably best to
> > address these separately.
> > 
> >    * Should get_raw_handled_socket_accesses be inlined
> It's a fairly simple and compact function, so compiler should inline it
> without any problems. Mickaël was against optional inlines [1].
> 
> [1] https://lore.kernel.org/linux-security-module/5c6c99f7-4218-1f79-477e-5d943c9809fd@digikod.net/

Sorry for the confusion -- what I meant was not "should we add the inline
keyword", but I meant "should we remove that function and place its
implementation in the place where we are currently calling it"?


> >    * Does the WARN_ON_ONCE(dom->num_layers < 1) check have the right return code?
> 
> Looks like a rudimental check. `dom` is always NULL when `num_layers`< 1
> (see get_*_domain functions).

What I found irritating about it is that with 0 layers (= no Landlock policy was
ever enabled), you would logically assume that we return a success?  But then I
realized that this code was copied verbatim from other places in fs.c and net.c,
and it is actually checking for an internal inconsistency that is never supposed
to happen.  If we were to actually hit that case at some point, we have probably
stumbled over our own feet and it might be better to not permit anything.


> >    * Can we refactor out commonalities (probably not worth it right now though)?
> 
> I had a few ideas about refactoring commonalities, as currently landlock
> has several repetitive patterns in the code. But solution requires a
> good design and a separate patch. Probably it's worth opening an issue
> on github. WDYT?

Absolutely, please do open one.  In my mind, patches in C which might not get
accepted are an expensive way to iterate on such ideas, and it might make sense
to collect some refactoring approaches on a bug or the mailing list before
jumping into the implementation.

(You might want to keep an eye on https://github.com/landlock-lsm/linux/issues/1
as well, which is about some ideas to refactor Landlock's internal data
structures.)


> > ## The only actionable feedback that I have that is specific to this commit is:
> > 
> > In the past, we have introduced new (non-test) Landlock functionality in a
> > single commit -- that way, we have no "loose ends" in the code between these two
> > commits, and that simplifies it for people who want to patch your feature onto
> > other kernel trees.  (e.g. I think we should maybe merge commit 01/12 and 02/12
> > into a single commit.)  WDYT?
> 
> Yeah, this two should be merged and tests commits as well. I just wanted
> to do this in one of the latest patch versions to simplify code review.

That sounds good, thanks!

—Günther

Re: [RFC PATCH v2 00/12] Socket type control for Landlock

[Mikhail Ivanov]

6/4/2024 11:22 PM, Günther Noack wrote:
> On Fri, May 24, 2024 at 05:30:03PM +0800, Mikhail Ivanov wrote:
>> Hello! This is v2 RFC patch dedicated to socket protocols restriction.
>>
>> It is based on the landlock's mic-next branch on top of v6.9 kernel
>> version.
> 
> Hello Mikhail!
> 
> I patched in your patchset and tried to use the feature with a small
> demo tool, but I ran into what I think is a bug -- do you happen to
> know what this might be?
> 
> I used 6.10-rc1 as a base and patched your patches on top.
> 
> The code is a small tool called "nonet", which does the following:
> 
>    - Disable socket creation with a Landlock ruleset with the following
>      attributes:
>    
>      struct landlock_ruleset_attr attr = {
>        .handled_access_socket = LANDLOCK_ACCESS_SOCKET_CREATE,
>      };
> 
>    - open("/dev/null", O_WRONLY)
> 
> Expected result:
> 
>    - open() should work
> 
> Observed result:
> 
>    - open() fails with EACCES.
> 
> I traced this with perf, and found that the open() gets rejected from
> Landlock's hook_file_open, whereas hook_socket_create does not get
> invoked.  This is surprising to me -- Enabling a policy for socket
> creation should not influence the outcome of opening files!
> 
> Tracing commands:
> 
>    sudo perf probe hook_socket_create '$params'
>    sudo perf probe 'hook_file_open%return $retval'
>    sudo perf record -e 'probe:*' -g -- ./nonet
>    sudo perf report
>   
> You can find the tool in my landlock-examples repo in the nonet_bug branch:
> https://github.com/gnoack/landlock-examples/blob/nonet_bug/nonet.c
> 
> Landlock is enabled like this:
> https://github.com/gnoack/landlock-examples/blob/nonet_bug/sandbox_socket.c
> 
> Do you have a hunch what might be going on?

Hello Günther!
Big thanks for this research!

I figured out that I define LANDLOCK_SHIFT_ACCESS_SOCKET macro in
really strange way (see landlock/limits.h):

   #define LANDLOCK_SHIFT_ACCESS_SOCKET	LANDLOCK_NUM_ACCESS_SOCKET

With this definition, socket access mask overlaps the fs access
mask in ruleset->access_masks[layer_level]. That's why
landlock_get_fs_access_mask() returns non-zero mask in hook_file_open().

So, the macro must be defined in this way:

   #define LANDLOCK_SHIFT_ACCESS_SOCKET	(LANDLOCK_NUM_ACCESS_NET +
                                          LANDLOCK_NUM_ACCESS_FS)

With this fix, open() doesn't fail in your example.

I'm really sorry that I somehow made such a stupid typo. I will try my
best to make sure this doesn't happen again.

> 
> Thanks,
> –Günther
> 

Re: [RFC PATCH v2 00/12] Socket type control for Landlock

[Günther Noack]

Hello Mikhail!

On Thu, Jun 06, 2024 at 02:44:23PM +0300, Mikhail Ivanov wrote:
> 6/4/2024 11:22 PM, Günther Noack wrote:
> > On Fri, May 24, 2024 at 05:30:03PM +0800, Mikhail Ivanov wrote:
> > > Hello! This is v2 RFC patch dedicated to socket protocols restriction.
> > > 
> > > It is based on the landlock's mic-next branch on top of v6.9 kernel
> > > version.
> > 
> > Hello Mikhail!
> > 
> > I patched in your patchset and tried to use the feature with a small
> > demo tool, but I ran into what I think is a bug -- do you happen to
> > know what this might be?
> > 
> > I used 6.10-rc1 as a base and patched your patches on top.
> > 
> > The code is a small tool called "nonet", which does the following:
> > 
> >    - Disable socket creation with a Landlock ruleset with the following
> >      attributes:
> >      struct landlock_ruleset_attr attr = {
> >        .handled_access_socket = LANDLOCK_ACCESS_SOCKET_CREATE,
> >      };
> > 
> >    - open("/dev/null", O_WRONLY)
> > 
> > Expected result:
> > 
> >    - open() should work
> > 
> > Observed result:
> > 
> >    - open() fails with EACCES.
> > 
> > I traced this with perf, and found that the open() gets rejected from
> > Landlock's hook_file_open, whereas hook_socket_create does not get
> > invoked.  This is surprising to me -- Enabling a policy for socket
> > creation should not influence the outcome of opening files!
> > 
> > Tracing commands:
> > 
> >    sudo perf probe hook_socket_create '$params'
> >    sudo perf probe 'hook_file_open%return $retval'
> >    sudo perf record -e 'probe:*' -g -- ./nonet
> >    sudo perf report
> > You can find the tool in my landlock-examples repo in the nonet_bug branch:
> > https://github.com/gnoack/landlock-examples/blob/nonet_bug/nonet.c
> > 
> > Landlock is enabled like this:
> > https://github.com/gnoack/landlock-examples/blob/nonet_bug/sandbox_socket.c
> > 
> > Do you have a hunch what might be going on?
> 
> Hello Günther!
> Big thanks for this research!
> 
> I figured out that I define LANDLOCK_SHIFT_ACCESS_SOCKET macro in
> really strange way (see landlock/limits.h):
> 
>   #define LANDLOCK_SHIFT_ACCESS_SOCKET	LANDLOCK_NUM_ACCESS_SOCKET
> 
> With this definition, socket access mask overlaps the fs access
> mask in ruleset->access_masks[layer_level]. That's why
> landlock_get_fs_access_mask() returns non-zero mask in hook_file_open().
> 
> So, the macro must be defined in this way:
> 
>   #define LANDLOCK_SHIFT_ACCESS_SOCKET	(LANDLOCK_NUM_ACCESS_NET +
>                                          LANDLOCK_NUM_ACCESS_FS)
> 
> With this fix, open() doesn't fail in your example.
> 
> I'm really sorry that I somehow made such a stupid typo. I will try my
> best to make sure this doesn't happen again.

Thanks for figuring it out so quickly.  With that change, I'm getting some
compilation errors (some bit shifts are becoming too wide for the underlying
types), but I'm sure you can address that easily for the next version of the
patch set.

IMHO this shows that our reliance on bit manipulation is probably getting in the
way of code clarity. :-/ I hope we can simplify these internal structures at
some point.  Once we have a better way to check for performance changes [1], we
can try to change this and measure whether these comprehensibility/performance
tradeoff is really worth it.

[1] https://github.com/landlock-lsm/linux/issues/24

The other takeaway in my mind is, we should probably have some tests for that,
to check that the enablement of one kind of policy does not affect the
operations that belong to other kinds of policies.  Like this, for instance (I
was about to send this test to help debugging):

TEST_F(mini, restricting_socket_does_not_affect_fs_actions)
{
	const struct landlock_ruleset_attr ruleset_attr = {
		.handled_access_socket = LANDLOCK_ACCESS_SOCKET_CREATE,
	};
	int ruleset_fd, fd;

	ruleset_fd = landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
	ASSERT_LE(0, ruleset_fd);

	enforce_ruleset(_metadata, ruleset_fd);
	ASSERT_EQ(0, close(ruleset_fd));

	/*
	 * Accessing /dev/null for writing should be permitted,
	 * because we did not add any file system restrictions.
	 */
	fd = open("/dev/null", O_WRONLY);
	EXPECT_LE(0, fd);

	ASSERT_EQ(0, close(fd));
}

Since these kinds of tests are a bit at the intersection between the
fs/net/socket tests, maybe they could go into a separate test file?  The next
time we add a new kind of Landlock restriction, it would come more naturally to
add the matching test there and spot such issues earlier.  Would you volunteer
to add such a test as part of your patch set? :)

Thanks,
Günther

Re: [RFC PATCH v2 00/12] Socket type control for Landlock

[Günther Noack]

On Thu, Jun 06, 2024 at 03:32:47PM +0200, Günther Noack wrote:
> Thanks for figuring it out so quickly.  With that change, I'm getting some
> compilation errors (some bit shifts are becoming too wide for the underlying
> types), but I'm sure you can address that easily for the next version of the
> patch set.

Addendum, please ignore the remark about me getting compilation errors - I made
a typo myself, and it worked in the way you suggested without warnings or
errors.

—Günther

Re: [RFC PATCH v2 01/12] landlock: Support socket access-control

[Mikhail Ivanov]

6/5/2024 8:04 PM, Günther Noack wrote:
> Hello!
> 
> On Thu, May 30, 2024 at 03:05:56PM +0300, Mikhail Ivanov wrote:
>> 5/27/2024 12:57 PM, Günther Noack wrote:
>>> On Fri, May 24, 2024 at 05:30:04PM +0800, Mikhail Ivanov wrote:
>>>> +/**
>>>> + * struct landlock_socket_attr - Socket definition
>>>> + *
>>>> + * Argument of sys_landlock_add_rule().
>>>> + */
>>>> +struct landlock_socket_attr {
>>>> +	/**
>>>> +	 * @allowed_access: Bitmask of allowed access for a socket
>>>> +	 * (cf. `Socket flags`_).
>>>> +	 */
>>>> +	__u64 allowed_access;
>>>> +	/**
>>>> +	 * @family: Protocol family used for communication
>>>> +	 * (same as domain in socket(2)).
>>>> +	 */
>>>> +	int family;
>>>> +	/**
>>>> +	 * @type: Socket type (see socket(2)).
>>>> +	 */
>>>> +	int type;
>>>> +};
>>>
>>> Regarding the naming of struct landlock_socket_attr and the associated
>>> LANDLOCK_RULE_SOCKET enum:
>>>
>>> For the two existing rule types LANDLOCK_RULE_PATH_BENEATH (struct
>>> landlock_path_beneath_attr) and LANDLOCK_RULE_NET_PORT (struct
>>> landlock_net_port_attr), the names of the rule types are describing the
>>> *properties* by which we are filtering (path *beneath*, *network port*), rather
>>> than just the kind of object that we are filtering on.
>>>
>>> Should the new enum and struct maybe be called differently as well to match that
>>> convention?  Maybe LANDLOCK_RULE_SOCKET_FAMILY_TYPE and struct
>>> landlock_socket_family_type_attr?
>>>
>>> Are there *other* properties apart from family and type, by which you are
>>> thinking of restricting the use of sockets in the future?
>>
>> There was a thought about adding `protocol` (socket(2)) restriction,
>> but Mickaël noted that it would be useless [1]. Therefore, no other
>> properties are planned until someone has good use cases.
>>
>> I agree that current naming can be associated with socket objects. But i
>> don't think using family-type words for naming of this rule would be
>> convenient for users. In comparison with net port and path beneath
>> family-type pair doesn't represent a single semantic unit, so it would
>> be a little harder to read the code.
>>
>> Perhaps LANDLOCK_RULE_SOCKET_PROTO (struct landlock_socket_proto_attr)
>> would be more suitable here? Although socket(2) has `protocol` argument
>> to specify the socket protocol in some cases (e.g. RAW sockets), in most
>> cases family-type pair defines protocol itself. Since the purpose of
>> this patchlist is to restrict protocols used in a sandboxed process, I
>> think that in the presence of well-written documentation, such naming
>> may be appropriate here. WDYT?
>>
>> [1]
>> https://lore.kernel.org/all/a6318388-e28a-e96f-b1ae-51948c13de4d@digikod.net/
> 
> It is difficult, I also can't come up with a much better name.  In doubt, we
> could stick with what you already have, I think.
> 
> LANDLOCK_RULE_SOCKET_PROTO alludes to "protocol" and even though that is the
> general term, it can be confused with the third argument to socket(2), which is
> also called "protocol" and is rarely used.
> 
> Mickaël, do you have any opinions on the naming of this?
> 
> 
>>> (More about the content)
>>>
>>> The Landlock documentation states the general approach up front:
>>>
>>>     A Landlock rule describes an *action* on an *object* which the process intends
>>>     to perform.
>>>
>>> (In your case, the object is a socket, and the action is the socket's creation.
>>> The Landlock rules describe predicates on objects to restrict the set of actions
>>> through the access_mask_t.)
>>>
>>> The implementation is perfectly in line with that, but it would help to phrase
>>> the documentation also in terms of that framework.  That means, what we are
>>> restricting are *actions*, not protocols.
>>>
>>> To make a more constructive suggestion:
>>>
>>>     "These flags restrict actions on sockets for a sandboxed process (e.g. socket
>>>     creation)."
>>
>> I think this has too general meaning (e.g. bind(2) is also an action on
>> socket). Probably this one would be more suitable:
>>
>>    "These flags restrict actions of adding sockets in a sandboxed
>>    process (e.g. socket creation, passing socket FDs to/from the
>>    process)."
> 
> Sounds good.  (Although I would not give "passing socket FDs to/from the
> process" as an example, as long as it's not supported yet.)

Agreed, thanks

> 
> 
>>>> + * - %LANDLOCK_ACCESS_SOCKET_CREATE: Create a socket.
>>>
>>> Can we be more specific here what operations are affected by this?  It is rather
>>> obvious that this affects socket(2), but does this also affect accept(2) and
>>> connect(2)?
>>>
>>> A scenario that I could imagine being useful is to sandbox a TCP server like
>>> this:
>>>
>>>     * create a socket, bind(2) and listen(2)
>>>     * sandbox yourself so that no new sockets can be created with socket(2)
>>>     * go into the main loop and start accept(2)ing new connections
>>>
>>> Is this an approach that would work with this patch set?
>>
>> Yes, such scenario is possible. This rule should apply to all socket
>> creation requests in the user space (socket(2), socketpair(2), io_uring
>> request). Perhaps it's necessary to clarify here that only user space
>> sockets are restricted?
>>
>> Btw, current implementation doesn't check that the socket creation
>> request doesn't come from the kernel space. Will be fixed.
> 
> Two brief side side discussions:
> 
> * What are the scenarios where that creation request comes from kernel space?
>    If this is used under the hood for network-backed file systems like NFS, can
>    this result in surprising interactions when the program tries to access the
>    file system?

Yes, i've figured out that NFS can use __sock_create() kernel method for
allocating per-client socket. If kernel allocation of sockets would be
restricted by Landlock that NFS client allocation may fail and proccess
won't be able to connect to the NFS server.

Some of the socket protocols uses kernel space methods like
sock_create_lite() or sock_create_kern() for their own needs.
For example netlink fou family uses sock_create_kern() for UDP
tunneling.

Landlock shouldn't allow to indirectly change the behavior of such
mechanisms from user space.

> 
> * To be clear, I think it would be useful to support the scenario above, where
>    accept() continues to work. - It would make it easy to create sandboxed server
>    processes and they could still accept connections, but do no other networking.

Yes, accept() should work with any restrictions.

> 
> But to bring it back to my original remark, and to unblock progress:
> 
> I think for this patch set (focused on userspace-requested socket creation), it
> would be enough to clarify in the documentation which operations are affected by
> the LANDLOCK_ACCESS_SOCKET_CREATE right.

Ok, I'll do it.

> 
> 
>>> (It might make a neat sample tool as well, if something like this works :))
>>>
>>>
>>> Regarding the list of socket access rights with only one item in it:
>>>
>>> I am still unsure what other socket actions are in scope in the future; it would
>>> probably help to phrase the documentation in those terms.  (listen(2), bind(2),
>>> connect(2), shutdown(2)?  On the other hand, bind(2) and connect(2) for TCP are
>>> already restrictable differently.))
>>
>> I think it would be useful to restrict sending and receiving socket
>> FDs via unix domain sockets (see SCM_RIGHTS in unix(7)).
> 
> That seems like a reasonable idea.  Would you like to file an issue on the
> Landlock bugtracker about it?
> 
> https://github.com/landlock-lsm/linux/issues

Ofc: https://github.com/landlock-lsm/linux/issues/33.

> 
> 
>>>> +	/* Checks that all supported socket families and types can be stored in socket_key. */
>>>> +	BUILD_BUG_ON(AF_MAX > (typeof(socket_key.data.family))~0);
>>>> +	BUILD_BUG_ON(SOCK_MAX > (typeof(socket_key.data.type))~0);
>>>
>>> Off-by-one nit: AF_MAX and SOCK_MAX are one higher than the last permitted value,
>>> so technically it would be ok if they are one higher than (unsigned short)~0.
> 
> (Did you see this remark?)

Yeah, sorry I've somehow lost my reply. This nit will be fixed.

> 
> 
>>> I see that this function traces back to Mickaël's comment in
>>> https://lore.kernel.org/all/20240412.phoh7laim7Th@digikod.net/
>>>
>>> In my understanding, the motivation was to keep the key size in check.
>>> But that does not mean that we need to turn it into a uintptr_t?
>>>
>>> Would it not have been possible to extend the union landlock_key in ruleset.h
>>> with a
>>>
>>>     struct {
>>>       unsigned short family, type;
>>>     }
>>>
>>> and then do the AF_MAX, SOCK_MAX build-time checks on that?
>>> It seems like that might be more in line with what we already have?
>>
>> I don't think that complicating general entity with such a specific
>> representation would be a good solution here. `landlock_key` shouldn't
>> contain any semantic information about the key content.
> 
> Hm, OK.  I think that is debatable, but these are all things that are
> implementation details and can be changed later if needed.  Sounds good to me if
> we fix the undefined behaviour in the key calculation.

Ok, agreed! Mikael can probably give his opinion on this.

> 
> 
>>>> +	/* Denies inserting a rule with unsupported socket family and type. */
>                                          ^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> Is the wording "unsupported socket family" misleading here?
> 
> (a) It is technically a "protocol family" and a "socket type", according to
>      socket(2). (BTW, the exact delineation between a "protocol family" and an
>      "address family" is not clear to me.)
> 
> (b) "unsupported" in the context of protocol families may mean that the kernel
>      does not know how to speak that protocol, which is slightly different than
>      saying that it's outside of the [0, AF_MAX) range.  If we wanted to check
>      for the protocol family being "supported", we should also probably return
>      -EAFNOSUPPORT, similar to what we already return when adding a "port" rule
>      with the wrong protocol [1]?
> 
>      [1] https://docs.kernel.org/userspace-api/landlock.html#extending-a-ruleset
> 
> I suspect that -EINVAL is slightly more correct here, because this is not about
> the protocols that the kernel supports, but only about the range.  If we wanted
> to return errors about the protocol that the kernel supports, I realized that
> we'd probably also have to check whether the *combination* of family and type
> makes sense.  In my understanding, the equivalent errors for type and protocol,
> ESOCKTNOSUPPORT and EPROTONOSUPPORT, only get returned based on whether they
> make sense together with the other values.

Thanks, "unsupported" is indeed an incorrect naming in this case.
To check if the protocol is supported, we'll have to validate 
family-type combination, which would be really ugly.

Taking into account the thoughts below regarding family-type checking,
the following description can be used.

   /* Denies inserting a rule with family and type outside the range. */

> 
> 
>>>> +	if (family < 0 || family >= AF_MAX)
>>>> +		return -EINVAL;
>>>> +	if (type < 0 || type >= SOCK_MAX)
>>>> +		return -EINVAL;
>>>
>>> enum sock_type (include/linux/net.h) has "holes": values 7, 8 and 9 are not
>>> defined in the header.  Should we check more specifically for the supported
>>> values here?  (Is there already a helper function for that?)
>>
>> I think that a more detailed check of the family-type values may have a
>> good effect here, since the rules will contain real codes of families
>> and types.
>>
>> I haven't found any helper to check the supported socket type value.
>> Performing a check inside landlock can lead to several minor problems,
>> which theoretically should not lead to any costs.
>>
>> * There are would be a dependency with constants of enum sock_types. But
>>    we are unlikely to see new types of sockets in the next few years, so
>>    it wouldn't be a problem to maintain such check.
>>
>> * enum sock_types can be redefined (see ARCH_HAS_SOCKET_TYPES in net.h),
>>    but i haven't found anyone to actually change the constants of socket
>>    types. It would be wrong to have a different landlock behavior for
>>    arch that redefines sock_types for some purposes, so probably this
>>    should also be maintained.
>>
>> WDYT?
> 
> Thinking about it again, from a Landlock safety perspective, I believe it is
> safe to keep the checks as they are and to check for the two values to be in the
> ranges [0, AF_MAX) and [0, SOCK_MAX).
> 
> Even if we permit the rule to be added for an invalid socket type, there does
> not seem to be any harm in that, as these sockets can't be created anyway.
> Also, given the semantics of these errors in socket(2), where also the
> *combinations* of the values are checked, it seems overly complicated to check
> all these combinations.  I think it would be fine to keep as is, I was mostly
> wondering whether you had done any deeper analysis?

No, I think that checking only ranges is also a good solution. We won't
care about maintaining valid type constants, and that would simplify
behavior of this method for users. Let's take this approach.

> 
> It might be worth spelling out in the struct documentation that the values which
> fulfil 0 <= family < AF_MAX and 0 <= type < SOCK_MAX are considered valid.  Does
> that sound reasonable?

Ofc, I'll add appropriate comments in the definition of struct
landlock_socket_attr and in the socket rule documentation. Thanks!

> 
> P.S., it seems that the security/apparmor/Makefile is turning the "#define"s
> into C code with lookup tables, but it seems that this is only used for
> human-readable audit-logging, not for validating the policies.
> 
> —Günther

Re: [RFC PATCH v2 00/12] Socket type control for Landlock

[Mikhail Ivanov]

6/6/2024 4:32 PM, Günther Noack wrote:
> Hello Mikhail!
> 
> On Thu, Jun 06, 2024 at 02:44:23PM +0300, Mikhail Ivanov wrote:
>> 6/4/2024 11:22 PM, Günther Noack wrote:
>>> On Fri, May 24, 2024 at 05:30:03PM +0800, Mikhail Ivanov wrote:
>>>> Hello! This is v2 RFC patch dedicated to socket protocols restriction.
>>>>
>>>> It is based on the landlock's mic-next branch on top of v6.9 kernel
>>>> version.
>>>
>>> Hello Mikhail!
>>>
>>> I patched in your patchset and tried to use the feature with a small
>>> demo tool, but I ran into what I think is a bug -- do you happen to
>>> know what this might be?
>>>
>>> I used 6.10-rc1 as a base and patched your patches on top.
>>>
>>> The code is a small tool called "nonet", which does the following:
>>>
>>>     - Disable socket creation with a Landlock ruleset with the following
>>>       attributes:
>>>       struct landlock_ruleset_attr attr = {
>>>         .handled_access_socket = LANDLOCK_ACCESS_SOCKET_CREATE,
>>>       };
>>>
>>>     - open("/dev/null", O_WRONLY)
>>>
>>> Expected result:
>>>
>>>     - open() should work
>>>
>>> Observed result:
>>>
>>>     - open() fails with EACCES.
>>>
>>> I traced this with perf, and found that the open() gets rejected from
>>> Landlock's hook_file_open, whereas hook_socket_create does not get
>>> invoked.  This is surprising to me -- Enabling a policy for socket
>>> creation should not influence the outcome of opening files!
>>>
>>> Tracing commands:
>>>
>>>     sudo perf probe hook_socket_create '$params'
>>>     sudo perf probe 'hook_file_open%return $retval'
>>>     sudo perf record -e 'probe:*' -g -- ./nonet
>>>     sudo perf report
>>> You can find the tool in my landlock-examples repo in the nonet_bug branch:
>>> https://github.com/gnoack/landlock-examples/blob/nonet_bug/nonet.c
>>>
>>> Landlock is enabled like this:
>>> https://github.com/gnoack/landlock-examples/blob/nonet_bug/sandbox_socket.c
>>>
>>> Do you have a hunch what might be going on?
>>
>> Hello Günther!
>> Big thanks for this research!
>>
>> I figured out that I define LANDLOCK_SHIFT_ACCESS_SOCKET macro in
>> really strange way (see landlock/limits.h):
>>
>>    #define LANDLOCK_SHIFT_ACCESS_SOCKET	LANDLOCK_NUM_ACCESS_SOCKET
>>
>> With this definition, socket access mask overlaps the fs access
>> mask in ruleset->access_masks[layer_level]. That's why
>> landlock_get_fs_access_mask() returns non-zero mask in hook_file_open().
>>
>> So, the macro must be defined in this way:
>>
>>    #define LANDLOCK_SHIFT_ACCESS_SOCKET	(LANDLOCK_NUM_ACCESS_NET +
>>                                           LANDLOCK_NUM_ACCESS_FS)
>>
>> With this fix, open() doesn't fail in your example.
>>
>> I'm really sorry that I somehow made such a stupid typo. I will try my
>> best to make sure this doesn't happen again.
> 
> Thanks for figuring it out so quickly.  With that change, I'm getting some
> compilation errors (some bit shifts are becoming too wide for the underlying
> types), but I'm sure you can address that easily for the next version of the
> patch set.
> 
> IMHO this shows that our reliance on bit manipulation is probably getting in the
> way of code clarity. :-/ I hope we can simplify these internal structures at
> some point.  Once we have a better way to check for performance changes [1], we
> can try to change this and measure whether these comprehensibility/performance
> tradeoff is really worth it.
> 
> [1] https://github.com/landlock-lsm/linux/issues/24

Sounds great, probably this idea should be added to this issue [1].

[1] https://github.com/landlock-lsm/linux/issues/34

> 
> The other takeaway in my mind is, we should probably have some tests for that,
> to check that the enablement of one kind of policy does not affect the
> operations that belong to other kinds of policies.  Like this, for instance (I
> was about to send this test to help debugging):
> 
> TEST_F(mini, restricting_socket_does_not_affect_fs_actions)
> {
> 	const struct landlock_ruleset_attr ruleset_attr = {
> 		.handled_access_socket = LANDLOCK_ACCESS_SOCKET_CREATE,
> 	};
> 	int ruleset_fd, fd;
> 
> 	ruleset_fd = landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
> 	ASSERT_LE(0, ruleset_fd);
> 
> 	enforce_ruleset(_metadata, ruleset_fd);
> 	ASSERT_EQ(0, close(ruleset_fd));
> 
> 	/*
> 	 * Accessing /dev/null for writing should be permitted,
> 	 * because we did not add any file system restrictions.
> 	 */
> 	fd = open("/dev/null", O_WRONLY);
> 	EXPECT_LE(0, fd);
> 
> 	ASSERT_EQ(0, close(fd));
> }
> 
> Since these kinds of tests are a bit at the intersection between the
> fs/net/socket tests, maybe they could go into a separate test file?  The next
> time we add a new kind of Landlock restriction, it would come more naturally to
> add the matching test there and spot such issues earlier.  Would you volunteer
> to add such a test as part of your patch set? :)

Good idea! This test should probably be a part of the patch I mentioned
here [1]. WDYT?

(Btw, [1] should also be a part of the issue mentioned above).

[1] 
https://lore.kernel.org/all/f4b5e2b9-e960-fd08-fdf4-328bb475e2ef@huawei-partners.com/

> 
> Thanks,
> Günther

Re: [RFC PATCH v2 02/12] landlock: Add hook on socket creation

[Mikhail Ivanov]

6/5/2024 8:27 PM, Günther Noack wrote:
> Hello!
> 
> On Thu, May 30, 2024 at 03:20:21PM +0300, Mikhail Ivanov wrote:
>> 5/27/2024 11:48 AM, Günther Noack wrote:
>>> On Fri, May 24, 2024 at 05:30:05PM +0800, Mikhail Ivanov wrote:
>>>> Add hook to security_socket_post_create(), which checks whether the socket
>>>> type and family are allowed by domain. Hook is called after initializing
>>>> the socket in the network stack to not wrongfully return EACCES for a
>>>> family-type pair, which is considered invalid by the protocol.
>>>>
>>>> Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
>>>
>>> ## Some observations that *do not* need to be addressed in this commit, IMHO:
>>>
>>> get_raw_handled_socket_accesses, get_current_socket_domain and
>>> current_check_access_socket are based on the similarly-named functions from
>>> net.c (and fs.c), and it makes sense to stay consistent with these.
>>>
>>> There are some possible refactorings that could maybe be applied to that code,
>>> but given that the same ones would apply to net.c as well, it's probably best to
>>> address these separately.
>>>
>>>     * Should get_raw_handled_socket_accesses be inlined
>> It's a fairly simple and compact function, so compiler should inline it
>> without any problems. Mickaël was against optional inlines [1].
>>
>> [1] https://lore.kernel.org/linux-security-module/5c6c99f7-4218-1f79-477e-5d943c9809fd@digikod.net/
> 
> Sorry for the confusion -- what I meant was not "should we add the inline
> keyword", but I meant "should we remove that function and place its
> implementation in the place where we are currently calling it"?

Oh, I got it, thanks!
It will be great to find a way how to generalize this helpers. But if
we won't come up with some good design, it will be really better to
simply inline them. I added a mark about this in code refactoring issue
[1].

[1] https://github.com/landlock-lsm/linux/issues/34

> 
> 
>>>     * Does the WARN_ON_ONCE(dom->num_layers < 1) check have the right return code?
>>
>> Looks like a rudimental check. `dom` is always NULL when `num_layers`< 1
>> (see get_*_domain functions).
> 
> What I found irritating about it is that with 0 layers (= no Landlock policy was
> ever enabled), you would logically assume that we return a success?  But then I
> realized that this code was copied verbatim from other places in fs.c and net.c,
> and it is actually checking for an internal inconsistency that is never supposed
> to happen.  If we were to actually hit that case at some point, we have probably
> stumbled over our own feet and it might be better to not permit anything.

This check is probably really useful for validating code changes.

> 
> 
>>>     * Can we refactor out commonalities (probably not worth it right now though)?
>>
>> I had a few ideas about refactoring commonalities, as currently landlock
>> has several repetitive patterns in the code. But solution requires a
>> good design and a separate patch. Probably it's worth opening an issue
>> on github. WDYT?
> 
> Absolutely, please do open one.  In my mind, patches in C which might not get
> accepted are an expensive way to iterate on such ideas, and it might make sense
> to collect some refactoring approaches on a bug or the mailing list before
> jumping into the implementation.
> 
> (You might want to keep an eye on https://github.com/landlock-lsm/linux/issues/1
> as well, which is about some ideas to refactor Landlock's internal data
> structures.)

Thank you! Discussing refactoring ideas before actually implementing
them sounds really great. We can collect multiple ideas, discuss them
and implement a single dedicated patchlist.

Issue: https://github.com/landlock-lsm/linux/issues/34.

> 
> 
>>> ## The only actionable feedback that I have that is specific to this commit is:
>>>
>>> In the past, we have introduced new (non-test) Landlock functionality in a
>>> single commit -- that way, we have no "loose ends" in the code between these two
>>> commits, and that simplifies it for people who want to patch your feature onto
>>> other kernel trees.  (e.g. I think we should maybe merge commit 01/12 and 02/12
>>> into a single commit.)  WDYT?
>>
>> Yeah, this two should be merged and tests commits as well. I just wanted
>> to do this in one of the latest patch versions to simplify code review.
> 
> That sounds good, thanks!
> 
> —Günther

Re: [RFC PATCH v2 00/12] Socket type control for Landlock

[Günther Noack]

On Thu, Jun 06, 2024 at 02:44:23PM +0300, Mikhail Ivanov wrote:
> 6/4/2024 11:22 PM, Günther Noack wrote:
> I figured out that I define LANDLOCK_SHIFT_ACCESS_SOCKET macro in
> really strange way (see landlock/limits.h):
> 
>   #define LANDLOCK_SHIFT_ACCESS_SOCKET	LANDLOCK_NUM_ACCESS_SOCKET
> 
> With this definition, socket access mask overlaps the fs access
> mask in ruleset->access_masks[layer_level]. That's why
> landlock_get_fs_access_mask() returns non-zero mask in hook_file_open().
> 
> So, the macro must be defined in this way:
> 
>   #define LANDLOCK_SHIFT_ACCESS_SOCKET	(LANDLOCK_NUM_ACCESS_NET +
>                                          LANDLOCK_NUM_ACCESS_FS)
> 
> With this fix, open() doesn't fail in your example.
> 
> I'm really sorry that I somehow made such a stupid typo. I will try my
> best to make sure this doesn't happen again.

I found that we had the exact same bug with a wrongly defined "SHIFT" value in
[1].

Maybe we should define access_masks_t as a bit-field rather than doing the
bit-shifts by hand.  Then the compiler would keep track of the bit-offsets
automatically.

Bit-fields have a bad reputation, but in my understanding, this is largely
because they make it hard to control the exact bit-by-bit layout.  In our case,
we do not need such an exact control though, and it would be fine.

To quote Linus Torvalds on [2],

  Bitfields are fine if you don't actually care about the underlying format,
  and want gcc to just randomly assign bits, and want things to be
  convenient in that situation.

Let me send you a proposal patch which replaces access_masks_t with a bit-field
and removes the need for the "SHIFT" definition, which we already got wrong in
two patch sets now.  It has the additional benefit of making the code a bit
shorter and also removing a few static_assert()s which are now guaranteed by the
compiler.

—Günther

[1] https://lore.kernel.org/all/ZmLEoBfHyUR3nKAV@google.com/
[2] https://yarchive.net/comp/linux/bitfields.html

[PATCH] landlock: Use bit-fields for storing handled layer access masks

[Günther Noack]

When defined using bit-fields, the compiler takes care of packing the
bits in a memory-efficient way and frees us from defining
LANDLOCK_SHIFT_ACCESS_* by hand.  The exact memory layout does not
matter in our use case.

The manual definition of LANDLOCK_SHIFT_ACCESS_* has resulted in bugs
in at least two recent patch sets where new kinds of handled access
rights were introduced.

Cc: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
Cc: Tahera Fahimi <fahimitahera@gmail.com>
Link: https://lore.kernel.org/all/ebd680cc-25d6-ee14-4856-310f5e5e28e4@huawei-partners.com/
Link: https://lore.kernel.org/all/ZmLEoBfHyUR3nKAV@google.com/
Signed-off-by: Günther Noack <gnoack@google.com>
---
 security/landlock/limits.h  |  2 --
 security/landlock/ruleset.c |  4 ----
 security/landlock/ruleset.h | 24 +++++++++---------------
 3 files changed, 9 insertions(+), 21 deletions(-)

diff --git a/security/landlock/limits.h b/security/landlock/limits.h
index 20fdb5ff3514..4eb643077a2a 100644
--- a/security/landlock/limits.h
+++ b/security/landlock/limits.h
@@ -21,12 +21,10 @@
 #define LANDLOCK_LAST_ACCESS_FS		LANDLOCK_ACCESS_FS_IOCTL_DEV
 #define LANDLOCK_MASK_ACCESS_FS		((LANDLOCK_LAST_ACCESS_FS << 1) - 1)
 #define LANDLOCK_NUM_ACCESS_FS		__const_hweight64(LANDLOCK_MASK_ACCESS_FS)
-#define LANDLOCK_SHIFT_ACCESS_FS	0
 
 #define LANDLOCK_LAST_ACCESS_NET	LANDLOCK_ACCESS_NET_CONNECT_TCP
 #define LANDLOCK_MASK_ACCESS_NET	((LANDLOCK_LAST_ACCESS_NET << 1) - 1)
 #define LANDLOCK_NUM_ACCESS_NET		__const_hweight64(LANDLOCK_MASK_ACCESS_NET)
-#define LANDLOCK_SHIFT_ACCESS_NET	LANDLOCK_NUM_ACCESS_FS
 
 /* clang-format on */
 
diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
index e0a5fbf9201a..6ff232f58618 100644
--- a/security/landlock/ruleset.c
+++ b/security/landlock/ruleset.c
@@ -169,13 +169,9 @@ static void build_check_ruleset(void)
 		.num_rules = ~0,
 		.num_layers = ~0,
 	};
-	typeof(ruleset.access_masks[0]) access_masks = ~0;
 
 	BUILD_BUG_ON(ruleset.num_rules < LANDLOCK_MAX_NUM_RULES);
 	BUILD_BUG_ON(ruleset.num_layers < LANDLOCK_MAX_NUM_LAYERS);
-	BUILD_BUG_ON(access_masks <
-		     ((LANDLOCK_MASK_ACCESS_FS << LANDLOCK_SHIFT_ACCESS_FS) |
-		      (LANDLOCK_MASK_ACCESS_NET << LANDLOCK_SHIFT_ACCESS_NET)));
 }
 
 /**
diff --git a/security/landlock/ruleset.h b/security/landlock/ruleset.h
index c7f1526784fd..0f1b5b4c8f6b 100644
--- a/security/landlock/ruleset.h
+++ b/security/landlock/ruleset.h
@@ -39,10 +39,10 @@ static_assert(BITS_PER_TYPE(access_mask_t) >= LANDLOCK_NUM_ACCESS_NET);
 static_assert(sizeof(unsigned long) >= sizeof(access_mask_t));
 
 /* Ruleset access masks. */
-typedef u32 access_masks_t;
-/* Makes sure all ruleset access rights can be stored. */
-static_assert(BITS_PER_TYPE(access_masks_t) >=
-	      LANDLOCK_NUM_ACCESS_FS + LANDLOCK_NUM_ACCESS_NET);
+struct access_masks {
+	access_mask_t fs : LANDLOCK_NUM_ACCESS_FS;
+	access_mask_t net : LANDLOCK_NUM_ACCESS_NET;
+};
 
 typedef u16 layer_mask_t;
 /* Makes sure all layers can be checked. */
@@ -226,7 +226,7 @@ struct landlock_ruleset {
 			 * layers are set once and never changed for the
 			 * lifetime of the ruleset.
 			 */
-			access_masks_t access_masks[];
+			struct access_masks access_masks[];
 		};
 	};
 };
@@ -265,8 +265,7 @@ landlock_add_fs_access_mask(struct landlock_ruleset *const ruleset,
 
 	/* Should already be checked in sys_landlock_create_ruleset(). */
 	WARN_ON_ONCE(fs_access_mask != fs_mask);
-	ruleset->access_masks[layer_level] |=
-		(fs_mask << LANDLOCK_SHIFT_ACCESS_FS);
+	ruleset->access_masks[layer_level].fs |= fs_mask;
 }
 
 static inline void
@@ -278,17 +277,14 @@ landlock_add_net_access_mask(struct landlock_ruleset *const ruleset,
 
 	/* Should already be checked in sys_landlock_create_ruleset(). */
 	WARN_ON_ONCE(net_access_mask != net_mask);
-	ruleset->access_masks[layer_level] |=
-		(net_mask << LANDLOCK_SHIFT_ACCESS_NET);
+	ruleset->access_masks[layer_level].net |= net_mask;
 }
 
 static inline access_mask_t
 landlock_get_raw_fs_access_mask(const struct landlock_ruleset *const ruleset,
 				const u16 layer_level)
 {
-	return (ruleset->access_masks[layer_level] >>
-		LANDLOCK_SHIFT_ACCESS_FS) &
-	       LANDLOCK_MASK_ACCESS_FS;
+	return ruleset->access_masks[layer_level].fs;
 }
 
 static inline access_mask_t
@@ -304,9 +300,7 @@ static inline access_mask_t
 landlock_get_net_access_mask(const struct landlock_ruleset *const ruleset,
 			     const u16 layer_level)
 {
-	return (ruleset->access_masks[layer_level] >>
-		LANDLOCK_SHIFT_ACCESS_NET) &
-	       LANDLOCK_MASK_ACCESS_NET;
+	return ruleset->access_masks[layer_level].net;
 }
 
 bool landlock_unmask_layers(const struct landlock_rule *const rule,
-- 
2.45.2.505.gda0bf45e8d-goog

Re: [RFC PATCH v2 00/12] Socket type control for Landlock

[Mikhail Ivanov]

6/10/2024 11:03 AM, Günther Noack wrote:
> On Thu, Jun 06, 2024 at 02:44:23PM +0300, Mikhail Ivanov wrote:
>> 6/4/2024 11:22 PM, Günther Noack wrote:
>> I figured out that I define LANDLOCK_SHIFT_ACCESS_SOCKET macro in
>> really strange way (see landlock/limits.h):
>>
>>    #define LANDLOCK_SHIFT_ACCESS_SOCKET	LANDLOCK_NUM_ACCESS_SOCKET
>>
>> With this definition, socket access mask overlaps the fs access
>> mask in ruleset->access_masks[layer_level]. That's why
>> landlock_get_fs_access_mask() returns non-zero mask in hook_file_open().
>>
>> So, the macro must be defined in this way:
>>
>>    #define LANDLOCK_SHIFT_ACCESS_SOCKET	(LANDLOCK_NUM_ACCESS_NET +
>>                                           LANDLOCK_NUM_ACCESS_FS)
>>
>> With this fix, open() doesn't fail in your example.
>>
>> I'm really sorry that I somehow made such a stupid typo. I will try my
>> best to make sure this doesn't happen again.
> 
> I found that we had the exact same bug with a wrongly defined "SHIFT" value in
> [1].
> 
> Maybe we should define access_masks_t as a bit-field rather than doing the
> bit-shifts by hand.  Then the compiler would keep track of the bit-offsets
> automatically.
> 
> Bit-fields have a bad reputation, but in my understanding, this is largely
> because they make it hard to control the exact bit-by-bit layout.  In our case,
> we do not need such an exact control though, and it would be fine.
> 
> To quote Linus Torvalds on [2],
> 
>    Bitfields are fine if you don't actually care about the underlying format,
>    and want gcc to just randomly assign bits, and want things to be
>    convenient in that situation.
> 
> Let me send you a proposal patch which replaces access_masks_t with a bit-field
> and removes the need for the "SHIFT" definition, which we already got wrong in
> two patch sets now.  It has the additional benefit of making the code a bit
> shorter and also removing a few static_assert()s which are now guaranteed by the
> compiler.
> 
> —Günther
> 
> [1] https://lore.kernel.org/all/ZmLEoBfHyUR3nKAV@google.com/
> [2] https://yarchive.net/comp/linux/bitfields.html

Thank you, Günther! It really looks more clear.

This patch should be applied to Landlock separately, right?

Re: [PATCH] landlock: Use bit-fields for storing handled layer access masks

[Mickaël Salaün]

Great!  Looking at the generated data structures with pahole, it doesn't
increase the whole size, and it should be fine with other (small) fields
too.

With this new struct, we don't need the landlock_get_* helpers anymore.
We might want to keep the landlock_add_*() helpers as safeguards
(because of the WARN_ON_ONCE) though.

On Mon, Jun 10, 2024 at 08:21:15AM +0000, Günther Noack wrote:
> When defined using bit-fields, the compiler takes care of packing the
> bits in a memory-efficient way and frees us from defining
> LANDLOCK_SHIFT_ACCESS_* by hand.  The exact memory layout does not
> matter in our use case.
> 
> The manual definition of LANDLOCK_SHIFT_ACCESS_* has resulted in bugs
> in at least two recent patch sets where new kinds of handled access
> rights were introduced.
> 
> Cc: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
> Cc: Tahera Fahimi <fahimitahera@gmail.com>
> Link: https://lore.kernel.org/all/ebd680cc-25d6-ee14-4856-310f5e5e28e4@huawei-partners.com/
> Link: https://lore.kernel.org/all/ZmLEoBfHyUR3nKAV@google.com/

Please add [1] and [2] at the end of each link to reference them in the
commit message.

> Signed-off-by: Günther Noack <gnoack@google.com>
> ---
>  security/landlock/limits.h  |  2 --
>  security/landlock/ruleset.c |  4 ----
>  security/landlock/ruleset.h | 24 +++++++++---------------
>  3 files changed, 9 insertions(+), 21 deletions(-)
> 
> diff --git a/security/landlock/limits.h b/security/landlock/limits.h
> index 20fdb5ff3514..4eb643077a2a 100644
> --- a/security/landlock/limits.h
> +++ b/security/landlock/limits.h
> @@ -21,12 +21,10 @@
>  #define LANDLOCK_LAST_ACCESS_FS		LANDLOCK_ACCESS_FS_IOCTL_DEV
>  #define LANDLOCK_MASK_ACCESS_FS		((LANDLOCK_LAST_ACCESS_FS << 1) - 1)
>  #define LANDLOCK_NUM_ACCESS_FS		__const_hweight64(LANDLOCK_MASK_ACCESS_FS)
> -#define LANDLOCK_SHIFT_ACCESS_FS	0
>  
>  #define LANDLOCK_LAST_ACCESS_NET	LANDLOCK_ACCESS_NET_CONNECT_TCP
>  #define LANDLOCK_MASK_ACCESS_NET	((LANDLOCK_LAST_ACCESS_NET << 1) - 1)
>  #define LANDLOCK_NUM_ACCESS_NET		__const_hweight64(LANDLOCK_MASK_ACCESS_NET)
> -#define LANDLOCK_SHIFT_ACCESS_NET	LANDLOCK_NUM_ACCESS_FS
>  
>  /* clang-format on */
>  
> diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
> index e0a5fbf9201a..6ff232f58618 100644
> --- a/security/landlock/ruleset.c
> +++ b/security/landlock/ruleset.c
> @@ -169,13 +169,9 @@ static void build_check_ruleset(void)
>  		.num_rules = ~0,
>  		.num_layers = ~0,
>  	};
> -	typeof(ruleset.access_masks[0]) access_masks = ~0;
>  
>  	BUILD_BUG_ON(ruleset.num_rules < LANDLOCK_MAX_NUM_RULES);
>  	BUILD_BUG_ON(ruleset.num_layers < LANDLOCK_MAX_NUM_LAYERS);
> -	BUILD_BUG_ON(access_masks <
> -		     ((LANDLOCK_MASK_ACCESS_FS << LANDLOCK_SHIFT_ACCESS_FS) |
> -		      (LANDLOCK_MASK_ACCESS_NET << LANDLOCK_SHIFT_ACCESS_NET)));
>  }
>  
>  /**
> diff --git a/security/landlock/ruleset.h b/security/landlock/ruleset.h
> index c7f1526784fd..0f1b5b4c8f6b 100644
> --- a/security/landlock/ruleset.h
> +++ b/security/landlock/ruleset.h
> @@ -39,10 +39,10 @@ static_assert(BITS_PER_TYPE(access_mask_t) >= LANDLOCK_NUM_ACCESS_NET);
>  static_assert(sizeof(unsigned long) >= sizeof(access_mask_t));
>  
>  /* Ruleset access masks. */
> -typedef u32 access_masks_t;
> -/* Makes sure all ruleset access rights can be stored. */
> -static_assert(BITS_PER_TYPE(access_masks_t) >=
> -	      LANDLOCK_NUM_ACCESS_FS + LANDLOCK_NUM_ACCESS_NET);
> +struct access_masks {
> +	access_mask_t fs : LANDLOCK_NUM_ACCESS_FS;
> +	access_mask_t net : LANDLOCK_NUM_ACCESS_NET;
> +};
>  
>  typedef u16 layer_mask_t;
>  /* Makes sure all layers can be checked. */
> @@ -226,7 +226,7 @@ struct landlock_ruleset {
>  			 * layers are set once and never changed for the
>  			 * lifetime of the ruleset.
>  			 */
> -			access_masks_t access_masks[];
> +			struct access_masks access_masks[];
>  		};
>  	};
>  };
> @@ -265,8 +265,7 @@ landlock_add_fs_access_mask(struct landlock_ruleset *const ruleset,
>  
>  	/* Should already be checked in sys_landlock_create_ruleset(). */
>  	WARN_ON_ONCE(fs_access_mask != fs_mask);
> -	ruleset->access_masks[layer_level] |=
> -		(fs_mask << LANDLOCK_SHIFT_ACCESS_FS);
> +	ruleset->access_masks[layer_level].fs |= fs_mask;
>  }
>  
>  static inline void
> @@ -278,17 +277,14 @@ landlock_add_net_access_mask(struct landlock_ruleset *const ruleset,
>  
>  	/* Should already be checked in sys_landlock_create_ruleset(). */
>  	WARN_ON_ONCE(net_access_mask != net_mask);
> -	ruleset->access_masks[layer_level] |=
> -		(net_mask << LANDLOCK_SHIFT_ACCESS_NET);
> +	ruleset->access_masks[layer_level].net |= net_mask;
>  }
>  
>  static inline access_mask_t
>  landlock_get_raw_fs_access_mask(const struct landlock_ruleset *const ruleset,
>  				const u16 layer_level)
>  {
> -	return (ruleset->access_masks[layer_level] >>
> -		LANDLOCK_SHIFT_ACCESS_FS) &
> -	       LANDLOCK_MASK_ACCESS_FS;
> +	return ruleset->access_masks[layer_level].fs;
>  }
>  
>  static inline access_mask_t
> @@ -304,9 +300,7 @@ static inline access_mask_t
>  landlock_get_net_access_mask(const struct landlock_ruleset *const ruleset,
>  			     const u16 layer_level)
>  {
> -	return (ruleset->access_masks[layer_level] >>
> -		LANDLOCK_SHIFT_ACCESS_NET) &
> -	       LANDLOCK_MASK_ACCESS_NET;
> +	return ruleset->access_masks[layer_level].net;
>  }
>  
>  bool landlock_unmask_layers(const struct landlock_rule *const rule,
> -- 
> 2.45.2.505.gda0bf45e8d-goog
> 
> 

Re: [PATCH] landlock: Use bit-fields for storing handled layer access masks

[Günther Noack]

On Thu, Jun 13, 2024 at 11:20:38PM +0200, Mickaël Salaün wrote:
> Great!  Looking at the generated data structures with pahole, it doesn't
> increase the whole size, and it should be fine with other (small) fields
> too.
> 
> With this new struct, we don't need the landlock_get_* helpers anymore.
> We might want to keep the landlock_add_*() helpers as safeguards
> (because of the WARN_ON_ONCE) though.

I am unsure about removing these helper functions, due to the following reasons:

 * landlock_get_fs_access_mask is the place where we transparently add the
   "refer" access right.  If we remove landlock_get_net_access_mask, it would be
   assymetric with keeping the same function for the file system restrictions.

 * landlock_init_layer_masks() is using landlock_get_fs_access_mask and
   landlock_get_net_access_mask through a function pointer.  When these
   functions are gone, we would have to redefine them locally anyway.

   Options to refactor this function include:
    * split it in two separate functions landlock_init_fs_layer_masks and
      landlock_init_net_layer_masks.  It would end up duplicating some of the
      bit manipulation code.
    * add another #if further down in the function

   Both variants seem not nice.

Do you think this is worth doing?

—Günther

Re: [PATCH] landlock: Use bit-fields for storing handled layer access masks

[Mickaël Salaün]

On Fri, Jun 14, 2024 at 02:06:54PM +0200, Günther Noack wrote:
> On Thu, Jun 13, 2024 at 11:20:38PM +0200, Mickaël Salaün wrote:
> > Great!  Looking at the generated data structures with pahole, it doesn't
> > increase the whole size, and it should be fine with other (small) fields
> > too.
> > 
> > With this new struct, we don't need the landlock_get_* helpers anymore.
> > We might want to keep the landlock_add_*() helpers as safeguards
> > (because of the WARN_ON_ONCE) though.
> 
> I am unsure about removing these helper functions, due to the following reasons:
> 
>  * landlock_get_fs_access_mask is the place where we transparently add the
>    "refer" access right.  If we remove landlock_get_net_access_mask, it would be
>    assymetric with keeping the same function for the file system restrictions.
> 
>  * landlock_init_layer_masks() is using landlock_get_fs_access_mask and
>    landlock_get_net_access_mask through a function pointer.  When these
>    functions are gone, we would have to redefine them locally anyway.
> 
>    Options to refactor this function include:
>     * split it in two separate functions landlock_init_fs_layer_masks and
>       landlock_init_net_layer_masks.  It would end up duplicating some of the
>       bit manipulation code.
>     * add another #if further down in the function
> 
>    Both variants seem not nice.
> 
> Do you think this is worth doing?

No, I agree with you.  It's applied to my next branch. Thanks!

Mikhail, Tahera, please base your next patch series on this branch:
https://git.kernel.org/pub/scm/linux/kernel/git/mic/linux.git/

Re: [RFC PATCH v2 02/12] landlock: Add hook on socket creation

[Mickaël Salaün]

On Fri, Jun 07, 2024 at 05:45:46PM +0300, Mikhail Ivanov wrote:
> 6/5/2024 8:27 PM, Günther Noack wrote:
> > Hello!
> > 
> > On Thu, May 30, 2024 at 03:20:21PM +0300, Mikhail Ivanov wrote:
> > > 5/27/2024 11:48 AM, Günther Noack wrote:
> > > > On Fri, May 24, 2024 at 05:30:05PM +0800, Mikhail Ivanov wrote:
> > > > > Add hook to security_socket_post_create(), which checks whether the socket
> > > > > type and family are allowed by domain. Hook is called after initializing
> > > > > the socket in the network stack to not wrongfully return EACCES for a
> > > > > family-type pair, which is considered invalid by the protocol.
> > > > > 
> > > > > Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
> > > > 
> > > > ## Some observations that *do not* need to be addressed in this commit, IMHO:
> > > > 
> > > > get_raw_handled_socket_accesses, get_current_socket_domain and
> > > > current_check_access_socket are based on the similarly-named functions from
> > > > net.c (and fs.c), and it makes sense to stay consistent with these.
> > > > 
> > > > There are some possible refactorings that could maybe be applied to that code,
> > > > but given that the same ones would apply to net.c as well, it's probably best to
> > > > address these separately.
> > > > 
> > > >     * Should get_raw_handled_socket_accesses be inlined
> > > It's a fairly simple and compact function, so compiler should inline it
> > > without any problems. Mickaël was against optional inlines [1].
> > > 
> > > [1] https://lore.kernel.org/linux-security-module/5c6c99f7-4218-1f79-477e-5d943c9809fd@digikod.net/
> > 
> > Sorry for the confusion -- what I meant was not "should we add the inline
> > keyword", but I meant "should we remove that function and place its
> > implementation in the place where we are currently calling it"?
> 
> Oh, I got it, thanks!
> It will be great to find a way how to generalize this helpers. But if
> we won't come up with some good design, it will be really better to
> simply inline them. I added a mark about this in code refactoring issue
> [1].

Making such simple helper more generic might not be worth it.  Landlock
doesn't handle a lot of different "objects".

> 
> [1] https://github.com/landlock-lsm/linux/issues/34
> 
> > 
> > 
> > > >     * Does the WARN_ON_ONCE(dom->num_layers < 1) check have the right return code?
> > > 
> > > Looks like a rudimental check. `dom` is always NULL when `num_layers`< 1
> > > (see get_*_domain functions).
> > 
> > What I found irritating about it is that with 0 layers (= no Landlock policy was
> > ever enabled), you would logically assume that we return a success?  But then I
> > realized that this code was copied verbatim from other places in fs.c and net.c,
> > and it is actually checking for an internal inconsistency that is never supposed
> > to happen.  If we were to actually hit that case at some point, we have probably
> > stumbled over our own feet and it might be better to not permit anything.
> 
> This check is probably really useful for validating code changes.

Correct, this is mostly useful for developers when changing the kernel
code.  We'll remove this kind of check when we'll have a proper struct
landlock_domain. ;)

> 
> > 
> > 
> > > >     * Can we refactor out commonalities (probably not worth it right now though)?
> > > 
> > > I had a few ideas about refactoring commonalities, as currently landlock
> > > has several repetitive patterns in the code. But solution requires a
> > > good design and a separate patch. Probably it's worth opening an issue
> > > on github. WDYT?
> > 
> > Absolutely, please do open one.  In my mind, patches in C which might not get
> > accepted are an expensive way to iterate on such ideas, and it might make sense
> > to collect some refactoring approaches on a bug or the mailing list before
> > jumping into the implementation.
> > 
> > (You might want to keep an eye on https://github.com/landlock-lsm/linux/issues/1
> > as well, which is about some ideas to refactor Landlock's internal data
> > structures.)
> 
> Thank you! Discussing refactoring ideas before actually implementing
> them sounds really great. We can collect multiple ideas, discuss them
> and implement a single dedicated patchlist.
> 
> Issue: https://github.com/landlock-lsm/linux/issues/34.

Yes, we are continuing the discussion there.

> 
> > 
> > 
> > > > ## The only actionable feedback that I have that is specific to this commit is:
> > > > 
> > > > In the past, we have introduced new (non-test) Landlock functionality in a
> > > > single commit -- that way, we have no "loose ends" in the code between these two
> > > > commits, and that simplifies it for people who want to patch your feature onto
> > > > other kernel trees.  (e.g. I think we should maybe merge commit 01/12 and 02/12
> > > > into a single commit.)  WDYT?
> > > 
> > > Yeah, this two should be merged and tests commits as well. I just wanted
> > > to do this in one of the latest patch versions to simplify code review.
> > 
> > That sounds good, thanks!
> > 
> > —Günther
>