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: <fd995e37-9369-4375-8718-8338e732860f@maowtm.org>
Date: Wed, 11 Jun 2025 01:23:49 +0100
From: Tingmao Wang <m@...wtm.org>
To: Song Liu <song@...nel.org>
Cc: Mickaël Salaün <mic@...ikod.net>,
 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/11/25 00:08, Song Liu wrote:
> On Tue, Jun 10, 2025 at 3:26 PM Tingmao Wang <m@...wtm.org> wrote:
> [..]
>>>
>>>                 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.
> 
> Could you please provide more specific information about this case?

Apologies, it looks like I was mistaken in the above statement.

I was thinking of something like

# mount --mkdir -t tmpfs none tmproot
# cp busybox tmproot/ && chmod +x tmproot/busybox
# mount --move tmproot /
# env LL_FS_RW=/ LL_FS_RO=/.. ./sandboxer chroot /.. /busybox sh
  # echo can write to root > /a
  sh: can't create /a: Permission denied
  ^^^^ this does not work, but I was mistakenly thinking it would

I think because choose_mountpoint_rcu only returns true if
    if (mountpoint != m->mnt.mnt_root)
passes, this situation won't cause ret to be true in your code.

But then I can't think of when
      if (unlikely(IS_ROOT(path->dentry)))
          return ret;
would ever return true, unless somehow d_parent is corrupted?  Maybe I'm
just missing something obvious here.

Anyway, since there's a suggestion from Neil to refactor this, this might
not be too important, so feel free to ignore for now.

> 
> Thanks,
> Song
> 
>> 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
>>