linux-kernel - Re: [PATCH bpf-next 3/4] bpf: Introduce path iterator

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: <c2d0bae8-691f-4bb6-9c0e-64ab7cdaebd6@maowtm.org>
Date: Sat, 31 May 2025 15:05:46 +0100
From: Tingmao Wang <m@...wtm.org>
To: Song Liu <song@...nel.org>, Mickaël Salaün
 <mic@...ikod.net>
Cc: Al Viro <viro@...iv.linux.org.uk>, Jan Kara <jack@...e.cz>,
 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, brauner@...nel.org,
 kpsingh@...nel.org, mattbobrowski@...gle.com, amir73il@...il.com,
 repnop@...gle.com, jlayton@...nel.org, josef@...icpanda.com,
 gnoack@...gle.com
Subject: Re: [PATCH bpf-next 3/4] bpf: Introduce path iterator

On 5/30/25 19:55, Song Liu wrote:
> On Fri, May 30, 2025 at 5:20 AM Mickaël Salaün <mic@...ikod.net> wrote:
> [...]
>>>
>>> If we update path_parent in this patchset with choose_mountpoint(),
>>> and use it in Landlock, we will close this race condition, right?
>>
>> choose_mountpoint() is currently private, but if we add a new filesystem
>> helper, I think the right approach would be to expose follow_dotdot(),
>> updating its arguments with public types.  This way the intermediates
>> mount points will not be exposed, RCU optimization will be leveraged,
>> and usage of this new helper will be simplified.
> 
> I think it is easier to add a helper similar to follow_dotdot(), but not with
> nameidata. follow_dotdot() touches so many things in nameidata, so it
> is better to keep it as-is. I am having the following:
> 
> /**
>  * path_parent - Find the parent of path
>  * @path: input and output path.
>  * @root: root of the path walk, do not go beyond this root. If @root is
>  *        zero'ed, walk all the way to real root.
>  *
>  * Given a path, find the parent path. Replace @path with the parent path.
>  * If we were already at the real root or a disconnected root, @path is
>  * not changed.
>  *
>  * Returns:
>  *  true  - if @path is updated to its parent.
>  *  false - if @path is already the root (real root or @root).
>  */
> bool path_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;
>                 return true;
>         }
> 
>         if (unlikely(IS_ROOT(path->dentry)))
>                 return false;
> 
>         parent = dget_parent(path->dentry);
>         if (unlikely(!path_connected(path->mnt, parent))) {
>                 dput(parent);
>                 return false;
>         }
>         dput(path->dentry);
>         path->dentry = parent;
>         return true;
> }
> EXPORT_SYMBOL_GPL(path_parent);
> 
> And for Landlock, it is simply:
> 
>                 if (path_parent(&walker_path, &root))
>                         continue;
> 
>                 if (unlikely(IS_ROOT(walker_path.dentry))) {
>                         /*
>                          * Stops at disconnected or real root directories.
>                          * Only allows access to internal filesystems
>                          * (e.g. nsfs, which is reachable through
>                          * /proc/<pid>/ns/<namespace>).
>                          */
>                         if (walker_path.mnt->mnt_flags & MNT_INTERNAL) {
>                                 allowed_parent1 = true;
>                                 allowed_parent2 = true;
>                         }
>                         break;


Hi, maybe I'm missing the complete picture of this code, but since
path_parent doesn't change walker_path if it returns false (e.g. if it's
disconnected, or choose_mountpoint fails), I think this `break;` should be
outside the

    if (unlikely(IS_ROOT(walker_path.dentry)))

right? (Assuming this whole thing is under a `while (true)`) Otherwise we
might get stuck at the current path and get infinite loop?

>                 }
> 
> Does this look right?
> 
> Thanks,
> Song