From: "Mickaël Salaün" <mic@digikod.net>
To: "Konstantin Meskhidze (A)" <konstantin.meskhidze@huawei.com>
Cc: willemdebruijn.kernel@gmail.com,
linux-security-module@vger.kernel.org, netdev@vger.kernel.org,
netfilter-devel@vger.kernel.org, anton.sirazetdinov@huawei.com
Subject: Re: [PATCH v6 00/17] Network support for Landlock
Date: Mon, 29 Aug 2022 15:10:12 +0200 [thread overview]
Message-ID: <f11b7754-b879-20be-0b22-94d94a68de71@digikod.net> (raw)
In-Reply-To: <8eb6509f-8e79-d75c-08f4-80f52c0a26e7@huawei.com>
On 27/08/2022 15:30, Konstantin Meskhidze (A) wrote:
>
>
> 7/28/2022 4:17 PM, Mickaël Salaün пишет:
[...]
>> 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 "=" > Is it kunit test? If so, do you want to add this kind of tests in future
> landlock versions?
In this sixth patch series, landlock_set_fs_access_mask() was replacing
the content of access_masks[] whereas landlock_set_net_access_mask() was
ORing it. It didn't lead to a bug because landlock_set_fs_access_mask()
was called before landlock_set_net_access_mask(), but it was brittle.
Anyway, it was a good reminder to add a test to check that filesystem
and network restrictions work well together. This can be added as a
basic filesystem test using a ruleset handling network restrictions but
no network rule (in fs_test.c), and as a basic network test using a
ruleset handling filesystem restrictions but no filestem rule (in
net_test.c).
This could also be part of a kunit test in the future.
>> + 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 "="
> The same above.
> I'm going add invalid network attribute checking into TEST_F(socket,
> inval) test in coming patch.
Good
next prev parent reply other threads:[~2022-08-29 13:11 UTC|newest]
Thread overview: 53+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-06-21 8:22 [PATCH v6 00/17] Network support for Landlock Konstantin Meskhidze
2022-06-21 8:22 ` [PATCH v6 01/17] landlock: renames access mask Konstantin Meskhidze
2022-07-01 17:08 ` Mickaël Salaün
2022-07-04 9:23 ` Konstantin Meskhidze (A)
2022-07-05 11:29 ` Konstantin Meskhidze (A)
2022-07-05 13:26 ` Mickaël Salaün
2022-07-08 12:56 ` Konstantin Meskhidze (A)
2022-06-21 8:22 ` [PATCH v6 02/17] landlock: refactors landlock_find/insert_rule Konstantin Meskhidze
2022-07-07 16:44 ` Mickaël Salaün
2022-07-08 12:53 ` Konstantin Meskhidze (A)
2022-07-08 13:56 ` Mickaël Salaün
2022-07-08 14:14 ` Konstantin Meskhidze (A)
2022-07-08 14:20 ` Konstantin Meskhidze (A)
2022-07-08 16:57 ` Mickaël Salaün
2022-07-11 8:16 ` Konstantin Meskhidze (A)
2022-07-08 13:10 ` Konstantin Meskhidze (A)
2022-07-08 13:59 ` Mickaël Salaün
2022-07-08 14:14 ` Konstantin Meskhidze (A)
2022-07-08 14:35 ` Mickaël Salaün
2022-07-11 14:05 ` Konstantin Meskhidze (A)
2022-07-28 14:48 ` Mickaël Salaün
2022-07-07 16:46 ` Mickaël Salaün
2022-07-08 12:54 ` Konstantin Meskhidze (A)
2022-06-21 8:22 ` [PATCH v6 03/17] landlock: refactors merge and inherit functions Konstantin Meskhidze
2022-06-21 8:23 ` [PATCH v6 04/17] landlock: moves helper functions Konstantin Meskhidze
2022-06-21 8:23 ` [PATCH v6 05/17] landlock: refactors " Konstantin Meskhidze
2022-06-21 8:23 ` [PATCH v6 06/17] landlock: refactors landlock_add_rule syscall Konstantin Meskhidze
2022-06-21 8:23 ` [PATCH v6 07/17] landlock: user space API network support Konstantin Meskhidze
2022-06-21 8:23 ` [PATCH v6 08/17] landlock: adds support network rules Konstantin Meskhidze
2022-06-21 8:23 ` [PATCH v6 09/17] landlock: implements TCP network hooks Konstantin Meskhidze
2022-06-21 8:23 ` [PATCH v6 10/17] seltests/landlock: moves helper function Konstantin Meskhidze
2022-06-21 8:23 ` [PATCH v6 11/17] seltests/landlock: adds tests for bind() hooks Konstantin Meskhidze
2022-07-28 13:24 ` Mickaël Salaün
2022-06-21 8:23 ` [PATCH v6 12/17] seltests/landlock: adds tests for connect() hooks Konstantin Meskhidze
2022-06-21 8:23 ` [PATCH v6 13/17] seltests/landlock: adds AF_UNSPEC family test Konstantin Meskhidze
2022-06-21 8:23 ` [PATCH v6 14/17] seltests/landlock: adds rules overlapping test Konstantin Meskhidze
2022-06-21 8:23 ` [PATCH v6 15/17] seltests/landlock: adds ruleset expanding test Konstantin Meskhidze
2022-06-21 8:23 ` [PATCH v6 16/17] seltests/landlock: adds invalid input data test Konstantin Meskhidze
2022-06-21 8:23 ` [PATCH v6 17/17] samples/landlock: adds network demo Konstantin Meskhidze
2022-07-27 20:26 ` Mickaël Salaün
2022-07-28 9:21 ` Konstantin Meskhidze (A)
2022-07-26 17:43 ` [PATCH v6 00/17] Network support for Landlock Mickaël Salaün
2022-07-27 19:54 ` Mickaël Salaün
2022-07-28 9:19 ` Konstantin Meskhidze (A)
2022-07-28 9:25 ` Konstantin Meskhidze (A)
2022-07-28 10:12 ` Mickaël Salaün
2022-07-28 11:27 ` Konstantin Meskhidze (A)
2022-07-28 13:17 ` Mickaël Salaün
2022-08-23 9:10 ` Konstantin Meskhidze (A)
2022-08-27 13:30 ` Konstantin Meskhidze (A)
2022-08-29 13:10 ` Mickaël Salaün [this message]
2022-07-27 20:21 ` Mickaël Salaün
2022-07-28 9:20 ` Konstantin Meskhidze (A)
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=f11b7754-b879-20be-0b22-94d94a68de71@digikod.net \
--to=mic@digikod.net \
--cc=anton.sirazetdinov@huawei.com \
--cc=konstantin.meskhidze@huawei.com \
--cc=linux-security-module@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=netfilter-devel@vger.kernel.org \
--cc=willemdebruijn.kernel@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).