Re: [PATCH v4 13/30] landlock: Optimize file path walks and prepare for audit support

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

On Wed, Jan 08, 2025 at 04:43:21PM +0100, Mickaël Salaün wrote:
> Always synchronize access_masked_parent* with access_request_parent*
> according to allowed_parent*.  This is required for audit support to be
> able to get back to the reason of denial.
> 
> In a rename/link action, instead of always checking a rule two times for
> the same parent directory of the source and the destination files, only
> check it when an action on a child was not already allowed.  This also
> enables us to keep consistent allowed_parent* status, which is required
> to get back to the reason of denial.
> 
> For internal mount points, only upgrade allowed_parent* to true but do
> not wrongfully set both of them to false otherwise.  This is also
> required to get back to the reason of denial.
> 
> This does not impact the current behavior but slightly optimize code and
> prepare for audit support that needs to know the exact reason why an
> access was denied.
> 
> Cc: Günther Noack <gnoack@google.com>
> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> Link: https://lore.kernel.org/r/20250108154338.1129069-14-mic@digikod.net

Pushed in my next tree to simplify next patch series.

> ---
> 
> Changes since v2:
> - New patch.
> ---
>  security/landlock/fs.c | 44 ++++++++++++++++++++++++++----------------
>  1 file changed, 27 insertions(+), 17 deletions(-)
> 
> diff --git a/security/landlock/fs.c b/security/landlock/fs.c
> index ddadc465581e..01f9d5e78218 100644
> --- a/security/landlock/fs.c
> +++ b/security/landlock/fs.c
> @@ -854,15 +854,6 @@ static bool is_access_to_paths_allowed(
>  				     child1_is_directory, layer_masks_parent2,
>  				     layer_masks_child2,
>  				     child2_is_directory))) {
> -			allowed_parent1 = scope_to_request(
> -				access_request_parent1, layer_masks_parent1);
> -			allowed_parent2 = scope_to_request(
> -				access_request_parent2, layer_masks_parent2);
> -
> -			/* Stops when all accesses are granted. */
> -			if (allowed_parent1 && allowed_parent2)
> -				break;
> -
>  			/*
>  			 * Now, downgrades the remaining checks from domain
>  			 * handled accesses to requested accesses.
> @@ -870,15 +861,32 @@ static bool is_access_to_paths_allowed(
>  			is_dom_check = false;
>  			access_masked_parent1 = access_request_parent1;
>  			access_masked_parent2 = access_request_parent2;
> +
> +			allowed_parent1 =
> +				allowed_parent1 ||
> +				scope_to_request(access_masked_parent1,
> +						 layer_masks_parent1);
> +			allowed_parent2 =
> +				allowed_parent2 ||
> +				scope_to_request(access_masked_parent2,
> +						 layer_masks_parent2);
> +
> +			/* Stops when all accesses are granted. */
> +			if (allowed_parent1 && allowed_parent2)
> +				break;
>  		}
>  
>  		rule = find_rule(domain, walker_path.dentry);
> -		allowed_parent1 = landlock_unmask_layers(
> -			rule, access_masked_parent1, layer_masks_parent1,
> -			ARRAY_SIZE(*layer_masks_parent1));
> -		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, 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));
>  
>  		/* Stops when a rule from each layer grants access. */
>  		if (allowed_parent1 && allowed_parent2)
> @@ -902,8 +910,10 @@ static bool is_access_to_paths_allowed(
>  			 * access to internal filesystems (e.g. nsfs, which is
>  			 * reachable through /proc/<pid>/ns/<namespace>).
>  			 */
> -			allowed_parent1 = allowed_parent2 =
> -				!!(walker_path.mnt->mnt_flags & MNT_INTERNAL);
> +			if (walker_path.mnt->mnt_flags & MNT_INTERNAL) {
> +				allowed_parent1 = true;
> +				allowed_parent2 = true;
> +			}
>  			break;
>  		}
>  		parent_dentry = dget_parent(walker_path.dentry);
> -- 
> 2.47.1
> 
>