landlock bug: procfs denials because inodes have unstable identity
Subsystems (2)
Hello!
The Landlock LSM lets processes define policies which are attached to struct inode objects internally. As a consequence, if filesystems forget these inodes and start using new inode objects for the same files or directories later on, these policies do not work as intended any more.
As it turns out, procfs has that property as well and can lose inode identities over time, even though it does not happen immediately.
In my understanding, this is not intended. Landlock has explicit selftests to exercise that Landlock policies work in Procfs.
For reference, similar issues have been investigated also for hostfs
[1] and 9p [2]:
[1] https://lore.kernel.org/all/[email protected]/
[2] https://lore.kernel.org/all/[email protected]/
Steps to reproduce
==================
Process A:
* Enter a Landlock domain where access to /proc/sys/kernel/random (and files below it) is allowed.
* Repeatedly open and close the boot_id file from that same directory
At the same time, the dentry caches get freed up.
We can emulate this by writing to /proc/sys/vm/drop_caches from another (root) process:
# echo 2 > /proc/sys/vm/drop_caches
Observed result: Process A fails because of a Landlock refusal opening
the boot_id file. When Landlock walks the path for the check, the inodes have now a different identity than before and don't match, so Landlock denies the operation.
Expected result: Dropping the caches should not affect Landlock's
behaviour.
At the bottom of this email, I am including the source code for a program that does Process A -- you can run this to reproduce, and then write into the drop_caches file from another process as root.
Solution...?
============
I don't have a fix for it yet.
I am open to suggestions. Is this feasible to change in procfs?
—Günther
Reproducer program, for easier reproduction (don't forget to drop caches in parallel):
---landlock_uuid.c
#define _GNU_SOURCE
#include <err.h>
#include <fcntl.h>
#include <linux/landlock.h>
#include <stddef.h>
#include <sys/prctl.h>
#include <sys/syscall.h>
#include <unistd.h>
static int landlock_create_ruleset(struct landlock_ruleset_attr *attr,
size_t size, __u32 flags)
{
return syscall(__NR_landlock_create_ruleset, attr, size, flags);
}static int landlock_add_rule(int ruleset_fd, enum landlock_rule_type rule_type,
void *rule_attr, __u32 flags)
{
return syscall(__NR_landlock_add_rule, ruleset_fd, rule_type,
rule_attr, flags);
}static int landlock_restrict_self(int ruleset_fd, __u32 flags)
{
return syscall(__NR_landlock_restrict_self, ruleset_fd, flags);
}int main(void)
{
struct landlock_ruleset_attr ruleset_attr = {
.handled_access_fs =
LANDLOCK_ACCESS_FS_EXECUTE |
LANDLOCK_ACCESS_FS_WRITE_FILE |
LANDLOCK_ACCESS_FS_READ_FILE |
LANDLOCK_ACCESS_FS_READ_DIR |
LANDLOCK_ACCESS_FS_REMOVE_DIR |
LANDLOCK_ACCESS_FS_REMOVE_FILE |
LANDLOCK_ACCESS_FS_MAKE_CHAR |
LANDLOCK_ACCESS_FS_MAKE_DIR |
LANDLOCK_ACCESS_FS_MAKE_REG |
LANDLOCK_ACCESS_FS_MAKE_SOCK |
LANDLOCK_ACCESS_FS_MAKE_FIFO |
LANDLOCK_ACCESS_FS_MAKE_BLOCK |
LANDLOCK_ACCESS_FS_MAKE_SYM |
LANDLOCK_ACCESS_FS_REFER |
LANDLOCK_ACCESS_FS_TRUNCATE |
LANDLOCK_ACCESS_FS_IOCTL_DEV |
LANDLOCK_ACCESS_FS_RESOLVE_UNIX,
};
/*
* Backwards compatibility checks omitted for brevity.
* Assuming Linux 7.3 here. :)
*/
int ruleset_fd = landlock_create_ruleset(&ruleset_attr,
sizeof(ruleset_attr), 0);
if (ruleset_fd < 0)
err(1, "landlock_create_ruleset");
int parent_fd = open("/proc/sys/kernel/random", O_PATH | O_CLOEXEC);
if (parent_fd < 0)
err(1, "open(/proc/sys/kernel/random)");
struct landlock_path_beneath_attr path_beneath = {
.allowed_access = LANDLOCK_ACCESS_FS_READ_FILE,
.parent_fd = parent_fd,
};
if (landlock_add_rule(ruleset_fd, LANDLOCK_RULE_PATH_BENEATH,
&path_beneath, 0) < 0)
err(1, "landlock_add_rule");
close(parent_fd);
if (prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0) < 0)
err(1, "prctl(PR_SET_NO_NEW_PRIVS)");
if (landlock_restrict_self(ruleset_fd, 0) < 0)
err(1, "landlock_restrict_self");
close(ruleset_fd);
/* Print out the boot_id file once a second. */
for (;;) {
int fd = open("/proc/sys/kernel/random/boot_id", O_RDONLY);
if (fd < 0)
err(1, "open(/proc/sys/kernel/random/boot_id)");
char buf[64];
ssize_t n;
while ((n = read(fd, buf, sizeof(buf))) > 0) {
if (write(STDOUT_FILENO, buf, n) < 0)
err(1, "write");
}
if (n < 0)
err(1, "read");
close(fd);
sleep(1);
}
return 0;
}---
Quoted 8 lines · Günther Noack
The Landlock LSM lets processes define policies which are attached to struct inode objects internally. As a consequence, if filesystems forget these inodes and start using new inode objects for the same files or directories later on, these policies do not work as intended any more.
As it turns out, procfs has that property as well and can lose inode identities over time, even though it does not happen immediately.
Right; basically, dentries can be dropped from caches while the corresponding inodes still exist. In normal filesystems, dentry lookup rediscovers already-existing inodes, but procfs calls new_inode() for non-persistent dentries from several places without checking whether an equivalent inode already exists, which works because procfs doesn't really tie any state to inodes. new_inode() is called from these places for non-persistent dentries:
- proc_pid_make_inode
- proc_get_inode
- proc_sys_make_inode
File systems that care about having something like a 1:1 relationship between an inode and the object it refers to can instead use mechanisms like path_from_stashed() or iget_locked().
Quoted 2 lines
In my understanding, this is not intended. Landlock has explicit selftests to exercise that Landlock policies work in Procfs.
I think from the procfs perspective, it's currently an intentional design choice that simplifies procfs implementation.
Quoted 35 lines
For reference, similar issues have been investigated also for hostfs
[1] and 9p [2]:[1] https://lore.kernel.org/all/[email protected]/
[2] https://lore.kernel.org/all/[email protected]/Steps to reproduce
==================Process A:
* Enter a Landlock domain where access to
/proc/sys/kernel/random(and files below it) is allowed.
* Repeatedly open and close theboot_idfile from that same directoryAt the same time, the dentry caches get freed up.
We can emulate this by writing to/proc/sys/vm/drop_cachesfrom another (root) process:# echo 2 > /proc/sys/vm/drop_cachesObserved result: Process A fails because of a Landlock refusal opening
theboot_idfile. When Landlock walks the path for the check, the inodes have now a different identity than before and don't match, so Landlock denies the operation.Expected result: Dropping the caches should not affect Landlock's
behaviour.At the bottom of this email, I am including the source code for a program that does Process A -- you can run this to reproduce, and then write into the
drop_cachesfile from another process as root.
Quoted 6 lines
Solution...?
============I don't have a fix for it yet.
I am open to suggestions. Is this feasible to change in procfs?
A landlock domain is supposed to be usable for a hierarchy of multiple processes, and for that usecase, if all those processes want a working /proc/self, landlock currently can't meaningfully restrict procfs beyond what is done implicitly through ptrace access checks, right? I think if there is a desire to have fine-grained landlock policies for procfs, that would first require some design decisions on how procfs peculiarities like per-process directories should be handled. I imagine one option would be to add a mechanism that automatically grants full access to {procfs mount}/$pid of all in-domain pids, and then consider whether sysctls should have persistent dentries or something like that?
Quoted 6 lines
—Günther
Reproducer program, for easier reproduction (don't forget to drop caches in parallel):
[...]
Quoted 26 lines · Jann Horn
Quoted 8 lines · Günther Noack
The Landlock LSM lets processes define policies which are attached to struct inode objects internally. As a consequence, if filesystems forget these inodes and start using new inode objects for the same files or directories later on, these policies do not work as intended any more.
As it turns out, procfs has that property as well and can lose inode identities over time, even though it does not happen immediately.
Right; basically, dentries can be dropped from caches while the corresponding inodes still exist. In normal filesystems, dentry lookup rediscovers already-existing inodes, but procfs calls
new_inode()for non-persistent dentries from several places without checking whether an equivalent inode already exists, which works because procfs doesn't really tie any state to inodes.new_inode()is called from these places for non-persistent dentries:-
proc_pid_make_inode
-proc_get_inode
-proc_sys_make_inodeFile systems that care about having something like a 1:1 relationship between an inode and the object it refers to can instead use mechanisms like
path_from_stashed()origet_locked().
Quoted 5 lines
Quoted 2 lines
In my understanding, this is not intended. Landlock has explicit selftests to exercise that Landlock policies work in Procfs.
I think from the procfs perspective, it's currently an intentional design choice that simplifies procfs implementation.
I don't understand why it could help to have a weak mapping between inode and a path while the inode is still referenced.
Christian, what do you think would be possible?
Quoted 43 lines
Quoted 36 lines
For reference, similar issues have been investigated also for hostfs
[1] and 9p [2]:[1] https://lore.kernel.org/all/[email protected]/
[2] https://lore.kernel.org/all/[email protected]/Steps to reproduce
==================Process A:
* Enter a Landlock domain where access to
/proc/sys/kernel/random(and files below it) is allowed.
* Repeatedly open and close theboot_idfile from that same directoryAt the same time, the dentry caches get freed up.
We can emulate this by writing to/proc/sys/vm/drop_cachesfrom another (root) process:# echo 2 > /proc/sys/vm/drop_cachesObserved result: Process A fails because of a Landlock refusal opening
theboot_idfile. When Landlock walks the path for the check, the inodes have now a different identity than before and don't match, so Landlock denies the operation.Expected result: Dropping the caches should not affect Landlock's
behaviour.At the bottom of this email, I am including the source code for a program that does Process A -- you can run this to reproduce, and then write into the
drop_cachesfile from another process as root.Quoted 6 lines
Solution...?
============I don't have a fix for it yet.
I am open to suggestions. Is this feasible to change in procfs?
Quoted 11 lines
A landlock domain is supposed to be usable for a hierarchy of multiple processes, and for that usecase, if all those processes want a working
/proc/self, landlock currently can't meaningfully restrict procfs beyond what is done implicitly through ptrace access checks, right? I think if there is a desire to have fine-grained landlock policies for procfs, that would first require some design decisions on how procfs peculiarities like per-process directories should be handled. I imagine one option would be to add a mechanism that automatically grants full access to {procfs mount}/$pid of all in-domain pids, and then consider whether sysctls should have persistent dentries or something like that?
I agree, it would be useful (and it has been requested) to properly handle procfs. I think the "scope" interface would make sense to access process/thread-related resources. Other kind of files/directory should use the current path_beneath mechanism, and I'm wondering if identifying these kind of files is already reliable (vs the process-related files).
Quoted 9 lines
Quoted 7 lines
—Günther
Reproducer program, for easier reproduction (don't forget to drop caches in parallel):
[...]
Christian Brauner
maintainerQuoted 27 lines · Mickaël Salaün
Quoted 26 lines · Jann Horn
Quoted 8 lines · Günther Noack
The Landlock LSM lets processes define policies which are attached to struct inode objects internally. As a consequence, if filesystems forget these inodes and start using new inode objects for the same files or directories later on, these policies do not work as intended any more.
As it turns out, procfs has that property as well and can lose inode identities over time, even though it does not happen immediately.
Right; basically, dentries can be dropped from caches while the corresponding inodes still exist. In normal filesystems, dentry lookup rediscovers already-existing inodes, but procfs calls
new_inode()for non-persistent dentries from several places without checking whether an equivalent inode already exists, which works because procfs doesn't really tie any state to inodes.new_inode()is called from these places for non-persistent dentries:-
proc_pid_make_inode
-proc_get_inode
-proc_sys_make_inodeFile systems that care about having something like a 1:1 relationship between an inode and the object it refers to can instead use mechanisms like
path_from_stashed()origet_locked().
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
In my understanding, this is not intended. Landlock has explicit selftests to exercise that Landlock policies work in Procfs.
I think from the procfs perspective, it's currently an intentional design choice that simplifies procfs implementation.
I don't understand why it could help to have a weak mapping between inode and a path while the inode is still referenced.
It simplifies the implementation as Jann quite a bit.
Quoted 1 line
Christian, what do you think would be possible?
I think anything that request changes to how procfs lookup handles needs to proof that a) it is a general improvement and b) that it doesn't impact performance meaningfully.
Even if you make use of the 64-bit pidfd inode number in struct pid you'd have to have a lookup data structure. And that will cost no matter how you spin it and in general we're not very forthcoming when it comes to degrading subsystem performance for special-purpose consumers.
Quoted 42 lines
Quoted 42 lines
Quoted 41 lines
For reference, similar issues have been investigated also for hostfs
[1] and 9p [2]:[1] https://lore.kernel.org/all/[email protected]/
[2] https://lore.kernel.org/all/[email protected]/Steps to reproduce
==================Process A:
* Enter a Landlock domain where access to
/proc/sys/kernel/random(and files below it) is allowed.
* Repeatedly open and close theboot_idfile from that same directoryAt the same time, the dentry caches get freed up.
We can emulate this by writing to/proc/sys/vm/drop_cachesfrom another (root) process:# echo 2 > /proc/sys/vm/drop_cachesObserved result: Process A fails because of a Landlock refusal opening
theboot_idfile. When Landlock walks the path for the check, the inodes have now a different identity than before and don't match, so Landlock denies the operation.Expected result: Dropping the caches should not affect Landlock's
behaviour.At the bottom of this email, I am including the source code for a program that does Process A -- you can run this to reproduce, and then write into the
drop_cachesfile from another process as root.Solution...?
============I don't have a fix for it yet.
I am open to suggestions. Is this feasible to change in procfs?
Quoted 10 lines
Quoted 10 lines
A landlock domain is supposed to be usable for a hierarchy of multiple processes, and for that usecase, if all those processes want a working
/proc/self, landlock currently can't meaningfully restrict procfs beyond what is done implicitly through ptrace access checks, right? I think if there is a desire to have fine-grained landlock policies for procfs, that would first require some design decisions on how procfs peculiarities like per-process directories should be handled. I imagine one option would be to add a mechanism that automatically grants full access to {procfs mount}/$pid of all in-domain pids, and then consider whether sysctls should have persistent dentries or
Meh, there's 1775 sysctls on my system alone. Making them permanent means they remain pinned and waste memory. kernfs avoids that because it pins kernfs nodes which is 136 bytes. An inode is about 650 bytes. If you pin all of then you're already wasting 1MB. Why do you want to make Ram Altman unhappy?
Quoted 18 lines
Quoted 1 line
something like that?
I agree, it would be useful (and it has been requested) to properly handle procfs. I think the "scope" interface would make sense to access process/thread-related resources. Other kind of files/directory should use the current
path_beneathmechanism, and I'm wondering if identifying these kind of files is already reliable (vs the process-related files).Quoted 9 lines
Quoted 7 lines
—Günther
Reproducer program, for easier reproduction (don't forget to drop caches in parallel):
[...]