linux-kernel - Re: [PATCH -next v2 2/6] landlock: abstract walk_to_visible

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Wed, 31 Aug 2022 19:56:41 +0800
From:   xiujianfeng <xiujianfeng@...wei.com>
To:     Mickaël Salaün <mic@...ikod.net>,
        <paul@...l-moore.com>, <jmorris@...ei.org>, <serge@...lyn.com>,
        <shuah@...nel.org>, <corbet@....net>
CC:     <linux-security-module@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <linux-kselftest@...r.kernel.org>,
        <linux-doc@...r.kernel.org>
Subject: Re: [PATCH -next v2 2/6] landlock: abstract walk_to_visible_parent()
 helper

Hi,

在 2022/8/30 19:22, Mickaël Salaün 写道:
> 
> 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@...wei.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.

I get your point, after moving it to the helper, here should be:

               while (true) {
                    ...
                    switch(walk_to_visible_parent(&walker_path)) {
                    case WALK_CONTINUE:
                       break;
                    case WALK_ALLOWED:
                       allowed_parent1 = allowed_parent2 = true;
                       goto out;
                    case WR_DENIED:
                    default:
                       allowed_parent1 = allowed_parent2 = false;
                       goto out;
                     }
             }
> 
> 
>> +            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)
> .