Re: [PATCH v8 07/12] landlock: Add network rules support

Re: [PATCH v8 07/12] landlock: Add network rules support

[Mickaël Salaün]

On 21/10/2022 17:26, Konstantin Meskhidze wrote:
> This commit adds network rules support in internal landlock functions
> (presented in ruleset.c) and landlock_create_ruleset syscall.

…in the ruleset management helpers and the landlock_create_ruleset syscall.


> Refactors user space API to support network actions. Adds new network

Refactor…

> access flags, network rule and network attributes. Increments Landlock

Increment…

> ABI version.
> 
> Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze@huawei.com>
> ---
> 
> Changes since v7:
> * Squashes commits.
> * Increments ABI version to 4.
> * Refactors commit message.
> * Minor fixes.
> 
> Changes since v6:
> * Renames landlock_set_net_access_mask() to landlock_add_net_access_mask()
>    because it OR values.
> * Makes landlock_add_net_access_mask() more resilient incorrect values.
> * Refactors landlock_get_net_access_mask().
> * Renames LANDLOCK_MASK_SHIFT_NET to LANDLOCK_SHIFT_ACCESS_NET and use
>    LANDLOCK_NUM_ACCESS_FS as value.
> * Updates access_masks_t to u32 to support network access actions.
> * Refactors landlock internal functions to support network actions with
>    landlock_key/key_type/id types.
> 
> Changes since v5:
> * Gets rid of partial revert from landlock_add_rule
> syscall.
> * Formats code with clang-format-14.
> 
> Changes since v4:
> * Refactors landlock_create_ruleset() - splits ruleset and
> masks checks.
> * Refactors landlock_create_ruleset() and landlock mask
> setters/getters to support two rule types.
> * Refactors landlock_add_rule syscall add_rule_path_beneath
> function by factoring out get_ruleset_from_fd() and
> landlock_put_ruleset().
> 
> Changes since v3:
> * Splits commit.
> * Adds network rule support for internal landlock functions.
> * Adds set_mask and get_mask for network.
> * Adds rb_root root_net_port.
> 
> ---
>   include/uapi/linux/landlock.h                | 49 ++++++++++++++
>   security/landlock/limits.h                   |  6 +-
>   security/landlock/ruleset.c                  | 55 ++++++++++++++--
>   security/landlock/ruleset.h                  | 68 ++++++++++++++++----
>   security/landlock/syscalls.c                 | 13 +++-
>   tools/testing/selftests/landlock/base_test.c |  2 +-
>   6 files changed, 170 insertions(+), 23 deletions(-)
> 
> diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
> index f3223f964691..096b683c6ff3 100644
> --- a/include/uapi/linux/landlock.h
> +++ b/include/uapi/linux/landlock.h
> @@ -31,6 +31,13 @@ struct landlock_ruleset_attr {
>   	 * this access right.
>   	 */
>   	__u64 handled_access_fs;
> +
> +	/**
> +	 * @handled_access_net: Bitmask of actions (cf. `Network flags`_)
> +	 * that is handled by this ruleset and should then be forbidden if no
> +	 * rule explicitly allow them.
> +	 */
> +	__u64 handled_access_net;
>   };
> 
>   /*
> @@ -54,6 +61,11 @@ enum landlock_rule_type {
>   	 * landlock_path_beneath_attr .
>   	 */
>   	LANDLOCK_RULE_PATH_BENEATH = 1,
> +	/**
> +	 * @LANDLOCK_RULE_NET_SERVICE: Type of a &struct
> +	 * landlock_net_service_attr .
> +	 */
> +	LANDLOCK_RULE_NET_SERVICE = 2,
>   };
> 
>   /**
> @@ -79,6 +91,24 @@ struct landlock_path_beneath_attr {
>   	 */
>   } __attribute__((packed));
> 
> +/**
> + * struct landlock_net_service_attr - TCP subnet definition
> + *
> + * Argument of sys_landlock_add_rule().
> + */
> +struct landlock_net_service_attr {
> +	/**
> +	 * @allowed_access: Bitmask of allowed access network for services
> +	 * (cf. `Network flags`_).
> +	 */
> +	__u64 allowed_access;
> +	/**
> +	 * @port: Network port.
> +	 */
> +	__u16 port;

 From an UAPI point of view, I think the port field should be __be16, as 
for sockaddr_in->port and other network-related APIs. This will require 
some kernel changes to please sparse: make C=2 security/landlock/ must 
not print any warning.

Using big-endian values as keys (casted to uintptr_t, not strictly 
__be16) in the rb-tree should not be an issue because there is no port 
range ordering (for now).

A dedicated test should check that endianness is correct, e.g. by using 
different port encoding. This should include passing and failing tests, 
but they should work on all architectures (i.e. big or little endian).


> +
> +} __attribute__((packed));
> +
>   /**
>    * DOC: fs_access
>    *
> @@ -173,4 +203,23 @@ struct landlock_path_beneath_attr {
>   #define LANDLOCK_ACCESS_FS_TRUNCATE			(1ULL << 14)
>   /* clang-format on */
> 
> +/**
> + * DOC: net_access
> + *
> + * Network flags
> + * ~~~~~~~~~~~~~~~~
> + *
> + * These flags enable to restrict a sandboxed process to a set of network
> + * actions.
> + *
> + * TCP sockets with allowed actions:
> + *
> + * - %LANDLOCK_ACCESS_NET_BIND_TCP: Bind a TCP socket to a local port.
> + * - %LANDLOCK_ACCESS_NET_CONNECT_TCP: Connect an active TCP socket to
> + *   a remote port.
> + */
> +/* clang-format off */
> +#define LANDLOCK_ACCESS_NET_BIND_TCP			(1ULL << 0)
> +#define LANDLOCK_ACCESS_NET_CONNECT_TCP			(1ULL << 1)
> +/* clang-format on */
>   #endif /* _UAPI_LINUX_LANDLOCK_H */
> diff --git a/security/landlock/limits.h b/security/landlock/limits.h
> index bafb3b8dc677..8a1a6463c64e 100644
> --- a/security/landlock/limits.h
> +++ b/security/landlock/limits.h
> @@ -23,6 +23,10 @@
>   #define LANDLOCK_NUM_ACCESS_FS		__const_hweight64(LANDLOCK_MASK_ACCESS_FS)
>   #define LANDLOCK_SHIFT_ACCESS_FS	0
> 
> -/* clang-format on */
> +#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 */
>   #endif /* _SECURITY_LANDLOCK_LIMITS_H */
> diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
> index c7cf54ba4f6d..9277c1295114 100644
> --- a/security/landlock/ruleset.c
> +++ b/security/landlock/ruleset.c
> @@ -36,6 +36,9 @@ static struct landlock_ruleset *create_ruleset(const u32 num_layers)
>   	refcount_set(&new_ruleset->usage, 1);
>   	mutex_init(&new_ruleset->lock);
>   	new_ruleset->root_inode = RB_ROOT;
> +#if IS_ENABLED(CONFIG_INET)
> +	new_ruleset->root_net_port = RB_ROOT;
> +#endif /* IS_ENABLED(CONFIG_INET) */
>   	new_ruleset->num_layers = num_layers;
>   	/*
>   	 * hierarchy = NULL
> @@ -46,16 +49,21 @@ static struct landlock_ruleset *create_ruleset(const u32 num_layers)
>   }
> 
>   struct landlock_ruleset *
> -landlock_create_ruleset(const access_mask_t fs_access_mask)
> +landlock_create_ruleset(const access_mask_t fs_access_mask,
> +			const access_mask_t net_access_mask)
>   {
>   	struct landlock_ruleset *new_ruleset;
> 
>   	/* Informs about useless ruleset. */
> -	if (!fs_access_mask)
> +	if (!fs_access_mask && !net_access_mask)
>   		return ERR_PTR(-ENOMSG);
>   	new_ruleset = create_ruleset(1);
> -	if (!IS_ERR(new_ruleset))
> +	if (IS_ERR(new_ruleset))
> +		return new_ruleset;
> +	if (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);
>   	return new_ruleset;
>   }
> 
> @@ -73,6 +81,10 @@ static inline bool is_object_pointer(const enum landlock_key_type key_type)
>   	switch (key_type) {
>   	case LANDLOCK_KEY_INODE:
>   		return true;
> +#if IS_ENABLED(CONFIG_INET)
> +	case LANDLOCK_KEY_NET_PORT:
> +		return false;
> +#endif /* IS_ENABLED(CONFIG_INET) */
>   	}
>   	WARN_ON_ONCE(1);
>   	return false;
> @@ -126,6 +138,11 @@ static inline struct rb_root *get_root(struct landlock_ruleset *const ruleset,
>   	case LANDLOCK_KEY_INODE:
>   		root = &ruleset->root_inode;
>   		break;
> +#if IS_ENABLED(CONFIG_INET)
> +	case LANDLOCK_KEY_NET_PORT:
> +		root = &ruleset->root_net_port;
> +		break;
> +#endif /* IS_ENABLED(CONFIG_INET) */
>   	}
>   	if (WARN_ON_ONCE(!root))
>   		return ERR_PTR(-EINVAL);
> @@ -154,7 +171,9 @@ static void build_check_ruleset(void)
>   	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_FS << LANDLOCK_SHIFT_ACCESS_FS) +

This is correct but because we are dealing with bitmasks I would prefer 
to use "|" instead of "+".


> +			     (LANDLOCK_MASK_ACCESS_NET
> +			      << LANDLOCK_SHIFT_ACCESS_NET));
>   }
> 
>   /**
> @@ -370,7 +389,12 @@ static int merge_ruleset(struct landlock_ruleset *const dst,
>   	err = merge_tree(dst, src, LANDLOCK_KEY_INODE);
>   	if (err)
>   		goto out_unlock;
> -

Please keep this newline.

> +#if IS_ENABLED(CONFIG_INET)
> +	/* Merges the @src network port tree. */
> +	err = merge_tree(dst, src, LANDLOCK_KEY_NET_PORT);
> +	if (err)
> +		goto out_unlock;
> +#endif /* IS_ENABLED(CONFIG_INET) */
>   out_unlock:
>   	mutex_unlock(&src->lock);
>   	mutex_unlock(&dst->lock);
> @@ -426,7 +450,12 @@ static int inherit_ruleset(struct landlock_ruleset *const parent,
>   	err = inherit_tree(parent, child, LANDLOCK_KEY_INODE);
>   	if (err)
>   		goto out_unlock;
> -

newline

> +#if IS_ENABLED(CONFIG_INET)
> +	/* Copies the @parent network port tree. */
> +	err = inherit_tree(parent, child, LANDLOCK_KEY_NET_PORT);
> +	if (err)
> +		goto out_unlock;
> +#endif /* IS_ENABLED(CONFIG_INET) */
>   	if (WARN_ON_ONCE(child->num_layers <= parent->num_layers)) {
>   		err = -EINVAL;
>   		goto out_unlock;
> @@ -459,6 +488,11 @@ static void free_ruleset(struct landlock_ruleset *const ruleset)
>   	rbtree_postorder_for_each_entry_safe(freeme, next, &ruleset->root_inode,
>   					     node)
>   		free_rule(freeme, LANDLOCK_KEY_INODE);
> +#if IS_ENABLED(CONFIG_INET)
> +	rbtree_postorder_for_each_entry_safe(freeme, next,
> +					     &ruleset->root_net_port, node)
> +		free_rule(freeme, LANDLOCK_KEY_NET_PORT);
> +#endif /* IS_ENABLED(CONFIG_INET) */
>   	put_hierarchy(ruleset->hierarchy);
>   	kfree(ruleset);
>   }
> @@ -637,6 +671,9 @@ get_access_mask_t(const struct landlock_ruleset *const ruleset,
>    * Populates @layer_masks such that for each access right in @access_request,
>    * the bits for all the layers are set where this access right is handled.
>    *
> + * @layer_masks must contain LANDLOCK_NUM_ACCESS_FS or LANDLOCK_NUM_ACCESS_NET
> + * elements according to @key_type.

Please include this sentence in the @layer_masks description below.

> + *
>    * @domain: The domain that defines the current restrictions.
>    * @access_request: The requested access rights to check.
>    * @layer_masks: The layer masks to populate.

"It must contain…"


> @@ -659,6 +696,12 @@ access_mask_t init_layer_masks(const struct landlock_ruleset *const domain,
>   		get_access_mask = landlock_get_fs_access_mask;
>   		num_access = LANDLOCK_NUM_ACCESS_FS;
>   		break;
> +#if IS_ENABLED(CONFIG_INET)
> +	case LANDLOCK_KEY_NET_PORT:
> +		get_access_mask = landlock_get_net_access_mask;
> +		num_access = LANDLOCK_NUM_ACCESS_NET;
> +		break;
> +#endif /* IS_ENABLED(CONFIG_INET) */
>   	default:
>   		WARN_ON_ONCE(1);
>   		return 0;
> diff --git a/security/landlock/ruleset.h b/security/landlock/ruleset.h
> index d9eb79ea9a89..f272d2cd518c 100644
> --- a/security/landlock/ruleset.h
> +++ b/security/landlock/ruleset.h
> @@ -19,16 +19,20 @@
>   #include "limits.h"
>   #include "object.h"
> 
> +/* Rule access mask. */
>   typedef u16 access_mask_t;
>   /* Makes sure all filesystem access rights can be stored. */
>   static_assert(BITS_PER_TYPE(access_mask_t) >= LANDLOCK_NUM_ACCESS_FS);
> +/* Makes sure all network access rights can be stored. */
> +static_assert(BITS_PER_TYPE(access_mask_t) >= LANDLOCK_NUM_ACCESS_NET);
>   /* Makes sure for_each_set_bit() and for_each_clear_bit() calls are OK. */
>   static_assert(sizeof(unsigned long) >= sizeof(access_mask_t));
> 
>   /* Ruleset access masks. */
> -typedef u16 access_masks_t;
> +typedef u32 access_masks_t;

This type change need to be explained in the commit message.


>   /* Makes sure all ruleset access rights can be stored. */
> -static_assert(BITS_PER_TYPE(access_masks_t) >= LANDLOCK_NUM_ACCESS_FS);
> +static_assert(BITS_PER_TYPE(access_masks_t) >=
> +	      LANDLOCK_NUM_ACCESS_FS + LANDLOCK_NUM_ACCESS_NET);
> 
>   typedef u16 layer_mask_t;
>   /* Makes sure all layers can be checked. */
> @@ -82,6 +86,13 @@ enum landlock_key_type {
>   	 * keys.
>   	 */
>   	LANDLOCK_KEY_INODE = 1,
> +#if IS_ENABLED(CONFIG_INET)
> +	/**
> +	 * @LANDLOCK_KEY_NET_PORT: Type of &landlock_ruleset.root_net_port's
> +	 * node keys.
> +	 */
> +	LANDLOCK_KEY_NET_PORT = 2,
> +#endif /* IS_ENABLED(CONFIG_INET) */
>   };
> 
>   /**
> @@ -156,6 +167,15 @@ struct landlock_ruleset {
>   	 * reaches zero.
>   	 */
>   	struct rb_root root_inode;
> +#if IS_ENABLED(CONFIG_INET)
> +	/**
> +	 * @root_net_port: Root of a red-black tree containing &struct
> +	 * landlock_rule nodes with network port. 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_net_port;
> +#endif /* IS_ENABLED(CONFIG_INET) */
>   	/**
>   	 * @hierarchy: Enables hierarchy identification even when a parent
>   	 * domain vanishes.  This is needed for the ptrace protection.
> @@ -166,8 +186,8 @@ struct landlock_ruleset {
>   		 * @work_free: Enables to free a ruleset within a lockless
>   		 * section.  This is only used by
>   		 * landlock_put_ruleset_deferred() when @usage reaches zero.
> -		 * The fields @lock, @usage, @num_rules, @num_layers and
> -		 * @access_masks are then unused.
> +		 * The fields @lock, @usage, @num_rules, @num_layers,
> +		 * @net_access_mask and @access_masks are then unused.

There is no net_access_mask anymore.

Re: [PATCH v8 08/12] landlock: Implement TCP network hooks

[Mickaël Salaün]

On 21/10/2022 17:26, Konstantin Meskhidze wrote:
> This patch adds support of socket_bind() and socket_connect() hooks.
> It's possible to restrict binding and connecting of TCP sockets to
> particular ports.

Implement socket_bind() and socket_connect LSM hooks, which enable to 
restrict TCP socket binding and connection to specific ports.


> 
> Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze@huawei.com>
> ---
> 
> Changes since v7:
> * Minor fixes.
> * Refactors commit message.
> 
> Changes since v6:
> * Updates copyright.
> * Refactors landlock_append_net_rule() and check_socket_access()
>    functions with landlock_id type.
> 
> Changes since v5:
> * Fixes some logic errors.
> * Formats code with clang-format-14.
> 
> Changes since v4:
> * Factors out CONFIG_INET into make file.
> * Refactors check_socket_access().
> * Adds helper get_port().
> * Adds CONFIG_IPV6 in get_port(), hook_socket_bind/connect
> functions to support AF_INET6 family.
> * Adds AF_UNSPEC family support in hook_socket_bind/connect
> functions.
> * Refactors add_rule_net_service() and landlock_add_rule
> syscall to support network rule inserting.
> * Refactors init_layer_masks() to support network rules.
> 
> Changes since v3:
> * Splits commit.
> * Adds SECURITY_NETWORK in config.
> * Adds IS_ENABLED(CONFIG_INET) if a kernel has no INET configuration.
> * Adds hook_socket_bind and hook_socket_connect hooks.
> 
> ---
>   security/landlock/Kconfig    |   1 +
>   security/landlock/Makefile   |   2 +
>   security/landlock/net.c      | 164 +++++++++++++++++++++++++++++++++++
>   security/landlock/net.h      |  26 ++++++
>   security/landlock/setup.c    |   2 +
>   security/landlock/syscalls.c |  59 ++++++++++++-
>   6 files changed, 251 insertions(+), 3 deletions(-)
>   create mode 100644 security/landlock/net.c
>   create mode 100644 security/landlock/net.h
> 
> diff --git a/security/landlock/Kconfig b/security/landlock/Kconfig
> index 8e33c4e8ffb8..10c099097533 100644
> --- a/security/landlock/Kconfig
> +++ b/security/landlock/Kconfig
> @@ -3,6 +3,7 @@
>   config SECURITY_LANDLOCK
>   	bool "Landlock support"
>   	depends on SECURITY && !ARCH_EPHEMERAL_INODES
> +	select SECURITY_NETWORK
>   	select SECURITY_PATH
>   	help
>   	  Landlock is a sandboxing mechanism that enables processes to restrict
> diff --git a/security/landlock/Makefile b/security/landlock/Makefile
> index 7bbd2f413b3e..53d3c92ae22e 100644
> --- a/security/landlock/Makefile
> +++ b/security/landlock/Makefile
> @@ -2,3 +2,5 @@ obj-$(CONFIG_SECURITY_LANDLOCK) := landlock.o
> 
>   landlock-y := setup.o syscalls.o object.o ruleset.o \
>   	cred.o ptrace.o fs.o
> +
> +landlock-$(CONFIG_INET) += net.o
> \ No newline at end of file
> diff --git a/security/landlock/net.c b/security/landlock/net.c
> new file mode 100644
> index 000000000000..39e8a156a1f4
> --- /dev/null
> +++ b/security/landlock/net.c
> @@ -0,0 +1,164 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Landlock LSM - Network management and hooks
> + *
> + * Copyright © 2022 Huawei Tech. Co., Ltd.
> + * Copyright © 2022 Microsoft Corporation
> + */
> +
> +#include <linux/in.h>
> +#include <linux/net.h>
> +#include <linux/socket.h>
> +#include <net/ipv6.h>
> +
> +#include "common.h"
> +#include "cred.h"
> +#include "limits.h"
> +#include "net.h"
> +#include "ruleset.h"
> +
> +int landlock_append_net_rule(struct landlock_ruleset *const ruleset,
> +			     const u16 port, access_mask_t access_rights)
> +{
> +	int err;
> +	const struct landlock_id id = {
> +		.key.data = port,
> +		.type = LANDLOCK_KEY_NET_PORT,
> +	};
> +	BUILD_BUG_ON(sizeof(port) > sizeof(id.key.data));
> +
> +	/* Transforms relative access rights to absolute ones. */
> +	access_rights |= LANDLOCK_MASK_ACCESS_NET &
> +			 ~landlock_get_net_access_mask(ruleset, 0);
> +
> +	mutex_lock(&ruleset->lock);
> +	err = landlock_insert_rule(ruleset, id, access_rights);
> +	mutex_unlock(&ruleset->lock);
> +
> +	return err;
> +}
> +
> +static int check_socket_access(const struct landlock_ruleset *const domain,
> +			       u16 port, access_mask_t access_request)
> +{
> +	bool allowed = false;
> +	layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_NET] = {};
> +	const struct landlock_rule *rule;
> +	access_mask_t handled_access;
> +	const struct landlock_id id = {
> +		.key.data = port,
> +		.type = LANDLOCK_KEY_NET_PORT,
> +	};
> +
> +	if (WARN_ON_ONCE(!domain))
> +		return 0;
> +	if (WARN_ON_ONCE(domain->num_layers < 1))
> +		return -EACCES;
> +
> +	rule = landlock_find_rule(domain, id);
> +	handled_access = init_layer_masks(domain, access_request, &layer_masks,
> +					  LANDLOCK_KEY_NET_PORT);
> +	allowed = unmask_layers(rule, handled_access, &layer_masks,
> +				ARRAY_SIZE(layer_masks));
> +
> +	return allowed ? 0 : -EACCES;
> +}
> +
> +static u16 get_port(const struct sockaddr *const address)

get_port() should return a __be16 type. This enables to avoid converting 
port when checking a rule.

make C=2 security/landlock/ must not print any warning.


> +{
> +	/* Gets port value in host byte order. */
> +	switch (address->sa_family) {
> +	case AF_UNSPEC:
> +	case AF_INET: {
> +		const struct sockaddr_in *const sockaddr =
> +			(struct sockaddr_in *)address;
> +		return ntohs(sockaddr->sin_port);
> +	}
> +#if IS_ENABLED(CONFIG_IPV6)
> +	case AF_INET6: {
> +		const struct sockaddr_in6 *const sockaddr_ip6 =
> +			(struct sockaddr_in6 *)address;
> +		return ntohs(sockaddr_ip6->sin6_port);
> +	}
> +#endif
> +	}
> +	WARN_ON_ONCE(1);
> +	return 0;
> +}
> +
> +static int hook_socket_bind(struct socket *sock, struct sockaddr *address,
> +			    int addrlen)
> +{
> +	const struct landlock_ruleset *const dom =
> +		landlock_get_current_domain();
> +
> +	if (!dom)
> +		return 0;
> +
> +	/* Check if it's a TCP socket. */
> +	if (sock->type != SOCK_STREAM)
> +		return 0;
> +
> +	switch (address->sa_family) {
> +	case AF_UNSPEC:
> +	case AF_INET:
> +#if IS_ENABLED(CONFIG_IPV6)
> +	case AF_INET6:
> +#endif
> +		return check_socket_access(dom, get_port(address),
> +					   LANDLOCK_ACCESS_NET_BIND_TCP);
> +	default:
> +		return 0;

You can remove this default case and move the return 0 at the end of the 
function.


> +	}
> +}
> +
> +static int hook_socket_connect(struct socket *sock, struct sockaddr *address,
> +			       int addrlen)
> +{
> +	const struct landlock_ruleset *const dom =
> +		landlock_get_current_domain();
> +
> +	if (!dom)
> +		return 0;
> +
> +	/* Check if it's a TCP socket. */
> +	if (sock->type != SOCK_STREAM)
> +		return 0;
> +
> +	/* Check if the hook is AF_INET* socket's action. */
> +	switch (address->sa_family) {
> +	case AF_INET:
> +#if IS_ENABLED(CONFIG_IPV6)
> +	case AF_INET6:
> +#endif
> +		return check_socket_access(dom, get_port(address),
> +					   LANDLOCK_ACCESS_NET_CONNECT_TCP);
> +	case AF_UNSPEC: {
> +		u16 i;

You can move "i" after the "dom" declaration to remove the extra braces.


> +
> +		/*
> +		 * If just in a layer a mask supports connect access,
> +		 * the socket_connect() hook with AF_UNSPEC family flag
> +		 * must be banned. This prevents from disconnecting already
> +		 * connected sockets.
> +		 */
> +		for (i = 0; i < dom->num_layers; i++) {
> +			if (landlock_get_net_access_mask(dom, i) &
> +			    LANDLOCK_ACCESS_NET_CONNECT_TCP)
> +				return -EACCES;

I'm wondering if this is the right error code for this case. EPERM may 
be more appropriate.

Thinking more about this case, I don't understand what is the rationale 
to deny such action. What would be the consequence to always allow 
connection with AF_UNSPEC (i.e. to disconnect a socket)?


> +		}
> +	}
> +	}
> +	return 0;
> +}
> +
> +static struct security_hook_list landlock_hooks[] __lsm_ro_after_init = {
> +	LSM_HOOK_INIT(socket_bind, hook_socket_bind),
> +	LSM_HOOK_INIT(socket_connect, hook_socket_connect),
> +};
> +
> +__init void landlock_add_net_hooks(void)
> +{
> +	security_add_hooks(landlock_hooks, ARRAY_SIZE(landlock_hooks),
> +			   LANDLOCK_NAME);
> +}
> diff --git a/security/landlock/net.h b/security/landlock/net.h
> new file mode 100644
> index 000000000000..0da1d9dff5ab
> --- /dev/null
> +++ b/security/landlock/net.h
> @@ -0,0 +1,26 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Landlock LSM - Network management and hooks
> + *
> + * Copyright © 2022 Huawei Tech. Co., Ltd.
> + */
> +
> +#ifndef _SECURITY_LANDLOCK_NET_H
> +#define _SECURITY_LANDLOCK_NET_H
> +
> +#include "common.h"
> +#include "ruleset.h"
> +#include "setup.h"
> +
> +#if IS_ENABLED(CONFIG_INET)
> +__init void landlock_add_net_hooks(void);
> +
> +int landlock_append_net_rule(struct landlock_ruleset *const ruleset,
> +			     const u16 port, access_mask_t access_rights);
> +#else /* IS_ENABLED(CONFIG_INET) */
> +static inline void landlock_add_net_hooks(void)
> +{
> +}
> +#endif /* IS_ENABLED(CONFIG_INET) */
> +
> +#endif /* _SECURITY_LANDLOCK_NET_H */
> diff --git a/security/landlock/setup.c b/security/landlock/setup.c
> index 3f196d2ce4f9..7e4a598177b8 100644
> --- a/security/landlock/setup.c
> +++ b/security/landlock/setup.c
> @@ -14,6 +14,7 @@
>   #include "fs.h"
>   #include "ptrace.h"
>   #include "setup.h"
> +#include "net.h"
> 
>   bool landlock_initialized __lsm_ro_after_init = false;
> 
> @@ -29,6 +30,7 @@ static int __init landlock_init(void)
>   	landlock_add_cred_hooks();
>   	landlock_add_ptrace_hooks();
>   	landlock_add_fs_hooks();
> +	landlock_add_net_hooks();
>   	landlock_initialized = true;
>   	pr_info("Up and running.\n");
>   	return 0;
> diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c
> index c5a6ad4e2fca..7853f32e8325 100644
> --- a/security/landlock/syscalls.c
> +++ b/security/landlock/syscalls.c
> @@ -29,6 +29,7 @@
>   #include "cred.h"
>   #include "fs.h"
>   #include "limits.h"
> +#include "net.h"
>   #include "ruleset.h"
>   #include "setup.h"
> 
> @@ -74,7 +75,8 @@ static void build_check_abi(void)
>   {
>   	struct landlock_ruleset_attr ruleset_attr;
>   	struct landlock_path_beneath_attr path_beneath_attr;
> -	size_t ruleset_size, path_beneath_size;
> +	struct landlock_net_service_attr net_service_attr;
> +	size_t ruleset_size, path_beneath_size, net_service_size;
> 
>   	/*
>   	 * For each user space ABI structures, first checks that there is no
> @@ -90,6 +92,11 @@ static void build_check_abi(void)
>   	path_beneath_size += sizeof(path_beneath_attr.parent_fd);
>   	BUILD_BUG_ON(sizeof(path_beneath_attr) != path_beneath_size);
>   	BUILD_BUG_ON(sizeof(path_beneath_attr) != 12);
> +
> +	net_service_size = sizeof(net_service_attr.allowed_access);
> +	net_service_size += sizeof(net_service_attr.port);
> +	BUILD_BUG_ON(sizeof(net_service_attr) != net_service_size);
> +	BUILD_BUG_ON(sizeof(net_service_attr) != 10);
>   }
> 
>   /* Ruleset handling */
> @@ -322,13 +329,54 @@ static int add_rule_path_beneath(struct landlock_ruleset *const ruleset,
>   	return err;
>   }
> 
> +static int add_rule_net_service(struct landlock_ruleset *ruleset,
> +				const void __user *const rule_attr)
> +{
> +#if IS_ENABLED(CONFIG_INET)
> +	struct landlock_net_service_attr net_service_attr;
> +	int res;
> +	u32 mask;

access_mask_t mask;


> +
> +	/* Copies raw user space buffer, only one type for now. */
> +	res = copy_from_user(&net_service_attr, rule_attr,
> +			     sizeof(net_service_attr));
> +	if (res)
> +		return -EFAULT;
> +
> +	/*
> +	 * Informs about useless rule: empty allowed_access (i.e. deny rules)
> +	 * are ignored by network actions.
> +	 */
> +	if (!net_service_attr.allowed_access)
> +		return -ENOMSG;
> +
> +	/*
> +	 * Checks that allowed_access matches the @ruleset constraints
> +	 * (ruleset->access_masks[0] is automatically upgraded to 64-bits).
> +	 */
> +	mask = landlock_get_net_access_mask(ruleset, 0);
> +	if ((net_service_attr.allowed_access | mask) != mask)
> +		return -EINVAL;
> +
> +	/* Denies inserting a rule with port 0. */
> +	if (net_service_attr.port == 0)
> +		return -EINVAL;
> +
> +	/* Imports the new rule. */
> +	return landlock_append_net_rule(ruleset, net_service_attr.port,
> +					net_service_attr.allowed_access);
> +#else /* IS_ENABLED(CONFIG_INET) */
> +	return -EAFNOSUPPORT;
> +#endif /* IS_ENABLED(CONFIG_INET) */
> +}
> +
>   /**
>    * sys_landlock_add_rule - Add a new rule to a ruleset
>    *
>    * @ruleset_fd: File descriptor tied to the ruleset that should be extended
>    *		with the new rule.
> - * @rule_type: Identify the structure type pointed to by @rule_attr (only
> - *             %LANDLOCK_RULE_PATH_BENEATH for now).
> + * @rule_type: Identify the structure type pointed to by @rule_attr:
> + *             %LANDLOCK_RULE_PATH_BENEATH or %LANDLOCK_RULE_NET_SERVICE.
>    * @rule_attr: Pointer to a rule (only of type &struct
>    *             landlock_path_beneath_attr for now).
>    * @flags: Must be 0.
> @@ -339,6 +387,8 @@ static int add_rule_path_beneath(struct landlock_ruleset *const ruleset,
>    * Possible returned errors are:
>    *
>    * - %EOPNOTSUPP: Landlock is supported by the kernel but disabled at boot time;
> + * - %EAFNOSUPPORT: @rule_type is LANDLOCK_RULE_NET_SERVICE but TCP/IP is not

%LANDLOCK_RULE_NET_SERVICE


> + *   supported by the running kernel;
>    * - %EINVAL: @flags is not 0, or inconsistent access in the rule (i.e.
>    *   &landlock_path_beneath_attr.allowed_access is not a subset of the
>    *   ruleset handled accesses);
> @@ -373,6 +423,9 @@ SYSCALL_DEFINE4(landlock_add_rule, const int, ruleset_fd,
>   	case LANDLOCK_RULE_PATH_BENEATH:
>   		err = add_rule_path_beneath(ruleset, rule_attr);
>   		break;
> +	case LANDLOCK_RULE_NET_SERVICE:
> +		err = add_rule_net_service(ruleset, rule_attr);
> +		break;
>   	default:
>   		err = -EINVAL;
>   		break;
> --
> 2.25.1
> 

Re: [PATCH v8 07/12] landlock: Add network rules support

[Konstantin Meskhidze (A)]

11/17/2022 9:43 PM, Mickaël Salaün пишет:
> 
> On 21/10/2022 17:26, Konstantin Meskhidze wrote:
>> This commit adds network rules support in internal landlock functions
>> (presented in ruleset.c) and landlock_create_ruleset syscall.
> 
> …in the ruleset management helpers and the landlock_create_ruleset syscall.
> 
> 
>> Refactors user space API to support network actions. Adds new network
> 
> Refactor…
> 
>> access flags, network rule and network attributes. Increments Landlock
> 
> Increment…

   The commit's message will be fixed. Thank you!
> 
>> ABI version.
>> 
>> Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze@huawei.com>
>> ---
>> 
>> Changes since v7:
>> * Squashes commits.
>> * Increments ABI version to 4.
>> * Refactors commit message.
>> * Minor fixes.
>> 
>> Changes since v6:
>> * Renames landlock_set_net_access_mask() to landlock_add_net_access_mask()
>>    because it OR values.
>> * Makes landlock_add_net_access_mask() more resilient incorrect values.
>> * Refactors landlock_get_net_access_mask().
>> * Renames LANDLOCK_MASK_SHIFT_NET to LANDLOCK_SHIFT_ACCESS_NET and use
>>    LANDLOCK_NUM_ACCESS_FS as value.
>> * Updates access_masks_t to u32 to support network access actions.
>> * Refactors landlock internal functions to support network actions with
>>    landlock_key/key_type/id types.
>> 
>> Changes since v5:
>> * Gets rid of partial revert from landlock_add_rule
>> syscall.
>> * Formats code with clang-format-14.
>> 
>> Changes since v4:
>> * Refactors landlock_create_ruleset() - splits ruleset and
>> masks checks.
>> * Refactors landlock_create_ruleset() and landlock mask
>> setters/getters to support two rule types.
>> * Refactors landlock_add_rule syscall add_rule_path_beneath
>> function by factoring out get_ruleset_from_fd() and
>> landlock_put_ruleset().
>> 
>> Changes since v3:
>> * Splits commit.
>> * Adds network rule support for internal landlock functions.
>> * Adds set_mask and get_mask for network.
>> * Adds rb_root root_net_port.
>> 
>> ---
>>   include/uapi/linux/landlock.h                | 49 ++++++++++++++
>>   security/landlock/limits.h                   |  6 +-
>>   security/landlock/ruleset.c                  | 55 ++++++++++++++--
>>   security/landlock/ruleset.h                  | 68 ++++++++++++++++----
>>   security/landlock/syscalls.c                 | 13 +++-
>>   tools/testing/selftests/landlock/base_test.c |  2 +-
>>   6 files changed, 170 insertions(+), 23 deletions(-)
>> 
>> diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
>> index f3223f964691..096b683c6ff3 100644
>> --- a/include/uapi/linux/landlock.h
>> +++ b/include/uapi/linux/landlock.h
>> @@ -31,6 +31,13 @@ struct landlock_ruleset_attr {
>>   	 * this access right.
>>   	 */
>>   	__u64 handled_access_fs;
>> +
>> +	/**
>> +	 * @handled_access_net: Bitmask of actions (cf. `Network flags`_)
>> +	 * that is handled by this ruleset and should then be forbidden if no
>> +	 * rule explicitly allow them.
>> +	 */
>> +	__u64 handled_access_net;
>>   };
>> 
>>   /*
>> @@ -54,6 +61,11 @@ enum landlock_rule_type {
>>   	 * landlock_path_beneath_attr .
>>   	 */
>>   	LANDLOCK_RULE_PATH_BENEATH = 1,
>> +	/**
>> +	 * @LANDLOCK_RULE_NET_SERVICE: Type of a &struct
>> +	 * landlock_net_service_attr .
>> +	 */
>> +	LANDLOCK_RULE_NET_SERVICE = 2,
>>   };
>> 
>>   /**
>> @@ -79,6 +91,24 @@ struct landlock_path_beneath_attr {
>>   	 */
>>   } __attribute__((packed));
>> 
>> +/**
>> + * struct landlock_net_service_attr - TCP subnet definition
>> + *
>> + * Argument of sys_landlock_add_rule().
>> + */
>> +struct landlock_net_service_attr {
>> +	/**
>> +	 * @allowed_access: Bitmask of allowed access network for services
>> +	 * (cf. `Network flags`_).
>> +	 */
>> +	__u64 allowed_access;
>> +	/**
>> +	 * @port: Network port.
>> +	 */
>> +	__u16 port;
> 
>   From an UAPI point of view, I think the port field should be __be16, as
> for sockaddr_in->port and other network-related APIs. This will require
> some kernel changes to please sparse: make C=2 security/landlock/ must
> not print any warning.

   Is sparse a default checker?
> 
> Using big-endian values as keys (casted to uintptr_t, not strictly
> __be16) in the rb-tree should not be an issue because there is no port
> range ordering (for now).
> 
> A dedicated test should check that endianness is correct, e.g. by using
> different port encoding. This should include passing and failing tests,
> but they should work on all architectures (i.e. big or little endian).
> 
   I got it. Will be fixed.
> 
>> +
>> +} __attribute__((packed));
>> +
>>   /**
>>    * DOC: fs_access
>>    *
>> @@ -173,4 +203,23 @@ struct landlock_path_beneath_attr {
>>   #define LANDLOCK_ACCESS_FS_TRUNCATE			(1ULL << 14)
>>   /* clang-format on */
>> 
>> +/**
>> + * DOC: net_access
>> + *
>> + * Network flags
>> + * ~~~~~~~~~~~~~~~~
>> + *
>> + * These flags enable to restrict a sandboxed process to a set of network
>> + * actions.
>> + *
>> + * TCP sockets with allowed actions:
>> + *
>> + * - %LANDLOCK_ACCESS_NET_BIND_TCP: Bind a TCP socket to a local port.
>> + * - %LANDLOCK_ACCESS_NET_CONNECT_TCP: Connect an active TCP socket to
>> + *   a remote port.
>> + */
>> +/* clang-format off */
>> +#define LANDLOCK_ACCESS_NET_BIND_TCP			(1ULL << 0)
>> +#define LANDLOCK_ACCESS_NET_CONNECT_TCP			(1ULL << 1)
>> +/* clang-format on */
>>   #endif /* _UAPI_LINUX_LANDLOCK_H */
>> diff --git a/security/landlock/limits.h b/security/landlock/limits.h
>> index bafb3b8dc677..8a1a6463c64e 100644
>> --- a/security/landlock/limits.h
>> +++ b/security/landlock/limits.h
>> @@ -23,6 +23,10 @@
>>   #define LANDLOCK_NUM_ACCESS_FS		__const_hweight64(LANDLOCK_MASK_ACCESS_FS)
>>   #define LANDLOCK_SHIFT_ACCESS_FS	0
>> 
>> -/* clang-format on */
>> +#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 */
>>   #endif /* _SECURITY_LANDLOCK_LIMITS_H */
>> diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
>> index c7cf54ba4f6d..9277c1295114 100644
>> --- a/security/landlock/ruleset.c
>> +++ b/security/landlock/ruleset.c
>> @@ -36,6 +36,9 @@ static struct landlock_ruleset *create_ruleset(const u32 num_layers)
>>   	refcount_set(&new_ruleset->usage, 1);
>>   	mutex_init(&new_ruleset->lock);
>>   	new_ruleset->root_inode = RB_ROOT;
>> +#if IS_ENABLED(CONFIG_INET)
>> +	new_ruleset->root_net_port = RB_ROOT;
>> +#endif /* IS_ENABLED(CONFIG_INET) */
>>   	new_ruleset->num_layers = num_layers;
>>   	/*
>>   	 * hierarchy = NULL
>> @@ -46,16 +49,21 @@ static struct landlock_ruleset *create_ruleset(const u32 num_layers)
>>   }
>> 
>>   struct landlock_ruleset *
>> -landlock_create_ruleset(const access_mask_t fs_access_mask)
>> +landlock_create_ruleset(const access_mask_t fs_access_mask,
>> +			const access_mask_t net_access_mask)
>>   {
>>   	struct landlock_ruleset *new_ruleset;
>> 
>>   	/* Informs about useless ruleset. */
>> -	if (!fs_access_mask)
>> +	if (!fs_access_mask && !net_access_mask)
>>   		return ERR_PTR(-ENOMSG);
>>   	new_ruleset = create_ruleset(1);
>> -	if (!IS_ERR(new_ruleset))
>> +	if (IS_ERR(new_ruleset))
>> +		return new_ruleset;
>> +	if (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);
>>   	return new_ruleset;
>>   }
>> 
>> @@ -73,6 +81,10 @@ static inline bool is_object_pointer(const enum landlock_key_type key_type)
>>   	switch (key_type) {
>>   	case LANDLOCK_KEY_INODE:
>>   		return true;
>> +#if IS_ENABLED(CONFIG_INET)
>> +	case LANDLOCK_KEY_NET_PORT:
>> +		return false;
>> +#endif /* IS_ENABLED(CONFIG_INET) */
>>   	}
>>   	WARN_ON_ONCE(1);
>>   	return false;
>> @@ -126,6 +138,11 @@ static inline struct rb_root *get_root(struct landlock_ruleset *const ruleset,
>>   	case LANDLOCK_KEY_INODE:
>>   		root = &ruleset->root_inode;
>>   		break;
>> +#if IS_ENABLED(CONFIG_INET)
>> +	case LANDLOCK_KEY_NET_PORT:
>> +		root = &ruleset->root_net_port;
>> +		break;
>> +#endif /* IS_ENABLED(CONFIG_INET) */
>>   	}
>>   	if (WARN_ON_ONCE(!root))
>>   		return ERR_PTR(-EINVAL);
>> @@ -154,7 +171,9 @@ static void build_check_ruleset(void)
>>   	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_FS << LANDLOCK_SHIFT_ACCESS_FS) +
> 
> This is correct but because we are dealing with bitmasks I would prefer
> to use "|" instead of "+".

   Ok. I will refactor it.
