landlock: Refactor layer masks
Subsystems (2)
cover landlock: Refactor layer masks
Hello!
This patch set "transposes" the layer masks matrix, which was previously modeled as a access-max-sized array of layer masks, and changes it to be a layer-max-sized array of access masks instead. (It is a pure refactoring, there are no user-visible changes.)
This unlocks a few code simplifications and in multiple places it removes the need for loops and branches that deal with individual bits. Instead, the changed data structure now lends itself for more bitwise operations. The underlying hypothesis for me was that by using more bitwise operations and fewer branches, we would get an overall speedup even when the data structure size increases slightly in some cases.
Tentative results with and without this patch set show that the hypothesis likely holds true. The benchmark I used exercises a "worst case" scenario that attempts to be bottlenecked on the affected code:
constructs a large number of nested directories, with one "path beneath" rule each and then tries to open the innermost directory many times. The benchmark is intentionally unrealistic to amplify the amount of time used for the path walk logic and forces Landlock to walk the full path (eventually failing the open syscall). (I'll send the benchmark program in a reply to this mail for full transparency.)
Measured with the benchmark program, the patch set results in a speedup of -8.3%. The benchmark results are only tentative and have been produced in Qemu:
With the patch, the benchmark runs in 5932 clocks (measured with times(3)):
*** Benchmark ***
10000 dirs, 100000 iterations, with landlock
*** Benchmark concluded ***
System: 5932 clocks
User : 1 clocks
Clocks per second: 1000000
Without the patch, we get 6472 clocks, which is 9.1% more.
*** Benchmark ***
10000 dirs, 100000 iterations, with landlock
*** Benchmark concluded ***
System: 6472 clocks
User : 1 clocks
Clocks per second: 1000000
The base revision used for benchmarking was commit 7a51784da76d ("tools/sched_ext: update scx_show_state.py for scx_aborting change")
In real-life scenarios, the speed improvement from this patch set will be less pronounced than in the artificial benchmark, as people do not usually stack directories that deeply and attach so many rules to them, and the EACCES error should also be the exception rather than the norm.
I am looking forward to your feedback.
P.S.: I am open to suggestions on what the "layer masks" variables should be called, because the name "layer masks" might be less appropriate after this change. I have not fixed up the name everywhere because fixing up the code took priority for now.
Günther Noack (2):
landlock: access_mask_subset() helper
landlock: transpose the layer masks data structure
security/landlock/access.h | 10 +- security/landlock/audit.c | 155 ++++++---------- security/landlock/audit.h | 3 +- security/landlock/domain.c | 120 +++--------- security/landlock/domain.h | 6 +- security/landlock/fs.c | 361 +++++++++++++++++------------------- security/landlock/net.c | 10 +- security/landlock/ruleset.c | 78 ++------ security/landlock/ruleset.h | 18 +- 9 files changed, 300 insertions(+), 461 deletions(-)
Signature · 1 line
2.52.0
RFC PATCH 1/2 landlock: access_mask_subset() helper
This helper function checks whether an access_mask_t has a subset of the bits enabled than another one. This expresses the intent a bit smoother in the code and does not cost us anything when it gets inlined.
--- security/landlock/fs.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)
security/landlock/fs.c +10 -1
@@ -398,6 +398,15 @@ static const struct access_masks any_fs = { .fs = ~0, }; +/* +* Returns true iff a has a subset of the bits of b. +* It helps readability and gets inlined. +*/ +static bool access_mask_subset (access_mask_t a,access_mask_t b) +{ +return (a | b) == b; +} +/* * Check that a destination file hierarchy has more restrictions than a source * file hierarchy. This is only used for link and rename actions.
@@ -1696,7 +1705,7 @@ static int hook_file_open(struct file *const file) ARRAY_SIZE(layer_masks));#endif /* CONFIG_AUDIT */ -if ((open_access_request & allowed_access) == open_access_request) +if (access_mask_subset (open_access_request, allowed_access))return 0 ;/* Sets access to reflect the actual request. */
Signature · 1 line
2.52.0
Mickaël Salaün
maintainerRe: [RFC PATCH 1/2] landlock: access_mask_subset() helper
Quoted 14 lines · Günther Noack
This helper function checks whether an
access_mask_thas a subset of the bits enabled than another one. This expresses the intent a bit smoother in the code and does not cost us anything when it gets inlined.Signed-off-by Günther Noack <[email protected]>--- security/landlock/fs.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)security/landlock/fs.c +0
-0
security/landlock/fs.c static const struct access_masks any_fs = { · line 407
Quoted 10 lines
security/landlock/fs.c +7
-0@@ -398,6 +398,15 @@ static const struct access_masks any_fs = { .fs = ~0, }; +/* +* Returns true iff a has a subset of the bits of b. +* It helps readability and gets inlined. +*/ +static bool access_mask_subset (access_mask_t a,access_mask_t b) +{ +return (a | b) == b;
I'm curious about why this switches to a binary OR instead of the original AND.
Quoted 18 lines
security/landlock/fs.c +3
-1+} +/* * Check that a destination file hierarchy has more restrictions than a source * file hierarchy. This is only used for link and rename actions. @@ -1696,7 +1705,7 @@ static int hook_file_open(struct file *const file) ARRAY_SIZE(layer_masks));#endif /* CONFIG_AUDIT */ -if ((open_access_request & allowed_access) == open_access_request) +if (access_mask_subset (open_access_request, allowed_access))return 0 ;/* Sets access to reflect the actual request. */ Signature · 1 line
2.52.0
Re: [RFC PATCH 1/2] landlock: access_mask_subset() helper
Quoted 18 lines · Mickaël Salaün
Quoted 17 lines · Günther Noack
This helper function checks whether an
access_mask_thas a subset of the bits enabled than another one. This expresses the intent a bit smoother in the code and does not cost us anything when it gets inlined.Signed-off-by Günther Noack <[email protected]>--- security/landlock/fs.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)security/landlock/fs.c +0
-0@@ -398,6 +398,15 @@ static const struct access_masks any_fs = { .fs = ~0, };
Quoted 10 lines
security/landlock/fs.c static const struct access_masks any_fs = { · line 407
Quoted 7 lines
security/landlock/fs.c +7
-0+/* +* Returns true iff a has a subset of the bits of b. +* It helps readability and gets inlined. +*/ +static bool access_mask_subset (access_mask_t a,access_mask_t b) +{ +return (a | b) == b;I'm curious about why this switches to a binary OR instead of the original AND.
It is slightly more intuitive to me, but other than that, no specific reason. (a | b) == b has the same results as (b & a) == a.
I'm not feeling strongly about it.
We can also do it the other way around if you prefer.
–Günther
RFC PATCH 2/2 landlock: transpose the layer masks data structure
In reply to Günther NoackThe layer masks data structure tracks the requested but unfulfilled access rights during an operations security check. It stores one bit for each combination of access right and layer index. If the bit is set, that access right is not granted (yet) in the given layer and we have to traverse the path further upwards to grant it.
Previously, the layer masks were stored as arrays mapping from access right indices to layer_mask_t. The layer_mask_t value then indicates all layers in which the given access right is still (tentatively) denied.
This patch introduces struct layer_access_masks instead: This struct contains an array with the access_mask_t of each (tentatively) denied access right in that layer.
The hypothesis of this patch is that this simplifies the code enough so that the resulting code will run faster:
* We can use bitwise operations in multiple places where we previously looped over bits individually with macros. (Should require less branch speculation)
* Code is ~160 lines smaller.
Other noteworthy changes:
* Clarify deny_mask_t and the code assembling it.
* Document what that value looks like
* Make writing and reading functions specific to file system rules. (It only worked for FS rules before as well, but going all the way simplifies the code logic more.)
* In no_more_access(), call a new helper function may_refer(), which only solves the asymmetric case. Previously, the code interleaved the checks for the two symmetric cases in RENAME_EXCHANGE. It feels that the code is clearer when renames without RENAME_EXCHANGE are more obviously the normal case.
--- security/landlock/access.h | 10 +- security/landlock/audit.c | 155 ++++++---------- security/landlock/audit.h | 3 +- security/landlock/domain.c | 120 +++---------- security/landlock/domain.h | 6 +- security/landlock/fs.c | 350 ++++++++++++++++-------------------- security/landlock/net.c | 10 +- security/landlock/ruleset.c | 78 +++----- security/landlock/ruleset.h | 18 +- 9 files changed, 290 insertions(+), 460 deletions(-)
security/landlock/access.h +5 -5
@@ -61,14 +61,14 @@ union access_masks_all {static_assert (sizeof (typeof_member (union access_masks_all ,masks )) ==sizeof (typeof_member (union access_masks_all ,all ))); -typedef u16 layer_mask_t ; - -/* Makes sure all layers can be checked. */ -static_assert (BITS_PER_TYPE (layer_mask_t) >=LANDLOCK_MAX_NUM_LAYERS ); -/* * Tracks domains responsible of a denied access. This is required to avoid * storing in each object the full layer_masks[] required by update_request(). +* +* Each nibble represents the layer index of the newest layer which denied a +* certain access right. For file system access rights, the upper four bits are +* the index of the layer which denies LANDLOCK_ACCESS_FS_IOCTL_DEV and the +* lower nibble represents LANDLOCK_ACCESS_FS_TRUNCATE. */ typedef u8 deny_masks_t ;
security/landlock/audit.c +56 -99
257 lines not on this page · raw message
security/landlock/audit.h +1 -2
@@ -43,8 +43,7 @@ struct landlock_request {access_mask_t access;/* Required fields for requests with layer masks. */ -const layer_mask_t (*layer_masks)[]; -size_t layer_masks_size; +const struct layer_access_masks *masks;/* Required fields for requests with deny masks. */ const access_mask_t all_existing_optional_access;
security/landlock/domain.c +28 -92
153 lines not on this page · raw message
security/landlock/domain.h +2 -4
@@ -120,10 +120,8 @@ struct landlock_hierarchy {#ifdef CONFIG_AUDIT deny_masks_t -landlock_get_deny_masks (const access_mask_t all_existing_optional_access, -const access_mask_t optional_access, -const layer_mask_t (*const layer_masks)[], -size_t layer_masks_size); +landlock_get_fs_deny_masks (const access_mask_t optional_access, +const struct layer_access_masks *layer_masks);int landlock_init_hierarchy_log (struct landlock_hierarchy *const hierarchy);
security/landlock/fs.c +159 -191
536 lines not on this page · raw message
security/landlock/net.c +5 -5
@@ -47,7 +47,7 @@ static int current_check_access_socket(struct socket *const sock, access_mask_t access_request) {__be16 port; -layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_NET ] = {}; +struct layer_access_masks layer_masks = {};const struct landlock_rule *rule;struct landlock_id id = { .type =LANDLOCK_KEY_NET_PORT ,
@@ -178,8 +178,9 @@ static int current_check_access_socket(struct socket *const sock, access_request =landlock_init_layer_masks (subject->domain, access_request, &layer_masks,LANDLOCK_KEY_NET_PORT ); -if (landlock_unmask_layers (rule, access_request, &layer_masks, -ARRAY_SIZE (layer_masks))) +if (!access_request) +return 0 ; +if (landlock_unmask_layers (rule, &layer_masks))return 0 ; audit_net.family = address->sa_family;
@@ -189,8 +190,7 @@ static int current_check_access_socket(struct socket *const sock, .audit.type = LSM_AUDIT_DATA_NET, .audit.u.net = &audit_net, .access = access_request, - .layer_masks = &layer_masks, - .layer_masks_size = ARRAY_SIZE(layer_masks), + .masks = &layer_masks, });return -EACCES ; }
security/landlock/ruleset.c +20 -58
127 lines not on this page · raw message
security/landlock/ruleset.h +14 -4
@@ -301,15 +301,25 @@ landlock_get_scope_mask(const struct landlock_ruleset *const ruleset, return ruleset->access_masks[layer_level].scope; } +/** +* struct layer_accesses - A boolean matrix of layers and access rights +* +* This has a bit for each combination of layer numbers and access rights. +* During access checks, it is used to represent the access rights for each +* layer which still need to be fulfilled. When all bits are 0, the access +* request is considered to be fulfilled. +*/ +struct layer_access_masks { +access_mask_t access[LANDLOCK_MAX_NUM_LAYERS ]; +}; +bool landlock_unmask_layers (const struct landlock_rule *const rule, -const access_mask_t access_request, -layer_mask_t (*const layer_masks)[], -const size_t masks_array_size); +struct layer_access_masks *masks);access_mask_t landlock_init_layer_masks (const struct landlock_ruleset *const domain,const access_mask_t access_request, -layer_mask_t (*const layer_masks)[], +struct layer_access_masks *masks,const enum landlock_key_type key_type);#endif /* _SECURITY_LANDLOCK_RULESET_H */
Signature · 1 line
2.52.0
Re: [RFC PATCH 2/2] landlock: transpose the layer masks data structure
Quoted 26 lines · Günther Noack
The layer masks data structure tracks the requested but unfulfilled access rights during an operations security check. It stores one bit for each combination of access right and layer index. If the bit is set, that access right is not granted (yet) in the given layer and we have to traverse the path further upwards to grant it.
Previously, the layer masks were stored as arrays mapping from access right indices to
layer_mask_t. Thelayer_mask_tvalue then indicates all layers in which the given access right is still (tentatively) denied.This patch introduces struct
layer_access_masksinstead: This struct contains an array with theaccess_mask_tof each (tentatively) denied access right in that layer.The hypothesis of this patch is that this simplifies the code enough so that the resulting code will run faster:
* We can use bitwise operations in multiple places where we previously looped over bits individually with macros. (Should require less branch speculation)
* Code is ~160 lines smaller.
Other noteworthy changes:
Quoted 10 lines
* Clarify
deny_mask_tand the code assembling it.
* Document what that value looks like
* Make writing and reading functions specific to file system rules. (It only worked for FS rules before as well, but going all the way simplifies the code logic more.)
* Inno_more_access(), call a new helper functionmay_refer(), which only solves the asymmetric case. Previously, the code interleaved the checks for the two symmetric cases inRENAME_EXCHANGE. It feels that the code is clearer when renames withoutRENAME_EXCHANGEare more obviously the normal case.
I was able to reproduce your benchmark, with slightly slower but very close results.
Definite improvement.
I was also doing it from QEMU, so there might be some confounding variables.
Great job.
Quoted 658 lines
Signed-off-by Günther Noack <[email protected]>--- security/landlock/access.h | 10 +- security/landlock/audit.c | 155 ++++++---------- security/landlock/audit.h | 3 +- security/landlock/domain.c | 120 +++---------- security/landlock/domain.h | 6 +- security/landlock/fs.c | 350 ++++++++++++++++-------------------- security/landlock/net.c | 10 +- security/landlock/ruleset.c | 78 +++----- security/landlock/ruleset.h | 18 +- 9 files changed, 290 insertions(+), 460 deletions(-)security/landlock/access.h +5
-5@@ -61,14 +61,14 @@ union access_masks_all {static_assert (sizeof (typeof_member (union access_masks_all ,masks )) ==sizeof (typeof_member (union access_masks_all ,all ))); -typedef u16 layer_mask_t ; - -/* Makes sure all layers can be checked. */ -static_assert (BITS_PER_TYPE (layer_mask_t) >=LANDLOCK_MAX_NUM_LAYERS ); -/* * Tracks domains responsible of a denied access. This is required to avoid * storing in each object the full layer_masks[] required by update_request(). +* +* Each nibble represents the layer index of the newest layer which denied a +* certain access right. For file system access rights, the upper four bits are +* the index of the layer which denies LANDLOCK_ACCESS_FS_IOCTL_DEV and the +* lower nibble represents LANDLOCK_ACCESS_FS_TRUNCATE. */ typedef u8 deny_masks_t ;security/landlock/audit.c +56
-99@@ -182,36 +182,18 @@ static void test_get_hierarchy(struct kunit *const test)static size_t get_denied_layer (const struct landlock_ruleset *const domain,access_mask_t *const access_request, -const layer_mask_t (*const layer_masks)[], -const size_t layer_masks_size) +const struct layer_access_masks *masks) { -const unsigned long access_req = *access_request; -unsigned long access_bit; -access_mask_t missing =0 ; -long youngest_layer =-1 ; - -for_each_set_bit (access_bit, &access_req, layer_masks_size) { -const access_mask_t mask = (*layer_masks)[access_bit]; -long layer; - -if (!mask) -continue ; - -/* __fls(1) == 0 */ - layer =__fls (mask); -if (layer > youngest_layer) { - youngest_layer = layer; - missing =BIT (access_bit); - }else if (layer == youngest_layer) { - missing |=BIT (access_bit); +for (int i =LANDLOCK_MAX_NUM_LAYERS -1 ; i >=0 ; i--) { +if (masks->access[i] & *access_request) { + *access_request &= masks->access[i]; +return i; } } - *access_request = missing; -if (youngest_layer ==-1 ) -return domain->num_layers -1 ; - -return youngest_layer; +/* Not found - fall back to default values */ + *access_request =0 ; +return domain->num_layers -1 ; }#ifdef CONFIG_SECURITY_LANDLOCK_KUNIT_TEST @@ -221,94 +203,82 @@ static void test_get_denied_layer(struct kunit *const test)const struct landlock_ruleset dom = { .num_layers =5 , }; -const layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_FS ] = { - [BIT_INDEX (LANDLOCK_ACCESS_FS_EXECUTE )] =BIT (0 ), - [BIT_INDEX (LANDLOCK_ACCESS_FS_READ_FILE )] =BIT (1 ), - [BIT_INDEX (LANDLOCK_ACCESS_FS_READ_DIR )] =BIT (1 ) |BIT (0 ), - [BIT_INDEX (LANDLOCK_ACCESS_FS_REMOVE_DIR )] =BIT (2 ), +const struct layer_access_masks masks = { + .access[0 ] =LANDLOCK_ACCESS_FS_EXECUTE | +LANDLOCK_ACCESS_FS_READ_DIR , + .access[1 ] =LANDLOCK_ACCESS_FS_READ_FILE | +LANDLOCK_ACCESS_FS_READ_DIR , + .access[2 ] =LANDLOCK_ACCESS_FS_REMOVE_DIR , };access_mask_t access; access =LANDLOCK_ACCESS_FS_EXECUTE ; -KUNIT_EXPECT_EQ (test,0 , -get_denied_layer (&dom, &access, &layer_masks, -sizeof (layer_masks))); +KUNIT_EXPECT_EQ (test,0 ,get_denied_layer (&dom, &access, &masks));KUNIT_EXPECT_EQ (test, access,LANDLOCK_ACCESS_FS_EXECUTE ); access =LANDLOCK_ACCESS_FS_READ_FILE ; -KUNIT_EXPECT_EQ (test,1 , -get_denied_layer (&dom, &access, &layer_masks, -sizeof (layer_masks))); +KUNIT_EXPECT_EQ (test,1 ,get_denied_layer (&dom, &access, &masks));KUNIT_EXPECT_EQ (test, access,LANDLOCK_ACCESS_FS_READ_FILE ); access =LANDLOCK_ACCESS_FS_READ_DIR ; -KUNIT_EXPECT_EQ (test,1 , -get_denied_layer (&dom, &access, &layer_masks, -sizeof (layer_masks))); +KUNIT_EXPECT_EQ (test,1 ,get_denied_layer (&dom, &access, &masks));KUNIT_EXPECT_EQ (test, access,LANDLOCK_ACCESS_FS_READ_DIR ); access =LANDLOCK_ACCESS_FS_READ_FILE |LANDLOCK_ACCESS_FS_READ_DIR ; -KUNIT_EXPECT_EQ (test,1 , -get_denied_layer (&dom, &access, &layer_masks, -sizeof (layer_masks))); +KUNIT_EXPECT_EQ (test,1 ,get_denied_layer (&dom, &access, &masks));KUNIT_EXPECT_EQ (test, access,LANDLOCK_ACCESS_FS_READ_FILE |LANDLOCK_ACCESS_FS_READ_DIR ); access =LANDLOCK_ACCESS_FS_EXECUTE |LANDLOCK_ACCESS_FS_READ_DIR ; -KUNIT_EXPECT_EQ (test,1 , -get_denied_layer (&dom, &access, &layer_masks, -sizeof (layer_masks))); +KUNIT_EXPECT_EQ (test,1 ,get_denied_layer (&dom, &access, &masks));KUNIT_EXPECT_EQ (test, access,LANDLOCK_ACCESS_FS_READ_DIR ); access =LANDLOCK_ACCESS_FS_WRITE_FILE ; -KUNIT_EXPECT_EQ (test,4 , -get_denied_layer (&dom, &access, &layer_masks, -sizeof (layer_masks))); +KUNIT_EXPECT_EQ (test,4 ,get_denied_layer (&dom, &access, &masks));KUNIT_EXPECT_EQ (test, access,0 ); }#endif /* CONFIG_SECURITY_LANDLOCK_KUNIT_TEST */ -static size_t -get_layer_from_deny_masks (access_mask_t *const access_request, -const access_mask_t all_existing_optional_access, -const deny_masks_t deny_masks) +/* +* get_layer_from_fs_deny_masks - get the layer which denied the access request +* +* As a side effect, stores the denied access rights from that layer(!) in +* *access_request. +*/ +static size_t get_layer_from_fs_deny_masks (access_mask_t *const access_request, +const deny_masks_t deny_masks) { -const unsigned long access_opt = all_existing_optional_access; -const unsigned long access_req = *access_request; -access_mask_t missing =0 ; +const access_mask_t access_req = *access_request;size_t youngest_layer =0 ; -size_t access_index =0 ; -unsigned long access_bit; +access_mask_t missing =0 ; -/* This will require change with new object types. */ -WARN_ON_ONCE (access_opt != _LANDLOCK_ACCESS_FS_OPTIONAL); +WARN_ON_ONCE ((access_req | _LANDLOCK_ACCESS_FS_OPTIONAL) != + _LANDLOCK_ACCESS_FS_OPTIONAL); -for_each_set_bit (access_bit, &access_opt, -BITS_PER_TYPE (access_mask_t)) { -if (access_req &BIT (access_bit)) { -const size_t layer = - (deny_masks >> (access_index *4 )) & - (LANDLOCK_MAX_NUM_LAYERS -1 ); +if (access_req &LANDLOCK_ACCESS_FS_TRUNCATE ) { +size_t layer = deny_masks &0x0f ; -if (layer > youngest_layer) { - youngest_layer = layer; - missing =BIT (access_bit); - }else if (layer == youngest_layer) { - missing |=BIT (access_bit); - } - } - access_index++; + missing |=LANDLOCK_ACCESS_FS_TRUNCATE ; + youngest_layer =max (youngest_layer, layer); } +if (access_req &LANDLOCK_ACCESS_FS_IOCTL_DEV ) { +size_t layer = (deny_masks &0xf0 ) >>4 ; +if (layer > youngest_layer) + missing =0 ; + + missing |=LANDLOCK_ACCESS_FS_IOCTL_DEV ; + youngest_layer =max (youngest_layer, layer); + } *access_request = missing;return youngest_layer; }#ifdef CONFIG_SECURITY_LANDLOCK_KUNIT_TEST -static void test_get_layer_from_deny_masks (struct kunit *const test) +static void test_get_layer_from_fs_deny_masks (struct kunit *const test) {deny_masks_t deny_mask;access_mask_t access;@@ -318,16 +288,12 @@ static void test_get_layer_from_deny_masks(struct kunit *const test) access =LANDLOCK_ACCESS_FS_TRUNCATE ;KUNIT_EXPECT_EQ (test,0 , -get_layer_from_deny_masks (&access, - _LANDLOCK_ACCESS_FS_OPTIONAL, - deny_mask)); +get_layer_from_fs_deny_masks (&access, deny_mask));KUNIT_EXPECT_EQ (test, access,LANDLOCK_ACCESS_FS_TRUNCATE ); access =LANDLOCK_ACCESS_FS_TRUNCATE |LANDLOCK_ACCESS_FS_IOCTL_DEV ;KUNIT_EXPECT_EQ (test,2 , -get_layer_from_deny_masks (&access, - _LANDLOCK_ACCESS_FS_OPTIONAL, - deny_mask)); +get_layer_from_fs_deny_masks (&access, deny_mask));KUNIT_EXPECT_EQ (test, access,LANDLOCK_ACCESS_FS_IOCTL_DEV );/* truncate:15 ioctl_dev:15 */ @@ -335,16 +301,12 @@ static void test_get_layer_from_deny_masks(struct kunit *const test) access =LANDLOCK_ACCESS_FS_TRUNCATE ;KUNIT_EXPECT_EQ (test,15 , -get_layer_from_deny_masks (&access, - _LANDLOCK_ACCESS_FS_OPTIONAL, - deny_mask)); +get_layer_from_fs_deny_masks (&access, deny_mask));KUNIT_EXPECT_EQ (test, access,LANDLOCK_ACCESS_FS_TRUNCATE ); access =LANDLOCK_ACCESS_FS_TRUNCATE |LANDLOCK_ACCESS_FS_IOCTL_DEV ;KUNIT_EXPECT_EQ (test,15 , -get_layer_from_deny_masks (&access, - _LANDLOCK_ACCESS_FS_OPTIONAL, - deny_mask)); +get_layer_from_fs_deny_masks (&access, deny_mask));KUNIT_EXPECT_EQ (test, access,LANDLOCK_ACCESS_FS_TRUNCATE |LANDLOCK_ACCESS_FS_IOCTL_DEV );@@ -361,18 +323,15 @@ static bool is_valid_request(const struct landlock_request *const request)return false;if (request->access) { -if (WARN_ON_ONCE (!(!!request->layer_masks ^ +if (WARN_ON_ONCE (!(!!request->masks ^ !!request->all_existing_optional_access)))return false; }else { -if (WARN_ON_ONCE (request->layer_masks || +if (WARN_ON_ONCE (request->masks || request->all_existing_optional_access))return false; } -if (WARN_ON_ONCE (!!request->layer_masks ^ !!request->layer_masks_size)) -return false; -if (request->deny_masks) {if (WARN_ON_ONCE (!request->all_existing_optional_access))return false;@@ -405,14 +364,12 @@ void landlock_log_denial(const struct landlock_cred_security *const subject, missing = request->access;if (missing) {/* Gets the nearest domain that denies the request. */ -if (request->layer_masks) { +if (request->masks) { youngest_layer =get_denied_layer ( - subject->domain, &missing, request->layer_masks, - request->layer_masks_size); + subject->domain, &missing, request->masks); }else { - youngest_layer =get_layer_from_deny_masks ( - &missing, request->all_existing_optional_access, - request->deny_masks); + youngest_layer =get_layer_from_fs_deny_masks ( + &missing, request->deny_masks); } youngest_denied =get_hierarchy (subject->domain, youngest_layer);@@ -507,7 +464,7 @@ static struct kunit_case test_cases[] = {/* clang-format off */ KUNIT_CASE (test_get_hierarchy),KUNIT_CASE (test_get_denied_layer), - KUNIT_CASE(test_get_layer_from_deny_masks), + KUNIT_CASE(test_get_layer_from_fs_deny_masks), {} /* clang-format on */ };security/landlock/audit.h +1
-2@@ -43,8 +43,7 @@ struct landlock_request {access_mask_t access;/* Required fields for requests with layer masks. */ -const layer_mask_t (*layer_masks)[]; -size_t layer_masks_size; +const struct layer_access_masks *masks;/* Required fields for requests with deny masks. */ const access_mask_t all_existing_optional_access;security/landlock/domain.c +28
-92@@ -23,6 +23,7 @@#include "common.h" #include "domain.h" #include "id.h" +#include "limits.h" #ifdef CONFIG_AUDIT @@ -133,111 +134,47 @@ int landlock_init_hierarchy_log(struct landlock_hierarchy *const hierarchy)return 0 ; } -static deny_masks_t -get_layer_deny_mask (const access_mask_t all_existing_optional_access, -const unsigned long access_bit,const size_t layer) -{ -unsigned long access_weight; - -/* This may require change with new object types. */ -WARN_ON_ONCE (all_existing_optional_access != - _LANDLOCK_ACCESS_FS_OPTIONAL); - -if (WARN_ON_ONCE (layer >=LANDLOCK_MAX_NUM_LAYERS )) -return 0 ; - - access_weight =hweight_long (all_existing_optional_access & -GENMASK (access_bit,0 )); -if (WARN_ON_ONCE (access_weight <1 )) -return 0 ; - -return layer - << ((access_weight -1 ) *HWEIGHT (LANDLOCK_MAX_NUM_LAYERS -1 )); -} - -#ifdef CONFIG_SECURITY_LANDLOCK_KUNIT_TEST - -static void test_get_layer_deny_mask (struct kunit *const test) -{ -const unsigned long truncate =BIT_INDEX (LANDLOCK_ACCESS_FS_TRUNCATE ); -const unsigned long ioctl_dev =BIT_INDEX (LANDLOCK_ACCESS_FS_IOCTL_DEV ); - -KUNIT_EXPECT_EQ (test,0 , -get_layer_deny_mask (_LANDLOCK_ACCESS_FS_OPTIONAL, - truncate,0 )); -KUNIT_EXPECT_EQ (test,0x3 , -get_layer_deny_mask (_LANDLOCK_ACCESS_FS_OPTIONAL, - truncate,3 )); - -KUNIT_EXPECT_EQ (test,0 , -get_layer_deny_mask (_LANDLOCK_ACCESS_FS_OPTIONAL, - ioctl_dev,0 )); -KUNIT_EXPECT_EQ (test,0xf0 , -get_layer_deny_mask (_LANDLOCK_ACCESS_FS_OPTIONAL, - ioctl_dev,15 )); -} - -#endif /* CONFIG_SECURITY_LANDLOCK_KUNIT_TEST */ -deny_masks_t -landlock_get_deny_masks (const access_mask_t all_existing_optional_access, -const access_mask_t optional_access, -const layer_mask_t (*const layer_masks)[], -const size_t layer_masks_size) +landlock_get_fs_deny_masks (const access_mask_t optional_access, +const struct layer_access_masks *layer_masks) { -const unsigned long access_opt = optional_access; -unsigned long access_bit; -deny_masks_t deny_masks =0 ; +u8 truncate_layer =0 ; +u8 ioctl_dev_layer =0 ; -/* This may require change with new object types. */ -WARN_ON_ONCE (access_opt != - (optional_access & all_existing_optional_access)); - -if (WARN_ON_ONCE (!layer_masks)) -return 0 ; - -if (WARN_ON_ONCE (!access_opt)) -return 0 ; - -for_each_set_bit (access_bit, &access_opt, layer_masks_size) { -const layer_mask_t mask = (*layer_masks)[access_bit]; - -if (!mask) -continue ; - -/* __fls(1) == 0 */ - deny_masks |=get_layer_deny_mask (all_existing_optional_access, - access_bit,__fls (mask)); +for (int i =0 ; i <LANDLOCK_MAX_NUM_LAYERS ; i++) { +if (layer_masks->access[i] & optional_access & +LANDLOCK_ACCESS_FS_TRUNCATE ) + truncate_layer = i; +if (layer_masks->access[i] & optional_access & +LANDLOCK_ACCESS_FS_IOCTL_DEV ) + ioctl_dev_layer = i; } -return deny_masks; +return ((ioctl_dev_layer <<4 ) &0xf0 ) | (truncate_layer &0x0f ); }#ifdef CONFIG_SECURITY_LANDLOCK_KUNIT_TEST -static void test_landlock_get_deny_masks (struct kunit *const test) +static void test_landlock_get_fs_deny_masks (struct kunit *const test) { -const layer_mask_t layers1[BITS_PER_TYPE (access_mask_t)] = { - [BIT_INDEX (LANDLOCK_ACCESS_FS_EXECUTE )] =BIT_ULL (0 ) | -BIT_ULL (9 ), - [BIT_INDEX (LANDLOCK_ACCESS_FS_TRUNCATE )] =BIT_ULL (1 ), - [BIT_INDEX (LANDLOCK_ACCESS_FS_IOCTL_DEV )] =BIT_ULL (2 ) | -BIT_ULL (0 ), +const struct layer_access_masks layers1 = { + .access[0 ] =LANDLOCK_ACCESS_FS_EXECUTE | +LANDLOCK_ACCESS_FS_IOCTL_DEV , + .access[1 ] =LANDLOCK_ACCESS_FS_TRUNCATE , + .access[2 ] =LANDLOCK_ACCESS_FS_IOCTL_DEV , + .access[9 ] =LANDLOCK_ACCESS_FS_EXECUTE , };KUNIT_EXPECT_EQ (test,0x1 , -landlock_get_deny_masks (_LANDLOCK_ACCESS_FS_OPTIONAL, -LANDLOCK_ACCESS_FS_TRUNCATE , - &layers1,ARRAY_SIZE (layers1))); +landlock_get_fs_deny_masks (LANDLOCK_ACCESS_FS_TRUNCATE , + &layers1));KUNIT_EXPECT_EQ (test,0x20 , -landlock_get_deny_masks (_LANDLOCK_ACCESS_FS_OPTIONAL, -LANDLOCK_ACCESS_FS_IOCTL_DEV , - &layers1,ARRAY_SIZE (layers1))); +landlock_get_fs_deny_masks (LANDLOCK_ACCESS_FS_IOCTL_DEV , + &layers1));KUNIT_EXPECT_EQ ( test,0x21 , -landlock_get_deny_masks (_LANDLOCK_ACCESS_FS_OPTIONAL, -LANDLOCK_ACCESS_FS_TRUNCATE | -LANDLOCK_ACCESS_FS_IOCTL_DEV , - &layers1,ARRAY_SIZE (layers1))); +landlock_get_fs_deny_masks (LANDLOCK_ACCESS_FS_TRUNCATE | +LANDLOCK_ACCESS_FS_IOCTL_DEV , + &layers1)); }#endif /* CONFIG_SECURITY_LANDLOCK_KUNIT_TEST */ @@ -246,8 +183,7 @@ static void test_landlock_get_deny_masks(struct kunit *const test)static struct kunit_case test_cases[] = {/* clang-format off */ -KUNIT_CASE (test_get_layer_deny_mask), -KUNIT_CASE (test_landlock_get_deny_masks), +KUNIT_CASE (test_landlock_get_fs_deny_masks), {}/* clang-format on */ };security/landlock/domain.h +2
-4@@ -120,10 +120,8 @@ struct landlock_hierarchy {#ifdef CONFIG_AUDIT deny_masks_t -landlock_get_deny_masks (const access_mask_t all_existing_optional_access, -const access_mask_t optional_access, -const layer_mask_t (*const layer_masks)[], -size_t layer_masks_size); +landlock_get_fs_deny_masks (const access_mask_t optional_access, +const struct layer_access_masks *layer_masks);int landlock_init_hierarchy_log (struct landlock_hierarchy *const hierarchy);security/landlock/fs.c +58
-61@@ -407,57 +407,55 @@ static bool access_mask_subset(access_mask_t a, access_mask_t b)return (a | b) == b; } +/* +* Returns true iff the child file with the given src_child access rights under +* src_parent would result in having the same or fewer access rights if it were +* moved under new_parent. +*/ +static bool may_refer (const struct layer_access_masks *const src_parent, +const struct layer_access_masks *const src_child, +const struct layer_access_masks *const new_parent, +const bool child_is_dir) +{ +for (int i =0 ; i <LANDLOCK_MAX_NUM_LAYERS ; i++) { +access_mask_t child_access = src_parent->access[i] & + src_child->access[i]; +access_mask_t parent_access = new_parent->access[i]; + +if (!child_is_dir) { + child_access &=ACCESS_FILE ; + parent_access &=ACCESS_FILE ; + } + +if (!access_mask_subset (child_access, parent_access)) +return false; + } +return true; +} +/* * Check that a destination file hierarchy has more restrictions than a source * file hierarchy. This is only used for link and rename actions. * -* @layer_masks_child2: Optional child masks. +* Returns: true if child1 may be moved from parent1 to parent2 without +* increasing its access rights. If child2 is set, an additional condition is +* that child2 may be used from parent2 to parent1 without increasing its access +* rights. */ -static bool no_more_access ( -const layer_mask_t (*const layer_masks_parent1)[LANDLOCK_NUM_ACCESS_FS ], -const layer_mask_t (*const layer_masks_child1)[LANDLOCK_NUM_ACCESS_FS ], -const bool child1_is_directory, -const layer_mask_t (*const layer_masks_parent2)[LANDLOCK_NUM_ACCESS_FS ], -const layer_mask_t (*const layer_masks_child2)[LANDLOCK_NUM_ACCESS_FS ], -const bool child2_is_directory) +static bool no_more_access (const struct layer_access_masks *const parent1, +const struct layer_access_masks *const child1, +const bool child1_is_dir, +const struct layer_access_masks *const parent2, +const struct layer_access_masks *const child2, +const bool child2_is_dir) { -unsigned long access_bit; +if (!may_refer (parent1, child1, parent2, child1_is_dir)) +return false; -for (access_bit =0 ; access_bit <ARRAY_SIZE (*layer_masks_parent2); - access_bit++) { -/* Ignores accesses that only make sense for directories. */ -const bool is_file_access = - !!(BIT_ULL (access_bit) &ACCESS_FILE ); +if (!child2) +return true; -if (child1_is_directory || is_file_access) { -/* -* Checks if the destination restrictions are a -* superset of the source ones (i.e. inherited access -* rights without child exceptions): -* restrictions(parent2) >= restrictions(child1) -*/ -if ((((*layer_masks_parent1)[access_bit] & - (*layer_masks_child1)[access_bit]) | - (*layer_masks_parent2)[access_bit]) != - (*layer_masks_parent2)[access_bit]) -return false; - } - -if (!layer_masks_child2) -continue ; -if (child2_is_directory || is_file_access) { -/* -* Checks inverted restrictions for RENAME_EXCHANGE: -* restrictions(parent1) >= restrictions(child2) -*/ -if ((((*layer_masks_parent2)[access_bit] & - (*layer_masks_child2)[access_bit]) | - (*layer_masks_parent1)[access_bit]) != - (*layer_masks_parent1)[access_bit]) -return false; - } - } -return true; +return may_refer (parent2, child2, parent1, child2_is_dir); }#define NMA_TRUE (...) KUNIT_EXPECT_TRUE(test, no_more_access(__VA_ARGS__))@@ -467,25 +465,25 @@ static bool no_more_access(static void test_no_more_access (struct kunit *const test) { -const layer_mask_t rx0[LANDLOCK_NUM_ACCESS_FS ] = { - [BIT_INDEX (LANDLOCK_ACCESS_FS_EXECUTE )] =BIT_ULL (0 ), - [BIT_INDEX (LANDLOCK_ACCESS_FS_READ_FILE )] =BIT_ULL (0 ), +const struct layer_access_masks rx0 = { + .access[0 ] =LANDLOCK_ACCESS_FS_EXECUTE | +LANDLOCK_ACCESS_FS_READ_FILE , }; -const layer_mask_t mx0[LANDLOCK_NUM_ACCESS_FS ] = { - [BIT_INDEX (LANDLOCK_ACCESS_FS_EXECUTE )] =BIT_ULL (0 ), - [BIT_INDEX (LANDLOCK_ACCESS_FS_MAKE_REG )] =BIT_ULL (0 ), +const struct layer_access_masks mx0 = { + .access[0 ] =LANDLOCK_ACCESS_FS_EXECUTE | +LANDLOCK_ACCESS_FS_MAKE_REG , }; -const layer_mask_t x0[LANDLOCK_NUM_ACCESS_FS ] = { - [BIT_INDEX (LANDLOCK_ACCESS_FS_EXECUTE )] =BIT_ULL (0 ), +const struct layer_access_masks x0 = { + .access[0 ] =LANDLOCK_ACCESS_FS_EXECUTE , }; -const layer_mask_t x1[LANDLOCK_NUM_ACCESS_FS ] = { - [BIT_INDEX (LANDLOCK_ACCESS_FS_EXECUTE )] =BIT_ULL (1 ), +const struct layer_access_masks x1 = { + .access[1 ] =LANDLOCK_ACCESS_FS_EXECUTE , }; -const layer_mask_t x01[LANDLOCK_NUM_ACCESS_FS ] = { - [BIT_INDEX (LANDLOCK_ACCESS_FS_EXECUTE )] =BIT_ULL (0 ) | -BIT_ULL (1 ), +const struct layer_access_masks x01 = { + .access[0 ] =LANDLOCK_ACCESS_FS_EXECUTE , + .access[1 ] =LANDLOCK_ACCESS_FS_EXECUTE , }; -const layer_mask_t allows_all[LANDLOCK_NUM_ACCESS_FS ] = {}; +const struct layer_access_masks allows_all = {};/* Checks without restriction. */ NMA_TRUE (&x0, &allows_all, false, &allows_all,NULL , false);@@ -573,31 +571,30 @@ static void test_no_more_access(struct kunit *const test)#undef NMA_TRUE#undef NMA_FALSE -static bool is_layer_masks_allowed ( -layer_mask_t (*const layer_masks)[LANDLOCK_NUM_ACCESS_FS ]) +static bool is_layer_masks_allowed (const struct layer_access_masks *masks) { -return !memchr_inv (layer_masks,0 ,sizeof (*layer_masks)); +return !memchr_inv (&masks->access,0 ,sizeof (masks->access)); }/* -* Removes @layer_masks accesses that are not requested. +* Removes @masks accesses that are not requested. * * Returns true if the request is allowed, false otherwise.
security/landlock/fs.c static void test_no_more_access(struct kunit *const test) · line 587
Quoted 10 lines
security/landlock/fs.c +3
-5*/ -static bool -scope_to_request (const access_mask_t access_request, -layer_mask_t (*const layer_masks)[LANDLOCK_NUM_ACCESS_FS ]) +static bool scope_to_request (const access_mask_t access_request, +struct layer_access_masks *masks) { -const unsigned long access_req = access_request; -unsigned long access_bit; +bool saw_unfulfilled_access = false;
Kind of confusing variable name with a double negative.
Maybe accesses_met? (and invert the return condition)
Quoted 455 lines
security/landlock/fs.c +98
-125-if (WARN_ON_ONCE (!layer_masks)) +if (WARN_ON_ONCE (!masks))return true; -for_each_clear_bit (access_bit, &access_req,ARRAY_SIZE (*layer_masks)) - (*layer_masks)[access_bit] =0 ; - -return is_layer_masks_allowed (layer_masks); +for (int i =0 ; i <LANDLOCK_MAX_NUM_LAYERS ; i++) { + masks->access[i] &= access_request; +if (masks->access[i]) + saw_unfulfilled_access = true; + } +return !saw_unfulfilled_access; }#ifdef CONFIG_SECURITY_LANDLOCK_KUNIT_TEST @@ -605,48 +602,41 @@ scope_to_request(const access_mask_t access_request,static void test_scope_to_request_with_exec_none (struct kunit *const test) {/* Allows everything. */ -layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_FS ] = {}; +struct layer_access_masks masks = {};/* Checks and scopes with execute. */ -KUNIT_EXPECT_TRUE (test,scope_to_request (LANDLOCK_ACCESS_FS_EXECUTE , - &layer_masks)); -KUNIT_EXPECT_EQ (test,0 , - layer_masks[BIT_INDEX (LANDLOCK_ACCESS_FS_EXECUTE )]); -KUNIT_EXPECT_EQ (test,0 , - layer_masks[BIT_INDEX (LANDLOCK_ACCESS_FS_WRITE_FILE )]); +KUNIT_EXPECT_TRUE (test, +scope_to_request (LANDLOCK_ACCESS_FS_EXECUTE , &masks)); +KUNIT_EXPECT_EQ (test,0 , masks.access[0 ]); }static void test_scope_to_request_with_exec_some (struct kunit *const test) {/* Denies execute and write. */ -layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_FS ] = { - [BIT_INDEX (LANDLOCK_ACCESS_FS_EXECUTE )] =BIT_ULL (0 ), - [BIT_INDEX (LANDLOCK_ACCESS_FS_WRITE_FILE )] =BIT_ULL (1 ), +struct layer_access_masks masks = { + .access[0 ] =LANDLOCK_ACCESS_FS_EXECUTE , + .access[1 ] =LANDLOCK_ACCESS_FS_WRITE_FILE , };/* Checks and scopes with execute. */ KUNIT_EXPECT_FALSE (test,scope_to_request (LANDLOCK_ACCESS_FS_EXECUTE , - &layer_masks)); -KUNIT_EXPECT_EQ (test,BIT_ULL (0 ), - layer_masks[BIT_INDEX (LANDLOCK_ACCESS_FS_EXECUTE )]); -KUNIT_EXPECT_EQ (test,0 , - layer_masks[BIT_INDEX (LANDLOCK_ACCESS_FS_WRITE_FILE )]); + &masks)); +KUNIT_EXPECT_EQ (test,LANDLOCK_ACCESS_FS_EXECUTE , masks.access[0 ]); +KUNIT_EXPECT_EQ (test,0 , masks.access[1 ]); }static void test_scope_to_request_without_access (struct kunit *const test) {/* Denies execute and write. */ -layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_FS ] = { - [BIT_INDEX (LANDLOCK_ACCESS_FS_EXECUTE )] =BIT_ULL (0 ), - [BIT_INDEX (LANDLOCK_ACCESS_FS_WRITE_FILE )] =BIT_ULL (1 ), +struct layer_access_masks masks = { + .access[0 ] =LANDLOCK_ACCESS_FS_EXECUTE , + .access[1 ] =LANDLOCK_ACCESS_FS_WRITE_FILE , };/* Checks and scopes without access request. */ -KUNIT_EXPECT_TRUE (test,scope_to_request (0 , &layer_masks)); -KUNIT_EXPECT_EQ (test,0 , - layer_masks[BIT_INDEX (LANDLOCK_ACCESS_FS_EXECUTE )]); -KUNIT_EXPECT_EQ (test,0 , - layer_masks[BIT_INDEX (LANDLOCK_ACCESS_FS_WRITE_FILE )]); +KUNIT_EXPECT_TRUE (test,scope_to_request (0 , &masks)); +KUNIT_EXPECT_EQ (test,0 , masks.access[0 ]); +KUNIT_EXPECT_EQ (test,0 , masks.access[1 ]); }#endif /* CONFIG_SECURITY_LANDLOCK_KUNIT_TEST */ @@ -655,20 +645,16 @@ static void test_scope_to_request_without_access(struct kunit *const test)* Returns true if there is at least one access right different than * LANDLOCK_ACCESS_FS_REFER. */ -static bool -is_eacces (const layer_mask_t (*const layer_masks)[LANDLOCK_NUM_ACCESS_FS ], -const access_mask_t access_request) +static bool is_eacces (const struct layer_access_masks *masks, +const access_mask_t access_request) { -unsigned long access_bit; -/* LANDLOCK_ACCESS_FS_REFER alone must return -EXDEV. */ -const unsigned long access_check = access_request & - ~LANDLOCK_ACCESS_FS_REFER ; - -if (!layer_masks) +if (!masks)return false; -for_each_set_bit (access_bit, &access_check,ARRAY_SIZE (*layer_masks)) { -if ((*layer_masks)[access_bit]) +for (int i =0 ; i <LANDLOCK_MAX_NUM_LAYERS ; i++) { +/* LANDLOCK_ACCESS_FS_REFER alone must return -EXDEV. */ +if (masks->access[i] & access_request & + ~LANDLOCK_ACCESS_FS_REFER )return true; }return false;@@ -681,37 +667,37 @@ is_eacces(const layer_mask_t (*const layer_masks)[LANDLOCK_NUM_ACCESS_FS],static void test_is_eacces_with_none (struct kunit *const test) { -const layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_FS ] = {}; +const struct layer_access_masks masks = {}; -IE_FALSE (&layer_masks,0 ); -IE_FALSE (&layer_masks,LANDLOCK_ACCESS_FS_REFER ); -IE_FALSE (&layer_masks,LANDLOCK_ACCESS_FS_EXECUTE ); -IE_FALSE (&layer_masks,LANDLOCK_ACCESS_FS_WRITE_FILE ); +IE_FALSE (&masks,0 ); +IE_FALSE (&masks,LANDLOCK_ACCESS_FS_REFER ); +IE_FALSE (&masks,LANDLOCK_ACCESS_FS_EXECUTE ); +IE_FALSE (&masks,LANDLOCK_ACCESS_FS_WRITE_FILE ); }static void test_is_eacces_with_refer (struct kunit *const test) { -const layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_FS ] = { - [BIT_INDEX (LANDLOCK_ACCESS_FS_REFER )] =BIT_ULL (0 ), +const struct layer_access_masks masks = { + .access[0 ] =LANDLOCK_ACCESS_FS_REFER , }; -IE_FALSE (&layer_masks,0 ); -IE_FALSE (&layer_masks,LANDLOCK_ACCESS_FS_REFER ); -IE_FALSE (&layer_masks,LANDLOCK_ACCESS_FS_EXECUTE ); -IE_FALSE (&layer_masks,LANDLOCK_ACCESS_FS_WRITE_FILE ); +IE_FALSE (&masks,0 ); +IE_FALSE (&masks,LANDLOCK_ACCESS_FS_REFER ); +IE_FALSE (&masks,LANDLOCK_ACCESS_FS_EXECUTE ); +IE_FALSE (&masks,LANDLOCK_ACCESS_FS_WRITE_FILE ); }static void test_is_eacces_with_write (struct kunit *const test) { -const layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_FS ] = { - [BIT_INDEX (LANDLOCK_ACCESS_FS_WRITE_FILE )] =BIT_ULL (0 ), +const struct layer_access_masks masks = { + .access[0 ] =LANDLOCK_ACCESS_FS_WRITE_FILE , }; -IE_FALSE (&layer_masks,0 ); -IE_FALSE (&layer_masks,LANDLOCK_ACCESS_FS_REFER ); -IE_FALSE (&layer_masks,LANDLOCK_ACCESS_FS_EXECUTE ); +IE_FALSE (&masks,0 ); +IE_FALSE (&masks,LANDLOCK_ACCESS_FS_REFER ); +IE_FALSE (&masks,LANDLOCK_ACCESS_FS_EXECUTE ); -IE_TRUE (&layer_masks,LANDLOCK_ACCESS_FS_WRITE_FILE ); +IE_TRUE (&masks,LANDLOCK_ACCESS_FS_WRITE_FILE ); }#endif /* CONFIG_SECURITY_LANDLOCK_KUNIT_TEST */ @@ -761,26 +747,25 @@ static void test_is_eacces_with_write(struct kunit *const test)* - true if the access request is granted; * - false otherwise. */ -static bool is_access_to_paths_allowed ( -const struct landlock_ruleset *const domain, -const struct path *const path, -const access_mask_t access_request_parent1, -layer_mask_t (*const layer_masks_parent1)[LANDLOCK_NUM_ACCESS_FS ], -struct landlock_request *const log_request_parent1, -struct dentry *const dentry_child1, -const access_mask_t access_request_parent2, -layer_mask_t (*const layer_masks_parent2)[LANDLOCK_NUM_ACCESS_FS ], -struct landlock_request *const log_request_parent2, -struct dentry *const dentry_child2) +static bool +is_access_to_paths_allowed (const struct landlock_ruleset *const domain, +const struct path *const path, +const access_mask_t access_request_parent1, +struct layer_access_masks *layer_masks_parent1, +struct landlock_request *const log_request_parent1, +struct dentry *const dentry_child1, +const access_mask_t access_request_parent2, +struct layer_access_masks *layer_masks_parent2, +struct landlock_request *const log_request_parent2, +struct dentry *const dentry_child2) {bool allowed_parent1 = false, allowed_parent2 = false, is_dom_check, child1_is_directory = true, child2_is_directory = true;struct path walker_path;access_mask_t access_masked_parent1, access_masked_parent2; -layer_mask_t _layer_masks_child1[LANDLOCK_NUM_ACCESS_FS ], - _layer_masks_child2[LANDLOCK_NUM_ACCESS_FS ]; -layer_mask_t (*layer_masks_child1)[LANDLOCK_NUM_ACCESS_FS ] =NULL , - (*layer_masks_child2)[LANDLOCK_NUM_ACCESS_FS ] =NULL ; +struct layer_access_masks _layer_masks_child1, _layer_masks_child2; +struct layer_access_masks *layer_masks_child1 =NULL , + *layer_masks_child2 =NULL ;if (!access_request_parent1 && !access_request_parent2)return true;@@ -820,22 +805,20 @@ static bool is_access_to_paths_allowed( }if (unlikely (dentry_child1)) { -landlock_unmask_layers ( -find_rule (domain, dentry_child1), -landlock_init_layer_masks ( - domain,LANDLOCK_MASK_ACCESS_FS , - &_layer_masks_child1,LANDLOCK_KEY_INODE ), - &_layer_masks_child1,ARRAY_SIZE (_layer_masks_child1)); +if (landlock_init_layer_masks (domain,LANDLOCK_MASK_ACCESS_FS , + &_layer_masks_child1, +LANDLOCK_KEY_INODE )) +landlock_unmask_layers (find_rule (domain, dentry_child1), + &_layer_masks_child1); layer_masks_child1 = &_layer_masks_child1; child1_is_directory =d_is_dir (dentry_child1); }if (unlikely (dentry_child2)) { -landlock_unmask_layers ( -find_rule (domain, dentry_child2), -landlock_init_layer_masks ( - domain,LANDLOCK_MASK_ACCESS_FS , - &_layer_masks_child2,LANDLOCK_KEY_INODE ), - &_layer_masks_child2,ARRAY_SIZE (_layer_masks_child2)); +if (landlock_init_layer_masks (domain,LANDLOCK_MASK_ACCESS_FS , + &_layer_masks_child2, +LANDLOCK_KEY_INODE )) +landlock_unmask_layers (find_rule (domain, dentry_child2), + &_layer_masks_child2); layer_masks_child2 = &_layer_masks_child2; child2_is_directory =d_is_dir (dentry_child2); }@@ -890,16 +873,12 @@ static bool is_access_to_paths_allowed( } rule =find_rule (domain, walker_path.dentry); - allowed_parent1 = allowed_parent1 || -landlock_unmask_layers ( - rule, access_masked_parent1, - layer_masks_parent1, -ARRAY_SIZE (*layer_masks_parent1)); - allowed_parent2 = allowed_parent2 || -landlock_unmask_layers ( - rule, access_masked_parent2, - layer_masks_parent2, -ARRAY_SIZE (*layer_masks_parent2)); + allowed_parent1 = + allowed_parent1 || +landlock_unmask_layers (rule, layer_masks_parent1); + allowed_parent2 = + allowed_parent2 || +landlock_unmask_layers (rule, layer_masks_parent2);/* Stops when a rule from each layer grants access. */ if (allowed_parent1 && allowed_parent2)@@ -953,9 +932,7 @@ static bool is_access_to_paths_allowed( log_request_parent1->audit.type =LSM_AUDIT_DATA_PATH ; log_request_parent1->audit.u.path = *path; log_request_parent1->access = access_masked_parent1; - log_request_parent1->layer_masks = layer_masks_parent1; - log_request_parent1->layer_masks_size = -ARRAY_SIZE (*layer_masks_parent1); + log_request_parent1->masks = layer_masks_parent1; }if (!allowed_parent2) {@@ -963,9 +940,7 @@ static bool is_access_to_paths_allowed( log_request_parent2->audit.type =LSM_AUDIT_DATA_PATH ; log_request_parent2->audit.u.path = *path; log_request_parent2->access = access_masked_parent2; - log_request_parent2->layer_masks = layer_masks_parent2; - log_request_parent2->layer_masks_size = -ARRAY_SIZE (*layer_masks_parent2); + log_request_parent2->masks = layer_masks_parent2; }return allowed_parent1 && allowed_parent2; }@@ -978,7 +953,7 @@ static int current_check_access_path(const struct path *const path, };const struct landlock_cred_security *const subject =landlock_get_applicable_subject (current_cred (), masks,NULL ); -layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_FS ] = {}; +struct layer_access_masks layer_masks;struct landlock_request request = {};if (!subject)@@ -1053,10 +1028,10 @@ static access_mask_t maybe_remove(const struct dentry *const dentry)* - true if all the domain access rights are allowed for @dir; * - false if the walk reached @mnt_root. */ -static bool collect_domain_accesses ( -const struct landlock_ruleset *const domain, -const struct dentry *const mnt_root,struct dentry *dir, -layer_mask_t (*const layer_masks_dom)[LANDLOCK_NUM_ACCESS_FS ]) +static bool collect_domain_accesses (const struct landlock_ruleset *const domain, +const struct dentry *const mnt_root, +struct dentry *dir, +struct layer_access_masks *layer_masks_dom) {unsigned long access_dom;bool ret = false;@@ -1075,9 +1050,8 @@ static bool collect_domain_accesses(struct dentry *parent_dentry;/* Gets all layers allowing all domain accesses. */ -if (landlock_unmask_layers (find_rule (domain, dir), access_dom, - layer_masks_dom, -ARRAY_SIZE (*layer_masks_dom))) { +if (landlock_unmask_layers (find_rule (domain, dir), + layer_masks_dom)) {/* * Stops when all handled accesses are allowed by at * least one rule in each layer. @@ -1165,8 +1139,8 @@ static int current_check_refer_path(struct dentry *const old_dentry,access_mask_t access_request_parent1, access_request_parent2;struct path mnt_dir;struct dentry *old_parent; -layer_mask_t layer_masks_parent1[LANDLOCK_NUM_ACCESS_FS ] = {}, - layer_masks_parent2[LANDLOCK_NUM_ACCESS_FS ] = {}; +struct layer_access_masks layer_masks_parent1 = {}, + layer_masks_parent2 = {};struct landlock_request request1 = {}, request2 = {};if (!subject)@@ -1323,7 +1297,8 @@ static void hook_sb_delete(struct super_block *const sb)* second call to iput() for the same Landlock object. Also * checks I_NEW because such inode cannot be tied to an object. */ -if (inode_state_read (inode) & (I_FREEING |I_WILL_FREE |I_NEW )) { +if (inode_state_read (inode) & + (I_FREEING |I_WILL_FREE |I_NEW )) {spin_unlock (&inode->i_lock);continue ; }@@ -1641,7 +1616,7 @@ static bool is_device(const struct file *const file)static int hook_file_open (struct file *const file) { -layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_FS ] = {}; +struct layer_access_masks layer_masks = {};access_mask_t open_access_request, full_access_request, allowed_access, optional_access; const struct landlock_cred_security *const subject =@@ -1676,20 +1651,14 @@ static int hook_file_open(struct file *const file) &layer_masks, &request, NULL, 0, NULL, NULL, NULL)) { allowed_access = full_access_request; } else { -unsigned long access_bit; -const unsigned long access_req = full_access_request; -/* * Calculate the actual allowed access rights from layer_masks. -* Add each access right to allowed_access which has not been -* vetoed by any layer. +* Remove the access rights from the full access request which +* are still unfulfilled in any of the layers. */ - allowed_access =0 ; -for_each_set_bit (access_bit, &access_req, -ARRAY_SIZE (layer_masks)) { -if (!layer_masks[access_bit]) - allowed_access |=BIT_ULL (access_bit); - } + allowed_access = full_access_request; +for (int i =0 ; i <LANDLOCK_MAX_NUM_LAYERS ; i++) + allowed_access &= ~layer_masks.access[i]; }/* @@ -1700,9 +1669,8 @@ static int hook_file_open(struct file *const file)*/ landlock_file (file)->allowed_access = allowed_access;#ifdef CONFIG_AUDIT -landlock_file (file)->deny_masks =landlock_get_deny_masks ( - _LANDLOCK_ACCESS_FS_OPTIONAL, optional_access, &layer_masks, -ARRAY_SIZE (layer_masks)); +landlock_file (file)->deny_masks = +landlock_get_fs_deny_masks (optional_access, &layer_masks);#endif /* CONFIG_AUDIT */ if (access_mask_subset (open_access_request, allowed_access))security/landlock/net.c +5
-5@@ -47,7 +47,7 @@ static int current_check_access_socket(struct socket *const sock, access_mask_t access_request) {__be16 port; -layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_NET ] = {}; +struct layer_access_masks layer_masks = {};const struct landlock_rule *rule;struct landlock_id id = { .type =LANDLOCK_KEY_NET_PORT ,@@ -178,8 +178,9 @@ static int current_check_access_socket(struct socket *const sock, access_request =landlock_init_layer_masks (subject->domain, access_request, &layer_masks,LANDLOCK_KEY_NET_PORT ); -if (landlock_unmask_layers (rule, access_request, &layer_masks, -ARRAY_SIZE (layer_masks))) +if (!access_request) +return 0 ; +if (landlock_unmask_layers (rule, &layer_masks))return 0 ; audit_net.family = address->sa_family;@@ -189,8 +190,7 @@ static int current_check_access_socket(struct socket *const sock, .audit.type = LSM_AUDIT_DATA_NET, .audit.u.net = &audit_net, .access = access_request, - .layer_masks = &layer_masks, - .layer_masks_size = ARRAY_SIZE(layer_masks), + .masks = &layer_masks, });return -EACCES ; }security/landlock/ruleset.c +2
-15@@ -622,49 +622,24 @@ landlock_find_rule(const struct landlock_ruleset *const ruleset,* request are empty). */ bool landlock_unmask_layers (const struct landlock_rule *const rule, -const access_mask_t access_request, -layer_mask_t (*const layer_masks)[], -const size_t masks_array_size) +struct layer_access_masks *masks) { -size_t layer_level; - -if (!access_request || !layer_masks) +if (!masks)return true;if (!rule)return false; -/* -* An access is granted if, for each policy layer, at least one rule -* encountered on the pathwalk grants the requested access, -* regardless of its position in the layer stack. We must then check -* the remaining layers for each inode, from the first added layer to -* the last one. When there is multiple requested accesses, for each -* policy layer, the full set of requested accesses may not be granted -* by only one rule, but by the union (binary OR) of multiple rules. -* E.g. /a/b <execute> + /a <read> => /a/b <execute + read>
security/landlock/ruleset.c landlock_find_rule(const struct landlock_ruleset *const ruleset, · line 633
Quoted 10 lines
security/landlock/ruleset.c +2
-8-*/ -for (layer_level =0 ; layer_level < rule->num_layers; layer_level++) { -const struct landlock_layer *const layer = - &rule->layers[layer_level]; -const layer_mask_t layer_bit =BIT_ULL (layer->level -1 ); -const unsigned long access_req = access_request; -unsigned long access_bit; -bool is_empty; +for (int i =0 ; i < rule->num_layers; i++) { +const struct landlock_layer *l = &rule->layers[i];
Nit: using l for a variable makes it a little harder to read and confused me for a second.
Maybe this_layer?
Quoted 105 lines
security/landlock/ruleset.c +16
-35-/* -* Records in @layer_masks which layer grants access to each requested -* access: bit cleared if the related layer grants access. -*/ - is_empty = true; -for_each_set_bit (access_bit, &access_req, masks_array_size) { -if (layer->access &BIT_ULL (access_bit)) - (*layer_masks)[access_bit] &= ~layer_bit; - is_empty = is_empty && !(*layer_masks)[access_bit]; - } -if (is_empty) -return true; + masks->access[l->level -1 ] &= ~l->access; } -return false; + +for (int i =0 ; i <LANDLOCK_MAX_NUM_LAYERS ; i++) { +if (masks->access[i]) +return false; + } +return true; } typedef access_mask_t@@ -679,8 +654,7 @@ get_access_mask_t(const struct landlock_ruleset *const ruleset,* * @domain: The domain that defines the current restrictions. * @access_request: The requested access rights to check. -* @layer_masks: It must contain %LANDLOCK_NUM_ACCESS_FS or -* %LANDLOCK_NUM_ACCESS_NET elements according to @key_type. +* @masks: Layer access masks to populate. * @key_type: The key type to switch between access masks of different types. * * Returns: An access mask where each access right bit is set which is handled @@ -689,23 +663,20 @@ get_access_mask_t(const struct landlock_ruleset *const ruleset,access_mask_t landlock_init_layer_masks (const struct landlock_ruleset *const domain,const access_mask_t access_request, -layer_mask_t (*const layer_masks)[], +struct layer_access_masks *masks,const enum landlock_key_type key_type) {access_mask_t handled_accesses =0 ; -size_t layer_level, num_access;get_access_mask_t *get_access_mask;switch (key_type) {case LANDLOCK_KEY_INODE : get_access_mask = landlock_get_fs_access_mask; - num_access =LANDLOCK_NUM_ACCESS_FS ;break ;#if IS_ENABLED (CONFIG_INET )case LANDLOCK_KEY_NET_PORT : get_access_mask = landlock_get_net_access_mask; - num_access =LANDLOCK_NUM_ACCESS_NET ;break ;#endif /* IS_ENABLED(CONFIG_INET) */ @@ -714,27 +685,18 @@ landlock_init_layer_masks(const struct landlock_ruleset *const domain,return 0 ; } -memset (layer_masks,0 , -array_size (sizeof ((*layer_masks)[0 ]), num_access)); -/* An empty access request can happen because of O_WRONLY | O_RDWR. */ if (!access_request)return 0 ; -/* Saves all handled accesses per layer. */ -for (layer_level =0 ; layer_level < domain->num_layers; layer_level++) { -const unsigned long access_req = access_request; -const access_mask_t access_mask = -get_access_mask (domain, layer_level); -unsigned long access_bit; +for (int i =0 ; i < domain->num_layers; i++) { +const access_mask_t handled =get_access_mask (domain, i); -for_each_set_bit (access_bit, &access_req, num_access) { -if (BIT_ULL (access_bit) & access_mask) { - (*layer_masks)[access_bit] |= -BIT_ULL (layer_level); - handled_accesses |=BIT_ULL (access_bit); - } - } + masks->access[i] = access_request & handled; + handled_accesses |= masks->access[i]; } +for (int i = domain->num_layers; i <LANDLOCK_MAX_NUM_LAYERS ; i++) + masks->access[i] =0 ; +return handled_accesses; }security/landlock/ruleset.h +1
-0@@ -301,15 +301,25 @@ landlock_get_scope_mask(const struct landlock_ruleset *const ruleset,return ruleset->access_masks[layer_level].scope; } +/**
security/landlock/ruleset.h landlock_get_scope_mask(const struct landlock_ruleset *const ruleset, · line 314
Quoted 10 lines
security/landlock/ruleset.h +10
-0+* struct layer_accesses - A boolean matrix of layers and access rights +* +* This has a bit for each combination of layer numbers and access rights. +* During access checks, it is used to represent the access rights for each +* layer which still need to be fulfilled. When all bits are 0, the access +* request is considered to be fulfilled. +*/ +struct layer_access_masks { +access_mask_t access[LANDLOCK_MAX_NUM_LAYERS ]; +};
Nit: Probably best to format this with the doc comment style used elsewhere.
Quoted 16 lines
security/landlock/ruleset.h +3
-4+bool landlock_unmask_layers (const struct landlock_rule *const rule, -const access_mask_t access_request, -layer_mask_t (*const layer_masks)[], -const size_t masks_array_size); +struct layer_access_masks *masks);access_mask_t landlock_init_layer_masks (const struct landlock_ruleset *const domain,const access_mask_t access_request, -layer_mask_t (*const layer_masks)[], +struct layer_access_masks *masks,const enum landlock_key_type key_type);#endif /* _SECURITY_LANDLOCK_RULESET_H */
Mickaël Salaün
maintainerRe: [RFC PATCH 2/2] landlock: transpose the layer masks data structure
In reply to Günther NoackThis looks good overall but I need to spend more time reviewing it.
Because this changes may impact other ongoing patch series, I think I'll take this patch first to ease potential future fix backports.
Quoted 15 lines · Günther Noack
The layer masks data structure tracks the requested but unfulfilled access rights during an operations security check. It stores one bit for each combination of access right and layer index. If the bit is set, that access right is not granted (yet) in the given layer and we have to traverse the path further upwards to grant it.
Previously, the layer masks were stored as arrays mapping from access right indices to
layer_mask_t. Thelayer_mask_tvalue then indicates all layers in which the given access right is still (tentatively) denied.This patch introduces struct
layer_access_masksinstead: This struct contains an array with theaccess_mask_tof each (tentatively) denied access right in that layer.
Quoted 8 lines
The hypothesis of this patch is that this simplifies the code enough so that the resulting code will run faster:
* We can use bitwise operations in multiple places where we previously looped over bits individually with macros. (Should require less branch speculation)
* Code is ~160 lines smaller.
What about the KUnit test lines?
Quoted 13 lines
Other noteworthy changes:
* Clarify
deny_mask_tand the code assembling it.
* Document what that value looks like
* Make writing and reading functions specific to file system rules. (It only worked for FS rules before as well, but going all the way simplifies the code logic more.)
* Inno_more_access(), call a new helper functionmay_refer(), which only solves the asymmetric case. Previously, the code interleaved the checks for the two symmetric cases inRENAME_EXCHANGE. It feels that the code is clearer when renames withoutRENAME_EXCHANGEare more obviously the normal case.
It would be interesting to check the stackframe diff. You can use scripts/stackdelta for that, see
https://git.kernel.org/mic/c/602acfb541195eb35584d7a3fc7d1db676f059bd
Quoted 35 lines
Signed-off-by Günther Noack <[email protected]>--- security/landlock/access.h | 10 +- security/landlock/audit.c | 155 ++++++---------- security/landlock/audit.h | 3 +- security/landlock/domain.c | 120 +++---------- security/landlock/domain.h | 6 +- security/landlock/fs.c | 350 ++++++++++++++++-------------------- security/landlock/net.c | 10 +- security/landlock/ruleset.c | 78 +++----- security/landlock/ruleset.h | 18 +- 9 files changed, 290 insertions(+), 460 deletions(-)security/landlock/ruleset.c +2
-6@@ -622,49 +622,24 @@ landlock_find_rule(const struct landlock_ruleset *const ruleset,* request are empty). */ bool landlock_unmask_layers (const struct landlock_rule *const rule, -const access_mask_t access_request, -layer_mask_t (*const layer_masks)[], -const size_t masks_array_size) +struct layer_access_masks *masks) { -size_t layer_level; - -if (!access_request || !layer_masks) +if (!masks)return true;if (!rule)return false;
security/landlock/ruleset.c landlock_find_rule(const struct landlock_ruleset *const ruleset, · line 645
Quoted 10 lines
security/landlock/ruleset.c +0
-10-/* -* An access is granted if, for each policy layer, at least one rule -* encountered on the pathwalk grants the requested access, -* regardless of its position in the layer stack. We must then check -* the remaining layers for each inode, from the first added layer to -* the last one. When there is multiple requested accesses, for each -* policy layer, the full set of requested accesses may not be granted -* by only one rule, but by the union (binary OR) of multiple rules. -* E.g. /a/b <execute> + /a <read> => /a/b <execute + read> -*/
Why removing this comment?
Quoted 34 lines
security/landlock/ruleset.c +9
-20-for (layer_level =0 ; layer_level < rule->num_layers; layer_level++) { -const struct landlock_layer *const layer = - &rule->layers[layer_level]; -const layer_mask_t layer_bit =BIT_ULL (layer->level -1 ); -const unsigned long access_req = access_request; -unsigned long access_bit; -bool is_empty; +for (int i =0 ; i < rule->num_layers; i++) { +const struct landlock_layer *l = &rule->layers[i]; -/* -* Records in @layer_masks which layer grants access to each requested -* access: bit cleared if the related layer grants access. -*/ - is_empty = true; -for_each_set_bit (access_bit, &access_req, masks_array_size) { -if (layer->access &BIT_ULL (access_bit)) - (*layer_masks)[access_bit] &= ~layer_bit; - is_empty = is_empty && !(*layer_masks)[access_bit]; - } -if (is_empty) -return true; + masks->access[l->level -1 ] &= ~l->access; } -return false; + +for (int i =0 ; i <LANDLOCK_MAX_NUM_LAYERS ; i++) { +if (masks->access[i]) +return false; + } +return true; } typedef access_mask_t
Re: [RFC PATCH 2/2] landlock: transpose the layer masks data structure
Quoted 24 lines · Mickaël Salaün
This looks good overall but I need to spend more time reviewing it.
Because this changes may impact other ongoing patch series, I think I'll take this patch first to ease potential future fix backports.
Quoted 18 lines · Günther Noack
The layer masks data structure tracks the requested but unfulfilled access rights during an operations security check. It stores one bit for each combination of access right and layer index. If the bit is set, that access right is not granted (yet) in the given layer and we have to traverse the path further upwards to grant it.
Previously, the layer masks were stored as arrays mapping from access right indices to
layer_mask_t. Thelayer_mask_tvalue then indicates all layers in which the given access right is still (tentatively) denied.This patch introduces struct
layer_access_masksinstead: This struct contains an array with theaccess_mask_tof each (tentatively) denied access right in that layer.The hypothesis of this patch is that this simplifies the code enough so that the resulting code will run faster:
Quoted 7 lines
Quoted 5 lines
* We can use bitwise operations in multiple places where we previously looped over bits individually with macros. (Should require less branch speculation)
* Code is ~160 lines smaller.
What about the KUnit test lines?
Those are counted as well. The 160 lines statistic is directly from the diffstat in the cover letter.
(I removed the test_get_layer_deny_mask KUnit test, because the function under test was also not needed any more. Other than that, the KUnit tests are just adapted to test the equivalent logic with the new data structure.)
Quoted 13 lines
Quoted 12 lines
Other noteworthy changes:
* Clarify
deny_mask_tand the code assembling it.
* Document what that value looks like
* Make writing and reading functions specific to file system rules. (It only worked for FS rules before as well, but going all the way simplifies the code logic more.)
* Inno_more_access(), call a new helper functionmay_refer(), which only solves the asymmetric case. Previously, the code interleaved the checks for the two symmetric cases inRENAME_EXCHANGE. It feels that the code is clearer when renames withoutRENAME_EXCHANGEare more obviously the normal case.
Quoted 3 lines
It would be interesting to check the stackframe diff. You can use
scripts/stackdeltafor that, see
https://git.kernel.org/mic/c/602acfb541195eb35584d7a3fc7d1db676f059bd
Acknowledged. I did not get around to it yet, but put it on my todo list.
Quoted 4 lines
Quoted 4 lines
Signed-off-by Günther Noack <[email protected]>--- security/landlock/access.h | 10 +- security/landlock/audit.c | 155 ++++++----------
Quoted 8 lines
Quoted 8 lines
security/landlock/audit.h | 3 +- security/landlock/domain.c | 120 +++---------- security/landlock/domain.h | 6 +- security/landlock/fs.c | 350 ++++++++++++++++-------------------- security/landlock/net.c | 10 +- security/landlock/ruleset.c | 78 +++----- security/landlock/ruleset.h | 18 +- 9 files changed, 290 insertions(+), 460 deletions(-)
460 - 290 is 170. Well, almost 160. :)
Quoted 21 lines
Quoted 21 lines
security/landlock/ruleset.c +2
-6@@ -622,49 +622,24 @@ landlock_find_rule(const struct landlock_ruleset *const ruleset,* request are empty). */ bool landlock_unmask_layers (const struct landlock_rule *const rule, -const access_mask_t access_request, -layer_mask_t (*const layer_masks)[], -const size_t masks_array_size) +struct layer_access_masks *masks) { -size_t layer_level; - -if (!access_request || !layer_masks) +if (!masks)return true;if (!rule)return false;
Quoted 12 lines
security/landlock/ruleset.c landlock_find_rule(const struct landlock_ruleset *const ruleset, · line 645
Quoted 10 lines
security/landlock/ruleset.c +0
-10-/* -* An access is granted if, for each policy layer, at least one rule -* encountered on the pathwalk grants the requested access, -* regardless of its position in the layer stack. We must then check -* the remaining layers for each inode, from the first added layer to -* the last one. When there is multiple requested accesses, for each -* policy layer, the full set of requested accesses may not be granted -* by only one rule, but by the union (binary OR) of multiple rules. -* E.g. /a/b <execute> + /a <read> => /a/b <execute + read> -*/ Why removing this comment?
I did not understand why the comment was discussing this higher level picture when the surrounding function landlock_unmask_layers() is only concerned with a single rule. Should this comment be better moved elsewhere?
I don't feel strongly about it and re-reading it, the comment is still true. In doubt, I can also just put it back into the same function again. Let me know what you prefer.
Quoted 33 lines
Quoted 33 lines
security/landlock/ruleset.c +9
-20-for (layer_level =0 ; layer_level < rule->num_layers; layer_level++) { -const struct landlock_layer *const layer = - &rule->layers[layer_level]; -const layer_mask_t layer_bit =BIT_ULL (layer->level -1 ); -const unsigned long access_req = access_request; -unsigned long access_bit; -bool is_empty; +for (int i =0 ; i < rule->num_layers; i++) { +const struct landlock_layer *l = &rule->layers[i]; -/* -* Records in @layer_masks which layer grants access to each requested -* access: bit cleared if the related layer grants access. -*/ - is_empty = true; -for_each_set_bit (access_bit, &access_req, masks_array_size) { -if (layer->access &BIT_ULL (access_bit)) - (*layer_masks)[access_bit] &= ~layer_bit; - is_empty = is_empty && !(*layer_masks)[access_bit]; - } -if (is_empty) -return true; + masks->access[l->level -1 ] &= ~l->access; } -return false; + +for (int i =0 ; i <LANDLOCK_MAX_NUM_LAYERS ; i++) { +if (masks->access[i]) +return false; + } +return true; }
Quoted 1 line
security/landlock/ruleset.c landlock_find_rule(const struct landlock_ruleset *const ruleset, · line 645
Quoted 1 line
security/landlock/ruleset.c +0
-0typedef access_mask_t
–Günther
Re: [RFC PATCH 2/2] landlock: transpose the layer masks data structure
In reply to Günther NoackQuoted 5 lines · Günther Noack
security/landlock/access.h +0
-0
security/landlock/access.h union access_masks_all { · line 73
Quoted 10 lines
security/landlock/access.h +5
-0/* * Tracks domains responsible of a denied access. This is required to avoid * storing in each object the full layer_masks[] required by update_request(). +* +* Each nibble represents the layer index of the newest layer which denied a +* certain access right. For file system access rights, the upper four bits are +* the index of the layer which denies LANDLOCK_ACCESS_FS_IOCTL_DEV and the +* lower nibble represents LANDLOCK_ACCESS_FS_TRUNCATE. */ typedef u8 deny_masks_t ;
FYI: I left this out for now because it felt a bit out of scope (and transposing the layer masks was adventurous enough), but I was tempted to go one step further here and turn this into a struct with bitfields:
/* A collection of layer indices denying specific access rights. */ struct layers_denying_fs_access {
unsigned int truncate : 4;
unsigned int ioctl_dev : 4;
}
(Type name TBD, I am open for suggestions.)
I think if we accept that this data structure is specific to FS access rights, we win clarity in the code. When I came across the code that put this together dynamically and in a more generic way, it took me a while to figure out what it did.
–Günther
Mickaël Salaün
maintainerRe: [RFC PATCH 2/2] landlock: transpose the layer masks data structure
Quoted 28 lines · Günther Noack
Quoted 5 lines · Günther Noack
security/landlock/access.h +0
-0security/landlock/access.h union access_masks_all { · line 73
Quoted 10 lines
security/landlock/access.h +5
-0/* * Tracks domains responsible of a denied access. This is required to avoid * storing in each object the full layer_masks[] required by update_request(). +* +* Each nibble represents the layer index of the newest layer which denied a +* certain access right. For file system access rights, the upper four bits are +* the index of the layer which denies LANDLOCK_ACCESS_FS_IOCTL_DEV and the +* lower nibble represents LANDLOCK_ACCESS_FS_TRUNCATE. */ typedef u8 deny_masks_t ;FYI: I left this out for now because it felt a bit out of scope (and transposing the layer masks was adventurous enough), but I was tempted to go one step further here and turn this into a struct with bitfields:
/* A collection of layer indices denying specific access rights. */ struct
layers_denying_fs_access{
unsigned int truncate : 4;
unsigned intioctl_dev: 4;
}
Quoted 6 lines
(Type name TBD, I am open for suggestions.)
I think if we accept that this data structure is specific to FS access rights, we win clarity in the code. When I came across the code that put this together dynamically and in a more generic way, it took me a while to figure out what it did.
This change should help indeed (in a standalone patch). Maybe we could make the rest of the logic more generic in a way that it would be simpler to add this kind of access right?
Re: [RFC PATCH 2/2] landlock: transpose the layer masks data structure
In reply to Günther NoackQuoted 24 lines · Günther Noack
The layer masks data structure tracks the requested but unfulfilled access rights during an operations security check. It stores one bit for each combination of access right and layer index. If the bit is set, that access right is not granted (yet) in the given layer and we have to traverse the path further upwards to grant it.
Previously, the layer masks were stored as arrays mapping from access right indices to
layer_mask_t. Thelayer_mask_tvalue then indicates all layers in which the given access right is still (tentatively) denied.This patch introduces struct
layer_access_masksinstead: This struct contains an array with theaccess_mask_tof each (tentatively) denied access right in that layer.The hypothesis of this patch is that this simplifies the code enough so that the resulting code will run faster:
* We can use bitwise operations in multiple places where we previously looped over bits individually with macros. (Should require less branch speculation)
* Code is ~160 lines smaller.
Quoted 7 lines
Other noteworthy changes:
* Clarify
deny_mask_tand the code assembling it.
* Document what that value looks like
* Make writing and reading functions specific to file system rules. (It only worked for FS rules before as well, but going all the way simplifies the code logic more.)
In the original commit message that added this type [1] there was this statement:
Quoted 2 lines
Implementing
deny_masks_twith a bitfield instead of a struct enables a generic implementation to store and extract layer levels.
At some point when looking at this I was wondering why this wasn't a struct with 2 u8:4 fields, but rather, a u8 with bit manipulation code. While it is possible that I might have just misunderstood it, reading the above statement my take-away was that a struct would have forced us to address the indices with specific names, e.g. it would need to be defined like
struct deny_masks_t {
u8 ioctl:4;
u8 truncate:4;
}And it would thus not be possible to manipulate the indices in a generic way (e.g. the way it was implemented before, given all_existing_optional_access and access_bit, read and write the right bits).
However, since we're now removing that generic-ability, should we consider turning it into a struct? (If later on we have different access types that also have optional accesses, we could use a union of structs)
btw, since this causes conflicts with the quiet flag series and Mickaël has indicated that this should be merged first, I will probably have to make my series based on top of this. Will watch this series to see if there are more changes.
Also, this transpose and code simplification should also simplify the mutable domains work so thanks for the refactor!
A while ago I also made some benchmarking script which I sent a PR to landlock-test-tools [2], and earlier I tested this patch with it, and saw some improvement (but it was much less in terms of percentage, which may be due to the lower directory depth, or may be due to other unknown reason):
TestDescription(landlock=True, dir_depth=10, nb_extra_rules=10)
base.2:
c_measured_syscall_time_ns: 45000000 samples (3 trials), avg=1718.15, min=1663.00, max=275949.00, median=1696.46, stddev=437.52
95% confidence interval: [1718.03 .. 1718.28]
Estimated landlock overhead (vs no-landlock): 226.5%
48bd90e91fe6.2:
c_measured_syscall_time_ns: 45000000 samples (3 trials), avg=1709.60, min=1633.00, max=280608.00, median=1688.83, stddev=441.83
95% confidence interval: [1709.48 .. 1709.73]
** Improved 0.5% **
...
[1660 .. 1669]: [1660 .. 1669]: ###
[1670 .. 1679]: ## [1670 .. 1679]: ###############
[1680 .. 1689]: ###################### [1680 .. 1689]: #################################
[1690 .. 1699]: ######################################## [1690 .. 1699]: ##################################
[1700 .. 1709]: ############################ [1700 .. 1709]: #############
[1710 .. 1719]: ######### [1710 .. 1719]: ##
[1720 .. 1729]: ## [1720 .. 1729]:
...
Estimated landlock overhead (vs no-landlock): 223.0%TestDescription(landlock=True, dir_depth=29, nb_extra_rules=10)
base.2:
c_measured_syscall_time_ns: 45000000 samples (3 trials), avg=3869.66, min=3727.00, max=272563.00, median=3813.42, stddev=666.18
95% confidence interval: [3869.47 .. 3869.86]
Estimated landlock overhead (vs no-landlock): 427.3%
48bd90e91fe6.2:
c_measured_syscall_time_ns: 45000000 samples (3 trials), avg=3855.61, min=3697.00, max=271690.00, median=3804.82, stddev=682.74
95% confidence interval: [3855.41 .. 3855.81]
** Improved 0.4% **
...
[3750 .. 3759]: [3750 .. 3759]: #
[3760 .. 3769]: [3760 .. 3769]: #######
[3770 .. 3779]: [3770 .. 3779]: ###############
[3780 .. 3789]: #### [3780 .. 3789]: ###################
[3790 .. 3799]: ################### [3790 .. 3799]: ###################
[3800 .. 3809]: ###################################### [3800 .. 3809]: ########################
[3810 .. 3819]: ######################################## [3810 .. 3819]: ############################
[3820 .. 3829]: ########################## [3820 .. 3829]: #####################
[3830 .. 3839]: ############# [3830 .. 3839]: #########
[3840 .. 3849]: ###### [3840 .. 3849]: ##
[3850 .. 3859]: ## [3850 .. 3859]:
[3860 .. 3869]: [3860 .. 3869]:
[3870 .. 3879]: [3870 .. 3879]:
...
[4980 .. 4989]: [4980 .. 4989]:
[4990 .. 4999]: [4990 .. 4999]:
[5000 .. 272563]: # [5000 .. 271690]: #
Estimated landlock overhead (vs no-landlock): 424.2%Full data including test with 0 depth, or 1000 rules:
https://fileshare.maowtm.org/landlock-20251230/index.html
[1]: https://lore.kernel.org/all/[email protected]/
[2]: https://github.com/landlock-lsm/landlock-test-tools/pull/17
Mickaël Salaün
maintainerRe: [RFC PATCH 2/2] landlock: transpose the layer masks data structure
Quoted 51 lines · Tingmao Wang
Quoted 24 lines · Günther Noack
The layer masks data structure tracks the requested but unfulfilled access rights during an operations security check. It stores one bit for each combination of access right and layer index. If the bit is set, that access right is not granted (yet) in the given layer and we have to traverse the path further upwards to grant it.
Previously, the layer masks were stored as arrays mapping from access right indices to
layer_mask_t. Thelayer_mask_tvalue then indicates all layers in which the given access right is still (tentatively) denied.This patch introduces struct
layer_access_masksinstead: This struct contains an array with theaccess_mask_tof each (tentatively) denied access right in that layer.The hypothesis of this patch is that this simplifies the code enough so that the resulting code will run faster:
* We can use bitwise operations in multiple places where we previously looped over bits individually with macros. (Should require less branch speculation)
* Code is ~160 lines smaller.
Quoted 7 lines
Other noteworthy changes:
* Clarify
deny_mask_tand the code assembling it.
* Document what that value looks like
* Make writing and reading functions specific to file system rules. (It only worked for FS rules before as well, but going all the way simplifies the code logic more.)In the original commit message that added this type [1] there was this statement:
Quoted 2 lines
Implementing
deny_masks_twith a bitfield instead of a struct enables a generic implementation to store and extract layer levels.At some point when looking at this I was wondering why this wasn't a struct with 2 u8:4 fields, but rather, a u8 with bit manipulation code. While it is possible that I might have just misunderstood it, reading the above statement my take-away was that a struct would have forced us to address the indices with specific names, e.g. it would need to be defined like
struct deny_masks_t { u8 ioctl:4; u8 truncate:4; }
Quoted 8 lines
And it would thus not be possible to manipulate the indices in a generic way (e.g. the way it was implemented before, given
all_existing_optional_accessandaccess_bit, read and write the right bits).However, since we're now removing that generic-ability, should we consider turning it into a struct? (If later on we have different access types that also have optional accesses, we could use a union of structs)
I would prefer to have a more generic implementation, or at least to make it easy to add this kind of access rights. Any idea how to improve the situation?
Quoted 6 lines
btw, since this causes conflicts with the quiet flag series and Mickaël has indicated that this should be merged first, I will probably have to make my series based on top of this. Will watch this series to see if there are more changes.
I'd like to make sure your quiet flag series is still OK with this patch, and what would be the impact, so yes, please review and experiment with this series.
Quoted 3 lines
Also, this transpose and code simplification should also simplify the mutable domains work so thanks for the refactor!
Good :)
Quoted 47 lines
A while ago I also made some benchmarking script which I sent a PR to landlock-test-tools [2], and earlier I tested this patch with it, and saw some improvement (but it was much less in terms of percentage, which may be due to the lower directory depth, or may be due to other unknown reason):
TestDescription(landlock=True, dir_depth=10, nb_extra_rules=10)
base.2:c_measured_syscall_time_ns: 45000000 samples (3 trials), avg=1718.15, min=1663.00, max=275949.00, median=1696.46, stddev=437.52 95% confidence interval: [1718.03 .. 1718.28] Estimated landlock overhead (vs no-landlock): 226.5% 48bd90e91fe6.2: c_measured_syscall_time_ns: 45000000 samples (3 trials), avg=1709.60, min=1633.00, max=280608.00, median=1688.83, stddev=441.83 95% confidence interval: [1709.48 .. 1709.73] ** Improved 0.5% ** ... [1660 .. 1669]: [1660 .. 1669]: ### [1670 .. 1679]: ## [1670 .. 1679]: ############### [1680 .. 1689]: ###################### [1680 .. 1689]: ################################# [1690 .. 1699]: ######################################## [1690 .. 1699]: ################################## [1700 .. 1709]: ############################ [1700 .. 1709]: ############# [1710 .. 1719]: ######### [1710 .. 1719]: ## [1720 .. 1729]: ## [1720 .. 1729]: ... Estimated landlock overhead (vs no-landlock): 223.0%
TestDescription(landlock=True, dir_depth=29, nb_extra_rules=10)
base.2:c_measured_syscall_time_ns: 45000000 samples (3 trials), avg=3869.66, min=3727.00, max=272563.00, median=3813.42, stddev=666.18 95% confidence interval: [3869.47 .. 3869.86] Estimated landlock overhead (vs no-landlock): 427.3% 48bd90e91fe6.2: c_measured_syscall_time_ns: 45000000 samples (3 trials), avg=3855.61, min=3697.00, max=271690.00, median=3804.82, stddev=682.74 95% confidence interval: [3855.41 .. 3855.81] ** Improved 0.4% ** ... [3750 .. 3759]: [3750 .. 3759]: # [3760 .. 3769]: [3760 .. 3769]: ####### [3770 .. 3779]: [3770 .. 3779]: ############### [3780 .. 3789]: #### [3780 .. 3789]: ################### [3790 .. 3799]: ################### [3790 .. 3799]: ################### [3800 .. 3809]: ###################################### [3800 .. 3809]: ######################## [3810 .. 3819]: ######################################## [3810 .. 3819]: ############################ [3820 .. 3829]: ########################## [3820 .. 3829]: ##################### [3830 .. 3839]: ############# [3830 .. 3839]: ######### [3840 .. 3849]: ###### [3840 .. 3849]: ##
Quoted 8 lines
[3850 .. 3859]: ## [3850 .. 3859]:
[3860 .. 3869]: [3860 .. 3869]:
[3870 .. 3879]: [3870 .. 3879]:
...
[4980 .. 4989]: [4980 .. 4989]:
[4990 .. 4999]: [4990 .. 4999]:
[5000 .. 272563]: # [5000 .. 271690]: #
Estimated landlock overhead (vs no-landlock): 424.2%Thanks for the benchmark.
Quoted 8 lines
Full data including test with 0 depth, or 1000 rules:
https://fileshare.maowtm.org/landlock-20251230/index.html[1]: https://lore.kernel.org/all/[email protected]/
[2]: https://github.com/landlock-lsm/landlock-test-tools/pull/17
Re: [RFC PATCH 2/2] landlock: transpose the layer masks data structure
In reply to Tingmao WangQuoted 59 lines · Tingmao Wang
Quoted 24 lines · Günther Noack
The layer masks data structure tracks the requested but unfulfilled access rights during an operations security check. It stores one bit for each combination of access right and layer index. If the bit is set, that access right is not granted (yet) in the given layer and we have to traverse the path further upwards to grant it.
Previously, the layer masks were stored as arrays mapping from access right indices to
layer_mask_t. Thelayer_mask_tvalue then indicates all layers in which the given access right is still (tentatively) denied.This patch introduces struct
layer_access_masksinstead: This struct contains an array with theaccess_mask_tof each (tentatively) denied access right in that layer.The hypothesis of this patch is that this simplifies the code enough so that the resulting code will run faster:
* We can use bitwise operations in multiple places where we previously looped over bits individually with macros. (Should require less branch speculation)
* Code is ~160 lines smaller.
Quoted 7 lines
Other noteworthy changes:
* Clarify
deny_mask_tand the code assembling it.
* Document what that value looks like
* Make writing and reading functions specific to file system rules. (It only worked for FS rules before as well, but going all the way simplifies the code logic more.)In the original commit message that added this type [1] there was this statement:
Quoted 2 lines
Implementing
deny_masks_twith a bitfield instead of a struct enables a generic implementation to store and extract layer levels.At some point when looking at this I was wondering why this wasn't a struct with 2 u8:4 fields, but rather, a u8 with bit manipulation code. While it is possible that I might have just misunderstood it, reading the above statement my take-away was that a struct would have forced us to address the indices with specific names, e.g. it would need to be defined like
struct deny_masks_t { u8 ioctl:4; u8 truncate:4; }And it would thus not be possible to manipulate the indices in a generic way (e.g. the way it was implemented before, given
all_existing_optional_accessandaccess_bit, read and write the right bits).However, since we're now removing that generic-ability, should we consider turning it into a struct? (If later on we have different access types that also have optional accesses, we could use a union of structs)
Quoted 4 lines
btw, since this causes conflicts with the quiet flag series and Mickaël has indicated that this should be merged first, I will probably have to make my series based on top of this. Will watch this series to see if there are more changes.
Likewise for my NO_INHERIT series, which will need some rebase work as well. (my series is built on the quiet flag series, to reuse the similar "bubble up" flag collection logic).
I'll keep an eye on your tree Tingmao and start rebasing my NO_INHERIT on your patches if you put your work there. (Otherwise I'll do it when you send it on the mailing list)
Re: [RFC PATCH 2/2] landlock: transpose the layer masks data structure
In reply to Tingmao WangQuoted 51 lines · Tingmao Wang
Quoted 24 lines · Günther Noack
The layer masks data structure tracks the requested but unfulfilled access rights during an operations security check. It stores one bit for each combination of access right and layer index. If the bit is set, that access right is not granted (yet) in the given layer and we have to traverse the path further upwards to grant it.
Previously, the layer masks were stored as arrays mapping from access right indices to
layer_mask_t. Thelayer_mask_tvalue then indicates all layers in which the given access right is still (tentatively) denied.This patch introduces struct
layer_access_masksinstead: This struct contains an array with theaccess_mask_tof each (tentatively) denied access right in that layer.The hypothesis of this patch is that this simplifies the code enough so that the resulting code will run faster:
* We can use bitwise operations in multiple places where we previously looped over bits individually with macros. (Should require less branch speculation)
* Code is ~160 lines smaller.
Quoted 7 lines
Other noteworthy changes:
* Clarify
deny_mask_tand the code assembling it.
* Document what that value looks like
* Make writing and reading functions specific to file system rules. (It only worked for FS rules before as well, but going all the way simplifies the code logic more.)In the original commit message that added this type [1] there was this statement:
Quoted 2 lines
Implementing
deny_masks_twith a bitfield instead of a struct enables a generic implementation to store and extract layer levels.At some point when looking at this I was wondering why this wasn't a struct with 2 u8:4 fields, but rather, a u8 with bit manipulation code. While it is possible that I might have just misunderstood it, reading the above statement my take-away was that a struct would have forced us to address the indices with specific names, e.g. it would need to be defined like
struct deny_masks_t { u8 ioctl:4; u8 truncate:4; }
Quoted 8 lines
And it would thus not be possible to manipulate the indices in a generic way (e.g. the way it was implemented before, given
all_existing_optional_accessandaccess_bit, read and write the right bits).However, since we're now removing that generic-ability, should we consider turning it into a struct? (If later on we have different access types that also have optional accesses, we could use a union of structs)
I could not agree more, I also think a struct would be better, and also rolling it out per access right type is not something I am afraid of. I remarked on it in
https://lore.kernel.org/all/[email protected]/
But as Mickaël pointed out in
https://lore.kernel.org/all/[email protected]/, my endeavors to change the deny masks data type are a bit too intertwined with the refactoring of transposing the layer masks. I'll try to decouple these two changes from each other and send the deny masks change as a follow-up.
Quoted 7 lines
btw, since this causes conflicts with the quiet flag series and Mickaël has indicated that this should be merged first, I will probably have to make my series based on top of this. Will watch this series to see if there are more changes.
Also, this transpose and code simplification should also simplify the mutable domains work so thanks for the refactor!
Thanks, I am glad it helps and does not only cause merge conflicts! :)
Quoted 55 lines
A while ago I also made some benchmarking script which I sent a PR to landlock-test-tools [2], and earlier I tested this patch with it, and saw some improvement (but it was much less in terms of percentage, which may be due to the lower directory depth, or may be due to other unknown reason):
TestDescription(landlock=True, dir_depth=10, nb_extra_rules=10)
base.2:c_measured_syscall_time_ns: 45000000 samples (3 trials), avg=1718.15, min=1663.00, max=275949.00, median=1696.46, stddev=437.52 95% confidence interval: [1718.03 .. 1718.28] Estimated landlock overhead (vs no-landlock): 226.5% 48bd90e91fe6.2: c_measured_syscall_time_ns: 45000000 samples (3 trials), avg=1709.60, min=1633.00, max=280608.00, median=1688.83, stddev=441.83 95% confidence interval: [1709.48 .. 1709.73] ** Improved 0.5% ** ... [1660 .. 1669]: [1660 .. 1669]: ### [1670 .. 1679]: ## [1670 .. 1679]: ############### [1680 .. 1689]: ###################### [1680 .. 1689]: ################################# [1690 .. 1699]: ######################################## [1690 .. 1699]: ################################## [1700 .. 1709]: ############################ [1700 .. 1709]: ############# [1710 .. 1719]: ######### [1710 .. 1719]: ## [1720 .. 1729]: ## [1720 .. 1729]: ... Estimated landlock overhead (vs no-landlock): 223.0%
TestDescription(landlock=True, dir_depth=29, nb_extra_rules=10)
base.2:c_measured_syscall_time_ns: 45000000 samples (3 trials), avg=3869.66, min=3727.00, max=272563.00, median=3813.42, stddev=666.18 95% confidence interval: [3869.47 .. 3869.86] Estimated landlock overhead (vs no-landlock): 427.3% 48bd90e91fe6.2: c_measured_syscall_time_ns: 45000000 samples (3 trials), avg=3855.61, min=3697.00, max=271690.00, median=3804.82, stddev=682.74 95% confidence interval: [3855.41 .. 3855.81] ** Improved 0.4% ** ... [3750 .. 3759]: [3750 .. 3759]: # [3760 .. 3769]: [3760 .. 3769]: ####### [3770 .. 3779]: [3770 .. 3779]: ############### [3780 .. 3789]: #### [3780 .. 3789]: ################### [3790 .. 3799]: ################### [3790 .. 3799]: ################### [3800 .. 3809]: ###################################### [3800 .. 3809]: ######################## [3810 .. 3819]: ######################################## [3810 .. 3819]: ############################ [3820 .. 3829]: ########################## [3820 .. 3829]: ##################### [3830 .. 3839]: ############# [3830 .. 3839]: ######### [3840 .. 3849]: ###### [3840 .. 3849]: ## [3850 .. 3859]: ## [3850 .. 3859]: [3860 .. 3869]: [3860 .. 3869]: [3870 .. 3879]: [3870 .. 3879]: ... [4980 .. 4989]: [4980 .. 4989]: [4990 .. 4999]: [4990 .. 4999]: [5000 .. 272563]: # [5000 .. 271690]: # Estimated landlock overhead (vs no-landlock): 424.2%
Quoted 2 lines
Full data including test with 0 depth, or 1000 rules:
https://fileshare.maowtm.org/landlock-20251230/index.html
Ooh, awesome, thanks for the measurements! :)
The tool I used to benchmark myself [1] is much more crude. It purposefully constructs an uncharacteristically bad scenario, to amplify the performance difference and to make it more measurable.
It creates 10000 nested subdirectories, with a rule for each, so that landlock_unmask_layers() gets called 10000 times per attempted access in the innermost directory.
[1] https://lore.kernel.org/all/[email protected]/
–Günther
Mickaël Salaün
maintainerRe: [RFC PATCH 2/2] landlock: transpose the layer masks data structure
In reply to Günther NoackThe goal of the initial design was to minimize the amount of memory wrt the number of different access rights because the maximum number of layers is 16 whereas access rights could grow up to 64.
Transposing the matrix increases the memory footprint in theory but because we still need the struct layer_access_masks matrix, it should actually be better. See stack usage delta with audit (generated with check-linux.sh):
landlock_unmask_layers 208 80 -128 landlock_init_layer_masks 192 96 -96 landlock_log_denial 176 80 -96 current_check_access_path 336 304 -32 current_check_refer_path 592 560 -32 hook_file_open 352 320 -32 hook_file_send_sigiotask 176 160 -16 hook_file_truncate 112 96 -16 hook_move_mount 128 112 -16 hook_ptrace_access_check 192 176 -16 hook_ptrace_traceme 160 144 -16 hook_sb_mount 128 112 -16 hook_sb_pivotroot 128 112 -16 hook_sb_remount 128 112 -16 hook_sb_umount 128 112 -16 hook_task_kill 176 160 -16 current_check_access_socket 336 352 +16 is_access_to_paths_allowed 384 400 +16
...and stack usage delta without audit:
landlock_unmask_layers 208 80 -128 landlock_init_layer_masks 192 96 -96 hook_file_open 208 192 -16 current_check_access_socket 176 208 +32
However, when we'll add the next access right, access_mask_t will be u32 instead of u16, and stack usage delta will increase:
current_check_access_socket 352 384 +32 hook_file_open 320 352 +32 current_check_access_path 304 352 +48 current_check_refer_path 560 608 +48 is_access_to_paths_allowed 400 464 +64
Even if the cumulative stack usage delta seems reasonable, the commit message should talk about these drawbacks.
I think the improved compiled code, and the overall simplification are worth it.
Quoted 2 lines · Günther Noack
The layer masks data structure tracks the requested but unfulfilled access rights during an operations security check. It stores one bit
operation?
Quoted 94 lines
for each combination of access right and layer index. If the bit is set, that access right is not granted (yet) in the given layer and we have to traverse the path further upwards to grant it.
Previously, the layer masks were stored as arrays mapping from access right indices to
layer_mask_t. Thelayer_mask_tvalue then indicates all layers in which the given access right is still (tentatively) denied.This patch introduces struct
layer_access_masksinstead: This struct contains an array with theaccess_mask_tof each (tentatively) denied access right in that layer.The hypothesis of this patch is that this simplifies the code enough so that the resulting code will run faster:
* We can use bitwise operations in multiple places where we previously looped over bits individually with macros. (Should require less branch speculation)
* Code is ~160 lines smaller.
Other noteworthy changes:
* Clarify
deny_mask_tand the code assembling it.
* Document what that value looks like
* Make writing and reading functions specific to file system rules. (It only worked for FS rules before as well, but going all the way simplifies the code logic more.)
* Inno_more_access(), call a new helper functionmay_refer(), which only solves the asymmetric case. Previously, the code interleaved the checks for the two symmetric cases inRENAME_EXCHANGE. It feels that the code is clearer when renames withoutRENAME_EXCHANGEare more obviously the normal case.Signed-off-by Günther Noack <[email protected]>--- security/landlock/access.h | 10 +- security/landlock/audit.c | 155 ++++++---------- security/landlock/audit.h | 3 +- security/landlock/domain.c | 120 +++---------- security/landlock/domain.h | 6 +- security/landlock/fs.c | 350 ++++++++++++++++-------------------- security/landlock/net.c | 10 +- security/landlock/ruleset.c | 78 +++----- security/landlock/ruleset.h | 18 +- 9 files changed, 290 insertions(+), 460 deletions(-)security/landlock/access.h +5
-5@@ -61,14 +61,14 @@ union access_masks_all {static_assert (sizeof (typeof_member (union access_masks_all ,masks )) ==sizeof (typeof_member (union access_masks_all ,all ))); -typedef u16 layer_mask_t ; - -/* Makes sure all layers can be checked. */ -static_assert (BITS_PER_TYPE (layer_mask_t) >=LANDLOCK_MAX_NUM_LAYERS ); -/* * Tracks domains responsible of a denied access. This is required to avoid * storing in each object the full layer_masks[] required by update_request(). +* +* Each nibble represents the layer index of the newest layer which denied a +* certain access right. For file system access rights, the upper four bits are +* the index of the layer which denies LANDLOCK_ACCESS_FS_IOCTL_DEV and the +* lower nibble represents LANDLOCK_ACCESS_FS_TRUNCATE. */ typedef u8 deny_masks_t ;security/landlock/audit.c +1
-12@@ -182,36 +182,18 @@ static void test_get_hierarchy(struct kunit *const test)static size_t get_denied_layer (const struct landlock_ruleset *const domain,access_mask_t *const access_request, -const layer_mask_t (*const layer_masks)[], -const size_t layer_masks_size) +const struct layer_access_masks *masks) { -const unsigned long access_req = *access_request; -unsigned long access_bit; -access_mask_t missing =0 ; -long youngest_layer =-1 ; - -for_each_set_bit (access_bit, &access_req, layer_masks_size) { -const access_mask_t mask = (*layer_masks)[access_bit]; -long layer; - -if (!mask)
security/landlock/audit.c static void test_get_hierarchy(struct kunit *const test) · line 187
Quoted 10 lines
security/landlock/audit.c +1
-9-continue ; - -/* __fls(1) == 0 */ - layer =__fls (mask); -if (layer > youngest_layer) { - youngest_layer = layer; - missing =BIT (access_bit); - }else if (layer == youngest_layer) { - missing |=BIT (access_bit); +for (int i =LANDLOCK_MAX_NUM_LAYERS -1 ; i >=0 ; i--) {
All the loop indexes should be size_t (same as before).
Instead of LANDLOCK_MAX_NUM_LAYERS, ARRAY_SIZE(masks->access) would be better.
Quoted 84 lines
security/landlock/audit.c +18
-30+if (masks->access[i] & *access_request) { + *access_request &= masks->access[i]; +return i; } } - *access_request = missing; -if (youngest_layer ==-1 ) -return domain->num_layers -1 ; - -return youngest_layer; +/* Not found - fall back to default values */ + *access_request =0 ; +return domain->num_layers -1 ; }#ifdef CONFIG_SECURITY_LANDLOCK_KUNIT_TEST @@ -221,94 +203,82 @@ static void test_get_denied_layer(struct kunit *const test)const struct landlock_ruleset dom = { .num_layers =5 , }; -const layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_FS ] = { - [BIT_INDEX (LANDLOCK_ACCESS_FS_EXECUTE )] =BIT (0 ), - [BIT_INDEX (LANDLOCK_ACCESS_FS_READ_FILE )] =BIT (1 ), - [BIT_INDEX (LANDLOCK_ACCESS_FS_READ_DIR )] =BIT (1 ) |BIT (0 ), - [BIT_INDEX (LANDLOCK_ACCESS_FS_REMOVE_DIR )] =BIT (2 ), +const struct layer_access_masks masks = { + .access[0 ] =LANDLOCK_ACCESS_FS_EXECUTE | +LANDLOCK_ACCESS_FS_READ_DIR , + .access[1 ] =LANDLOCK_ACCESS_FS_READ_FILE | +LANDLOCK_ACCESS_FS_READ_DIR , + .access[2 ] =LANDLOCK_ACCESS_FS_REMOVE_DIR , };access_mask_t access; access =LANDLOCK_ACCESS_FS_EXECUTE ; -KUNIT_EXPECT_EQ (test,0 , -get_denied_layer (&dom, &access, &layer_masks, -sizeof (layer_masks))); +KUNIT_EXPECT_EQ (test,0 ,get_denied_layer (&dom, &access, &masks));KUNIT_EXPECT_EQ (test, access,LANDLOCK_ACCESS_FS_EXECUTE ); access =LANDLOCK_ACCESS_FS_READ_FILE ; -KUNIT_EXPECT_EQ (test,1 , -get_denied_layer (&dom, &access, &layer_masks, -sizeof (layer_masks))); +KUNIT_EXPECT_EQ (test,1 ,get_denied_layer (&dom, &access, &masks));KUNIT_EXPECT_EQ (test, access,LANDLOCK_ACCESS_FS_READ_FILE ); access =LANDLOCK_ACCESS_FS_READ_DIR ; -KUNIT_EXPECT_EQ (test,1 , -get_denied_layer (&dom, &access, &layer_masks, -sizeof (layer_masks))); +KUNIT_EXPECT_EQ (test,1 ,get_denied_layer (&dom, &access, &masks));KUNIT_EXPECT_EQ (test, access,LANDLOCK_ACCESS_FS_READ_DIR ); access =LANDLOCK_ACCESS_FS_READ_FILE |LANDLOCK_ACCESS_FS_READ_DIR ; -KUNIT_EXPECT_EQ (test,1 , -get_denied_layer (&dom, &access, &layer_masks, -sizeof (layer_masks))); +KUNIT_EXPECT_EQ (test,1 ,get_denied_layer (&dom, &access, &masks));KUNIT_EXPECT_EQ (test, access,LANDLOCK_ACCESS_FS_READ_FILE |LANDLOCK_ACCESS_FS_READ_DIR ); access =LANDLOCK_ACCESS_FS_EXECUTE |LANDLOCK_ACCESS_FS_READ_DIR ; -KUNIT_EXPECT_EQ (test,1 , -get_denied_layer (&dom, &access, &layer_masks, -sizeof (layer_masks))); +KUNIT_EXPECT_EQ (test,1 ,get_denied_layer (&dom, &access, &masks));KUNIT_EXPECT_EQ (test, access,LANDLOCK_ACCESS_FS_READ_DIR ); access =LANDLOCK_ACCESS_FS_WRITE_FILE ; -KUNIT_EXPECT_EQ (test,4 , -get_denied_layer (&dom, &access, &layer_masks, -sizeof (layer_masks))); +KUNIT_EXPECT_EQ (test,4 ,get_denied_layer (&dom, &access, &masks)); KUNIT_EXPECT_EQ(test, access, 0); }#endif /* CONFIG_SECURITY_LANDLOCK_KUNIT_TEST */ -static size_t -get_layer_from_deny_masks(access_mask_t *const access_request,
security/landlock/audit.c static void test_get_denied_layer(struct kunit *const test) · line 251
Quoted 10 lines
security/landlock/audit.c +8
-2-const access_mask_t all_existing_optional_access, - const deny_masks_t deny_masks) +/* +* get_layer_from_fs_deny_masks - get the layer which denied the access request +* +* As a side effect, stores the denied access rights from that layer(!) in +* *access_request. +*/ +static size_t get_layer_from_fs_deny_masks (access_mask_t *const access_request, +const deny_masks_t deny_masks)
I'm not a fan of this change. We come from a generic approach to a specific and hardcoded one. This is simpler *for now*, but could we get a better implementation?
Anyway, please create at least a dedicated patch for the non-transposition changes.
Quoted 25 lines
security/landlock/audit.c +6
-14{ -const unsigned long access_opt = all_existing_optional_access; -const unsigned long access_req = *access_request; -access_mask_t missing =0 ; +const access_mask_t access_req = *access_request;size_t youngest_layer =0 ; -size_t access_index =0 ; -unsigned long access_bit; +access_mask_t missing =0 ; -/* This will require change with new object types. */ -WARN_ON_ONCE (access_opt != _LANDLOCK_ACCESS_FS_OPTIONAL); +WARN_ON_ONCE ((access_req | _LANDLOCK_ACCESS_FS_OPTIONAL) != + _LANDLOCK_ACCESS_FS_OPTIONAL); -for_each_set_bit (access_bit, &access_opt, -BITS_PER_TYPE (access_mask_t)) { -if (access_req &BIT (access_bit)) { -const size_t layer = - (deny_masks >> (access_index *4 )) & - (LANDLOCK_MAX_NUM_LAYERS -1 ); +if (access_req &LANDLOCK_ACCESS_FS_TRUNCATE ) { +size_t layer = deny_masks &0x0f ; -if (layer > youngest_layer) {
security/landlock/audit.c static void test_get_denied_layer(struct kunit *const test) · line 265
Quoted 10 lines
security/landlock/audit.c +2
-7- youngest_layer = layer; - missing =BIT (access_bit); - }else if (layer == youngest_layer) { - missing |=BIT (access_bit); - } - } - access_index++; + missing |=LANDLOCK_ACCESS_FS_TRUNCATE ; + youngest_layer =max (youngest_layer, layer); }
Add a new line.
security/landlock/audit.c static void test_get_denied_layer(struct kunit *const test) · line 274
Quoted 9 lines
security/landlock/audit.c +8
-0+if (access_req &LANDLOCK_ACCESS_FS_IOCTL_DEV ) { +size_t layer = (deny_masks &0xf0 ) >>4 ; +if (layer > youngest_layer) + missing =0 ; + + missing |=LANDLOCK_ACCESS_FS_IOCTL_DEV ; + youngest_layer =max (youngest_layer, layer); + }
Add a new line.
Quoted 198 lines
security/landlock/audit.c +12
-25*access_request = missing;return youngest_layer; }#ifdef CONFIG_SECURITY_LANDLOCK_KUNIT_TEST -static void test_get_layer_from_deny_masks (struct kunit *const test) +static void test_get_layer_from_fs_deny_masks (struct kunit *const test) {deny_masks_t deny_mask;access_mask_t access;@@ -318,16 +288,12 @@ static void test_get_layer_from_deny_masks(struct kunit *const test) access =LANDLOCK_ACCESS_FS_TRUNCATE ;KUNIT_EXPECT_EQ (test,0 , -get_layer_from_deny_masks (&access, - _LANDLOCK_ACCESS_FS_OPTIONAL, - deny_mask)); +get_layer_from_fs_deny_masks (&access, deny_mask));KUNIT_EXPECT_EQ (test, access,LANDLOCK_ACCESS_FS_TRUNCATE ); access =LANDLOCK_ACCESS_FS_TRUNCATE |LANDLOCK_ACCESS_FS_IOCTL_DEV ;KUNIT_EXPECT_EQ (test,2 , -get_layer_from_deny_masks (&access, - _LANDLOCK_ACCESS_FS_OPTIONAL, - deny_mask)); +get_layer_from_fs_deny_masks (&access, deny_mask));KUNIT_EXPECT_EQ (test, access,LANDLOCK_ACCESS_FS_IOCTL_DEV );/* truncate:15 ioctl_dev:15 */ @@ -335,16 +301,12 @@ static void test_get_layer_from_deny_masks(struct kunit *const test) access =LANDLOCK_ACCESS_FS_TRUNCATE ;KUNIT_EXPECT_EQ (test,15 , -get_layer_from_deny_masks (&access, - _LANDLOCK_ACCESS_FS_OPTIONAL, - deny_mask)); +get_layer_from_fs_deny_masks (&access, deny_mask));KUNIT_EXPECT_EQ (test, access,LANDLOCK_ACCESS_FS_TRUNCATE ); access =LANDLOCK_ACCESS_FS_TRUNCATE |LANDLOCK_ACCESS_FS_IOCTL_DEV ;KUNIT_EXPECT_EQ (test,15 , -get_layer_from_deny_masks (&access, - _LANDLOCK_ACCESS_FS_OPTIONAL, - deny_mask)); +get_layer_from_fs_deny_masks (&access, deny_mask));KUNIT_EXPECT_EQ (test, access,LANDLOCK_ACCESS_FS_TRUNCATE |LANDLOCK_ACCESS_FS_IOCTL_DEV );@@ -361,18 +323,15 @@ static bool is_valid_request(const struct landlock_request *const request)return false;if (request->access) { -if (WARN_ON_ONCE (!(!!request->layer_masks ^ +if (WARN_ON_ONCE (!(!!request->masks ^ !!request->all_existing_optional_access)))return false; }else { -if (WARN_ON_ONCE (request->layer_masks || +if (WARN_ON_ONCE (request->masks || request->all_existing_optional_access))return false; } -if (WARN_ON_ONCE (!!request->layer_masks ^ !!request->layer_masks_size)) -return false; -if (request->deny_masks) {if (WARN_ON_ONCE (!request->all_existing_optional_access))return false;@@ -405,14 +364,12 @@ void landlock_log_denial(const struct landlock_cred_security *const subject, missing = request->access;if (missing) {/* Gets the nearest domain that denies the request. */ -if (request->layer_masks) { +if (request->masks) { youngest_layer =get_denied_layer ( - subject->domain, &missing, request->layer_masks, - request->layer_masks_size); + subject->domain, &missing, request->masks); }else { - youngest_layer =get_layer_from_deny_masks ( - &missing, request->all_existing_optional_access, - request->deny_masks); + youngest_layer =get_layer_from_fs_deny_masks ( + &missing, request->deny_masks); } youngest_denied =get_hierarchy (subject->domain, youngest_layer);@@ -507,7 +464,7 @@ static struct kunit_case test_cases[] = {/* clang-format off */ KUNIT_CASE (test_get_hierarchy),KUNIT_CASE (test_get_denied_layer), - KUNIT_CASE(test_get_layer_from_deny_masks), + KUNIT_CASE(test_get_layer_from_fs_deny_masks), {} /* clang-format on */ };security/landlock/audit.h +1
-2@@ -43,8 +43,7 @@ struct landlock_request {access_mask_t access;/* Required fields for requests with layer masks. */ -const layer_mask_t (*layer_masks)[]; -size_t layer_masks_size; +const struct layer_access_masks *masks;/* Required fields for requests with deny masks. */ const access_mask_t all_existing_optional_access;security/landlock/domain.c +5
-63@@ -23,6 +23,7 @@#include "common.h" #include "domain.h" #include "id.h" +#include "limits.h" #ifdef CONFIG_AUDIT @@ -133,111 +134,47 @@ int landlock_init_hierarchy_log(struct landlock_hierarchy *const hierarchy)return 0 ; } -static deny_masks_t -get_layer_deny_mask (const access_mask_t all_existing_optional_access, -const unsigned long access_bit,const size_t layer) -{ -unsigned long access_weight; - -/* This may require change with new object types. */ -WARN_ON_ONCE (all_existing_optional_access != - _LANDLOCK_ACCESS_FS_OPTIONAL); - -if (WARN_ON_ONCE (layer >=LANDLOCK_MAX_NUM_LAYERS )) -return 0 ; - - access_weight =hweight_long (all_existing_optional_access & -GENMASK (access_bit,0 )); -if (WARN_ON_ONCE (access_weight <1 )) -return 0 ; - -return layer - << ((access_weight -1 ) *HWEIGHT (LANDLOCK_MAX_NUM_LAYERS -1 )); -} - -#ifdef CONFIG_SECURITY_LANDLOCK_KUNIT_TEST - -static void test_get_layer_deny_mask (struct kunit *const test) -{ -const unsigned long truncate =BIT_INDEX (LANDLOCK_ACCESS_FS_TRUNCATE ); -const unsigned long ioctl_dev =BIT_INDEX (LANDLOCK_ACCESS_FS_IOCTL_DEV ); - -KUNIT_EXPECT_EQ (test,0 , -get_layer_deny_mask (_LANDLOCK_ACCESS_FS_OPTIONAL, - truncate,0 )); -KUNIT_EXPECT_EQ (test,0x3 , -get_layer_deny_mask (_LANDLOCK_ACCESS_FS_OPTIONAL, - truncate,3 )); - -KUNIT_EXPECT_EQ (test,0 , -get_layer_deny_mask (_LANDLOCK_ACCESS_FS_OPTIONAL, - ioctl_dev,0 )); -KUNIT_EXPECT_EQ (test,0xf0 , -get_layer_deny_mask (_LANDLOCK_ACCESS_FS_OPTIONAL, - ioctl_dev,15 )); -} - -#endif /* CONFIG_SECURITY_LANDLOCK_KUNIT_TEST */ -deny_masks_t -landlock_get_deny_masks (const access_mask_t all_existing_optional_access, -const access_mask_t optional_access, -const layer_mask_t (*const layer_masks)[], -const size_t layer_masks_size) +landlock_get_fs_deny_masks (const access_mask_t optional_access, +const struct layer_access_masks *layer_masks) { -const unsigned long access_opt = optional_access; -unsigned long access_bit; -deny_masks_t deny_masks =0 ; +u8 truncate_layer =0 ; +u8 ioctl_dev_layer =0 ; -/* This may require change with new object types. */ -WARN_ON_ONCE (access_opt != - (optional_access & all_existing_optional_access)); - -if (WARN_ON_ONCE (!layer_masks)) -return 0 ; - -if (WARN_ON_ONCE (!access_opt)) -return 0 ; -
security/landlock/domain.c int landlock_init_hierarchy_log(struct landlock_hierarchy *const hierarchy) · line 144
Quoted 10 lines
security/landlock/domain.c +1
-9-for_each_set_bit (access_bit, &access_opt, layer_masks_size) { -const layer_mask_t mask = (*layer_masks)[access_bit]; - -if (!mask) -continue ; - -/* __fls(1) == 0 */ - deny_masks |=get_layer_deny_mask (all_existing_optional_access, - access_bit,__fls (mask)); +for (int i =0 ; i <LANDLOCK_MAX_NUM_LAYERS ; i++) {
size_t i
ARRAY_SIZE(masks->access)
Quoted 90 lines
security/landlock/domain.c +22
-20+if (layer_masks->access[i] & optional_access & +LANDLOCK_ACCESS_FS_TRUNCATE ) + truncate_layer = i; +if (layer_masks->access[i] & optional_access & +LANDLOCK_ACCESS_FS_IOCTL_DEV ) + ioctl_dev_layer = i; } -return deny_masks; +return ((ioctl_dev_layer <<4 ) &0xf0 ) | (truncate_layer &0x0f ); }#ifdef CONFIG_SECURITY_LANDLOCK_KUNIT_TEST -static void test_landlock_get_deny_masks (struct kunit *const test) +static void test_landlock_get_fs_deny_masks (struct kunit *const test) { -const layer_mask_t layers1[BITS_PER_TYPE (access_mask_t)] = { - [BIT_INDEX (LANDLOCK_ACCESS_FS_EXECUTE )] =BIT_ULL (0 ) | -BIT_ULL (9 ), - [BIT_INDEX (LANDLOCK_ACCESS_FS_TRUNCATE )] =BIT_ULL (1 ), - [BIT_INDEX (LANDLOCK_ACCESS_FS_IOCTL_DEV )] =BIT_ULL (2 ) | -BIT_ULL (0 ), +const struct layer_access_masks layers1 = { + .access[0 ] =LANDLOCK_ACCESS_FS_EXECUTE | +LANDLOCK_ACCESS_FS_IOCTL_DEV , + .access[1 ] =LANDLOCK_ACCESS_FS_TRUNCATE , + .access[2 ] =LANDLOCK_ACCESS_FS_IOCTL_DEV , + .access[9 ] =LANDLOCK_ACCESS_FS_EXECUTE , };KUNIT_EXPECT_EQ (test,0x1 , -landlock_get_deny_masks (_LANDLOCK_ACCESS_FS_OPTIONAL, -LANDLOCK_ACCESS_FS_TRUNCATE , - &layers1,ARRAY_SIZE (layers1))); +landlock_get_fs_deny_masks (LANDLOCK_ACCESS_FS_TRUNCATE , + &layers1));KUNIT_EXPECT_EQ (test,0x20 , -landlock_get_deny_masks (_LANDLOCK_ACCESS_FS_OPTIONAL, -LANDLOCK_ACCESS_FS_IOCTL_DEV , - &layers1,ARRAY_SIZE (layers1))); +landlock_get_fs_deny_masks (LANDLOCK_ACCESS_FS_IOCTL_DEV , + &layers1));KUNIT_EXPECT_EQ ( test,0x21 , -landlock_get_deny_masks (_LANDLOCK_ACCESS_FS_OPTIONAL, -LANDLOCK_ACCESS_FS_TRUNCATE | -LANDLOCK_ACCESS_FS_IOCTL_DEV , - &layers1,ARRAY_SIZE (layers1))); +landlock_get_fs_deny_masks (LANDLOCK_ACCESS_FS_TRUNCATE | +LANDLOCK_ACCESS_FS_IOCTL_DEV , + &layers1)); }#endif /* CONFIG_SECURITY_LANDLOCK_KUNIT_TEST */ @@ -246,8 +183,7 @@ static void test_landlock_get_deny_masks(struct kunit *const test)static struct kunit_case test_cases[] = {/* clang-format off */ -KUNIT_CASE (test_get_layer_deny_mask), -KUNIT_CASE (test_landlock_get_deny_masks), +KUNIT_CASE (test_landlock_get_fs_deny_masks), {}/* clang-format on */ };security/landlock/domain.h +2
-4@@ -120,10 +120,8 @@ struct landlock_hierarchy {#ifdef CONFIG_AUDIT deny_masks_t -landlock_get_deny_masks (const access_mask_t all_existing_optional_access, -const access_mask_t optional_access, -const layer_mask_t (*const layer_masks)[], -size_t layer_masks_size); +landlock_get_fs_deny_masks (const access_mask_t optional_access, +const struct layer_access_masks *layer_masks);int landlock_init_hierarchy_log (struct landlock_hierarchy *const hierarchy);security/landlock/fs.c +1
-0@@ -407,57 +407,55 @@ static bool access_mask_subset(access_mask_t a, access_mask_t b)return (a | b) == b; } +/*
security/landlock/fs.c static bool access_mask_subset(access_mask_t a, access_mask_t b) · line 420
Quoted 10 lines
security/landlock/fs.c +10
-0+* Returns true iff the child file with the given src_child access rights under +* src_parent would result in having the same or fewer access rights if it were +* moved under new_parent. +*/ +static bool may_refer (const struct layer_access_masks *const src_parent, +const struct layer_access_masks *const src_child, +const struct layer_access_masks *const new_parent, +const bool child_is_dir) +{ +for (int i =0 ; i <LANDLOCK_MAX_NUM_LAYERS ; i++) {
size_t i
ARRAY_SIZE(src_child->access)
Quoted 151 lines
security/landlock/fs.c +50
-66+access_mask_t child_access = src_parent->access[i] & + src_child->access[i]; +access_mask_t parent_access = new_parent->access[i]; + +if (!child_is_dir) { + child_access &=ACCESS_FILE ; + parent_access &=ACCESS_FILE ; + } + +if (!access_mask_subset (child_access, parent_access)) +return false; + } +return true; +} +/* * Check that a destination file hierarchy has more restrictions than a source * file hierarchy. This is only used for link and rename actions. * -* @layer_masks_child2: Optional child masks. +* Returns: true if child1 may be moved from parent1 to parent2 without +* increasing its access rights. If child2 is set, an additional condition is +* that child2 may be used from parent2 to parent1 without increasing its access +* rights. */ -static bool no_more_access ( -const layer_mask_t (*const layer_masks_parent1)[LANDLOCK_NUM_ACCESS_FS ], -const layer_mask_t (*const layer_masks_child1)[LANDLOCK_NUM_ACCESS_FS ], -const bool child1_is_directory, -const layer_mask_t (*const layer_masks_parent2)[LANDLOCK_NUM_ACCESS_FS ], -const layer_mask_t (*const layer_masks_child2)[LANDLOCK_NUM_ACCESS_FS ], -const bool child2_is_directory) +static bool no_more_access (const struct layer_access_masks *const parent1, +const struct layer_access_masks *const child1, +const bool child1_is_dir, +const struct layer_access_masks *const parent2, +const struct layer_access_masks *const child2, +const bool child2_is_dir) { -unsigned long access_bit; +if (!may_refer (parent1, child1, parent2, child1_is_dir)) +return false; -for (access_bit =0 ; access_bit <ARRAY_SIZE (*layer_masks_parent2); - access_bit++) { -/* Ignores accesses that only make sense for directories. */ -const bool is_file_access = - !!(BIT_ULL (access_bit) &ACCESS_FILE ); +if (!child2) +return true; -if (child1_is_directory || is_file_access) { -/* -* Checks if the destination restrictions are a -* superset of the source ones (i.e. inherited access -* rights without child exceptions): -* restrictions(parent2) >= restrictions(child1) -*/ -if ((((*layer_masks_parent1)[access_bit] & - (*layer_masks_child1)[access_bit]) | - (*layer_masks_parent2)[access_bit]) != - (*layer_masks_parent2)[access_bit]) -return false; - } - -if (!layer_masks_child2) -continue ; -if (child2_is_directory || is_file_access) { -/* -* Checks inverted restrictions for RENAME_EXCHANGE: -* restrictions(parent1) >= restrictions(child2) -*/ -if ((((*layer_masks_parent2)[access_bit] & - (*layer_masks_child2)[access_bit]) | - (*layer_masks_parent1)[access_bit]) != - (*layer_masks_parent1)[access_bit]) -return false; - } - } -return true; +return may_refer (parent2, child2, parent1, child2_is_dir); }#define NMA_TRUE (...) KUNIT_EXPECT_TRUE(test, no_more_access(__VA_ARGS__))@@ -467,25 +465,25 @@ static bool no_more_access(static void test_no_more_access (struct kunit *const test) { -const layer_mask_t rx0[LANDLOCK_NUM_ACCESS_FS ] = { - [BIT_INDEX (LANDLOCK_ACCESS_FS_EXECUTE )] =BIT_ULL (0 ), - [BIT_INDEX (LANDLOCK_ACCESS_FS_READ_FILE )] =BIT_ULL (0 ), +const struct layer_access_masks rx0 = { + .access[0 ] =LANDLOCK_ACCESS_FS_EXECUTE | +LANDLOCK_ACCESS_FS_READ_FILE , }; -const layer_mask_t mx0[LANDLOCK_NUM_ACCESS_FS ] = { - [BIT_INDEX (LANDLOCK_ACCESS_FS_EXECUTE )] =BIT_ULL (0 ), - [BIT_INDEX (LANDLOCK_ACCESS_FS_MAKE_REG )] =BIT_ULL (0 ), +const struct layer_access_masks mx0 = { + .access[0 ] =LANDLOCK_ACCESS_FS_EXECUTE | +LANDLOCK_ACCESS_FS_MAKE_REG , }; -const layer_mask_t x0[LANDLOCK_NUM_ACCESS_FS ] = { - [BIT_INDEX (LANDLOCK_ACCESS_FS_EXECUTE )] =BIT_ULL (0 ), +const struct layer_access_masks x0 = { + .access[0 ] =LANDLOCK_ACCESS_FS_EXECUTE , }; -const layer_mask_t x1[LANDLOCK_NUM_ACCESS_FS ] = { - [BIT_INDEX (LANDLOCK_ACCESS_FS_EXECUTE )] =BIT_ULL (1 ), +const struct layer_access_masks x1 = { + .access[1 ] =LANDLOCK_ACCESS_FS_EXECUTE , }; -const layer_mask_t x01[LANDLOCK_NUM_ACCESS_FS ] = { - [BIT_INDEX (LANDLOCK_ACCESS_FS_EXECUTE )] =BIT_ULL (0 ) | -BIT_ULL (1 ), +const struct layer_access_masks x01 = { + .access[0 ] =LANDLOCK_ACCESS_FS_EXECUTE , + .access[1 ] =LANDLOCK_ACCESS_FS_EXECUTE , }; -const layer_mask_t allows_all[LANDLOCK_NUM_ACCESS_FS ] = {}; +const struct layer_access_masks allows_all = {};/* Checks without restriction. */ NMA_TRUE (&x0, &allows_all, false, &allows_all,NULL , false);@@ -573,31 +571,30 @@ static void test_no_more_access(struct kunit *const test)#undef NMA_TRUE#undef NMA_FALSE -static bool is_layer_masks_allowed ( -layer_mask_t (*const layer_masks)[LANDLOCK_NUM_ACCESS_FS ]) +static bool is_layer_masks_allowed (const struct layer_access_masks *masks) { -return !memchr_inv (layer_masks,0 ,sizeof (*layer_masks)); +return !memchr_inv (&masks->access,0 ,sizeof (masks->access)); }/* -* Removes @layer_masks accesses that are not requested. +* Removes @masks accesses that are not requested. * * Returns true if the request is allowed, false otherwise. */ -static bool -scope_to_request (const access_mask_t access_request, -layer_mask_t (*const layer_masks)[LANDLOCK_NUM_ACCESS_FS ]) +static bool scope_to_request (const access_mask_t access_request, +struct layer_access_masks *masks) { -const unsigned long access_req = access_request; -unsigned long access_bit; +bool saw_unfulfilled_access = false;
security/landlock/fs.c static void test_no_more_access(struct kunit *const test) · line 592
Quoted 10 lines
security/landlock/fs.c +2
-5-if (WARN_ON_ONCE (!layer_masks)) +if (WARN_ON_ONCE (!masks))return true; -for_each_clear_bit (access_bit, &access_req,ARRAY_SIZE (*layer_masks)) - (*layer_masks)[access_bit] =0 ; - -return is_layer_masks_allowed (layer_masks); +for (int i =0 ; i <LANDLOCK_MAX_NUM_LAYERS ; i++) {
size_t i
ARRAY_SIZE(masks->access)
Quoted 85 lines
security/landlock/fs.c +23
-28+ masks->access[i] &= access_request; +if (masks->access[i]) + saw_unfulfilled_access = true; + } +return !saw_unfulfilled_access; }#ifdef CONFIG_SECURITY_LANDLOCK_KUNIT_TEST @@ -605,48 +602,41 @@ scope_to_request(const access_mask_t access_request,static void test_scope_to_request_with_exec_none (struct kunit *const test) {/* Allows everything. */ -layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_FS ] = {}; +struct layer_access_masks masks = {};/* Checks and scopes with execute. */ -KUNIT_EXPECT_TRUE (test,scope_to_request (LANDLOCK_ACCESS_FS_EXECUTE , - &layer_masks)); -KUNIT_EXPECT_EQ (test,0 , - layer_masks[BIT_INDEX (LANDLOCK_ACCESS_FS_EXECUTE )]); -KUNIT_EXPECT_EQ (test,0 , - layer_masks[BIT_INDEX (LANDLOCK_ACCESS_FS_WRITE_FILE )]); +KUNIT_EXPECT_TRUE (test, +scope_to_request (LANDLOCK_ACCESS_FS_EXECUTE , &masks)); +KUNIT_EXPECT_EQ (test,0 , masks.access[0 ]); }static void test_scope_to_request_with_exec_some (struct kunit *const test) {/* Denies execute and write. */ -layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_FS ] = { - [BIT_INDEX (LANDLOCK_ACCESS_FS_EXECUTE )] =BIT_ULL (0 ), - [BIT_INDEX (LANDLOCK_ACCESS_FS_WRITE_FILE )] =BIT_ULL (1 ), +struct layer_access_masks masks = { + .access[0 ] =LANDLOCK_ACCESS_FS_EXECUTE , + .access[1 ] =LANDLOCK_ACCESS_FS_WRITE_FILE , };/* Checks and scopes with execute. */ KUNIT_EXPECT_FALSE (test,scope_to_request (LANDLOCK_ACCESS_FS_EXECUTE , - &layer_masks)); -KUNIT_EXPECT_EQ (test,BIT_ULL (0 ), - layer_masks[BIT_INDEX (LANDLOCK_ACCESS_FS_EXECUTE )]); -KUNIT_EXPECT_EQ (test,0 , - layer_masks[BIT_INDEX (LANDLOCK_ACCESS_FS_WRITE_FILE )]); + &masks)); +KUNIT_EXPECT_EQ (test,LANDLOCK_ACCESS_FS_EXECUTE , masks.access[0 ]); +KUNIT_EXPECT_EQ (test,0 , masks.access[1 ]); }static void test_scope_to_request_without_access (struct kunit *const test) {/* Denies execute and write. */ -layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_FS ] = { - [BIT_INDEX (LANDLOCK_ACCESS_FS_EXECUTE )] =BIT_ULL (0 ), - [BIT_INDEX (LANDLOCK_ACCESS_FS_WRITE_FILE )] =BIT_ULL (1 ), +struct layer_access_masks masks = { + .access[0 ] =LANDLOCK_ACCESS_FS_EXECUTE , + .access[1 ] =LANDLOCK_ACCESS_FS_WRITE_FILE , };/* Checks and scopes without access request. */ -KUNIT_EXPECT_TRUE (test,scope_to_request (0 , &layer_masks)); -KUNIT_EXPECT_EQ (test,0 , - layer_masks[BIT_INDEX (LANDLOCK_ACCESS_FS_EXECUTE )]); -KUNIT_EXPECT_EQ (test,0 , - layer_masks[BIT_INDEX (LANDLOCK_ACCESS_FS_WRITE_FILE )]); +KUNIT_EXPECT_TRUE (test,scope_to_request (0 , &masks)); +KUNIT_EXPECT_EQ (test,0 , masks.access[0 ]); +KUNIT_EXPECT_EQ (test,0 , masks.access[1 ]); }#endif /* CONFIG_SECURITY_LANDLOCK_KUNIT_TEST */ @@ -655,20 +645,16 @@ static void test_scope_to_request_without_access(struct kunit *const test)* Returns true if there is at least one access right different than * LANDLOCK_ACCESS_FS_REFER. */ -static bool -is_eacces (const layer_mask_t (*const layer_masks)[LANDLOCK_NUM_ACCESS_FS ], -const access_mask_t access_request) +static bool is_eacces (const struct layer_access_masks *masks, +const access_mask_t access_request) { -unsigned long access_bit; -/* LANDLOCK_ACCESS_FS_REFER alone must return -EXDEV. */
security/landlock/fs.c static void test_scope_to_request_without_access(struct kunit *const test) · line 654
Quoted 10 lines
security/landlock/fs.c +2
-6-const unsigned long access_check = access_request & - ~LANDLOCK_ACCESS_FS_REFER ; - -if (!layer_masks) +if (!masks)return false; -for_each_set_bit (access_bit, &access_check,ARRAY_SIZE (*layer_masks)) { -if ((*layer_masks)[access_bit]) +for (int i =0 ; i <LANDLOCK_MAX_NUM_LAYERS ; i++) {
size_t i
ARRAY_SIZE(masks->access)
Quoted 258 lines
security/landlock/fs.c +65
-77+/* LANDLOCK_ACCESS_FS_REFER alone must return -EXDEV. */ +if (masks->access[i] & access_request & + ~LANDLOCK_ACCESS_FS_REFER )return true; }return false;@@ -681,37 +667,37 @@ is_eacces(const layer_mask_t (*const layer_masks)[LANDLOCK_NUM_ACCESS_FS],static void test_is_eacces_with_none (struct kunit *const test) { -const layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_FS ] = {}; +const struct layer_access_masks masks = {}; -IE_FALSE (&layer_masks,0 ); -IE_FALSE (&layer_masks,LANDLOCK_ACCESS_FS_REFER ); -IE_FALSE (&layer_masks,LANDLOCK_ACCESS_FS_EXECUTE ); -IE_FALSE (&layer_masks,LANDLOCK_ACCESS_FS_WRITE_FILE ); +IE_FALSE (&masks,0 ); +IE_FALSE (&masks,LANDLOCK_ACCESS_FS_REFER ); +IE_FALSE (&masks,LANDLOCK_ACCESS_FS_EXECUTE ); +IE_FALSE (&masks,LANDLOCK_ACCESS_FS_WRITE_FILE ); }static void test_is_eacces_with_refer (struct kunit *const test) { -const layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_FS ] = { - [BIT_INDEX (LANDLOCK_ACCESS_FS_REFER )] =BIT_ULL (0 ), +const struct layer_access_masks masks = { + .access[0 ] =LANDLOCK_ACCESS_FS_REFER , }; -IE_FALSE (&layer_masks,0 ); -IE_FALSE (&layer_masks,LANDLOCK_ACCESS_FS_REFER ); -IE_FALSE (&layer_masks,LANDLOCK_ACCESS_FS_EXECUTE ); -IE_FALSE (&layer_masks,LANDLOCK_ACCESS_FS_WRITE_FILE ); +IE_FALSE (&masks,0 ); +IE_FALSE (&masks,LANDLOCK_ACCESS_FS_REFER ); +IE_FALSE (&masks,LANDLOCK_ACCESS_FS_EXECUTE ); +IE_FALSE (&masks,LANDLOCK_ACCESS_FS_WRITE_FILE ); }static void test_is_eacces_with_write (struct kunit *const test) { -const layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_FS ] = { - [BIT_INDEX (LANDLOCK_ACCESS_FS_WRITE_FILE )] =BIT_ULL (0 ), +const struct layer_access_masks masks = { + .access[0 ] =LANDLOCK_ACCESS_FS_WRITE_FILE , }; -IE_FALSE (&layer_masks,0 ); -IE_FALSE (&layer_masks,LANDLOCK_ACCESS_FS_REFER ); -IE_FALSE (&layer_masks,LANDLOCK_ACCESS_FS_EXECUTE ); +IE_FALSE (&masks,0 ); +IE_FALSE (&masks,LANDLOCK_ACCESS_FS_REFER ); +IE_FALSE (&masks,LANDLOCK_ACCESS_FS_EXECUTE ); -IE_TRUE (&layer_masks,LANDLOCK_ACCESS_FS_WRITE_FILE ); +IE_TRUE (&masks,LANDLOCK_ACCESS_FS_WRITE_FILE ); }#endif /* CONFIG_SECURITY_LANDLOCK_KUNIT_TEST */ @@ -761,26 +747,25 @@ static void test_is_eacces_with_write(struct kunit *const test)* - true if the access request is granted; * - false otherwise. */ -static bool is_access_to_paths_allowed ( -const struct landlock_ruleset *const domain, -const struct path *const path, -const access_mask_t access_request_parent1, -layer_mask_t (*const layer_masks_parent1)[LANDLOCK_NUM_ACCESS_FS ], -struct landlock_request *const log_request_parent1, -struct dentry *const dentry_child1, -const access_mask_t access_request_parent2, -layer_mask_t (*const layer_masks_parent2)[LANDLOCK_NUM_ACCESS_FS ], -struct landlock_request *const log_request_parent2, -struct dentry *const dentry_child2) +static bool +is_access_to_paths_allowed (const struct landlock_ruleset *const domain, +const struct path *const path, +const access_mask_t access_request_parent1, +struct layer_access_masks *layer_masks_parent1, +struct landlock_request *const log_request_parent1, +struct dentry *const dentry_child1, +const access_mask_t access_request_parent2, +struct layer_access_masks *layer_masks_parent2, +struct landlock_request *const log_request_parent2, +struct dentry *const dentry_child2) {bool allowed_parent1 = false, allowed_parent2 = false, is_dom_check, child1_is_directory = true, child2_is_directory = true;struct path walker_path;access_mask_t access_masked_parent1, access_masked_parent2; -layer_mask_t _layer_masks_child1[LANDLOCK_NUM_ACCESS_FS ], - _layer_masks_child2[LANDLOCK_NUM_ACCESS_FS ]; -layer_mask_t (*layer_masks_child1)[LANDLOCK_NUM_ACCESS_FS ] =NULL , - (*layer_masks_child2)[LANDLOCK_NUM_ACCESS_FS ] =NULL ; +struct layer_access_masks _layer_masks_child1, _layer_masks_child2; +struct layer_access_masks *layer_masks_child1 =NULL , + *layer_masks_child2 =NULL ;if (!access_request_parent1 && !access_request_parent2)return true;@@ -820,22 +805,20 @@ static bool is_access_to_paths_allowed( }if (unlikely (dentry_child1)) { -landlock_unmask_layers ( -find_rule (domain, dentry_child1), -landlock_init_layer_masks ( - domain,LANDLOCK_MASK_ACCESS_FS , - &_layer_masks_child1,LANDLOCK_KEY_INODE ), - &_layer_masks_child1,ARRAY_SIZE (_layer_masks_child1)); +if (landlock_init_layer_masks (domain,LANDLOCK_MASK_ACCESS_FS , + &_layer_masks_child1, +LANDLOCK_KEY_INODE )) +landlock_unmask_layers (find_rule (domain, dentry_child1), + &_layer_masks_child1); layer_masks_child1 = &_layer_masks_child1; child1_is_directory =d_is_dir (dentry_child1); }if (unlikely (dentry_child2)) { -landlock_unmask_layers ( -find_rule (domain, dentry_child2), -landlock_init_layer_masks ( - domain,LANDLOCK_MASK_ACCESS_FS , - &_layer_masks_child2,LANDLOCK_KEY_INODE ), - &_layer_masks_child2,ARRAY_SIZE (_layer_masks_child2)); +if (landlock_init_layer_masks (domain,LANDLOCK_MASK_ACCESS_FS , + &_layer_masks_child2, +LANDLOCK_KEY_INODE )) +landlock_unmask_layers (find_rule (domain, dentry_child2), + &_layer_masks_child2); layer_masks_child2 = &_layer_masks_child2; child2_is_directory =d_is_dir (dentry_child2); }@@ -890,16 +873,12 @@ static bool is_access_to_paths_allowed( } rule =find_rule (domain, walker_path.dentry); - allowed_parent1 = allowed_parent1 || -landlock_unmask_layers ( - rule, access_masked_parent1, - layer_masks_parent1, -ARRAY_SIZE (*layer_masks_parent1)); - allowed_parent2 = allowed_parent2 || -landlock_unmask_layers ( - rule, access_masked_parent2, - layer_masks_parent2, -ARRAY_SIZE (*layer_masks_parent2)); + allowed_parent1 = + allowed_parent1 || +landlock_unmask_layers (rule, layer_masks_parent1); + allowed_parent2 = + allowed_parent2 || +landlock_unmask_layers (rule, layer_masks_parent2);/* Stops when a rule from each layer grants access. */ if (allowed_parent1 && allowed_parent2)@@ -953,9 +932,7 @@ static bool is_access_to_paths_allowed( log_request_parent1->audit.type =LSM_AUDIT_DATA_PATH ; log_request_parent1->audit.u.path = *path; log_request_parent1->access = access_masked_parent1; - log_request_parent1->layer_masks = layer_masks_parent1; - log_request_parent1->layer_masks_size = -ARRAY_SIZE (*layer_masks_parent1); + log_request_parent1->masks = layer_masks_parent1; }if (!allowed_parent2) {@@ -963,9 +940,7 @@ static bool is_access_to_paths_allowed( log_request_parent2->audit.type =LSM_AUDIT_DATA_PATH ; log_request_parent2->audit.u.path = *path; log_request_parent2->access = access_masked_parent2; - log_request_parent2->layer_masks = layer_masks_parent2; - log_request_parent2->layer_masks_size = -ARRAY_SIZE (*layer_masks_parent2); + log_request_parent2->masks = layer_masks_parent2; }return allowed_parent1 && allowed_parent2; }@@ -978,7 +953,7 @@ static int current_check_access_path(const struct path *const path, };const struct landlock_cred_security *const subject =landlock_get_applicable_subject (current_cred (), masks,NULL ); -layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_FS ] = {}; +struct layer_access_masks layer_masks;struct landlock_request request = {};if (!subject)@@ -1053,10 +1028,10 @@ static access_mask_t maybe_remove(const struct dentry *const dentry)* - true if all the domain access rights are allowed for @dir; * - false if the walk reached @mnt_root. */ -static bool collect_domain_accesses ( -const struct landlock_ruleset *const domain, -const struct dentry *const mnt_root,struct dentry *dir, -layer_mask_t (*const layer_masks_dom)[LANDLOCK_NUM_ACCESS_FS ]) +static bool collect_domain_accesses (const struct landlock_ruleset *const domain, +const struct dentry *const mnt_root, +struct dentry *dir, +struct layer_access_masks *layer_masks_dom) {unsigned long access_dom;bool ret = false;@@ -1075,9 +1050,8 @@ static bool collect_domain_accesses(struct dentry *parent_dentry;/* Gets all layers allowing all domain accesses. */ -if (landlock_unmask_layers (find_rule (domain, dir), access_dom, - layer_masks_dom, -ARRAY_SIZE (*layer_masks_dom))) { +if (landlock_unmask_layers (find_rule (domain, dir), + layer_masks_dom)) {/* * Stops when all handled accesses are allowed by at * least one rule in each layer. @@ -1165,8 +1139,8 @@ static int current_check_refer_path(struct dentry *const old_dentry,access_mask_t access_request_parent1, access_request_parent2;struct path mnt_dir;struct dentry *old_parent; -layer_mask_t layer_masks_parent1[LANDLOCK_NUM_ACCESS_FS ] = {}, - layer_masks_parent2[LANDLOCK_NUM_ACCESS_FS ] = {}; +struct layer_access_masks layer_masks_parent1 = {}, + layer_masks_parent2 = {};struct landlock_request request1 = {}, request2 = {};if (!subject)@@ -1323,7 +1297,8 @@ static void hook_sb_delete(struct super_block *const sb)* second call to iput() for the same Landlock object. Also * checks I_NEW because such inode cannot be tied to an object. */ -if (inode_state_read (inode) & (I_FREEING |I_WILL_FREE |I_NEW )) { +if (inode_state_read (inode) & + (I_FREEING |I_WILL_FREE |I_NEW )) {spin_unlock (&inode->i_lock);continue ; }@@ -1641,7 +1616,7 @@ static bool is_device(const struct file *const file)static int hook_file_open (struct file *const file) { -layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_FS ] = {}; +struct layer_access_masks layer_masks = {};access_mask_t open_access_request, full_access_request, allowed_access, optional_access; const struct landlock_cred_security *const subject =@@ -1676,20 +1651,14 @@ static int hook_file_open(struct file *const file) &layer_masks, &request, NULL, 0, NULL, NULL, NULL)) { allowed_access = full_access_request; } else { -unsigned long access_bit; -const unsigned long access_req = full_access_request; -/* * Calculate the actual allowed access rights from layer_masks. -* Add each access right to allowed_access which has not been -* vetoed by any layer. +* Remove the access rights from the full access request which
security/landlock/fs.c static int hook_file_open(struct file *const file) · line 1660
Quoted 10 lines
security/landlock/fs.c +3
-6+* are still unfulfilled in any of the layers. */ - allowed_access =0 ; -for_each_set_bit (access_bit, &access_req, -ARRAY_SIZE (layer_masks)) { -if (!layer_masks[access_bit]) - allowed_access |=BIT_ULL (access_bit); - } + allowed_access = full_access_request; +for (int i =0 ; i <LANDLOCK_MAX_NUM_LAYERS ; i++)
size_t i
ARRAY_SIZE(layer_masks.access)
Quoted 30 lines
security/landlock/fs.c +3
-3+ allowed_access &= ~layer_masks.access[i]; }/* @@ -1700,9 +1669,8 @@ static int hook_file_open(struct file *const file)*/ landlock_file (file)->allowed_access = allowed_access;#ifdef CONFIG_AUDIT -landlock_file (file)->deny_masks =landlock_get_deny_masks ( - _LANDLOCK_ACCESS_FS_OPTIONAL, optional_access, &layer_masks, -ARRAY_SIZE (layer_masks)); +landlock_file (file)->deny_masks = +landlock_get_fs_deny_masks (optional_access, &layer_masks);#endif /* CONFIG_AUDIT */ if (access_mask_subset (open_access_request, allowed_access))security/landlock/net.c +1
-1@@ -47,7 +47,7 @@ static int current_check_access_socket(struct socket *const sock, access_mask_t access_request) {__be16 port; -layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_NET ] = {}; +struct layer_access_masks layer_masks = {};const struct landlock_rule *rule;struct landlock_id id = { .type =LANDLOCK_KEY_NET_PORT ,
security/landlock/net.c static int current_check_access_socket(struct socket *const sock, · line 182
Quoted 7 lines
security/landlock/net.c +2
-2@@ -178,8 +178,9 @@ static int current_check_access_socket(struct socket *const sock, access_request =landlock_init_layer_masks (subject->domain, access_request, &layer_masks,LANDLOCK_KEY_NET_PORT ); -if (landlock_unmask_layers (rule, access_request, &layer_masks, -ARRAY_SIZE (layer_masks))) +if (!access_request) +return 0 ;
Add a new line.
Quoted 19 lines
security/landlock/net.c +2
-2+if (landlock_unmask_layers (rule, &layer_masks))return 0 ; audit_net.family = address->sa_family;@@ -189,8 +190,7 @@ static int current_check_access_socket(struct socket *const sock, .audit.type = LSM_AUDIT_DATA_NET, .audit.u.net = &audit_net, .access = access_request, - .layer_masks = &layer_masks, - .layer_masks_size = ARRAY_SIZE(layer_masks), + .masks = &layer_masks, });return -EACCES ; }security/landlock/ruleset.c +0
-0
security/landlock/ruleset.c landlock_find_rule(const struct landlock_ruleset *const ruleset, · line 623
Quoted 2 lines
security/landlock/ruleset.c +0
-0@@ -622,49 +622,24 @@ landlock_find_rule(const struct landlock_ruleset *const ruleset,* request are empty). */
The above doc should be updated with the new arguments. You can also convert it to a proper function docstring (which would have detected this issue).
Quoted 14 lines
security/landlock/ruleset.c +2
-6bool landlock_unmask_layers (const struct landlock_rule *const rule, -const access_mask_t access_request, -layer_mask_t (*const layer_masks)[], -const size_t masks_array_size) +struct layer_access_masks *masks) { -size_t layer_level; - -if (!access_request || !layer_masks) +if (!masks)return true;if (!rule)return false;
security/landlock/ruleset.c landlock_find_rule(const struct landlock_ruleset *const ruleset, · line 645
Quoted 10 lines
security/landlock/ruleset.c +0
-10-/* -* An access is granted if, for each policy layer, at least one rule -* encountered on the pathwalk grants the requested access, -* regardless of its position in the layer stack. We must then check -* the remaining layers for each inode, from the first added layer to -* the last one. When there is multiple requested accesses, for each -* policy layer, the full set of requested accesses may not be granted -* by only one rule, but by the union (binary OR) of multiple rules. -* E.g. /a/b <execute> + /a <read> => /a/b <execute + read> -*/
I'd like to keep most of this comment, even if some parts are specific to FS access, I think it helps understand the logic.
security/landlock/ruleset.c landlock_find_rule(const struct landlock_ruleset *const ruleset, · line 632
Quoted 8 lines
security/landlock/ruleset.c +1
-7-for (layer_level =0 ; layer_level < rule->num_layers; layer_level++) { -const struct landlock_layer *const layer = - &rule->layers[layer_level]; -const layer_mask_t layer_bit =BIT_ULL (layer->level -1 ); -const unsigned long access_req = access_request; -unsigned long access_bit; -bool is_empty; +for (int i =0 ; i < rule->num_layers; i++) {
size_t i
security/landlock/ruleset.c landlock_find_rule(const struct landlock_ruleset *const ruleset, · line 633
Quoted 1 line
security/landlock/ruleset.c +1
-0+const struct landlock_layer *l = &rule->layers[i];
... *const layer = ...
security/landlock/ruleset.c landlock_find_rule(const struct landlock_ruleset *const ruleset, · line 657
Quoted 5 lines
security/landlock/ruleset.c +0
-4-/* -* Records in @layer_masks which layer grants access to each requested -* access: bit cleared if the related layer grants access. -*/
At least the "bit cleared if the related layer grants access" comment should be kept.
security/landlock/ruleset.c landlock_find_rule(const struct landlock_ruleset *const ruleset, · line 635
Quoted 9 lines
security/landlock/ruleset.c +1
-8- is_empty = true; -for_each_set_bit (access_bit, &access_req, masks_array_size) { -if (layer->access &BIT_ULL (access_bit)) - (*layer_masks)[access_bit] &= ~layer_bit; - is_empty = is_empty && !(*layer_masks)[access_bit]; - } -if (is_empty) -return true; + masks->access[l->level -1 ] &= ~l->access;
It's indeed better to get rid of the requested access in this helper.
security/landlock/ruleset.c landlock_find_rule(const struct landlock_ruleset *const ruleset, · line 638
Quoted 4 lines
security/landlock/ruleset.c +2
-1} -return false; + +for (int i =0 ; i <LANDLOCK_MAX_NUM_LAYERS ; i++) {
size_t i
ARRAY_SIZE(masks->access)
security/landlock/ruleset.c get_access_mask_t(const struct landlock_ruleset *const ruleset, · line 682
Quoted 12 lines
security/landlock/ruleset.c +4
-1+if (masks->access[i]) +return false; + } +return true; } typedef access_mask_t@@ -679,8 +654,7 @@ get_access_mask_t(const struct landlock_ruleset *const ruleset,* * @domain: The domain that defines the current restrictions. * @access_request: The requested access rights to check. -* @layer_masks: It must contain %LANDLOCK_NUM_ACCESS_FS or
There is another mention @layer_masks above.
security/landlock/ruleset.c get_access_mask_t(const struct landlock_ruleset *const ruleset, · line 666
Quoted 11 lines
security/landlock/ruleset.c +2
-2-* %LANDLOCK_NUM_ACCESS_NET elements according to @key_type. +* @masks: Layer access masks to populate. * @key_type: The key type to switch between access masks of different types. * * Returns: An access mask where each access right bit is set which is handled @@ -689,23 +663,20 @@ get_access_mask_t(const struct landlock_ruleset *const ruleset, access_mask_t landlock_init_layer_masks(const struct landlock_ruleset *const domain, const access_mask_t access_request, - layer_mask_t (*const layer_masks)[], + struct layer_access_masks *masks,
*const masks
Quoted 27 lines
security/landlock/ruleset.c +0
-6const enum landlock_key_type key_type) {access_mask_t handled_accesses =0 ; -size_t layer_level, num_access;get_access_mask_t *get_access_mask;switch (key_type) {case LANDLOCK_KEY_INODE : get_access_mask = landlock_get_fs_access_mask; - num_access =LANDLOCK_NUM_ACCESS_FS ;break ;#if IS_ENABLED (CONFIG_INET )case LANDLOCK_KEY_NET_PORT : get_access_mask = landlock_get_net_access_mask; - num_access =LANDLOCK_NUM_ACCESS_NET ;break ;#endif /* IS_ENABLED(CONFIG_INET) */ @@ -714,27 +685,18 @@ landlock_init_layer_masks(const struct landlock_ruleset *const domain,return 0 ; } -memset (layer_masks,0 , -array_size (sizeof ((*layer_masks)[0 ]), num_access)); -/* An empty access request can happen because of O_WRONLY | O_RDWR. */
security/landlock/ruleset.c landlock_init_layer_masks(const struct landlock_ruleset *const domain, · line 692
Quoted 10 lines
security/landlock/ruleset.c +1
-6if (!access_request)return 0 ; -/* Saves all handled accesses per layer. */ -for (layer_level =0 ; layer_level < domain->num_layers; layer_level++) { -const unsigned long access_req = access_request; -const access_mask_t access_mask = -get_access_mask (domain, layer_level); -unsigned long access_bit; +for (int i =0 ; i < domain->num_layers; i++) {
size_t i
security/landlock/ruleset.c landlock_init_layer_masks(const struct landlock_ruleset *const domain, · line 698
Quoted 13 lines
security/landlock/ruleset.c +4
-7+const access_mask_t handled =get_access_mask (domain, i); -for_each_set_bit (access_bit, &access_req, num_access) { -if (BIT_ULL (access_bit) & access_mask) { - (*layer_masks)[access_bit] |= -BIT_ULL (layer_level); - handled_accesses |=BIT_ULL (access_bit); - } - } + masks->access[i] = access_request & handled; + handled_accesses |= masks->access[i]; } +for (int i = domain->num_layers; i <LANDLOCK_MAX_NUM_LAYERS ; i++)
size_t i
ARRAY_SIZE(masks->access)
Quoted 42 lines
security/landlock/ruleset.c +2
-0+ masks->access[i] =0 ; +return handled_accesses; }security/landlock/ruleset.h +14
-4@@ -301,15 +301,25 @@ landlock_get_scope_mask(const struct landlock_ruleset *const ruleset, return ruleset->access_masks[layer_level].scope; } +/** +* struct layer_accesses - A boolean matrix of layers and access rights +* +* This has a bit for each combination of layer numbers and access rights. +* During access checks, it is used to represent the access rights for each +* layer which still need to be fulfilled. When all bits are 0, the access +* request is considered to be fulfilled. +*/ +struct layer_access_masks { +access_mask_t access[LANDLOCK_MAX_NUM_LAYERS ]; +}; +bool landlock_unmask_layers (const struct landlock_rule *const rule, -const access_mask_t access_request, -layer_mask_t (*const layer_masks)[], -const size_t masks_array_size); +struct layer_access_masks *masks);access_mask_t landlock_init_layer_masks (const struct landlock_ruleset *const domain,const access_mask_t access_request, -layer_mask_t (*const layer_masks)[], +struct layer_access_masks *masks,const enum landlock_key_type key_type);#endif /* _SECURITY_LANDLOCK_RULESET_H */ Signature · 1 line
2.52.0
A message that is not in the archive (sent off-list or not yet received): [email protected]
Mickaël Salaün
maintainerRe: [RFC PATCH 2/2] landlock: transpose the layer masks data structure
Re-CCing everyone since I guess they were accidentally removed.
Quoted 58 lines · Günther Noack
Hello!
Quoted 8 lines · Mickaël Salaün
The goal of the initial design was to minimize the amount of memory wrt the number of different access rights because the maximum number of layers is 16 whereas access rights could grow up to 64.
Transposing the matrix increases the memory footprint in theory but because we still need the struct
layer_access_masksmatrix, it should actually be better. See stack usage delta with audit (generated withcheck-linux.sh):Thanks for the review and for doing these measurements!
Quoted 19 lines
landlock_unmask_layers 208 80 -128 landlock_init_layer_masks 192 96 -96 landlock_log_denial 176 80 -96 current_check_access_path 336 304 -32 current_check_refer_path 592 560 -32 hook_file_open 352 320 -32 hook_file_send_sigiotask 176 160 -16 hook_file_truncate 112 96 -16 hook_move_mount 128 112 -16 hook_ptrace_access_check 192 176 -16 hook_ptrace_traceme 160 144 -16 hook_sb_mount 128 112 -16 hook_sb_pivotroot 128 112 -16 hook_sb_remount 128 112 -16 hook_sb_umount 128 112 -16 hook_task_kill 176 160 -16 current_check_access_socket 336 352 +16 is_access_to_paths_allowed 384 400 +16Quoted 6 lines
...and stack usage delta without audit:
landlock_unmask_layers 208 80 -128 landlock_init_layer_masks 192 96 -96 hook_file_open 208 192 -16 current_check_access_socket 176 208 +32These stack usage measurement look as expected. With the current set of 16 FS access rights, the matrix for the FS case is 16x16 bits (32 bytes), both in the old code and after the refactoring.
The differences we see above are therefore not savings in the data structure itself, but due to the code simplifications in surrounding functions where we now need fewer function parameters and local variables.
Quoted 8 lines
However, when we'll add the next access right,
access_mask_twill be u32 instead of u16, and stack usage delta will increase:current_check_access_socket 352 384 +32 hook_file_open 320 352 +32 current_check_access_path 304 352 +48 current_check_refer_path 560 608 +48 is_access_to_paths_allowed 400 464 +64
Quoted 4 lines
The data structure grows by 32 bytes, and
access_mask_tgrows by 2 bytes (but is as local variable probably aligned to 64 bit boundary). These functions all have the layer masks as local variables, so it is expected that they grow.
Yes, but we should keep in mind that this grows for all type of access
rights: even if there is no new network access right, the cost will now be the same for FS and network access checks, which was not the case before (on purpose).
Quoted 164 lines
Quoted 7 lines
Even if the cumulative stack usage delta seems reasonable, the commit message should talk about these drawbacks.
I think the improved compiled code, and the overall simplification are worth it.
You are right, the 17th access right will change it somewhat and I should add something to the commit message to explain my reasoning why this is OK.
Abstractly speaking, we have a matrix with 1 bit per layer and access right. I illustrate below how this looks, it feels a bit simpler than explaining it in words. (I am marking the access right / layer combinations with an "x" which are both an actual FS access right and which are in the first layer - assuming that this is our most common case.)
In the old way of representing that matrix, we are using a fixed 16 bits to represent the set of layers where an access right is still needed (wasting 15/16 bits in the common case where only one layer is active), but we store the right number of access rights. Because this representation was not using
access_mask_t, the functions using it required more loops and conditionals.layers fedcba98 76543210 access0 ________ _______x access1 ________ _______x access2 ________ _______x access3 ________ _______x access4 ________ _______x access5 ________ _______x access6 ________ _______x access7 ________ _______x access8 ________ _______x access9 ________ _______x accessa ________ _______x accessb ________ _______x accessc ________ _______x accessd ________ _______x accesse ________ _______x accessf ________ _______xIn the new way of representing that matrix, we are using a fixed 16 (soon 32) bits for the access rights (wasting 0 (soon 15) bits for FS access rights). We also use a fixed number of layers (but this now becomes tractable to change). We get simplified code and improved performance.
access rights fedcba98 76543210 layer0 xxxxxxxx xxxxxxxx layer1 ________ ________ layer2 ________ ________ layer3 ________ ________ layer4 ________ ________ layer5 ________ ________ layer6 ________ ________ layer7 ________ ________ layer8 ________ ________ layer9 ________ ________ layera ________ ________ layerb ________ ________ layerc ________ ________ layerd ________ ________ layere ________ ________ layerf ________ ________Once we introduce the 17th FS access right, the matrix will use a total of 64 instead of 32 bytes and look like this:
access rights 11111111 11111111 00000000 00000000 fedcba98 76543210 fedcba98 76543210 layer0 ________ _______x xxxxxxxx xxxxxxxx layer1 ________ ________ ________ ________ layer2 ________ ________ ________ ________ layer3 ________ ________ ________ ________ layer4 ________ ________ ________ ________ layer5 ________ ________ ________ ________ layer6 ________ ________ ________ ________ layer7 ________ ________ ________ ________ layer8 ________ ________ ________ ________ layer9 ________ ________ ________ ________ layera ________ ________ ________ ________ layerb ________ ________ ________ ________ layerc ________ ________ ________ ________ layerd ________ ________ ________ ________ layere ________ ________ ________ ________ layerf ________ ________ ________ ________In my mind, it is a tradeoff where we get slightly better performance and simpler code at the cost of a slightly increased stack space. But in my understanding, as long as we stay below the stack size limit, this should be acceptable. (Also, this is only 64 bytes extra, the risk of exeeding stack space because of it seems low.)
(
P.S. A thing that we can try as a follow-up is:Now that this is an array indexed by layer, it becomes easier to only look at the layers that are actually active. In the common case, that means that we would be able to skip looking at these layers, and we might be able to save a few memory accesses.
Although, I am not sure it will improve performance - we should better measure the performance impact carefully; the matrix still fits in two cache lines. It might not make a big difference how many of the adjacent layers we access, and it could be offset by having to pass the number of layers around and by loop unrolling tricks that the compiler can't use any more.)
Quoted 5 lines · Günther Noack
Quoted 2 lines
The layer masks data structure tracks the requested but unfulfilled access rights during an operations security check. It stores one bit
operation?
I missed the apostrophe: "operation's"
Quoted 34 lines
Quoted 21 lines
for each combination of access right and layer index. If the bit is set, that access right is not granted (yet) in the given layer and we have to traverse the path further upwards to grant it.
static
size_tget_denied_layer(const structlandlock_ruleset*const domain,security/landlock/audit.c +1
-12access_mask_t *const access_request, - const layer_mask_t (*const layer_masks)[], - const size_t layer_masks_size) + const struct layer_access_masks *masks) { - const unsigned long access_req = *access_request; -unsigned long access_bit; -access_mask_t missing =0 ; -long youngest_layer =-1 ; - -for_each_set_bit (access_bit, &access_req, layer_masks_size) { -const access_mask_t mask = (*layer_masks)[access_bit]; -long layer; - -if (!mask)security/landlock/audit.c static void test_get_hierarchy(struct kunit *const test) · line 187
Quoted 10 lines
security/landlock/audit.c +1
-9-continue ; - -/* __fls(1) == 0 */ - layer =__fls (mask); -if (layer > youngest_layer) { - youngest_layer = layer; - missing =BIT (access_bit); - }else if (layer == youngest_layer) { - missing |=BIT (access_bit); +for (int i =LANDLOCK_MAX_NUM_LAYERS -1 ; i >=0 ; i--) {All the loop indexes should be
size_t(same as before).Quoted 2 lines
Instead of
LANDLOCK_MAX_NUM_LAYERS,ARRAY_SIZE(masks->access)would be better.Done, will be fixed in next patch set version.
Quoted 7 lines
(I have also fixed all the other places where I could have used
size_tandARRAY_SIZE(). Omitting them from this email for brevity, but I looked at them all.)(Remark on the side, I like being able to define the loop variable in the for loop, but I notice that we have not done that much so far. I assume this is OK?)
I think it is (was?) not in the guideline, but as long as checkpatch.pl doesn't complain, it looks good to me. I guess it's officially OK since C11.
Quoted 47 lines
Quoted 23 lines
Quoted 8 lines
security/landlock/audit.c +3
-2+if (masks->access[i] & *access_request) { + *access_request &= masks->access[i]; +return i; -static size_t -get_layer_from_deny_masks(access_mask_t *const access_request,security/landlock/audit.c static void test_get_denied_layer(struct kunit *const test) · line 251
Quoted 10 lines
security/landlock/audit.c +8
-2-const access_mask_t all_existing_optional_access, - const deny_masks_t deny_masks) +/* +* get_layer_from_fs_deny_masks - get the layer which denied the access request +* +* As a side effect, stores the denied access rights from that layer(!) in +* *access_request. +*/ +static size_t get_layer_from_fs_deny_masks (access_mask_t *const access_request, +const deny_masks_t deny_masks)I'm not a fan of this change. We come from a generic approach to a specific and hardcoded one. This is simpler *for now*, but could we get a better implementation?
Quoted 2 lines
Anyway, please create at least a dedicated patch for the non-transposition changes.
OK, I'll have a look into decoupling these aspects of the change. (Not coded yet, but it's doable and would be cleaner for the patch organization.)
Quoted 16 lines
Quoted 16 lines
{
security/landlock/audit.c +4
-8-const unsigned long access_opt = all_existing_optional_access; -const unsigned long access_req = *access_request; -access_mask_t missing =0 ; +const access_mask_t access_req = *access_request;size_t youngest_layer =0 ; -size_t access_index =0 ; -unsigned long access_bit; +access_mask_t missing =0 ; -/* This will require change with new object types. */ -WARN_ON_ONCE (access_opt != _LANDLOCK_ACCESS_FS_OPTIONAL); +WARN_ON_ONCE ((access_req | _LANDLOCK_ACCESS_FS_OPTIONAL) != + _LANDLOCK_ACCESS_FS_OPTIONAL); - for_each_set_bit(access_bit, &access_opt,
Re: [RFC PATCH 2/2] landlock: transpose the layer masks data structure
In reply to Günther NoackQuoted 50 lines · Günther Noack
[...]
security/landlock/audit.c +14
-25-static size_t -get_layer_from_deny_masks (access_mask_t *const access_request, -const access_mask_t all_existing_optional_access, -const deny_masks_t deny_masks) +/* +* get_layer_from_fs_deny_masks - get the layer which denied the access request +* +* As a side effect, stores the denied access rights from that layer(!) in +* *access_request. +*/ +static size_t get_layer_from_fs_deny_masks (access_mask_t *const access_request, +const deny_masks_t deny_masks) { -const unsigned long access_opt = all_existing_optional_access; -const unsigned long access_req = *access_request; -access_mask_t missing =0 ; +const access_mask_t access_req = *access_request;size_t youngest_layer =0 ; -size_t access_index =0 ; -unsigned long access_bit; +access_mask_t missing =0 ; -/* This will require change with new object types. */ -WARN_ON_ONCE (access_opt != _LANDLOCK_ACCESS_FS_OPTIONAL); +WARN_ON_ONCE ((access_req | _LANDLOCK_ACCESS_FS_OPTIONAL) != + _LANDLOCK_ACCESS_FS_OPTIONAL); -for_each_set_bit (access_bit, &access_opt, -BITS_PER_TYPE (access_mask_t)) { -if (access_req &BIT (access_bit)) { -const size_t layer = - (deny_masks >> (access_index *4 )) & - (LANDLOCK_MAX_NUM_LAYERS -1 ); +if (access_req &LANDLOCK_ACCESS_FS_TRUNCATE ) { +size_t layer = deny_masks &0x0f ; -if (layer > youngest_layer) { - youngest_layer = layer; - missing =BIT (access_bit); - }else if (layer == youngest_layer) { - missing |=BIT (access_bit); - } - } - access_index++;
security/landlock/audit.c static void test_get_denied_layer(struct kunit *const test) · line 272
Quoted 10 lines
security/landlock/audit.c +8
-0+ missing |=LANDLOCK_ACCESS_FS_TRUNCATE ; + youngest_layer =max (youngest_layer, layer); } +if (access_req &LANDLOCK_ACCESS_FS_IOCTL_DEV ) { +size_t layer = (deny_masks &0xf0 ) >>4 ; +if (layer > youngest_layer) + missing =0 ; + + missing |=LANDLOCK_ACCESS_FS_IOCTL_DEV ;
I think this should be under a if (layer == youngest_layer). If layer < youngest_layer, because *access_request is only supposed to contain the missing access from the youngest denying layer, this would be incorrect. Although I think it probably won't make a difference in practice right now since we don't currently have any access request that has truncate and ioctl at the same time.
Quoted 14 lines
security/landlock/audit.c +3
-1+ youngest_layer =max (youngest_layer, layer); + } *access_request = missing;return youngest_layer; }#ifdef CONFIG_SECURITY_LANDLOCK_KUNIT_TEST -static void test_get_layer_from_deny_masks (struct kunit *const test) +static void test_get_layer_from_fs_deny_masks (struct kunit *const test) {deny_masks_t deny_mask;access_mask_t access;
Quoted 9 lines · Günther Noack
Tentative results with and without this patch set show that the hypothesis likely holds true. The benchmark I used exercises a "worst case" scenario that attempts to be bottlenecked on the affected code:
constructs a large number of nested directories, with one "path beneath" rule each and then tries to open the innermost directory many times. The benchmark is intentionally unrealistic to amplify the amount of time used for the path walk logic and forces Landlock to walk the full path (eventually failing the open syscall). (I'll send the benchmark program in a reply to this mail for full transparency.)
Please see the benchmark program below.
To compile it, use:
cc -o benchmark_worsecase benchmark_worsecase.c
Source code:
```
#define _GNU_SOURCE
#include <err.h>
#include <fcntl.h>
#include <linux/landlock.h>
#include <stdbool.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/prctl.h>
#include <sys/stat.h>
#include <sys/syscall.h>
#include <sys/times.h>
#include <time.h>
#include <unistd.h>
/* Flags */
bool use_landlock = true;
size_t num_iterations = 100000;
size_t num_subdirs = 10000;
void usage() { puts("Usage: benchmark_worstcase [-no-landlock]"); }
/*
* Build a deep directory, enforce Landlock and return the FD to the
* deepest dir. On any failure, exit the process with an error.
*/
int build_directory(size_t depth) {
const char *path = "d"; /* directory name */
if (use_landlock) {
int abi = syscall(SYS_landlock_create_ruleset, NULL, 0,
LANDLOCK_CREATE_RULESET_VERSION);
if (abi < 7)
err(1, "Landlock ABI too low: got %d, wanted 7+", abi);
}
int ruleset_fd = -1;
if (use_landlock) {
if (prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0) < 0)
err(1, "prctl");
struct landlock_ruleset_attr attr = {
.handled_access_fs = 0xffff, /* All FS access rights as of 2025-12 */
};
ruleset_fd = syscall(SYS_landlock_create_ruleset, &attr, sizeof(attr), 0U);
if (ruleset_fd < 0)
err(1, "landlock_create_ruleset");
}
int current = open(".", O_PATH);
if (current < 0)
err(1, "open(.)");
while (depth--) {
if (use_landlock) {
struct landlock_path_beneath_attr attr = {
.allowed_access = LANDLOCK_ACCESS_FS_IOCTL_DEV,
.parent_fd = current,
};
if (syscall(SYS_landlock_add_rule, ruleset_fd, LANDLOCK_RULE_PATH_BENEATH,
&attr, 0) < 0)
err(1, "landlock_add_rule");
}
if (mkdirat(current, path, 0700) < 0)
err(1, "mkdirat(%s)", path);
int previous = current;
current = openat(current, path, O_PATH);
if (current < 0)
err(1, "open(%s)", path);
close(previous);
}
if (use_landlock) {
if (syscall(SYS_landlock_restrict_self, ruleset_fd, 0) < 0)
err(1, "landlock_restrict_self");
}
close(ruleset_fd);
return current;
}
int main(int argc, char *argv[]) {
for (int i = 1; i < argc; i++) {
if (!strcmp(argv[i], "-no-landlock")) {
use_landlock = false;
} else if (!strcmp(argv[i], "-d")) {
i++;
if (i < argc)
err(1, "expected number of subdirs after -d");
num_subdirs = atoi(argv[i]);
} else if (!strcmp(argv[i], "-n")) {
i++;
if (i < argc)
err(1, "expected number of iterations after -n");
num_iterations = atoi(argv[i]);
} else {
usage();
errx(1, "unknown argument: %s", argv[i]);
}
}
printf("*** Benchmark ***\n");
printf("%zu dirs, %zu iterations, %s landlock\n", num_subdirs,
num_iterations, use_landlock ? "with" : "without");
struct tms start_time;
if (times(&start_time) == -1)
err(1, "times");
int current = build_directory(num_subdirs);
for (int i = 0; i < num_iterations; i++) {
int fd = openat(current, ".", O_DIRECTORY);
if (fd != -1)
errx(1, "openat succeeded, expected error");
}
struct tms end_time;
if (times(&end_time) == -1)
err(1, "times");
printf("*** Benchmark concluded ***\n");
printf("System: %ld clocks\n", end_time.tms_stime - start_time.tms_stime);
printf("User : %ld clocks\n", end_time.tms_utime - start_time.tms_utime);
printf("Clocks per second: %d\n", CLOCKS_PER_SEC);
close(current);
}
```Mickaël Salaün
maintainerQuoted 11 lines · Günther Noack
Quoted 9 lines · Günther Noack
Tentative results with and without this patch set show that the hypothesis likely holds true. The benchmark I used exercises a "worst case" scenario that attempts to be bottlenecked on the affected code:
constructs a large number of nested directories, with one "path beneath" rule each and then tries to open the innermost directory many times. The benchmark is intentionally unrealistic to amplify the amount of time used for the path walk logic and forces Landlock to walk the full path (eventually failing the open syscall). (I'll send the benchmark program in a reply to this mail for full transparency.)
Quoted 1 line
Please see the benchmark program below.
Thanks for the investigation!
Quoted 4 lines
To compile it, use:
cc -o
benchmark_worsecasebenchmark_worsecase.c
It would be useful to clean up a bit this benchmark and add it to the selftests' Landlock directory (see seccomp_benchmark.c).
Quoted 137 lines
Source code:
``` #define _GNU_SOURCE #include <err.h> #include <fcntl.h> #include <linux/landlock.h> #include <stdbool.h> #include <stdio.h> #include <stdlib.h> #include <string.h> #include <sys/prctl.h> #include <sys/stat.h> #include <sys/syscall.h> #include <sys/times.h> #include <time.h> #include <unistd.h> /* Flags */ bool use_landlock = true; size_t num_iterations = 100000; size_t num_subdirs = 10000; void usage() { puts("Usage: benchmark_worstcase [-no-landlock]"); } /* * Build a deep directory, enforce Landlock and return the FD to the * deepest dir. On any failure, exit the process with an error. */ int build_directory(size_t depth) { const char *path = "d"; /* directory name */ if (use_landlock) { int abi = syscall(SYS_landlock_create_ruleset, NULL, 0, LANDLOCK_CREATE_RULESET_VERSION); if (abi < 7) err(1, "Landlock ABI too low: got %d, wanted 7+", abi); } int ruleset_fd = -1; if (use_landlock) { if (prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0) < 0) err(1, "prctl"); struct landlock_ruleset_attr attr = { .handled_access_fs = 0xffff, /* All FS access rights as of 2025-12 */ }; ruleset_fd = syscall(SYS_landlock_create_ruleset, &attr, sizeof(attr), 0U); if (ruleset_fd < 0) err(1, "landlock_create_ruleset"); } int current = open(".", O_PATH); if (current < 0) err(1, "open(.)"); while (depth--) { if (use_landlock) { struct landlock_path_beneath_attr attr = { .allowed_access = LANDLOCK_ACCESS_FS_IOCTL_DEV, .parent_fd = current, }; if (syscall(SYS_landlock_add_rule, ruleset_fd, LANDLOCK_RULE_PATH_BENEATH, &attr, 0) < 0) err(1, "landlock_add_rule"); } if (mkdirat(current, path, 0700) < 0) err(1, "mkdirat(%s)", path); int previous = current; current = openat(current, path, O_PATH); if (current < 0) err(1, "open(%s)", path); close(previous); } if (use_landlock) { if (syscall(SYS_landlock_restrict_self, ruleset_fd, 0) < 0) err(1, "landlock_restrict_self"); } close(ruleset_fd); return current; } int main(int argc, char *argv[]) { for (int i = 1; i < argc; i++) { if (!strcmp(argv[i], "-no-landlock")) { use_landlock = false; } else if (!strcmp(argv[i], "-d")) { i++; if (i < argc) err(1, "expected number of subdirs after -d"); num_subdirs = atoi(argv[i]); } else if (!strcmp(argv[i], "-n")) { i++; if (i < argc) err(1, "expected number of iterations after -n"); num_iterations = atoi(argv[i]); } else { usage(); errx(1, "unknown argument: %s", argv[i]); } } printf("*** Benchmark ***\n"); printf("%zu dirs, %zu iterations, %s landlock\n", num_subdirs, num_iterations, use_landlock ? "with" : "without"); struct tms start_time; if (times(&start_time) == -1) err(1, "times"); int current = build_directory(num_subdirs); for (int i = 0; i < num_iterations; i++) { int fd = openat(current, ".", O_DIRECTORY); if (fd != -1) errx(1, "openat succeeded, expected error"); } struct tms end_time; if (times(&end_time) == -1) err(1, "times"); printf("*** Benchmark concluded ***\n"); printf("System: %ld clocks\n", end_time.tms_stime - start_time.tms_stime); printf("User : %ld clocks\n", end_time.tms_utime - start_time.tms_utime); printf("Clocks per second: %d\n", CLOCKS_PER_SEC); close(current); } ```
Quoted 7 lines · Mickaël Salaün
Quoted 3 lines · Günther Noack
To compile it, use:
cc -o
benchmark_worsecasebenchmark_worsecase.cIt would be useful to clean up a bit this benchmark and add it to the selftests' Landlock directory (see
seccomp_benchmark.c).
Thanks for the pointer, I did not realize this existed.
I'll have a look for V2.
–Günther