[PATCH v6 00/17] Network support for Landlock
Mickaël Salaün
mic at digikod.net
Thu Jul 28 13:17:43 UTC 2022
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@huawei.com
>>> v4:
>>> https://lore.kernel.org/linux-security-module/20220309134459.6448-1-konstantin.meskhidze@huawei.com/
>>> v3:
>>> https://lore.kernel.org/linux-security-module/20220124080215.265538-1-konstantin.meskhidze@huawei.com/
>>> v2:
>>> https://lore.kernel.org/linux-security-module/20211228115212.703084-1-konstantin.meskhidze@huawei.com/
>>> v1:
>>> https://lore.kernel.org/linux-security-module/20211210072123.386713-1-konstantin.meskhidze@huawei.com/
>>>
>>> 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 */
More information about the Linux-security-module-archive
mailing list