> 
> 
>> +			     (LANDLOCK_MASK_ACCESS_NET
>> +			      << LANDLOCK_SHIFT_ACCESS_NET));
>>   }
>> 
>>   /**
>> @@ -370,7 +389,12 @@ static int merge_ruleset(struct landlock_ruleset *const dst,
>>   	err = merge_tree(dst, src, LANDLOCK_KEY_INODE);
>>   	if (err)
>>   		goto out_unlock;
>> -
> 
> Please keep this newline.

   Got it.
> 
>> +#if IS_ENABLED(CONFIG_INET)
>> +	/* Merges the @src network port tree. */
>> +	err = merge_tree(dst, src, LANDLOCK_KEY_NET_PORT);
>> +	if (err)
>> +		goto out_unlock;
>> +#endif /* IS_ENABLED(CONFIG_INET) */
>>   out_unlock:
>>   	mutex_unlock(&src->lock);
>>   	mutex_unlock(&dst->lock);
>> @@ -426,7 +450,12 @@ static int inherit_ruleset(struct landlock_ruleset *const parent,
>>   	err = inherit_tree(parent, child, LANDLOCK_KEY_INODE);
>>   	if (err)
>>   		goto out_unlock;
>> -
> 
> newline

  Ok.
> 
>> +#if IS_ENABLED(CONFIG_INET)
>> +	/* Copies the @parent network port tree. */
>> +	err = inherit_tree(parent, child, LANDLOCK_KEY_NET_PORT);
>> +	if (err)
>> +		goto out_unlock;
>> +#endif /* IS_ENABLED(CONFIG_INET) */
>>   	if (WARN_ON_ONCE(child->num_layers <= parent->num_layers)) {
>>   		err = -EINVAL;
>>   		goto out_unlock;
>> @@ -459,6 +488,11 @@ static void free_ruleset(struct landlock_ruleset *const ruleset)
>>   	rbtree_postorder_for_each_entry_safe(freeme, next, &ruleset->root_inode,
>>   					     node)
>>   		free_rule(freeme, LANDLOCK_KEY_INODE);
>> +#if IS_ENABLED(CONFIG_INET)
>> +	rbtree_postorder_for_each_entry_safe(freeme, next,
>> +					     &ruleset->root_net_port, node)
>> +		free_rule(freeme, LANDLOCK_KEY_NET_PORT);
>> +#endif /* IS_ENABLED(CONFIG_INET) */
>>   	put_hierarchy(ruleset->hierarchy);
>>   	kfree(ruleset);
>>   }
>> @@ -637,6 +671,9 @@ get_access_mask_t(const struct landlock_ruleset *const ruleset,
>>    * Populates @layer_masks such that for each access right in @access_request,
>>    * the bits for all the layers are set where this access right is handled.
>>    *
>> + * @layer_masks must contain LANDLOCK_NUM_ACCESS_FS or LANDLOCK_NUM_ACCESS_NET
>> + * elements according to @key_type.
> 
> Please include this sentence in the @layer_masks description below.

   Ok.
> 
>> + *
>>    * @domain: The domain that defines the current restrictions.
>>    * @access_request: The requested access rights to check.
>>    * @layer_masks: The layer masks to populate.
> 
> "It must contain…"
> 
> 
>> @@ -659,6 +696,12 @@ access_mask_t init_layer_masks(const struct landlock_ruleset *const domain,
>>   		get_access_mask = landlock_get_fs_access_mask;
>>   		num_access = LANDLOCK_NUM_ACCESS_FS;
>>   		break;
>> +#if IS_ENABLED(CONFIG_INET)
>> +	case LANDLOCK_KEY_NET_PORT:
>> +		get_access_mask = landlock_get_net_access_mask;
>> +		num_access = LANDLOCK_NUM_ACCESS_NET;
>> +		break;
>> +#endif /* IS_ENABLED(CONFIG_INET) */
>>   	default:
>>   		WARN_ON_ONCE(1);
>>   		return 0;
>> diff --git a/security/landlock/ruleset.h b/security/landlock/ruleset.h
>> index d9eb79ea9a89..f272d2cd518c 100644
>> --- a/security/landlock/ruleset.h
>> +++ b/security/landlock/ruleset.h
>> @@ -19,16 +19,20 @@
>>   #include "limits.h"
>>   #include "object.h"
>> 
>> +/* Rule access mask. */
>>   typedef u16 access_mask_t;
>>   /* Makes sure all filesystem access rights can be stored. */
>>   static_assert(BITS_PER_TYPE(access_mask_t) >= LANDLOCK_NUM_ACCESS_FS);
>> +/* Makes sure all network access rights can be stored. */
>> +static_assert(BITS_PER_TYPE(access_mask_t) >= LANDLOCK_NUM_ACCESS_NET);
>>   /* Makes sure for_each_set_bit() and for_each_clear_bit() calls are OK. */
>>   static_assert(sizeof(unsigned long) >= sizeof(access_mask_t));
>> 
>>   /* Ruleset access masks. */
>> -typedef u16 access_masks_t;
>> +typedef u32 access_masks_t;
> 
> This type change need to be explained in the commit message.
> 
   Ok. I will explain it.
> 
>>   /* Makes sure all ruleset access rights can be stored. */
>> -static_assert(BITS_PER_TYPE(access_masks_t) >= LANDLOCK_NUM_ACCESS_FS);
>> +static_assert(BITS_PER_TYPE(access_masks_t) >=
>> +	      LANDLOCK_NUM_ACCESS_FS + LANDLOCK_NUM_ACCESS_NET);
>> 
>>   typedef u16 layer_mask_t;
>>   /* Makes sure all layers can be checked. */
>> @@ -82,6 +86,13 @@ enum landlock_key_type {
>>   	 * keys.
>>   	 */
>>   	LANDLOCK_KEY_INODE = 1,
>> +#if IS_ENABLED(CONFIG_INET)
>> +	/**
>> +	 * @LANDLOCK_KEY_NET_PORT: Type of &landlock_ruleset.root_net_port's
>> +	 * node keys.
>> +	 */
>> +	LANDLOCK_KEY_NET_PORT = 2,
>> +#endif /* IS_ENABLED(CONFIG_INET) */
>>   };
>> 
>>   /**
>> @@ -156,6 +167,15 @@ struct landlock_ruleset {
>>   	 * reaches zero.
>>   	 */
>>   	struct rb_root root_inode;
>> +#if IS_ENABLED(CONFIG_INET)
>> +	/**
>> +	 * @root_net_port: Root of a red-black tree containing &struct
>> +	 * landlock_rule nodes with network port. 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_net_port;
>> +#endif /* IS_ENABLED(CONFIG_INET) */
>>   	/**
>>   	 * @hierarchy: Enables hierarchy identification even when a parent
>>   	 * domain vanishes.  This is needed for the ptrace protection.
>> @@ -166,8 +186,8 @@ struct landlock_ruleset {
>>   		 * @work_free: Enables to free a ruleset within a lockless
>>   		 * section.  This is only used by
>>   		 * landlock_put_ruleset_deferred() when @usage reaches zero.
>> -		 * The fields @lock, @usage, @num_rules, @num_layers and
>> -		 * @access_masks are then unused.
>> +		 * The fields @lock, @usage, @num_rules, @num_layers,
>> +		 * @net_access_mask and @access_masks are then unused.
> 
> There is no net_access_mask anymore.
> .

Re: [PATCH v8 08/12] landlock: Implement TCP network hooks

[Konstantin Meskhidze (A)]

11/17/2022 9:43 PM, Mickaël Salaün пишет:
> 
> On 21/10/2022 17:26, Konstantin Meskhidze wrote:
>> This patch adds support of socket_bind() and socket_connect() hooks.
>> It's possible to restrict binding and connecting of TCP sockets to
>> particular ports.
> 
> Implement socket_bind() and socket_connect LSM hooks, which enable to
> restrict TCP socket binding and connection to specific ports.
> 
   Ok. Thanks.
> 
>> 
>> Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze@huawei.com>
>> ---
>> 
>> Changes since v7:
>> * Minor fixes.
>> * Refactors commit message.
>> 
>> Changes since v6:
>> * Updates copyright.
>> * Refactors landlock_append_net_rule() and check_socket_access()
>>    functions with landlock_id type.
>> 
>> Changes since v5:
>> * Fixes some logic errors.
>> * Formats code with clang-format-14.
>> 
>> Changes since v4:
>> * Factors out CONFIG_INET into make file.
>> * Refactors check_socket_access().
>> * Adds helper get_port().
>> * Adds CONFIG_IPV6 in get_port(), hook_socket_bind/connect
>> functions to support AF_INET6 family.
>> * Adds AF_UNSPEC family support in hook_socket_bind/connect
>> functions.
>> * Refactors add_rule_net_service() and landlock_add_rule
>> syscall to support network rule inserting.
>> * Refactors init_layer_masks() to support network rules.
>> 
>> Changes since v3:
>> * Splits commit.
>> * Adds SECURITY_NETWORK in config.
>> * Adds IS_ENABLED(CONFIG_INET) if a kernel has no INET configuration.
>> * Adds hook_socket_bind and hook_socket_connect hooks.
>> 
>> ---
>>   security/landlock/Kconfig    |   1 +
>>   security/landlock/Makefile   |   2 +
>>   security/landlock/net.c      | 164 +++++++++++++++++++++++++++++++++++
>>   security/landlock/net.h      |  26 ++++++
>>   security/landlock/setup.c    |   2 +
>>   security/landlock/syscalls.c |  59 ++++++++++++-
>>   6 files changed, 251 insertions(+), 3 deletions(-)
>>   create mode 100644 security/landlock/net.c
>>   create mode 100644 security/landlock/net.h
>> 
>> diff --git a/security/landlock/Kconfig b/security/landlock/Kconfig
>> index 8e33c4e8ffb8..10c099097533 100644
>> --- a/security/landlock/Kconfig
>> +++ b/security/landlock/Kconfig
>> @@ -3,6 +3,7 @@
>>   config SECURITY_LANDLOCK
>>   	bool "Landlock support"
>>   	depends on SECURITY && !ARCH_EPHEMERAL_INODES
>> +	select SECURITY_NETWORK
>>   	select SECURITY_PATH
>>   	help
>>   	  Landlock is a sandboxing mechanism that enables processes to restrict
>> diff --git a/security/landlock/Makefile b/security/landlock/Makefile
>> index 7bbd2f413b3e..53d3c92ae22e 100644
>> --- a/security/landlock/Makefile
>> +++ b/security/landlock/Makefile
>> @@ -2,3 +2,5 @@ obj-$(CONFIG_SECURITY_LANDLOCK) := landlock.o
>> 
>>   landlock-y := setup.o syscalls.o object.o ruleset.o \
>>   	cred.o ptrace.o fs.o
>> +
>> +landlock-$(CONFIG_INET) += net.o
>> \ No newline at end of file
>> diff --git a/security/landlock/net.c b/security/landlock/net.c
>> new file mode 100644
>> index 000000000000..39e8a156a1f4
>> --- /dev/null
>> +++ b/security/landlock/net.c
>> @@ -0,0 +1,164 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Landlock LSM - Network management and hooks
>> + *
>> + * Copyright © 2022 Huawei Tech. Co., Ltd.
>> + * Copyright © 2022 Microsoft Corporation
>> + */
>> +
>> +#include <linux/in.h>
>> +#include <linux/net.h>
>> +#include <linux/socket.h>
>> +#include <net/ipv6.h>
>> +
>> +#include "common.h"
>> +#include "cred.h"
>> +#include "limits.h"
>> +#include "net.h"
>> +#include "ruleset.h"
>> +
>> +int landlock_append_net_rule(struct landlock_ruleset *const ruleset,
>> +			     const u16 port, access_mask_t access_rights)
>> +{
>> +	int err;
>> +	const struct landlock_id id = {
>> +		.key.data = port,
>> +		.type = LANDLOCK_KEY_NET_PORT,
>> +	};
>> +	BUILD_BUG_ON(sizeof(port) > sizeof(id.key.data));
>> +
>> +	/* Transforms relative access rights to absolute ones. */
>> +	access_rights |= LANDLOCK_MASK_ACCESS_NET &
>> +			 ~landlock_get_net_access_mask(ruleset, 0);
>> +
>> +	mutex_lock(&ruleset->lock);
>> +	err = landlock_insert_rule(ruleset, id, access_rights);
>> +	mutex_unlock(&ruleset->lock);
>> +
>> +	return err;
>> +}
>> +
>> +static int check_socket_access(const struct landlock_ruleset *const domain,
>> +			       u16 port, access_mask_t access_request)
>> +{
>> +	bool allowed = false;
>> +	layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_NET] = {};
>> +	const struct landlock_rule *rule;
>> +	access_mask_t handled_access;
>> +	const struct landlock_id id = {
>> +		.key.data = port,
>> +		.type = LANDLOCK_KEY_NET_PORT,
>> +	};
>> +
>> +	if (WARN_ON_ONCE(!domain))
>> +		return 0;
>> +	if (WARN_ON_ONCE(domain->num_layers < 1))
>> +		return -EACCES;
>> +
>> +	rule = landlock_find_rule(domain, id);
>> +	handled_access = init_layer_masks(domain, access_request, &layer_masks,
>> +					  LANDLOCK_KEY_NET_PORT);
>> +	allowed = unmask_layers(rule, handled_access, &layer_masks,
>> +				ARRAY_SIZE(layer_masks));
>> +
>> +	return allowed ? 0 : -EACCES;
>> +}
>> +
>> +static u16 get_port(const struct sockaddr *const address)
> 
> get_port() should return a __be16 type. This enables to avoid converting
> port when checking a rule.
> 
> make C=2 security/landlock/ must not print any warning.

   Got it.
> 
> 
>> +{
>> +	/* Gets port value in host byte order. */
>> +	switch (address->sa_family) {
>> +	case AF_UNSPEC:
>> +	case AF_INET: {
>> +		const struct sockaddr_in *const sockaddr =
>> +			(struct sockaddr_in *)address;
>> +		return ntohs(sockaddr->sin_port);
>> +	}
>> +#if IS_ENABLED(CONFIG_IPV6)
>> +	case AF_INET6: {
>> +		const struct sockaddr_in6 *const sockaddr_ip6 =
>> +			(struct sockaddr_in6 *)address;
>> +		return ntohs(sockaddr_ip6->sin6_port);
>> +	}
>> +#endif
>> +	}
>> +	WARN_ON_ONCE(1);
>> +	return 0;
>> +}
>> +
>> +static int hook_socket_bind(struct socket *sock, struct sockaddr *address,
>> +			    int addrlen)
>> +{
>> +	const struct landlock_ruleset *const dom =
>> +		landlock_get_current_domain();
>> +
>> +	if (!dom)
>> +		return 0;
>> +
>> +	/* Check if it's a TCP socket. */
>> +	if (sock->type != SOCK_STREAM)
>> +		return 0;
>> +
>> +	switch (address->sa_family) {
>> +	case AF_UNSPEC:
>> +	case AF_INET:
>> +#if IS_ENABLED(CONFIG_IPV6)
>> +	case AF_INET6:
>> +#endif
>> +		return check_socket_access(dom, get_port(address),
>> +					   LANDLOCK_ACCESS_NET_BIND_TCP);
>> +	default:
>> +		return 0;
> 
> You can remove this default case and move the return 0 at the end of the
> function.
> 
   Ok. Will be refactored.
> 
>> +	}
>> +}
>> +
>> +static int hook_socket_connect(struct socket *sock, struct sockaddr *address,
>> +			       int addrlen)
>> +{
>> +	const struct landlock_ruleset *const dom =
>> +		landlock_get_current_domain();
>> +
>> +	if (!dom)
>> +		return 0;
>> +
>> +	/* Check if it's a TCP socket. */
>> +	if (sock->type != SOCK_STREAM)
>> +		return 0;
>> +
>> +	/* Check if the hook is AF_INET* socket's action. */
>> +	switch (address->sa_family) {
>> +	case AF_INET:
>> +#if IS_ENABLED(CONFIG_IPV6)
>> +	case AF_INET6:
>> +#endif
>> +		return check_socket_access(dom, get_port(address),
>> +					   LANDLOCK_ACCESS_NET_CONNECT_TCP);
>> +	case AF_UNSPEC: {
>> +		u16 i;
> 
> You can move "i" after the "dom" declaration to remove the extra braces.
> 
   Ok. Thanks.
> 
>> +
>> +		/*
>> +		 * If just in a layer a mask supports connect access,
>> +		 * the socket_connect() hook with AF_UNSPEC family flag
>> +		 * must be banned. This prevents from disconnecting already
>> +		 * connected sockets.
>> +		 */
>> +		for (i = 0; i < dom->num_layers; i++) {
>> +			if (landlock_get_net_access_mask(dom, i) &
>> +			    LANDLOCK_ACCESS_NET_CONNECT_TCP)
>> +				return -EACCES;
> 
> I'm wondering if this is the right error code for this case. EPERM may
> be more appropriate.

   Ok. Will be refactored.
> 
> Thinking more about this case, I don't understand what is the rationale
> to deny such action. What would be the consequence to always allow
> connection with AF_UNSPEC (i.e. to disconnect a socket)?
> 
   I thought we have come to a conclusion about connect(...AF_UNSPEC..) 
  behaviour in the patchset V3:
https://lore.kernel.org/linux-security-module/19ad3a01-d76e-0e73-7833-99acd4afd97e@huawei.com/
> 
>> +		}
>> +	}
>> +	}
>> +	return 0;
>> +}
>> +
>> +static struct security_hook_list landlock_hooks[] __lsm_ro_after_init = {
>> +	LSM_HOOK_INIT(socket_bind, hook_socket_bind),
>> +	LSM_HOOK_INIT(socket_connect, hook_socket_connect),
>> +};
>> +
>> +__init void landlock_add_net_hooks(void)
>> +{
>> +	security_add_hooks(landlock_hooks, ARRAY_SIZE(landlock_hooks),
>> +			   LANDLOCK_NAME);
>> +}
>> diff --git a/security/landlock/net.h b/security/landlock/net.h
>> new file mode 100644
>> index 000000000000..0da1d9dff5ab
>> --- /dev/null
>> +++ b/security/landlock/net.h
>> @@ -0,0 +1,26 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * Landlock LSM - Network management and hooks
>> + *
>> + * Copyright © 2022 Huawei Tech. Co., Ltd.
>> + */
>> +
>> +#ifndef _SECURITY_LANDLOCK_NET_H
>> +#define _SECURITY_LANDLOCK_NET_H
>> +
>> +#include "common.h"
>> +#include "ruleset.h"
>> +#include "setup.h"
>> +
>> +#if IS_ENABLED(CONFIG_INET)
>> +__init void landlock_add_net_hooks(void);
>> +
>> +int landlock_append_net_rule(struct landlock_ruleset *const ruleset,
>> +			     const u16 port, access_mask_t access_rights);
>> +#else /* IS_ENABLED(CONFIG_INET) */
>> +static inline void landlock_add_net_hooks(void)
>> +{
>> +}
>> +#endif /* IS_ENABLED(CONFIG_INET) */
>> +
>> +#endif /* _SECURITY_LANDLOCK_NET_H */
>> diff --git a/security/landlock/setup.c b/security/landlock/setup.c
>> index 3f196d2ce4f9..7e4a598177b8 100644
>> --- a/security/landlock/setup.c
>> +++ b/security/landlock/setup.c
>> @@ -14,6 +14,7 @@
>>   #include "fs.h"
>>   #include "ptrace.h"
>>   #include "setup.h"
>> +#include "net.h"
>> 
>>   bool landlock_initialized __lsm_ro_after_init = false;
>> 
>> @@ -29,6 +30,7 @@ static int __init landlock_init(void)
>>   	landlock_add_cred_hooks();
>>   	landlock_add_ptrace_hooks();
>>   	landlock_add_fs_hooks();
>> +	landlock_add_net_hooks();
>>   	landlock_initialized = true;
>>   	pr_info("Up and running.\n");
>>   	return 0;
>> diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c
>> index c5a6ad4e2fca..7853f32e8325 100644
>> --- a/security/landlock/syscalls.c
>> +++ b/security/landlock/syscalls.c
>> @@ -29,6 +29,7 @@
>>   #include "cred.h"
>>   #include "fs.h"
>>   #include "limits.h"
>> +#include "net.h"
>>   #include "ruleset.h"
>>   #include "setup.h"
>> 
>> @@ -74,7 +75,8 @@ static void build_check_abi(void)
>>   {
>>   	struct landlock_ruleset_attr ruleset_attr;
>>   	struct landlock_path_beneath_attr path_beneath_attr;
>> -	size_t ruleset_size, path_beneath_size;
>> +	struct landlock_net_service_attr net_service_attr;
>> +	size_t ruleset_size, path_beneath_size, net_service_size;
>> 
>>   	/*
>>   	 * For each user space ABI structures, first checks that there is no
>> @@ -90,6 +92,11 @@ static void build_check_abi(void)
>>   	path_beneath_size += sizeof(path_beneath_attr.parent_fd);
>>   	BUILD_BUG_ON(sizeof(path_beneath_attr) != path_beneath_size);
>>   	BUILD_BUG_ON(sizeof(path_beneath_attr) != 12);
>> +
>> +	net_service_size = sizeof(net_service_attr.allowed_access);
>> +	net_service_size += sizeof(net_service_attr.port);
>> +	BUILD_BUG_ON(sizeof(net_service_attr) != net_service_size);
>> +	BUILD_BUG_ON(sizeof(net_service_attr) != 10);
>>   }
>> 
>>   /* Ruleset handling */
>> @@ -322,13 +329,54 @@ static int add_rule_path_beneath(struct landlock_ruleset *const ruleset,
>>   	return err;
>>   }
>> 
>> +static int add_rule_net_service(struct landlock_ruleset *ruleset,
>> +				const void __user *const rule_attr)
>> +{
>> +#if IS_ENABLED(CONFIG_INET)
>> +	struct landlock_net_service_attr net_service_attr;
>> +	int res;
>> +	u32 mask;
> 
> access_mask_t mask;

  Got it. Thanks.
> 
> 
>> +
>> +	/* Copies raw user space buffer, only one type for now. */
>> +	res = copy_from_user(&net_service_attr, rule_attr,
>> +			     sizeof(net_service_attr));
>> +	if (res)
>> +		return -EFAULT;
>> +
>> +	/*
>> +	 * Informs about useless rule: empty allowed_access (i.e. deny rules)
>> +	 * are ignored by network actions.
>> +	 */
>> +	if (!net_service_attr.allowed_access)
>> +		return -ENOMSG;
>> +
>> +	/*
>> +	 * Checks that allowed_access matches the @ruleset constraints
>> +	 * (ruleset->access_masks[0] is automatically upgraded to 64-bits).
>> +	 */
>> +	mask = landlock_get_net_access_mask(ruleset, 0);
>> +	if ((net_service_attr.allowed_access | mask) != mask)
>> +		return -EINVAL;
>> +
>> +	/* Denies inserting a rule with port 0. */
>> +	if (net_service_attr.port == 0)
>> +		return -EINVAL;
>> +
>> +	/* Imports the new rule. */
>> +	return landlock_append_net_rule(ruleset, net_service_attr.port,
>> +					net_service_attr.allowed_access);
>> +#else /* IS_ENABLED(CONFIG_INET) */
>> +	return -EAFNOSUPPORT;
>> +#endif /* IS_ENABLED(CONFIG_INET) */
>> +}
>> +
>>   /**
>>    * sys_landlock_add_rule - Add a new rule to a ruleset
>>    *
>>    * @ruleset_fd: File descriptor tied to the ruleset that should be extended
>>    *		with the new rule.
>> - * @rule_type: Identify the structure type pointed to by @rule_attr (only
>> - *             %LANDLOCK_RULE_PATH_BENEATH for now).
>> + * @rule_type: Identify the structure type pointed to by @rule_attr:
>> + *             %LANDLOCK_RULE_PATH_BENEATH or %LANDLOCK_RULE_NET_SERVICE.
>>    * @rule_attr: Pointer to a rule (only of type &struct
>>    *             landlock_path_beneath_attr for now).
>>    * @flags: Must be 0.
>> @@ -339,6 +387,8 @@ static int add_rule_path_beneath(struct landlock_ruleset *const ruleset,
>>    * Possible returned errors are:
>>    *
>>    * - %EOPNOTSUPP: Landlock is supported by the kernel but disabled at boot time;
>> + * - %EAFNOSUPPORT: @rule_type is LANDLOCK_RULE_NET_SERVICE but TCP/IP is not
> 
> %LANDLOCK_RULE_NET_SERVICE

  Ok.
> 
> 
>> + *   supported by the running kernel;
>>    * - %EINVAL: @flags is not 0, or inconsistent access in the rule (i.e.
>>    *   &landlock_path_beneath_attr.allowed_access is not a subset of the
>>    *   ruleset handled accesses);
>> @@ -373,6 +423,9 @@ SYSCALL_DEFINE4(landlock_add_rule, const int, ruleset_fd,
>>   	case LANDLOCK_RULE_PATH_BENEATH:
>>   		err = add_rule_path_beneath(ruleset, rule_attr);
>>   		break;
>> +	case LANDLOCK_RULE_NET_SERVICE:
>> +		err = add_rule_net_service(ruleset, rule_attr);
>> +		break;
>>   	default:
>>   		err = -EINVAL;
>>   		break;
>> --
>> 2.25.1
>> 
> .

Re: [PATCH v8 07/12] landlock: Add network rules support

[Mickaël Salaün]

On 28/11/2022 05:01, Konstantin Meskhidze (A) wrote:
> 
> 
> 11/17/2022 9:43 PM, Mickaël Salaün пишет:
>>
>> On 21/10/2022 17:26, Konstantin Meskhidze wrote:
>>> This commit adds network rules support in internal landlock functions
>>> (presented in ruleset.c) and landlock_create_ruleset syscall.
>>
>> …in the ruleset management helpers and the landlock_create_ruleset syscall.
>>
>>
>>> Refactors user space API to support network actions. Adds new network
>>
>> Refactor…
>>
>>> access flags, network rule and network attributes. Increments Landlock
>>
>> Increment…
> 
>     The commit's message will be fixed. Thank you!
>>
>>> ABI version.
>>>
>>> Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze@huawei.com>
>>> ---
>>>
>>> Changes since v7:
>>> * Squashes commits.
>>> * Increments ABI version to 4.
>>> * Refactors commit message.
>>> * Minor fixes.
>>>
>>> Changes since v6:
>>> * Renames landlock_set_net_access_mask() to landlock_add_net_access_mask()
>>>     because it OR values.
>>> * Makes landlock_add_net_access_mask() more resilient incorrect values.
>>> * Refactors landlock_get_net_access_mask().
>>> * Renames LANDLOCK_MASK_SHIFT_NET to LANDLOCK_SHIFT_ACCESS_NET and use
>>>     LANDLOCK_NUM_ACCESS_FS as value.
>>> * Updates access_masks_t to u32 to support network access actions.
>>> * Refactors landlock internal functions to support network actions with
>>>     landlock_key/key_type/id types.
>>>
>>> Changes since v5:
>>> * Gets rid of partial revert from landlock_add_rule
>>> syscall.
>>> * Formats code with clang-format-14.
>>>
>>> Changes since v4:
>>> * Refactors landlock_create_ruleset() - splits ruleset and
>>> masks checks.
>>> * Refactors landlock_create_ruleset() and landlock mask
>>> setters/getters to support two rule types.
>>> * Refactors landlock_add_rule syscall add_rule_path_beneath
>>> function by factoring out get_ruleset_from_fd() and
>>> landlock_put_ruleset().
>>>
>>> Changes since v3:
>>> * Splits commit.
>>> * Adds network rule support for internal landlock functions.
>>> * Adds set_mask and get_mask for network.
>>> * Adds rb_root root_net_port.
>>>
>>> ---
>>>    include/uapi/linux/landlock.h                | 49 ++++++++++++++
>>>    security/landlock/limits.h                   |  6 +-
>>>    security/landlock/ruleset.c                  | 55 ++++++++++++++--
>>>    security/landlock/ruleset.h                  | 68 ++++++++++++++++----
>>>    security/landlock/syscalls.c                 | 13 +++-
>>>    tools/testing/selftests/landlock/base_test.c |  2 +-
>>>    6 files changed, 170 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
>>> index f3223f964691..096b683c6ff3 100644
>>> --- a/include/uapi/linux/landlock.h
>>> +++ b/include/uapi/linux/landlock.h
>>> @@ -31,6 +31,13 @@ struct landlock_ruleset_attr {
>>>    	 * this access right.
>>>    	 */
>>>    	__u64 handled_access_fs;
>>> +
>>> +	/**
>>> +	 * @handled_access_net: Bitmask of actions (cf. `Network flags`_)
>>> +	 * that is handled by this ruleset and should then be forbidden if no
>>> +	 * rule explicitly allow them.
>>> +	 */
>>> +	__u64 handled_access_net;
>>>    };
>>>
>>>    /*
>>> @@ -54,6 +61,11 @@ enum landlock_rule_type {
>>>    	 * landlock_path_beneath_attr .
>>>    	 */
>>>    	LANDLOCK_RULE_PATH_BENEATH = 1,
>>> +	/**
>>> +	 * @LANDLOCK_RULE_NET_SERVICE: Type of a &struct
>>> +	 * landlock_net_service_attr .
>>> +	 */
>>> +	LANDLOCK_RULE_NET_SERVICE = 2,
>>>    };
>>>
>>>    /**
>>> @@ -79,6 +91,24 @@ struct landlock_path_beneath_attr {
>>>    	 */
>>>    } __attribute__((packed));
>>>
>>> +/**
>>> + * struct landlock_net_service_attr - TCP subnet definition
>>> + *
>>> + * Argument of sys_landlock_add_rule().
>>> + */
>>> +struct landlock_net_service_attr {
>>> +	/**
>>> +	 * @allowed_access: Bitmask of allowed access network for services
>>> +	 * (cf. `Network flags`_).
>>> +	 */
>>> +	__u64 allowed_access;
>>> +	/**
>>> +	 * @port: Network port.
>>> +	 */
>>> +	__u16 port;
>>
>>    From an UAPI point of view, I think the port field should be __be16, as
>> for sockaddr_in->port and other network-related APIs. This will require
>> some kernel changes to please sparse: make C=2 security/landlock/ must
>> not print any warning.
> 
>     Is sparse a default checker?

You should be able to easily install it with your Linux distro.

Re: [PATCH v8 08/12] landlock: Implement TCP network hooks

[Mickaël Salaün]

The previous commit provides an interface to theoretically restrict 
network access (i.e. ruleset handled network accesses), but in fact this 
is not enforced until this commit. I like this split but to avoid any 
inconsistency, please squash this commit into the previous one: "7/12 
landlock: Add network rules support"
You should keep all the commit messages but maybe tweak them a bit.


On 28/11/2022 09:21, Konstantin Meskhidze (A) wrote:
> 
> 
> 11/17/2022 9:43 PM, Mickaël Salaün пишет:
>>
>> On 21/10/2022 17:26, Konstantin Meskhidze wrote:
>>> This patch adds support of socket_bind() and socket_connect() hooks.
>>> It's possible to restrict binding and connecting of TCP sockets to
>>> particular ports.
>>
>> Implement socket_bind() and socket_connect LSM hooks, which enable to
>> restrict TCP socket binding and connection to specific ports.
>>
>     Ok. Thanks.
>>
>>>
>>> Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze@huawei.com>
>>> ---

[...]

>>> +static int hook_socket_connect(struct socket *sock, struct sockaddr *address,
>>> +			       int addrlen)
>>> +{
>>> +	const struct landlock_ruleset *const dom =
>>> +		landlock_get_current_domain();
>>> +
>>> +	if (!dom)
>>> +		return 0;
>>> +
>>> +	/* Check if it's a TCP socket. */
>>> +	if (sock->type != SOCK_STREAM)
>>> +		return 0;
>>> +
>>> +	/* Check if the hook is AF_INET* socket's action. */
>>> +	switch (address->sa_family) {
>>> +	case AF_INET:
>>> +#if IS_ENABLED(CONFIG_IPV6)
>>> +	case AF_INET6:
>>> +#endif
>>> +		return check_socket_access(dom, get_port(address),
>>> +					   LANDLOCK_ACCESS_NET_CONNECT_TCP);
>>> +	case AF_UNSPEC: {
>>> +		u16 i;
>>
>> You can move "i" after the "dom" declaration to remove the extra braces.
>>
>     Ok. Thanks.
>>
>>> +
>>> +		/*
>>> +		 * If just in a layer a mask supports connect access,
>>> +		 * the socket_connect() hook with AF_UNSPEC family flag
>>> +		 * must be banned. This prevents from disconnecting already
>>> +		 * connected sockets.
>>> +		 */
>>> +		for (i = 0; i < dom->num_layers; i++) {
>>> +			if (landlock_get_net_access_mask(dom, i) &
>>> +			    LANDLOCK_ACCESS_NET_CONNECT_TCP)
>>> +				return -EACCES;
>>
>> I'm wondering if this is the right error code for this case. EPERM may
>> be more appropriate.
> 
>     Ok. Will be refactored.
>>
>> Thinking more about this case, I don't understand what is the rationale
>> to deny such action. What would be the consequence to always allow
>> connection with AF_UNSPEC (i.e. to disconnect a socket)?
>>
>     I thought we have come to a conclusion about connect(...AF_UNSPEC..)
>    behaviour in the patchset V3:
> https://lore.kernel.org/linux-security-module/19ad3a01-d76e-0e73-7833-99acd4afd97e@huawei.com/

The conclusion was that AF_UNSPEC disconnects a socket, but I'm asking 
if this is a security issue. I don't think it is more dangerous than a 
new (unconnected) socket. Am I missing something? Which kind of rule 
could be bypassed? What are we protecting against by restricting AF_UNSPEC?

We could then reduce the hook codes to just:
return current_check_access_socket(sock, address, LANDLOCK_ACCESS_NET_*);

Re: [PATCH v8 07/12] landlock: Add network rules support

[Konstantin Meskhidze (A)]

11/28/2022 11:26 PM, Mickaël Salaün пишет:
> 
> On 28/11/2022 05:01, Konstantin Meskhidze (A) wrote:
>> 
>> 
>> 11/17/2022 9:43 PM, Mickaël Salaün пишет:
>>>
>>> On 21/10/2022 17:26, Konstantin Meskhidze wrote:
>>>> This commit adds network rules support in internal landlock functions
>>>> (presented in ruleset.c) and landlock_create_ruleset syscall.
>>>
>>> …in the ruleset management helpers and the landlock_create_ruleset syscall.
>>>
>>>
>>>> Refactors user space API to support network actions. Adds new network
>>>
>>> Refactor…
>>>
>>>> access flags, network rule and network attributes. Increments Landlock
>>>
>>> Increment…
>> 
>>     The commit's message will be fixed. Thank you!
>>>
>>>> ABI version.
>>>>
>>>> Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze@huawei.com>
>>>> ---
>>>>
>>>> Changes since v7:
>>>> * Squashes commits.
>>>> * Increments ABI version to 4.
>>>> * Refactors commit message.
>>>> * Minor fixes.
>>>>
>>>> Changes since v6:
>>>> * Renames landlock_set_net_access_mask() to landlock_add_net_access_mask()
>>>>     because it OR values.
>>>> * Makes landlock_add_net_access_mask() more resilient incorrect values.
>>>> * Refactors landlock_get_net_access_mask().
>>>> * Renames LANDLOCK_MASK_SHIFT_NET to LANDLOCK_SHIFT_ACCESS_NET and use
>>>>     LANDLOCK_NUM_ACCESS_FS as value.
>>>> * Updates access_masks_t to u32 to support network access actions.
>>>> * Refactors landlock internal functions to support network actions with
>>>>     landlock_key/key_type/id types.
>>>>
>>>> Changes since v5:
>>>> * Gets rid of partial revert from landlock_add_rule
>>>> syscall.
>>>> * Formats code with clang-format-14.
>>>>
>>>> Changes since v4:
>>>> * Refactors landlock_create_ruleset() - splits ruleset and
>>>> masks checks.
>>>> * Refactors landlock_create_ruleset() and landlock mask
>>>> setters/getters to support two rule types.
>>>> * Refactors landlock_add_rule syscall add_rule_path_beneath
>>>> function by factoring out get_ruleset_from_fd() and
>>>> landlock_put_ruleset().
>>>>
>>>> Changes since v3:
>>>> * Splits commit.
>>>> * Adds network rule support for internal landlock functions.
>>>> * Adds set_mask and get_mask for network.
>>>> * Adds rb_root root_net_port.
>>>>
>>>> ---
>>>>    include/uapi/linux/landlock.h                | 49 ++++++++++++++
>>>>    security/landlock/limits.h                   |  6 +-
>>>>    security/landlock/ruleset.c                  | 55 ++++++++++++++--
>>>>    security/landlock/ruleset.h                  | 68 ++++++++++++++++----
>>>>    security/landlock/syscalls.c                 | 13 +++-
>>>>    tools/testing/selftests/landlock/base_test.c |  2 +-
>>>>    6 files changed, 170 insertions(+), 23 deletions(-)
>>>>
>>>> diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
>>>> index f3223f964691..096b683c6ff3 100644
>>>> --- a/include/uapi/linux/landlock.h
>>>> +++ b/include/uapi/linux/landlock.h
>>>> @@ -31,6 +31,13 @@ struct landlock_ruleset_attr {
>>>>    	 * this access right.
>>>>    	 */
>>>>    	__u64 handled_access_fs;
>>>> +
>>>> +	/**
>>>> +	 * @handled_access_net: Bitmask of actions (cf. `Network flags`_)
>>>> +	 * that is handled by this ruleset and should then be forbidden if no
>>>> +	 * rule explicitly allow them.
>>>> +	 */
>>>> +	__u64 handled_access_net;
>>>>    };
>>>>
>>>>    /*
>>>> @@ -54,6 +61,11 @@ enum landlock_rule_type {
>>>>    	 * landlock_path_beneath_attr .
>>>>    	 */
>>>>    	LANDLOCK_RULE_PATH_BENEATH = 1,
>>>> +	/**
>>>> +	 * @LANDLOCK_RULE_NET_SERVICE: Type of a &struct
>>>> +	 * landlock_net_service_attr .
>>>> +	 */
>>>> +	LANDLOCK_RULE_NET_SERVICE = 2,
>>>>    };
>>>>
>>>>    /**
>>>> @@ -79,6 +91,24 @@ struct landlock_path_beneath_attr {
>>>>    	 */
>>>>    } __attribute__((packed));
>>>>
>>>> +/**
>>>> + * struct landlock_net_service_attr - TCP subnet definition
>>>> + *
>>>> + * Argument of sys_landlock_add_rule().
>>>> + */
>>>> +struct landlock_net_service_attr {
>>>> +	/**
>>>> +	 * @allowed_access: Bitmask of allowed access network for services
>>>> +	 * (cf. `Network flags`_).
>>>> +	 */
>>>> +	__u64 allowed_access;
>>>> +	/**
>>>> +	 * @port: Network port.
>>>> +	 */
>>>> +	__u16 port;
>>>
>>>    From an UAPI point of view, I think the port field should be __be16, as
>>> for sockaddr_in->port and other network-related APIs. This will require
>>> some kernel changes to please sparse: make C=2 security/landlock/ must
>>> not print any warning.
>> 
>>     Is sparse a default checker?
> 
> You should be able to easily install it with your Linux distro.

  Ok. Thank you.
> .

Re: [PATCH v8 08/12] landlock: Implement TCP network hooks

[Konstantin Meskhidze (A)]

11/29/2022 12:00 AM, Mickaël Salaün пишет:
> The previous commit provides an interface to theoretically restrict
> network access (i.e. ruleset handled network accesses), but in fact this
> is not enforced until this commit. I like this split but to avoid any
> inconsistency, please squash this commit into the previous one: "7/12
> landlock: Add network rules support"
> You should keep all the commit messages but maybe tweak them a bit.
> 
   Ok. Will be squashed.
> 
> On 28/11/2022 09:21, Konstantin Meskhidze (A) wrote:
>> 
>> 
>> 11/17/2022 9:43 PM, Mickaël Salaün пишет:
>>>
>>> On 21/10/2022 17:26, Konstantin Meskhidze wrote:
>>>> This patch adds support of socket_bind() and socket_connect() hooks.
>>>> It's possible to restrict binding and connecting of TCP sockets to
>>>> particular ports.
>>>
>>> Implement socket_bind() and socket_connect LSM hooks, which enable to
>>> restrict TCP socket binding and connection to specific ports.
>>>
>>     Ok. Thanks.
>>>
>>>>
>>>> Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze@huawei.com>
>>>> ---
> 
> [...]
> 
>>>> +static int hook_socket_connect(struct socket *sock, struct sockaddr *address,
>>>> +			       int addrlen)
>>>> +{
>>>> +	const struct landlock_ruleset *const dom =
>>>> +		landlock_get_current_domain();
>>>> +
>>>> +	if (!dom)
>>>> +		return 0;
>>>> +
>>>> +	/* Check if it's a TCP socket. */
>>>> +	if (sock->type != SOCK_STREAM)
>>>> +		return 0;
>>>> +
>>>> +	/* Check if the hook is AF_INET* socket's action. */
>>>> +	switch (address->sa_family) {
>>>> +	case AF_INET:
>>>> +#if IS_ENABLED(CONFIG_IPV6)
>>>> +	case AF_INET6:
>>>> +#endif
>>>> +		return check_socket_access(dom, get_port(address),
>>>> +					   LANDLOCK_ACCESS_NET_CONNECT_TCP);
>>>> +	case AF_UNSPEC: {
>>>> +		u16 i;
>>>
>>> You can move "i" after the "dom" declaration to remove the extra braces.
>>>
>>     Ok. Thanks.
>>>
>>>> +
>>>> +		/*
>>>> +		 * If just in a layer a mask supports connect access,
>>>> +		 * the socket_connect() hook with AF_UNSPEC family flag
>>>> +		 * must be banned. This prevents from disconnecting already
>>>> +		 * connected sockets.
>>>> +		 */
>>>> +		for (i = 0; i < dom->num_layers; i++) {
>>>> +			if (landlock_get_net_access_mask(dom, i) &
>>>> +			    LANDLOCK_ACCESS_NET_CONNECT_TCP)
>>>> +				return -EACCES;
>>>
>>> I'm wondering if this is the right error code for this case. EPERM may
>>> be more appropriate.
>> 
>>     Ok. Will be refactored.
>>>
>>> Thinking more about this case, I don't understand what is the rationale
>>> to deny such action. What would be the consequence to always allow
>>> connection with AF_UNSPEC (i.e. to disconnect a socket)?
>>>
>>     I thought we have come to a conclusion about connect(...AF_UNSPEC..)
>>    behaviour in the patchset V3:
>> https://lore.kernel.org/linux-security-module/19ad3a01-d76e-0e73-7833-99acd4afd97e@huawei.com/
> 
> The conclusion was that AF_UNSPEC disconnects a socket, but I'm asking
> if this is a security issue. I don't think it is more dangerous than a
> new (unconnected) socket. Am I missing something? Which kind of rule
> could be bypassed? What are we protecting against by restricting AF_UNSPEC?

I just follow Willem de Bruijn concerns about this issue:

quote: "It is valid to pass an address with AF_UNSPEC to a PF_INET(6) 
socket. And there are legitimate reasons to want to deny this. Such as 
passing a connection to a unprivileged process and disallow it from 
disconnect and opening a different new connection."

https://lore.kernel.org/linux-security-module/CA+FuTSf4EjgjBCCOiu-PHJcTMia41UkTh8QJ0+qdxL_J8445EA@mail.gmail.com/


quote: "The intended use-case is for a privileged process to open a 
connection (i.e., bound and connected socket) and pass that to a 
restricted process. The intent is for that process to only be allowed to
communicate over this pre-established channel.

In practice, it is able to disconnect (while staying bound) and
elevate its privileges to that of a listening server: ..."

https://lore.kernel.org/linux-security-module/CA+FuTScaoby-=xRKf_Dz3koSYHqrMN0cauCg4jMmy_nDxwPADA@mail.gmail.com/

Looks like it's a security issue here.

> 
> We could then reduce the hook codes to just:
> return current_check_access_socket(sock, address, LANDLOCK_ACCESS_NET_*);
> .

Re: [PATCH v8 08/12] landlock: Implement TCP network hooks

[Mickaël Salaün]

On 02/12/2022 04:13, Konstantin Meskhidze (A) wrote:
> 
> 
> 11/29/2022 12:00 AM, Mickaël Salaün пишет:
>> The previous commit provides an interface to theoretically restrict
>> network access (i.e. ruleset handled network accesses), but in fact this
>> is not enforced until this commit. I like this split but to avoid any
>> inconsistency, please squash this commit into the previous one: "7/12
>> landlock: Add network rules support"
>> You should keep all the commit messages but maybe tweak them a bit.
>>
>     Ok. Will be squashed.
>>
>> On 28/11/2022 09:21, Konstantin Meskhidze (A) wrote:
>>>
>>>
>>> 11/17/2022 9:43 PM, Mickaël Salaün пишет:
>>>>
>>>> On 21/10/2022 17:26, Konstantin Meskhidze wrote:
>>>>> This patch adds support of socket_bind() and socket_connect() hooks.
>>>>> It's possible to restrict binding and connecting of TCP sockets to
>>>>> particular ports.
>>>>
>>>> Implement socket_bind() and socket_connect LSM hooks, which enable to
>>>> restrict TCP socket binding and connection to specific ports.
>>>>
>>>      Ok. Thanks.
>>>>
>>>>>
>>>>> Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze@huawei.com>
>>>>> ---
>>
>> [...]
>>
>>>>> +static int hook_socket_connect(struct socket *sock, struct sockaddr *address,
>>>>> +			       int addrlen)
>>>>> +{
>>>>> +	const struct landlock_ruleset *const dom =
>>>>> +		landlock_get_current_domain();
>>>>> +
>>>>> +	if (!dom)
>>>>> +		return 0;
>>>>> +
>>>>> +	/* Check if it's a TCP socket. */
>>>>> +	if (sock->type != SOCK_STREAM)
>>>>> +		return 0;
>>>>> +
>>>>> +	/* Check if the hook is AF_INET* socket's action. */
>>>>> +	switch (address->sa_family) {
>>>>> +	case AF_INET:
>>>>> +#if IS_ENABLED(CONFIG_IPV6)
>>>>> +	case AF_INET6:
>>>>> +#endif
>>>>> +		return check_socket_access(dom, get_port(address),
>>>>> +					   LANDLOCK_ACCESS_NET_CONNECT_TCP);
>>>>> +	case AF_UNSPEC: {
>>>>> +		u16 i;
>>>>
>>>> You can move "i" after the "dom" declaration to remove the extra braces.
>>>>
>>>      Ok. Thanks.
>>>>
>>>>> +
>>>>> +		/*
>>>>> +		 * If just in a layer a mask supports connect access,
>>>>> +		 * the socket_connect() hook with AF_UNSPEC family flag
>>>>> +		 * must be banned. This prevents from disconnecting already
>>>>> +		 * connected sockets.
>>>>> +		 */
>>>>> +		for (i = 0; i < dom->num_layers; i++) {
>>>>> +			if (landlock_get_net_access_mask(dom, i) &
>>>>> +			    LANDLOCK_ACCESS_NET_CONNECT_TCP)
>>>>> +				return -EACCES;
>>>>
>>>> I'm wondering if this is the right error code for this case. EPERM may
>>>> be more appropriate.
>>>
>>>      Ok. Will be refactored.
>>>>
>>>> Thinking more about this case, I don't understand what is the rationale
>>>> to deny such action. What would be the consequence to always allow
>>>> connection with AF_UNSPEC (i.e. to disconnect a socket)?
>>>>
>>>      I thought we have come to a conclusion about connect(...AF_UNSPEC..)
>>>     behaviour in the patchset V3:
>>> https://lore.kernel.org/linux-security-module/19ad3a01-d76e-0e73-7833-99acd4afd97e@huawei.com/
>>
>> The conclusion was that AF_UNSPEC disconnects a socket, but I'm asking
>> if this is a security issue. I don't think it is more dangerous than a
>> new (unconnected) socket. Am I missing something? Which kind of rule
>> could be bypassed? What are we protecting against by restricting AF_UNSPEC?
> 
> I just follow Willem de Bruijn concerns about this issue:
> 
> quote: "It is valid to pass an address with AF_UNSPEC to a PF_INET(6)
> socket. And there are legitimate reasons to want to deny this. Such as
> passing a connection to a unprivileged process and disallow it from
> disconnect and opening a different new connection."
> 
> https://lore.kernel.org/linux-security-module/CA+FuTSf4EjgjBCCOiu-PHJcTMia41UkTh8QJ0+qdxL_J8445EA@mail.gmail.com/

I agree with the fact that we want to deny this, but in this example the 
new connection should still be restricted by the Landlock domain. Using 
AF_UNSPEC on a connected socket should not make this socket allowed to 
create any connection if the process is restricted with TCP_CONNECT. 
Being allowed to close a connection should not be an issue, and any new 
connection must be vetted by Landlock.

> 
> 
> quote: "The intended use-case is for a privileged process to open a
> connection (i.e., bound and connected socket) and pass that to a
> restricted process. The intent is for that process to only be allowed to
> communicate over this pre-established channel.
> 
> In practice, it is able to disconnect (while staying bound) and
> elevate its privileges to that of a listening server: ..."
> 
> https://lore.kernel.org/linux-security-module/CA+FuTScaoby-=xRKf_Dz3koSYHqrMN0cauCg4jMmy_nDxwPADA@mail.gmail.com/
> 
> Looks like it's a security issue here.

It the provided example, if child_process() is restricted with 
TCP_CONNECT and TCP_BIND, any call to connect() or bind() will return an 
access error. listen() and accept() would work if the socket is bound, 
which is the case here, and then implicitly allowed by the parent 
process. I don' see any security issue. Am I missing something?

In fact, connect with AF_UNSPEC should always be allowed to be 
consistent with close(2), which is a way to drop privileges.


What Willem said:
> It would be good to also
> ensure that a now-bound socket cannot call listen.

This is not relevant for Landlock because the security model is to check 
process's requests to get new accesses (e.g. create a new file 
descriptor), but not to check passed accesses (e.g. inherited from a 
parent process, or pass through a unix socket) which are delegated to 
the sender/parent. The goal of a sandbox is to limit the set of new 
access requested (to the kernel) from within this sandbox. All already 
opened file descriptors were previously vetted by Landlock (and other 
access control systems).

> 
>>
>> We could then reduce the hook codes to just:
>> return current_check_access_socket(sock, address, LANDLOCK_ACCESS_NET_*);
>> .

As for SELinux, the connect hook should first do this check (with an 
appropriate comment):
if (address->sa_family == AF_UNSPEC)
	return 0;

Re: [PATCH v8 08/12] landlock: Implement TCP network hooks

[Konstantin Meskhidze (A)]

12/2/2022 4:01 PM, Mickaël Salaün пишет:
> 
> On 02/12/2022 04:13, Konstantin Meskhidze (A) wrote:
>> 
>> 
>> 11/29/2022 12:00 AM, Mickaël Salaün пишет:
>>> The previous commit provides an interface to theoretically restrict
>>> network access (i.e. ruleset handled network accesses), but in fact this
>>> is not enforced until this commit. I like this split but to avoid any
>>> inconsistency, please squash this commit into the previous one: "7/12
>>> landlock: Add network rules support"
>>> You should keep all the commit messages but maybe tweak them a bit.
>>>
>>     Ok. Will be squashed.
>>>
>>> On 28/11/2022 09:21, Konstantin Meskhidze (A) wrote:
>>>>
>>>>
>>>> 11/17/2022 9:43 PM, Mickaël Salaün пишет:
>>>>>
>>>>> On 21/10/2022 17:26, Konstantin Meskhidze wrote:
>>>>>> This patch adds support of socket_bind() and socket_connect() hooks.
>>>>>> It's possible to restrict binding and connecting of TCP sockets to
>>>>>> particular ports.
>>>>>
>>>>> Implement socket_bind() and socket_connect LSM hooks, which enable to
>>>>> restrict TCP socket binding and connection to specific ports.
>>>>>
>>>>      Ok. Thanks.
>>>>>
>>>>>>
>>>>>> Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze@huawei.com>
>>>>>> ---
>>>
>>> [...]
>>>
>>>>>> +static int hook_socket_connect(struct socket *sock, struct sockaddr *address,
>>>>>> +			       int addrlen)
>>>>>> +{
>>>>>> +	const struct landlock_ruleset *const dom =
>>>>>> +		landlock_get_current_domain();
>>>>>> +
>>>>>> +	if (!dom)
>>>>>> +		return 0;
>>>>>> +
>>>>>> +	/* Check if it's a TCP socket. */
>>>>>> +	if (sock->type != SOCK_STREAM)
>>>>>> +		return 0;
>>>>>> +
>>>>>> +	/* Check if the hook is AF_INET* socket's action. */
>>>>>> +	switch (address->sa_family) {
>>>>>> +	case AF_INET:
>>>>>> +#if IS_ENABLED(CONFIG_IPV6)
>>>>>> +	case AF_INET6:
>>>>>> +#endif
>>>>>> +		return check_socket_access(dom, get_port(address),
>>>>>> +					   LANDLOCK_ACCESS_NET_CONNECT_TCP);
>>>>>> +	case AF_UNSPEC: {
>>>>>> +		u16 i;
>>>>>
>>>>> You can move "i" after the "dom" declaration to remove the extra braces.
>>>>>
>>>>      Ok. Thanks.
>>>>>
>>>>>> +
>>>>>> +		/*
>>>>>> +		 * If just in a layer a mask supports connect access,
>>>>>> +		 * the socket_connect() hook with AF_UNSPEC family flag
>>>>>> +		 * must be banned. This prevents from disconnecting already
>>>>>> +		 * connected sockets.
>>>>>> +		 */
>>>>>> +		for (i = 0; i < dom->num_layers; i++) {
>>>>>> +			if (landlock_get_net_access_mask(dom, i) &
>>>>>> +			    LANDLOCK_ACCESS_NET_CONNECT_TCP)
>>>>>> +				return -EACCES;
>>>>>
>>>>> I'm wondering if this is the right error code for this case. EPERM may
>>>>> be more appropriate.
>>>>
>>>>      Ok. Will be refactored.
>>>>>
>>>>> Thinking more about this case, I don't understand what is the rationale
>>>>> to deny such action. What would be the consequence to always allow
>>>>> connection with AF_UNSPEC (i.e. to disconnect a socket)?
>>>>>
>>>>      I thought we have come to a conclusion about connect(...AF_UNSPEC..)
>>>>     behaviour in the patchset V3:
>>>> https://lore.kernel.org/linux-security-module/19ad3a01-d76e-0e73-7833-99acd4afd97e@huawei.com/
>>>
>>> The conclusion was that AF_UNSPEC disconnects a socket, but I'm asking
>>> if this is a security issue. I don't think it is more dangerous than a
>>> new (unconnected) socket. Am I missing something? Which kind of rule
>>> could be bypassed? What are we protecting against by restricting AF_UNSPEC?
>> 
>> I just follow Willem de Bruijn concerns about this issue:
>> 
>> quote: "It is valid to pass an address with AF_UNSPEC to a PF_INET(6)
>> socket. And there are legitimate reasons to want to deny this. Such as
>> passing a connection to a unprivileged process and disallow it from
>> disconnect and opening a different new connection."
>> 
>> https://lore.kernel.org/linux-security-module/CA+FuTSf4EjgjBCCOiu-PHJcTMia41UkTh8QJ0+qdxL_J8445EA@mail.gmail.com/
> 
> I agree with the fact that we want to deny this, but in this example the
> new connection should still be restricted by the Landlock domain. Using
> AF_UNSPEC on a connected socket should not make this socket allowed to
> create any connection if the process is restricted with TCP_CONNECT.
> Being allowed to close a connection should not be an issue, and any new
> connection must be vetted by Landlock.
> 

   You are right. This makes sense. Thanks for the comment.
>> 
>> 
>> quote: "The intended use-case is for a privileged process to open a
>> connection (i.e., bound and connected socket) and pass that to a
>> restricted process. The intent is for that process to only be allowed to
>> communicate over this pre-established channel.
>> 
>> In practice, it is able to disconnect (while staying bound) and
>> elevate its privileges to that of a listening server: ..."
>> 
>> https://lore.kernel.org/linux-security-module/CA+FuTScaoby-=xRKf_Dz3koSYHqrMN0cauCg4jMmy_nDxwPADA@mail.gmail.com/
>> 
>> Looks like it's a security issue here.
> 
> It the provided example, if child_process() is restricted with
> TCP_CONNECT and TCP_BIND, any call to connect() or bind() will return an
> access error. listen() and accept() would work if the socket is bound,
> which is the case here, and then implicitly allowed by the parent
> process. I don' see any security issue. Am I missing something?
> 
> In fact, connect with AF_UNSPEC should always be allowed to be
> consistent with close(2), which is a way to drop privileges.
> 

  It should be allowed with checking:
"return check_socket_access(dom, get_port(address),
                                  LANDLOCK_ACCESS_NET_CONNECT_TCP);
> 
> What Willem said:
>> It would be good to also
>> ensure that a now-bound socket cannot call listen.
> 
> This is not relevant for Landlock because the security model is to check
> process's requests to get new accesses (e.g. create a new file
> descriptor), but not to check passed accesses (e.g. inherited from a
> parent process, or pass through a unix socket) which are delegated to
> the sender/parent. The goal of a sandbox is to limit the set of new
> access requested (to the kernel) from within this sandbox. All already
> opened file descriptors were previously vetted by Landlock (and other
> access control systems).

    I got your point. Thanks.
> 
>> 
>>>
>>> We could then reduce the hook codes to just:
>>> return current_check_access_socket(sock, address, LANDLOCK_ACCESS_NET_*);
>>> .
> 
> As for SELinux, the connect hook should first do this check (with an
> appropriate comment):
> if (address->sa_family == AF_UNSPEC)
> 	return 0;

   In case of Landlock it looks like a landlocked process could connnect 
to the ports it's not allowed to connect to.
So we need just to return check_socket_access(dom, get_port(address),
				   LANDLOCK_ACCESS_NET_CONNECT_TCP);
I'm I correct? Did I miss something?
> .

Re: [PATCH v8 08/12] landlock: Implement TCP network hooks

[Mickaël Salaün]

On 05/12/2022 03:55, Konstantin Meskhidze (A) wrote:
> 
> 
> 12/2/2022 4:01 PM, Mickaël Salaün пишет:
>>
>> On 02/12/2022 04:13, Konstantin Meskhidze (A) wrote:
>>>
>>>
>>> 11/29/2022 12:00 AM, Mickaël Salaün пишет:
>>>> The previous commit provides an interface to theoretically restrict
>>>> network access (i.e. ruleset handled network accesses), but in fact this
>>>> is not enforced until this commit. I like this split but to avoid any
>>>> inconsistency, please squash this commit into the previous one: "7/12
>>>> landlock: Add network rules support"
>>>> You should keep all the commit messages but maybe tweak them a bit.
>>>>
>>>      Ok. Will be squashed.
>>>>
>>>> On 28/11/2022 09:21, Konstantin Meskhidze (A) wrote:
>>>>>
>>>>>
>>>>> 11/17/2022 9:43 PM, Mickaël Salaün пишет:
>>>>>>
>>>>>> On 21/10/2022 17:26, Konstantin Meskhidze wrote:
>>>>>>> This patch adds support of socket_bind() and socket_connect() hooks.
>>>>>>> It's possible to restrict binding and connecting of TCP sockets to
>>>>>>> particular ports.
>>>>>>
>>>>>> Implement socket_bind() and socket_connect LSM hooks, which enable to
>>>>>> restrict TCP socket binding and connection to specific ports.
>>>>>>
>>>>>       Ok. Thanks.
>>>>>>
>>>>>>>
>>>>>>> Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze@huawei.com>
>>>>>>> ---
>>>>
>>>> [...]
>>>>
>>>>>>> +static int hook_socket_connect(struct socket *sock, struct sockaddr *address,
>>>>>>> +			       int addrlen)
>>>>>>> +{
>>>>>>> +	const struct landlock_ruleset *const dom =
>>>>>>> +		landlock_get_current_domain();
>>>>>>> +
>>>>>>> +	if (!dom)
>>>>>>> +		return 0;
>>>>>>> +
>>>>>>> +	/* Check if it's a TCP socket. */
>>>>>>> +	if (sock->type != SOCK_STREAM)
>>>>>>> +		return 0;
>>>>>>> +
>>>>>>> +	/* Check if the hook is AF_INET* socket's action. */
>>>>>>> +	switch (address->sa_family) {
>>>>>>> +	case AF_INET:
>>>>>>> +#if IS_ENABLED(CONFIG_IPV6)
>>>>>>> +	case AF_INET6:
>>>>>>> +#endif
>>>>>>> +		return check_socket_access(dom, get_port(address),
>>>>>>> +					   LANDLOCK_ACCESS_NET_CONNECT_TCP);
>>>>>>> +	case AF_UNSPEC: {
>>>>>>> +		u16 i;
>>>>>>
>>>>>> You can move "i" after the "dom" declaration to remove the extra braces.
>>>>>>
>>>>>       Ok. Thanks.
>>>>>>
>>>>>>> +
>>>>>>> +		/*
>>>>>>> +		 * If just in a layer a mask supports connect access,
>>>>>>> +		 * the socket_connect() hook with AF_UNSPEC family flag
>>>>>>> +		 * must be banned. This prevents from disconnecting already
>>>>>>> +		 * connected sockets.
>>>>>>> +		 */
>>>>>>> +		for (i = 0; i < dom->num_layers; i++) {
>>>>>>> +			if (landlock_get_net_access_mask(dom, i) &
>>>>>>> +			    LANDLOCK_ACCESS_NET_CONNECT_TCP)
>>>>>>> +				return -EACCES;
>>>>>>
>>>>>> I'm wondering if this is the right error code for this case. EPERM may
>>>>>> be more appropriate.
>>>>>
>>>>>       Ok. Will be refactored.
>>>>>>
>>>>>> Thinking more about this case, I don't understand what is the rationale
>>>>>> to deny such action. What would be the consequence to always allow
>>>>>> connection with AF_UNSPEC (i.e. to disconnect a socket)?
>>>>>>
>>>>>       I thought we have come to a conclusion about connect(...AF_UNSPEC..)
>>>>>      behaviour in the patchset V3:
>>>>> https://lore.kernel.org/linux-security-module/19ad3a01-d76e-0e73-7833-99acd4afd97e@huawei.com/
>>>>
>>>> The conclusion was that AF_UNSPEC disconnects a socket, but I'm asking
>>>> if this is a security issue. I don't think it is more dangerous than a
>>>> new (unconnected) socket. Am I missing something? Which kind of rule
>>>> could be bypassed? What are we protecting against by restricting AF_UNSPEC?
>>>
>>> I just follow Willem de Bruijn concerns about this issue:
>>>
>>> quote: "It is valid to pass an address with AF_UNSPEC to a PF_INET(6)
>>> socket. And there are legitimate reasons to want to deny this. Such as
>>> passing a connection to a unprivileged process and disallow it from
>>> disconnect and opening a different new connection."
>>>
>>> https://lore.kernel.org/linux-security-module/CA+FuTSf4EjgjBCCOiu-PHJcTMia41UkTh8QJ0+qdxL_J8445EA@mail.gmail.com/
>>
>> I agree with the fact that we want to deny this, but in this example the
>> new connection should still be restricted by the Landlock domain. Using
>> AF_UNSPEC on a connected socket should not make this socket allowed to
>> create any connection if the process is restricted with TCP_CONNECT.
>> Being allowed to close a connection should not be an issue, and any new
>> connection must be vetted by Landlock.
>>
> 
>     You are right. This makes sense. Thanks for the comment.
>>>
>>>
>>> quote: "The intended use-case is for a privileged process to open a
>>> connection (i.e., bound and connected socket) and pass that to a
>>> restricted process. The intent is for that process to only be allowed to
>>> communicate over this pre-established channel.
>>>
>>> In practice, it is able to disconnect (while staying bound) and
>>> elevate its privileges to that of a listening server: ..."
>>>
>>> https://lore.kernel.org/linux-security-module/CA+FuTScaoby-=xRKf_Dz3koSYHqrMN0cauCg4jMmy_nDxwPADA@mail.gmail.com/
>>>
>>> Looks like it's a security issue here.
>>
>> It the provided example, if child_process() is restricted with
>> TCP_CONNECT and TCP_BIND, any call to connect() or bind() will return an
>> access error. listen() and accept() would work if the socket is bound,
>> which is the case here, and then implicitly allowed by the parent
>> process. I don' see any security issue. Am I missing something?
>>
>> In fact, connect with AF_UNSPEC should always be allowed to be
>> consistent with close(2), which is a way to drop privileges.
>>
> 
>    It should be allowed with checking:
> "return check_socket_access(dom, get_port(address),
>                                    LANDLOCK_ACCESS_NET_CONNECT_TCP);
>>
>> What Willem said:
>>> It would be good to also
>>> ensure that a now-bound socket cannot call listen.
>>
>> This is not relevant for Landlock because the security model is to check
>> process's requests to get new accesses (e.g. create a new file
>> descriptor), but not to check passed accesses (e.g. inherited from a
>> parent process, or pass through a unix socket) which are delegated to
>> the sender/parent. The goal of a sandbox is to limit the set of new
>> access requested (to the kernel) from within this sandbox. All already
>> opened file descriptors were previously vetted by Landlock (and other
>> access control systems).
> 
>      I got your point. Thanks.
>>
>>>
>>>>
>>>> We could then reduce the hook codes to just:
>>>> return current_check_access_socket(sock, address, LANDLOCK_ACCESS_NET_*);

This current_check_access_socket() helper should contain all the access 
control code.

>>>> .
>>
>> As for SELinux, the connect hook should first do this check (with an
>> appropriate comment):
>> if (address->sa_family == AF_UNSPEC)
>> 	return 0;
> 
>     In case of Landlock it looks like a landlocked process could connnect
> to the ports it's not allowed to connect to.
> So we need just to return check_socket_access(dom, get_port(address),
> 				   LANDLOCK_ACCESS_NET_CONNECT_TCP);
> I'm I correct? Did I miss something?

Using AF_UNSPEC with connect(2) doesn't connect the socket to a port, 
and in fact completely ignore the port. We can move the AF_UNSPEC check 
to the current_check_access_socket() helper:

  	switch (address->sa_family) {
  	case AF_UNSPEC:
+		/*
+		 * Connecting to an address with AF_UNSPEC dissolves the TCP
+		 * association, which have the same effect as closing the
+		 * connection while retaining the socket object (i.e., the file
+		 * descriptor).  As for dropping privileges, closing
+		 * connections is always allowed.
+		 */
+		if (access_request == LANDLOCK_ACCESS_NET_CONNECT_TCP)
+			return 0;
+
+		/*
+		 * For compatibility reason, accept AF_UNSPEC for bind
+		 * accesses (mapped to AF_INET) only if the address is
+		 * INADDR_ANY (cf. __inet_bind).  Checking the address is
+		 * required to not wrongfully return -EACCES instead of
+		 * -EAFNOSUPPORT.
+		 */
+		if (access_request == LANDLOCK_ACCESS_NET_BIND_TCP) {
+			const struct sockaddr_in *const sockaddr =
+				(struct sockaddr_in *)address;
+
+			if (sockaddr->sin_addr.s_addr != htonl(INADDR_ANY))
+				return -EAFNOSUPPORT;
+		}
+
+		fallthrough;
  	case AF_INET:
  #if IS_ENABLED(CONFIG_IPV6)
  	case AF_INET6:


I also added another check (copied from SELinux) with the appropriate 
explanation. All this needs dedicated tests to make sure everything is 
covered.

We also need to add extra checks (and related tests) for addrlen as do 
other LSMs.

Re: [PATCH v8 07/12] landlock: Add network rules support

[Konstantin Meskhidze (A)]

11/17/2022 9:43 PM, Mickaël Salaün пишет:
> 
> On 21/10/2022 17:26, Konstantin Meskhidze wrote:
>> This commit adds network rules support in internal landlock functions
>> (presented in ruleset.c) and landlock_create_ruleset syscall.
> 
> …in the ruleset management helpers and the landlock_create_ruleset syscall.
> 
> 
>> Refactors user space API to support network actions. Adds new network
> 
> Refactor…
> 
>> access flags, network rule and network attributes. Increments Landlock
> 
> Increment…
> 
>> ABI version.
>> 
>> Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze@huawei.com>
>> ---
>> 
>> Changes since v7:
>> * Squashes commits.
>> * Increments ABI version to 4.
>> * Refactors commit message.
>> * Minor fixes.
>> 
>> Changes since v6:
>> * Renames landlock_set_net_access_mask() to landlock_add_net_access_mask()
>>    because it OR values.
>> * Makes landlock_add_net_access_mask() more resilient incorrect values.
>> * Refactors landlock_get_net_access_mask().
>> * Renames LANDLOCK_MASK_SHIFT_NET to LANDLOCK_SHIFT_ACCESS_NET and use
>>    LANDLOCK_NUM_ACCESS_FS as value.
>> * Updates access_masks_t to u32 to support network access actions.
>> * Refactors landlock internal functions to support network actions with
>>    landlock_key/key_type/id types.
>> 
>> Changes since v5:
>> * Gets rid of partial revert from landlock_add_rule
>> syscall.
>> * Formats code with clang-format-14.
>> 
>> Changes since v4:
>> * Refactors landlock_create_ruleset() - splits ruleset and
>> masks checks.
>> * Refactors landlock_create_ruleset() and landlock mask
>> setters/getters to support two rule types.
>> * Refactors landlock_add_rule syscall add_rule_path_beneath
>> function by factoring out get_ruleset_from_fd() and
>> landlock_put_ruleset().
>> 
>> Changes since v3:
>> * Splits commit.
>> * Adds network rule support for internal landlock functions.
>> * Adds set_mask and get_mask for network.
>> * Adds rb_root root_net_port.
>> 
>> ---
>>   include/uapi/linux/landlock.h                | 49 ++++++++++++++
>>   security/landlock/limits.h                   |  6 +-
>>   security/landlock/ruleset.c                  | 55 ++++++++++++++--
>>   security/landlock/ruleset.h                  | 68 ++++++++++++++++----
>>   security/landlock/syscalls.c                 | 13 +++-
>>   tools/testing/selftests/landlock/base_test.c |  2 +-
>>   6 files changed, 170 insertions(+), 23 deletions(-)
>> 
>> diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
>> index f3223f964691..096b683c6ff3 100644
>> --- a/include/uapi/linux/landlock.h
>> +++ b/include/uapi/linux/landlock.h
>> @@ -31,6 +31,13 @@ struct landlock_ruleset_attr {
>>   	 * this access right.
>>   	 */
>>   	__u64 handled_access_fs;
>> +
>> +	/**
>> +	 * @handled_access_net: Bitmask of actions (cf. `Network flags`_)
>> +	 * that is handled by this ruleset and should then be forbidden if no
>> +	 * rule explicitly allow them.
>> +	 */
>> +	__u64 handled_access_net;
>>   };
>> 
>>   /*
>> @@ -54,6 +61,11 @@ enum landlock_rule_type {
>>   	 * landlock_path_beneath_attr .
>>   	 */
>>   	LANDLOCK_RULE_PATH_BENEATH = 1,
>> +	/**
>> +	 * @LANDLOCK_RULE_NET_SERVICE: Type of a &struct
>> +	 * landlock_net_service_attr .
>> +	 */
>> +	LANDLOCK_RULE_NET_SERVICE = 2,
>>   };
>> 
>>   /**
>> @@ -79,6 +91,24 @@ struct landlock_path_beneath_attr {
>>   	 */
>>   } __attribute__((packed));
>> 
>> +/**
>> + * struct landlock_net_service_attr - TCP subnet definition
>> + *
>> + * Argument of sys_landlock_add_rule().
>> + */
>> +struct landlock_net_service_attr {
>> +	/**
>> +	 * @allowed_access: Bitmask of allowed access network for services
>> +	 * (cf. `Network flags`_).
>> +	 */
>> +	__u64 allowed_access;
>> +	/**
>> +	 * @port: Network port.
>> +	 */
>> +	__u16 port;
> 
>   From an UAPI point of view, I think the port field should be __be16, as
> for sockaddr_in->port and other network-related APIs. This will require
> some kernel changes to please sparse: make C=2 security/landlock/ must
> not print any warning.
> 
> Using big-endian values as keys (casted to uintptr_t, not strictly
> __be16) in the rb-tree should not be an issue because there is no port
> range ordering (for now).
> 
> A dedicated test should check that endianness is correct, e.g. by using
> different port encoding. This should include passing and failing tests,
> but they should work on all architectures (i.e. big or little endian).
> 
   Hi Mickaёl.
   Could you please give me a piece of advice about these kind of tests?
   I have not entirely understood this point.
> 
>> +
>> +} __attribute__((packed));
>> +
>>   /**
>>    * DOC: fs_access
>>    *
>> @@ -173,4 +203,23 @@ struct landlock_path_beneath_attr {
>>   #define LANDLOCK_ACCESS_FS_TRUNCATE			(1ULL << 14)
>>   /* clang-format on */
>> 
>> +/**
>> + * DOC: net_access
>> + *
>> + * Network flags
>> + * ~~~~~~~~~~~~~~~~
>> + *
>> + * These flags enable to restrict a sandboxed process to a set of network
>> + * actions.
>> + *
>> + * TCP sockets with allowed actions:
>> + *
>> + * - %LANDLOCK_ACCESS_NET_BIND_TCP: Bind a TCP socket to a local port.
>> + * - %LANDLOCK_ACCESS_NET_CONNECT_TCP: Connect an active TCP socket to
>> + *   a remote port.
>> + */
>> +/* clang-format off */
>> +#define LANDLOCK_ACCESS_NET_BIND_TCP			(1ULL << 0)
>> +#define LANDLOCK_ACCESS_NET_CONNECT_TCP			(1ULL << 1)
>> +/* clang-format on */
>>   #endif /* _UAPI_LINUX_LANDLOCK_H */
>> diff --git a/security/landlock/limits.h b/security/landlock/limits.h
>> index bafb3b8dc677..8a1a6463c64e 100644
>> --- a/security/landlock/limits.h
>> +++ b/security/landlock/limits.h
>> @@ -23,6 +23,10 @@
>>   #define LANDLOCK_NUM_ACCESS_FS		__const_hweight64(LANDLOCK_MASK_ACCESS_FS)
>>   #define LANDLOCK_SHIFT_ACCESS_FS	0
>> 
>> -/* clang-format on */
>> +#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 */
>>   #endif /* _SECURITY_LANDLOCK_LIMITS_H */
>> diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
>> index c7cf54ba4f6d..9277c1295114 100644
>> --- a/security/landlock/ruleset.c
>> +++ b/security/landlock/ruleset.c
>> @@ -36,6 +36,9 @@ static struct landlock_ruleset *create_ruleset(const u32 num_layers)
>>   	refcount_set(&new_ruleset->usage, 1);
>>   	mutex_init(&new_ruleset->lock);
>>   	new_ruleset->root_inode = RB_ROOT;
>> +#if IS_ENABLED(CONFIG_INET)
>> +	new_ruleset->root_net_port = RB_ROOT;
>> +#endif /* IS_ENABLED(CONFIG_INET) */
>>   	new_ruleset->num_layers = num_layers;
>>   	/*
>>   	 * hierarchy = NULL
>> @@ -46,16 +49,21 @@ static struct landlock_ruleset *create_ruleset(const u32 num_layers)
>>   }
>> 
>>   struct landlock_ruleset *
>> -landlock_create_ruleset(const access_mask_t fs_access_mask)
>> +landlock_create_ruleset(const access_mask_t fs_access_mask,
>> +			const access_mask_t net_access_mask)
>>   {
>>   	struct landlock_ruleset *new_ruleset;
>> 
>>   	/* Informs about useless ruleset. */
>> -	if (!fs_access_mask)
>> +	if (!fs_access_mask && !net_access_mask)
>>   		return ERR_PTR(-ENOMSG);
>>   	new_ruleset = create_ruleset(1);
>> -	if (!IS_ERR(new_ruleset))
>> +	if (IS_ERR(new_ruleset))
>> +		return new_ruleset;
>> +	if (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);
>>   	return new_ruleset;
>>   }
>> 
>> @@ -73,6 +81,10 @@ static inline bool is_object_pointer(const enum landlock_key_type key_type)
>>   	switch (key_type) {
>>   	case LANDLOCK_KEY_INODE:
>>   		return true;
>> +#if IS_ENABLED(CONFIG_INET)
>> +	case LANDLOCK_KEY_NET_PORT:
>> +		return false;
>> +#endif /* IS_ENABLED(CONFIG_INET) */
>>   	}
>>   	WARN_ON_ONCE(1);
>>   	return false;
>> @@ -126,6 +138,11 @@ static inline struct rb_root *get_root(struct landlock_ruleset *const ruleset,
>>   	case LANDLOCK_KEY_INODE:
>>   		root = &ruleset->root_inode;
>>   		break;
>> +#if IS_ENABLED(CONFIG_INET)
>> +	case LANDLOCK_KEY_NET_PORT:
>> +		root = &ruleset->root_net_port;
>> +		break;
>> +#endif /* IS_ENABLED(CONFIG_INET) */
>>   	}
>>   	if (WARN_ON_ONCE(!root))
>>   		return ERR_PTR(-EINVAL);
>> @@ -154,7 +171,9 @@ static void build_check_ruleset(void)
>>   	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_FS << LANDLOCK_SHIFT_ACCESS_FS) +
> 
> This is correct but because we are dealing with bitmasks I would prefer
> to use "|" instead of "+".
> 
> 
>> +			     (LANDLOCK_MASK_ACCESS_NET
>> +			      << LANDLOCK_SHIFT_ACCESS_NET));
>>   }
>> 
>>   /**
>> @@ -370,7 +389,12 @@ static int merge_ruleset(struct landlock_ruleset *const dst,
>>   	err = merge_tree(dst, src, LANDLOCK_KEY_INODE);
>>   	if (err)
>>   		goto out_unlock;
>> -
> 
> Please keep this newline.
> 
>> +#if IS_ENABLED(CONFIG_INET)
>> +	/* Merges the @src network port tree. */
>> +	err = merge_tree(dst, src, LANDLOCK_KEY_NET_PORT);
>> +	if (err)
>> +		goto out_unlock;
>> +#endif /* IS_ENABLED(CONFIG_INET) */
>>   out_unlock:
>>   	mutex_unlock(&src->lock);
>>   	mutex_unlock(&dst->lock);
>> @@ -426,7 +450,12 @@ static int inherit_ruleset(struct landlock_ruleset *const parent,
>>   	err = inherit_tree(parent, child, LANDLOCK_KEY_INODE);
>>   	if (err)
>>   		goto out_unlock;
>> -
> 
> newline
> 
>> +#if IS_ENABLED(CONFIG_INET)
>> +	/* Copies the @parent network port tree. */
>> +	err = inherit_tree(parent, child, LANDLOCK_KEY_NET_PORT);
>> +	if (err)
>> +		goto out_unlock;
>> +#endif /* IS_ENABLED(CONFIG_INET) */
>>   	if (WARN_ON_ONCE(child->num_layers <= parent->num_layers)) {
>>   		err = -EINVAL;
>>   		goto out_unlock;
>> @@ -459,6 +488,11 @@ static void free_ruleset(struct landlock_ruleset *const ruleset)
>>   	rbtree_postorder_for_each_entry_safe(freeme, next, &ruleset->root_inode,
>>   					     node)
>>   		free_rule(freeme, LANDLOCK_KEY_INODE);
>> +#if IS_ENABLED(CONFIG_INET)
>> +	rbtree_postorder_for_each_entry_safe(freeme, next,
>> +					     &ruleset->root_net_port, node)
>> +		free_rule(freeme, LANDLOCK_KEY_NET_PORT);
>> +#endif /* IS_ENABLED(CONFIG_INET) */
>>   	put_hierarchy(ruleset->hierarchy);
>>   	kfree(ruleset);
>>   }
>> @@ -637,6 +671,9 @@ get_access_mask_t(const struct landlock_ruleset *const ruleset,
>>    * Populates @layer_masks such that for each access right in @access_request,
>>    * the bits for all the layers are set where this access right is handled.
>>    *
>> + * @layer_masks must contain LANDLOCK_NUM_ACCESS_FS or LANDLOCK_NUM_ACCESS_NET
>> + * elements according to @key_type.
> 
> Please include this sentence in the @layer_masks description below.
> 
>> + *
>>    * @domain: The domain that defines the current restrictions.
>>    * @access_request: The requested access rights to check.
>>    * @layer_masks: The layer masks to populate.
> 
> "It must contain…"
> 
> 
>> @@ -659,6 +696,12 @@ access_mask_t init_layer_masks(const struct landlock_ruleset *const domain,
>>   		get_access_mask = landlock_get_fs_access_mask;
>>   		num_access = LANDLOCK_NUM_ACCESS_FS;
>>   		break;
>> +#if IS_ENABLED(CONFIG_INET)
>> +	case LANDLOCK_KEY_NET_PORT:
>> +		get_access_mask = landlock_get_net_access_mask;
>> +		num_access = LANDLOCK_NUM_ACCESS_NET;
>> +		break;
>> +#endif /* IS_ENABLED(CONFIG_INET) */
>>   	default:
>>   		WARN_ON_ONCE(1);
>>   		return 0;
>> diff --git a/security/landlock/ruleset.h b/security/landlock/ruleset.h
>> index d9eb79ea9a89..f272d2cd518c 100644
>> --- a/security/landlock/ruleset.h
>> +++ b/security/landlock/ruleset.h
>> @@ -19,16 +19,20 @@
>>   #include "limits.h"
>>   #include "object.h"
>> 
>> +/* Rule access mask. */
>>   typedef u16 access_mask_t;
>>   /* Makes sure all filesystem access rights can be stored. */
>>   static_assert(BITS_PER_TYPE(access_mask_t) >= LANDLOCK_NUM_ACCESS_FS);
>> +/* Makes sure all network access rights can be stored. */
>> +static_assert(BITS_PER_TYPE(access_mask_t) >= LANDLOCK_NUM_ACCESS_NET);
>>   /* Makes sure for_each_set_bit() and for_each_clear_bit() calls are OK. */
>>   static_assert(sizeof(unsigned long) >= sizeof(access_mask_t));
>> 
>>   /* Ruleset access masks. */
>> -typedef u16 access_masks_t;
>> +typedef u32 access_masks_t;
> 
> This type change need to be explained in the commit message.
> 
> 
>>   /* Makes sure all ruleset access rights can be stored. */
>> -static_assert(BITS_PER_TYPE(access_masks_t) >= LANDLOCK_NUM_ACCESS_FS);
>> +static_assert(BITS_PER_TYPE(access_masks_t) >=
>> +	      LANDLOCK_NUM_ACCESS_FS + LANDLOCK_NUM_ACCESS_NET);
>> 
>>   typedef u16 layer_mask_t;
>>   /* Makes sure all layers can be checked. */
>> @@ -82,6 +86,13 @@ enum landlock_key_type {
>>   	 * keys.
>>   	 */
>>   	LANDLOCK_KEY_INODE = 1,
>> +#if IS_ENABLED(CONFIG_INET)
>> +	/**
>> +	 * @LANDLOCK_KEY_NET_PORT: Type of &landlock_ruleset.root_net_port's
>> +	 * node keys.
>> +	 */
>> +	LANDLOCK_KEY_NET_PORT = 2,
>> +#endif /* IS_ENABLED(CONFIG_INET) */
>>   };
>> 
>>   /**
>> @@ -156,6 +167,15 @@ struct landlock_ruleset {
>>   	 * reaches zero.
>>   	 */
>>   	struct rb_root root_inode;
>> +#if IS_ENABLED(CONFIG_INET)
>> +	/**
>> +	 * @root_net_port: Root of a red-black tree containing &struct
>> +	 * landlock_rule nodes with network port. 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_net_port;
>> +#endif /* IS_ENABLED(CONFIG_INET) */
>>   	/**
>>   	 * @hierarchy: Enables hierarchy identification even when a parent
>>   	 * domain vanishes.  This is needed for the ptrace protection.
>> @@ -166,8 +186,8 @@ struct landlock_ruleset {
>>   		 * @work_free: Enables to free a ruleset within a lockless
>>   		 * section.  This is only used by
>>   		 * landlock_put_ruleset_deferred() when @usage reaches zero.
>> -		 * The fields @lock, @usage, @num_rules, @num_layers and
>> -		 * @access_masks are then unused.
>> +		 * The fields @lock, @usage, @num_rules, @num_layers,
>> +		 * @net_access_mask and @access_masks are then unused.
> 
> There is no net_access_mask anymore.
> .

Re: [PATCH v8 07/12] landlock: Add network rules support

[Konstantin Meskhidze (A)]

11/17/2022 9:43 PM, Mickaël Salaün пишет:
> 
> On 21/10/2022 17:26, Konstantin Meskhidze wrote:
>> This commit adds network rules support in internal landlock functions
>> (presented in ruleset.c) and landlock_create_ruleset syscall.
> 
> …in the ruleset management helpers and the landlock_create_ruleset syscall.
> 
> 
>> Refactors user space API to support network actions. Adds new network
> 
> Refactor…
> 
>> access flags, network rule and network attributes. Increments Landlock
> 
> Increment…
> 
>> ABI version.
>> 
>> Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze@huawei.com>
>> ---
>> 
>> Changes since v7:
>> * Squashes commits.
>> * Increments ABI version to 4.
>> * Refactors commit message.
>> * Minor fixes.
>> 
>> Changes since v6:
>> * Renames landlock_set_net_access_mask() to landlock_add_net_access_mask()
>>    because it OR values.
>> * Makes landlock_add_net_access_mask() more resilient incorrect values.
>> * Refactors landlock_get_net_access_mask().
>> * Renames LANDLOCK_MASK_SHIFT_NET to LANDLOCK_SHIFT_ACCESS_NET and use
>>    LANDLOCK_NUM_ACCESS_FS as value.
>> * Updates access_masks_t to u32 to support network access actions.
>> * Refactors landlock internal functions to support network actions with
>>    landlock_key/key_type/id types.
>> 
>> Changes since v5:
>> * Gets rid of partial revert from landlock_add_rule
>> syscall.
>> * Formats code with clang-format-14.
>> 
>> Changes since v4:
>> * Refactors landlock_create_ruleset() - splits ruleset and
>> masks checks.
>> * Refactors landlock_create_ruleset() and landlock mask
>> setters/getters to support two rule types.
>> * Refactors landlock_add_rule syscall add_rule_path_beneath
>> function by factoring out get_ruleset_from_fd() and
>> landlock_put_ruleset().
>> 
>> Changes since v3:
>> * Splits commit.
>> * Adds network rule support for internal landlock functions.
>> * Adds set_mask and get_mask for network.
>> * Adds rb_root root_net_port.
>> 
>> ---
>>   include/uapi/linux/landlock.h                | 49 ++++++++++++++
>>   security/landlock/limits.h                   |  6 +-
>>   security/landlock/ruleset.c                  | 55 ++++++++++++++--
>>   security/landlock/ruleset.h                  | 68 ++++++++++++++++----
>>   security/landlock/syscalls.c                 | 13 +++-
>>   tools/testing/selftests/landlock/base_test.c |  2 +-
>>   6 files changed, 170 insertions(+), 23 deletions(-)
>> 
>> diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
>> index f3223f964691..096b683c6ff3 100644
>> --- a/include/uapi/linux/landlock.h
>> +++ b/include/uapi/linux/landlock.h
>> @@ -31,6 +31,13 @@ struct landlock_ruleset_attr {
>>   	 * this access right.
>>   	 */
>>   	__u64 handled_access_fs;
>> +
>> +	/**
>> +	 * @handled_access_net: Bitmask of actions (cf. `Network flags`_)
>> +	 * that is handled by this ruleset and should then be forbidden if no
>> +	 * rule explicitly allow them.
>> +	 */
>> +	__u64 handled_access_net;
>>   };
>> 
>>   /*
>> @@ -54,6 +61,11 @@ enum landlock_rule_type {
>>   	 * landlock_path_beneath_attr .
>>   	 */
>>   	LANDLOCK_RULE_PATH_BENEATH = 1,
>> +	/**
>> +	 * @LANDLOCK_RULE_NET_SERVICE: Type of a &struct
>> +	 * landlock_net_service_attr .
>> +	 */
>> +	LANDLOCK_RULE_NET_SERVICE = 2,
>>   };
>> 
>>   /**
>> @@ -79,6 +91,24 @@ struct landlock_path_beneath_attr {
>>   	 */
>>   } __attribute__((packed));
>> 
>> +/**
>> + * struct landlock_net_service_attr - TCP subnet definition
>> + *
>> + * Argument of sys_landlock_add_rule().
>> + */
>> +struct landlock_net_service_attr {
>> +	/**
>> +	 * @allowed_access: Bitmask of allowed access network for services
>> +	 * (cf. `Network flags`_).
>> +	 */
>> +	__u64 allowed_access;
>> +	/**
>> +	 * @port: Network port.
>> +	 */
>> +	__u16 port;
> 
>   From an UAPI point of view, I think the port field should be __be16, as
> for sockaddr_in->port and other network-related APIs. This will require
> some kernel changes to please sparse: make C=2 security/landlock/ must
> not print any warning.

   I have this errors trying to launch sparse checking:

   DESCEND objtool
   DESCEND bpf/resolve_btfids
   CALL    scripts/checksyscalls.sh
   CHK     kernel/kheaders_data.tar.xz
   CC      security/landlock/setup.o
   CHECK   security/landlock/setup.c
./include/asm-generic/rwonce.h:67:16: error: typename in expression
./include/asm-generic/rwonce.h:67:16: error: Expected ) in function call
./include/asm-generic/rwonce.h:67:16: error: got :
./include/linux/list.h:292:16: error: typename in expression
./include/linux/list.h:292:16: error: Expected ) in function call
./include/linux/list.h:292:16: error: got :

....

./include/linux/seqlock.h:682:16: error: Expected ) in function call
./include/linux/seqlock.h:682:16: error: got :
./include/linux/seqlock.h:695:16: error: typename in expression
./include/linux/seqlock.h:695:16: error: Expected ) in function call
./include/linux/seqlock.h:695:16: error: too many errors
Segmentation fault (core dumped)
make[3]: *** [scripts/Makefile.build:250: security/landlock/setup.o] 
Error 139
make[3]: *** Deleting file 'security/landlock/setup.o'
make[3]: *** Waiting for unfinished jobs....
Segmentation fault (core dumped)
make[3]: *** [scripts/Makefile.build:250: security/landlock/syscalls.o] 
Error 139
make[3]: *** Deleting file 'security/landlock/syscalls.o'
make[2]: *** [scripts/Makefile.build:502: security/landlock] Error 2
make[1]: *** [scripts/Makefile.build:502: security] Error 2
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:1994: .] Error 2



> 
> Using big-endian values as keys (casted to uintptr_t, not strictly
> __be16) in the rb-tree should not be an issue because there is no port
> range ordering (for now).
> 
> A dedicated test should check that endianness is correct, e.g. by using
> different port encoding. This should include passing and failing tests,
> but they should work on all architectures (i.e. big or little endian).
> 
> 
>> +
>> +} __attribute__((packed));
>> +
>>   /**
>>    * DOC: fs_access
>>    *
>> @@ -173,4 +203,23 @@ struct landlock_path_beneath_attr {
>>   #define LANDLOCK_ACCESS_FS_TRUNCATE			(1ULL << 14)
>>   /* clang-format on */
>> 
>> +/**
>> + * DOC: net_access
>> + *
>> + * Network flags
>> + * ~~~~~~~~~~~~~~~~
>> + *
>> + * These flags enable to restrict a sandboxed process to a set of network
>> + * actions.
>> + *
>> + * TCP sockets with allowed actions:
>> + *
>> + * - %LANDLOCK_ACCESS_NET_BIND_TCP: Bind a TCP socket to a local port.
>> + * - %LANDLOCK_ACCESS_NET_CONNECT_TCP: Connect an active TCP socket to
>> + *   a remote port.
>> + */
>> +/* clang-format off */
>> +#define LANDLOCK_ACCESS_NET_BIND_TCP			(1ULL << 0)
>> +#define LANDLOCK_ACCESS_NET_CONNECT_TCP			(1ULL << 1)
>> +/* clang-format on */
>>   #endif /* _UAPI_LINUX_LANDLOCK_H */
>> diff --git a/security/landlock/limits.h b/security/landlock/limits.h
>> index bafb3b8dc677..8a1a6463c64e 100644
>> --- a/security/landlock/limits.h
>> +++ b/security/landlock/limits.h
>> @@ -23,6 +23,10 @@
>>   #define LANDLOCK_NUM_ACCESS_FS		__const_hweight64(LANDLOCK_MASK_ACCESS_FS)
>>   #define LANDLOCK_SHIFT_ACCESS_FS	0
>> 
>> -/* clang-format on */
>> +#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 */
>>   #endif /* _SECURITY_LANDLOCK_LIMITS_H */
>> diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
>> index c7cf54ba4f6d..9277c1295114 100644
>> --- a/security/landlock/ruleset.c
>> +++ b/security/landlock/ruleset.c
>> @@ -36,6 +36,9 @@ static struct landlock_ruleset *create_ruleset(const u32 num_layers)
>>   	refcount_set(&new_ruleset->usage, 1);
>>   	mutex_init(&new_ruleset->lock);
>>   	new_ruleset->root_inode = RB_ROOT;
>> +#if IS_ENABLED(CONFIG_INET)
>> +	new_ruleset->root_net_port = RB_ROOT;
>> +#endif /* IS_ENABLED(CONFIG_INET) */
>>   	new_ruleset->num_layers = num_layers;
>>   	/*
>>   	 * hierarchy = NULL
>> @@ -46,16 +49,21 @@ static struct landlock_ruleset *create_ruleset(const u32 num_layers)
>>   }
>> 
>>   struct landlock_ruleset *
>> -landlock_create_ruleset(const access_mask_t fs_access_mask)
>> +landlock_create_ruleset(const access_mask_t fs_access_mask,
>> +			const access_mask_t net_access_mask)
>>   {
>>   	struct landlock_ruleset *new_ruleset;
>> 
>>   	/* Informs about useless ruleset. */
>> -	if (!fs_access_mask)
>> +	if (!fs_access_mask && !net_access_mask)
>>   		return ERR_PTR(-ENOMSG);
>>   	new_ruleset = create_ruleset(1);
>> -	if (!IS_ERR(new_ruleset))
>> +	if (IS_ERR(new_ruleset))
>> +		return new_ruleset;
>> +	if (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);
>>   	return new_ruleset;
>>   }
>> 
>> @@ -73,6 +81,10 @@ static inline bool is_object_pointer(const enum landlock_key_type key_type)
>>   	switch (key_type) {
>>   	case LANDLOCK_KEY_INODE:
>>   		return true;
>> +#if IS_ENABLED(CONFIG_INET)
>> +	case LANDLOCK_KEY_NET_PORT:
>> +		return false;
>> +#endif /* IS_ENABLED(CONFIG_INET) */
>>   	}
>>   	WARN_ON_ONCE(1);
>>   	return false;
>> @@ -126,6 +138,11 @@ static inline struct rb_root *get_root(struct landlock_ruleset *const ruleset,
>>   	case LANDLOCK_KEY_INODE:
>>   		root = &ruleset->root_inode;
>>   		break;
>> +#if IS_ENABLED(CONFIG_INET)
>> +	case LANDLOCK_KEY_NET_PORT:
>> +		root = &ruleset->root_net_port;
>> +		break;
>> +#endif /* IS_ENABLED(CONFIG_INET) */
>>   	}
>>   	if (WARN_ON_ONCE(!root))
>>   		return ERR_PTR(-EINVAL);
>> @@ -154,7 +171,9 @@ static void build_check_ruleset(void)
>>   	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_FS << LANDLOCK_SHIFT_ACCESS_FS) +
> 
> This is correct but because we are dealing with bitmasks I would prefer
> to use "|" instead of "+".
> 
> 
>> +			     (LANDLOCK_MASK_ACCESS_NET
>> +			      << LANDLOCK_SHIFT_ACCESS_NET));
>>   }
>> 
>>   /**
>> @@ -370,7 +389,12 @@ static int merge_ruleset(struct landlock_ruleset *const dst,
>>   	err = merge_tree(dst, src, LANDLOCK_KEY_INODE);
>>   	if (err)
>>   		goto out_unlock;
>> -
> 
> Please keep this newline.
> 
>> +#if IS_ENABLED(CONFIG_INET)
>> +	/* Merges the @src network port tree. */
>> +	err = merge_tree(dst, src, LANDLOCK_KEY_NET_PORT);
>> +	if (err)
>> +		goto out_unlock;
>> +#endif /* IS_ENABLED(CONFIG_INET) */
>>   out_unlock:
>>   	mutex_unlock(&src->lock);
>>   	mutex_unlock(&dst->lock);
>> @@ -426,7 +450,12 @@ static int inherit_ruleset(struct landlock_ruleset *const parent,
>>   	err = inherit_tree(parent, child, LANDLOCK_KEY_INODE);
>>   	if (err)
>>   		goto out_unlock;
>> -
> 
> newline
> 
>> +#if IS_ENABLED(CONFIG_INET)
>> +	/* Copies the @parent network port tree. */
>> +	err = inherit_tree(parent, child, LANDLOCK_KEY_NET_PORT);
>> +	if (err)
>> +		goto out_unlock;
>> +#endif /* IS_ENABLED(CONFIG_INET) */
>>   	if (WARN_ON_ONCE(child->num_layers <= parent->num_layers)) {
>>   		err = -EINVAL;
>>   		goto out_unlock;
>> @@ -459,6 +488,11 @@ static void free_ruleset(struct landlock_ruleset *const ruleset)
>>   	rbtree_postorder_for_each_entry_safe(freeme, next, &ruleset->root_inode,
>>   					     node)
>>   		free_rule(freeme, LANDLOCK_KEY_INODE);
>> +#if IS_ENABLED(CONFIG_INET)
>> +	rbtree_postorder_for_each_entry_safe(freeme, next,
>> +					     &ruleset->root_net_port, node)
>> +		free_rule(freeme, LANDLOCK_KEY_NET_PORT);
>> +#endif /* IS_ENABLED(CONFIG_INET) */
>>   	put_hierarchy(ruleset->hierarchy);
>>   	kfree(ruleset);
>>   }
>> @@ -637,6 +671,9 @@ get_access_mask_t(const struct landlock_ruleset *const ruleset,
>>    * Populates @layer_masks such that for each access right in @access_request,
>>    * the bits for all the layers are set where this access right is handled.
>>    *
>> + * @layer_masks must contain LANDLOCK_NUM_ACCESS_FS or LANDLOCK_NUM_ACCESS_NET
>> + * elements according to @key_type.
> 
> Please include this sentence in the @layer_masks description below.
> 
>> + *
>>    * @domain: The domain that defines the current restrictions.
>>    * @access_request: The requested access rights to check.
>>    * @layer_masks: The layer masks to populate.
> 
> "It must contain…"
> 
> 
>> @@ -659,6 +696,12 @@ access_mask_t init_layer_masks(const struct landlock_ruleset *const domain,
>>   		get_access_mask = landlock_get_fs_access_mask;
>>   		num_access = LANDLOCK_NUM_ACCESS_FS;
>>   		break;
>> +#if IS_ENABLED(CONFIG_INET)
>> +	case LANDLOCK_KEY_NET_PORT:
>> +		get_access_mask = landlock_get_net_access_mask;
>> +		num_access = LANDLOCK_NUM_ACCESS_NET;
>> +		break;
>> +#endif /* IS_ENABLED(CONFIG_INET) */
>>   	default:
>>   		WARN_ON_ONCE(1);
>>   		return 0;
>> diff --git a/security/landlock/ruleset.h b/security/landlock/ruleset.h
>> index d9eb79ea9a89..f272d2cd518c 100644
>> --- a/security/landlock/ruleset.h
>> +++ b/security/landlock/ruleset.h
>> @@ -19,16 +19,20 @@
>>   #include "limits.h"
>>   #include "object.h"
>> 
>> +/* Rule access mask. */
>>   typedef u16 access_mask_t;
>>   /* Makes sure all filesystem access rights can be stored. */
>>   static_assert(BITS_PER_TYPE(access_mask_t) >= LANDLOCK_NUM_ACCESS_FS);
>> +/* Makes sure all network access rights can be stored. */
>> +static_assert(BITS_PER_TYPE(access_mask_t) >= LANDLOCK_NUM_ACCESS_NET);
>>   /* Makes sure for_each_set_bit() and for_each_clear_bit() calls are OK. */
>>   static_assert(sizeof(unsigned long) >= sizeof(access_mask_t));
>> 
>>   /* Ruleset access masks. */
>> -typedef u16 access_masks_t;
>> +typedef u32 access_masks_t;
> 
> This type change need to be explained in the commit message.
> 
> 
>>   /* Makes sure all ruleset access rights can be stored. */
>> -static_assert(BITS_PER_TYPE(access_masks_t) >= LANDLOCK_NUM_ACCESS_FS);
>> +static_assert(BITS_PER_TYPE(access_masks_t) >=
>> +	      LANDLOCK_NUM_ACCESS_FS + LANDLOCK_NUM_ACCESS_NET);
>> 
>>   typedef u16 layer_mask_t;
>>   /* Makes sure all layers can be checked. */
>> @@ -82,6 +86,13 @@ enum landlock_key_type {
>>   	 * keys.
>>   	 */
>>   	LANDLOCK_KEY_INODE = 1,
>> +#if IS_ENABLED(CONFIG_INET)
>> +	/**
>> +	 * @LANDLOCK_KEY_NET_PORT: Type of &landlock_ruleset.root_net_port's
>> +	 * node keys.
>> +	 */
>> +	LANDLOCK_KEY_NET_PORT = 2,
>> +#endif /* IS_ENABLED(CONFIG_INET) */
>>   };
>> 
>>   /**
>> @@ -156,6 +167,15 @@ struct landlock_ruleset {
>>   	 * reaches zero.
>>   	 */
>>   	struct rb_root root_inode;
>> +#if IS_ENABLED(CONFIG_INET)
>> +	/**
>> +	 * @root_net_port: Root of a red-black tree containing &struct
>> +	 * landlock_rule nodes with network port. 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_net_port;
>> +#endif /* IS_ENABLED(CONFIG_INET) */
>>   	/**
>>   	 * @hierarchy: Enables hierarchy identification even when a parent
>>   	 * domain vanishes.  This is needed for the ptrace protection.
>> @@ -166,8 +186,8 @@ struct landlock_ruleset {
>>   		 * @work_free: Enables to free a ruleset within a lockless
>>   		 * section.  This is only used by
>>   		 * landlock_put_ruleset_deferred() when @usage reaches zero.
>> -		 * The fields @lock, @usage, @num_rules, @num_layers and
>> -		 * @access_masks are then unused.
>> +		 * The fields @lock, @usage, @num_rules, @num_layers,
>> +		 * @net_access_mask and @access_masks are then unused.
> 
> There is no net_access_mask anymore.
> .

Re: [PATCH v8 08/12] landlock: Implement TCP network hooks

[Konstantin Meskhidze (A)]

11/17/2022 9:43 PM, Mickaël Salaün пишет:
> 
> On 21/10/2022 17:26, Konstantin Meskhidze wrote:
>> This patch adds support of socket_bind() and socket_connect() hooks.
>> It's possible to restrict binding and connecting of TCP sockets to
>> particular ports.
> 
> Implement socket_bind() and socket_connect LSM hooks, which enable to
> restrict TCP socket binding and connection to specific ports.
> 
> 
>> 
>> Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze@huawei.com>
>> ---
>> 
>> Changes since v7:
>> * Minor fixes.
>> * Refactors commit message.
>> 
>> Changes since v6:
>> * Updates copyright.
>> * Refactors landlock_append_net_rule() and check_socket_access()
>>    functions with landlock_id type.
>> 
>> Changes since v5:
>> * Fixes some logic errors.
>> * Formats code with clang-format-14.
>> 
>> Changes since v4:
>> * Factors out CONFIG_INET into make file.
>> * Refactors check_socket_access().
>> * Adds helper get_port().
>> * Adds CONFIG_IPV6 in get_port(), hook_socket_bind/connect
>> functions to support AF_INET6 family.
>> * Adds AF_UNSPEC family support in hook_socket_bind/connect
>> functions.
>> * Refactors add_rule_net_service() and landlock_add_rule
>> syscall to support network rule inserting.
>> * Refactors init_layer_masks() to support network rules.
>> 
>> Changes since v3:
>> * Splits commit.
>> * Adds SECURITY_NETWORK in config.
>> * Adds IS_ENABLED(CONFIG_INET) if a kernel has no INET configuration.
>> * Adds hook_socket_bind and hook_socket_connect hooks.
>> 
>> ---
>>   security/landlock/Kconfig    |   1 +
>>   security/landlock/Makefile   |   2 +
>>   security/landlock/net.c      | 164 +++++++++++++++++++++++++++++++++++
>>   security/landlock/net.h      |  26 ++++++
>>   security/landlock/setup.c    |   2 +
>>   security/landlock/syscalls.c |  59 ++++++++++++-
>>   6 files changed, 251 insertions(+), 3 deletions(-)
>>   create mode 100644 security/landlock/net.c
>>   create mode 100644 security/landlock/net.h
>> 
>> diff --git a/security/landlock/Kconfig b/security/landlock/Kconfig
>> index 8e33c4e8ffb8..10c099097533 100644
>> --- a/security/landlock/Kconfig
>> +++ b/security/landlock/Kconfig
>> @@ -3,6 +3,7 @@
>>   config SECURITY_LANDLOCK
>>   	bool "Landlock support"
>>   	depends on SECURITY && !ARCH_EPHEMERAL_INODES
>> +	select SECURITY_NETWORK
>>   	select SECURITY_PATH
>>   	help
>>   	  Landlock is a sandboxing mechanism that enables processes to restrict
>> diff --git a/security/landlock/Makefile b/security/landlock/Makefile
>> index 7bbd2f413b3e..53d3c92ae22e 100644
>> --- a/security/landlock/Makefile
>> +++ b/security/landlock/Makefile
>> @@ -2,3 +2,5 @@ obj-$(CONFIG_SECURITY_LANDLOCK) := landlock.o
>> 
>>   landlock-y := setup.o syscalls.o object.o ruleset.o \
>>   	cred.o ptrace.o fs.o
>> +
>> +landlock-$(CONFIG_INET) += net.o
>> \ No newline at end of file
>> diff --git a/security/landlock/net.c b/security/landlock/net.c
>> new file mode 100644
>> index 000000000000..39e8a156a1f4
>> --- /dev/null
>> +++ b/security/landlock/net.c
>> @@ -0,0 +1,164 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Landlock LSM - Network management and hooks
>> + *
>> + * Copyright © 2022 Huawei Tech. Co., Ltd.
>> + * Copyright © 2022 Microsoft Corporation
>> + */
>> +
>> +#include <linux/in.h>
>> +#include <linux/net.h>
>> +#include <linux/socket.h>
>> +#include <net/ipv6.h>
>> +
>> +#include "common.h"
>> +#include "cred.h"
>> +#include "limits.h"
>> +#include "net.h"
>> +#include "ruleset.h"
>> +
>> +int landlock_append_net_rule(struct landlock_ruleset *const ruleset,
>> +			     const u16 port, access_mask_t access_rights)
>> +{
>> +	int err;
>> +	const struct landlock_id id = {
>> +		.key.data = port,
>> +		.type = LANDLOCK_KEY_NET_PORT,
>> +	};
>> +	BUILD_BUG_ON(sizeof(port) > sizeof(id.key.data));
>> +
>> +	/* Transforms relative access rights to absolute ones. */
>> +	access_rights |= LANDLOCK_MASK_ACCESS_NET &
>> +			 ~landlock_get_net_access_mask(ruleset, 0);
>> +
>> +	mutex_lock(&ruleset->lock);
>> +	err = landlock_insert_rule(ruleset, id, access_rights);
>> +	mutex_unlock(&ruleset->lock);
>> +
>> +	return err;
>> +}
>> +
>> +static int check_socket_access(const struct landlock_ruleset *const domain,
>> +			       u16 port, access_mask_t access_request)
>> +{
>> +	bool allowed = false;
>> +	layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_NET] = {};
>> +	const struct landlock_rule *rule;
>> +	access_mask_t handled_access;
>> +	const struct landlock_id id = {
>> +		.key.data = port,
>> +		.type = LANDLOCK_KEY_NET_PORT,
>> +	};
>> +
>> +	if (WARN_ON_ONCE(!domain))
>> +		return 0;
>> +	if (WARN_ON_ONCE(domain->num_layers < 1))
>> +		return -EACCES;
>> +
>> +	rule = landlock_find_rule(domain, id);
>> +	handled_access = init_layer_masks(domain, access_request, &layer_masks,
>> +					  LANDLOCK_KEY_NET_PORT);
>> +	allowed = unmask_layers(rule, handled_access, &layer_masks,
>> +				ARRAY_SIZE(layer_masks));
>> +
>> +	return allowed ? 0 : -EACCES;
>> +}
>> +
>> +static u16 get_port(const struct sockaddr *const address)
> 
> get_port() should return a __be16 type. This enables to avoid converting
> port when checking a rule.

   In this case a user must do a coverting port into __be16:

   struct landlock_net_service_attr net_service = {
                 .allowed_access = LANDLOCK_ACCESS_NET_BIND_TCP,

                 .port = htons(sock_port),
         };
  I think that a user should not think about this conversion cause it 
makes UAPI more complex to use. Lets do this under kernel's hood and let 
it as it is now -> u16 port.

What do you think?


> 
> make C=2 security/landlock/ must not print any warning.
> 
> 
>> +{
>> +	/* Gets port value in host byte order. */
>> +	switch (address->sa_family) {
>> +	case AF_UNSPEC:
>> +	case AF_INET: {
>> +		const struct sockaddr_in *const sockaddr =
>> +			(struct sockaddr_in *)address;
>> +		return ntohs(sockaddr->sin_port);
>> +	}
>> +#if IS_ENABLED(CONFIG_IPV6)
>> +	case AF_INET6: {
>> +		const struct sockaddr_in6 *const sockaddr_ip6 =
>> +			(struct sockaddr_in6 *)address;
>> +		return ntohs(sockaddr_ip6->sin6_port);
>> +	}
>> +#endif
>> +	}
>> +	WARN_ON_ONCE(1);
>> +	return 0;
>> +}
>> +
>> +static int hook_socket_bind(struct socket *sock, struct sockaddr *address,
>> +			    int addrlen)
>> +{
>> +	const struct landlock_ruleset *const dom =
>> +		landlock_get_current_domain();
>> +
>> +	if (!dom)
>> +		return 0;
>> +
>> +	/* Check if it's a TCP socket. */
>> +	if (sock->type != SOCK_STREAM)
>> +		return 0;
>> +
>> +	switch (address->sa_family) {
>> +	case AF_UNSPEC:
>> +	case AF_INET:
>> +#if IS_ENABLED(CONFIG_IPV6)
>> +	case AF_INET6:
>> +#endif
>> +		return check_socket_access(dom, get_port(address),
>> +					   LANDLOCK_ACCESS_NET_BIND_TCP);
>> +	default:
>> +		return 0;
> 
> You can remove this default case and move the return 0 at the end of the
> function.
> 
> 
>> +	}
>> +}
>> +
>> +static int hook_socket_connect(struct socket *sock, struct sockaddr *address,
>> +			       int addrlen)
>> +{
>> +	const struct landlock_ruleset *const dom =
>> +		landlock_get_current_domain();
>> +
>> +	if (!dom)
>> +		return 0;
>> +
>> +	/* Check if it's a TCP socket. */
>> +	if (sock->type != SOCK_STREAM)
>> +		return 0;
>> +
>> +	/* Check if the hook is AF_INET* socket's action. */
>> +	switch (address->sa_family) {
>> +	case AF_INET:
>> +#if IS_ENABLED(CONFIG_IPV6)
>> +	case AF_INET6:
>> +#endif
>> +		return check_socket_access(dom, get_port(address),
>> +					   LANDLOCK_ACCESS_NET_CONNECT_TCP);
>> +	case AF_UNSPEC: {
>> +		u16 i;
> 
> You can move "i" after the "dom" declaration to remove the extra braces.
> 
> 
>> +
>> +		/*
>> +		 * If just in a layer a mask supports connect access,
>> +		 * the socket_connect() hook with AF_UNSPEC family flag
>> +		 * must be banned. This prevents from disconnecting already
>> +		 * connected sockets.
>> +		 */
>> +		for (i = 0; i < dom->num_layers; i++) {
>> +			if (landlock_get_net_access_mask(dom, i) &
>> +			    LANDLOCK_ACCESS_NET_CONNECT_TCP)
>> +				return -EACCES;
> 
> I'm wondering if this is the right error code for this case. EPERM may
> be more appropriate.
> 
> Thinking more about this case, I don't understand what is the rationale
> to deny such action. What would be the consequence to always allow
> connection with AF_UNSPEC (i.e. to disconnect a socket)?
> 
> 
>> +		}
>> +	}
>> +	}
>> +	return 0;
>> +}
>> +
>> +static struct security_hook_list landlock_hooks[] __lsm_ro_after_init = {
>> +	LSM_HOOK_INIT(socket_bind, hook_socket_bind),
>> +	LSM_HOOK_INIT(socket_connect, hook_socket_connect),
>> +};
>> +
>> +__init void landlock_add_net_hooks(void)
>> +{
>> +	security_add_hooks(landlock_hooks, ARRAY_SIZE(landlock_hooks),
>> +			   LANDLOCK_NAME);
>> +}
>> diff --git a/security/landlock/net.h b/security/landlock/net.h
>> new file mode 100644
>> index 000000000000..0da1d9dff5ab
>> --- /dev/null
>> +++ b/security/landlock/net.h
>> @@ -0,0 +1,26 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * Landlock LSM - Network management and hooks
>> + *
>> + * Copyright © 2022 Huawei Tech. Co., Ltd.
>> + */
>> +
>> +#ifndef _SECURITY_LANDLOCK_NET_H
>> +#define _SECURITY_LANDLOCK_NET_H
>> +
>> +#include "common.h"
>> +#include "ruleset.h"
>> +#include "setup.h"
>> +
>> +#if IS_ENABLED(CONFIG_INET)
>> +__init void landlock_add_net_hooks(void);
>> +
>> +int landlock_append_net_rule(struct landlock_ruleset *const ruleset,
>> +			     const u16 port, access_mask_t access_rights);
>> +#else /* IS_ENABLED(CONFIG_INET) */
>> +static inline void landlock_add_net_hooks(void)
>> +{
>> +}
>> +#endif /* IS_ENABLED(CONFIG_INET) */
>> +
>> +#endif /* _SECURITY_LANDLOCK_NET_H */
>> diff --git a/security/landlock/setup.c b/security/landlock/setup.c
>> index 3f196d2ce4f9..7e4a598177b8 100644
>> --- a/security/landlock/setup.c
>> +++ b/security/landlock/setup.c
>> @@ -14,6 +14,7 @@
>>   #include "fs.h"
>>   #include "ptrace.h"
>>   #include "setup.h"
>> +#include "net.h"
>> 
>>   bool landlock_initialized __lsm_ro_after_init = false;
>> 
>> @@ -29,6 +30,7 @@ static int __init landlock_init(void)
>>   	landlock_add_cred_hooks();
>>   	landlock_add_ptrace_hooks();
>>   	landlock_add_fs_hooks();
>> +	landlock_add_net_hooks();
>>   	landlock_initialized = true;
>>   	pr_info("Up and running.\n");
>>   	return 0;
>> diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c
>> index c5a6ad4e2fca..7853f32e8325 100644
>> --- a/security/landlock/syscalls.c
>> +++ b/security/landlock/syscalls.c
>> @@ -29,6 +29,7 @@
>>   #include "cred.h"
>>   #include "fs.h"
>>   #include "limits.h"
>> +#include "net.h"
>>   #include "ruleset.h"
>>   #include "setup.h"
>> 
>> @@ -74,7 +75,8 @@ static void build_check_abi(void)
>>   {
>>   	struct landlock_ruleset_attr ruleset_attr;
>>   	struct landlock_path_beneath_attr path_beneath_attr;
>> -	size_t ruleset_size, path_beneath_size;
>> +	struct landlock_net_service_attr net_service_attr;
>> +	size_t ruleset_size, path_beneath_size, net_service_size;
>> 
>>   	/*
>>   	 * For each user space ABI structures, first checks that there is no
>> @@ -90,6 +92,11 @@ static void build_check_abi(void)
>>   	path_beneath_size += sizeof(path_beneath_attr.parent_fd);
>>   	BUILD_BUG_ON(sizeof(path_beneath_attr) != path_beneath_size);
>>   	BUILD_BUG_ON(sizeof(path_beneath_attr) != 12);
>> +
>> +	net_service_size = sizeof(net_service_attr.allowed_access);
>> +	net_service_size += sizeof(net_service_attr.port);
>> +	BUILD_BUG_ON(sizeof(net_service_attr) != net_service_size);
>> +	BUILD_BUG_ON(sizeof(net_service_attr) != 10);
>>   }
>> 
>>   /* Ruleset handling */
>> @@ -322,13 +329,54 @@ static int add_rule_path_beneath(struct landlock_ruleset *const ruleset,
>>   	return err;
>>   }
>> 
>> +static int add_rule_net_service(struct landlock_ruleset *ruleset,
>> +				const void __user *const rule_attr)
>> +{
>> +#if IS_ENABLED(CONFIG_INET)
>> +	struct landlock_net_service_attr net_service_attr;
>> +	int res;
>> +	u32 mask;
> 
> access_mask_t mask;
> 
> 
>> +
>> +	/* Copies raw user space buffer, only one type for now. */
>> +	res = copy_from_user(&net_service_attr, rule_attr,
>> +			     sizeof(net_service_attr));
>> +	if (res)
>> +		return -EFAULT;
>> +
>> +	/*
>> +	 * Informs about useless rule: empty allowed_access (i.e. deny rules)
>> +	 * are ignored by network actions.
>> +	 */
>> +	if (!net_service_attr.allowed_access)
>> +		return -ENOMSG;
>> +
>> +	/*
>> +	 * Checks that allowed_access matches the @ruleset constraints
>> +	 * (ruleset->access_masks[0] is automatically upgraded to 64-bits).
>> +	 */
>> +	mask = landlock_get_net_access_mask(ruleset, 0);
>> +	if ((net_service_attr.allowed_access | mask) != mask)
>> +		return -EINVAL;
>> +
>> +	/* Denies inserting a rule with port 0. */
>> +	if (net_service_attr.port == 0)
>> +		return -EINVAL;
>> +
>> +	/* Imports the new rule. */
>> +	return landlock_append_net_rule(ruleset, net_service_attr.port,
>> +					net_service_attr.allowed_access);
>> +#else /* IS_ENABLED(CONFIG_INET) */
>> +	return -EAFNOSUPPORT;
>> +#endif /* IS_ENABLED(CONFIG_INET) */
>> +}
>> +
>>   /**
>>    * sys_landlock_add_rule - Add a new rule to a ruleset
>>    *
>>    * @ruleset_fd: File descriptor tied to the ruleset that should be extended
>>    *		with the new rule.
>> - * @rule_type: Identify the structure type pointed to by @rule_attr (only
>> - *             %LANDLOCK_RULE_PATH_BENEATH for now).
>> + * @rule_type: Identify the structure type pointed to by @rule_attr:
>> + *             %LANDLOCK_RULE_PATH_BENEATH or %LANDLOCK_RULE_NET_SERVICE.
>>    * @rule_attr: Pointer to a rule (only of type &struct
>>    *             landlock_path_beneath_attr for now).
>>    * @flags: Must be 0.
>> @@ -339,6 +387,8 @@ static int add_rule_path_beneath(struct landlock_ruleset *const ruleset,
>>    * Possible returned errors are:
>>    *
>>    * - %EOPNOTSUPP: Landlock is supported by the kernel but disabled at boot time;
>> + * - %EAFNOSUPPORT: @rule_type is LANDLOCK_RULE_NET_SERVICE but TCP/IP is not
> 
> %LANDLOCK_RULE_NET_SERVICE
> 
> 
>> + *   supported by the running kernel;
>>    * - %EINVAL: @flags is not 0, or inconsistent access in the rule (i.e.
>>    *   &landlock_path_beneath_attr.allowed_access is not a subset of the
>>    *   ruleset handled accesses);
>> @@ -373,6 +423,9 @@ SYSCALL_DEFINE4(landlock_add_rule, const int, ruleset_fd,
>>   	case LANDLOCK_RULE_PATH_BENEATH:
>>   		err = add_rule_path_beneath(ruleset, rule_attr);
>>   		break;
>> +	case LANDLOCK_RULE_NET_SERVICE:
>> +		err = add_rule_net_service(ruleset, rule_attr);
>> +		break;
>>   	default:
>>   		err = -EINVAL;
>>   		break;
>> --
>> 2.25.1
>> 
> .

Re: [PATCH v8 07/12] landlock: Add network rules support

[Mickaël Salaün]

On 04/01/2023 12:41, Konstantin Meskhidze (A) wrote:
> 
> 
> 11/17/2022 9:43 PM, Mickaël Salaün пишет:

[...]

>>>    /**
>>> @@ -79,6 +91,24 @@ struct landlock_path_beneath_attr {
>>>    	 */
>>>    } __attribute__((packed));
>>>
>>> +/**
>>> + * struct landlock_net_service_attr - TCP subnet definition
>>> + *
>>> + * Argument of sys_landlock_add_rule().
>>> + */
>>> +struct landlock_net_service_attr {
>>> +	/**
>>> +	 * @allowed_access: Bitmask of allowed access network for services
>>> +	 * (cf. `Network flags`_).
>>> +	 */
>>> +	__u64 allowed_access;
>>> +	/**
>>> +	 * @port: Network port.
>>> +	 */
>>> +	__u16 port;
>>
>>    From an UAPI point of view, I think the port field should be __be16, as
>> for sockaddr_in->port and other network-related APIs. This will require
>> some kernel changes to please sparse: make C=2 security/landlock/ must
>> not print any warning.
> 
>     I have this errors trying to launch sparse checking:
> 
>     DESCEND objtool
>     DESCEND bpf/resolve_btfids
>     CALL    scripts/checksyscalls.sh
>     CHK     kernel/kheaders_data.tar.xz
>     CC      security/landlock/setup.o
>     CHECK   security/landlock/setup.c
> ./include/asm-generic/rwonce.h:67:16: error: typename in expression
> ./include/asm-generic/rwonce.h:67:16: error: Expected ) in function call
> ./include/asm-generic/rwonce.h:67:16: error: got :
> ./include/linux/list.h:292:16: error: typename in expression
> ./include/linux/list.h:292:16: error: Expected ) in function call
> ./include/linux/list.h:292:16: error: got :
> 
> ....
> 
> ./include/linux/seqlock.h:682:16: error: Expected ) in function call
> ./include/linux/seqlock.h:682:16: error: got :
> ./include/linux/seqlock.h:695:16: error: typename in expression
> ./include/linux/seqlock.h:695:16: error: Expected ) in function call
> ./include/linux/seqlock.h:695:16: error: too many errors
> Segmentation fault (core dumped)
> make[3]: *** [scripts/Makefile.build:250: security/landlock/setup.o]
> Error 139
> make[3]: *** Deleting file 'security/landlock/setup.o'
> make[3]: *** Waiting for unfinished jobs....
> Segmentation fault (core dumped)
> make[3]: *** [scripts/Makefile.build:250: security/landlock/syscalls.o]
> Error 139
> make[3]: *** Deleting file 'security/landlock/syscalls.o'
> make[2]: *** [scripts/Makefile.build:502: security/landlock] Error 2
> make[1]: *** [scripts/Makefile.build:502: security] Error 2
> make[1]: *** Waiting for unfinished jobs....
> make: *** [Makefile:1994: .] Error 2

