Re: [PATCH v4 1/5] landlock: Implement LANDLOCK_ADD_RULE_NO_INHERIT

["Mickaël Salaün" <mic@digikod.net>]

Thanks for this patch series.  This feature would be useful, but the
related patches add a lot of new code.  We should try to minimize that
as much as possible, especially when similar/close logic already exist
(e.g. path walk).

On Sat, Dec 06, 2025 at 08:51:27PM -0500, Justin Suess wrote:
> Implements a flag to prevent access grant inheritance within the filesystem
> hierarchy for landlock rules.
> 
> If a landlock rule on an inode has this flag, any access grants on parent
> inodes will be ignored. Moreover, operations that involve altering the
> ancestors of the subject with LANDLOCK_ADD_RULE_NO_INHERIT will be
> denied up to the VFS root (new in v4).
> 
> For example, if /a/b/c/ = read only + LANDLOCK_ADD_RULE_NO_INHERIT and
> / = read write, writes to files in /a/b/c will be denied. Moreover,
> moving /a to /bad, removing /a/b/c, or creating links to /a will be
> prohibited.
> 
> Parent flag inheritance is automatically suppressed by the permission
> harvesting logic, which will finish processing early if all relevant
> layers are tagged with NO_INHERIT.
> 
> And if / has LANDLOCK_ADD_RULE_QUIET, /a/b/c will still audit (handled)
> accesses. This is because LANDLOCK_ADD_RULE_NO_INHERIT also
> suppresses flag inheritance from parent objects.
> 
> The parent directory restrictions mitigate sandbox-restart attacks. For
> example, if a sandboxed program is able to move a
> LANDLOCK_ADD_RULE_NO_INHERIT restricted directory, upon sandbox restart,
> the policy applied naively on the same filenames would be invalid.
> Preventing these operations mitigates these attacks.
> 

Your Signed-off-by and Cc should be here, followed by "---", followed by
the changelog (to exclude it from the git message e.g., when using
git am).

