WTF: patch "[PATCH] landlock: Fix same-layer rule unions" was seriously submitted to be applied to the 5.18-stable tree?
Subsystems (4)
[email protected]
maintainerThe patch below was submitted to be applied to the 5.18-stable tree.
I fail to see how this patch meets the stable kernel rules as found at Documentation/process/stable-kernel-rules.rst.
I could be totally wrong, and if so, please respond to <[email protected]> and let me know why this patch should be applied. Otherwise, it is now dropped from my patch queues, never to be seen again.
thanks,
greg k-h
------------------ original commit in Linus's tree ------------------
From 8ba0005ff418ec356e176b26eaa04a6ac755d05b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Micka=C3=ABl=20Sala=C3=BCn?= <[email protected]>
Date: Fri, 6 May 2022 18:10:54 +0200
Subject: [PATCH] landlock: Fix same-layer rule unions
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
The original behavior was to check if the full set of requested accesses was allowed by at least a rule of every relevant layer. This didn't take into account requests for multiple accesses and same-layer rules allowing the union of these accesses in a complementary way. As a result, multiple accesses requested on a file hierarchy matching rules that, together, allowed these accesses, but without a unique rule allowing all of them, was illegitimately denied. This case should be rare in practice and it can only be triggered by the path_rename or file_open hook implementations.
For instance, if, for the same layer, a rule allows execution beneath /a/b and another rule allows read beneath /a, requesting access to read and execute at the same time for /a/b should be allowed for this layer.
This was an inconsistency because the union of same-layer rule accesses was already allowed if requested once at a time anyway.
This fix changes the way allowed accesses are gathered over a path walk. To take into account all these rule accesses, we store in a matrix all layer granting the set of requested accesses, according to the handled accesses. To avoid heap allocation, we use an array on the stack which is 2*13 bytes. A following commit bringing the LANDLOCK_ACCESS_FS_REFER access right will increase this size to reach 112 bytes (2*14*4) in case of link or rename actions.
Add a new layout1.layer_rule_unions test to check that accesses from different rules pertaining to the same layer are ORed in a file hierarchy. Also test that it is not the case for rules from different layers.
security/landlock/fs.c +52 -26
@@ -207,45 +207,67 @@ find_rule(const struct landlock_ruleset *const domain,return rule; } -static inline layer_mask_t unmask_layers (const struct landlock_rule *const rule, -const access_mask_t access_request, -layer_mask_t layer_mask) +/* +* @layer_masks is read and may be updated according to the access request and +* the matching rule. +* +* Returns true if the request is allowed (i.e. relevant layer masks for the +* request are empty). +*/ +static inline bool +unmask_layers (const struct landlock_rule *const rule, +const access_mask_t access_request, +layer_mask_t (*const layer_masks)[LANDLOCK_NUM_ACCESS_FS ]) {size_t layer_level; +if (!access_request || !layer_masks) +return true;if (!rule) -return layer_mask; +return false;/* * An access is granted if, for each policy layer, at least one rule -* encountered on the pathwalk grants the requested accesses, -* regardless of their position in the layer stack. We must then check +* 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. +* 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> */ 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; -/* Checks that the layer grants access to the full request. */ -if ((layer->access & access_request) == access_request) { - layer_mask &= ~layer_bit; - -if (layer_mask ==0 ) -return layer_mask; +/* +* Records in @layer_masks which layer grants access to each +* requested access. +*/ + is_empty = true; +for_each_set_bit (access_bit, &access_req, +ARRAY_SIZE (*layer_masks)) { +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; } -return layer_mask; +return false; }static int check_access_path (const struct landlock_ruleset *const domain,const struct path *const path,const access_mask_t access_request) { -bool allowed = false; +layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_FS ] = {}; +bool allowed = false, has_access = false;struct path walker_path; -layer_mask_t layer_mask;size_t i;if (!access_request)
@@ -265,13 +287,20 @@ static int check_access_path(const struct landlock_ruleset *const domain,return -EACCES ;/* Saves all layers handling a subset of requested accesses. */ - layer_mask =0 ;for (i =0 ; i < domain->num_layers; i++) { -if (domain->fs_access_masks[i] & access_request) - layer_mask |=BIT_ULL (i); +const unsigned long access_req = access_request; +unsigned long access_bit; + +for_each_set_bit (access_bit, &access_req, +ARRAY_SIZE (layer_masks)) { +if (domain->fs_access_masks[i] &BIT_ULL (access_bit)) { + layer_masks[access_bit] |=BIT_ULL (i); + has_access = true; + } + } }/* An access request not handled by the domain is allowed. */ -if (layer_mask ==0 ) +if (!has_access)return 0 ; walker_path = *path;
@@ -283,14 +312,11 @@ static int check_access_path(const struct landlock_ruleset *const domain,while (true) {struct dentry *parent_dentry; - layer_mask = -unmask_layers (find_rule (domain, walker_path.dentry), - access_request, layer_mask); -if (layer_mask ==0 ) { + allowed =unmask_layers (find_rule (domain, walker_path.dentry), + access_request, &layer_masks); +if (allowed)/* Stops when a rule from each layer grants access. */ - allowed = true;break ; - } jump_up:if (walker_path.dentry == walker_path.mnt->mnt_root) {
security/landlock/ruleset.h +2 -0
@@ -22,6 +22,8 @@typedef u16 access_mask_t ;/* Makes sure all filesystem access rights can be stored. */ static_assert (BITS_PER_TYPE (access_mask_t) >=LANDLOCK_NUM_ACCESS_FS ); +/* Makes sure for_each_set_bit() and for_each_clear_bit() calls are OK. */ +static_assert (sizeof (unsigned long ) >=sizeof (access_mask_t));typedef u16 layer_mask_t ;/* Makes sure all layers can be checked. */
tools/testing/selftests/landlock/fs_test.c +107 -0
113 lines not on this page · raw message