Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <4f5adce6-50a6-ca2e-6146-71626d2af197@digikod.net>
Date:   Tue, 30 Aug 2022 13:22:36 +0200
MIME-Version: 1.0
User-Agent: 
Subject: Re: [PATCH -next v2 2/6] landlock: abstract walk_to_visible_parent()
 helper
Content-Language: en-US
To:     Xiu Jianfeng <xiujianfeng@huawei.com>, paul@paul-moore.com,
        jmorris@namei.org, serge@hallyn.com, shuah@kernel.org,
        corbet@lwn.net
Cc:     linux-security-module@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-kselftest@vger.kernel.org,
        linux-doc@vger.kernel.org
References: <20220827111215.131442-1-xiujianfeng@huawei.com>
 <20220827111215.131442-3-xiujianfeng@huawei.com>
From:   =?UTF-8?Q?Micka=c3=abl_Sala=c3=bcn?= <mic@digikod.net>
In-Reply-To: <20220827111215.131442-3-xiujianfeng@huawei.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 27/08/2022 13:12, Xiu Jianfeng wrote:
> This helper will be used in the next commit which supports chmod and
> chown access rights restriction.
> 
> Signed-off-by: Xiu Jianfeng <xiujianfeng@huawei.com>
> ---
>   security/landlock/fs.c | 67 ++++++++++++++++++++++++++++++------------
>   1 file changed, 49 insertions(+), 18 deletions(-)
> 
> diff --git a/security/landlock/fs.c b/security/landlock/fs.c
> index c57f581a9cd5..4ef614a4ea22 100644
> --- a/security/landlock/fs.c
> +++ b/security/landlock/fs.c
> @@ -38,6 +38,44 @@
>   #include "ruleset.h"
>   #include "setup.h"
>   
> +enum walk_result {
> +	WALK_CONTINUE,
> +	WALK_TO_REAL_ROOT,
> +	WALK_TO_DISCONN_ROOT,

Why did you created these results instead of the ones I proposed?


> +};
> +
> +/*
> + * walk to the visible parent, caller should call path_get()/path_put()
> + * before/after this helpler.
> + *
> + * Returns:
> + * - WALK_TO_REAL_ROOT if walk to the real root;
> + * - WALK_TO_DISCONN_ROOT if walk to disconnected root;
> + * - WALK_CONTINUE otherwise.
> + */
> +static enum walk_result walk_to_visible_parent(struct path *path)
> +{
> +	struct dentry *parent_dentry;
> +jump_up:
> +	if (path->dentry == path->mnt->mnt_root) {
> +		if (follow_up(path)) {
> +			/* Ignores hidden mount points. */
> +			goto jump_up;
> +		} else {
> +			/* Stop at the real root. */
> +			return WALK_TO_REAL_ROOT;
> +		}
> +	}
> +	/* Stops at disconnected root directories. */
> +	if (unlikely(IS_ROOT(path->dentry)))
> +		return WALK_TO_DISCONN_ROOT;
> +	parent_dentry = dget_parent(path->dentry);
> +	dput(path->dentry);
> +	path->dentry = parent_dentry;
> +
> +	return WALK_CONTINUE;
> +}
> +
>   /* Underlying object management */
>   
>   static void release_inode(struct landlock_object *const object)
> @@ -539,8 +577,8 @@ static int check_access_path_dual(
>   	 * restriction.
>   	 */
>   	while (true) {
> -		struct dentry *parent_dentry;
>   		const struct landlock_rule *rule;
> +		enum walk_result wr;

Please make the names understandable. In this case this variable may not 
be needed anyway.


>   
>   		/*
>   		 * If at least all accesses allowed on the destination are
> @@ -588,20 +626,12 @@ static int check_access_path_dual(
>   		if (allowed_parent1 && allowed_parent2)
>   			break;
>   
> -jump_up:
> -		if (walker_path.dentry == walker_path.mnt->mnt_root) {
> -			if (follow_up(&walker_path)) {
> -				/* Ignores hidden mount points. */
> -				goto jump_up;
> -			} else {
> -				/*
> -				 * Stops at the real root.  Denies access
> -				 * because not all layers have granted access.
> -				 */
> -				break;
> -			}
> -		}
> -		if (unlikely(IS_ROOT(walker_path.dentry))) {
> +		wr = walk_to_visible_parent(&walker_path);
> +		switch (wr) {
> +		case WALK_TO_REAL_ROOT:
> +			/* Stop at the real root. */
> +			goto out;
> +		case WALK_TO_DISCONN_ROOT:
>   			/*
>   			 * Stops at disconnected root directories.  Only allows
>   			 * access to internal filesystems (e.g. nsfs, which is
> @@ -609,12 +639,13 @@ static int check_access_path_dual(
>   			 */
>   			allowed_parent1 = allowed_parent2 =
>   				!!(walker_path.mnt->mnt_flags & MNT_INTERNAL);

Why not include this check in the helper? This is then not checked in 
patch 3 with current_check_access_path_context_only(), which is a bug.


> +			goto out;
> +		case WALK_CONTINUE:
> +		default:
>   			break;
>   		}
> -		parent_dentry = dget_parent(walker_path.dentry);
> -		dput(walker_path.dentry);
> -		walker_path.dentry = parent_dentry;
>   	}
> +out:
>   	path_put(&walker_path);
>   
>   	if (allowed_parent1 && allowed_parent2)