> v3..v4 changes:
> 
>   * Rebased on v6 of Tingmao Wang's "quiet flag" series.
>   * Removed unnecessary mask_no_inherit_descendant_layers and related
>     code at Tingmao Wang's suggestion, simplifying patch.
>   * Updated to use new disconnected directory handling.
>   * Improved WARN_ON_ONCE usage. (Thanks Tingmao Wang!)
>   * Removed redundant loop for single-layer rulesets (again thanks Tingmao
>     Wang!)
>   * Protections now apply up to the VFS root, not just the mountpoint.
>   * Indentation fixes.
>   * Removed redundant flag marker blocked_flag_masks.
> 
> v2..v3 changes:
> 
>   * Parent directory topology protections now work by lazily
>     inserting blank rules on parent inodes if they do not
>     exist. This replaces the previous xarray implementation
>     with simplified logic.
>   * Added an optimization to skip further processing if all layers
>     collected have no inherit.
>   * Added support to block flag inheritance.
> 
> Cc: Tingmao Wang <m@maowtm.org>
> Signed-off-by: Justin Suess <utilityemal77@gmail.com>
> ---
>  security/landlock/fs.c      | 389 +++++++++++++++++++++++++++++++++++-
>  security/landlock/ruleset.c |  19 +-
>  security/landlock/ruleset.h |  29 ++-
>  3 files changed, 433 insertions(+), 4 deletions(-)
> 
> diff --git a/security/landlock/fs.c b/security/landlock/fs.c
> index 0b589263ea42..7b0b77859778 100644
> --- a/security/landlock/fs.c
> +++ b/security/landlock/fs.c
> @@ -317,6 +317,207 @@ static struct landlock_object *get_inode_object(struct inode *const inode)
>  	LANDLOCK_ACCESS_FS_IOCTL_DEV)
>  /* clang-format on */
>  
> +static const struct landlock_rule *find_rule(const struct landlock_ruleset *const domain,
> +					     const struct dentry *const dentry);
> +
> +/**
> + * landlock_domain_layers_mask - Build a mask covering all layers of a domain
> + * @domain: The ruleset (domain) to inspect.
> + *
> + * Return a layer mask with a 1 bit for each existing layer of @domain.
> + * If @domain has no layers 0 is returned.  If the number of layers is
> + * greater than or equal to the number of bits in layer_mask_t, all bits
> + * are set.
> + */
> +static layer_mask_t landlock_domain_layers_mask(const struct landlock_ruleset
> +						*const domain)
> +{
> +	if (!domain || !domain->num_layers)
> +		return 0;
> +
> +	if (domain->num_layers >= sizeof(layer_mask_t) * BITS_PER_BYTE)
> +		return (layer_mask_t)~0ULL;
> +
> +	return GENMASK_ULL(domain->num_layers - 1, 0);
> +}
> +
> +/**
> + * rule_blocks_all_layers_no_inherit - check whether a rule disables inheritance
> + * @domain_layers_mask: Mask describing the domain's active layers.
> + * @rule: Rule to inspect.
> + *
> + * Return true if every layer present in @rule has its no_inherit flag set
> + * and the set of layers covered by the rule equals @domain_layers_mask.
> + * This indicates that the rule prevents inheritance on all layers of the
> + * domain and thus further walking for inheritance checks can stop.
> + */
> +static bool rule_blocks_all_layers_no_inherit(const layer_mask_t domain_layers_mask,
> +					      const struct landlock_rule *const rule)
> +{
> +	layer_mask_t rule_layers = 0;
> +	u32 layer_index;
> +
> +	if (!domain_layers_mask || !rule)
> +		return false;
> +
> +	for (layer_index = 0; layer_index < rule->num_layers; layer_index++) {
> +		const struct landlock_layer *const layer =
> +			&rule->layers[layer_index];
> +		const layer_mask_t layer_bit = BIT_ULL(layer->level - 1);
> +
> +		if (!layer->flags.no_inherit)
> +			return false;
> +
> +		rule_layers |= layer_bit;
> +	}
> +
> +	return rule_layers && rule_layers == domain_layers_mask;
> +}
> +
> +/**
> + * ensure_rule_for_dentry - ensure a ruleset contains a rule entry for dentry,
> + * inserting a blank rule if needed.
> + * @ruleset: Ruleset to modify/inspect.  Caller must hold @ruleset->lock.
> + * @dentry: Dentry to ensure a rule exists for.
> + *
> + * If no rule is currently associated with @dentry, insert an empty rule
> + * (with zero access) tied to the backing inode.  Returns a pointer to the
> + * rule associated with @dentry on success, NULL when @dentry is negative, or
> + * an ERR_PTR()-encoded error if the rule cannot be created.
> + *
> + * This is useful for LANDLOCK_ADD_RULE_NO_INHERIT processing, where a rule
> + * may need to be created for an ancestor dentry that does not yet have one
> + * to properly track no_inherit flags.
> + *
> + * The flags are set to zero if a rule is newly created, and the caller
> + * is responsible for setting them appropriately.
> + *
> + * The returned rule pointer's lifetime is tied to @ruleset.
> + */
> +static const struct landlock_rule *
> +ensure_rule_for_dentry(struct landlock_ruleset *const ruleset,
> +		       struct dentry *const dentry)
> +{
> +	struct landlock_id id = {
> +		.type = LANDLOCK_KEY_INODE,
> +	};
> +	const struct landlock_rule *rule;
> +	int err;
> +
> +	if (WARN_ON_ONCE(!ruleset || !dentry || d_is_negative(dentry)))
> +		return NULL;
> +
> +	lockdep_assert_held(&ruleset->lock);
> +
> +	rule = find_rule(ruleset, dentry);
> +	if (rule)
> +		return rule;
> +
> +	id.key.object = get_inode_object(d_backing_inode(dentry));
> +	if (IS_ERR(id.key.object))
> +		return ERR_CAST(id.key.object);
> +
> +	err = landlock_insert_rule(ruleset, id, 0, 0);
> +	landlock_put_object(id.key.object);
> +	if (err)
> +		return ERR_PTR(err);
> +
> +	rule = find_rule(ruleset, dentry);
> +	if (WARN_ON_ONCE(!rule))
> +		return ERR_PTR(-ENOENT);
> +	return rule;
> +}
> +
> +/**
> + * mark_no_inherit_ancestors - mark ancestors as having no_inherit descendants
> + * @ruleset: Ruleset to modify.  Caller must hold @ruleset->lock.
> + * @path: Path representing the descendant that carries no_inherit bits.
> + * @descendant_layers: Mask of layers from the descendant that should be
> + *                     advertised to ancestors via has_no_inherit_descendant.
> + *
> + * Walks upward from @dentry and ensures that any ancestor rule contains the
> + * has_no_inherit_descendant marker for the specified @descendant_layers so
> + * parent lookups can quickly detect descendant no_inherit influence.
> + *
> + * Returns 0 on success or a negative errno if ancestor bookkeeping fails.
> + */
> +static int mark_no_inherit_ancestors(struct landlock_ruleset *ruleset,
> +				     const struct path *const path,
> +				     layer_mask_t descendant_layers)
> +{
> +	struct dentry *cursor;
> +	struct path walk_path;
> +	int err = 0;
> +
> +	if (WARN_ON_ONCE(!ruleset || !path || !path->dentry || !path->mnt ||
> +			 !descendant_layers))
> +		return -EINVAL;
> +
> +	lockdep_assert_held(&ruleset->lock);
> +
> +	walk_path.mnt = path->mnt;
> +	walk_path.dentry = path->dentry;
> +	path_get(&walk_path);
> +
> +	cursor = dget(walk_path.dentry);
> +	while (cursor) {
> +		struct dentry *parent;
> +		const struct landlock_rule *rule;
> +
> +		/* Follow mounts all the way up to the root. */
> +		if (IS_ROOT(cursor)) {
> +			dput(cursor);
> +			if (!follow_up(&walk_path)) {

This path walk is inconsistent and a duplicate of the (correct)
is_access_to_paths_allowed() one.  Please add a patch to factor out the
common parts as much as possible.  Ditto for the
collect_domain_accesses() and collect_topology_sealed_layers().

> +				cursor = NULL;
> +				continue;
> +			}
> +			cursor = dget(walk_path.dentry);
> +		}
> +
> +		parent = dget_parent(cursor);
> +		dput(cursor);
> +		if (!parent)
> +			break;
> +
> +		if (WARN_ON_ONCE(d_is_negative(parent))) {
> +			dput(parent);
> +			break;
> +		}
> +		/*
> +		 * Ensures a rule exists for the parent dentry,
> +		 * inserting a blank one if needed.
> +		 */
> +		rule = ensure_rule_for_dentry(ruleset, parent);
> +		if (IS_ERR(rule)) {
> +			err = PTR_ERR(rule);
> +			dput(parent);
> +			cursor = NULL;
> +			break;
> +		}
> +		if (rule) {
> +			struct landlock_rule *mutable_rule =
> +				(struct landlock_rule *)rule;
> +			/*
> +			 * Unmerged rulesets should only have one layer.
> +			 */
> +			if (WARN_ON_ONCE(mutable_rule->num_layers != 1)) {
> +				dput(parent);
> +				err = -EINVAL;
> +				cursor = NULL;
> +				break;
> +			}
> +
> +			if (descendant_layers & BIT_ULL(0))
> +				mutable_rule->layers[0]
> +					.flags.has_no_inherit_descendant = true;
> +		}
> +
> +		cursor = parent;
> +	}
> +	path_put(&walk_path);
> +	return err;
> +}