I don't know about this error. Did you follow the documentation?
https://docs.kernel.org/dev-tools/sparse.html#getting-sparse



>>
>> Using big-endian values as keys (casted to uintptr_t, not strictly
>> __be16) in the rb-tree should not be an issue because there is no port
>> range ordering (for now).
>>
>> A dedicated test should check that endianness is correct, e.g. by using
>> different port encoding. This should include passing and failing tests,
>> but they should work on all architectures (i.e. big or little endian).

Re: [PATCH v8 08/12] landlock: Implement TCP network hooks

[Mickaël Salaün]

On 05/01/2023 09:57, Konstantin Meskhidze (A) wrote:
> 
> 
> 11/17/2022 9:43 PM, Mickaël Salaün пишет:
>>
>> On 21/10/2022 17:26, Konstantin Meskhidze wrote:
>>> This patch adds support of socket_bind() and socket_connect() hooks.
>>> It's possible to restrict binding and connecting of TCP sockets to
>>> particular ports.
>>
>> Implement socket_bind() and socket_connect LSM hooks, which enable to
>> restrict TCP socket binding and connection to specific ports.
>>
>>
>>>
>>> Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze@huawei.com>
>>> ---
>>>
>>> Changes since v7:
>>> * Minor fixes.
>>> * Refactors commit message.
>>>
>>> Changes since v6:
>>> * Updates copyright.
>>> * Refactors landlock_append_net_rule() and check_socket_access()
>>>     functions with landlock_id type.
>>>
>>> Changes since v5:
>>> * Fixes some logic errors.
>>> * Formats code with clang-format-14.
>>>
>>> Changes since v4:
>>> * Factors out CONFIG_INET into make file.
>>> * Refactors check_socket_access().
>>> * Adds helper get_port().
>>> * Adds CONFIG_IPV6 in get_port(), hook_socket_bind/connect
>>> functions to support AF_INET6 family.
>>> * Adds AF_UNSPEC family support in hook_socket_bind/connect
>>> functions.
>>> * Refactors add_rule_net_service() and landlock_add_rule
>>> syscall to support network rule inserting.
>>> * Refactors init_layer_masks() to support network rules.
>>>
>>> Changes since v3:
>>> * Splits commit.
>>> * Adds SECURITY_NETWORK in config.
>>> * Adds IS_ENABLED(CONFIG_INET) if a kernel has no INET configuration.
>>> * Adds hook_socket_bind and hook_socket_connect hooks.
>>>
>>> ---
>>>    security/landlock/Kconfig    |   1 +
>>>    security/landlock/Makefile   |   2 +
>>>    security/landlock/net.c      | 164 +++++++++++++++++++++++++++++++++++
>>>    security/landlock/net.h      |  26 ++++++
>>>    security/landlock/setup.c    |   2 +
>>>    security/landlock/syscalls.c |  59 ++++++++++++-
>>>    6 files changed, 251 insertions(+), 3 deletions(-)
>>>    create mode 100644 security/landlock/net.c
>>>    create mode 100644 security/landlock/net.h
>>>
>>> diff --git a/security/landlock/Kconfig b/security/landlock/Kconfig
>>> index 8e33c4e8ffb8..10c099097533 100644
>>> --- a/security/landlock/Kconfig
>>> +++ b/security/landlock/Kconfig
>>> @@ -3,6 +3,7 @@
>>>    config SECURITY_LANDLOCK
>>>    	bool "Landlock support"
>>>    	depends on SECURITY && !ARCH_EPHEMERAL_INODES
>>> +	select SECURITY_NETWORK
>>>    	select SECURITY_PATH
>>>    	help
>>>    	  Landlock is a sandboxing mechanism that enables processes to restrict
>>> diff --git a/security/landlock/Makefile b/security/landlock/Makefile
>>> index 7bbd2f413b3e..53d3c92ae22e 100644
>>> --- a/security/landlock/Makefile
>>> +++ b/security/landlock/Makefile
>>> @@ -2,3 +2,5 @@ obj-$(CONFIG_SECURITY_LANDLOCK) := landlock.o
>>>
>>>    landlock-y := setup.o syscalls.o object.o ruleset.o \
>>>    	cred.o ptrace.o fs.o
>>> +
>>> +landlock-$(CONFIG_INET) += net.o
>>> \ No newline at end of file
>>> diff --git a/security/landlock/net.c b/security/landlock/net.c
>>> new file mode 100644
>>> index 000000000000..39e8a156a1f4
>>> --- /dev/null
>>> +++ b/security/landlock/net.c
>>> @@ -0,0 +1,164 @@
>>> +// SPDX-License-Identifier: GPL-2.0-only
>>> +/*
>>> + * Landlock LSM - Network management and hooks
>>> + *
>>> + * Copyright © 2022 Huawei Tech. Co., Ltd.
>>> + * Copyright © 2022 Microsoft Corporation
>>> + */
>>> +
>>> +#include <linux/in.h>
>>> +#include <linux/net.h>
>>> +#include <linux/socket.h>
>>> +#include <net/ipv6.h>
>>> +
>>> +#include "common.h"
>>> +#include "cred.h"
>>> +#include "limits.h"
>>> +#include "net.h"
>>> +#include "ruleset.h"
>>> +
>>> +int landlock_append_net_rule(struct landlock_ruleset *const ruleset,
>>> +			     const u16 port, access_mask_t access_rights)
>>> +{
>>> +	int err;
>>> +	const struct landlock_id id = {
>>> +		.key.data = port,
>>> +		.type = LANDLOCK_KEY_NET_PORT,
>>> +	};
>>> +	BUILD_BUG_ON(sizeof(port) > sizeof(id.key.data));
>>> +
>>> +	/* Transforms relative access rights to absolute ones. */
>>> +	access_rights |= LANDLOCK_MASK_ACCESS_NET &
>>> +			 ~landlock_get_net_access_mask(ruleset, 0);
>>> +
>>> +	mutex_lock(&ruleset->lock);
>>> +	err = landlock_insert_rule(ruleset, id, access_rights);
>>> +	mutex_unlock(&ruleset->lock);
>>> +
>>> +	return err;
>>> +}
>>> +
>>> +static int check_socket_access(const struct landlock_ruleset *const domain,
>>> +			       u16 port, access_mask_t access_request)
>>> +{
>>> +	bool allowed = false;
>>> +	layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_NET] = {};
>>> +	const struct landlock_rule *rule;
>>> +	access_mask_t handled_access;
>>> +	const struct landlock_id id = {
>>> +		.key.data = port,
>>> +		.type = LANDLOCK_KEY_NET_PORT,
>>> +	};
>>> +
>>> +	if (WARN_ON_ONCE(!domain))
>>> +		return 0;
>>> +	if (WARN_ON_ONCE(domain->num_layers < 1))
>>> +		return -EACCES;
>>> +
>>> +	rule = landlock_find_rule(domain, id);
>>> +	handled_access = init_layer_masks(domain, access_request, &layer_masks,
>>> +					  LANDLOCK_KEY_NET_PORT);
>>> +	allowed = unmask_layers(rule, handled_access, &layer_masks,
>>> +				ARRAY_SIZE(layer_masks));
>>> +
>>> +	return allowed ? 0 : -EACCES;
>>> +}
>>> +
>>> +static u16 get_port(const struct sockaddr *const address)
>>
>> get_port() should return a __be16 type. This enables to avoid converting
>> port when checking a rule.
> 
>     In this case a user must do a coverting port into __be16:
> 
>     struct landlock_net_service_attr net_service = {
>                   .allowed_access = LANDLOCK_ACCESS_NET_BIND_TCP,
> 
>                   .port = htons(sock_port),
>           };
>    I think that a user should not think about this conversion cause it
> makes UAPI more complex to use. Lets do this under kernel's hood and let
> it as it is now -> u16 port.
> 
> What do you think?

