This fixes the issue mentioned in the previous patch, by essentially
having two "modes" for the pathwalk code - in the pathwalk_ref == false
case we don't take references and just inspect `d_parent` (unless we have
to `follow_up`). In the pathwalk_ref == true case, this is the same as
before.
When we detect any renames during a pathwalk_ref == false walk, we restart
with pathwalk_ref == true, re-initializing the layer masks. I'm not sure
if this is completely correct in regards to is_dom_check - but seems to
work for now. I can revisit this later.
Signed-off-by: Tingmao Wang <m@xxxxxxxxxx>
---
security/landlock/fs.c | 109 ++++++++++++++++++++++++++++++++++++-----
1 file changed, 98 insertions(+), 11 deletions(-)
diff --git a/security/landlock/fs.c b/security/landlock/fs.c
index 923737412cfa..6dff5fb6b181 100644
--- a/security/landlock/fs.c
+++ b/security/landlock/fs.c
@@ -771,6 +771,9 @@ static bool is_access_to_paths_allowed(
_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;
+ unsigned int rename_seqcount;
+ bool pathwalk_ref = false;
+ const struct landlock_rule *rule;
if (!access_request_parent1 && !access_request_parent2)
return true;
@@ -811,6 +814,7 @@ static bool is_access_to_paths_allowed(
rcu_read_lock();
+restart_pathwalk:
if (unlikely(dentry_child1)) {
landlock_unmask_layers(
find_rule_rcu(domain, dentry_child1),
@@ -833,13 +837,32 @@ static bool is_access_to_paths_allowed(
}
walker_path = *path;
+
+ /*
+ * Attempt to do a pathwalk without taking dentry references first,
+ * but if any rename happens while we are doing this, give up and do a
+ * walk with dget_parent instead. See comments in
+ * collect_domain_accesses().
+ */
+
+ if (!pathwalk_ref) {
+ rename_seqcount = read_seqbegin(&rename_lock);
+ if (rename_seqcount % 2 == 1) {
+ pathwalk_ref = true;
+ path_get(&walker_path);
+ }
+ } else {
+ path_get(&walker_path);
+ }
+
+ rule = find_rule_rcu(domain, walker_path.dentry);
+
/*
* We need to walk through all the hierarchy to not miss any relevant
* restriction.
*/
while (true) {
struct dentry *parent_dentry;
- const struct landlock_rule *rule;
/*
* If at least all accesses allowed on the destination are
@@ -881,7 +904,6 @@ static bool is_access_to_paths_allowed(
break;
}
- rule = find_rule_rcu(domain, walker_path.dentry);
allowed_parent1 = allowed_parent1 ||
landlock_unmask_layers(
rule, access_masked_parent1,
@@ -899,13 +921,16 @@ static bool is_access_to_paths_allowed(
jump_up:
if (walker_path.dentry == walker_path.mnt->mnt_root) {
/* follow_up gets the parent and puts the passed in path */
- path_get(&walker_path);
+ if (!pathwalk_ref)
+ path_get(&walker_path);
if (follow_up(&walker_path)) {
- path_put(&walker_path);
+ if (!pathwalk_ref)
+ path_put(&walker_path);
/* Ignores hidden mount points. */
goto jump_up;
} else {
- path_put(&walker_path);
+ if (!pathwalk_ref)
+ path_put(&walker_path);
/*
* Stops at the real root. Denies access
* because not all layers have granted access.
@@ -925,10 +950,27 @@ static bool is_access_to_paths_allowed(
}
break;
}
- parent_dentry = walker_path.dentry->d_parent;
- walker_path.dentry = parent_dentry;
+ if (!pathwalk_ref) {
+ parent_dentry = walker_path.dentry->d_parent;
+
+ rule = find_rule_rcu(domain, parent_dentry);
+ if (read_seqretry(&rename_lock, rename_seqcount)) {
+ pathwalk_ref = true;
+ goto restart_pathwalk;
+ } else {
+ walker_path.dentry = parent_dentry;
+ }
+ } else {
+ parent_dentry = dget_parent(walker_path.dentry);
+ dput(walker_path.dentry);
+ walker_path.dentry = parent_dentry;
+ rule = find_rule_rcu(domain, walker_path.dentry);
+ }
}
+ if (pathwalk_ref)
+ path_put(&walker_path);
+
rcu_read_unlock();
if (!allowed_parent1) {
@@ -1040,22 +1082,55 @@ static bool collect_domain_accesses(
{
unsigned long access_dom;
bool ret = false;
+ bool pathwalk_ref = false;
+ unsigned int rename_seqcount;
+ const struct landlock_rule *rule;
+ struct dentry *parent_dentry;
if (WARN_ON_ONCE(!domain || !mnt_root || !dir || !layer_masks_dom))
return true;
if (is_nouser_or_private(dir))
return true;
+ rcu_read_lock();
+
+restart_pathwalk:
access_dom = landlock_init_layer_masks(domain, LANDLOCK_MASK_ACCESS_FS,
layer_masks_dom,
LANDLOCK_KEY_INODE);
- rcu_read_lock();
+ /*
+ * Attempt to do a pathwalk without taking dentry references first, but
+ * if any rename happens while we are doing this, give up and do a walk
+ * with dget_parent instead. This prevents wrong denials in the
+ * presence of a move followed by an immediate rmdir of the old parent,
+ * where even when both the original and the new parent has allow
+ * rules, we might still hit a negative dentry (the deleted old parent)
+ * and being unable to find either rules.
+ */
+
+ if (!pathwalk_ref) {
+ rename_seqcount = read_seqbegin(&rename_lock);
+ if (rename_seqcount % 2 == 1) {
+ pathwalk_ref = true;
+ dget(dir);
+ }
+ } else {
+ dget(dir);
+ }
+ rule = find_rule_rcu(domain, dir);
+ /*
+ * We don't need to check rename_seqcount here because we haven't
+ * followed any d_parent yet, and the d_inode of the path being
+ * accessed can't change under us as we have ref on path.dentry. But
+ * once we start walking up the path, we need to check the seqcount to
+ * make sure the rule we got isn't based on a wrong/changing/negative
+ * dentry.
+ */
while (true) {
/* Gets all layers allowing all domain accesses. */
- if (landlock_unmask_layers(find_rule_rcu(domain, dir), access_dom,
- layer_masks_dom,
+ if (landlock_unmask_layers(rule, access_dom, layer_masks_dom,
ARRAY_SIZE(*layer_masks_dom))) {
/*
* Stops when all handled accesses are allowed by at
@@ -1069,9 +1144,21 @@ static bool collect_domain_accesses(
if (dir == mnt_root || WARN_ON_ONCE(IS_ROOT(dir)))
break;
- dir = dir->d_parent;
+ if (!pathwalk_ref) {
+ parent_dentry = dir->d_parent;
+ rule = find_rule_rcu(domain, dir);
+ if (read_seqretry(&rename_lock, rename_seqcount)) {
+ pathwalk_ref = true;
+ goto restart_pathwalk;
+ } else {
+ dir = parent_dentry;
+ }
+ }
}
+ if (pathwalk_ref)
+ dput(dir);
+
rcu_read_unlock();
return ret;
--
2.49.0
