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: <20250531.NaQu4eic9ieN@digikod.net>
Date: Sat, 31 May 2025 10:40:37 +0200
From: Mickaël Salaün <mic@...ikod.net>
To: Song Liu <song@...nel.org>
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 11:55:22AM -0700, 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:

I was not suggesting to expose nameidata (only struct path and int), but
yes, a standalone helper is OK and it will not tie it to the current
follow_dotdot() internals.

> 
> /**
>  * path_parent - Find the parent of path

Because we update @path, I'd suggest a name containing "walk", something
like path_walk_parent().

>  * @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.

We should explain that the semantic is the same as follow_dotdot(), but
not follow_dots().

>  *
>  * 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?

Yes, thanks.

Al, Christian, would that be OK to backport this helper to fix the
Landlock issue?  If yes, Song could you please put it in in a place that
could be easily backported down to 5.15 e.g., just after handle_dots()?

> 
> Thanks,
> Song
>