BE and LE conversions may be error prone without strong typing, but the 
current Linux network UAPI uses this convention (see related syscalls), 
so developers already use htons() in their applications. I think it is 
less hazardous to use the same convention. It would be nice to have the 
point of view of network and API folks though.

Re: [PATCH v8 07/12] landlock: Add network rules support

[Konstantin Meskhidze (A)]

1/6/2023 10:22 PM, Mickaël Salaün пишет:
> 
> On 04/01/2023 12:41, Konstantin Meskhidze (A) wrote:
>> 
>> 
>> 11/17/2022 9:43 PM, Mickaël Salaün пишет:
> 
> [...]
> 
>>>>    /**
>>>> @@ -79,6 +91,24 @@ struct landlock_path_beneath_attr {
>>>>    	 */
>>>>    } __attribute__((packed));
>>>>
>>>> +/**
>>>> + * struct landlock_net_service_attr - TCP subnet definition
>>>> + *
>>>> + * Argument of sys_landlock_add_rule().
>>>> + */
>>>> +struct landlock_net_service_attr {
>>>> +	/**
>>>> +	 * @allowed_access: Bitmask of allowed access network for services
>>>> +	 * (cf. `Network flags`_).
>>>> +	 */
>>>> +	__u64 allowed_access;
>>>> +	/**
>>>> +	 * @port: Network port.
>>>> +	 */
>>>> +	__u16 port;
>>>
>>>    From an UAPI point of view, I think the port field should be __be16, as
>>> for sockaddr_in->port and other network-related APIs. This will require
>>> some kernel changes to please sparse: make C=2 security/landlock/ must
>>> not print any warning.
>> 
>>     I have this errors trying to launch sparse checking:
>> 
>>     DESCEND objtool
>>     DESCEND bpf/resolve_btfids
>>     CALL    scripts/checksyscalls.sh
>>     CHK     kernel/kheaders_data.tar.xz
>>     CC      security/landlock/setup.o
>>     CHECK   security/landlock/setup.c
>> ./include/asm-generic/rwonce.h:67:16: error: typename in expression
>> ./include/asm-generic/rwonce.h:67:16: error: Expected ) in function call
>> ./include/asm-generic/rwonce.h:67:16: error: got :
>> ./include/linux/list.h:292:16: error: typename in expression
>> ./include/linux/list.h:292:16: error: Expected ) in function call
>> ./include/linux/list.h:292:16: error: got :
>> 
>> ....
>> 
>> ./include/linux/seqlock.h:682:16: error: Expected ) in function call
>> ./include/linux/seqlock.h:682:16: error: got :
>> ./include/linux/seqlock.h:695:16: error: typename in expression
>> ./include/linux/seqlock.h:695:16: error: Expected ) in function call
>> ./include/linux/seqlock.h:695:16: error: too many errors
>> Segmentation fault (core dumped)
>> make[3]: *** [scripts/Makefile.build:250: security/landlock/setup.o]
>> Error 139
>> make[3]: *** Deleting file 'security/landlock/setup.o'
>> make[3]: *** Waiting for unfinished jobs....
>> Segmentation fault (core dumped)
>> make[3]: *** [scripts/Makefile.build:250: security/landlock/syscalls.o]
>> Error 139
>> make[3]: *** Deleting file 'security/landlock/syscalls.o'
>> make[2]: *** [scripts/Makefile.build:502: security/landlock] Error 2
>> make[1]: *** [scripts/Makefile.build:502: security] Error 2
>> make[1]: *** Waiting for unfinished jobs....
>> make: *** [Makefile:1994: .] Error 2
> 
> I don't know about this error. Did you follow the documentation?
> https://docs.kernel.org/dev-tools/sparse.html#getting-sparse
> 
   Yes, I did as in the documentation. that's strange.
