[RFC PATCH 0/2] Landlock network PoC implementation
Mickaël Salaün
mic at digikod.net
Mon Dec 13 08:29:37 UTC 2021
Hi Konstantin,
On 10/12/2021 08:21, Konstantin Meskhidze wrote:
> According the main idea of Landlock's author Mickaёl Salun the goal
> of Landlock is to enable to restrict ambient rights
> (e.g. global filesystem access) for a set of processes.
> Landlock is stackable LSM. Landlock empowers any process,
> including unprivileged ones, to securely restrict themselves.
> This feature could be very helpful in containerized environment
> and different sandboxed applications.
>
> Now Landlock supports a number of filesytem access rights:
> - file execute
> - write to file
> - read file
> - make directory
> and etc.
>
> So, this PoC patch provides the basic idea for Landlock network
> access-control implementation.
> eBPF-based sandboxes like BPFcontain and BPFbox sandboxes confine
> network traffic at the socket level. Network rules grant access
> to various socket operations on the IPv4 and IPv6 socket families.
> Socket operations could be grouped by different use cases:
> • Client, server.
> • Send and receive categories.
> BPFbox operation example:
> - connect
> - bind
> - accept
> - listen
> - send
> and etc.
>
> Landlock network enhancement should follow similar logic
> like in mentioned sandboxes!
I think this list is inspired from SELinux:
https://selinuxproject.org/page/ObjectClassesPerms#common_socket
Other LSMs (e.g. Smack as mention by Casey, but also AppArmor and
Tomoyo) may be a good source of inspiration too.
I think the most relevant access-rights for Landlock are those used by
Capsicum: https://www.freebsd.org/cgi/man.cgi?query=rights&sektion=4
One thing to keep in mind is test coverage. At the end, all access
rights must be tested and with a good kernel coverage, see
tools/testing/selftests/landlock . This means that the more access
rights are handled, the more work/time to write tests, and the more
work/time for review. Depending on how straightforward it is to handle
these access rights and test them, you may want to start with a subset
of them.
It may make sense to support all of these access rights but for only one
network domain. At the same time, we should get confident that these
access rights can be implemented, for the Landlock use case, in a
consistent way whatever the network domain is.
>
> Implementation related issues
> =============================
>
> 1. Its needed to add a new type of rules:
>
> enum landlock_rule_type {
> LANDLOCK_RULE_PATH_BENEATH = 1,
> LANDLOCK_RULE_NET = 2,
A rule type identifies the type of the kernel object (here socket or a
set of addresses) and also the way to describe it (e.g. subnet, path for
a UNIX socket…) along with associated access rights. In a nutshell, it
is an unique identifier for a landlock_*_attr struct type.
> LANDLOCK_RULE_TYPE_NUM
This must not be in the public UAPI. See security/landlock/limits.h
> }
>
> struct landlock_ruleset_attr {
> _u64 handled_access_fs;
> _u64 handled_actions_net;
Correct, but it makes sense to name it handled_access_net instead (as in
your current code).
> }
>
> struct landlock_net_beneath_attr {
"beneath" is not appropriate here. E.g. using landlock_net_cidr4_attr
looks good to identify IPv4 subnets.
> /**
> * @allowed_actions: Bitmask of allowed network actions
> * for a current process
> */
> __u64 allowed_actions;
You need a way to describe a network object (e.g. a subnet) and also the
communication semantic (e.g. TCP, UPD…). We should take inspiration from
names used in the socket(2) man page.
> }
>
>
> 2. Added 2 defines for network allowed actions.
> Now the current prototype patch support creating 2 types
> of sockets - unix and inet:
> LANDLOCK_ACCESS_FS_MAKE_UNIX_SOCK (1ULL << 0)
It should be LANDLOCK_ACCESS_NET_{BIND,CREATE} or something similar. The
name UNIX and INET should not appear here but only in the
landlock_net_cidr4_attr.
> LANDLOCK_ACCESS_FS_MAKE_INET_SOCK (1ULL << 1)
>
> Its needed to discuss what kind of sockets types and actions
> should be supported in MINIMAL version!!!
I think IPv4 UDP/TCP sockets are the best way to go, to not rely on
process identification and to be sure that access rights are consistent
with different protocols.
You can start with access rights that are easy to implement, and split
patches at least per use of hook or something that makes sense from an
implementation point of view. Keep in mind that all access rights must
be tested, so it may be wise to start with a few. Tests must be
submitted within the same patch series in dedicated patches. See the
tools/testing/selftest/landlock files. Good tests should take more time
to write than kernel code.
>
>
> 3. Syscalls interfaces.
> Current syscall interface look like enough for support different
> rule types, but it’s needed to modify some internal functions
> to support it.
>
> As an example:
> struct landlock_ruleset *landlock_create_ruleset(const u32 fs_access_mask,
> const u32 net_access_mask)
> {
> struct landlock_ruleset *new_ruleset;
>
> /* Informs about useless ruleset. */
> if (!access_mask && !net_access_mask )
> return ERR_PTR(-ENOMSG);
> new_ruleset = create_ruleset(1);
> if (!IS_ERR(new_ruleset) && access_mask)
> new_ruleset->access_masks[0][0] = access_mask;
> if (!IS_ERR(new_ruleset) && net_access_mask)
You can wrap the two mask tests with only one if(!IS_ERR(new_ruleset)).
> new_ruleset->fs_access_masks[1][0] = net_access_mask;
We need to be careful about type confusion and a two-dimensional array
is not a good fit for a ruleset.
>
> return new_ruleset;
> }
>
> In future, it's to use an array access_mask[LANDLOCK_RULE_TYPE_NUM - 1]
> enum landlock_rule_type {
> LANDLOCK_RULE_PATH_BENEATH = 1
> LANDLOCK_RULE_NET = 2,
>
> LANDLOCK_RULE_TYPE_NUM
> };
>
>
> 4. Kernel objects.
> For filesystem restrictions inodes objects are used to tie landlock rules.
> But for socket operations it's preferred to use task_struct object of
> a process, cause sockets' inodes are created just after
> security_socket_create() hook is called, and if its needed to have
> some restriction rule for creating sockets, this rule can't be tied
> to a socket inode cause there is no any has been created at the hook's
> catching moment, see the sock_create_lite() function below:
For the file system, we use inodes to identify hierarchies. We can't
safely rely on stateless objects (e.g. path strings) because the file
system changes, and then the rules must change with it.
To identify network objects (from the user point of view), we can rely
on stateless rule definitions because they may be absolute (i.e. IP
address), e.g. sandbox process creating a new connection or receveing an
UDP packet. It is not be the case with UNIX socket if they are come from
a path (i.e. inode) though. In this case we'll have to use the existing
file system identification mechanism and probably extend the current FS
access rights.
>
> int sock_create_lite(int family, int type, int protocol,
> struct socket **res)
> {
> int err;
> struct socket *sock = NULL;
>
> err = security_socket_create(family, type, protocol, 1);
> if (err)
> goto out;
> ...
>
> sock = sock_alloc();
> if (!sock) {
> err = -ENOMEM;
> goto out;
> }
> ...
> }
> And all other socket rules can be tied to a process object, cause
> it would be easier to maintaint and control it.
A sandbox is a set of processes handled as "subjects". Generic inet
rules should not be tied to processes (for now) but on subnets/protocols.
> For containerized application a user could create a bunch container with
> individual network restriction rules tied to them.
>
> This solution (what kernel object to use) must be established first,
> cause the following design depends on it.
>
>
> 5. Landlock internal structures modifications.
>
> - In further patches its needed network_rules structure in landlock_layer
> structure to support network rules:
>
> struct landlock_layer {
> /**
> * @level: Position of this layer in the layer stack.
> */
> u16 level;
> /**
> * @access: Bitfield of allowed actions on the kernel object. They are
> * relative to the object type (e.g. %LANDLOCK_ACTION_FS_READ).
> */
> u16 access;
>
> struct network_rules;
The current landlock_layer is designed to handle different types of
actions relative to the object type. This struct doesn't need to be
modified.
> };
>
> The architecture of struct network_rules has not been established yet,
> and it could be like this :
>
> struct network_rules {
> Socket_domains = {AF_UNIX, AF_INET
> Socket_types = {SOCK_STREAM, ...
> Socket_protocols = {IP, TCP, ...
> Socket_actions = {"connect", "bind"
> IP_address_allowed[]
> }
To sum up, for IPv4 restrictions, we need a new rule type identified
with LANDLOCK_RULE_NET_CIDR4. This will handle a new
struct landlock_net_cidr4_attr {
__u64 allowed_access;
__u32 address; // IPv4
__u8 prefix; // From 0 to 32
__u8 type; // SOCK_DGRAM, SOCK_STREAM
__u16 port;
} __attribute__((packed));
// https://datatracker.ietf.org/doc/html/rfc4632
This data structure (*_attr) identifies actions (allowed accesses) on
objects (subnet and protocol).
For UNIX abstract sockets, we might have LANDLOCK_RULE_NET_UNIX_ABSTRACT
handling something similar to
struct landlock_net_unix_abstract_attr {
__u64 allowed_access;
char *abstract_path;
};
It may not make sense to have Landlock rules for unnamed UNIX sockets
(socket pairs) for now. Indeed, it would require to identify local peers.
>
> - Using pointer to FAM arrays of different types of access masks: fs,
> network, ipc etc.
>
> struct landlock_ruleset {
>
> struct rb_root root;
> ...
> ...
>
> u16 *access_masks[];
u16 access_masks[] should be just fine.
> };
>
>
> This is just the basic ideas for the open discussion.
> Please give your opinion and suggestion.
It would be easier to review if you (mainly) describe code as comments
in the patches (or commit messages), but please don't duplicate it in
the cover letter. Most of my above comments could be duplicated in the
patch comments below.
>
> Konstantin Meskhidze (2):
> landlock: base template for network hooks
> landlock: socket_create() hook implemented
>
> include/uapi/linux/landlock.h | 69 +++++++++++
> security/landlock/Makefile | 2 +-
> security/landlock/fs.c | 4 +-
> security/landlock/limits.h | 3 +
> security/landlock/net.c | 209 ++++++++++++++++++++++++++++++++++
> security/landlock/net.h | 50 ++++++++
> security/landlock/ruleset.c | 46 ++++++--
> security/landlock/ruleset.h | 11 +-
> security/landlock/setup.c | 3 +
> security/landlock/syscalls.c | 140 ++++++++++++++++-------
> 10 files changed, 479 insertions(+), 58 deletions(-)
> create mode 100644 security/landlock/net.c
> create mode 100644 security/landlock/net.h
>
> --
> 2.25.1
It seems that you manually cancatenated your three patches. The git
format-patch and send-email commands enable to create proper patch
series (i.e. one email per patch + cover letter) which is the way to
submit kernel patches and eases review. See
https://www.kernel.org/doc/html/latest/process/submitting-patches.html
and https://kernelnewbies.org/FirstKernelPatch
More generally, please take a look at
https://www.kernel.org/doc/html/latest/process/development-process.html
and especially the fifth part.
The latest Landlock patch series can give you an idea of the expected
format: https://lore.kernel.org/r/20210422154123.13086-1-mic@digikod.net/
>
>>From 8f1dff03a9722601334a31ee3c6680240a0e6e1c Mon Sep 17 00:00:00 2001
> From: Konstantin Meskhidze <konstantin.meskhidze at huawei.com>
> Date: Fri, 19 Nov 2021 11:18:23 +0800
> Subject: [RFC PATCH 1/2] landlock: base template for network hooks
>
> Add a new rule type LANDLOCK_RULE_NET to
> landlock_rule_type.
> Add landlock_net_beneath_attr to support network
> allowed actions.
> Add two network flags for socket creation confinement:
> LANDLOCK_ACCESS_FS_MAKE_UNIX_SOCK,
> LANDLOCK_ACCESS_FS_MAKE_INET_SOCK.
> Update Landlock syscalls to support network rules
> injections.
> Update landlock_ruleset structure to support network access mask.
> Add net.c and net.h files with network template functions.
> Add new blob size with lbs_task and landlock_add_net_hooks
> into landlock_init().
>
> Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze at huawei.com>
> ---
> include/uapi/linux/landlock.h | 69 +++++++++++++++++
> security/landlock/Makefile | 2 +-
> security/landlock/fs.c | 4 +-
> security/landlock/limits.h | 3 +
> security/landlock/net.c | 88 +++++++++++++++++++++
> security/landlock/net.h | 51 +++++++++++++
> security/landlock/ruleset.c | 46 ++++++++---
> security/landlock/ruleset.h | 9 ++-
> security/landlock/setup.c | 3 +
> security/landlock/syscalls.c | 140 ++++++++++++++++++++++++----------
> 10 files changed, 358 insertions(+), 57 deletions(-)
> create mode 100644 security/landlock/net.c
> create mode 100644 security/landlock/net.h
>
> diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
> index b3d952067f59..7d980b55d345 100644
> --- a/include/uapi/linux/landlock.h
> +++ b/include/uapi/linux/landlock.h
> @@ -25,6 +25,14 @@ struct landlock_ruleset_attr {
> * compatibility reasons.
> */
> __u64 handled_access_fs;
> +
> + /**
> + * @handled_actions_net: Bitmask of actions (cf. `Network flags`_)
@handled_access_net
> + * that is handled by this ruleset and should then be forbidden if no
> + * rule explicitly allow them. This is needed for backward
> + * compatibility reasons.
> + */
> + __u64 handled_access_net;
> };
>
> /*
> @@ -46,6 +54,18 @@ enum landlock_rule_type {
> * landlock_path_beneath_attr .
> */
> LANDLOCK_RULE_PATH_BENEATH = 1,
> +
> + /**
> + * @LANDLOCK_RULE_NET: Type of a &struct
> + * landlock_net_beneath_attr .
> + */
> + LANDLOCK_RULE_NET = 2,
Not an appropriate name (see comment above).
> +
> + /**
> + * @LANDLOCK_RULE_TYPE_NUM: Number of
> + * Landlock rules type: fs, network, ipc ect.
> + */
> + LANDLOCK_RULE_TYPE_NUM
Must not be in the UAPI (see comment above).
> };
>
> /**
> @@ -70,6 +90,22 @@ struct landlock_path_beneath_attr {
> */
> } __attribute__((packed));
>
> +/**
> + * struct landlock_path_beneath_attr - Network actions definition
> + *
> + * Argument of sys_landlock_add_rule().
> + */
> +struct landlock_net_beneath_attr {
Should be struct landlock_net_cidr4_attr or similar.
> + /**
> + * @allowed_actions: Bitmask of allowed network actions for a current process
> + */
> + __u64 allowed_actions;
See suggested fields for landlock_net_cidr4_attr above.
> + /*
> + * This struct is packed to avoid trailing reserved members.
> + * Cf. security/landlock/syscalls.c:build_check_abi()
> + */
> +} __attribute__((packed));
No need for the packed attribute here.
> +
> /**
> * DOC: fs_access
> *
> @@ -134,4 +170,37 @@ struct landlock_path_beneath_attr {
> #define LANDLOCK_ACCESS_FS_MAKE_BLOCK (1ULL << 11)
> #define LANDLOCK_ACCESS_FS_MAKE_SYM (1ULL << 12)
>
> +/**
> + * DOC: net_actions
> + *
> + * A set of actions on kernel objects may be defined by an attribute (e.g.
> + * &struct landlock_net_beneath_attr) including a bitmask of access.
> + *
> + * Network flags
> + * ~~~~~~~~~~~~~~~~
> + *
> + * These flags enable to restrict a sandboxed process to a set of actions on
> + * network actions.
Need some update.
> + *
> + * A process can only create these type sockets with allowed actions:
> + *
> + * - %LANDLOCK_ACCESS_NET_MAKE...: Create a unix/inet socket.
> + *
> + * - %LANDLOCK_ACCESS_NET_MAKE_UNIX_SOCK: Create (or rename or link) a UNIX domain
> + * socket.
> + * - %LANDLOCK_ACCESS_NET_MAKE_INET_SOCK: Create (or rename or link) a IPv4 Internet
> + * protocols domain socket.
> + *
> + * .. warning::
> + *
> + * It is currently not possible to restrict some network-related actions
> + * accessible through these syscall families: accept(2), bind(2),
> + * close(2), connect(2), fcntl(2),getpeername(2), getsockname(2),
> + * getsockopt(2), listen(2), read(2), recv(2), select(2), send(2),
> + * shutdown(2), socketpair(2),
> + * Future Landlock evolutions will enable to restrict them.
Please don't copy unrelated documentation.
> + */
> +#define LANDLOCK_ACCESS_FS_MAKE_UNIX_SOCK (1ULL << 0)
> +#define LANDLOCK_ACCESS_FS_MAKE_INET_SOCK (1ULL << 1)
These accesses are not appropriate (see comment above).
> +
> #endif /* _UAPI_LINUX_LANDLOCK_H */
> diff --git a/security/landlock/Makefile b/security/landlock/Makefile
> index 7bbd2f413b3e..afa44baaa83a 100644
> --- a/security/landlock/Makefile
> +++ b/security/landlock/Makefile
> @@ -1,4 +1,4 @@
> obj-$(CONFIG_SECURITY_LANDLOCK) := landlock.o
>
> landlock-y := setup.o syscalls.o object.o ruleset.o \
> - cred.o ptrace.o fs.o
> + cred.o ptrace.o fs.o net.o
> diff --git a/security/landlock/fs.c b/security/landlock/fs.c
> index 97b8e421f617..bb5967fc5dd4 100644
> --- a/security/landlock/fs.c
> +++ b/security/landlock/fs.c
> @@ -163,7 +163,7 @@ int landlock_append_fs_rule(struct landlock_ruleset *const ruleset,
> return -EINVAL;
>
> /* Transforms relative access rights to absolute ones. */
> - access_rights |= LANDLOCK_MASK_ACCESS_FS & ~ruleset->fs_access_masks[0];
> + access_rights |= LANDLOCK_MASK_ACCESS_FS & ~ruleset->access_masks[0][0];
We should rename fs_access_masks to access_masks but keep it as a
one-dimensional array and identify the type of the access (file system
or network) with e.g. with the 16th (upper) bit which would be checked
every time accesses are used thanks to a dedicated helper, and…
> object = get_inode_object(d_backing_inode(path->dentry));
> if (IS_ERR(object))
> return PTR_ERR(object);
> @@ -252,7 +252,7 @@ static int check_access_path(const struct landlock_ruleset *const domain,
> /* Saves all layers handling a subset of requested accesses. */
> layer_mask = 0;
> for (i = 0; i < domain->num_layers; i++) {
…it would then require to exclude network access layers here.
> - if (domain->fs_access_masks[i] & access_request)
> + if (domain->access_masks[0][i] & access_request)
> layer_mask |= BIT_ULL(i);
> }
> /* An access request not handled by the domain is allowed. */
> diff --git a/security/landlock/limits.h b/security/landlock/limits.h
> index 2a0a1095ee27..56f883530545 100644
> --- a/security/landlock/limits.h
> +++ b/security/landlock/limits.h
> @@ -18,4 +18,7 @@
> #define LANDLOCK_LAST_ACCESS_FS LANDLOCK_ACCESS_FS_MAKE_SYM
> #define LANDLOCK_MASK_ACCESS_FS ((LANDLOCK_LAST_ACCESS_FS << 1) - 1)
>
> +#define LANDLOCK_LAST_ACCESS_NET LANDLOCK_ACCESS_FS_MAKE_INET_SOCK
No FS here. I guess you'll like LANDLOCK_ACCESS_NET_CREATE.
Accesses/suffixes should be:
- CREATE
- ACCEPT
- BIND
- LISTEN
- CONNECT
- RECEIVE (RECEIVE_FROM and SEND_TO should not be needed)
- SEND
- SHUTDOWN
- GET_OPTION (GETSOCKOPT)
- SET_OPTION (SETSOCKOPT)
> +#define LANDLOCK_MASK_ACCESS_NET ((LANDLOCK_LAST_ACCESS_NET << 1) - 1)
> +
> #endif /* _SECURITY_LANDLOCK_LIMITS_H */
> diff --git a/security/landlock/net.c b/security/landlock/net.c
> new file mode 100644
> index 000000000000..49af2c821f79
> --- /dev/null
> +++ b/security/landlock/net.c
> @@ -0,0 +1,88 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Landlock LSM - Filesystem management and hooks
> + *
> + * Copyright © 2016-2020 Mickaël Salaün <mic at digikod.net>
> + * Copyright © 2018-2020 ANSSI
> + */
> +
> +#include <linux/atomic.h>
> +#include <linux/bitops.h>
> +#include <linux/bits.h>
> +#include <linux/compiler_types.h>
> +#include <linux/dcache.h>
> +#include <linux/err.h>
> +#include <linux/fs.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/limits.h>
> +#include <linux/list.h>
> +#include <linux/lsm_hooks.h>
> +#include <linux/mount.h>
> +#include <linux/namei.h>
> +#include <linux/path.h>
> +#include <linux/rcupdate.h>
> +#include <linux/spinlock.h>
> +#include <linux/stat.h>
> +#include <linux/types.h>
> +#include <linux/wait_bit.h>
> +#include <linux/workqueue.h>
> +#include <uapi/linux/landlock.h>
> +#include <linux/thread_info.h>
> +#include <linux/sched.h>
> +
> +
> +#include "common.h"
> +#include "cred.h"
> +#include "fs.h"
> +#include "limits.h"
> +#include "object.h"
> +#include "ruleset.h"
> +#include "setup.h"
> +#include "net.h"
Please review all the included files and remove those that are not
needed, and keep the explicit list of directly required ones (i.e. *not*
removing those that are indirectly included).
> +
> +/* Underlying object management */
> +
> +static void release_task_object(struct landlock_object *const object)
> +{
> + struct task_struct *const task = object->underobj;
> +
> + /* Untie landlock object from kernel object */
Please use full sentences with third person.
> + object->underobj = NULL;
> +
> + /* Untie kernel object from landlock object */
> + rcu_assign_pointer(landlock_task(task)->object, NULL);
We need a full explanation of how this should work (object owners,
locking, RCU window… or why such mechanism is not required) to protect
against concurrent accesses and race conditions.
> +
> +}
> +
> +static const struct landlock_object_underops landlock_net_underops = {
> + .release = release_task_object
Do we need this?
> +};
> +
> +struct task_struct;
> +
> +static inline struct task_struct *get_current_task(void)
> +{
> + return current;
Why such helper?
> +}
> +
> +int landlock_append_net_rule(struct landlock_ruleset *const ruleset,
> + u32 access_hierarchy)
> +{
> + return 0; > +}
> +
> +static int hook_socket_create(int family, int type, int protocol, int kern)
> +{
> + return 0;
This hook doesn't seem to be needed in this patch, they add boilerplate
for no value. Try to only add the minimal required code for each patch.
> +}
> +
> +static struct security_hook_list landlock_hooks[] __lsm_ro_after_init = {
> + LSM_HOOK_INIT(socket_create, hook_socket_create),
> +};
> +
> +__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..094ac150ac3c
> --- /dev/null
> +++ b/security/landlock/net.h
> @@ -0,0 +1,51 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Landlock LSM - Network management and hooks
> + *
> + * Copyright © 2017-2020 Mickaël Salaün <mic at digikod.net>
> + * Copyright © 2018-2020 ANSSI
> + */
> +
> +#ifndef _SECURITY_LANDLOCK_NET_H
> +#define _SECURITY_LANDLOCK_NET_H
> +
> +#include <linux/fs.h>
> +#include <linux/init.h>
> +#include <asm/current.h>
> +#include <linux/cred.h>
> +#include <linux/errno.h>
> +#include <linux/kernel.h>
> +#include <linux/lsm_hooks.h>
> +#include <linux/rcupdate.h>
> +#include <linux/sched.h>
> +
> +#include "common.h"
> +#include "ruleset.h"
> +#include "setup.h"
Please clean up the included files.
> +
> +
> +/**
> + * struct landlock_task_security - Process task security blob
> + *
> + * Enable to reference a &struct landlock_object tied to a task (i.e.
> + * underlying object).
> + */
> +struct landlock_task_security {
> + /**
> + * @object: Weak pointer to an allocated object.
> + */
> + struct landlock_object __rcu *object;
> +};
> +
> +static inline struct landlock_task_security *landlock_task(
> + const struct task_struct *const task)
> +{
> + return task->security + landlock_blob_sizes.lbs_task;
> +}
> +
> +__init void landlock_add_net_hooks(void);
> +
> +int landlock_append_net_rule(struct landlock_ruleset *const ruleset,
> + u32 access_hierarchy);
> +
> +#endif /* _SECURITY_LANDLOCK_NET_H */
> diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
> index ec72b9262bf3..cec0fd1f83df 100644
> --- a/security/landlock/ruleset.c
> +++ b/security/landlock/ruleset.c
> @@ -19,6 +19,7 @@
> #include <linux/slab.h>
> #include <linux/spinlock.h>
> #include <linux/workqueue.h>
> +#include <uapi/linux/landlock.h>
>
> #include "limits.h"
> #include "object.h"
> @@ -27,9 +28,23 @@
> static struct landlock_ruleset *create_ruleset(const u32 num_layers)
> {
> struct landlock_ruleset *new_ruleset;
> + u16 row, col, rules_types_num;
> +
> + new_ruleset = kmalloc(sizeof(*new_ruleset) +
> + sizeof *(new_ruleset->access_masks),
> + GFP_KERNEL_ACCOUNT);
Why do you replace kzalloc with kmalloc?
> +
> + rules_types_num = LANDLOCK_RULE_TYPE_NUM - 1;
> + for (row = 0; row < rules_types_num; row++) {
> + new_ruleset->access_masks[row] = kmalloc(sizeof *(new_ruleset->access_masks[row]),
> + GFP_KERNEL_ACCOUNT);
> + for (col = 0; col < num_layers; col++) {
> + static int count = 1;
> +
> + new_ruleset->access_masks[row][col] = count++;
> + }
> + }
This extra complexity is not needed.
>
> - new_ruleset = kzalloc(struct_size(new_ruleset, fs_access_masks,
> - num_layers), GFP_KERNEL_ACCOUNT);
This call is just fine if you rename fs_access_masks as I commented above.
> if (!new_ruleset)
> return ERR_PTR(-ENOMEM);
> refcount_set(&new_ruleset->usage, 1);
> @@ -39,21 +54,25 @@ static struct landlock_ruleset *create_ruleset(const u32 num_layers)
> /*
> * hierarchy = NULL
> * num_rules = 0
> - * fs_access_masks[] = 0
> + * access_masks[] = 0
> */
> return new_ruleset;
> }
>
> -struct landlock_ruleset *landlock_create_ruleset(const u32 fs_access_mask)
> +struct landlock_ruleset *landlock_create_ruleset(const u32 fs_access_mask,
> + const u32 net_access_mask)
> {
> struct landlock_ruleset *new_ruleset;
>
> /* Informs about useless ruleset. */
> - if (!fs_access_mask)
> + if (!fs_access_mask && !net_access_mask)
This check is not correct.
> return ERR_PTR(-ENOMSG);
> new_ruleset = create_ruleset(1);
> - if (!IS_ERR(new_ruleset))
> - new_ruleset->fs_access_masks[0] = fs_access_mask;
> + if (!IS_ERR(new_ruleset) && fs_access_mask)
> + new_ruleset->access_masks[0][0] = fs_access_mask;
This needs to be updated and simplified.
> + if (!IS_ERR(new_ruleset) && net_access_mask)
> + new_ruleset->access_masks[1][0] = net_access_mask;
> +
> return new_ruleset;
> }
>
> @@ -116,11 +135,13 @@ static void build_check_ruleset(void)
> .num_rules = ~0,
> .num_layers = ~0,
> };
> - typeof(ruleset.fs_access_masks[0]) fs_access_mask = ~0;
> + typeof(ruleset.access_masks[0][0]) fs_access_mask = ~0;
> + typeof(ruleset.access_masks[1][0]) net_access_mask = ~0;
>
> BUILD_BUG_ON(ruleset.num_rules < LANDLOCK_MAX_NUM_RULES);
> BUILD_BUG_ON(ruleset.num_layers < LANDLOCK_MAX_NUM_LAYERS);
> BUILD_BUG_ON(fs_access_mask < LANDLOCK_MASK_ACCESS_FS);
> + BUILD_BUG_ON(net_access_mask < LANDLOCK_MASK_ACCESS_NET);
Good to update the build checks.
> }
>
> /**
> @@ -279,7 +300,10 @@ static int merge_ruleset(struct landlock_ruleset *const dst,
> err = -EINVAL;
> goto out_unlock;
> }
> - dst->fs_access_masks[dst->num_layers - 1] = src->fs_access_masks[0];
> + /* Copy fs access masks. */
> + dst->access_masks[0][dst->num_layers - 1] = src->access_masks[0][0];
> + /* Copy network access masks. */
> + dst->access_masks[1][dst->num_layers - 1] = src->access_masks[1][0];
This will be simplified.
>
> /* Merges the @src tree. */
> rbtree_postorder_for_each_entry_safe(walker_rule, next_rule,
> @@ -337,8 +361,8 @@ static int inherit_ruleset(struct landlock_ruleset *const parent,
> goto out_unlock;
> }
> /* Copies the parent layer stack and leaves a space for the new layer. */
> - memcpy(child->fs_access_masks, parent->fs_access_masks,
> - flex_array_size(parent, fs_access_masks, parent->num_layers));
> + memcpy(child->access_masks, parent->access_masks,
> + flex_array_size(parent, access_masks, parent->num_layers));
>
> if (WARN_ON_ONCE(!parent->hierarchy)) {
> err = -EINVAL;
> diff --git a/security/landlock/ruleset.h b/security/landlock/ruleset.h
> index 2d3ed7ec5a0a..8f4285fab372 100644
> --- a/security/landlock/ruleset.h
> +++ b/security/landlock/ruleset.h
> @@ -97,7 +97,7 @@ struct landlock_ruleset {
> * section. This is only used by
> * landlock_put_ruleset_deferred() when @usage reaches zero.
> * The fields @lock, @usage, @num_rules, @num_layers and
> - * @fs_access_masks are then unused.
> + * @access_masks are then unused.
> */
> struct work_struct work_free;
> struct {
> @@ -124,7 +124,7 @@ struct landlock_ruleset {
> */
> u32 num_layers;
> /**
> - * @fs_access_masks: Contains the subset of filesystem
> + * @access_masks: Contains the subset of filesystem
Need to update the documentation, including how to differentiate access
types. It would probably require to tweak num_layers or add a new field.
> * actions that are restricted by a ruleset. A domain
> * saves all layers of merged rulesets in a stack
> * (FAM), starting from the first layer to the last
> @@ -135,12 +135,13 @@ struct landlock_ruleset {
> * layers are set once and never changed for the
> * lifetime of the ruleset.
> */
> - u16 fs_access_masks[];
> + u16 *access_masks[];
u16 access_masks[];
> };
> };
> };
>
> -struct landlock_ruleset *landlock_create_ruleset(const u32 fs_access_mask);
> +struct landlock_ruleset *landlock_create_ruleset(const u32 fs_access_mask,
> + const u32 net_access_mask);
>
> void landlock_put_ruleset(struct landlock_ruleset *const ruleset);
> void landlock_put_ruleset_deferred(struct landlock_ruleset *const ruleset);
> diff --git a/security/landlock/setup.c b/security/landlock/setup.c
> index f8e8e980454c..91ab06ec8ce0 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;
>
> @@ -21,6 +22,7 @@ struct lsm_blob_sizes landlock_blob_sizes __lsm_ro_after_init = {
> .lbs_cred = sizeof(struct landlock_cred_security),
> .lbs_inode = sizeof(struct landlock_inode_security),
> .lbs_superblock = sizeof(struct landlock_superblock_security),
> + .lbs_task = sizeof(struct landlock_task_security),
> };
>
> static int __init landlock_init(void)
> @@ -28,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 32396962f04d..72cb1b0cb8a9 100644
> --- a/security/landlock/syscalls.c
> +++ b/security/landlock/syscalls.c
> @@ -31,6 +31,7 @@
> #include "limits.h"
> #include "ruleset.h"
> #include "setup.h"
> +#include "net.h"
>
> /**
> * copy_min_struct_from_user - Safe future-proof argument copying
> @@ -73,7 +74,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_beneath_attr net_beneath_attr;
No need for a "beneath" here.
> + size_t ruleset_size, path_beneath_size, net_beneath_size;
Please don't change the declaration order.
>
> /*
> * For each user space ABI structures, first checks that there is no
> @@ -81,13 +83,18 @@ static void build_check_abi(void)
> * struct size.
> */
> ruleset_size = sizeof(ruleset_attr.handled_access_fs);
> + ruleset_size += sizeof(ruleset_attr.handled_access_net);
> BUILD_BUG_ON(sizeof(ruleset_attr) != ruleset_size);
> - BUILD_BUG_ON(sizeof(ruleset_attr) != 8);
> + BUILD_BUG_ON(sizeof(ruleset_attr) != 16);
>
> path_beneath_size = sizeof(path_beneath_attr.allowed_access);
> 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_beneath_size = sizeof(net_beneath_attr.allowed_actions);
> + BUILD_BUG_ON(sizeof(net_beneath_attr) != net_beneath_size);
> + BUILD_BUG_ON(sizeof(net_beneath_attr) != 8);
> }
>
> /* Ruleset handling */
> @@ -174,20 +181,33 @@ SYSCALL_DEFINE3(landlock_create_ruleset,
> return -EINVAL;
> }
>
> - /* Copies raw user space buffer. */
> + /* Copies raw user space buffer for fs. */
> err = copy_min_struct_from_user(&ruleset_attr, sizeof(ruleset_attr),
> offsetofend(typeof(ruleset_attr), handled_access_fs),
> attr, size);
> if (err)
> return err;
>
> + /* Copies raw user space buffer for network. */
> + err = copy_min_struct_from_user(&ruleset_attr, sizeof(ruleset_attr),
> + offsetofend(typeof(ruleset_attr), handled_access_net),
Why two calls to copy_min_struct_from_user()? The first one is now useless.
> + attr, size);
> + if (err)
> + return err;
> +
> /* Checks content (and 32-bits cast). */
> if ((ruleset_attr.handled_access_fs | LANDLOCK_MASK_ACCESS_FS) !=
> LANDLOCK_MASK_ACCESS_FS)
> return -EINVAL;
>
> + /* Checks content (and 32-bits cast). */
> + if ((ruleset_attr.handled_access_net | LANDLOCK_MASK_ACCESS_NET) !=
> + LANDLOCK_MASK_ACCESS_NET)
> + return -EINVAL;
> +
> /* Checks arguments and transforms to kernel struct. */
> - ruleset = landlock_create_ruleset(ruleset_attr.handled_access_fs);
> + ruleset = landlock_create_ruleset(ruleset_attr.handled_access_fs,
> + ruleset_attr.handled_access_net);
> if (IS_ERR(ruleset))
> return PTR_ERR(ruleset);
>
> @@ -306,6 +326,7 @@ SYSCALL_DEFINE4(landlock_add_rule,
> const void __user *const, rule_attr, const __u32, flags)
> {
> struct landlock_path_beneath_attr path_beneath_attr;
> + struct landlock_net_beneath_attr net_beneath_attr;
> struct path path;
> struct landlock_ruleset *ruleset;
> int res, err;
> @@ -317,47 +338,88 @@ SYSCALL_DEFINE4(landlock_add_rule,
> if (flags)
> return -EINVAL;
>
> - if (rule_type != LANDLOCK_RULE_PATH_BENEATH)
> + if ((rule_type != LANDLOCK_RULE_PATH_BENEATH) &&
> + (rule_type != LANDLOCK_RULE_NET))
> return -EINVAL;
>
> - /* Copies raw user space buffer, only one type for now. */
> - res = copy_from_user(&path_beneath_attr, rule_attr,
> - sizeof(path_beneath_attr));
> - if (res)
> - return -EFAULT;
> -
> - /* Gets and checks the ruleset. */
> - ruleset = get_ruleset_from_fd(ruleset_fd, FMODE_CAN_WRITE);
> - if (IS_ERR(ruleset))
> - return PTR_ERR(ruleset);
> -
> - /*
> - * Informs about useless rule: empty allowed_access (i.e. deny rules)
> - * are ignored in path walks.
> - */
> - if (!path_beneath_attr.allowed_access) {
> - err = -ENOMSG;
> - goto out_put_ruleset;
> + if (rule_type == LANDLOCK_RULE_PATH_BENEATH) {
Please use a switch statement and set temporary pointers in this
statement to not duplicate the below code.
> + /* Copies raw user space buffer, only one type for now. */
> + res = copy_from_user(&path_beneath_attr, rule_attr,
> + sizeof(path_beneath_attr));
> + if (res)
> + return -EFAULT;
> }
> - /*
> - * Checks that allowed_access matches the @ruleset constraints
> - * (ruleset->fs_access_masks[0] is automatically upgraded to 64-bits).
> - */
> - if ((path_beneath_attr.allowed_access | ruleset->fs_access_masks[0]) !=
> - ruleset->fs_access_masks[0]) {
> - err = -EINVAL;
> - goto out_put_ruleset;
> +
> + if (rule_type == LANDLOCK_RULE_NET) {
> + /* Copies raw user space buffer, only one type for now. */
> + res = copy_from_user(&net_beneath_attr, rule_attr,
> + sizeof(net_beneath_attr));
> + if (res)
> + return -EFAULT;
> }
>
> - /* Gets and checks the new rule. */
> - err = get_path_from_fd(path_beneath_attr.parent_fd, &path);
> - if (err)
> - goto out_put_ruleset;
> + if (rule_type == LANDLOCK_RULE_PATH_BENEATH) {
> + /* Gets and checks the ruleset. */
> + ruleset = get_ruleset_from_fd(ruleset_fd, FMODE_CAN_WRITE);
> + if (IS_ERR(ruleset))
> + return PTR_ERR(ruleset);
> +
> + /*
> + * Informs about useless rule: empty allowed_access (i.e. deny rules)
> + * are ignored in path walks.
> + */
> + if (!path_beneath_attr.allowed_access) {
> + err = -ENOMSG;
> + goto out_put_ruleset;
> + }
Most of the code can be shared, no need to duplicate.
> + /*
> + * Checks that allowed_access matches the @ruleset constraints
> + * (ruleset->access_masks[0][0] is automatically upgraded to 64-bits).
> + */
> + if ((path_beneath_attr.allowed_access | ruleset->access_masks[0][0]) !=
> + ruleset->access_masks[0][0]) {
> + err = -EINVAL;
> + goto out_put_ruleset;
> + }
> +
> + /* Gets and checks the new rule. */
> + err = get_path_from_fd(path_beneath_attr.parent_fd, &path);
> + if (err)
> + goto out_put_ruleset;
> +
> + /* Imports the new rule. */
> + err = landlock_append_fs_rule(ruleset, &path,
> + path_beneath_attr.allowed_access);
> + path_put(&path);
> + }
>
> - /* Imports the new rule. */
> - err = landlock_append_fs_rule(ruleset, &path,
> - path_beneath_attr.allowed_access);
> - path_put(&path);
> + if (rule_type == LANDLOCK_RULE_NET) {
> + /* Gets and checks the ruleset. */
> + ruleset = get_ruleset_from_fd(ruleset_fd, FMODE_CAN_WRITE);
> + if (IS_ERR(ruleset))
> + return PTR_ERR(ruleset);
> +
> + /*
> + * Informs about useless rule: empty allowed_access (i.e. deny rules)
> + * are ignored in network actions
> + */
> + if (!net_beneath_attr.allowed_actions) {
> + err = -ENOMSG;
> + goto out_put_ruleset;
> + }
> + /*
> + * Checks that allowed_access matches the @ruleset constraints
> + * (ruleset->access_masks[1][0] is automatically upgraded to 64-bits).
> + */
> + if ((net_beneath_attr.allowed_actions | ruleset->access_masks[1][0]) !=
> + ruleset->access_masks[1][0]) {
> + err = -EINVAL;
> + goto out_put_ruleset;
> + }
> +
> + /* Imports the new rule. */
> + err = landlock_append_net_rule(ruleset, net_beneath_attr.allowed_actions);
> + }
>
> out_put_ruleset:
> landlock_put_ruleset(ruleset);
> --
> 2.25.1
>
>
>>From 5216f8c21805879cd41d57f858817417669a7f40 Mon Sep 17 00:00:00 2001
> From: Konstantin Meskhidze <konstantin.meskhidze at huawei.com>
> Date: Fri, 3 Dec 2021 12:04:11 +0800
> Subject: [RFC PATCH 2/2] landlock: socket_create() hook implemented
>
> Implement socket_create() hook.
> Current prototype can restrict creating two
> types of sockets: INET and UNIX. Its just basic idea
> how Landlock could support network confinement.
All commit messages should contain a set of selfsufficient paragraphs
with useful full sentences describing at least the context (with
potential background), the purpose of the patch, and how it is acheived.
Implementation details can be explain as comments in the patch.
>
> Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze at huawei.com>
> ---
> security/landlock/net.c | 141 ++++++++++++++++++++++++++++++++---
> security/landlock/net.h | 5 +-
> security/landlock/ruleset.h | 2 +-
> security/landlock/syscalls.c | 2 +-
> 4 files changed, 135 insertions(+), 15 deletions(-)
>
> diff --git a/security/landlock/net.c b/security/landlock/net.c
> index 49af2c821f79..3c6b3a6da322 100644
> --- a/security/landlock/net.c
> +++ b/security/landlock/net.c
> @@ -42,17 +42,10 @@
> #include "net.h"
>
> /* Underlying object management */
> -
> static void release_task_object(struct landlock_object *const object)
> {
> - struct task_struct *const task = object->underobj;
> -
> /* Untie landlock object from kernel object */
> object->underobj = NULL;
> -
> - /* Untie kernel object from landlock object */
> - rcu_assign_pointer(landlock_task(task)->object, NULL);
What is happening here?
> -
> }
>
> static const struct landlock_object_underops landlock_net_underops = {
> @@ -66,15 +59,143 @@ static inline struct task_struct *get_current_task(void)
> return current;
> }
>
> +static struct landlock_object *get_task_object(struct task_struct *const task)
> +{
> + struct landlock_object *object, *new_object;
> + struct landlock_task_security *task_sec = landlock_task(task);
> +
> + object = task_sec->object;
> + if (object)
> + if (likely(refcount_inc_not_zero(&object->usage)))
> + return object;
There is a race condition here.
> +
> + /*
> + * If there is no object tied to task, then create a new one.
> + */
> + new_object = landlock_create_object(&landlock_net_underops, task);
> + if (IS_ERR(new_object))
> + return new_object;
> +
> + task_sec->object = new_object;
> + return new_object;
> +}
> +
> +
> int landlock_append_net_rule(struct landlock_ruleset *const ruleset,
> - u32 access_hierarchy)
> + u32 access_rights)
Please update previous patches accordingly.
> {
> - return 0;
> + int err;
> + struct landlock_object *object;
> +
> + /* Transforms relative access rights to absolute ones. */
> + access_rights |= LANDLOCK_MASK_ACCESS_NET & ~ruleset->access_masks[1][0];
> +
> + object = get_task_object(get_current_task());
This patch series tries to restrict socket creation in a sandbox except
for the processes that create a rule that excludes themselves. This
doesn't fit with the Landlock model. This information is key to
understand this patch series (what it does and how) and it is missing
from the introduction (cover letter and this patch description).
What network control access rules would be useful to is to block network
access to all processes in a sandbox, except when receiving or sending
packets to or from a set of specific subnets (e.g. IPs) with a set of
specific protocols (e.g. TCP, UDP) and ports (e.g. 80, 443). In this
case, objects could be identified with IPs, protocols and ports.
> + if (IS_ERR(object))
> + return PTR_ERR(object);
> + mutex_lock(&ruleset->lock);
> + err = landlock_insert_rule(ruleset, object, access_rights << 16);
Why this shift? If this is legitimate, all such shift should be
self-documented with an (inlined) function helper.
> + mutex_unlock(&ruleset->lock);
> + /*
> + * No need to check for an error because landlock_insert_rule()
> + * increments the refcount for the new object if needed.
> + */
> + landlock_put_object(object);
> + return err;
> +}
> +
> +/* Access-control management */
> +static inline bool unmask_layers(
> + const struct landlock_ruleset *const domain,
> + const u32 access_request, u64 layer_mask)
> +{
> + const struct landlock_rule *rule;
> + const struct task_struct *task;
> + size_t i;
> + bool allowed = false;
> +
> + task = get_current_task();
> +
Please take a look at the RCU documentation. The sparse tool and
CONFIG_DEBUG_OBJECTS_* options can help.
> + rule = landlock_find_rule(domain, landlock_task(task)->object);
> +
> + /* Grant access if there is no rule for an oject */
typos
> + if (!rule)
> + return allowed = true;
> +
> + /*
> + * An access is granted if, for each policy layer, at least one rule
> + * encountered on network actions requested,
> + * regardless of their position in the layer stack. We must then check
> + * the remaining layers, from the first added layer to
> + * the last one.
> + */
There is a lot of duplicated code. This should not be the case.
> + for (i = 0; i < rule->num_layers; i++) {
> + const struct landlock_layer *const layer = &rule->layers[i];
> + const u64 layer_level = BIT_ULL(layer->level - 1);
> +
> + /* Checks that the layer grants access to the request. */
> + if ((layer->access & (access_request << 16)) == (access_request << 16)) {
> + layer_mask &= ~layer_level;
> + allowed = true;
> +
> + if (layer_mask == 0)
> + return allowed;
> + } else {
> + layer_mask &= ~layer_level;
> +
> + if (layer_mask == 0)
> + return allowed;
> + }
> + }
> + return allowed;
> +}
> +
> +static int check_socket_access(const struct landlock_ruleset *const domain,
> + int family)
> +{
> + bool allowed = false;
> + u64 layer_mask;
> + size_t i;
> +
> + /* Make sure all layers can be checked. */
> + BUILD_BUG_ON(BITS_PER_TYPE(layer_mask) < LANDLOCK_MAX_NUM_LAYERS);
> +
> + if (!family)
> + return 0;
> + if (WARN_ON_ONCE(!domain))
> + return 0;
> + if (WARN_ON_ONCE(domain->num_layers < 1))
> + return -EACCES;
> +
> + /* Saves all layers handling a subset of requested
> + * socket family.
> + */
> + layer_mask = 0;
> + for (i = 0; i < domain->num_layers; i++) {
> + if (domain->access_masks[1][i] & family)
> + layer_mask |= BIT_ULL(i);
> + }
> + /* An access request not handled by the domain is allowed. */
> + if (layer_mask == 0)
> + return 0;
> +
> + /*
> + * We need to walk through all the hierarchy to not miss any relevant
> + * restriction.
> + */
Please don't blindly copy-paste code, check that it still makes sense,
and update all related comments.
> + allowed = unmask_layers(domain, family, layer_mask);
Not a good idea to directly use "family" as an access request. You
defined one but don't use it here. Are you sure it works as expected?
> +
> + return allowed ? 0 : -EACCES;
> }
>
> static int hook_socket_create(int family, int type, int protocol, int kern)
> {
> - return 0;
> + const struct landlock_ruleset *const dom = landlock_get_current_domain();
> +
> + if (!dom)
> + return 0;
> +
> + return check_socket_access(dom, family);
> }
>
> static struct security_hook_list landlock_hooks[] __lsm_ro_after_init = {
> diff --git a/security/landlock/net.h b/security/landlock/net.h
> index 094ac150ac3c..2036272ff106 100644
> --- a/security/landlock/net.h
> +++ b/security/landlock/net.h
> @@ -23,7 +23,6 @@
> #include "ruleset.h"
> #include "setup.h"
>
> -
Don't introduce fixes for previous patches but rebase them.
> /**
> * struct landlock_task_security - Process task security blob
> *
> @@ -32,9 +31,9 @@
> */
> struct landlock_task_security {
> /**
> - * @object: Weak pointer to an allocated object.
> + * @object: Pointer to an allocated object.
> */
> - struct landlock_object __rcu *object;
> + struct landlock_object *object;
Same here.
> };
>
> static inline struct landlock_task_security *landlock_task(
> diff --git a/security/landlock/ruleset.h b/security/landlock/ruleset.h
> index 8f4285fab372..cec3fc489531 100644
> --- a/security/landlock/ruleset.h
> +++ b/security/landlock/ruleset.h
> @@ -28,7 +28,7 @@ struct landlock_layer {
> * @access: Bitfield of allowed actions on the kernel object. They are
> * relative to the object type (e.g. %LANDLOCK_ACTION_FS_READ).
> */
> - u16 access;
> + u32 access;
There is no need to change this size. As explained in the comment, this
struct is dedicated per object, and then per type.
> };
>
> /**
> diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c
> index 72cb1b0cb8a9..27653e7bf201 100644
> --- a/security/landlock/syscalls.c
> +++ b/security/landlock/syscalls.c
> @@ -339,7 +339,7 @@ SYSCALL_DEFINE4(landlock_add_rule,
> return -EINVAL;
>
> if ((rule_type != LANDLOCK_RULE_PATH_BENEATH) &&
> - (rule_type != LANDLOCK_RULE_NET))
> + (rule_type != LANDLOCK_RULE_NET))
Please squash such fixes in the corresponding previous patch.
> return -EINVAL;
>
> if (rule_type == LANDLOCK_RULE_PATH_BENEATH) {
> --
> 2.25.1
>
Thanks for this initial RFC Konstantin, this review should help you
build the next one.
Regards,
Mickaël
More information about the Linux-security-module-archive
mailing list