> +/**
> + * collect_topology_sealed_layers - collect layers sealed against topology changes
> + * @domain: Ruleset to consult.
> + * @dentry: Starting dentry for the upward walk.
> + * @override_layers: Optional out parameter filled with layers that are
> + *                   present on ancestors but considered overrides (not
> + *                   sealing the topology for descendants).
> + *
> + * Walk upwards from @dentry and return a mask of layers where either the
> + * visited dentry contains a no_inherit rule or ancestors were previously
> + * marked as having a descendant with no_inherit.  @override_layers, if not
> + * NULL, is filled with layers that would normally be overridden by more
> + * specific descendant rules.
> + *
> + * Returns a layer mask where set bits indicate layers that are "sealed"
> + * (topology changes like rename/rmdir are denied) for the subtree rooted at
> + * @dentry.
> + *
> + * Useful for LANDLOCK_ADD_RULE_NO_INHERIT parent directory enforcement to ensure
> + * that topology changes do not violate the no_inherit constraints.
> + */
> +static layer_mask_t
> +collect_topology_sealed_layers(const struct landlock_ruleset *const domain,
> +			       struct dentry *dentry,
> +			       layer_mask_t *const override_layers)
> +{
> +	struct dentry *cursor, *parent;
> +	bool include_descendants = true;
> +	layer_mask_t sealed_layers = 0;
> +
> +	if (override_layers)
> +		*override_layers = 0;
> +
> +	if (WARN_ON_ONCE(!domain || !dentry || d_is_negative(dentry)))
> +		return 0;
> +
> +	cursor = dget(dentry);
> +	while (cursor) {
> +		const struct landlock_rule *rule;
> +		u32 layer_index;
> +
> +		rule = find_rule(domain, cursor);
> +		if (rule) {
> +			for (layer_index = 0; layer_index < rule->num_layers;
> +			     layer_index++) {
> +				const struct landlock_layer *layer =
> +					&rule->layers[layer_index];
> +				layer_mask_t layer_bit =
> +					BIT_ULL(layer->level - 1);
> +
> +				if (include_descendants &&
> +				    (layer->flags.no_inherit ||
> +				     layer->flags.has_no_inherit_descendant)) {
> +					sealed_layers |= layer_bit;
> +				} else if (override_layers) {
> +					*override_layers |= layer_bit;
> +				}
> +			}
> +		}
> +
> +		if (sealed_layers || IS_ROOT(cursor))
> +			break;
> +
> +		parent = dget_parent(cursor);
> +		dput(cursor);
> +		if (!parent)
> +			return sealed_layers;
> +
> +		cursor = parent;
> +		include_descendants = false;
> +	}
> +	dput(cursor);
> +	return sealed_layers;
> +}