If you dont mind can you please check it when I sent a new patch?

> 
> 
>>>
>>> Using big-endian values as keys (casted to uintptr_t, not strictly
>>> __be16) in the rb-tree should not be an issue because there is no port
>>> range ordering (for now).
>>>
>>> A dedicated test should check that endianness is correct, e.g. by using
>>> different port encoding. This should include passing and failing tests,
>>> but they should work on all architectures (i.e. big or little endian).
> .

Re: [PATCH v8 08/12] landlock: Implement TCP network hooks

[Konstantin Meskhidze (A)]

1/6/2023 10:30 PM, Mickaël Salaün пишет:
> 
> On 05/01/2023 09:57, Konstantin Meskhidze (A) wrote:
>> 
>> 
>> 11/17/2022 9:43 PM, Mickaël Salaün пишет:
>>>
>>> On 21/10/2022 17:26, Konstantin Meskhidze wrote:
>>>> This patch adds support of socket_bind() and socket_connect() hooks.
>>>> It's possible to restrict binding and connecting of TCP sockets to
>>>> particular ports.
>>>
>>> Implement socket_bind() and socket_connect LSM hooks, which enable to
>>> restrict TCP socket binding and connection to specific ports.
>>>
>>>
>>>>
>>>> Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze@huawei.com>
>>>> ---
>>>>
>>>> Changes since v7:
>>>> * Minor fixes.
>>>> * Refactors commit message.
>>>>
>>>> Changes since v6:
>>>> * Updates copyright.
>>>> * Refactors landlock_append_net_rule() and check_socket_access()
>>>>     functions with landlock_id type.
>>>>
>>>> Changes since v5:
>>>> * Fixes some logic errors.
>>>> * Formats code with clang-format-14.
>>>>
>>>> Changes since v4:
>>>> * Factors out CONFIG_INET into make file.
>>>> * Refactors check_socket_access().
>>>> * Adds helper get_port().
>>>> * Adds CONFIG_IPV6 in get_port(), hook_socket_bind/connect
>>>> functions to support AF_INET6 family.
>>>> * Adds AF_UNSPEC family support in hook_socket_bind/connect
>>>> functions.
>>>> * Refactors add_rule_net_service() and landlock_add_rule
>>>> syscall to support network rule inserting.
>>>> * Refactors init_layer_masks() to support network rules.
>>>>
>>>> Changes since v3:
>>>> * Splits commit.
>>>> * Adds SECURITY_NETWORK in config.
>>>> * Adds IS_ENABLED(CONFIG_INET) if a kernel has no INET configuration.
>>>> * Adds hook_socket_bind and hook_socket_connect hooks.
>>>>
>>>> ---
>>>>    security/landlock/Kconfig    |   1 +
>>>>    security/landlock/Makefile   |   2 +
>>>>    security/landlock/net.c      | 164 +++++++++++++++++++++++++++++++++++
>>>>    security/landlock/net.h      |  26 ++++++
>>>>    security/landlock/setup.c    |   2 +
>>>>    security/landlock/syscalls.c |  59 ++++++++++++-
>>>>    6 files changed, 251 insertions(+), 3 deletions(-)
>>>>    create mode 100644 security/landlock/net.c
>>>>    create mode 100644 security/landlock/net.h
>>>>
>>>> diff --git a/security/landlock/Kconfig b/security/landlock/Kconfig
>>>> index 8e33c4e8ffb8..10c099097533 100644
>>>> --- a/security/landlock/Kconfig
>>>> +++ b/security/landlock/Kconfig
>>>> @@ -3,6 +3,7 @@
>>>>    config SECURITY_LANDLOCK
>>>>    	bool "Landlock support"
>>>>    	depends on SECURITY && !ARCH_EPHEMERAL_INODES
>>>> +	select SECURITY_NETWORK
>>>>    	select SECURITY_PATH
>>>>    	help
>>>>    	  Landlock is a sandboxing mechanism that enables processes to restrict
>>>> diff --git a/security/landlock/Makefile b/security/landlock/Makefile
>>>> index 7bbd2f413b3e..53d3c92ae22e 100644
>>>> --- a/security/landlock/Makefile
>>>> +++ b/security/landlock/Makefile
>>>> @@ -2,3 +2,5 @@ obj-$(CONFIG_SECURITY_LANDLOCK) := landlock.o
>>>>
>>>>    landlock-y := setup.o syscalls.o object.o ruleset.o \
>>>>    	cred.o ptrace.o fs.o
>>>> +
>>>> +landlock-$(CONFIG_INET) += net.o
>>>> \ No newline at end of file
>>>> diff --git a/security/landlock/net.c b/security/landlock/net.c
>>>> new file mode 100644
>>>> index 000000000000..39e8a156a1f4
>>>> --- /dev/null
>>>> +++ b/security/landlock/net.c
>>>> @@ -0,0 +1,164 @@
>>>> +// SPDX-License-Identifier: GPL-2.0-only
>>>> +/*
>>>> + * Landlock LSM - Network management and hooks
>>>> + *
>>>> + * Copyright © 2022 Huawei Tech. Co., Ltd.
>>>> + * Copyright © 2022 Microsoft Corporation
>>>> + */
>>>> +
>>>> +#include <linux/in.h>
>>>> +#include <linux/net.h>
>>>> +#include <linux/socket.h>
>>>> +#include <net/ipv6.h>
>>>> +
>>>> +#include "common.h"
>>>> +#include "cred.h"
>>>> +#include "limits.h"
>>>> +#include "net.h"
>>>> +#include "ruleset.h"
>>>> +
>>>> +int landlock_append_net_rule(struct landlock_ruleset *const ruleset,
>>>> +			     const u16 port, access_mask_t access_rights)
>>>> +{
>>>> +	int err;
>>>> +	const struct landlock_id id = {
>>>> +		.key.data = port,
>>>> +		.type = LANDLOCK_KEY_NET_PORT,
>>>> +	};
>>>> +	BUILD_BUG_ON(sizeof(port) > sizeof(id.key.data));
>>>> +
>>>> +	/* Transforms relative access rights to absolute ones. */
>>>> +	access_rights |= LANDLOCK_MASK_ACCESS_NET &
>>>> +			 ~landlock_get_net_access_mask(ruleset, 0);
>>>> +
>>>> +	mutex_lock(&ruleset->lock);
>>>> +	err = landlock_insert_rule(ruleset, id, access_rights);
>>>> +	mutex_unlock(&ruleset->lock);
>>>> +
>>>> +	return err;
>>>> +}
>>>> +
>>>> +static int check_socket_access(const struct landlock_ruleset *const domain,
>>>> +			       u16 port, access_mask_t access_request)
>>>> +{
>>>> +	bool allowed = false;
>>>> +	layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_NET] = {};
>>>> +	const struct landlock_rule *rule;
>>>> +	access_mask_t handled_access;
>>>> +	const struct landlock_id id = {
>>>> +		.key.data = port,
>>>> +		.type = LANDLOCK_KEY_NET_PORT,
>>>> +	};
>>>> +
>>>> +	if (WARN_ON_ONCE(!domain))
>>>> +		return 0;
>>>> +	if (WARN_ON_ONCE(domain->num_layers < 1))
>>>> +		return -EACCES;
>>>> +
>>>> +	rule = landlock_find_rule(domain, id);
>>>> +	handled_access = init_layer_masks(domain, access_request, &layer_masks,
>>>> +					  LANDLOCK_KEY_NET_PORT);
>>>> +	allowed = unmask_layers(rule, handled_access, &layer_masks,
>>>> +				ARRAY_SIZE(layer_masks));
>>>> +
>>>> +	return allowed ? 0 : -EACCES;
>>>> +}
>>>> +
>>>> +static u16 get_port(const struct sockaddr *const address)
>>>
>>> get_port() should return a __be16 type. This enables to avoid converting
>>> port when checking a rule.
>> 
>>     In this case a user must do a coverting port into __be16:
>> 
>>     struct landlock_net_service_attr net_service = {
>>                   .allowed_access = LANDLOCK_ACCESS_NET_BIND_TCP,
>> 
>>                   .port = htons(sock_port),
>>           };
>>    I think that a user should not think about this conversion cause it
>> makes UAPI more complex to use. Lets do this under kernel's hood and let
>> it as it is now -> u16 port.
>> 
>> What do you think?
> 
> BE and LE conversions may be error prone without strong typing, but the
> current Linux network UAPI uses this convention (see related syscalls),
> so developers already use htons() in their applications. I think it is
> less hazardous to use the same convention. It would be nice to have the
> point of view of network and API folks though.

   Ok. Thanks. Let ports be in BE format like in network packets.

   What should a selftest with port conversion be like?

   1. Set a port with a Landlock rule with no conversion. get an error 
