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: <CAPhsuW5U-nPk4MFdZSeBNds0qEHjQZrC=c5q+AGNpsKiveC2wA@mail.gmail.com>
Date: Fri, 30 May 2025 11:55:22 -0700
From: Song Liu <song@...nel.org>
To: 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, 
	Tingmao Wang <m@...wtm.org>
Subject: Re: [PATCH bpf-next 3/4] bpf: Introduce path iterator

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;
                }

Does this look right?

Thanks,
Song