| 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
| ||
|
Message-ID: <20250602.Oqu6piethung@digikod.net>
Date: Mon, 2 Jun 2025 19:35:47 +0200
From: Mickaël Salaün <mic@...ikod.net>
To: Tingmao Wang <m@...wtm.org>
Cc: Song Liu <song@...nel.org>, linux-fsdevel@...r.kernel.org,
linux-security-module@...r.kernel.org, bpf@...r.kernel.org, linux-kernel@...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 bpf-next 2/4] landlock: Use path_parent()
On Sat, May 31, 2025 at 02:51:22PM +0100, Tingmao Wang wrote:
> On 5/28/25 23:26, Song Liu wrote:
> > Use path_parent() to walk a path up to its parent.
> >
> > While path_parent() has an extra check with path_connected() than existing
> > code, there is no functional changes intended for landlock.
> >
> > Signed-off-by: Song Liu <song@...nel.org>
> > ---
> > security/landlock/fs.c | 34 +++++++++++++++++-----------------
> > 1 file changed, 17 insertions(+), 17 deletions(-)
> >
> > diff --git a/security/landlock/fs.c b/security/landlock/fs.c
> > index 6fee7c20f64d..32a24758ad6e 100644
> > --- a/security/landlock/fs.c
> > +++ b/security/landlock/fs.c
> > @@ -837,7 +837,6 @@ static bool is_access_to_paths_allowed(
> > * restriction.
> > */
> > while (true) {
> > - struct dentry *parent_dentry;
> > const struct landlock_rule *rule;
> >
> > /*
> > @@ -896,19 +895,17 @@ static bool is_access_to_paths_allowed(
> > 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))) {
> > + switch (path_parent(&walker_path)) {
> > + case PATH_PARENT_CHANGED_MOUNT:
> > + /* Ignores hidden mount points. */
> > + goto jump_up;
> > + case PATH_PARENT_REAL_ROOT:
> > + /*
> > + * Stops at the real root. Denies access
> > + * because not all layers have granted access.
> > + */
> > + goto walk_done;
> > + case PATH_PARENT_DISCONNECTED_ROOT:
> > /*
> > * Stops at disconnected root directories. Only allows
> > * access to internal filesystems (e.g. nsfs, which is
>
> I was looking at the existing handling of disconnected root in Landlock
> and I realized that the comment here confused me a bit:
>
> /*
> * Stops at disconnected root directories. Only allows
> * access to internal filesystems (e.g. nsfs, which is
> * reachable through /proc/<pid>/ns/<namespace>).
> */
>
> In the original code, this was under a
>
> if (unlikely(IS_ROOT(walker_path.dentry)))
>
> which means that it only stops walking if we found out we're disconnected
> after reaching a filesystem boundary. However if before we got to this
> point, we have already collected enough rules to allow access, access
> would be allowed, even if we're currently disconnected. Demo:
>
> / # cd /
> / # cp /linux/samples/landlock/sandboxer .
> / # mkdir a b
> / # mkdir a/foo
> / # echo baz > a/foo/bar
> / # mount --bind a b
> / # LL_FS_RO=/ LL_FS_RW=/ ./sandboxer bash
> Executing the sandboxed command...
> / # cd /b/foo
> /b/foo # cat bar
> baz
> /b/foo # mv /a/foo /foo
> /b/foo # cd .. # <- We're now disconnected
> bash: cd: ..: No such file or directory
> /b/foo # cat bar
> baz # <- but landlock still lets us read the file
>
> However, I think this patch will change this behavior due to the use of
> path_connected
>
> root@...8fff999ce:/# mkdir a b
> root@...8fff999ce:/# mkdir a/foo
> root@...8fff999ce:/# echo baz > a/foo/bar
> root@...8fff999ce:/# mount --bind a b
> root@...8fff999ce:/# LL_FS_RO=/ LL_FS_RW=/ ./sandboxer bash
> Executing the sandboxed command...
> bash: cannot set terminal process group (191): Inappropriate ioctl for device
> bash: no job control in this shell
> root@...8fff999ce:/# cd /b/foo
> root@...8fff999ce:/b/foo# cat bar
> baz
> root@...8fff999ce:/b/foo# mv /a/foo /foo
> root@...8fff999ce:/b/foo# cd ..
> bash: cd: ..: No such file or directory
> root@...8fff999ce:/b/foo# cat bar
> cat: bar: Permission denied
This is a good test case, we should add a test for that.
>
> I'm not sure if the original behavior was intentional, but since this
> technically counts as a functional changes, just pointing this out.
This is indeed an issue.
>
> Also I'm slightly worried about the performance overhead of doing
> path_connected for every hop in the iteration (but ultimately it's
> Mickaël's call).
Yes, we need to check with a benchmark. We might want to keep the
walker_path.dentry == walker_path.mnt->mnt_root check inlined.
> At least for Landlock, I think if we want to block all
> access to disconnected files, as long as we eventually realize we have
> been disconnected (by doing the "if dentry == path.mnt" check once when we
> reach root), and in that case deny access, we should be good.
>
>
> > @@ -918,12 +915,15 @@ static bool is_access_to_paths_allowed(
> > allowed_parent1 = true;
> > allowed_parent2 = true;
> > }
> > + goto walk_done;
> > + case PATH_PARENT_SAME_MOUNT:
> > break;
> > + default:
> > + WARN_ON_ONCE(1);
> > + goto walk_done;
> > }
> > - parent_dentry = dget_parent(walker_path.dentry);
> > - dput(walker_path.dentry);
> > - walker_path.dentry = parent_dentry;
> > }
> > +walk_done:
> > path_put(&walker_path);
> >
> > if (!allowed_parent1) {
>
>
Powered by blists - more mailing lists