wit bind/connect actions.
   2. Convert a port with htons(sock_port). get no error.

   What do you think?
> .

Re: [PATCH v8 07/12] landlock: Add network rules support

[Dan Carpenter]

These warnings seem like something I have seen before.  Maybe it was an
issue with _Generic() support?

Are you really sure you're running the latest git version of Sparse?

I tested this patch with the latest version of Sparse on my system and
it worked fine.

regards,
dan carpenter

Re: [PATCH v8 07/12] landlock: Add network rules support

[Konstantin Meskhidze (A)]

1/9/2023 11:58 AM, Dan Carpenter пишет:
> These warnings seem like something I have seen before.  Maybe it was an
> issue with _Generic() support?
> 
> Are you really sure you're running the latest git version of Sparse?
> 
> I tested this patch with the latest version of Sparse on my system and
> it worked fine.

  Hi Dan,

  git is on the master branch now - hash ce1a6720 (dated 27 June 2022)

  Is this correct version?

  regards,
  Konstantin.
> 
> regards,
> dan carpenter
> 
> .

Re: [PATCH v8 07/12] landlock: Add network rules support

[Dan Carpenter]

On Mon, Jan 09, 2023 at 12:26:52PM +0300, Konstantin Meskhidze (A) wrote:
> 
> 
> 1/9/2023 11:58 AM, Dan Carpenter пишет:
> > These warnings seem like something I have seen before.  Maybe it was an
> > issue with _Generic() support?
> > 
> > Are you really sure you're running the latest git version of Sparse?
> > 
> > I tested this patch with the latest version of Sparse on my system and
> > it worked fine.
> 
>  Hi Dan,
> 
>  git is on the master branch now - hash ce1a6720 (dated 27 June 2022)
> 
>  Is this correct version?

