linux-kernel - Re: [PATCH bpf-next 2/4] landlock: Use path

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <39e4c079-aa62-404d-a415-8a53af039122@maowtm.org>
Date: Mon, 2 Jun 2025 23:56:43 +0100
From: Tingmao Wang <m@...wtm.org>
To: Mickaël Salaün <mic@...ikod.net>,
 Song Liu <song@...nel.org>
Cc: 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 6/2/25 18:35, Mickaël Salaün wrote:
> On Sat, May 31, 2025 at 02:51:22PM +0100, Tingmao Wang wrote:
>> [...]
>>
>> / # 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.

Agreed with Mickaël that I will work on a Landlock selftest for this.

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

Because in the non-Landlocked case, applications with a disconnected CWD
still can access files under its CWD, just not above, my thinking is that
it's best to keep the current Landlock behaviour.  Mickaël might reply
here too but he also thought that, from the point of view of the person
creating the policy, the current behaviour is less surprising.

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

I think this will depend on how the final implementation goes, but if we
can check it only once at the very end (potentially for free by having
logic that can realize the walk never reached path.mnt?), I would think it
ought to not make a difference.

For Song's benefit, if you want to do it, here are some scripts that might
come in handy in benchmarking (created by Mickaël):
https://github.com/landlock-lsm/landlock-test-tools/pull/16
and comparing results / BPF-based overhead tracing:
https://github.com/landlock-lsm/landlock-test-tools/pull/17

> >> 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) {
>>
>>