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

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

* Support socket rule storage in landlock ruleset.

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

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

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

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

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

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

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

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

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

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


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

(Some phrasing remarks)

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

(More about the content)

The Landlock documentation states the general approach up front:

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

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

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

To make a more constructive suggestion:

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

Does it also need the following addition?

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


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

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

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

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

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

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

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


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

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

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

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

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

Can socket_key.packed end up containing uninitialized memory here?

> +}

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

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

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

  struct {
    unsigned short family, type;
  }

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

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

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


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

—Günther

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

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

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

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

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

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

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

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

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

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

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

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

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

agreed, thanks

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

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

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

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

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

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


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

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

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

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

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

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

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

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

WDYT?

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

Hello!

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

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

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

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


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

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


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

Two brief side discussions:

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

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

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

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


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

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

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


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

(Did you see this remark?)


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

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


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

Is the wording "unsupported socket family" misleading here?

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

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

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

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


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

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

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

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

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

—Günther

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

Agreed, thanks

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

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

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

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

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

Yes, accept() should work with any restrictions.

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

Ok, I'll do it.

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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