Yes, that's correct.  What is your .config?

regards,
dan carpenter

Re: [PATCH v8 07/12] landlock: Add network rules support

[Konstantin Meskhidze (A)]

1/9/2023 1:20 PM, Dan Carpenter пишет:
> On Mon, Jan 09, 2023 at 12:26:52PM +0300, Konstantin Meskhidze (A) wrote:
>> 
>> 
>> 1/9/2023 11:58 AM, Dan Carpenter пишет:
>> > These warnings seem like something I have seen before.  Maybe it was an
>> > issue with _Generic() support?
>> > 
>> > Are you really sure you're running the latest git version of Sparse?
>> > 
>> > I tested this patch with the latest version of Sparse on my system and
>> > it worked fine.
>> 
>>  Hi Dan,
>> 
>>  git is on the master branch now - hash ce1a6720 (dated 27 June 2022)
>> 
>>  Is this correct version?
> 
> Yes, that's correct.  What is your .config?

   What parameters do I need to check in .config?
> 
> regards,
> dan carpenter
> 
> .

Re: [PATCH v8 07/12] landlock: Add network rules support

[Dan Carpenter]

On Mon, Jan 09, 2023 at 02:39:36PM +0300, Konstantin Meskhidze (A) wrote:
> 
> 
> 1/9/2023 1:20 PM, Dan Carpenter пишет:
> > On Mon, Jan 09, 2023 at 12:26:52PM +0300, Konstantin Meskhidze (A) wrote:
> > > 
> > > 
> > > 1/9/2023 11:58 AM, Dan Carpenter пишет:
> > > > These warnings seem like something I have seen before.  Maybe it was an
> > > > issue with _Generic() support?
> > > > > Are you really sure you're running the latest git version of
> > > Sparse?
> > > > > I tested this patch with the latest version of Sparse on my
> > > system and
> > > > it worked fine.
> > > 
> > >  Hi Dan,
> > > 
> > >  git is on the master branch now - hash ce1a6720 (dated 27 June 2022)
> > > 
> > >  Is this correct version?
> > 
> > Yes, that's correct.  What is your .config?
> 
>   What parameters do I need to check in .config?

I don't know.  I was hoping you could just email me the whole thing
and/or the results from make security/landlock/ruleset.i.  That way
we could see what line was making Sparse complain.

regards,
dan carpenter

Re: [PATCH v8 07/12] landlock: Add network rules support

[Konstantin Meskhidze (A)]

1/9/2023 2:53 PM, Dan Carpenter пишет:
> On Mon, Jan 09, 2023 at 02:39:36PM +0300, Konstantin Meskhidze (A) wrote:
>> 
>> 
>> 1/9/2023 1:20 PM, Dan Carpenter пишет:
>> > On Mon, Jan 09, 2023 at 12:26:52PM +0300, Konstantin Meskhidze (A) wrote:
>> > > 
>> > > 
>> > > 1/9/2023 11:58 AM, Dan Carpenter пишет:
>> > > > These warnings seem like something I have seen before.  Maybe it was an
>> > > > issue with _Generic() support?
>> > > > > Are you really sure you're running the latest git version of
>> > > Sparse?
>> > > > > I tested this patch with the latest version of Sparse on my
>> > > system and
>> > > > it worked fine.
>> > > 
>> > >  Hi Dan,
>> > > 
>> > >  git is on the master branch now - hash ce1a6720 (dated 27 June 2022)
>> > > 
>> > >  Is this correct version?
>> > 
>> > Yes, that's correct.  What is your .config?
>> 
>>   What parameters do I need to check in .config?
> 
> I don't know.  I was hoping you could just email me the whole thing
> and/or the results from make security/landlock/ruleset.i.  That way
> we could see what line was making Sparse complain.

   here is the whole error message:

   make C=2 security/landlock/
   CHECK   scripts/mod/empty.c
   CALL    scripts/checksyscalls.sh
   DESCEND objtool
   DESCEND bpf/resolve_btfids
   CHECK   security/landlock/setup.c
./include/asm-generic/rwonce.h:67:16: error: typename in expression
./include/asm-generic/rwonce.h:67:16: error: Expected ) in function call
./include/asm-generic/rwonce.h:67:16: error: got :
./include/linux/list.h:292:16: error: typename in expression
./include/linux/list.h:292:16: error: Expected ) in function call
./include/linux/list.h:292:16: error: got :
./include/linux/list.h:328:34: error: typename in expression
./include/linux/list.h:328:34: error: Expected ) in function call
./include/linux/list.h:328:34: error: got :
./include/linux/list.h:329:53: error: typename in expression
./include/linux/list.h:329:53: error: Expected ) in function call
./include/linux/list.h:329:53: error: got :
./include/linux/list.h:867:17: error: typename in expression
./include/linux/list.h:867:17: error: Expected ) in function call
./include/linux/list.h:867:17: error: got :
./include/linux/list.h:876:17: error: typename in expression
./include/linux/list.h:876:17: error: Expected ) in function call
./include/linux/list.h:876:17: error: got :
./arch/x86/include/asm/atomic.h:29:16: error: typename in expression
./arch/x86/include/asm/atomic.h:29:16: error: Expected ) in function call
./arch/x86/include/asm/atomic.h:29:16: error: got :
./arch/x86/include/asm/atomic64_64.h:22:16: error: typename in expression
./arch/x86/include/asm/atomic64_64.h:22:16: error: Expected ) in 
function call
./arch/x86/include/asm/atomic64_64.h:22:16: error: got :
./include/linux/atomic/atomic-arch-fallback.h:227:23: error: typename in 
expression
./include/linux/atomic/atomic-arch-fallback.h:227:23: error: Expected ) 
in function call
./include/linux/atomic/atomic-arch-fallback.h:227:23: error: got :
./include/linux/atomic/atomic-arch-fallback.h:1348:23: error: typename 
in expression
./include/linux/atomic/atomic-arch-fallback.h:1348:23: error: Expected ) 
in function call
./include/linux/atomic/atomic-arch-fallback.h:1348:23: error: got :
./include/linux/jump_label.h:286:9: error: Expected ; at end of statement
./include/linux/jump_label.h:286:9: error: got __flags
./include/linux/jump_label.h:302:9: error: Expected ; at end of statement
./include/linux/jump_label.h:302:9: error: got __flags
./include/linux/jump_label.h:319:9: error: Expected ; at end of statement
./include/linux/jump_label.h:319:9: error: got __flags
./include/linux/jump_label.h:322:17: error: Expected ; at end of statement
./include/linux/jump_label.h:322:17: error: got __flags
./include/linux/jump_label.h:330:9: error: Expected ; at end of statement
./include/linux/jump_label.h:330:9: error: got __flags
./include/linux/jump_label.h:333:17: error: Expected ; at end of statement
./include/linux/jump_label.h:333:17: error: got __flags
./include/asm-generic/bitops/generic-non-atomic.h:140:23: error: 
typename in expression
./include/asm-generic/bitops/generic-non-atomic.h:140:23: error: 
Expected ) in function call
./include/asm-generic/bitops/generic-non-atomic.h:140:23: error: got :
./include/linux/bitmap.h:268:17: error: Expected ; at end of statement
./include/linux/bitmap.h:268:17: error: got __flags
./include/linux/thread_info.h:127:16: error: typename in expression
./include/linux/thread_info.h:127:16: error: Expected ) in function call
./include/linux/thread_info.h:127:16: error: got :
./include/linux/thread_info.h:233:13: error: Expected ; at end of statement
./include/linux/thread_info.h:233:13: error: got __flags
./include/linux/llist.h:191:16: error: typename in expression
./include/linux/llist.h:191:16: error: Expected ) in function call
./include/linux/llist.h:191:16: error: got :
./include/linux/rcupdate.h:1073:31: error: typename in expression
./include/linux/rcupdate.h:1073:31: error: Expected ) in function call
./include/linux/rcupdate.h:1073:31: error: got :
./include/linux/rcupdate.h:1077:9: error: Expected ; at end of statement
./include/linux/rcupdate.h:1077:9: error: got __flags
./include/linux/key.h:453:16: error: typename in expression
./include/linux/key.h:453:16: error: Expected ) in function call
./include/linux/key.h:453:16: error: got :
./include/linux/list_bl.h:74:33: error: typename in expression
./include/linux/list_bl.h:74:33: error: Expected ) in function call
./include/linux/list_bl.h:74:33: error: got :
./include/linux/rculist_bl.h:24:33: error: typename in expression
./include/linux/rculist_bl.h:24:33: error: Expected ) in function call
./include/linux/rculist_bl.h:24:33: error: got :
./include/linux/seqlock.h:259:16: error: typename in expression
./include/linux/seqlock.h:259:16: error: Expected ) in function call
./include/linux/seqlock.h:259:16: error: got :
./include/linux/seqlock.h:274:1: error: typename in expression
./include/linux/seqlock.h:274:1: error: Expected ) in function call
./include/linux/seqlock.h:274:1: error: got :
./include/linux/seqlock.h:274:1: error: typename in expression
./include/linux/seqlock.h:274:1: error: Expected ) in function call
./include/linux/seqlock.h:274:1: error: got :
./include/linux/seqlock.h:275:1: error: typename in expression
./include/linux/seqlock.h:275:1: error: Expected ) in function call
./include/linux/seqlock.h:275:1: error: got :
./include/linux/seqlock.h:275:1: error: typename in expression
./include/linux/seqlock.h:275:1: error: Expected ) in function call
./include/linux/seqlock.h:275:1: error: got :
./include/linux/seqlock.h:276:1: error: typename in expression
./include/linux/seqlock.h:276:1: error: Expected ) in function call
./include/linux/seqlock.h:276:1: error: got :
./include/linux/seqlock.h:276:1: error: typename in expression
./include/linux/seqlock.h:276:1: error: Expected ) in function call
./include/linux/seqlock.h:276:1: error: got :
./include/linux/seqlock.h:277:1: error: typename in expression
./include/linux/seqlock.h:277:1: error: Expected ) in function call
./include/linux/seqlock.h:277:1: error: got :
./include/linux/seqlock.h:277:1: error: typename in expression
./include/linux/seqlock.h:277:1: error: Expected ) in function call
./include/linux/seqlock.h:277:1: error: got :
./include/linux/seqlock.h:429:16: error: typename in expression
./include/linux/seqlock.h:429:16: error: Expected ) in function call
./include/linux/seqlock.h:429:16: error: got :
./include/linux/seqlock.h:682:16: error: typename in expression
./include/linux/seqlock.h:682:16: error: Expected ) in function call
./include/linux/seqlock.h:682:16: error: too many errors
Segmentation fault (core dumped)
make[3]: *** [scripts/Makefile.build:251: security/landlock/setup.o] 
Error 139
make[2]: *** [scripts/Makefile.build:502: security/landlock] Error 2
make[1]: *** [scripts/Makefile.build:502: security] Error 2
make: *** [Makefile:1994: .] Error 2

Please tell me if you need some more info.

regards,
Konstantin

> 
> regards,
> dan carpenter
> 
> .

Re: [PATCH v8 08/12] landlock: Implement TCP network hooks

[Mickaël Salaün]

On 09/01/2023 09:07, Konstantin Meskhidze (A) wrote:
> 
> 
> 1/6/2023 10:30 PM, Mickaël Salaün пишет:
>>
>> On 05/01/2023 09:57, Konstantin Meskhidze (A) wrote:
>>>
>>>
>>> 11/17/2022 9:43 PM, Mickaël Salaün пишет:
>>>>
>>>> On 21/10/2022 17:26, Konstantin Meskhidze wrote:
>>>>> This patch adds support of socket_bind() and socket_connect() hooks.
>>>>> It's possible to restrict binding and connecting of TCP sockets to
>>>>> particular ports.
>>>>
>>>> Implement socket_bind() and socket_connect LSM hooks, which enable to
>>>> restrict TCP socket binding and connection to specific ports.
>>>>
>>>>
>>>>>
>>>>> Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze@huawei.com>
>>>>> ---
>>>>>
>>>>> Changes since v7:
>>>>> * Minor fixes.
>>>>> * Refactors commit message.
>>>>>
>>>>> Changes since v6:
>>>>> * Updates copyright.
>>>>> * Refactors landlock_append_net_rule() and check_socket_access()
>>>>>      functions with landlock_id type.
>>>>>
>>>>> Changes since v5:
>>>>> * Fixes some logic errors.
>>>>> * Formats code with clang-format-14.
>>>>>
>>>>> Changes since v4:
>>>>> * Factors out CONFIG_INET into make file.
>>>>> * Refactors check_socket_access().
>>>>> * Adds helper get_port().
>>>>> * Adds CONFIG_IPV6 in get_port(), hook_socket_bind/connect
>>>>> functions to support AF_INET6 family.
>>>>> * Adds AF_UNSPEC family support in hook_socket_bind/connect
>>>>> functions.
>>>>> * Refactors add_rule_net_service() and landlock_add_rule
>>>>> syscall to support network rule inserting.
>>>>> * Refactors init_layer_masks() to support network rules.
>>>>>
>>>>> Changes since v3:
>>>>> * Splits commit.
>>>>> * Adds SECURITY_NETWORK in config.
>>>>> * Adds IS_ENABLED(CONFIG_INET) if a kernel has no INET configuration.
>>>>> * Adds hook_socket_bind and hook_socket_connect hooks.
>>>>>
>>>>> ---
>>>>>     security/landlock/Kconfig    |   1 +
>>>>>     security/landlock/Makefile   |   2 +
>>>>>     security/landlock/net.c      | 164 +++++++++++++++++++++++++++++++++++
>>>>>     security/landlock/net.h      |  26 ++++++
>>>>>     security/landlock/setup.c    |   2 +
>>>>>     security/landlock/syscalls.c |  59 ++++++++++++-
>>>>>     6 files changed, 251 insertions(+), 3 deletions(-)
>>>>>     create mode 100644 security/landlock/net.c
>>>>>     create mode 100644 security/landlock/net.h
>>>>>
>>>>> diff --git a/security/landlock/Kconfig b/security/landlock/Kconfig
>>>>> index 8e33c4e8ffb8..10c099097533 100644
>>>>> --- a/security/landlock/Kconfig
>>>>> +++ b/security/landlock/Kconfig
>>>>> @@ -3,6 +3,7 @@
>>>>>     config SECURITY_LANDLOCK
>>>>>     	bool "Landlock support"
>>>>>     	depends on SECURITY && !ARCH_EPHEMERAL_INODES
>>>>> +	select SECURITY_NETWORK
>>>>>     	select SECURITY_PATH
>>>>>     	help
>>>>>     	  Landlock is a sandboxing mechanism that enables processes to restrict
>>>>> diff --git a/security/landlock/Makefile b/security/landlock/Makefile
>>>>> index 7bbd2f413b3e..53d3c92ae22e 100644
>>>>> --- a/security/landlock/Makefile
>>>>> +++ b/security/landlock/Makefile
>>>>> @@ -2,3 +2,5 @@ obj-$(CONFIG_SECURITY_LANDLOCK) := landlock.o
>>>>>
>>>>>     landlock-y := setup.o syscalls.o object.o ruleset.o \
>>>>>     	cred.o ptrace.o fs.o
>>>>> +
>>>>> +landlock-$(CONFIG_INET) += net.o
>>>>> \ No newline at end of file
>>>>> diff --git a/security/landlock/net.c b/security/landlock/net.c
>>>>> new file mode 100644
>>>>> index 000000000000..39e8a156a1f4
>>>>> --- /dev/null
>>>>> +++ b/security/landlock/net.c
>>>>> @@ -0,0 +1,164 @@
>>>>> +// SPDX-License-Identifier: GPL-2.0-only
>>>>> +/*
>>>>> + * Landlock LSM - Network management and hooks
>>>>> + *
>>>>> + * Copyright © 2022 Huawei Tech. Co., Ltd.
>>>>> + * Copyright © 2022 Microsoft Corporation
>>>>> + */
>>>>> +
>>>>> +#include <linux/in.h>
>>>>> +#include <linux/net.h>
>>>>> +#include <linux/socket.h>
>>>>> +#include <net/ipv6.h>
>>>>> +
>>>>> +#include "common.h"
>>>>> +#include "cred.h"
>>>>> +#include "limits.h"
>>>>> +#include "net.h"
>>>>> +#include "ruleset.h"
>>>>> +
>>>>> +int landlock_append_net_rule(struct landlock_ruleset *const ruleset,
>>>>> +			     const u16 port, access_mask_t access_rights)
>>>>> +{
>>>>> +	int err;
>>>>> +	const struct landlock_id id = {
>>>>> +		.key.data = port,
>>>>> +		.type = LANDLOCK_KEY_NET_PORT,
>>>>> +	};
>>>>> +	BUILD_BUG_ON(sizeof(port) > sizeof(id.key.data));
>>>>> +
>>>>> +	/* Transforms relative access rights to absolute ones. */
>>>>> +	access_rights |= LANDLOCK_MASK_ACCESS_NET &
>>>>> +			 ~landlock_get_net_access_mask(ruleset, 0);
>>>>> +
>>>>> +	mutex_lock(&ruleset->lock);
>>>>> +	err = landlock_insert_rule(ruleset, id, access_rights);
>>>>> +	mutex_unlock(&ruleset->lock);
>>>>> +
>>>>> +	return err;
>>>>> +}
>>>>> +
>>>>> +static int check_socket_access(const struct landlock_ruleset *const domain,
>>>>> +			       u16 port, access_mask_t access_request)
>>>>> +{
>>>>> +	bool allowed = false;
>>>>> +	layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_NET] = {};
>>>>> +	const struct landlock_rule *rule;
>>>>> +	access_mask_t handled_access;
>>>>> +	const struct landlock_id id = {
>>>>> +		.key.data = port,
>>>>> +		.type = LANDLOCK_KEY_NET_PORT,
>>>>> +	};
>>>>> +
>>>>> +	if (WARN_ON_ONCE(!domain))
>>>>> +		return 0;
>>>>> +	if (WARN_ON_ONCE(domain->num_layers < 1))
>>>>> +		return -EACCES;
>>>>> +
>>>>> +	rule = landlock_find_rule(domain, id);
>>>>> +	handled_access = init_layer_masks(domain, access_request, &layer_masks,
>>>>> +					  LANDLOCK_KEY_NET_PORT);
>>>>> +	allowed = unmask_layers(rule, handled_access, &layer_masks,
>>>>> +				ARRAY_SIZE(layer_masks));
>>>>> +
>>>>> +	return allowed ? 0 : -EACCES;
>>>>> +}
>>>>> +
>>>>> +static u16 get_port(const struct sockaddr *const address)
>>>>
>>>> get_port() should return a __be16 type. This enables to avoid converting
>>>> port when checking a rule.
>>>
>>>      In this case a user must do a coverting port into __be16:
>>>
>>>      struct landlock_net_service_attr net_service = {
>>>                    .allowed_access = LANDLOCK_ACCESS_NET_BIND_TCP,
>>>
>>>                    .port = htons(sock_port),
>>>            };
>>>     I think that a user should not think about this conversion cause it
>>> makes UAPI more complex to use. Lets do this under kernel's hood and let
>>> it as it is now -> u16 port.
>>>
>>> What do you think?
>>
>> BE and LE conversions may be error prone without strong typing, but the
>> current Linux network UAPI uses this convention (see related syscalls),
>> so developers already use htons() in their applications. I think it is
>> less hazardous to use the same convention. It would be nice to have the
>> point of view of network and API folks though.
> 
>     Ok. Thanks. Let ports be in BE format like in network packets.
> 
>     What should a selftest with port conversion be like?
> 
>     1. Set a port with a Landlock rule with no conversion. get an error
> wit bind/connect actions.
>     2. Convert a port with htons(sock_port). get no error.
> 
>     What do you think?

Right, you can do both on a LE architecture (that must be checked in the 
test or it should be skipped), test with a port value that has different 
representation in LE and BE.

Re: [PATCH v8 08/12] landlock: Implement TCP network hooks

[Konstantin Meskhidze (A)]

