On 27/07/2022 21:54, Mickaël Salaün wrote:
On 26/07/2022 19:43, Mickaël Salaün wrote:
On 21/06/2022 10:22, Konstantin Meskhidze wrote:
Hi,
This is a new V6 patch related to Landlock LSM network confinement.
It is based on the latest landlock-wip branch on top of v5.19-rc2:
https://git.kernel.org/pub/scm/linux/kernel/git/mic/linux.git/log/?h=landlock-wip
It brings refactoring of previous patch version V5:
- Fixes some logic errors and typos.
- Adds additional FIXTURE_VARIANT and FIXTURE_VARIANT_ADD helpers
to support both ip4 and ip6 families and shorten seltests' code.
- Makes TCP sockets confinement support optional in sandboxer demo.
- Formats the code with clang-format-14
All test were run in QEMU evironment and compiled with
-static flag.
1. network_test: 18/18 tests passed.
2. base_test: 7/7 tests passed.
3. fs_test: 59/59 tests passed.
4. ptrace_test: 8/8 tests passed.
Still have issue with base_test were compiled without -static flag
(landlock-wip branch without network support)
1. base_test: 6/7 tests passed.
Error:
# RUN global.inconsistent_attr ...
# base_test.c:54:inconsistent_attr:Expected ENOMSG (42) == errno (22)
# inconsistent_attr: Test terminated by assertion
# FAIL global.inconsistent_attr
not ok 1 global.inconsistent_attr
LCOV - code coverage report:
Hit Total Coverage
Lines: 952 1010 94.3 %
Functions: 79 82 96.3 %
Previous versions:
v5:
https://lore.kernel.org/linux-security-module/20220516152038.39594-1-konstantin.meskhidze@xxxxxxxxxx
v4:
https://lore.kernel.org/linux-security-module/20220309134459.6448-1-konstantin.meskhidze@xxxxxxxxxx/
v3:
https://lore.kernel.org/linux-security-module/20220124080215.265538-1-konstantin.meskhidze@xxxxxxxxxx/
v2:
https://lore.kernel.org/linux-security-module/20211228115212.703084-1-konstantin.meskhidze@xxxxxxxxxx/
v1:
https://lore.kernel.org/linux-security-module/20211210072123.386713-1-konstantin.meskhidze@xxxxxxxxxx/
Konstantin Meskhidze (17):
landlock: renames access mask
landlock: refactors landlock_find/insert_rule
landlock: refactors merge and inherit functions
landlock: moves helper functions
landlock: refactors helper functions
landlock: refactors landlock_add_rule syscall
landlock: user space API network support
landlock: adds support network rules
landlock: implements TCP network hooks
seltests/landlock: moves helper function
seltests/landlock: adds tests for bind() hooks
seltests/landlock: adds tests for connect() hooks
seltests/landlock: adds AF_UNSPEC family test
seltests/landlock: adds rules overlapping test
seltests/landlock: adds ruleset expanding test
seltests/landlock: adds invalid input data test
samples/landlock: adds network demo
include/uapi/linux/landlock.h | 49 ++
samples/landlock/sandboxer.c | 118 ++-
security/landlock/Kconfig | 1 +
security/landlock/Makefile | 2 +
security/landlock/fs.c | 162 +---
security/landlock/limits.h | 8 +-
security/landlock/net.c | 155 ++++
security/landlock/net.h | 26 +
security/landlock/ruleset.c | 448 +++++++++--
security/landlock/ruleset.h | 91 ++-
security/landlock/setup.c | 2 +
security/landlock/syscalls.c | 168 +++--
tools/testing/selftests/landlock/common.h | 10 +
tools/testing/selftests/landlock/config | 4 +
tools/testing/selftests/landlock/fs_test.c | 10 -
tools/testing/selftests/landlock/net_test.c | 774 ++++++++++++++++++++
16 files changed, 1737 insertions(+), 291 deletions(-)
create mode 100644 security/landlock/net.c
create mode 100644 security/landlock/net.h
create mode 100644 tools/testing/selftests/landlock/net_test.c
--
2.25.1
I did a thorough review of all the code. I found that the main issue
with this version is that we stick to the layers limit whereas it is
only relevant for filesystem hierarchies. You'll find in the following
patch miscellaneous fixes and improvement, with some TODOs to get rid of
this layer limit. We'll need a test to check that too. You'll need to
integrate this diff into your patches though.
You can find the related patch here:
https://git.kernel.org/mic/c/8f4104b3dc59e7f110c9b83cdf034d010a2d006f
Thinking more about the layer limit, I think you should keep your
changes and continue using ruleset.access_masks . Indeed, while it might
be a good thing to not be limited by the 16 layers, it would be an issue
with the upcoming audit feature, i.e. being able to point to the ruleset
responsible for a denied access. Here is a new patch (on top of the
other) to improve the current code:
https://git.kernel.org/mic/c/7d6cf40a6f81adf607ad3cc17aaa11e256beeea4
* Add a new access_masks_t for struct ruleset. This is now u16 but
would soon need to change to u32 (because of the upcoming
LANDLOCK_ACCESS_FS_TRUNCATE). Set back rules' access_masks_t to u16,
it should be enough for a while.
* Rename LANDLOCK_MASK_SHIFT_NET to LANDLOCK_SHIFT_ACCESS_NET and use
LANDLOCK_NUM_ACCESS_FS as value.
* Rename landlock_set_*_access_mask() to landlock_add_*_access_mask()
because it OR values.
* Make landlock_add_*_access_mask() more resilient incorrect values.
* Rename some variable to make them more consistent with the rest of the
code.
* Add and update kernel documentation.
* Remove unused code.
---
security/landlock/limits.h | 8 +-
security/landlock/ruleset.c | 31 +++-----
security/landlock/ruleset.h | 150 +++++++++++++++++++++---------------
3 files changed, 105 insertions(+), 84 deletions(-)
diff --git a/security/landlock/limits.h b/security/landlock/limits.h
index 660335258466..e50a12c1b797 100644
--- a/security/landlock/limits.h
+++ b/security/landlock/limits.h
@@ -21,15 +21,13 @@
#define LANDLOCK_LAST_ACCESS_FS LANDLOCK_ACCESS_FS_REFER
#define LANDLOCK_MASK_ACCESS_FS ((LANDLOCK_LAST_ACCESS_FS << 1) - 1)
#define LANDLOCK_NUM_ACCESS_FS __const_hweight64(LANDLOCK_MASK_ACCESS_FS)
+#define LANDLOCK_SHIFT_ACCESS_FS 0
#define LANDLOCK_LAST_ACCESS_NET LANDLOCK_ACCESS_NET_CONNECT_TCP
#define LANDLOCK_MASK_ACCESS_NET ((LANDLOCK_LAST_ACCESS_NET << 1) - 1)
#define LANDLOCK_NUM_ACCESS_NET __const_hweight64(LANDLOCK_MASK_ACCESS_NET)
-// TODO: LANDLOCK_MASK_SHIFT_NET will not be useful with the new
-// ruleset->net_access_mask
-#define LANDLOCK_MASK_SHIFT_NET 16
-
-#define LANDLOCK_RULE_TYPE_NUM LANDLOCK_RULE_NET_SERVICE
+#define LANDLOCK_SHIFT_ACCESS_NET LANDLOCK_NUM_ACCESS_FS
/* clang-format on */
+
#endif /* _SECURITY_LANDLOCK_LIMITS_H */
diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
index e7555b16069a..1b3433ea4c61 100644
--- a/security/landlock/ruleset.c
+++ b/security/landlock/ruleset.c
@@ -47,21 +47,21 @@ static struct landlock_ruleset *create_ruleset(const u32 num_layers)
}
struct landlock_ruleset *
-landlock_create_ruleset(const access_mask_t access_mask_fs,
- const access_mask_t access_mask_net)
+landlock_create_ruleset(const access_mask_t fs_access_mask,
+ const access_mask_t net_access_mask)
{
struct landlock_ruleset *new_ruleset;
/* Informs about useless ruleset. */
- if (!access_mask_fs && !access_mask_net)
+ if (!fs_access_mask && !net_access_mask)
return ERR_PTR(-ENOMSG);
new_ruleset = create_ruleset(1);
if (IS_ERR(new_ruleset))
return new_ruleset;
- if (access_mask_fs)
- landlock_set_fs_access_mask(new_ruleset, access_mask_fs, 0);
- if (access_mask_net)
- landlock_set_net_access_mask(new_ruleset, access_mask_net, 0);
+ if (fs_access_mask)
+ landlock_add_fs_access_mask(new_ruleset, fs_access_mask, 0);
+ if (net_access_mask)
+ landlock_add_net_access_mask(new_ruleset, net_access_mask, 0);
return new_ruleset;
}
@@ -160,13 +160,14 @@ static void build_check_ruleset(void)
.num_rules = ~0,
.num_layers = ~0,
};
- typeof(ruleset.access_masks[0]) fs_access_mask = ~0;
- typeof(ruleset.access_masks[0]) net_access_mask = ~0;
+ typeof(ruleset.access_masks[0]) access_masks = ~0;
BUILD_BUG_ON(ruleset.num_rules < LANDLOCK_MAX_NUM_RULES);
BUILD_BUG_ON(ruleset.num_layers < LANDLOCK_MAX_NUM_LAYERS);
- BUILD_BUG_ON(fs_access_mask < LANDLOCK_MASK_ACCESS_FS);
- BUILD_BUG_ON(net_access_mask < LANDLOCK_MASK_ACCESS_NET);
+ BUILD_BUG_ON(access_masks <
+ (LANDLOCK_MASK_ACCESS_FS << LANDLOCK_SHIFT_ACCESS_FS) +
+ (LANDLOCK_MASK_ACCESS_NET
+ << LANDLOCK_SHIFT_ACCESS_NET));
}
/**
@@ -250,8 +251,6 @@ static int insert_rule(struct landlock_ruleset *const ruleset,
* Intersects access rights when it is a merge between a
* ruleset and a domain.
*/
- // TODO: Don't create a new rule but AND accesses (of the first
- // and only layer) if !is_object_pointer(id.type)
new_rule = create_rule(id, &this->layers, this->num_layers,
&(*layers)[0]);
if (IS_ERR(new_rule))
@@ -332,7 +331,6 @@ static int merge_tree(struct landlock_ruleset *const dst,
rbtree_postorder_for_each_entry_safe(walker_rule, next_rule, src_root,
node) {
struct landlock_layer layers[] = { {
- // TODO: Set level to 1 if !is_object_pointer(key_type).
.level = dst->num_layers,
} };
const struct landlock_id id = {
@@ -531,7 +529,6 @@ landlock_merge_ruleset(struct landlock_ruleset *const parent,
if (parent->num_layers >= LANDLOCK_MAX_NUM_LAYERS)
return ERR_PTR(-E2BIG);
num_layers = parent->num_layers + 1;
- // TODO: Don't increment num_layers if RB_EMPTY_ROOT(&ruleset->root_inode).
} else {
num_layers = 1;
}
@@ -698,10 +695,6 @@ access_mask_t init_layer_masks(const struct landlock_ruleset *const domain,
switch (key_type) {
case LANDLOCK_KEY_INODE:
- // XXX: landlock_get_fs_access_mask() should not be removed
- // once we use ruleset->net_access_mask, and we can then
- // replace the @key_type argument with num_access to make the
- // code simpler.
get_access_mask = landlock_get_fs_access_mask;
num_access = LANDLOCK_NUM_ACCESS_FS;
break;
diff --git a/security/landlock/ruleset.h b/security/landlock/ruleset.h
index 59229be378d6..669de66094ed 100644
--- a/security/landlock/ruleset.h
+++ b/security/landlock/ruleset.h
@@ -19,8 +19,8 @@
#include "limits.h"
#include "object.h"
-// TODO: get back to u16 thanks to ruleset->net_access_mask
-typedef u32 access_mask_t;
+/* Rule access mask. */
+typedef u16 access_mask_t;
/* Makes sure all filesystem access rights can be stored. */
static_assert(BITS_PER_TYPE(access_mask_t) >= LANDLOCK_NUM_ACCESS_FS);
/* Makes sure all network access rights can be stored. */
@@ -28,6 +28,12 @@ static_assert(BITS_PER_TYPE(access_mask_t) >= LANDLOCK_NUM_ACCESS_NET);
/* Makes sure for_each_set_bit() and for_each_clear_bit() calls are OK. */
static_assert(sizeof(unsigned long) >= sizeof(access_mask_t));
+/* Ruleset access masks. */
+typedef u16 access_masks_t;
+/* Makes sure all ruleset access rights can be stored. */
+static_assert(BITS_PER_TYPE(access_masks_t) >=
+ LANDLOCK_NUM_ACCESS_FS + LANDLOCK_NUM_ACCESS_NET);
+
typedef u16 layer_mask_t;
/* Makes sure all layers can be checked. */
static_assert(BITS_PER_TYPE(layer_mask_t) >= LANDLOCK_MAX_NUM_LAYERS);
@@ -47,16 +53,33 @@ struct landlock_layer {
access_mask_t access;
};
+/**
+ * union landlock_key - Key of a ruleset's red-black tree
+ */
union landlock_key {
struct landlock_object *object;
uintptr_t data;
};
+/**
+ * enum landlock_key_type - Type of &union landlock_key
+ */
enum landlock_key_type {
+ /**
+ * @LANDLOCK_KEY_INODE: Type of &landlock_ruleset.root_inode's node
+ * keys.
+ */
LANDLOCK_KEY_INODE = 1,
+ /**
+ * @LANDLOCK_KEY_NET_PORT: Type of &landlock_ruleset.root_net_port's
+ * node keys.
+ */
LANDLOCK_KEY_NET_PORT = 2,
};
+/**
+ * struct landlock_id - Unique rule identifier for a ruleset
+ */
struct landlock_id {
union landlock_key key;
const enum landlock_key_type type;
@@ -113,15 +136,17 @@ struct landlock_hierarchy {
*/
struct landlock_ruleset {
/**
- * @root: Root of a red-black tree containing &struct landlock_rule
- * nodes. Once a ruleset is tied to a process (i.e. as a domain), this
- * tree is immutable until @usage reaches zero.
+ * @root_inode: Root of a red-black tree containing &struct
+ * landlock_rule nodes with inode object. 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_inode;
/**
- * @root_net_port: Root of a red-black tree containing object nodes
- * for network port. Once a ruleset is tied to a process (i.e. as a domain),
- * this tree is immutable until @usage reaches zero.
+ * @root_net_port: Root of a red-black tree containing &struct
+ * landlock_rule nodes with network port. Once a ruleset is tied to a
+ * process (i.e. as a domain), this tree is immutable until @usage
+ * reaches zero.
*/
struct rb_root root_net_port;
/**
@@ -162,32 +187,25 @@ struct landlock_ruleset {
*/
u32 num_layers;
/**
- * TODO: net_access_mask: Contains the subset of network
- * actions that are restricted by a ruleset.
- */
- access_mask_t net_access_mask;
- /**
- * @access_masks: Contains the subset of filesystem
- * 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
- * one. These layers are used when merging rulesets,
- * for user space backward compatibility (i.e.
- * future-proof), and to properly handle merged
+ * @access_masks: Contains the subset of filesystem and
+ * network 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 one. These layers are used when merging
+ * rulesets, for user space backward compatibility
+ * (i.e. future-proof), and to properly handle merged
* rulesets without overlapping access rights. These
* layers are set once and never changed for the
* lifetime of the ruleset.
*/
- // TODO: rename (back) to fs_access_mask because layers
- // are only useful for file hierarchies.
- access_mask_t access_masks[];
+ access_masks_t access_masks[];
};
};
};
struct landlock_ruleset *
-landlock_create_ruleset(const access_mask_t access_mask_fs,
- const access_mask_t access_mask_net);
+landlock_create_ruleset(const access_mask_t fs_access_mask,
+ const access_mask_t net_access_mask);
void landlock_put_ruleset(struct landlock_ruleset *const ruleset);
void landlock_put_ruleset_deferred(struct landlock_ruleset *const ruleset);
@@ -210,41 +228,7 @@ static inline void landlock_get_ruleset(struct landlock_ruleset *const ruleset)
refcount_inc(&ruleset->usage);
}
-// TODO: These helpers should not be required thanks to the new ruleset->net_access_mask.
-/* A helper function to set a filesystem mask. */
-static inline void
-landlock_set_fs_access_mask(struct landlock_ruleset *ruleset,
- const access_mask_t access_mask_fs, u16 mask_level)
-{
- ruleset->access_masks[mask_level] = access_mask_fs;
-}
-
-/* A helper function to get a filesystem mask. */
-static inline u32
-landlock_get_fs_access_mask(const struct landlock_ruleset *ruleset,
- u16 mask_level)
-{
- return (ruleset->access_masks[mask_level] & LANDLOCK_MASK_ACCESS_FS);
-}
-
-/* A helper function to set a network mask. */
-static inline void
-landlock_set_net_access_mask(struct landlock_ruleset *ruleset,
- const access_mask_t access_mask_net,
- u16 mask_level)
-{
- ruleset->access_masks[mask_level] |=
- (access_mask_net << LANDLOCK_MASK_SHIFT_NET);
-}
-
-/* A helper function to get a network mask. */
-static inline u32
-landlock_get_net_access_mask(const struct landlock_ruleset *ruleset,
- u16 mask_level)
-{
- return (ruleset->access_masks[mask_level] >> LANDLOCK_MASK_SHIFT_NET);
-}
-
+// TODO: Remove if only relevant for fs.c
access_mask_t get_handled_accesses(const struct landlock_ruleset *const domain,
const u16 rule_type, const u16 num_access);
@@ -258,4 +242,50 @@ access_mask_t init_layer_masks(const struct landlock_ruleset *const domain,
layer_mask_t (*const layer_masks)[],
const enum landlock_key_type key_type);
+static inline void
+landlock_add_fs_access_mask(struct landlock_ruleset *const ruleset,
+ const access_mask_t fs_access_mask,
+ const u16 layer_level)
+{
+ access_mask_t fs_mask = fs_access_mask & LANDLOCK_MASK_ACCESS_FS;
+
+ /* Should already be checked in sys_landlock_create_ruleset(). */
+ WARN_ON_ONCE(fs_access_mask != fs_mask);
+ // TODO: Add tests to check "|=" and not "="
+ ruleset->access_masks[layer_level] |=
+ (fs_mask << LANDLOCK_SHIFT_ACCESS_FS);
+}
+
+static inline void
+landlock_add_net_access_mask(struct landlock_ruleset *const ruleset,
+ const access_mask_t net_access_mask,
+ const u16 layer_level)
+{
+ access_mask_t net_mask = net_access_mask & LANDLOCK_MASK_ACCESS_NET;
+
+ /* Should already be checked in sys_landlock_create_ruleset(). */
+ WARN_ON_ONCE(net_access_mask != net_mask);
+ // TODO: Add tests to check "|=" and not "="
+ ruleset->access_masks[layer_level] |=
+ (net_mask << LANDLOCK_SHIFT_ACCESS_NET);
+}
+
+static inline access_mask_t
+landlock_get_fs_access_mask(const struct landlock_ruleset *const ruleset,
+ const u16 layer_level)
+{
+ return (ruleset->access_masks[layer_level] >>
+ LANDLOCK_SHIFT_ACCESS_FS) &
+ LANDLOCK_MASK_ACCESS_FS;
+}
+
+static inline access_mask_t
+landlock_get_net_access_mask(const struct landlock_ruleset *const ruleset,
+ const u16 layer_level)
+{
+ return (ruleset->access_masks[layer_level] >>
+ LANDLOCK_SHIFT_ACCESS_NET) &
+ LANDLOCK_MASK_ACCESS_NET;
+}
+
#endif /* _SECURITY_LANDLOCK_RULESET_H */
