linux-kernel - Re: [PATCH v3 bpf-next 1/5] namei: Introduce new helper function path_walk

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]

Message-ID: <d7d755ea-5942-440b-8154-21198cb6a0f1@maowtm.org>
Date: Tue, 10 Jun 2025 23:26:41 +0100
From: Tingmao Wang <m@...wtm.org>
To: Song Liu <song@...nel.org>, Mickaël Salaün
 <mic@...ikod.net>
Cc: bpf@...r.kernel.org, linux-fsdevel@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-security-module@...r.kernel.org,
 kernel-team@...a.com, andrii@...nel.org, eddyz87@...il.com, ast@...nel.org,
 daniel@...earbox.net, martin.lau@...ux.dev, viro@...iv.linux.org.uk,
 brauner@...nel.org, jack@...e.cz, kpsingh@...nel.org,
 mattbobrowski@...gle.com, amir73il@...il.com, repnop@...gle.com,
 jlayton@...nel.org, josef@...icpanda.com, gnoack@...gle.com
Subject: Re: [PATCH v3 bpf-next 1/5] namei: Introduce new helper function
 path_walk_parent()

On 6/10/25 18:26, Song Liu wrote:
> On Tue, Jun 10, 2025 at 10:19 AM Mickaël Salaün <mic@...ikod.net> wrote:
>>
>> On Fri, Jun 06, 2025 at 02:30:11PM -0700, Song Liu wrote:
>>> [...]
>>> + * Returns:
>>> + *  true  - if @path is updated to its parent.
>>> + *  false - if @path is already the root (real root or @root).
>>> + */
>>> +bool path_walk_parent(struct path *path, const struct path *root)
>>> +{
>>> +     struct dentry *parent;
>>> +
>>> +     if (path_equal(path, root))
>>> +             return false;
>>> +
>>> +     if (unlikely(path->dentry == path->mnt->mnt_root)) {
>>> +             struct path p;
>>> +
>>> +             if (!choose_mountpoint(real_mount(path->mnt), root, &p))
>>> +                     return false;
>>> +             path_put(path);
>>> +             *path = p;
>>> +     }
>>> +
>>> +     if (unlikely(IS_ROOT(path->dentry)))
>>
>> path would be updated while false is returned, which is not correct.
> 
> Good catch.. How about the following:
> 
> bool path_walk_parent(struct path *path, const struct path *root)
> {
>         struct dentry *parent;
>         bool ret = false;
> 
>         if (path_equal(path, root))
>                 return false;
> 
>         if (unlikely(path->dentry == path->mnt->mnt_root)) {
>                 struct path p;
> 
>                 if (!choose_mountpoint(real_mount(path->mnt), root, &p))
>                         return false;
>                 path_put(path);
>                 *path = p;
>                 ret = true;
>         }
> 
>         if (unlikely(IS_ROOT(path->dentry)))
>                 return ret;

Returning true here would be the wrong semantic right?  This whole thing
is only possible when some mount shadows "/".  Say if you have a landlock
rule on the old "/", but then we mount a new "/" and chroot into it (via
"/.."), the landlock rule on the old "/" should not apply, but if we
change *path and return true here then this will "expose" that old "/" to
landlock.

A quick suggestion although I haven't tested anything - maybe we should do
a special case check for IS_ROOT inside the
    if (unlikely(path->dentry == path->mnt->mnt_root))
? Before "path_put(path);", if IS_ROOT(p.dentry) then we just path_get(p)
and return false.

> 
>         parent = dget_parent(path->dentry);
>         dput(path->dentry);
>         path->dentry = parent;
>         return true;
> }
> 
> Thanks,
> Song