1/9/2023 3:38 PM, Mickaël Salaün пишет:
> 
> On 09/01/2023 09:07, Konstantin Meskhidze (A) wrote:
>> 
>> 
>> 1/6/2023 10:30 PM, Mickaël Salaün пишет:
>>>
>>> On 05/01/2023 09:57, Konstantin Meskhidze (A) wrote:
>>>>
>>>>
>>>> 11/17/2022 9:43 PM, Mickaël Salaün пишет:
>>>>>
>>>>> On 21/10/2022 17:26, Konstantin Meskhidze wrote:
>>>>>> This patch adds support of socket_bind() and socket_connect() hooks.
>>>>>> It's possible to restrict binding and connecting of TCP sockets to
>>>>>> particular ports.
>>>>>
>>>>> Implement socket_bind() and socket_connect LSM hooks, which enable to
>>>>> restrict TCP socket binding and connection to specific ports.
>>>>>
>>>>>
>>>>>>
>>>>>> Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze@huawei.com>
>>>>>> ---
>>>>>>
>>>>>> Changes since v7:
>>>>>> * Minor fixes.
>>>>>> * Refactors commit message.
>>>>>>
>>>>>> Changes since v6:
>>>>>> * Updates copyright.
>>>>>> * Refactors landlock_append_net_rule() and check_socket_access()
>>>>>>      functions with landlock_id type.
>>>>>>
>>>>>> Changes since v5:
>>>>>> * Fixes some logic errors.
>>>>>> * Formats code with clang-format-14.
>>>>>>
>>>>>> Changes since v4:
>>>>>> * Factors out CONFIG_INET into make file.
>>>>>> * Refactors check_socket_access().
>>>>>> * Adds helper get_port().
>>>>>> * Adds CONFIG_IPV6 in get_port(), hook_socket_bind/connect
>>>>>> functions to support AF_INET6 family.
>>>>>> * Adds AF_UNSPEC family support in hook_socket_bind/connect
>>>>>> functions.
>>>>>> * Refactors add_rule_net_service() and landlock_add_rule
>>>>>> syscall to support network rule inserting.
>>>>>> * Refactors init_layer_masks() to support network rules.
>>>>>>
>>>>>> Changes since v3:
>>>>>> * Splits commit.
>>>>>> * Adds SECURITY_NETWORK in config.
>>>>>> * Adds IS_ENABLED(CONFIG_INET) if a kernel has no INET configuration.
>>>>>> * Adds hook_socket_bind and hook_socket_connect hooks.
>>>>>>
>>>>>> ---
>>>>>>     security/landlock/Kconfig    |   1 +
>>>>>>     security/landlock/Makefile   |   2 +
>>>>>>     security/landlock/net.c      | 164 +++++++++++++++++++++++++++++++++++
>>>>>>     security/landlock/net.h      |  26 ++++++
>>>>>>     security/landlock/setup.c    |   2 +
>>>>>>     security/landlock/syscalls.c |  59 ++++++++++++-
>>>>>>     6 files changed, 251 insertions(+), 3 deletions(-)
>>>>>>     create mode 100644 security/landlock/net.c
>>>>>>     create mode 100644 security/landlock/net.h
>>>>>>
>>>>>> diff --git a/security/landlock/Kconfig b/security/landlock/Kconfig
>>>>>> index 8e33c4e8ffb8..10c099097533 100644
>>>>>> --- a/security/landlock/Kconfig
>>>>>> +++ b/security/landlock/Kconfig
>>>>>> @@ -3,6 +3,7 @@
>>>>>>     config SECURITY_LANDLOCK
>>>>>>     	bool "Landlock support"
>>>>>>     	depends on SECURITY && !ARCH_EPHEMERAL_INODES
>>>>>> +	select SECURITY_NETWORK
>>>>>>     	select SECURITY_PATH
>>>>>>     	help
>>>>>>     	  Landlock is a sandboxing mechanism that enables processes to restrict
>>>>>> diff --git a/security/landlock/Makefile b/security/landlock/Makefile
>>>>>> index 7bbd2f413b3e..53d3c92ae22e 100644
>>>>>> --- a/security/landlock/Makefile
>>>>>> +++ b/security/landlock/Makefile
>>>>>> @@ -2,3 +2,5 @@ obj-$(CONFIG_SECURITY_LANDLOCK) := landlock.o
>>>>>>
>>>>>>     landlock-y := setup.o syscalls.o object.o ruleset.o \
>>>>>>     	cred.o ptrace.o fs.o
>>>>>> +
>>>>>> +landlock-$(CONFIG_INET) += net.o
>>>>>> \ No newline at end of file
>>>>>> diff --git a/security/landlock/net.c b/security/landlock/net.c
>>>>>> new file mode 100644
>>>>>> index 000000000000..39e8a156a1f4
>>>>>> --- /dev/null
>>>>>> +++ b/security/landlock/net.c
>>>>>> @@ -0,0 +1,164 @@
>>>>>> +// SPDX-License-Identifier: GPL-2.0-only
>>>>>> +/*
>>>>>> + * Landlock LSM - Network management and hooks
>>>>>> + *
>>>>>> + * Copyright © 2022 Huawei Tech. Co., Ltd.
>>>>>> + * Copyright © 2022 Microsoft Corporation
>>>>>> + */
>>>>>> +
>>>>>> +#include <linux/in.h>
>>>>>> +#include <linux/net.h>
>>>>>> +#include <linux/socket.h>
>>>>>> +#include <net/ipv6.h>
>>>>>> +
>>>>>> +#include "common.h"
>>>>>> +#include "cred.h"
>>>>>> +#include "limits.h"
>>>>>> +#include "net.h"
>>>>>> +#include "ruleset.h"
>>>>>> +
>>>>>> +int landlock_append_net_rule(struct landlock_ruleset *const ruleset,
>>>>>> +			     const u16 port, access_mask_t access_rights)
>>>>>> +{
>>>>>> +	int err;
>>>>>> +	const struct landlock_id id = {
>>>>>> +		.key.data = port,
>>>>>> +		.type = LANDLOCK_KEY_NET_PORT,
>>>>>> +	};
>>>>>> +	BUILD_BUG_ON(sizeof(port) > sizeof(id.key.data));
>>>>>> +
>>>>>> +	/* Transforms relative access rights to absolute ones. */
>>>>>> +	access_rights |= LANDLOCK_MASK_ACCESS_NET &
>>>>>> +			 ~landlock_get_net_access_mask(ruleset, 0);
>>>>>> +
>>>>>> +	mutex_lock(&ruleset->lock);
>>>>>> +	err = landlock_insert_rule(ruleset, id, access_rights);
>>>>>> +	mutex_unlock(&ruleset->lock);
>>>>>> +
>>>>>> +	return err;
>>>>>> +}
>>>>>> +
>>>>>> +static int check_socket_access(const struct landlock_ruleset *const domain,
>>>>>> +			       u16 port, access_mask_t access_request)
>>>>>> +{
>>>>>> +	bool allowed = false;
>>>>>> +	layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_NET] = {};
>>>>>> +	const struct landlock_rule *rule;
>>>>>> +	access_mask_t handled_access;
>>>>>> +	const struct landlock_id id = {
>>>>>> +		.key.data = port,
>>>>>> +		.type = LANDLOCK_KEY_NET_PORT,
>>>>>> +	};
>>>>>> +
>>>>>> +	if (WARN_ON_ONCE(!domain))
>>>>>> +		return 0;
>>>>>> +	if (WARN_ON_ONCE(domain->num_layers < 1))
>>>>>> +		return -EACCES;
>>>>>> +
>>>>>> +	rule = landlock_find_rule(domain, id);
>>>>>> +	handled_access = init_layer_masks(domain, access_request, &layer_masks,
>>>>>> +					  LANDLOCK_KEY_NET_PORT);
>>>>>> +	allowed = unmask_layers(rule, handled_access, &layer_masks,
>>>>>> +				ARRAY_SIZE(layer_masks));
>>>>>> +
>>>>>> +	return allowed ? 0 : -EACCES;
>>>>>> +}
>>>>>> +
>>>>>> +static u16 get_port(const struct sockaddr *const address)
>>>>>
>>>>> get_port() should return a __be16 type. This enables to avoid converting
>>>>> port when checking a rule.
>>>>
>>>>      In this case a user must do a coverting port into __be16:
>>>>
>>>>      struct landlock_net_service_attr net_service = {
>>>>                    .allowed_access = LANDLOCK_ACCESS_NET_BIND_TCP,
>>>>
>>>>                    .port = htons(sock_port),
>>>>            };
>>>>     I think that a user should not think about this conversion cause it
>>>> makes UAPI more complex to use. Lets do this under kernel's hood and let
>>>> it as it is now -> u16 port.
>>>>
>>>> What do you think?
>>>
>>> BE and LE conversions may be error prone without strong typing, but the
>>> current Linux network UAPI uses this convention (see related syscalls),
>>> so developers already use htons() in their applications. I think it is
>>> less hazardous to use the same convention. It would be nice to have the
>>> point of view of network and API folks though.
>> 
>>     Ok. Thanks. Let ports be in BE format like in network packets.
>> 
>>     What should a selftest with port conversion be like?
>> 
>>     1. Set a port with a Landlock rule with no conversion. get an error
>> wit bind/connect actions.
>>     2. Convert a port with htons(sock_port). get no error.
>> 
>>     What do you think?
> 
> Right, you can do both on a LE architecture (that must be checked in the
> test or it should be skipped), test with a port value that has different
> representation in LE and BE.

   Do you mean to check architecture in a test first and then port 
representaton? What about BE architectures? My current VM is X86-64 
architecture a LE one. I can test just it now.
> .

Re: [PATCH v8 08/12] landlock: Implement TCP network hooks

[Mickaël Salaün]

On 10/01/2023 05:45, Konstantin Meskhidze (A) wrote:
> 
> 
> 1/9/2023 3:38 PM, Mickaël Salaün пишет:
>>
>> On 09/01/2023 09:07, Konstantin Meskhidze (A) wrote:
>>>
>>>
>>> 1/6/2023 10:30 PM, Mickaël Salaün пишет:
>>>>
>>>> On 05/01/2023 09:57, Konstantin Meskhidze (A) wrote:
>>>>>
>>>>>
>>>>> 11/17/2022 9:43 PM, Mickaël Salaün пишет:
>>>>>>
>>>>>> On 21/10/2022 17:26, Konstantin Meskhidze wrote:
>>>>>>> This patch adds support of socket_bind() and socket_connect() hooks.
>>>>>>> It's possible to restrict binding and connecting of TCP sockets to
>>>>>>> particular ports.
>>>>>>
>>>>>> Implement socket_bind() and socket_connect LSM hooks, which enable to
>>>>>> restrict TCP socket binding and connection to specific ports.
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze@huawei.com>
>>>>>>> ---
>>>>>>>
>>>>>>> Changes since v7:
>>>>>>> * Minor fixes.
>>>>>>> * Refactors commit message.
>>>>>>>
>>>>>>> Changes since v6:
>>>>>>> * Updates copyright.
>>>>>>> * Refactors landlock_append_net_rule() and check_socket_access()
>>>>>>>       functions with landlock_id type.
>>>>>>>
>>>>>>> Changes since v5:
>>>>>>> * Fixes some logic errors.
>>>>>>> * Formats code with clang-format-14.
>>>>>>>
>>>>>>> Changes since v4:
>>>>>>> * Factors out CONFIG_INET into make file.
>>>>>>> * Refactors check_socket_access().
>>>>>>> * Adds helper get_port().
>>>>>>> * Adds CONFIG_IPV6 in get_port(), hook_socket_bind/connect
>>>>>>> functions to support AF_INET6 family.
>>>>>>> * Adds AF_UNSPEC family support in hook_socket_bind/connect
>>>>>>> functions.
>>>>>>> * Refactors add_rule_net_service() and landlock_add_rule
>>>>>>> syscall to support network rule inserting.
>>>>>>> * Refactors init_layer_masks() to support network rules.
>>>>>>>
>>>>>>> Changes since v3:
>>>>>>> * Splits commit.
>>>>>>> * Adds SECURITY_NETWORK in config.
>>>>>>> * Adds IS_ENABLED(CONFIG_INET) if a kernel has no INET configuration.
>>>>>>> * Adds hook_socket_bind and hook_socket_connect hooks.
>>>>>>>
>>>>>>> ---
>>>>>>>      security/landlock/Kconfig    |   1 +
>>>>>>>      security/landlock/Makefile   |   2 +
>>>>>>>      security/landlock/net.c      | 164 +++++++++++++++++++++++++++++++++++
>>>>>>>      security/landlock/net.h      |  26 ++++++
>>>>>>>      security/landlock/setup.c    |   2 +
>>>>>>>      security/landlock/syscalls.c |  59 ++++++++++++-
>>>>>>>      6 files changed, 251 insertions(+), 3 deletions(-)
>>>>>>>      create mode 100644 security/landlock/net.c
>>>>>>>      create mode 100644 security/landlock/net.h
>>>>>>>
>>>>>>> diff --git a/security/landlock/Kconfig b/security/landlock/Kconfig
>>>>>>> index 8e33c4e8ffb8..10c099097533 100644
>>>>>>> --- a/security/landlock/Kconfig
>>>>>>> +++ b/security/landlock/Kconfig
>>>>>>> @@ -3,6 +3,7 @@
>>>>>>>      config SECURITY_LANDLOCK
>>>>>>>      	bool "Landlock support"
>>>>>>>      	depends on SECURITY && !ARCH_EPHEMERAL_INODES
>>>>>>> +	select SECURITY_NETWORK
>>>>>>>      	select SECURITY_PATH
>>>>>>>      	help
>>>>>>>      	  Landlock is a sandboxing mechanism that enables processes to restrict
>>>>>>> diff --git a/security/landlock/Makefile b/security/landlock/Makefile
>>>>>>> index 7bbd2f413b3e..53d3c92ae22e 100644
>>>>>>> --- a/security/landlock/Makefile
>>>>>>> +++ b/security/landlock/Makefile
>>>>>>> @@ -2,3 +2,5 @@ obj-$(CONFIG_SECURITY_LANDLOCK) := landlock.o
>>>>>>>
>>>>>>>      landlock-y := setup.o syscalls.o object.o ruleset.o \
>>>>>>>      	cred.o ptrace.o fs.o
>>>>>>> +
>>>>>>> +landlock-$(CONFIG_INET) += net.o
>>>>>>> \ No newline at end of file
>>>>>>> diff --git a/security/landlock/net.c b/security/landlock/net.c
>>>>>>> new file mode 100644
>>>>>>> index 000000000000..39e8a156a1f4
>>>>>>> --- /dev/null
>>>>>>> +++ b/security/landlock/net.c
>>>>>>> @@ -0,0 +1,164 @@
>>>>>>> +// SPDX-License-Identifier: GPL-2.0-only
>>>>>>> +/*
>>>>>>> + * Landlock LSM - Network management and hooks
>>>>>>> + *
>>>>>>> + * Copyright © 2022 Huawei Tech. Co., Ltd.
>>>>>>> + * Copyright © 2022 Microsoft Corporation
>>>>>>> + */
>>>>>>> +
>>>>>>> +#include <linux/in.h>
>>>>>>> +#include <linux/net.h>
>>>>>>> +#include <linux/socket.h>
>>>>>>> +#include <net/ipv6.h>
>>>>>>> +
>>>>>>> +#include "common.h"
>>>>>>> +#include "cred.h"
>>>>>>> +#include "limits.h"
>>>>>>> +#include "net.h"
>>>>>>> +#include "ruleset.h"
>>>>>>> +
>>>>>>> +int landlock_append_net_rule(struct landlock_ruleset *const ruleset,
>>>>>>> +			     const u16 port, access_mask_t access_rights)
>>>>>>> +{
>>>>>>> +	int err;
>>>>>>> +	const struct landlock_id id = {
>>>>>>> +		.key.data = port,
>>>>>>> +		.type = LANDLOCK_KEY_NET_PORT,
>>>>>>> +	};
>>>>>>> +	BUILD_BUG_ON(sizeof(port) > sizeof(id.key.data));
>>>>>>> +
>>>>>>> +	/* Transforms relative access rights to absolute ones. */
>>>>>>> +	access_rights |= LANDLOCK_MASK_ACCESS_NET &
>>>>>>> +			 ~landlock_get_net_access_mask(ruleset, 0);
>>>>>>> +
>>>>>>> +	mutex_lock(&ruleset->lock);
>>>>>>> +	err = landlock_insert_rule(ruleset, id, access_rights);
>>>>>>> +	mutex_unlock(&ruleset->lock);
>>>>>>> +
>>>>>>> +	return err;
>>>>>>> +}
>>>>>>> +
>>>>>>> +static int check_socket_access(const struct landlock_ruleset *const domain,
>>>>>>> +			       u16 port, access_mask_t access_request)
>>>>>>> +{
>>>>>>> +	bool allowed = false;
>>>>>>> +	layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_NET] = {};
>>>>>>> +	const struct landlock_rule *rule;
>>>>>>> +	access_mask_t handled_access;
>>>>>>> +	const struct landlock_id id = {
>>>>>>> +		.key.data = port,
>>>>>>> +		.type = LANDLOCK_KEY_NET_PORT,
>>>>>>> +	};
>>>>>>> +
>>>>>>> +	if (WARN_ON_ONCE(!domain))
>>>>>>> +		return 0;
>>>>>>> +	if (WARN_ON_ONCE(domain->num_layers < 1))
>>>>>>> +		return -EACCES;
>>>>>>> +
>>>>>>> +	rule = landlock_find_rule(domain, id);
>>>>>>> +	handled_access = init_layer_masks(domain, access_request, &layer_masks,
>>>>>>> +					  LANDLOCK_KEY_NET_PORT);
>>>>>>> +	allowed = unmask_layers(rule, handled_access, &layer_masks,
>>>>>>> +				ARRAY_SIZE(layer_masks));
>>>>>>> +
>>>>>>> +	return allowed ? 0 : -EACCES;
>>>>>>> +}
>>>>>>> +
>>>>>>> +static u16 get_port(const struct sockaddr *const address)
>>>>>>
>>>>>> get_port() should return a __be16 type. This enables to avoid converting
>>>>>> port when checking a rule.
>>>>>
>>>>>       In this case a user must do a coverting port into __be16:
>>>>>
>>>>>       struct landlock_net_service_attr net_service = {
>>>>>                     .allowed_access = LANDLOCK_ACCESS_NET_BIND_TCP,
>>>>>
>>>>>                     .port = htons(sock_port),
>>>>>             };
>>>>>      I think that a user should not think about this conversion cause it
>>>>> makes UAPI more complex to use. Lets do this under kernel's hood and let
>>>>> it as it is now -> u16 port.
>>>>>
>>>>> What do you think?
>>>>
>>>> BE and LE conversions may be error prone without strong typing, but the
>>>> current Linux network UAPI uses this convention (see related syscalls),
>>>> so developers already use htons() in their applications. I think it is
>>>> less hazardous to use the same convention. It would be nice to have the
>>>> point of view of network and API folks though.
>>>
>>>      Ok. Thanks. Let ports be in BE format like in network packets.
>>>
>>>      What should a selftest with port conversion be like?
>>>
>>>      1. Set a port with a Landlock rule with no conversion. get an error
>>> wit bind/connect actions.
>>>      2. Convert a port with htons(sock_port). get no error.
>>>
>>>      What do you think?
>>
>> Right, you can do both on a LE architecture (that must be checked in the
>> test or it should be skipped), test with a port value that has different
>> representation in LE and BE.
> 
>     Do you mean to check architecture in a test first and then port
> representaton? What about BE architectures? My current VM is X86-64
> architecture a LE one. I can test just it now.

It's just that tests should pass whatever architecture they are run on. 
So we need to check that the current architecture is LE to check against 
an LE result but not against a BE one, and vice versa. In fact no test 
should be skipped, just the result to compare with adjusted (i.e. either 
it pass or it failed).

Re: [PATCH v8 08/12] landlock: Implement TCP network hooks

[Konstantin Meskhidze (A)]

1/10/2023 8:24 PM, Mickaël Salaün пишет:
> 
> On 10/01/2023 05:45, Konstantin Meskhidze (A) wrote:
>> 
>> 
>> 1/9/2023 3:38 PM, Mickaël Salaün пишет:
>>>
>>> On 09/01/2023 09:07, Konstantin Meskhidze (A) wrote:
>>>>
>>>>
>>>> 1/6/2023 10:30 PM, Mickaël Salaün пишет:
>>>>>
>>>>> On 05/01/2023 09:57, Konstantin Meskhidze (A) wrote:
>>>>>>
>>>>>>
>>>>>> 11/17/2022 9:43 PM, Mickaël Salaün пишет:
>>>>>>>
>>>>>>> On 21/10/2022 17:26, Konstantin Meskhidze wrote:
>>>>>>>> This patch adds support of socket_bind() and socket_connect() hooks.
>>>>>>>> It's possible to restrict binding and connecting of TCP sockets to
>>>>>>>> particular ports.
>>>>>>>
>>>>>>> Implement socket_bind() and socket_connect LSM hooks, which enable to
>>>>>>> restrict TCP socket binding and connection to specific ports.
>>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>> Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze@huawei.com>
>>>>>>>> ---
>>>>>>>>
>>>>>>>> Changes since v7:
>>>>>>>> * Minor fixes.
>>>>>>>> * Refactors commit message.
>>>>>>>>
>>>>>>>> Changes since v6:
>>>>>>>> * Updates copyright.
>>>>>>>> * Refactors landlock_append_net_rule() and check_socket_access()
>>>>>>>>       functions with landlock_id type.
>>>>>>>>
>>>>>>>> Changes since v5:
>>>>>>>> * Fixes some logic errors.
>>>>>>>> * Formats code with clang-format-14.
>>>>>>>>
>>>>>>>> Changes since v4:
>>>>>>>> * Factors out CONFIG_INET into make file.
>>>>>>>> * Refactors check_socket_access().
>>>>>>>> * Adds helper get_port().
>>>>>>>> * Adds CONFIG_IPV6 in get_port(), hook_socket_bind/connect
>>>>>>>> functions to support AF_INET6 family.
>>>>>>>> * Adds AF_UNSPEC family support in hook_socket_bind/connect
>>>>>>>> functions.
>>>>>>>> * Refactors add_rule_net_service() and landlock_add_rule
>>>>>>>> syscall to support network rule inserting.
>>>>>>>> * Refactors init_layer_masks() to support network rules.
>>>>>>>>
>>>>>>>> Changes since v3:
>>>>>>>> * Splits commit.
>>>>>>>> * Adds SECURITY_NETWORK in config.
>>>>>>>> * Adds IS_ENABLED(CONFIG_INET) if a kernel has no INET configuration.
>>>>>>>> * Adds hook_socket_bind and hook_socket_connect hooks.
>>>>>>>>
>>>>>>>> ---
>>>>>>>>      security/landlock/Kconfig    |   1 +
>>>>>>>>      security/landlock/Makefile   |   2 +
>>>>>>>>      security/landlock/net.c      | 164 +++++++++++++++++++++++++++++++++++
>>>>>>>>      security/landlock/net.h      |  26 ++++++
>>>>>>>>      security/landlock/setup.c    |   2 +
>>>>>>>>      security/landlock/syscalls.c |  59 ++++++++++++-
>>>>>>>>      6 files changed, 251 insertions(+), 3 deletions(-)
>>>>>>>>      create mode 100644 security/landlock/net.c
>>>>>>>>      create mode 100644 security/landlock/net.h
>>>>>>>>
>>>>>>>> diff --git a/security/landlock/Kconfig b/security/landlock/Kconfig
>>>>>>>> index 8e33c4e8ffb8..10c099097533 100644
>>>>>>>> --- a/security/landlock/Kconfig
>>>>>>>> +++ b/security/landlock/Kconfig
>>>>>>>> @@ -3,6 +3,7 @@
>>>>>>>>      config SECURITY_LANDLOCK
>>>>>>>>      	bool "Landlock support"
>>>>>>>>      	depends on SECURITY && !ARCH_EPHEMERAL_INODES
>>>>>>>> +	select SECURITY_NETWORK
>>>>>>>>      	select SECURITY_PATH
>>>>>>>>      	help
>>>>>>>>      	  Landlock is a sandboxing mechanism that enables processes to restrict
>>>>>>>> diff --git a/security/landlock/Makefile b/security/landlock/Makefile
>>>>>>>> index 7bbd2f413b3e..53d3c92ae22e 100644
>>>>>>>> --- a/security/landlock/Makefile
>>>>>>>> +++ b/security/landlock/Makefile
>>>>>>>> @@ -2,3 +2,5 @@ obj-$(CONFIG_SECURITY_LANDLOCK) := landlock.o
>>>>>>>>
>>>>>>>>      landlock-y := setup.o syscalls.o object.o ruleset.o \
>>>>>>>>      	cred.o ptrace.o fs.o
>>>>>>>> +
>>>>>>>> +landlock-$(CONFIG_INET) += net.o
>>>>>>>> \ No newline at end of file
>>>>>>>> diff --git a/security/landlock/net.c b/security/landlock/net.c
>>>>>>>> new file mode 100644
>>>>>>>> index 000000000000..39e8a156a1f4
>>>>>>>> --- /dev/null
>>>>>>>> +++ b/security/landlock/net.c
>>>>>>>> @@ -0,0 +1,164 @@
>>>>>>>> +// SPDX-License-Identifier: GPL-2.0-only
>>>>>>>> +/*
>>>>>>>> + * Landlock LSM - Network management and hooks
>>>>>>>> + *
>>>>>>>> + * Copyright © 2022 Huawei Tech. Co., Ltd.
>>>>>>>> + * Copyright © 2022 Microsoft Corporation
>>>>>>>> + */
>>>>>>>> +
>>>>>>>> +#include <linux/in.h>
>>>>>>>> +#include <linux/net.h>
>>>>>>>> +#include <linux/socket.h>
>>>>>>>> +#include <net/ipv6.h>
>>>>>>>> +
>>>>>>>> +#include "common.h"
>>>>>>>> +#include "cred.h"
>>>>>>>> +#include "limits.h"
>>>>>>>> +#include "net.h"
>>>>>>>> +#include "ruleset.h"
>>>>>>>> +
>>>>>>>> +int landlock_append_net_rule(struct landlock_ruleset *const ruleset,
>>>>>>>> +			     const u16 port, access_mask_t access_rights)
>>>>>>>> +{
>>>>>>>> +	int err;
>>>>>>>> +	const struct landlock_id id = {
>>>>>>>> +		.key.data = port,
>>>>>>>> +		.type = LANDLOCK_KEY_NET_PORT,
>>>>>>>> +	};
>>>>>>>> +	BUILD_BUG_ON(sizeof(port) > sizeof(id.key.data));
>>>>>>>> +
>>>>>>>> +	/* Transforms relative access rights to absolute ones. */
>>>>>>>> +	access_rights |= LANDLOCK_MASK_ACCESS_NET &
>>>>>>>> +			 ~landlock_get_net_access_mask(ruleset, 0);
>>>>>>>> +
>>>>>>>> +	mutex_lock(&ruleset->lock);
>>>>>>>> +	err = landlock_insert_rule(ruleset, id, access_rights);
>>>>>>>> +	mutex_unlock(&ruleset->lock);
>>>>>>>> +
>>>>>>>> +	return err;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +static int check_socket_access(const struct landlock_ruleset *const domain,
>>>>>>>> +			       u16 port, access_mask_t access_request)
>>>>>>>> +{
>>>>>>>> +	bool allowed = false;
>>>>>>>> +	layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_NET] = {};
>>>>>>>> +	const struct landlock_rule *rule;
>>>>>>>> +	access_mask_t handled_access;
>>>>>>>> +	const struct landlock_id id = {
>>>>>>>> +		.key.data = port,
>>>>>>>> +		.type = LANDLOCK_KEY_NET_PORT,
>>>>>>>> +	};
>>>>>>>> +
>>>>>>>> +	if (WARN_ON_ONCE(!domain))
>>>>>>>> +		return 0;
>>>>>>>> +	if (WARN_ON_ONCE(domain->num_layers < 1))
>>>>>>>> +		return -EACCES;
>>>>>>>> +
>>>>>>>> +	rule = landlock_find_rule(domain, id);
>>>>>>>> +	handled_access = init_layer_masks(domain, access_request, &layer_masks,
>>>>>>>> +					  LANDLOCK_KEY_NET_PORT);
>>>>>>>> +	allowed = unmask_layers(rule, handled_access, &layer_masks,
>>>>>>>> +				ARRAY_SIZE(layer_masks));
>>>>>>>> +
>>>>>>>> +	return allowed ? 0 : -EACCES;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +static u16 get_port(const struct sockaddr *const address)
>>>>>>>
>>>>>>> get_port() should return a __be16 type. This enables to avoid converting
>>>>>>> port when checking a rule.
>>>>>>
>>>>>>       In this case a user must do a coverting port into __be16:
>>>>>>
>>>>>>       struct landlock_net_service_attr net_service = {
>>>>>>                     .allowed_access = LANDLOCK_ACCESS_NET_BIND_TCP,
>>>>>>
>>>>>>                     .port = htons(sock_port),
>>>>>>             };
>>>>>>      I think that a user should not think about this conversion cause it
>>>>>> makes UAPI more complex to use. Lets do this under kernel's hood and let
>>>>>> it as it is now -> u16 port.
>>>>>>
>>>>>> What do you think?
>>>>>
>>>>> BE and LE conversions may be error prone without strong typing, but the
>>>>> current Linux network UAPI uses this convention (see related syscalls),
>>>>> so developers already use htons() in their applications. I think it is
>>>>> less hazardous to use the same convention. It would be nice to have the
>>>>> point of view of network and API folks though.
>>>>
>>>>      Ok. Thanks. Let ports be in BE format like in network packets.
>>>>
>>>>      What should a selftest with port conversion be like?
>>>>
>>>>      1. Set a port with a Landlock rule with no conversion. get an error
>>>> wit bind/connect actions.
>>>>      2. Convert a port with htons(sock_port). get no error.
>>>>
>>>>      What do you think?
>>>
>>> Right, you can do both on a LE architecture (that must be checked in the
>>> test or it should be skipped), test with a port value that has different
>>> representation in LE and BE.
>> 
>>     Do you mean to check architecture in a test first and then port
>> representaton? What about BE architectures? My current VM is X86-64
>> architecture a LE one. I can test just it now.
> 
> It's just that tests should pass whatever architecture they are run on.
> So we need to check that the current architecture is LE to check against
> an LE result but not against a BE one, and vice versa. In fact no test
> should be skipped, just the result to compare with adjusted (i.e. either
> it pass or it failed).

   Ok. Thanks.
> .