* Re: [PATCH v2 1/3] pidfd: extend pidfd_get_pid() and de-duplicate pid lookup
From: Christian Brauner @ 2024-10-16 13:00 UTC (permalink / raw)
To: Lorenzo Stoakes
Cc: Christian Brauner, Shuah Khan, Liam R . Howlett,
Suren Baghdasaryan, Vlastimil Babka, pedro.falcato,
linux-kselftest, linux-mm, linux-fsdevel, linux-api, linux-kernel
In-Reply-To: <8e7edaf2f648fb01a71def749f17f76c0502dee1.1728643714.git.lorenzo.stoakes@oracle.com>
On Fri, Oct 11, 2024 at 12:05:55PM +0100, Lorenzo Stoakes wrote:
> The means by which a pid is determined from a pidfd is duplicated, with
> some callers holding a reference to the (pid)fd, and others explicitly
> pinning the pid.
>
> Introduce __pidfd_get_pid() which abstracts both approaches and provide
> optional output parameters for file->f_flags and the fd (the latter of
> which, if provided, prevents the function from decrementing the fd's
> refernce count).
>
> Additionally, allow the ability to open a pidfd by opening a /proc/<pid>
> directory, utilised by the pidfd_send_signal() system call, providing a
> pidfd_get_pid_proc() helper function to do so.
>
> Doing this allows us to eliminate open-coded pidfd pid lookup and to
> consistently handle this in one place.
>
> This lays the groundwork for a subsequent patch which adds a new sentinel
> pidfd to explicitly reference the current process (i.e. thread group
> leader) without the need for a pidfd.
>
> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> ---
> include/linux/pid.h | 42 +++++++++++++++++++++++++++++++-
> kernel/pid.c | 58 ++++++++++++++++++++++++++++++---------------
> kernel/signal.c | 22 ++++-------------
> 3 files changed, 84 insertions(+), 38 deletions(-)
>
> diff --git a/include/linux/pid.h b/include/linux/pid.h
> index a3aad9b4074c..68b02eab7509 100644
> --- a/include/linux/pid.h
> +++ b/include/linux/pid.h
> @@ -2,6 +2,7 @@
> #ifndef _LINUX_PID_H
> #define _LINUX_PID_H
>
> +#include <linux/file.h>
> #include <linux/pid_types.h>
> #include <linux/rculist.h>
> #include <linux/rcupdate.h>
> @@ -72,8 +73,47 @@ extern struct pid init_struct_pid;
>
> struct file;
>
> +
> +/**
> + * __pidfd_get_pid() - Retrieve a pid associated with the specified pidfd.
> + *
> + * @pidfd: The pidfd whose pid we want, or the fd of a /proc/<pid> file if
> + * @alloc_proc is also set.
> + * @pin_pid: If set, then the reference counter of the returned pid is
> + * incremented. If not set, then @fd should be provided to pin the
> + * pidfd.
> + * @allow_proc: If set, then an fd of a /proc/<pid> file can be passed instead
> + * of a pidfd, and this will be used to determine the pid.
> + * @flags: Output variable, if non-NULL, then the file->f_flags of the
> + * pidfd will be set here.
> + * @fd: Output variable, if non-NULL, then the pidfd reference will
> + * remain elevated and the caller will need to decrement it
> + * themselves.
> + *
> + * Returns: If successful, the pid associated with the pidfd, otherwise an
> + * error.
> + */
> +struct pid *__pidfd_get_pid(unsigned int pidfd, bool pin_pid,
> + bool allow_proc, unsigned int *flags,
> + struct fd *fd);
> +
> +static inline struct pid *pidfd_get_pid(unsigned int pidfd, unsigned int *flags)
> +{
> + return __pidfd_get_pid(pidfd, /* pin_pid = */ true,
> + /* allow_proc = */ false,
> + flags, /* fd = */ NULL);
> +}
> +
> +static inline struct pid *pidfd_to_pid_proc(unsigned int pidfd,
> + unsigned int *flags,
> + struct fd *fd)
> +{
> + return __pidfd_get_pid(pidfd, /* pin_pid = */ false,
> + /* allow_proc = */ true,
> + flags, fd);
> +}
> +
> struct pid *pidfd_pid(const struct file *file);
> -struct pid *pidfd_get_pid(unsigned int fd, unsigned int *flags);
> struct task_struct *pidfd_get_task(int pidfd, unsigned int *flags);
> int pidfd_prepare(struct pid *pid, unsigned int flags, struct file **ret);
> void do_notify_pidfd(struct task_struct *task);
> diff --git a/kernel/pid.c b/kernel/pid.c
> index 2715afb77eab..25cc1c36a1b1 100644
> --- a/kernel/pid.c
> +++ b/kernel/pid.c
> @@ -36,6 +36,7 @@
> #include <linux/pid_namespace.h>
> #include <linux/init_task.h>
> #include <linux/syscalls.h>
> +#include <linux/proc_fs.h>
> #include <linux/proc_ns.h>
> #include <linux/refcount.h>
> #include <linux/anon_inodes.h>
> @@ -534,22 +535,46 @@ struct pid *find_ge_pid(int nr, struct pid_namespace *ns)
> }
> EXPORT_SYMBOL_GPL(find_ge_pid);
>
> -struct pid *pidfd_get_pid(unsigned int fd, unsigned int *flags)
> +struct pid *__pidfd_get_pid(unsigned int pidfd, bool pin_pid,
> + bool allow_proc, unsigned int *flags,
> + struct fd *fd)
Hm, we should never return a struct fd. A struct fd is an inherently
scoped-bound concept - or at least aims to be. Simply put, we always
want to have the fdget() and the fdput() in the same scope as the file
pointer you can access via fd_file() is only valid as long as we're in
the syscall.
Ideally we mostly use CLASS(fd/fd_raw) and nearly never fdget(). The
point is that this is the wrong api to expose.
It would probably be wiser if you added a pidfd based fdget() inspired
primitive.
^ permalink raw reply
* [PATCH v3 3/3] selftests: pidfd: add tests for PIDFD_SELF_*
From: Lorenzo Stoakes @ 2024-10-16 10:20 UTC (permalink / raw)
To: Christian Brauner
Cc: Shuah Khan, Liam R . Howlett, Suren Baghdasaryan, Vlastimil Babka,
pedro.falcato, linux-kselftest, linux-mm, linux-fsdevel,
linux-api, linux-kernel, Oliver Sang
In-Reply-To: <cover.1729073310.git.lorenzo.stoakes@oracle.com>
Add tests to assert that PIDFD_SELF_* correctly refers to the current
thread and process.
This is only practically meaningful to pidfd_send_signal() and
pidfd_getfd(), but also explicitly test that we disallow this feature for
setns() where it would make no sense.
We cannot reasonably wait on ourself using waitid(P_PIDFD, ...) so while in
theory PIDFD_SELF_* would work here, we'd be left blocked if we tried it.
We defer testing of mm-specific functionality which uses pidfd, namely
process_madvise() and process_mrelease() to mm testing (though note the
latter can not be sensibly tested as it would require the testing process
to be dying).
Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
---
tools/testing/selftests/pidfd/pidfd.h | 8 +
.../selftests/pidfd/pidfd_getfd_test.c | 141 ++++++++++++++++++
.../selftests/pidfd/pidfd_setns_test.c | 11 ++
tools/testing/selftests/pidfd/pidfd_test.c | 76 ++++++++--
4 files changed, 224 insertions(+), 12 deletions(-)
diff --git a/tools/testing/selftests/pidfd/pidfd.h b/tools/testing/selftests/pidfd/pidfd.h
index 88d6830ee004..1640b711889b 100644
--- a/tools/testing/selftests/pidfd/pidfd.h
+++ b/tools/testing/selftests/pidfd/pidfd.h
@@ -50,6 +50,14 @@
#define PIDFD_NONBLOCK O_NONBLOCK
#endif
+/* System header file may not have this available. */
+#ifndef PIDFD_SELF_THREAD
+#define PIDFD_SELF_THREAD -100
+#endif
+#ifndef PIDFD_SELF_THREAD_GROUP
+#define PIDFD_SELF_THREAD_GROUP -200
+#endif
+
/*
* The kernel reserves 300 pids via RESERVED_PIDS in kernel/pid.c
* That means, when it wraps around any pid < 300 will be skipped.
diff --git a/tools/testing/selftests/pidfd/pidfd_getfd_test.c b/tools/testing/selftests/pidfd/pidfd_getfd_test.c
index cd51d547b751..48d224b13c01 100644
--- a/tools/testing/selftests/pidfd/pidfd_getfd_test.c
+++ b/tools/testing/selftests/pidfd/pidfd_getfd_test.c
@@ -6,6 +6,7 @@
#include <limits.h>
#include <linux/types.h>
#include <poll.h>
+#include <pthread.h>
#include <sched.h>
#include <signal.h>
#include <stdio.h>
@@ -15,6 +16,7 @@
#include <sys/prctl.h>
#include <sys/wait.h>
#include <unistd.h>
+#include <sys/mman.h>
#include <sys/socket.h>
#include <linux/kcmp.h>
@@ -114,6 +116,94 @@ static int child(int sk)
return ret;
}
+static int __pidfd_self_thread_worker(unsigned long page_size)
+{
+ int memfd;
+ int newfd;
+ char *ptr;
+ int err = 0;
+
+ /*
+ * Unshare our FDs so we have our own set. This means
+ * PIDFD_SELF_THREAD_GROUP will fal.
+ */
+ if (unshare(CLONE_FILES) < 0) {
+ err = -errno;
+ goto exit;
+ }
+
+ /* Truncate, map in and write to our memfd. */
+ memfd = sys_memfd_create("test_self_child", 0);
+ if (memfd < 0) {
+ err = -errno;
+ goto exit;
+ }
+
+ if (ftruncate(memfd, page_size)) {
+ err = -errno;
+ goto exit_close_memfd;
+ }
+
+ ptr = mmap(NULL, page_size, PROT_READ | PROT_WRITE,
+ MAP_SHARED, memfd, 0);
+ if (ptr == MAP_FAILED) {
+ err = -errno;
+ goto exit_close_memfd;
+ }
+ ptr[0] = 'y';
+ if (munmap(ptr, page_size)) {
+ err = -errno;
+ goto exit_close_memfd;
+ }
+
+ /* Get a thread-local duplicate of our memfd. */
+ newfd = sys_pidfd_getfd(PIDFD_SELF_THREAD, memfd, 0);
+ if (newfd < 0) {
+ err = -errno;
+ goto exit_close_memfd;
+ }
+
+ if (memfd == newfd) {
+ err = -EINVAL;
+ goto exit_close_fds;
+ }
+
+ /* Map in new fd and make sure that the data is as expected. */
+ ptr = mmap(NULL, page_size, PROT_READ | PROT_WRITE,
+ MAP_SHARED, newfd, 0);
+ if (ptr == MAP_FAILED) {
+ err = -errno;
+ goto exit_close_fds;
+ }
+
+ if (ptr[0] != 'y') {
+ err = -EINVAL;
+ goto exit_close_fds;
+ }
+
+ if (munmap(ptr, page_size)) {
+ err = -errno;
+ goto exit_close_fds;
+ }
+
+exit_close_fds:
+ close(newfd);
+exit_close_memfd:
+ close(memfd);
+exit:
+ return err;
+}
+
+static void *pidfd_self_thread_worker(void *arg)
+{
+ unsigned long page_size = (unsigned long)arg;
+ int ret;
+
+ /* We forward any errors for the caller to handle. */
+ ret = __pidfd_self_thread_worker(page_size);
+ return (void *)(intptr_t)ret;
+}
+
FIXTURE(child)
{
/*
@@ -264,6 +354,57 @@ TEST_F(child, no_strange_EBADF)
EXPECT_EQ(errno, ESRCH);
}
+TEST(pidfd_self)
+{
+ int memfd = sys_memfd_create("test_self", 0);
+ unsigned long page_size = sysconf(_SC_PAGESIZE);
+ int newfd;
+ char *ptr;
+ pthread_t thread;
+ void *res;
+ int err;
+
+ ASSERT_GE(memfd, 0);
+ ASSERT_EQ(ftruncate(memfd, page_size), 0);
+
+ /*
+ * Map so we can assert that the duplicated fd references the same
+ * memory.
+ */
+ ptr = mmap(NULL, page_size, PROT_READ | PROT_WRITE,
+ MAP_SHARED, memfd, 0);
+ ASSERT_NE(ptr, MAP_FAILED);
+ ptr[0] = 'x';
+ ASSERT_EQ(munmap(ptr, page_size), 0);
+
+ /* Now get a duplicate of our memfd. */
+ newfd = sys_pidfd_getfd(PIDFD_SELF_THREAD_GROUP, memfd, 0);
+ ASSERT_GE(newfd, 0);
+ ASSERT_NE(memfd, newfd);
+
+ /* Now map duplicate fd and make sure it references the same memory. */
+ ptr = mmap(NULL, page_size, PROT_READ | PROT_WRITE,
+ MAP_SHARED, newfd, 0);
+ ASSERT_NE(ptr, MAP_FAILED);
+ ASSERT_EQ(ptr[0], 'x');
+ ASSERT_EQ(munmap(ptr, page_size), 0);
+
+ /* Cleanup. */
+ close(memfd);
+ close(newfd);
+
+ /*
+ * Fire up the thread and assert that we can lookup the thread-specific
+ * PIDFD_SELF_THREAD (also aliased by PIDFD_SELF).
+ */
+ ASSERT_EQ(pthread_create(&thread, NULL, pidfd_self_thread_worker,
+ (void *)page_size), 0);
+ ASSERT_EQ(pthread_join(thread, &res), 0);
+ err = (int)(intptr_t)res;
+
+ ASSERT_EQ(err, 0);
+}
+
#if __NR_pidfd_getfd == -1
int main(void)
{
diff --git a/tools/testing/selftests/pidfd/pidfd_setns_test.c b/tools/testing/selftests/pidfd/pidfd_setns_test.c
index 7c2a4349170a..bbd39dc5ceb7 100644
--- a/tools/testing/selftests/pidfd/pidfd_setns_test.c
+++ b/tools/testing/selftests/pidfd/pidfd_setns_test.c
@@ -752,4 +752,15 @@ TEST(setns_einval)
close(fd);
}
+TEST(setns_pidfd_self_disallowed)
+{
+ ASSERT_EQ(setns(PIDFD_SELF_THREAD, 0), -1);
+ EXPECT_EQ(errno, EBADF);
+
+ errno = 0;
+
+ ASSERT_EQ(setns(PIDFD_SELF_THREAD_GROUP, 0), -1);
+ EXPECT_EQ(errno, EBADF);
+}
+
TEST_HARNESS_MAIN
diff --git a/tools/testing/selftests/pidfd/pidfd_test.c b/tools/testing/selftests/pidfd/pidfd_test.c
index 9faa686f90e4..440447cf89ba 100644
--- a/tools/testing/selftests/pidfd/pidfd_test.c
+++ b/tools/testing/selftests/pidfd/pidfd_test.c
@@ -42,12 +42,41 @@ static pid_t pidfd_clone(int flags, int *pidfd, int (*fn)(void *))
#endif
}
-static int signal_received;
+static pthread_t signal_received;
static void set_signal_received_on_sigusr1(int sig)
{
if (sig == SIGUSR1)
- signal_received = 1;
+ signal_received = pthread_self();
+}
+
+static int send_signal(int pidfd)
+{
+ int ret = 0;
+
+ if (sys_pidfd_send_signal(pidfd, SIGUSR1, NULL, 0) < 0) {
+ ret = -EINVAL;
+ goto exit;
+ }
+
+ if (signal_received != pthread_self()) {
+ ret = -EINVAL;
+ goto exit;
+ }
+
+exit:
+ signal_received = 0;
+ return ret;
+}
+
+static void *send_signal_worker(void *arg)
+{
+ int pidfd = (int)(intptr_t)arg;
+ int ret;
+
+ /* We forward any errors for the caller to handle. */
+ ret = send_signal(pidfd);
+ return (void *)(intptr_t)ret;
}
/*
@@ -56,8 +85,11 @@ static void set_signal_received_on_sigusr1(int sig)
*/
static int test_pidfd_send_signal_simple_success(void)
{
- int pidfd, ret;
+ int pidfd;
const char *test_name = "pidfd_send_signal send SIGUSR1";
+ pthread_t thread;
+ void *thread_res;
+ int err;
if (!have_pidfd_send_signal) {
ksft_test_result_skip(
@@ -66,25 +98,45 @@ static int test_pidfd_send_signal_simple_success(void)
return 0;
}
+ signal(SIGUSR1, set_signal_received_on_sigusr1);
+
+ /* Try sending a signal to ourselves via /proc/self. */
pidfd = open("/proc/self", O_DIRECTORY | O_CLOEXEC);
if (pidfd < 0)
ksft_exit_fail_msg(
"%s test: Failed to open process file descriptor\n",
test_name);
+ err = send_signal(pidfd);
+ if (err)
+ ksft_exit_fail_msg(
+ "%s test: Error %d on sending pidfd signal\n",
+ test_name, err);
+ close(pidfd);
- signal(SIGUSR1, set_signal_received_on_sigusr1);
+ /* Now try the same thing only using PIDFD_SELF_THREAD_GROUP. */
+ err = send_signal(PIDFD_SELF_THREAD_GROUP);
+ if (err)
+ ksft_exit_fail_msg(
+ "%s test: Error %d on PIDFD_SELF_THREAD_GROUP signal\n",
+ test_name, err);
- ret = sys_pidfd_send_signal(pidfd, SIGUSR1, NULL, 0);
- close(pidfd);
- if (ret < 0)
- ksft_exit_fail_msg("%s test: Failed to send signal\n",
+ /*
+ * Now try the same thing in a thread and assert thread ID is equal to
+ * worker thread ID.
+ */
+ if (pthread_create(&thread, NULL, send_signal_worker,
+ (void *)(intptr_t)PIDFD_SELF_THREAD))
+ ksft_exit_fail_msg("%s test: Failed to create thread\n",
test_name);
-
- if (signal_received != 1)
- ksft_exit_fail_msg("%s test: Failed to receive signal\n",
+ if (pthread_join(thread, &thread_res))
+ ksft_exit_fail_msg("%s test: Failed to join thread\n",
test_name);
+ err = (int)(intptr_t)thread_res;
+ if (err)
+ ksft_exit_fail_msg(
+ "%s test: Error %d on PIDFD_SELF_THREAD signal\n",
+ test_name, err);
- signal_received = 0;
ksft_test_result_pass("%s test: Sent signal\n", test_name);
return 0;
}
--
2.46.2
^ permalink raw reply related
* [PATCH v3 2/3] pidfd: add PIDFD_SELF_* sentinels to refer to own thread/process
From: Lorenzo Stoakes @ 2024-10-16 10:20 UTC (permalink / raw)
To: Christian Brauner
Cc: Shuah Khan, Liam R . Howlett, Suren Baghdasaryan, Vlastimil Babka,
pedro.falcato, linux-kselftest, linux-mm, linux-fsdevel,
linux-api, linux-kernel, Oliver Sang
In-Reply-To: <cover.1729073310.git.lorenzo.stoakes@oracle.com>
It is useful to be able to utilise pidfd mechanisms to reference the
current thread or process (from a userland point of view - thread group
leader from the kernel's point of view).
Therefore introduce PIDFD_SELF_THREAD to refer to the current thread, and
PIDFD_SELF_THREAD_GROUP to refer to the current thread group leader.
For convenience and to avoid confusion from userland's perspective we alias
these:
* PIDFD_SELF is an alias for PIDFD_SELF_THREAD - This is nearly always what
the user will want to use, as they would find it surprising if for
instance fd's were unshared()'d and they wanted to invoke pidfd_getfd()
and that failed.
* PIDFD_SELF_PROCESS is an alias for PIDFD_SELF_THREAD_GROUP - Most users
have no concept of thread groups or what a thread group leader is, and
from userland's perspective and nomenclature this is what userland
considers to be a process.
Due to the refactoring of the central __pidfd_get_pid() function we can
implement this functionality centrally, providing the use of this sentinel
in most functionality which utilises pidfd's.
We need to explicitly adjust kernel_waitid_prepare() to permit this (though
it wouldn't really make sense to use this there, we provide the ability for
consistency).
We explicitly disallow use of this in setns(), which would otherwise have
required explicit custom handling, as it doesn't make sense to set the
current calling thread to join the namespace of itself.
As the callers of pidfd_get_pid() expect an increased reference count on
the pid we do so in the self case, reducing churn and avoiding any breakage
from existing logic which decrements this reference count.
In the pidfd_send_signal() system call, we can continue to fdput() the
struct fd output by pidfs_to_pid_proc() even if PIDFD_SELF_* is specified,
as this will be empty and the invocation will be a no-op.
This change implicitly provides PIDFD_SELF_* support in the waitid(P_PIDFS,
...), process_madvise(), process_mrelease(), pidfd_send_signal(), and
pidfd_getfd() system calls.
Things such as polling a pidfs and general fd operations are not supported,
this strictly provides the sentinel for APIs which explicitly accept a
pidfd.
Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
---
include/linux/pid.h | 9 +++---
include/uapi/linux/pidfd.h | 15 +++++++++
kernel/exit.c | 3 +-
kernel/nsproxy.c | 1 +
kernel/pid.c | 65 +++++++++++++++++++++++---------------
5 files changed, 62 insertions(+), 31 deletions(-)
diff --git a/include/linux/pid.h b/include/linux/pid.h
index 68b02eab7509..7c9ed1b5d16f 100644
--- a/include/linux/pid.h
+++ b/include/linux/pid.h
@@ -77,18 +77,19 @@ struct file;
/**
* __pidfd_get_pid() - Retrieve a pid associated with the specified pidfd.
*
- * @pidfd: The pidfd whose pid we want, or the fd of a /proc/<pid> file if
- * @alloc_proc is also set.
+ * @pidfd: The pidfd whose pid we want, the fd of a /proc/<pid> file if
+ * @alloc_proc is also set, or PIDFD_SELF_* to refer to the current
+ * thread or thread group leader.
* @pin_pid: If set, then the reference counter of the returned pid is
* incremented. If not set, then @fd should be provided to pin the
* pidfd.
* @allow_proc: If set, then an fd of a /proc/<pid> file can be passed instead
* of a pidfd, and this will be used to determine the pid.
* @flags: Output variable, if non-NULL, then the file->f_flags of the
- * pidfd will be set here.
+ * pidfd will be set here. If PIDFD_SELF_* set, this is zero.
* @fd: Output variable, if non-NULL, then the pidfd reference will
* remain elevated and the caller will need to decrement it
- * themselves.
+ * themselves. If PIDFD_SELF_* set, this is empty.
*
* Returns: If successful, the pid associated with the pidfd, otherwise an
* error.
diff --git a/include/uapi/linux/pidfd.h b/include/uapi/linux/pidfd.h
index 565fc0629fff..f4db20d76f4b 100644
--- a/include/uapi/linux/pidfd.h
+++ b/include/uapi/linux/pidfd.h
@@ -29,4 +29,19 @@
#define PIDFD_GET_USER_NAMESPACE _IO(PIDFS_IOCTL_MAGIC, 9)
#define PIDFD_GET_UTS_NAMESPACE _IO(PIDFS_IOCTL_MAGIC, 10)
+/*
+ * Special sentinel values which can be used to refer to the current thread or
+ * thread group leader (which from a userland perspective is the process).
+ */
+#define PIDFD_SELF PIDFD_SELF_THREAD
+#define PIDFD_SELF_PROCESS PIDFD_SELF_THREAD_GROUP
+
+#define PIDFD_SELF_THREAD -100 /* Current thread. */
+#define PIDFD_SELF_THREAD_GROUP -200 /* Current thread group leader. */
+
+static inline bool pidfd_is_self_sentinel(pid_t pid)
+{
+ return pid == PIDFD_SELF_THREAD || pid == PIDFD_SELF_THREAD_GROUP;
+}
+
#endif /* _UAPI_LINUX_PIDFD_H */
diff --git a/kernel/exit.c b/kernel/exit.c
index 619f0014c33b..3eb20f8252ee 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -71,6 +71,7 @@
#include <linux/user_events.h>
#include <linux/uaccess.h>
+#include <uapi/linux/pidfd.h>
#include <uapi/linux/wait.h>
#include <asm/unistd.h>
@@ -1739,7 +1740,7 @@ int kernel_waitid_prepare(struct wait_opts *wo, int which, pid_t upid,
break;
case P_PIDFD:
type = PIDTYPE_PID;
- if (upid < 0)
+ if (upid < 0 && !pidfd_is_self_sentinel(upid))
return -EINVAL;
pid = pidfd_get_pid(upid, &f_flags);
diff --git a/kernel/nsproxy.c b/kernel/nsproxy.c
index dc952c3b05af..d239f7eeaa1f 100644
--- a/kernel/nsproxy.c
+++ b/kernel/nsproxy.c
@@ -550,6 +550,7 @@ SYSCALL_DEFINE2(setns, int, fd, int, flags)
struct nsset nsset = {};
int err = 0;
+ /* If fd is PIDFD_SELF_*, implicitly fail here, as invalid. */
if (!fd_file(f))
return -EBADF;
diff --git a/kernel/pid.c b/kernel/pid.c
index 25cc1c36a1b1..0f8943ecc471 100644
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -539,22 +539,31 @@ struct pid *__pidfd_get_pid(unsigned int pidfd, bool pin_pid,
bool allow_proc, unsigned int *flags,
struct fd *fd)
{
- struct file *file;
+ struct file *file = NULL;
struct pid *pid;
- struct fd f = fdget(pidfd);
-
- file = fd_file(f);
- if (!file)
- return ERR_PTR(-EBADF);
-
- pid = pidfd_pid(file);
- /* If we allow opening a pidfd via /proc/<pid>, do so. */
- if (IS_ERR(pid) && allow_proc)
- pid = tgid_pidfd_to_pid(file);
-
- if (IS_ERR(pid)) {
- fdput(f);
- return pid;
+ unsigned int f_flags = 0;
+ struct fd f = {};
+
+ if (pidfd == PIDFD_SELF_THREAD) {
+ pid = *task_pid_ptr(current, PIDTYPE_PID);
+ f_flags = PIDFD_THREAD;
+ } else if (pidfd == PIDFD_SELF_THREAD_GROUP) {
+ pid = *task_pid_ptr(current, PIDTYPE_TGID);
+ } else {
+ f = fdget(pidfd);
+ file = fd_file(f);
+ if (!file)
+ return ERR_PTR(-EBADF);
+
+ pid = pidfd_pid(file);
+ /* If we allow opening a pidfd via /proc/<pid>, do so. */
+ if (IS_ERR(pid) && allow_proc)
+ pid = tgid_pidfd_to_pid(file);
+
+ if (IS_ERR(pid)) {
+ fdput(f);
+ return pid;
+ }
}
if (pin_pid)
@@ -562,18 +571,22 @@ struct pid *__pidfd_get_pid(unsigned int pidfd, bool pin_pid,
else
WARN_ON_ONCE(!fd); /* Nothing to keep pid/pidfd around? */
- if (flags)
- *flags = file->f_flags;
+ if (file) {
+ f_flags = file->f_flags;
- /*
- * If the user provides an fd output then it will handle decrementing
- * its reference counter.
- */
- if (fd)
- *fd = f;
- else
- /* Otherwise we release it. */
- fdput(f);
+ /*
+ * If the user provides an fd output then it will handle decrementing
+ * its reference counter.
+ */
+ if (fd)
+ *fd = f;
+ else
+ /* Otherwise we release it. */
+ fdput(f);
+ }
+
+ if (flags)
+ *flags = f_flags;
return pid;
}
--
2.46.2
^ permalink raw reply related
* [PATCH v3 1/3] pidfd: extend pidfd_get_pid() and de-duplicate pid lookup
From: Lorenzo Stoakes @ 2024-10-16 10:20 UTC (permalink / raw)
To: Christian Brauner
Cc: Shuah Khan, Liam R . Howlett, Suren Baghdasaryan, Vlastimil Babka,
pedro.falcato, linux-kselftest, linux-mm, linux-fsdevel,
linux-api, linux-kernel, Oliver Sang
In-Reply-To: <cover.1729073310.git.lorenzo.stoakes@oracle.com>
The means by which a pid is determined from a pidfd is duplicated, with
some callers holding a reference to the (pid)fd, and others explicitly
pinning the pid.
Introduce __pidfd_get_pid() which abstracts both approaches and provide
optional output parameters for file->f_flags and the fd (the latter of
which, if provided, prevents the function from decrementing the fd's
refernce count).
Additionally, allow the ability to open a pidfd by opening a /proc/<pid>
directory, utilised by the pidfd_send_signal() system call, providing a
pidfd_get_pid_proc() helper function to do so.
Doing this allows us to eliminate open-coded pidfd pid lookup and to
consistently handle this in one place.
This lays the groundwork for a subsequent patch which adds a new sentinel
pidfd to explicitly reference the current process (i.e. thread group
leader) without the need for a pidfd.
Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
---
include/linux/pid.h | 42 +++++++++++++++++++++++++++++++-
kernel/pid.c | 58 ++++++++++++++++++++++++++++++---------------
kernel/signal.c | 26 ++++----------------
3 files changed, 85 insertions(+), 41 deletions(-)
diff --git a/include/linux/pid.h b/include/linux/pid.h
index a3aad9b4074c..68b02eab7509 100644
--- a/include/linux/pid.h
+++ b/include/linux/pid.h
@@ -2,6 +2,7 @@
#ifndef _LINUX_PID_H
#define _LINUX_PID_H
+#include <linux/file.h>
#include <linux/pid_types.h>
#include <linux/rculist.h>
#include <linux/rcupdate.h>
@@ -72,8 +73,47 @@ extern struct pid init_struct_pid;
struct file;
+
+/**
+ * __pidfd_get_pid() - Retrieve a pid associated with the specified pidfd.
+ *
+ * @pidfd: The pidfd whose pid we want, or the fd of a /proc/<pid> file if
+ * @alloc_proc is also set.
+ * @pin_pid: If set, then the reference counter of the returned pid is
+ * incremented. If not set, then @fd should be provided to pin the
+ * pidfd.
+ * @allow_proc: If set, then an fd of a /proc/<pid> file can be passed instead
+ * of a pidfd, and this will be used to determine the pid.
+ * @flags: Output variable, if non-NULL, then the file->f_flags of the
+ * pidfd will be set here.
+ * @fd: Output variable, if non-NULL, then the pidfd reference will
+ * remain elevated and the caller will need to decrement it
+ * themselves.
+ *
+ * Returns: If successful, the pid associated with the pidfd, otherwise an
+ * error.
+ */
+struct pid *__pidfd_get_pid(unsigned int pidfd, bool pin_pid,
+ bool allow_proc, unsigned int *flags,
+ struct fd *fd);
+
+static inline struct pid *pidfd_get_pid(unsigned int pidfd, unsigned int *flags)
+{
+ return __pidfd_get_pid(pidfd, /* pin_pid = */ true,
+ /* allow_proc = */ false,
+ flags, /* fd = */ NULL);
+}
+
+static inline struct pid *pidfd_to_pid_proc(unsigned int pidfd,
+ unsigned int *flags,
+ struct fd *fd)
+{
+ return __pidfd_get_pid(pidfd, /* pin_pid = */ false,
+ /* allow_proc = */ true,
+ flags, fd);
+}
+
struct pid *pidfd_pid(const struct file *file);
-struct pid *pidfd_get_pid(unsigned int fd, unsigned int *flags);
struct task_struct *pidfd_get_task(int pidfd, unsigned int *flags);
int pidfd_prepare(struct pid *pid, unsigned int flags, struct file **ret);
void do_notify_pidfd(struct task_struct *task);
diff --git a/kernel/pid.c b/kernel/pid.c
index 2715afb77eab..25cc1c36a1b1 100644
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -36,6 +36,7 @@
#include <linux/pid_namespace.h>
#include <linux/init_task.h>
#include <linux/syscalls.h>
+#include <linux/proc_fs.h>
#include <linux/proc_ns.h>
#include <linux/refcount.h>
#include <linux/anon_inodes.h>
@@ -534,22 +535,46 @@ struct pid *find_ge_pid(int nr, struct pid_namespace *ns)
}
EXPORT_SYMBOL_GPL(find_ge_pid);
-struct pid *pidfd_get_pid(unsigned int fd, unsigned int *flags)
+struct pid *__pidfd_get_pid(unsigned int pidfd, bool pin_pid,
+ bool allow_proc, unsigned int *flags,
+ struct fd *fd)
{
- struct fd f;
+ struct file *file;
struct pid *pid;
+ struct fd f = fdget(pidfd);
- f = fdget(fd);
- if (!fd_file(f))
+ file = fd_file(f);
+ if (!file)
return ERR_PTR(-EBADF);
- pid = pidfd_pid(fd_file(f));
- if (!IS_ERR(pid)) {
- get_pid(pid);
- *flags = fd_file(f)->f_flags;
+ pid = pidfd_pid(file);
+ /* If we allow opening a pidfd via /proc/<pid>, do so. */
+ if (IS_ERR(pid) && allow_proc)
+ pid = tgid_pidfd_to_pid(file);
+
+ if (IS_ERR(pid)) {
+ fdput(f);
+ return pid;
}
- fdput(f);
+ if (pin_pid)
+ get_pid(pid);
+ else
+ WARN_ON_ONCE(!fd); /* Nothing to keep pid/pidfd around? */
+
+ if (flags)
+ *flags = file->f_flags;
+
+ /*
+ * If the user provides an fd output then it will handle decrementing
+ * its reference counter.
+ */
+ if (fd)
+ *fd = f;
+ else
+ /* Otherwise we release it. */
+ fdput(f);
+
return pid;
}
@@ -747,23 +772,18 @@ SYSCALL_DEFINE3(pidfd_getfd, int, pidfd, int, fd,
unsigned int, flags)
{
struct pid *pid;
- struct fd f;
int ret;
/* flags is currently unused - make sure it's unset */
if (flags)
return -EINVAL;
- f = fdget(pidfd);
- if (!fd_file(f))
- return -EBADF;
-
- pid = pidfd_pid(fd_file(f));
+ pid = pidfd_get_pid(pidfd, NULL);
if (IS_ERR(pid))
- ret = PTR_ERR(pid);
- else
- ret = pidfd_getfd(pid, fd);
+ return PTR_ERR(pid);
- fdput(f);
+ ret = pidfd_getfd(pid, fd);
+
+ put_pid(pid);
return ret;
}
diff --git a/kernel/signal.c b/kernel/signal.c
index 4344860ffcac..e64da346a90b 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -3875,17 +3875,6 @@ static int copy_siginfo_from_user_any(kernel_siginfo_t *kinfo,
return copy_siginfo_from_user(kinfo, info);
}
-static struct pid *pidfd_to_pid(const struct file *file)
-{
- struct pid *pid;
-
- pid = pidfd_pid(file);
- if (!IS_ERR(pid))
- return pid;
-
- return tgid_pidfd_to_pid(file);
-}
-
#define PIDFD_SEND_SIGNAL_FLAGS \
(PIDFD_SIGNAL_THREAD | PIDFD_SIGNAL_THREAD_GROUP | \
PIDFD_SIGNAL_PROCESS_GROUP)
@@ -3912,6 +3901,7 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig,
struct pid *pid;
kernel_siginfo_t kinfo;
enum pid_type type;
+ unsigned int f_flags;
/* Enforce flags be set to 0 until we add an extension. */
if (flags & ~PIDFD_SEND_SIGNAL_FLAGS)
@@ -3921,16 +3911,10 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig,
if (hweight32(flags & PIDFD_SEND_SIGNAL_FLAGS) > 1)
return -EINVAL;
- f = fdget(pidfd);
- if (!fd_file(f))
- return -EBADF;
-
/* Is this a pidfd? */
- pid = pidfd_to_pid(fd_file(f));
- if (IS_ERR(pid)) {
- ret = PTR_ERR(pid);
- goto err;
- }
+ pid = pidfd_to_pid_proc(pidfd, &f_flags, &f);
+ if (IS_ERR(pid))
+ return PTR_ERR(pid);
ret = -EINVAL;
if (!access_pidfd_pidns(pid))
@@ -3939,7 +3923,7 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig,
switch (flags) {
case 0:
/* Infer scope from the type of pidfd. */
- if (fd_file(f)->f_flags & PIDFD_THREAD)
+ if (f_flags & PIDFD_THREAD)
type = PIDTYPE_PID;
else
type = PIDTYPE_TGID;
--
2.46.2
^ permalink raw reply related
* [PATCH v3 0/3] introduce PIDFD_SELF* sentinels
From: Lorenzo Stoakes @ 2024-10-16 10:20 UTC (permalink / raw)
To: Christian Brauner
Cc: Shuah Khan, Liam R . Howlett, Suren Baghdasaryan, Vlastimil Babka,
pedro.falcato, linux-kselftest, linux-mm, linux-fsdevel,
linux-api, linux-kernel, Oliver Sang
If you wish to utilise a pidfd interface to refer to the current process or
thread it is rather cumbersome, requiring something like:
int pidfd = pidfd_open(getpid(), 0 or PIDFD_THREAD);
...
close(pidfd);
Or the equivalent call opening /proc/self. It is more convenient to use a
sentinel value to indicate to an interface that accepts a pidfd that we
simply wish to refer to the current process thread.
This series introduces sentinels for this purposes which can be passed as
the pidfd in this instance rather than having to establish a dummy fd for
this purpose.
It is useful to refer to both the current thread from the userland's
perspective for which we use PIDFD_SELF, and the current process from the
userland's perspective, for which we use PIDFD_SELF_PROCESS.
There is unfortunately some confusion between the kernel and userland as to
what constitutes a process - a thread from the userland perspective is a
process in userland, and a userland process is a thread group (more
specifically the thread group leader from the kernel perspective). We
therefore alias things thusly:
* PIDFD_SELF_THREAD aliased by PIDFD_SELF - use PIDTYPE_PID.
* PIDFD_SELF_THREAD_GROUP alised by PIDFD_SELF_PROCESS - use PIDTYPE_TGID.
In all of the kernel code we refer to PIDFD_SELF_THREAD and
PIDFD_SELF_THREAD_GROUP. However we expect users to use PIDFD_SELF and
PIDFD_SELF_PROCESS.
This matters for cases where, for instance, a user unshare()'s FDs or does
thread-specific signal handling and where the user would be hugely confused
if the FDs referenced or signal processed referred to the thread group
leader rather than the individual thread.
We ensure that pidfd_send_signal() and pidfd_getfd() work correctly, and
assert as much in selftests. All other interfaces except setns() will work
implicitly with this new interface, however it doesn't make sense to test
waitid(P_PIDFD, ...) as waiting on ourselves is a blocking operation.
In the case of setns() we explicitly disallow use of PIDFD_SELF* as it
doesn't make sense to obtain the namespaces of our own process, and it
would require work to implement this functionality there that would be of
no use.
We also do not provide the ability to utilise PIDFD_SELF* in ordinary fd
operations such as open() or poll(), as this would require extensive work
and be of no real use.
v3:
* Do not fput() an invalid fd as reported by kernel test bot.
* Fix unintended churn from moving variable declaration.
v2:
* Fix tests as reported by Shuah.
* Correct RFC version lore link.
https://lore.kernel.org/linux-mm/cover.1728643714.git.lorenzo.stoakes@oracle.com/
Non-RFC v1:
* Removed RFC tag - there seems to be general consensus that this change is
a good idea, but perhaps some debate to be had on implementation. It
seems sensible then to move forward with the RFC flag removed.
* Introduced PIDFD_SELF_THREAD, PIDFD_SELF_THREAD_GROUP and their aliases
PIDFD_SELF and PIDFD_SELF_PROCESS respectively.
* Updated testing accordingly.
https://lore.kernel.org/linux-mm/cover.1728578231.git.lorenzo.stoakes@oracle.com/
RFC version:
https://lore.kernel.org/linux-mm/cover.1727644404.git.lorenzo.stoakes@oracle.com/
Lorenzo Stoakes (3):
pidfd: extend pidfd_get_pid() and de-duplicate pid lookup
pidfd: add PIDFD_SELF_* sentinels to refer to own thread/process
selftests: pidfd: add tests for PIDFD_SELF_*
include/linux/pid.h | 43 +++++-
include/uapi/linux/pidfd.h | 15 ++
kernel/exit.c | 3 +-
kernel/nsproxy.c | 1 +
kernel/pid.c | 73 ++++++---
kernel/signal.c | 26 +---
tools/testing/selftests/pidfd/pidfd.h | 8 +
.../selftests/pidfd/pidfd_getfd_test.c | 141 ++++++++++++++++++
.../selftests/pidfd/pidfd_setns_test.c | 11 ++
tools/testing/selftests/pidfd/pidfd_test.c | 76 ++++++++--
10 files changed, 342 insertions(+), 55 deletions(-)
--
2.46.2
^ permalink raw reply
* Re: [PATCH v2 1/3] pidfd: extend pidfd_get_pid() and de-duplicate pid lookup
From: Lorenzo Stoakes @ 2024-10-16 9:46 UTC (permalink / raw)
To: kernel test robot
Cc: oe-lkp, lkp, linux-kernel, Christian Brauner, Shuah Khan,
Liam R . Howlett, Suren Baghdasaryan, Vlastimil Babka,
pedro.falcato, linux-kselftest, linux-mm, linux-fsdevel,
linux-api
In-Reply-To: <202410161634.abca3854-lkp@intel.com>
On Wed, Oct 16, 2024 at 04:50:56PM +0800, kernel test robot wrote:
>
>
> Hello,
>
> kernel test robot noticed "BUG:unable_to_handle_page_fault_for_address" on:
Thanks, see below for analysis.
>
> commit: e65dbb5c9051a4da2305787fd558e1d60de2275a ("[PATCH v2 1/3] pidfd: extend pidfd_get_pid() and de-duplicate pid lookup")
> url: https://github.com/intel-lab-lkp/linux/commits/Lorenzo-Stoakes/pidfd-extend-pidfd_get_pid-and-de-duplicate-pid-lookup/20241011-191241
> base: https://git.kernel.org/cgit/linux/kernel/git/shuah/linux-kselftest.git next
> patch link: https://lore.kernel.org/all/8e7edaf2f648fb01a71def749f17f76c0502dee1.1728643714.git.lorenzo.stoakes@oracle.com/
> patch subject: [PATCH v2 1/3] pidfd: extend pidfd_get_pid() and de-duplicate pid lookup
>
> in testcase: trinity
> version: trinity-i386-abe9de86-1_20230429
> with following parameters:
>
> runtime: 600s
>
>
>
> config: x86_64-randconfig-072-20241015
> compiler: gcc-12
> test machine: qemu-system-x86_64 -enable-kvm -cpu SandyBridge -smp 2 -m 16G
>
> (please refer to attached dmesg/kmsg for entire log/backtrace)
>
>
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <oliver.sang@intel.com>
> | Closes: https://lore.kernel.org/oe-lkp/202410161634.abca3854-lkp@intel.com
>
>
> [ 416.054386][ T1959] BUG: unable to handle page fault for address: ffffffff8fed9474
> [ 416.055651][ T1959] #PF: supervisor write access in kernel mode
> [ 416.056550][ T1959] #PF: error_code(0x0003) - permissions violation
> [ 416.057502][ T1959] PGD 3e90f5067 P4D 3e90f5067 PUD 3e90f6063 PMD 3e50001a1
> [ 416.058587][ T1959] Oops: Oops: 0003 [#1] PREEMPT SMP KASAN
> [ 416.059414][ T1959] CPU: 1 UID: 65534 PID: 1959 Comm: trinity-c3 Not tainted 6.12.0-rc1-00004-ge65dbb5c9051 #1 d7a38916ac9252f968706afc2c77f70fbdabe689
> [ 416.061328][ T1959] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.2-debian-1.16.2-1 04/01/2014
> [ 416.062850][ T1959] RIP: 0010:fput (arch/x86/include/asm/atomic64_64.h:61 include/linux/atomic/atomic-arch-fallback.h:4404 include/linux/atomic/atomic-long.h:1571 include/linux/atomic/atomic-instrumented.h:4540 fs/file_table.c:482)
> [ 416.063578][ T1959] Code: ff ff 66 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 00 f3 0f 1e fa 55 48 89 e5 41 55 41 54 53 48 89 fb be 08 00 00 00 e8 96 c6 f7 ff <f0> 48 ff 0b 0f 85 dd 00 00 00 65 4c 8b 25 04 ff 0e 70 4c 8d 6b 48
> All code
> ========
> 0: ff (bad)
> 1: ff 66 66 jmp *0x66(%rsi)
> 4: 2e 0f 1f 84 00 00 00 cs nopl 0x0(%rax,%rax,1)
> b: 00 00
> d: 0f 1f 00 nopl (%rax)
> 10: f3 0f 1e fa endbr64
> 14: 55 push %rbp
> 15: 48 89 e5 mov %rsp,%rbp
> 18: 41 55 push %r13
> 1a: 41 54 push %r12
> 1c: 53 push %rbx
> 1d: 48 89 fb mov %rdi,%rbx
> 20: be 08 00 00 00 mov $0x8,%esi
> 25: e8 96 c6 f7 ff call 0xfffffffffff7c6c0
> 2a:* f0 48 ff 0b lock decq (%rbx) <-- trapping instruction
OK so this looks like the fput() invoking atomic_long_dec_and_test() on an
invalid &file->f_count.
It looks like 0xffffffff8fed9474 in RBX is the file...
And that's because I'm not setting f in
SYSCALL_DEFINE4(pidfd_send_signal, ...) at:
pidfd_to_pid_proc(pidfd, &f_flags, &f);
On error and yet then jump to
err:
fdput(f);
return ret;
Which is trying to fdput() (thus fput()) the f, ugh.
OK I will fix this + respin, thanks for the report!
[snip]
^ permalink raw reply
* Re: [PATCH v2 1/3] pidfd: extend pidfd_get_pid() and de-duplicate pid lookup
From: Suren Baghdasaryan @ 2024-10-16 9:02 UTC (permalink / raw)
To: Lorenzo Stoakes
Cc: Christian Brauner, Shuah Khan, Liam R . Howlett, Vlastimil Babka,
pedro.falcato, linux-kselftest, linux-mm, linux-fsdevel,
linux-api, linux-kernel
In-Reply-To: <2c783f7d-78d4-4d91-9999-12d7772b6272@lucifer.local>
On Wed, Oct 16, 2024 at 1:22 AM Lorenzo Stoakes
<lorenzo.stoakes@oracle.com> wrote:
>
> On Wed, Oct 16, 2024 at 01:16:15AM -0700, Suren Baghdasaryan wrote:
> > On Tue, Oct 15, 2024 at 11:05 PM Lorenzo Stoakes
> > <lorenzo.stoakes@oracle.com> wrote:
> > >
> > > On Tue, Oct 15, 2024 at 12:40:41PM -0700, Suren Baghdasaryan wrote:
> > > [snip]
> > > > > -struct pid *pidfd_get_pid(unsigned int fd, unsigned int *flags)
> > > > > +struct pid *__pidfd_get_pid(unsigned int pidfd, bool pin_pid,
> > > > > + bool allow_proc, unsigned int *flags,
> > > > > + struct fd *fd)
> > > > > {
> > > > > - struct fd f;
> > > > > + struct file *file;
> > > > > struct pid *pid;
> > > > > + struct fd f = fdget(pidfd);
> > > > >
> > > > > - f = fdget(fd);
> > > > > - if (!fd_file(f))
> > > > > + file = fd_file(f);
> > > > > + if (!file)
> > > > > return ERR_PTR(-EBADF);
> > > > >
> > > > > - pid = pidfd_pid(fd_file(f));
> > > > > - if (!IS_ERR(pid)) {
> > > > > - get_pid(pid);
> > > > > - *flags = fd_file(f)->f_flags;
> > > > > + pid = pidfd_pid(file);
> > > > > + /* If we allow opening a pidfd via /proc/<pid>, do so. */
> > > > > + if (IS_ERR(pid) && allow_proc)
> > > > > + pid = tgid_pidfd_to_pid(file);
> > > > > +
> > > > > + if (IS_ERR(pid)) {
> > > > > + fdput(f);
> > > > > + return pid;
> > > > > }
> > > > >
> > > > > - fdput(f);
> > > > > + if (pin_pid)
> > > > > + get_pid(pid);
> > > > > + else
> > > > > + WARN_ON_ONCE(!fd); /* Nothing to keep pid/pidfd around? */
> > > > > +
> > > > > + if (flags)
> > > > > + *flags = file->f_flags;
> > > > > +
> > > > > + /*
> > > > > + * If the user provides an fd output then it will handle decrementing
> > > > > + * its reference counter.
> > > > > + */
> > > > > + if (fd)
> > > > > + *fd = f;
> > > > > + else
> > > > > + /* Otherwise we release it. */
> > > > > + fdput(f);
> > > > > +
> > > > > return pid;
> > > > > }
> > > >
> > > > There is an EXPORT_SYMBOL_GPL(pidfd_get_pid) right after this line. It
> > > > should also be changed to EXPORT_SYMBOL_GPL(__pidfd_get_pid),
> > > > otherwise __pidfd_get_pid() will not be exported. A module calling
> > > > pidfd_get_pid() now inlined in the header file will try to call
> > > > __pidfd_get_pid() and will have trouble resolving this symbol.
> > >
> > > Hmm hang on not there isn't? I don't see that anywhere?
> >
> > Doh! Sorry, I didn't realize the export was an out-of-tree Android
> > change. Never mind...
>
> No probs :P just glad I didn't miss something in this series!
>
> Hey maybe a motivation to upstream some of this? ;)
I wish... Without an upstream user the exports are not accepted
upstream and unfortunately Android vendors often resist upstreaming
their modules.
>
> >
> > >
> > > [snip]
^ permalink raw reply
* Re: [PATCH v2 1/3] pidfd: extend pidfd_get_pid() and de-duplicate pid lookup
From: kernel test robot @ 2024-10-16 8:50 UTC (permalink / raw)
To: Lorenzo Stoakes
Cc: oe-lkp, lkp, linux-kernel, Christian Brauner, Shuah Khan,
Liam R . Howlett, Suren Baghdasaryan, Vlastimil Babka,
pedro.falcato, linux-kselftest, linux-mm, linux-fsdevel,
linux-api, oliver.sang
In-Reply-To: <8e7edaf2f648fb01a71def749f17f76c0502dee1.1728643714.git.lorenzo.stoakes@oracle.com>
Hello,
kernel test robot noticed "BUG:unable_to_handle_page_fault_for_address" on:
commit: e65dbb5c9051a4da2305787fd558e1d60de2275a ("[PATCH v2 1/3] pidfd: extend pidfd_get_pid() and de-duplicate pid lookup")
url: https://github.com/intel-lab-lkp/linux/commits/Lorenzo-Stoakes/pidfd-extend-pidfd_get_pid-and-de-duplicate-pid-lookup/20241011-191241
base: https://git.kernel.org/cgit/linux/kernel/git/shuah/linux-kselftest.git next
patch link: https://lore.kernel.org/all/8e7edaf2f648fb01a71def749f17f76c0502dee1.1728643714.git.lorenzo.stoakes@oracle.com/
patch subject: [PATCH v2 1/3] pidfd: extend pidfd_get_pid() and de-duplicate pid lookup
in testcase: trinity
version: trinity-i386-abe9de86-1_20230429
with following parameters:
runtime: 600s
config: x86_64-randconfig-072-20241015
compiler: gcc-12
test machine: qemu-system-x86_64 -enable-kvm -cpu SandyBridge -smp 2 -m 16G
(please refer to attached dmesg/kmsg for entire log/backtrace)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <oliver.sang@intel.com>
| Closes: https://lore.kernel.org/oe-lkp/202410161634.abca3854-lkp@intel.com
[ 416.054386][ T1959] BUG: unable to handle page fault for address: ffffffff8fed9474
[ 416.055651][ T1959] #PF: supervisor write access in kernel mode
[ 416.056550][ T1959] #PF: error_code(0x0003) - permissions violation
[ 416.057502][ T1959] PGD 3e90f5067 P4D 3e90f5067 PUD 3e90f6063 PMD 3e50001a1
[ 416.058587][ T1959] Oops: Oops: 0003 [#1] PREEMPT SMP KASAN
[ 416.059414][ T1959] CPU: 1 UID: 65534 PID: 1959 Comm: trinity-c3 Not tainted 6.12.0-rc1-00004-ge65dbb5c9051 #1 d7a38916ac9252f968706afc2c77f70fbdabe689
[ 416.061328][ T1959] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.2-debian-1.16.2-1 04/01/2014
[ 416.062850][ T1959] RIP: 0010:fput (arch/x86/include/asm/atomic64_64.h:61 include/linux/atomic/atomic-arch-fallback.h:4404 include/linux/atomic/atomic-long.h:1571 include/linux/atomic/atomic-instrumented.h:4540 fs/file_table.c:482)
[ 416.063578][ T1959] Code: ff ff 66 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 00 f3 0f 1e fa 55 48 89 e5 41 55 41 54 53 48 89 fb be 08 00 00 00 e8 96 c6 f7 ff <f0> 48 ff 0b 0f 85 dd 00 00 00 65 4c 8b 25 04 ff 0e 70 4c 8d 6b 48
All code
========
0: ff (bad)
1: ff 66 66 jmp *0x66(%rsi)
4: 2e 0f 1f 84 00 00 00 cs nopl 0x0(%rax,%rax,1)
b: 00 00
d: 0f 1f 00 nopl (%rax)
10: f3 0f 1e fa endbr64
14: 55 push %rbp
15: 48 89 e5 mov %rsp,%rbp
18: 41 55 push %r13
1a: 41 54 push %r12
1c: 53 push %rbx
1d: 48 89 fb mov %rdi,%rbx
20: be 08 00 00 00 mov $0x8,%esi
25: e8 96 c6 f7 ff call 0xfffffffffff7c6c0
2a:* f0 48 ff 0b lock decq (%rbx) <-- trapping instruction
2e: 0f 85 dd 00 00 00 jne 0x111
34: 65 4c 8b 25 04 ff 0e mov %gs:0x700eff04(%rip),%r12 # 0x700eff40
3b: 70
3c: 4c 8d 6b 48 lea 0x48(%rbx),%r13
Code starting with the faulting instruction
===========================================
0: f0 48 ff 0b lock decq (%rbx)
4: 0f 85 dd 00 00 00 jne 0xe7
a: 65 4c 8b 25 04 ff 0e mov %gs:0x700eff04(%rip),%r12 # 0x700eff16
11: 70
12: 4c 8d 6b 48 lea 0x48(%rbx),%r13
[ 416.066250][ T1959] RSP: 0018:ffffc9000299fa70 EFLAGS: 00010246
[ 416.067156][ T1959] RAX: 0000000000000001 RBX: ffffffff8fed9474 RCX: 0000000000000000
[ 416.068377][ T1959] RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000000
[ 416.069091][ T1980] module: module-autoload: duplicate request for module net-pf-12
[ 416.069532][ T1959] RBP: ffffc9000299fa88 R08: 0000000000000000 R09: 0000000000000000
[ 416.069538][ T1959] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000
[ 416.069541][ T1959] R13: fffffffffffffff7 R14: ffffc9000299fb70 R15: dffffc0000000000
[ 416.078460][ T1959] FS: 0000000000000000(0000) GS:ffff8883a8500000(0063) knlGS:00000000f7ef8280
[ 416.079775][ T1959] CS: 0010 DS: 002b ES: 002b CR0: 0000000080050033
[ 416.080740][ T1959] CR2: ffffffff8fed9474 CR3: 0000000120fe6000 CR4: 00000000000406f0
[ 416.081938][ T1959] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 416.083156][ T1959] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[ 416.084359][ T1959] Call Trace:
[ 416.084939][ T1959] <TASK>
[ 416.085461][ T1959] ? show_regs (arch/x86/kernel/dumpstack.c:479)
[ 416.088241][ T1964] module: module-autoload: duplicate request for module net-pf-32
[ 416.089149][ T1959] ? __die (arch/x86/kernel/dumpstack.c:421 arch/x86/kernel/dumpstack.c:434)
[ 416.089165][ T1959] ? __kasan_check_read (mm/kasan/shadow.c:31)
[ 416.089175][ T1959] ? page_fault_oops (arch/x86/mm/fault.c:710)
[ 416.092872][ T1959] ? fput (arch/x86/include/asm/atomic64_64.h:61 include/linux/atomic/atomic-arch-fallback.h:4404 include/linux/atomic/atomic-long.h:1571 include/linux/atomic/atomic-instrumented.h:4540 fs/file_table.c:482)
[ 416.093516][ T1959] ? show_fault_oops (arch/x86/mm/fault.c:643)
[ 416.094304][ T1959] ? fput (arch/x86/include/asm/atomic64_64.h:61 include/linux/atomic/atomic-arch-fallback.h:4404 include/linux/atomic/atomic-long.h:1571 include/linux/atomic/atomic-instrumented.h:4540 fs/file_table.c:482)
[ 416.094957][ T1959] ? search_exception_tables (kernel/extable.c:64)
[ 416.095760][ T1959] ? fixup_exception (arch/x86/mm/extable.c:320)
[ 416.096496][ T1959] ? validate_chain (arch/x86/include/asm/bitops.h:227 arch/x86/include/asm/bitops.h:239 include/asm-generic/bitops/instrumented-non-atomic.h:142 kernel/locking/lockdep.c:228 kernel/locking/lockdep.c:3816 kernel/locking/lockdep.c:3872)
[ 416.097269][ T1959] ? __kasan_check_read (mm/kasan/shadow.c:31)
[ 416.098069][ T1959] ? kernelmode_fixup_or_oops+0x84/0xb0
[ 416.099036][ T1959] ? __kasan_check_read (mm/kasan/shadow.c:31)
[ 416.099822][ T1959] ? __bad_area_nosemaphore (arch/x86/mm/fault.c:828)
[ 416.100680][ T1959] ? __kasan_check_read (mm/kasan/shadow.c:32)
[ 416.101464][ T1959] ? check_prev_add (kernel/locking/lockdep.c:3860)
[ 416.102255][ T1959] ? __kasan_check_read (mm/kasan/shadow.c:31)
[ 416.103036][ T1959] ? __kasan_check_read (mm/kasan/shadow.c:31)
[ 416.103805][ T1959] ? bad_area_nosemaphore (arch/x86/mm/fault.c:835)
[ 416.104574][ T1959] ? do_kern_addr_fault (arch/x86/mm/fault.c:862 arch/x86/mm/fault.c:881)
[ 416.105445][ T1959] ? __kasan_check_read (mm/kasan/shadow.c:31)
[ 416.106242][ T1959] ? do_kern_addr_fault (arch/x86/mm/fault.c:1199)
[ 416.107023][ T1959] ? exc_page_fault (arch/x86/mm/fault.c:1479 arch/x86/mm/fault.c:1539)
[ 416.107781][ T1959] ? asm_exc_page_fault (arch/x86/include/asm/idtentry.h:623)
[ 416.108538][ T1959] ? __kasan_check_read (mm/kasan/shadow.c:31)
[ 416.109332][ T1959] ? fput (arch/x86/include/asm/atomic64_64.h:61 include/linux/atomic/atomic-arch-fallback.h:4404 include/linux/atomic/atomic-long.h:1571 include/linux/atomic/atomic-instrumented.h:4540 fs/file_table.c:482)
[ 416.110003][ T1959] __do_sys_pidfd_send_signal (kernel/signal.c:3968)
[ 416.110881][ T1959] ? copy_siginfo_from_user32 (kernel/signal.c:3898)
[ 416.111737][ T1959] ? __kasan_check_read (mm/kasan/shadow.c:32)
[ 416.112533][ T1959] ? check_prev_add (kernel/locking/lockdep.c:3860)
[ 416.113327][ T1959] __ia32_sys_pidfd_send_signal (kernel/signal.c:3896)
[ 416.115877][ T1959] ? trace_hardirqs_on (kernel/trace/trace_preemptirq.c:63 (discriminator 22))
[ 416.116669][ T1959] ia32_sys_call (arch/x86/entry/syscall_32.c:44)
[ 416.117417][ T1959] __do_fast_syscall_32 (arch/x86/entry/common.c:165 arch/x86/entry/common.c:386)
[ 416.118224][ T1959] ? __lock_acquire (kernel/locking/lockdep.c:5202)
[ 416.119009][ T1959] ? __task_pid_nr_ns (include/linux/rcupdate.h:337 include/linux/rcupdate.h:849 kernel/pid.c:511)
[ 416.119778][ T1959] ? lock_acquire (include/trace/events/lock.h:24 kernel/locking/lockdep.c:5796)
[ 416.120563][ T1959] ? __task_pid_nr_ns (include/linux/rcupdate.h:337 include/linux/rcupdate.h:849 kernel/pid.c:511)
[ 416.121348][ T1959] ? find_held_lock (kernel/locking/lockdep.c:5315)
[ 416.122114][ T1959] ? __lock_release+0x100/0x530
[ 416.122949][ T1959] ? __task_pid_nr_ns (include/linux/rcupdate.h:347 include/linux/rcupdate.h:880 kernel/pid.c:515)
[ 416.123737][ T1959] ? reacquire_held_locks (kernel/locking/lockdep.c:5476)
[ 416.124570][ T1959] ? __task_pid_nr_ns (include/linux/rcupdate.h:347 include/linux/rcupdate.h:880 kernel/pid.c:515)
[ 416.125359][ T1959] ? syscall_exit_to_user_mode (include/linux/entry-common.h:321 kernel/entry/common.c:207 kernel/entry/common.c:218)
[ 416.126234][ T1959] ? syscall_exit_to_user_mode (kernel/entry/common.c:221)
[ 416.127090][ T1959] ? __do_fast_syscall_32 (arch/x86/entry/common.c:390)
[ 416.127922][ T1959] ? syscall_exit_to_user_mode (kernel/entry/common.c:221)
[ 416.128760][ T1959] ? __do_fast_syscall_32 (arch/x86/entry/common.c:390)
[ 416.129599][ T1959] ? __do_fast_syscall_32 (arch/x86/entry/common.c:390)
[ 416.130419][ T1959] do_fast_syscall_32 (arch/x86/entry/common.c:411)
[ 416.131158][ T1959] do_SYSENTER_32 (arch/x86/entry/common.c:450)
[ 416.131840][ T1959] entry_SYSENTER_compat_after_hwframe (arch/x86/entry/entry_64_compat.S:127)
[ 416.132788][ T1959] RIP: 0023:0xf7efd579
[ 416.133446][ T1959] Code: b8 01 10 06 03 74 b4 01 10 07 03 74 b0 01 10 08 03 74 d8 01 00 00 00 00 00 00 00 00 00 00 00 00 00 51 52 55 89 e5 0f 34 cd 80 <5d> 5a 59 c3 90 90 90 90 8d b4 26 00 00 00 00 8d b4 26 00 00 00 00
All code
========
0: b8 01 10 06 03 mov $0x3061001,%eax
5: 74 b4 je 0xffffffffffffffbb
7: 01 10 add %edx,(%rax)
9: 07 (bad)
a: 03 74 b0 01 add 0x1(%rax,%rsi,4),%esi
e: 10 08 adc %cl,(%rax)
10: 03 74 d8 01 add 0x1(%rax,%rbx,8),%esi
...
20: 00 51 52 add %dl,0x52(%rcx)
23: 55 push %rbp
24:* 89 e5 mov %esp,%ebp <-- trapping instruction
26: 0f 34 sysenter
28: cd 80 int $0x80
2a: 5d pop %rbp
2b: 5a pop %rdx
2c: 59 pop %rcx
2d: c3 ret
2e: 90 nop
2f: 90 nop
30: 90 nop
31: 90 nop
32: 8d b4 26 00 00 00 00 lea 0x0(%rsi,%riz,1),%esi
39: 8d b4 26 00 00 00 00 lea 0x0(%rsi,%riz,1),%esi
Code starting with the faulting instruction
===========================================
0: 5d pop %rbp
1: 5a pop %rdx
2: 59 pop %rcx
3: c3 ret
4: 90 nop
5: 90 nop
6: 90 nop
7: 90 nop
8: 8d b4 26 00 00 00 00 lea 0x0(%rsi,%riz,1),%esi
f: 8d b4 26 00 00 00 00 lea 0x0(%rsi,%riz,1),%esi
The kernel config and materials to reproduce are available at:
https://download.01.org/0day-ci/archive/20241016/202410161634.abca3854-lkp@intel.com
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply
* Re: [PATCH v2 1/3] pidfd: extend pidfd_get_pid() and de-duplicate pid lookup
From: Lorenzo Stoakes @ 2024-10-16 8:22 UTC (permalink / raw)
To: Suren Baghdasaryan
Cc: Christian Brauner, Shuah Khan, Liam R . Howlett, Vlastimil Babka,
pedro.falcato, linux-kselftest, linux-mm, linux-fsdevel,
linux-api, linux-kernel
In-Reply-To: <CAJuCfpFXCKAH+fc6=fg-nVC5tjpGG--Pvk4D2NOn-zdA1LXS=Q@mail.gmail.com>
On Wed, Oct 16, 2024 at 01:16:15AM -0700, Suren Baghdasaryan wrote:
> On Tue, Oct 15, 2024 at 11:05 PM Lorenzo Stoakes
> <lorenzo.stoakes@oracle.com> wrote:
> >
> > On Tue, Oct 15, 2024 at 12:40:41PM -0700, Suren Baghdasaryan wrote:
> > [snip]
> > > > -struct pid *pidfd_get_pid(unsigned int fd, unsigned int *flags)
> > > > +struct pid *__pidfd_get_pid(unsigned int pidfd, bool pin_pid,
> > > > + bool allow_proc, unsigned int *flags,
> > > > + struct fd *fd)
> > > > {
> > > > - struct fd f;
> > > > + struct file *file;
> > > > struct pid *pid;
> > > > + struct fd f = fdget(pidfd);
> > > >
> > > > - f = fdget(fd);
> > > > - if (!fd_file(f))
> > > > + file = fd_file(f);
> > > > + if (!file)
> > > > return ERR_PTR(-EBADF);
> > > >
> > > > - pid = pidfd_pid(fd_file(f));
> > > > - if (!IS_ERR(pid)) {
> > > > - get_pid(pid);
> > > > - *flags = fd_file(f)->f_flags;
> > > > + pid = pidfd_pid(file);
> > > > + /* If we allow opening a pidfd via /proc/<pid>, do so. */
> > > > + if (IS_ERR(pid) && allow_proc)
> > > > + pid = tgid_pidfd_to_pid(file);
> > > > +
> > > > + if (IS_ERR(pid)) {
> > > > + fdput(f);
> > > > + return pid;
> > > > }
> > > >
> > > > - fdput(f);
> > > > + if (pin_pid)
> > > > + get_pid(pid);
> > > > + else
> > > > + WARN_ON_ONCE(!fd); /* Nothing to keep pid/pidfd around? */
> > > > +
> > > > + if (flags)
> > > > + *flags = file->f_flags;
> > > > +
> > > > + /*
> > > > + * If the user provides an fd output then it will handle decrementing
> > > > + * its reference counter.
> > > > + */
> > > > + if (fd)
> > > > + *fd = f;
> > > > + else
> > > > + /* Otherwise we release it. */
> > > > + fdput(f);
> > > > +
> > > > return pid;
> > > > }
> > >
> > > There is an EXPORT_SYMBOL_GPL(pidfd_get_pid) right after this line. It
> > > should also be changed to EXPORT_SYMBOL_GPL(__pidfd_get_pid),
> > > otherwise __pidfd_get_pid() will not be exported. A module calling
> > > pidfd_get_pid() now inlined in the header file will try to call
> > > __pidfd_get_pid() and will have trouble resolving this symbol.
> >
> > Hmm hang on not there isn't? I don't see that anywhere?
>
> Doh! Sorry, I didn't realize the export was an out-of-tree Android
> change. Never mind...
No probs :P just glad I didn't miss something in this series!
Hey maybe a motivation to upstream some of this? ;)
>
> >
> > [snip]
^ permalink raw reply
* Re: [PATCH v2 1/3] pidfd: extend pidfd_get_pid() and de-duplicate pid lookup
From: Suren Baghdasaryan @ 2024-10-16 8:16 UTC (permalink / raw)
To: Lorenzo Stoakes
Cc: Christian Brauner, Shuah Khan, Liam R . Howlett, Vlastimil Babka,
pedro.falcato, linux-kselftest, linux-mm, linux-fsdevel,
linux-api, linux-kernel
In-Reply-To: <8c75620d-8920-4c19-8ebc-0f2b056d49fa@lucifer.local>
On Tue, Oct 15, 2024 at 11:05 PM Lorenzo Stoakes
<lorenzo.stoakes@oracle.com> wrote:
>
> On Tue, Oct 15, 2024 at 12:40:41PM -0700, Suren Baghdasaryan wrote:
> [snip]
> > > -struct pid *pidfd_get_pid(unsigned int fd, unsigned int *flags)
> > > +struct pid *__pidfd_get_pid(unsigned int pidfd, bool pin_pid,
> > > + bool allow_proc, unsigned int *flags,
> > > + struct fd *fd)
> > > {
> > > - struct fd f;
> > > + struct file *file;
> > > struct pid *pid;
> > > + struct fd f = fdget(pidfd);
> > >
> > > - f = fdget(fd);
> > > - if (!fd_file(f))
> > > + file = fd_file(f);
> > > + if (!file)
> > > return ERR_PTR(-EBADF);
> > >
> > > - pid = pidfd_pid(fd_file(f));
> > > - if (!IS_ERR(pid)) {
> > > - get_pid(pid);
> > > - *flags = fd_file(f)->f_flags;
> > > + pid = pidfd_pid(file);
> > > + /* If we allow opening a pidfd via /proc/<pid>, do so. */
> > > + if (IS_ERR(pid) && allow_proc)
> > > + pid = tgid_pidfd_to_pid(file);
> > > +
> > > + if (IS_ERR(pid)) {
> > > + fdput(f);
> > > + return pid;
> > > }
> > >
> > > - fdput(f);
> > > + if (pin_pid)
> > > + get_pid(pid);
> > > + else
> > > + WARN_ON_ONCE(!fd); /* Nothing to keep pid/pidfd around? */
> > > +
> > > + if (flags)
> > > + *flags = file->f_flags;
> > > +
> > > + /*
> > > + * If the user provides an fd output then it will handle decrementing
> > > + * its reference counter.
> > > + */
> > > + if (fd)
> > > + *fd = f;
> > > + else
> > > + /* Otherwise we release it. */
> > > + fdput(f);
> > > +
> > > return pid;
> > > }
> >
> > There is an EXPORT_SYMBOL_GPL(pidfd_get_pid) right after this line. It
> > should also be changed to EXPORT_SYMBOL_GPL(__pidfd_get_pid),
> > otherwise __pidfd_get_pid() will not be exported. A module calling
> > pidfd_get_pid() now inlined in the header file will try to call
> > __pidfd_get_pid() and will have trouble resolving this symbol.
>
> Hmm hang on not there isn't? I don't see that anywhere?
Doh! Sorry, I didn't realize the export was an out-of-tree Android
change. Never mind...
>
> [snip]
^ permalink raw reply
* Re: [PATCH v2 1/3] pidfd: extend pidfd_get_pid() and de-duplicate pid lookup
From: Lorenzo Stoakes @ 2024-10-16 6:05 UTC (permalink / raw)
To: Suren Baghdasaryan
Cc: Christian Brauner, Shuah Khan, Liam R . Howlett, Vlastimil Babka,
pedro.falcato, linux-kselftest, linux-mm, linux-fsdevel,
linux-api, linux-kernel
In-Reply-To: <CAJuCfpHLGyrBWZ9JyJ5FdJQtGO1-tuUqHawjKX_mtwnAhSY6Ow@mail.gmail.com>
On Tue, Oct 15, 2024 at 12:40:41PM -0700, Suren Baghdasaryan wrote:
[snip]
> > -struct pid *pidfd_get_pid(unsigned int fd, unsigned int *flags)
> > +struct pid *__pidfd_get_pid(unsigned int pidfd, bool pin_pid,
> > + bool allow_proc, unsigned int *flags,
> > + struct fd *fd)
> > {
> > - struct fd f;
> > + struct file *file;
> > struct pid *pid;
> > + struct fd f = fdget(pidfd);
> >
> > - f = fdget(fd);
> > - if (!fd_file(f))
> > + file = fd_file(f);
> > + if (!file)
> > return ERR_PTR(-EBADF);
> >
> > - pid = pidfd_pid(fd_file(f));
> > - if (!IS_ERR(pid)) {
> > - get_pid(pid);
> > - *flags = fd_file(f)->f_flags;
> > + pid = pidfd_pid(file);
> > + /* If we allow opening a pidfd via /proc/<pid>, do so. */
> > + if (IS_ERR(pid) && allow_proc)
> > + pid = tgid_pidfd_to_pid(file);
> > +
> > + if (IS_ERR(pid)) {
> > + fdput(f);
> > + return pid;
> > }
> >
> > - fdput(f);
> > + if (pin_pid)
> > + get_pid(pid);
> > + else
> > + WARN_ON_ONCE(!fd); /* Nothing to keep pid/pidfd around? */
> > +
> > + if (flags)
> > + *flags = file->f_flags;
> > +
> > + /*
> > + * If the user provides an fd output then it will handle decrementing
> > + * its reference counter.
> > + */
> > + if (fd)
> > + *fd = f;
> > + else
> > + /* Otherwise we release it. */
> > + fdput(f);
> > +
> > return pid;
> > }
>
> There is an EXPORT_SYMBOL_GPL(pidfd_get_pid) right after this line. It
> should also be changed to EXPORT_SYMBOL_GPL(__pidfd_get_pid),
> otherwise __pidfd_get_pid() will not be exported. A module calling
> pidfd_get_pid() now inlined in the header file will try to call
> __pidfd_get_pid() and will have trouble resolving this symbol.
Hmm hang on not there isn't? I don't see that anywhere?
[snip]
^ permalink raw reply
* Re: [PATCH v2 1/3] pidfd: extend pidfd_get_pid() and de-duplicate pid lookup
From: Suren Baghdasaryan @ 2024-10-15 19:40 UTC (permalink / raw)
To: Lorenzo Stoakes
Cc: Christian Brauner, Shuah Khan, Liam R . Howlett, Vlastimil Babka,
pedro.falcato, linux-kselftest, linux-mm, linux-fsdevel,
linux-api, linux-kernel
In-Reply-To: <8e7edaf2f648fb01a71def749f17f76c0502dee1.1728643714.git.lorenzo.stoakes@oracle.com>
On Fri, Oct 11, 2024 at 4:06 AM Lorenzo Stoakes
<lorenzo.stoakes@oracle.com> wrote:
>
> The means by which a pid is determined from a pidfd is duplicated, with
> some callers holding a reference to the (pid)fd, and others explicitly
> pinning the pid.
>
> Introduce __pidfd_get_pid() which abstracts both approaches and provide
> optional output parameters for file->f_flags and the fd (the latter of
> which, if provided, prevents the function from decrementing the fd's
> refernce count).
>
> Additionally, allow the ability to open a pidfd by opening a /proc/<pid>
> directory, utilised by the pidfd_send_signal() system call, providing a
> pidfd_get_pid_proc() helper function to do so.
>
> Doing this allows us to eliminate open-coded pidfd pid lookup and to
> consistently handle this in one place.
>
> This lays the groundwork for a subsequent patch which adds a new sentinel
> pidfd to explicitly reference the current process (i.e. thread group
> leader) without the need for a pidfd.
>
> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> ---
> include/linux/pid.h | 42 +++++++++++++++++++++++++++++++-
> kernel/pid.c | 58 ++++++++++++++++++++++++++++++---------------
> kernel/signal.c | 22 ++++-------------
> 3 files changed, 84 insertions(+), 38 deletions(-)
>
> diff --git a/include/linux/pid.h b/include/linux/pid.h
> index a3aad9b4074c..68b02eab7509 100644
> --- a/include/linux/pid.h
> +++ b/include/linux/pid.h
> @@ -2,6 +2,7 @@
> #ifndef _LINUX_PID_H
> #define _LINUX_PID_H
>
> +#include <linux/file.h>
> #include <linux/pid_types.h>
> #include <linux/rculist.h>
> #include <linux/rcupdate.h>
> @@ -72,8 +73,47 @@ extern struct pid init_struct_pid;
>
> struct file;
>
> +
> +/**
> + * __pidfd_get_pid() - Retrieve a pid associated with the specified pidfd.
> + *
> + * @pidfd: The pidfd whose pid we want, or the fd of a /proc/<pid> file if
> + * @alloc_proc is also set.
> + * @pin_pid: If set, then the reference counter of the returned pid is
> + * incremented. If not set, then @fd should be provided to pin the
> + * pidfd.
> + * @allow_proc: If set, then an fd of a /proc/<pid> file can be passed instead
> + * of a pidfd, and this will be used to determine the pid.
> + * @flags: Output variable, if non-NULL, then the file->f_flags of the
> + * pidfd will be set here.
> + * @fd: Output variable, if non-NULL, then the pidfd reference will
> + * remain elevated and the caller will need to decrement it
> + * themselves.
> + *
> + * Returns: If successful, the pid associated with the pidfd, otherwise an
> + * error.
> + */
> +struct pid *__pidfd_get_pid(unsigned int pidfd, bool pin_pid,
> + bool allow_proc, unsigned int *flags,
> + struct fd *fd);
> +
> +static inline struct pid *pidfd_get_pid(unsigned int pidfd, unsigned int *flags)
> +{
> + return __pidfd_get_pid(pidfd, /* pin_pid = */ true,
> + /* allow_proc = */ false,
> + flags, /* fd = */ NULL);
> +}
> +
> +static inline struct pid *pidfd_to_pid_proc(unsigned int pidfd,
> + unsigned int *flags,
> + struct fd *fd)
> +{
> + return __pidfd_get_pid(pidfd, /* pin_pid = */ false,
> + /* allow_proc = */ true,
> + flags, fd);
> +}
> +
> struct pid *pidfd_pid(const struct file *file);
> -struct pid *pidfd_get_pid(unsigned int fd, unsigned int *flags);
> struct task_struct *pidfd_get_task(int pidfd, unsigned int *flags);
> int pidfd_prepare(struct pid *pid, unsigned int flags, struct file **ret);
> void do_notify_pidfd(struct task_struct *task);
> diff --git a/kernel/pid.c b/kernel/pid.c
> index 2715afb77eab..25cc1c36a1b1 100644
> --- a/kernel/pid.c
> +++ b/kernel/pid.c
> @@ -36,6 +36,7 @@
> #include <linux/pid_namespace.h>
> #include <linux/init_task.h>
> #include <linux/syscalls.h>
> +#include <linux/proc_fs.h>
> #include <linux/proc_ns.h>
> #include <linux/refcount.h>
> #include <linux/anon_inodes.h>
> @@ -534,22 +535,46 @@ struct pid *find_ge_pid(int nr, struct pid_namespace *ns)
> }
> EXPORT_SYMBOL_GPL(find_ge_pid);
>
> -struct pid *pidfd_get_pid(unsigned int fd, unsigned int *flags)
> +struct pid *__pidfd_get_pid(unsigned int pidfd, bool pin_pid,
> + bool allow_proc, unsigned int *flags,
> + struct fd *fd)
> {
> - struct fd f;
> + struct file *file;
> struct pid *pid;
> + struct fd f = fdget(pidfd);
>
> - f = fdget(fd);
> - if (!fd_file(f))
> + file = fd_file(f);
> + if (!file)
> return ERR_PTR(-EBADF);
>
> - pid = pidfd_pid(fd_file(f));
> - if (!IS_ERR(pid)) {
> - get_pid(pid);
> - *flags = fd_file(f)->f_flags;
> + pid = pidfd_pid(file);
> + /* If we allow opening a pidfd via /proc/<pid>, do so. */
> + if (IS_ERR(pid) && allow_proc)
> + pid = tgid_pidfd_to_pid(file);
> +
> + if (IS_ERR(pid)) {
> + fdput(f);
> + return pid;
> }
>
> - fdput(f);
> + if (pin_pid)
> + get_pid(pid);
> + else
> + WARN_ON_ONCE(!fd); /* Nothing to keep pid/pidfd around? */
> +
> + if (flags)
> + *flags = file->f_flags;
> +
> + /*
> + * If the user provides an fd output then it will handle decrementing
> + * its reference counter.
> + */
> + if (fd)
> + *fd = f;
> + else
> + /* Otherwise we release it. */
> + fdput(f);
> +
> return pid;
> }
There is an EXPORT_SYMBOL_GPL(pidfd_get_pid) right after this line. It
should also be changed to EXPORT_SYMBOL_GPL(__pidfd_get_pid),
otherwise __pidfd_get_pid() will not be exported. A module calling
pidfd_get_pid() now inlined in the header file will try to call
__pidfd_get_pid() and will have trouble resolving this symbol.
>
> @@ -747,23 +772,18 @@ SYSCALL_DEFINE3(pidfd_getfd, int, pidfd, int, fd,
> unsigned int, flags)
> {
> struct pid *pid;
> - struct fd f;
> int ret;
>
> /* flags is currently unused - make sure it's unset */
> if (flags)
> return -EINVAL;
>
> - f = fdget(pidfd);
> - if (!fd_file(f))
> - return -EBADF;
> -
> - pid = pidfd_pid(fd_file(f));
> + pid = pidfd_get_pid(pidfd, NULL);
> if (IS_ERR(pid))
> - ret = PTR_ERR(pid);
> - else
> - ret = pidfd_getfd(pid, fd);
> + return PTR_ERR(pid);
>
> - fdput(f);
> + ret = pidfd_getfd(pid, fd);
> +
> + put_pid(pid);
> return ret;
> }
> diff --git a/kernel/signal.c b/kernel/signal.c
> index 4344860ffcac..868bfa674c62 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -3875,17 +3875,6 @@ static int copy_siginfo_from_user_any(kernel_siginfo_t *kinfo,
> return copy_siginfo_from_user(kinfo, info);
> }
>
> -static struct pid *pidfd_to_pid(const struct file *file)
> -{
> - struct pid *pid;
> -
> - pid = pidfd_pid(file);
> - if (!IS_ERR(pid))
> - return pid;
> -
> - return tgid_pidfd_to_pid(file);
> -}
> -
> #define PIDFD_SEND_SIGNAL_FLAGS \
> (PIDFD_SIGNAL_THREAD | PIDFD_SIGNAL_THREAD_GROUP | \
> PIDFD_SIGNAL_PROCESS_GROUP)
> @@ -3908,10 +3897,11 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig,
> siginfo_t __user *, info, unsigned int, flags)
> {
> int ret;
> - struct fd f;
> struct pid *pid;
> kernel_siginfo_t kinfo;
> enum pid_type type;
> + unsigned int f_flags;
> + struct fd f;
>
> /* Enforce flags be set to 0 until we add an extension. */
> if (flags & ~PIDFD_SEND_SIGNAL_FLAGS)
> @@ -3921,12 +3911,8 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig,
> if (hweight32(flags & PIDFD_SEND_SIGNAL_FLAGS) > 1)
> return -EINVAL;
>
> - f = fdget(pidfd);
> - if (!fd_file(f))
> - return -EBADF;
> -
> /* Is this a pidfd? */
> - pid = pidfd_to_pid(fd_file(f));
> + pid = pidfd_to_pid_proc(pidfd, &f_flags, &f);
> if (IS_ERR(pid)) {
> ret = PTR_ERR(pid);
> goto err;
> @@ -3939,7 +3925,7 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig,
> switch (flags) {
> case 0:
> /* Infer scope from the type of pidfd. */
> - if (fd_file(f)->f_flags & PIDFD_THREAD)
> + if (f_flags & PIDFD_THREAD)
> type = PIDTYPE_PID;
> else
> type = PIDTYPE_TGID;
> --
> 2.46.2
>
^ permalink raw reply
* Re: [f2fs-dev] [PATCH v5] f2fs: introduce device aliasing file
From: Jaegeuk Kim @ 2024-10-15 16:56 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Daeho Jeong, Daeho Jeong, kernel-team, linux-kernel,
linux-f2fs-devel, linux-fsdevel, linux-api
In-Reply-To: <Zw34CMxJB-THlGW0@infradead.org>
On 10/14, Christoph Hellwig wrote:
> On Mon, Oct 14, 2024 at 04:42:07PM +0000, Jaegeuk Kim wrote:
> > >
> > > Plz, refer to this patch and the description there.
> > >
> > > https://git.kernel.org/pub/scm/linux/kernel/git/jaegeuk/f2fs-tools.git/commit/?h=dev-test&id=8cc4e257ec20bee207bb034d5ac406e1ab31eaea
> >
> > Also, I added this in the description.
> >
> > ---
> > For example,
> > "mkfs.f2fs -c /dev/block/test@test_alias /dev/block/main" gives
> > a file $root/test_alias which carves out /dev/block/test partition.
>
> What partition?
>
> So mkfs.f2fs adds additional devices based on the man page.
>
> So the above creates a file system with two devices, but the second
> device is not added to the general space pool, but mapped to a specific
> file? How does this file work. I guess it can't be unlinked and
> renamed. It probably also can't be truncated and hole punched,
> or use insert/collapse range. How does the user find out about this
> magic file? What is the use case? Are the exact semantics documented
> somewhere?
Let me ask for putting some design in Documentation. Just for a quick reference,
the use-case looks like:
# ls /dev/vd*
/dev/vdb (32GB) /dev/vdc (32GB)
# mkfs.ext4 /dev/vdc
# mkfs.f2fs -c /dev/vdc@vdc.file /dev/vdb
# mount /dev/vdb /mnt/f2fs
# ls -l /mnt/f2fs
vdc.file
# df -h
/dev/vdb 64G 33G 32G 52% /mnt/f2fs
# mount -o loop /dev/vdc /mnt/ext4
# df -h
/dev/vdb 64G 33G 32G 52% /mnt/f2fs
/dev/loop7 32G 24K 30G 1% /mnt/ext4
# umount /mnt/ext4
# f2fs_io getflags /mnt/f2fs/vdc.file
get a flag on /mnt/f2fs/vdc.file ret=0, flags=nocow(pinned),immutable
# f2fs_io setflags noimmutable /mnt/f2fs/vdc.file
get a flag on noimmutable ret=0, flags=800010
set a flag on /mnt/f2fs/vdc.file ret=0, flags=noimmutable
# rm /mnt/f2fs/vdc.file
# df -h
/dev/vdb 64G 753M 64G 2% /mnt/f2fs
So, key idea is, user can do any file operations on /dev/vdc, and
reclaim the space after the use, while the space is counted as /data.
That doesn't require modifying partition size and filesystem format.
^ permalink raw reply
* Re: [f2fs-dev] [PATCH v5] f2fs: introduce device aliasing file
From: Christoph Hellwig @ 2024-10-15 5:05 UTC (permalink / raw)
To: Jaegeuk Kim
Cc: Daeho Jeong, Christoph Hellwig, Daeho Jeong, kernel-team,
linux-kernel, linux-f2fs-devel, linux-fsdevel, linux-api
In-Reply-To: <Zw1J30Fn48uYCwK7@google.com>
On Mon, Oct 14, 2024 at 04:42:07PM +0000, Jaegeuk Kim wrote:
> >
> > Plz, refer to this patch and the description there.
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/jaegeuk/f2fs-tools.git/commit/?h=dev-test&id=8cc4e257ec20bee207bb034d5ac406e1ab31eaea
>
> Also, I added this in the description.
>
> ---
> For example,
> "mkfs.f2fs -c /dev/block/test@test_alias /dev/block/main" gives
> a file $root/test_alias which carves out /dev/block/test partition.
What partition?
So mkfs.f2fs adds additional devices based on the man page.
So the above creates a file system with two devices, but the second
device is not added to the general space pool, but mapped to a specific
file? How does this file work. I guess it can't be unlinked and
renamed. It probably also can't be truncated and hole punched,
or use insert/collapse range. How does the user find out about this
magic file? What is the use case? Are the exact semantics documented
somewhere?
^ permalink raw reply
* Re: [PATCH v20 2/6] security: Add EXEC_RESTRICT_FILE and EXEC_DENY_INTERACTIVE securebits
From: sergeh @ 2024-10-15 3:26 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Al Viro, Christian Brauner, Kees Cook, Linus Torvalds, Paul Moore,
Serge Hallyn, Theodore Ts'o, Adhemerval Zanella Netto,
Alejandro Colomar, Aleksa Sarai, Andrew Morton, Andy Lutomirski,
Arnd Bergmann, Casey Schaufler, Christian Heimes, Dmitry Vyukov,
Elliott Hughes, Eric Biggers, Eric Chiang, Fan Wu, Florian Weimer,
Geert Uytterhoeven, James Morris, Jan Kara, Jann Horn, Jeff Xu,
Jonathan Corbet, Jordan R Abrahams, Lakshmi Ramasubramanian,
Luca Boccassi, Luis Chamberlain, Madhavan T . Venkataraman,
Matt Bobrowski, Matthew Garrett, Matthew Wilcox, Miklos Szeredi,
Mimi Zohar, Nicolas Bouchinet, Scott Shell, Shuah Khan,
Stephen Rothwell, Steve Dower, Steve Grubb, Thibaut Sautereau,
Vincent Strubel, Xiaoming Ni, Yin Fengwei, kernel-hardening,
linux-api, linux-fsdevel, linux-integrity, linux-kernel,
linux-security-module, Andy Lutomirski
In-Reply-To: <20241011184422.977903-3-mic@digikod.net>
On Fri, Oct 11, 2024 at 08:44:18PM +0200, Mickaël Salaün wrote:
> The new SECBIT_EXEC_RESTRICT_FILE, SECBIT_EXEC_DENY_INTERACTIVE, and
> their *_LOCKED counterparts are designed to be set by processes setting
> up an execution environment, such as a user session, a container, or a
> security sandbox. Unlike other securebits, these ones can be set by
> unprivileged processes. Like seccomp filters or Landlock domains, the
> securebits are inherited across processes.
>
> When SECBIT_EXEC_RESTRICT_FILE is set, programs interpreting code should
> control executable resources according to execveat(2) + AT_CHECK (see
> previous commit).
>
> When SECBIT_EXEC_DENY_INTERACTIVE is set, a process should deny
> execution of user interactive commands (which excludes executable
> regular files).
>
> Being able to configure each of these securebits enables system
> administrators or owner of image containers to gradually validate the
> related changes and to identify potential issues (e.g. with interpreter
> or audit logs).
>
> It should be noted that unlike other security bits, the
> SECBIT_EXEC_RESTRICT_FILE and SECBIT_EXEC_DENY_INTERACTIVE bits are
> dedicated to user space willing to restrict itself. Because of that,
> they only make sense in the context of a trusted environment (e.g.
> sandbox, container, user session, full system) where the process
> changing its behavior (according to these bits) and all its parent
> processes are trusted. Otherwise, any parent process could just execute
> its own malicious code (interpreting a script or not), or even enforce a
> seccomp filter to mask these bits.
>
> Such a secure environment can be achieved with an appropriate access
> control (e.g. mount's noexec option, file access rights, LSM policy) and
> an enlighten ld.so checking that libraries are allowed for execution
> e.g., to protect against illegitimate use of LD_PRELOAD.
>
> Ptrace restrictions according to these securebits would not make sense
> because of the processes' trust assumption.
>
> Scripts may need some changes to deal with untrusted data (e.g. stdin,
> environment variables), but that is outside the scope of the kernel.
>
> See chromeOS's documentation about script execution control and the
> related threat model:
> https://www.chromium.org/chromium-os/developer-library/guides/security/noexec-shell-scripts/
>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Cc: Andy Lutomirski <luto@amacapital.net>
> Cc: Christian Brauner <brauner@kernel.org>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Paul Moore <paul@paul-moore.com>
> Cc: Serge Hallyn <serge@hallyn.com>
Reviewed-by: Serge Hallyn <serge@hallyn.com>
thanks,
-serge
> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> Link: https://lore.kernel.org/r/20241011184422.977903-3-mic@digikod.net
> ---
>
> Changes since v19:
> * Replace SECBIT_SHOULD_EXEC_CHECK and SECBIT_SHOULD_EXEC_RESTRICT with
> SECBIT_EXEC_RESTRICT_FILE and SECBIT_EXEC_DENY_INTERACTIVE:
> https://lore.kernel.org/all/20240710.eiKohpa4Phai@digikod.net/
> * Remove the ptrace restrictions, suggested by Andy.
> * Improve documentation according to the discussion with Jeff.
>
> New design since v18:
> https://lore.kernel.org/r/20220104155024.48023-3-mic@digikod.net
> ---
> include/uapi/linux/securebits.h | 113 +++++++++++++++++++++++++++++++-
> security/commoncap.c | 29 ++++++--
> 2 files changed, 135 insertions(+), 7 deletions(-)
>
> diff --git a/include/uapi/linux/securebits.h b/include/uapi/linux/securebits.h
> index d6d98877ff1a..351b6ecefc76 100644
> --- a/include/uapi/linux/securebits.h
> +++ b/include/uapi/linux/securebits.h
> @@ -52,10 +52,121 @@
> #define SECBIT_NO_CAP_AMBIENT_RAISE_LOCKED \
> (issecure_mask(SECURE_NO_CAP_AMBIENT_RAISE_LOCKED))
>
> +/*
> + * The SECBIT_EXEC_RESTRICT_FILE and SECBIT_EXEC_DENY_INTERACTIVE securebits
> + * are intended for script interpreters and dynamic linkers to enforce a
> + * consistent execution security policy handled by the kernel.
> + *
> + * Whether an interpreter should check these securebits or not depends on the
> + * security risk of running malicious scripts with respect to the execution
> + * environment, and whether the kernel can check if a script is trustworthy or
> + * not. For instance, Python scripts running on a server can use arbitrary
> + * syscalls and access arbitrary files. Such interpreters should then be
> + * enlighten to use these securebits and let users define their security
> + * policy. However, a JavaScript engine running in a web browser should
> + * already be sandboxed and then should not be able to harm the user's
> + * environment.
> + *
> + * When SECBIT_EXEC_RESTRICT_FILE is set, a process should only interpret or
> + * execute a file if a call to execveat(2) with the related file descriptor and
> + * the AT_CHECK flag succeed.
> + *
> + * This secure bit may be set by user session managers, service managers,
> + * container runtimes, sandboxer tools... Except for test environments, the
> + * related SECBIT_EXEC_RESTRICT_FILE_LOCKED bit should also be set.
> + *
> + * Programs should only enforce consistent restrictions according to the
> + * securebits but without relying on any other user-controlled configuration.
> + * Indeed, the use case for these securebits is to only trust executable code
> + * vetted by the system configuration (through the kernel), so we should be
> + * careful to not let untrusted users control this configuration.
> + *
> + * However, script interpreters may still use user configuration such as
> + * environment variables as long as it is not a way to disable the securebits
> + * checks. For instance, the PATH and LD_PRELOAD variables can be set by a
> + * script's caller. Changing these variables may lead to unintended code
> + * executions, but only from vetted executable programs, which is OK. For this
> + * to make sense, the system should provide a consistent security policy to
> + * avoid arbitrary code execution e.g., by enforcing a write xor execute
> + * policy.
> + *
> + * SECBIT_EXEC_RESTRICT_FILE is complementary and should also be checked.
> + */
> +#define SECURE_EXEC_RESTRICT_FILE 8
> +#define SECURE_EXEC_RESTRICT_FILE_LOCKED 9 /* make bit-8 immutable */
> +
> +#define SECBIT_EXEC_RESTRICT_FILE (issecure_mask(SECURE_EXEC_RESTRICT_FILE))
> +#define SECBIT_EXEC_RESTRICT_FILE_LOCKED \
> + (issecure_mask(SECURE_EXEC_RESTRICT_FILE_LOCKED))
> +
> +/*
> + * When SECBIT_EXEC_DENY_INTERACTIVE is set, a process should never interpret
> + * interactive user commands (e.g. scripts). However, if such commands are
> + * passed through a file descriptor (e.g. stdin), its content should be
> + * interpreted if a call to execveat(2) with the related file descriptor and
> + * the AT_CHECK flag succeed.
> + *
> + * For instance, script interpreters called with a script snippet as argument
> + * should always deny such execution if SECBIT_EXEC_DENY_INTERACTIVE is set.
> + *
> + * This secure bit may be set by user session managers, service managers,
> + * container runtimes, sandboxer tools... Except for test environments, the
> + * related SECBIT_EXEC_DENY_INTERACTIVE_LOCKED bit should also be set.
> + *
> + * See the SECBIT_EXEC_RESTRICT_FILE documentation.
> + *
> + * Here is the expected behavior for a script interpreter according to
> + * combination of any exec securebits:
> + *
> + * 1. SECURE_EXEC_RESTRICT_FILE=0 SECURE_EXEC_DENY_INTERACTIVE=0 (default)
> + * Always interpret scripts, and allow arbitrary user commands.
> + * => No threat, everyone and everything is trusted, but we can get ahead of
> + * potential issues thanks to the call to execveat with AT_CHECK which
> + * should always be performed but ignored by the script interpreter.
> + * Indeed, this check is still important to enable systems administrators
> + * to verify requests (e.g. with audit) and prepare for migration to a
> + * secure mode.
> + *
> + * 2. SECURE_EXEC_RESTRICT_FILE=1 SECURE_EXEC_DENY_INTERACTIVE=0
> + * Deny script interpretation if they are not executable, but allow
> + * arbitrary user commands.
> + * => The threat is (potential) malicious scripts run by trusted (and not
> + * fooled) users. That can protect against unintended script executions
> + * (e.g. sh /tmp/*.sh). This makes sense for (semi-restricted) user
> + * sessions.
> + *
> + * 3. SECURE_EXEC_RESTRICT_FILE=0 SECURE_EXEC_DENY_INTERACTIVE=1
> + * Always interpret scripts, but deny arbitrary user commands.
> + * => This use case may be useful for secure services (i.e. without
> + * interactive user session) where scripts' integrity is verified (e.g.
> + * with IMA/EVM or dm-verity/IPE) but where access rights might not be
> + * ready yet. Indeed, arbitrary interactive commands would be much more
> + * difficult to check.
> + *
> + * 4. SECURE_EXEC_RESTRICT_FILE=1 SECURE_EXEC_DENY_INTERACTIVE=1
> + * Deny script interpretation if they are not executable, and also deny
> + * any arbitrary user commands.
> + * => The threat is malicious scripts run by untrusted users (but trusted
> + * code). This makes sense for system services that may only execute
> + * trusted scripts.
> + */
> +#define SECURE_EXEC_DENY_INTERACTIVE 10
> +#define SECURE_EXEC_DENY_INTERACTIVE_LOCKED 11 /* make bit-10 immutable */
> +
> +#define SECBIT_EXEC_DENY_INTERACTIVE \
> + (issecure_mask(SECURE_EXEC_DENY_INTERACTIVE))
> +#define SECBIT_EXEC_DENY_INTERACTIVE_LOCKED \
> + (issecure_mask(SECURE_EXEC_DENY_INTERACTIVE_LOCKED))
> +
> #define SECURE_ALL_BITS (issecure_mask(SECURE_NOROOT) | \
> issecure_mask(SECURE_NO_SETUID_FIXUP) | \
> issecure_mask(SECURE_KEEP_CAPS) | \
> - issecure_mask(SECURE_NO_CAP_AMBIENT_RAISE))
> + issecure_mask(SECURE_NO_CAP_AMBIENT_RAISE) | \
> + issecure_mask(SECURE_EXEC_RESTRICT_FILE) | \
> + issecure_mask(SECURE_EXEC_DENY_INTERACTIVE))
> #define SECURE_ALL_LOCKS (SECURE_ALL_BITS << 1)
>
> +#define SECURE_ALL_UNPRIVILEGED (issecure_mask(SECURE_EXEC_RESTRICT_FILE) | \
> + issecure_mask(SECURE_EXEC_DENY_INTERACTIVE))
> +
> #endif /* _UAPI_LINUX_SECUREBITS_H */
> diff --git a/security/commoncap.c b/security/commoncap.c
> index cefad323a0b1..52ea01acb453 100644
> --- a/security/commoncap.c
> +++ b/security/commoncap.c
> @@ -1302,21 +1302,38 @@ int cap_task_prctl(int option, unsigned long arg2, unsigned long arg3,
> & (old->securebits ^ arg2)) /*[1]*/
> || ((old->securebits & SECURE_ALL_LOCKS & ~arg2)) /*[2]*/
> || (arg2 & ~(SECURE_ALL_LOCKS | SECURE_ALL_BITS)) /*[3]*/
> - || (cap_capable(current_cred(),
> - current_cred()->user_ns,
> - CAP_SETPCAP,
> - CAP_OPT_NONE) != 0) /*[4]*/
> /*
> * [1] no changing of bits that are locked
> * [2] no unlocking of locks
> * [3] no setting of unsupported bits
> - * [4] doing anything requires privilege (go read about
> - * the "sendmail capabilities bug")
> */
> )
> /* cannot change a locked bit */
> return -EPERM;
>
> + /*
> + * Doing anything requires privilege (go read about the
> + * "sendmail capabilities bug"), except for unprivileged bits.
> + * Indeed, the SECURE_ALL_UNPRIVILEGED bits are not
> + * restrictions enforced by the kernel but by user space on
> + * itself.
> + */
> + if (cap_capable(current_cred(), current_cred()->user_ns,
> + CAP_SETPCAP, CAP_OPT_NONE) != 0) {
> + const unsigned long unpriv_and_locks =
> + SECURE_ALL_UNPRIVILEGED |
> + SECURE_ALL_UNPRIVILEGED << 1;
> + const unsigned long changed = old->securebits ^ arg2;
> +
> + /* For legacy reason, denies non-change. */
> + if (!changed)
> + return -EPERM;
> +
> + /* Denies privileged changes. */
> + if (changed & ~unpriv_and_locks)
> + return -EPERM;
> + }
> +
> new = prepare_creds();
> if (!new)
> return -ENOMEM;
> --
> 2.46.1
>
^ permalink raw reply
* Re: [PATCH v20 1/6] exec: Add a new AT_CHECK flag to execveat(2)
From: sergeh @ 2024-10-15 3:20 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Serge E. Hallyn, Al Viro, Christian Brauner, Kees Cook,
Linus Torvalds, Paul Moore, Theodore Ts'o,
Adhemerval Zanella Netto, Alejandro Colomar, Aleksa Sarai,
Andrew Morton, Andy Lutomirski, Arnd Bergmann, Casey Schaufler,
Christian Heimes, Dmitry Vyukov, Elliott Hughes, Eric Biggers,
Eric Chiang, Fan Wu, Florian Weimer, Geert Uytterhoeven,
James Morris, Jan Kara, Jann Horn, Jeff Xu, Jonathan Corbet,
Jordan R Abrahams, Lakshmi Ramasubramanian, Luca Boccassi,
Luis Chamberlain, Madhavan T . Venkataraman, Matt Bobrowski,
Matthew Garrett, Matthew Wilcox, Miklos Szeredi, Mimi Zohar,
Nicolas Bouchinet, Scott Shell, Shuah Khan, Stephen Rothwell,
Steve Dower, Steve Grubb, Thibaut Sautereau, Vincent Strubel,
Xiaoming Ni, Yin Fengwei, kernel-hardening, linux-api,
linux-fsdevel, linux-integrity, linux-kernel,
linux-security-module
In-Reply-To: <20241014.ke5eeKoo6doh@digikod.net>
On Mon, Oct 14, 2024 at 09:39:52AM +0200, Mickaël Salaün wrote:
> On Sat, Oct 12, 2024 at 10:04:16PM -0500, Serge E. Hallyn wrote:
> > On Fri, Oct 11, 2024 at 08:44:17PM +0200, Mickaël Salaün wrote:
> > > Add a new AT_CHECK flag to execveat(2) to check if a file would be
> >
> > Apologies for both bikeshedding and missing earlier discussions.
> >
> > But AT_CHECK sounds quite generic. How about AT_EXEC_CHECK, or
> > AT_CHECK_EXEC_CREDS? (I would suggest just AT_CHECK_CREDS since
> > it's for use in execveat(2), but as it's an AT_ flag, it's
> > probably worth being more precise).
>
> As Amir pointed out, we need at least to use the AT_EXECVE_CHECK_
> prefix, and I agree with the AT_EXECVE_CHECK name because it's about
> checking the whole execve request, not sepcifically a "creds" part.
Well, not the whole. You are explicitly not checking the validity of the
files.
But ok. With that,
Reviewed-by: Serge Hallyn <sergeh@kernel.org>
thanks,
-serge
^ permalink raw reply
* Re: [PATCH v20 2/6] security: Add EXEC_RESTRICT_FILE and EXEC_DENY_INTERACTIVE securebits
From: sergeh @ 2024-10-15 3:15 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Serge E. Hallyn, Al Viro, Christian Brauner, Kees Cook,
Linus Torvalds, Paul Moore, Theodore Ts'o,
Adhemerval Zanella Netto, Alejandro Colomar, Aleksa Sarai,
Andrew Morton, Andy Lutomirski, Arnd Bergmann, Casey Schaufler,
Christian Heimes, Dmitry Vyukov, Elliott Hughes, Eric Biggers,
Eric Chiang, Fan Wu, Florian Weimer, Geert Uytterhoeven,
James Morris, Jan Kara, Jann Horn, Jeff Xu, Jonathan Corbet,
Jordan R Abrahams, Lakshmi Ramasubramanian, Luca Boccassi,
Luis Chamberlain, Madhavan T . Venkataraman, Matt Bobrowski,
Matthew Garrett, Matthew Wilcox, Miklos Szeredi, Mimi Zohar,
Nicolas Bouchinet, Scott Shell, Shuah Khan, Stephen Rothwell,
Steve Dower, Steve Grubb, Thibaut Sautereau, Vincent Strubel,
Xiaoming Ni, Yin Fengwei, kernel-hardening, linux-api,
linux-fsdevel, linux-integrity, linux-kernel,
linux-security-module, Andy Lutomirski
In-Reply-To: <20241014.jahHeitoo0uo@digikod.net>
On Mon, Oct 14, 2024 at 09:40:34AM +0200, Mickaël Salaün wrote:
> On Sat, Oct 12, 2024 at 09:51:50PM -0500, Serge E. Hallyn wrote:
> > On Fri, Oct 11, 2024 at 08:44:18PM +0200, Mickaël Salaün wrote:
> > > The new SECBIT_EXEC_RESTRICT_FILE, SECBIT_EXEC_DENY_INTERACTIVE, and
> > > their *_LOCKED counterparts are designed to be set by processes setting
> > > up an execution environment, such as a user session, a container, or a
> > > security sandbox. Unlike other securebits, these ones can be set by
> > > unprivileged processes. Like seccomp filters or Landlock domains, the
> > > securebits are inherited across processes.
> > >
> > > When SECBIT_EXEC_RESTRICT_FILE is set, programs interpreting code should
> > > control executable resources according to execveat(2) + AT_CHECK (see
> > > previous commit).
> > >
> > > When SECBIT_EXEC_DENY_INTERACTIVE is set, a process should deny
> > > execution of user interactive commands (which excludes executable
> > > regular files).
> > >
> > > Being able to configure each of these securebits enables system
> > > administrators or owner of image containers to gradually validate the
> > > related changes and to identify potential issues (e.g. with interpreter
> > > or audit logs).
> > >
> > > It should be noted that unlike other security bits, the
> > > SECBIT_EXEC_RESTRICT_FILE and SECBIT_EXEC_DENY_INTERACTIVE bits are
> > > dedicated to user space willing to restrict itself. Because of that,
> > > they only make sense in the context of a trusted environment (e.g.
> > > sandbox, container, user session, full system) where the process
> > > changing its behavior (according to these bits) and all its parent
> > > processes are trusted. Otherwise, any parent process could just execute
> > > its own malicious code (interpreting a script or not), or even enforce a
> > > seccomp filter to mask these bits.
> > >
> > > Such a secure environment can be achieved with an appropriate access
> > > control (e.g. mount's noexec option, file access rights, LSM policy) and
> > > an enlighten ld.so checking that libraries are allowed for execution
> > > e.g., to protect against illegitimate use of LD_PRELOAD.
> > >
> > > Ptrace restrictions according to these securebits would not make sense
> > > because of the processes' trust assumption.
> > >
> > > Scripts may need some changes to deal with untrusted data (e.g. stdin,
> > > environment variables), but that is outside the scope of the kernel.
> > >
> > > See chromeOS's documentation about script execution control and the
> > > related threat model:
> > > https://www.chromium.org/chromium-os/developer-library/guides/security/noexec-shell-scripts/
> > >
> > > Cc: Al Viro <viro@zeniv.linux.org.uk>
> > > Cc: Andy Lutomirski <luto@amacapital.net>
> > > Cc: Christian Brauner <brauner@kernel.org>
> > > Cc: Kees Cook <keescook@chromium.org>
> > > Cc: Paul Moore <paul@paul-moore.com>
> > > Cc: Serge Hallyn <serge@hallyn.com>
> > > Signed-off-by: Mickaël Salaün <mic@digikod.net>
> > > Link: https://lore.kernel.org/r/20241011184422.977903-3-mic@digikod.net
> > > ---
> > >
> > > Changes since v19:
> > > * Replace SECBIT_SHOULD_EXEC_CHECK and SECBIT_SHOULD_EXEC_RESTRICT with
> > > SECBIT_EXEC_RESTRICT_FILE and SECBIT_EXEC_DENY_INTERACTIVE:
> > > https://lore.kernel.org/all/20240710.eiKohpa4Phai@digikod.net/
> > > * Remove the ptrace restrictions, suggested by Andy.
> > > * Improve documentation according to the discussion with Jeff.
> > >
> > > New design since v18:
> > > https://lore.kernel.org/r/20220104155024.48023-3-mic@digikod.net
> > > ---
> > > include/uapi/linux/securebits.h | 113 +++++++++++++++++++++++++++++++-
> > > security/commoncap.c | 29 ++++++--
> > > 2 files changed, 135 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/include/uapi/linux/securebits.h b/include/uapi/linux/securebits.h
> > > index d6d98877ff1a..351b6ecefc76 100644
> > > --- a/include/uapi/linux/securebits.h
> > > +++ b/include/uapi/linux/securebits.h
> > > @@ -52,10 +52,121 @@
> > > #define SECBIT_NO_CAP_AMBIENT_RAISE_LOCKED \
> > > (issecure_mask(SECURE_NO_CAP_AMBIENT_RAISE_LOCKED))
> > >
> > > +/*
> > > + * The SECBIT_EXEC_RESTRICT_FILE and SECBIT_EXEC_DENY_INTERACTIVE securebits
> > > + * are intended for script interpreters and dynamic linkers to enforce a
> > > + * consistent execution security policy handled by the kernel.
> > > + *
> > > + * Whether an interpreter should check these securebits or not depends on the
> > > + * security risk of running malicious scripts with respect to the execution
> > > + * environment, and whether the kernel can check if a script is trustworthy or
> > > + * not. For instance, Python scripts running on a server can use arbitrary
> > > + * syscalls and access arbitrary files. Such interpreters should then be
> > > + * enlighten to use these securebits and let users define their security
> > > + * policy. However, a JavaScript engine running in a web browser should
> > > + * already be sandboxed and then should not be able to harm the user's
> > > + * environment.
> > > + *
> > > + * When SECBIT_EXEC_RESTRICT_FILE is set, a process should only interpret or
> > > + * execute a file if a call to execveat(2) with the related file descriptor and
> > > + * the AT_CHECK flag succeed.
> > > + *
> > > + * This secure bit may be set by user session managers, service managers,
> > > + * container runtimes, sandboxer tools... Except for test environments, the
> > > + * related SECBIT_EXEC_RESTRICT_FILE_LOCKED bit should also be set.
> > > + *
> > > + * Programs should only enforce consistent restrictions according to the
> > > + * securebits but without relying on any other user-controlled configuration.
> > > + * Indeed, the use case for these securebits is to only trust executable code
> > > + * vetted by the system configuration (through the kernel), so we should be
> > > + * careful to not let untrusted users control this configuration.
> > > + *
> > > + * However, script interpreters may still use user configuration such as
> > > + * environment variables as long as it is not a way to disable the securebits
> > > + * checks. For instance, the PATH and LD_PRELOAD variables can be set by a
> > > + * script's caller. Changing these variables may lead to unintended code
> > > + * executions, but only from vetted executable programs, which is OK. For this
> > > + * to make sense, the system should provide a consistent security policy to
> > > + * avoid arbitrary code execution e.g., by enforcing a write xor execute
> > > + * policy.
> > > + *
> > > + * SECBIT_EXEC_RESTRICT_FILE is complementary and should also be checked.
> > > + */
> > > +#define SECURE_EXEC_RESTRICT_FILE 8
> > > +#define SECURE_EXEC_RESTRICT_FILE_LOCKED 9 /* make bit-8 immutable */
> > > +
> > > +#define SECBIT_EXEC_RESTRICT_FILE (issecure_mask(SECURE_EXEC_RESTRICT_FILE))
> > > +#define SECBIT_EXEC_RESTRICT_FILE_LOCKED \
> > > + (issecure_mask(SECURE_EXEC_RESTRICT_FILE_LOCKED))
> > > +
> > > +/*
> > > + * When SECBIT_EXEC_DENY_INTERACTIVE is set, a process should never interpret
> > > + * interactive user commands (e.g. scripts). However, if such commands are
> > > + * passed through a file descriptor (e.g. stdin), its content should be
> > > + * interpreted if a call to execveat(2) with the related file descriptor and
> > > + * the AT_CHECK flag succeed.
> > > + *
> > > + * For instance, script interpreters called with a script snippet as argument
> > > + * should always deny such execution if SECBIT_EXEC_DENY_INTERACTIVE is set.
> > > + *
> > > + * This secure bit may be set by user session managers, service managers,
> > > + * container runtimes, sandboxer tools... Except for test environments, the
> > > + * related SECBIT_EXEC_DENY_INTERACTIVE_LOCKED bit should also be set.
> > > + *
> > > + * See the SECBIT_EXEC_RESTRICT_FILE documentation.
> > > + *
> > > + * Here is the expected behavior for a script interpreter according to
> > > + * combination of any exec securebits:
> > > + *
> > > + * 1. SECURE_EXEC_RESTRICT_FILE=0 SECURE_EXEC_DENY_INTERACTIVE=0 (default)
> > > + * Always interpret scripts, and allow arbitrary user commands.
> > > + * => No threat, everyone and everything is trusted, but we can get ahead of
> > > + * potential issues thanks to the call to execveat with AT_CHECK which
> > > + * should always be performed but ignored by the script interpreter.
> > > + * Indeed, this check is still important to enable systems administrators
> > > + * to verify requests (e.g. with audit) and prepare for migration to a
> > > + * secure mode.
> > > + *
> > > + * 2. SECURE_EXEC_RESTRICT_FILE=1 SECURE_EXEC_DENY_INTERACTIVE=0
> > > + * Deny script interpretation if they are not executable, but allow
> > > + * arbitrary user commands.
> > > + * => The threat is (potential) malicious scripts run by trusted (and not
> > > + * fooled) users. That can protect against unintended script executions
> > > + * (e.g. sh /tmp/*.sh). This makes sense for (semi-restricted) user
> > > + * sessions.
> > > + *
> > > + * 3. SECURE_EXEC_RESTRICT_FILE=0 SECURE_EXEC_DENY_INTERACTIVE=1
> > > + * Always interpret scripts, but deny arbitrary user commands.
> > > + * => This use case may be useful for secure services (i.e. without
> > > + * interactive user session) where scripts' integrity is verified (e.g.
> > > + * with IMA/EVM or dm-verity/IPE) but where access rights might not be
> > > + * ready yet. Indeed, arbitrary interactive commands would be much more
> > > + * difficult to check.
> > > + *
> > > + * 4. SECURE_EXEC_RESTRICT_FILE=1 SECURE_EXEC_DENY_INTERACTIVE=1
> > > + * Deny script interpretation if they are not executable, and also deny
> > > + * any arbitrary user commands.
> > > + * => The threat is malicious scripts run by untrusted users (but trusted
> > > + * code). This makes sense for system services that may only execute
> > > + * trusted scripts.
> > > + */
> > > +#define SECURE_EXEC_DENY_INTERACTIVE 10
> > > +#define SECURE_EXEC_DENY_INTERACTIVE_LOCKED 11 /* make bit-10 immutable */
> > > +
> > > +#define SECBIT_EXEC_DENY_INTERACTIVE \
> > > + (issecure_mask(SECURE_EXEC_DENY_INTERACTIVE))
> > > +#define SECBIT_EXEC_DENY_INTERACTIVE_LOCKED \
> > > + (issecure_mask(SECURE_EXEC_DENY_INTERACTIVE_LOCKED))
> > > +
> > > #define SECURE_ALL_BITS (issecure_mask(SECURE_NOROOT) | \
> > > issecure_mask(SECURE_NO_SETUID_FIXUP) | \
> > > issecure_mask(SECURE_KEEP_CAPS) | \
> > > - issecure_mask(SECURE_NO_CAP_AMBIENT_RAISE))
> > > + issecure_mask(SECURE_NO_CAP_AMBIENT_RAISE) | \
> > > + issecure_mask(SECURE_EXEC_RESTRICT_FILE) | \
> > > + issecure_mask(SECURE_EXEC_DENY_INTERACTIVE))
> > > #define SECURE_ALL_LOCKS (SECURE_ALL_BITS << 1)
> > >
> > > +#define SECURE_ALL_UNPRIVILEGED (issecure_mask(SECURE_EXEC_RESTRICT_FILE) | \
> > > + issecure_mask(SECURE_EXEC_DENY_INTERACTIVE))
> > > +
> > > #endif /* _UAPI_LINUX_SECUREBITS_H */
> > > diff --git a/security/commoncap.c b/security/commoncap.c
> > > index cefad323a0b1..52ea01acb453 100644
> > > --- a/security/commoncap.c
> > > +++ b/security/commoncap.c
> > > @@ -1302,21 +1302,38 @@ int cap_task_prctl(int option, unsigned long arg2, unsigned long arg3,
> > > & (old->securebits ^ arg2)) /*[1]*/
> > > || ((old->securebits & SECURE_ALL_LOCKS & ~arg2)) /*[2]*/
> > > || (arg2 & ~(SECURE_ALL_LOCKS | SECURE_ALL_BITS)) /*[3]*/
> > > - || (cap_capable(current_cred(),
> > > - current_cred()->user_ns,
> > > - CAP_SETPCAP,
> > > - CAP_OPT_NONE) != 0) /*[4]*/
> > > /*
> > > * [1] no changing of bits that are locked
> > > * [2] no unlocking of locks
> > > * [3] no setting of unsupported bits
> > > - * [4] doing anything requires privilege (go read about
> > > - * the "sendmail capabilities bug")
> > > */
> > > )
> > > /* cannot change a locked bit */
> > > return -EPERM;
> > >
> > > + /*
> > > + * Doing anything requires privilege (go read about the
> > > + * "sendmail capabilities bug"), except for unprivileged bits.
> > > + * Indeed, the SECURE_ALL_UNPRIVILEGED bits are not
> > > + * restrictions enforced by the kernel but by user space on
> > > + * itself.
> > > + */
> > > + if (cap_capable(current_cred(), current_cred()->user_ns,
> > > + CAP_SETPCAP, CAP_OPT_NONE) != 0) {
> > > + const unsigned long unpriv_and_locks =
> > > + SECURE_ALL_UNPRIVILEGED |
> > > + SECURE_ALL_UNPRIVILEGED << 1;
> > > + const unsigned long changed = old->securebits ^ arg2;
> > > +
> > > + /* For legacy reason, denies non-change. */
> > > + if (!changed)
> > > + return -EPERM;
> >
> > This is odd to me. You say for legacy reasons, but, currently, calling
> > PR_SET_SECUREBITS with no changes returns 0. So you may be breaking
> > a lot of programs here, unless I'm mistaken.
>
> When we call PR_SET_SECUREBITS with 0 (and if it was 0 too), it
> currently goes through the capability check and return -EPERM if the
> caller doesn't have CAP_SETCAP. This is tested with
> TEST_F(secbits, legacy) in tools/testing/selftests/exec/check-exec.c
> (patch 3/6).
Drat, my manual test case had a typo. Right you are - it fails now.
thanks,
-serge
^ permalink raw reply
* Re: [PATCH v20 1/6] exec: Add a new AT_CHECK flag to execveat(2)
From: Amir Goldstein @ 2024-10-14 8:24 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Al Viro, Christian Brauner, Kees Cook, Linus Torvalds, Paul Moore,
Serge Hallyn, Theodore Ts'o, Adhemerval Zanella Netto,
Alejandro Colomar, Aleksa Sarai, Andrew Morton, Andy Lutomirski,
Arnd Bergmann, Casey Schaufler, Christian Heimes, Dmitry Vyukov,
Elliott Hughes, Eric Biggers, Eric Chiang, Fan Wu, Florian Weimer,
Geert Uytterhoeven, James Morris, Jan Kara, Jann Horn, Jeff Xu,
Jonathan Corbet, Jordan R Abrahams, Lakshmi Ramasubramanian,
Luca Boccassi, Luis Chamberlain, Madhavan T . Venkataraman,
Matt Bobrowski, Matthew Garrett, Matthew Wilcox, Miklos Szeredi,
Mimi Zohar, Nicolas Bouchinet, Scott Shell, Shuah Khan,
Stephen Rothwell, Steve Dower, Steve Grubb, Thibaut Sautereau,
Vincent Strubel, Xiaoming Ni, Yin Fengwei, kernel-hardening,
linux-api, linux-fsdevel, linux-integrity, linux-kernel,
linux-security-module
In-Reply-To: <20241014.waet2se6Jeiw@digikod.net>
On Mon, Oct 14, 2024 at 9:39 AM Mickaël Salaün <mic@digikod.net> wrote:
>
> On Sun, Oct 13, 2024 at 11:25:11AM +0200, Amir Goldstein wrote:
> > On Fri, Oct 11, 2024 at 8:45 PM Mickaël Salaün <mic@digikod.net> wrote:
> > >
> > > Add a new AT_CHECK flag to execveat(2) to check if a file would be
> > > allowed for execution. The main use case is for script interpreters and
> > > dynamic linkers to check execution permission according to the kernel's
> > > security policy. Another use case is to add context to access logs e.g.,
> > > which script (instead of interpreter) accessed a file. As any
> > > executable code, scripts could also use this check [1].
> > >
> > > This is different from faccessat(2) + X_OK which only checks a subset of
> > > access rights (i.e. inode permission and mount options for regular
> > > files), but not the full context (e.g. all LSM access checks). The main
> > > use case for access(2) is for SUID processes to (partially) check access
> > > on behalf of their caller. The main use case for execveat(2) + AT_CHECK
> > > is to check if a script execution would be allowed, according to all the
> > > different restrictions in place. Because the use of AT_CHECK follows
> > > the exact kernel semantic as for a real execution, user space gets the
> > > same error codes.
> > >
> > > An interesting point of using execveat(2) instead of openat2(2) is that
> > > it decouples the check from the enforcement. Indeed, the security check
> > > can be logged (e.g. with audit) without blocking an execution
> > > environment not yet ready to enforce a strict security policy.
> > >
> > > LSMs can control or log execution requests with
> > > security_bprm_creds_for_exec(). However, to enforce a consistent and
> > > complete access control (e.g. on binary's dependencies) LSMs should
> > > restrict file executability, or mesure executed files, with
> > > security_file_open() by checking file->f_flags & __FMODE_EXEC.
> > >
> > > Because AT_CHECK is dedicated to user space interpreters, it doesn't
> > > make sense for the kernel to parse the checked files, look for
> > > interpreters known to the kernel (e.g. ELF, shebang), and return ENOEXEC
> > > if the format is unknown. Because of that, security_bprm_check() is
> > > never called when AT_CHECK is used.
> > >
> > > It should be noted that script interpreters cannot directly use
> > > execveat(2) (without this new AT_CHECK flag) because this could lead to
> > > unexpected behaviors e.g., `python script.sh` could lead to Bash being
> > > executed to interpret the script. Unlike the kernel, script
> > > interpreters may just interpret the shebang as a simple comment, which
> > > should not change for backward compatibility reasons.
> > >
> > > Because scripts or libraries files might not currently have the
> > > executable permission set, or because we might want specific users to be
> > > allowed to run arbitrary scripts, the following patch provides a dynamic
> > > configuration mechanism with the SECBIT_EXEC_RESTRICT_FILE and
> > > SECBIT_EXEC_DENY_INTERACTIVE securebits.
> > >
> > > This is a redesign of the CLIP OS 4's O_MAYEXEC:
> > > https://github.com/clipos-archive/src_platform_clip-patches/blob/f5cb330d6b684752e403b4e41b39f7004d88e561/1901_open_mayexec.patch
> > > This patch has been used for more than a decade with customized script
> > > interpreters. Some examples can be found here:
> > > https://github.com/clipos-archive/clipos4_portage-overlay/search?q=O_MAYEXEC
> > >
> > > Cc: Al Viro <viro@zeniv.linux.org.uk>
> > > Cc: Christian Brauner <brauner@kernel.org>
> > > Cc: Kees Cook <keescook@chromium.org>
> > > Cc: Paul Moore <paul@paul-moore.com>
> > > Cc: Serge Hallyn <serge@hallyn.com>
> > > Link: https://docs.python.org/3/library/io.html#io.open_code [1]
> > > Signed-off-by: Mickaël Salaün <mic@digikod.net>
> > > Link: https://lore.kernel.org/r/20241011184422.977903-2-mic@digikod.net
> > > ---
> > >
> > > Changes since v19:
> > > * Remove mention of "role transition" as suggested by Andy.
> > > * Highlight the difference between security_bprm_creds_for_exec() and
> > > the __FMODE_EXEC check for LSMs (in commit message and LSM's hooks) as
> > > discussed with Jeff.
> > > * Improve documentation both in UAPI comments and kernel comments
> > > (requested by Kees).
> > >
> > > New design since v18:
> > > https://lore.kernel.org/r/20220104155024.48023-3-mic@digikod.net
> > > ---
> > > fs/exec.c | 18 ++++++++++++++++--
> > > include/linux/binfmts.h | 7 ++++++-
> > > include/uapi/linux/fcntl.h | 31 +++++++++++++++++++++++++++++++
> > > kernel/audit.h | 1 +
> > > kernel/auditsc.c | 1 +
> > > security/security.c | 10 ++++++++++
> > > 6 files changed, 65 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/fs/exec.c b/fs/exec.c
> > > index 6c53920795c2..163c659d9ae6 100644
> > > --- a/fs/exec.c
> > > +++ b/fs/exec.c
> > > @@ -891,7 +891,7 @@ static struct file *do_open_execat(int fd, struct filename *name, int flags)
> > > .lookup_flags = LOOKUP_FOLLOW,
> > > };
> > >
> > > - if ((flags & ~(AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH)) != 0)
> > > + if ((flags & ~(AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH | AT_CHECK)) != 0)
> > > return ERR_PTR(-EINVAL);
> > > if (flags & AT_SYMLINK_NOFOLLOW)
> > > open_exec_flags.lookup_flags &= ~LOOKUP_FOLLOW;
> > > @@ -1545,6 +1545,20 @@ static struct linux_binprm *alloc_bprm(int fd, struct filename *filename, int fl
> > > }
> > > bprm->interp = bprm->filename;
> > >
> > > + /*
> > > + * At this point, security_file_open() has already been called (with
> > > + * __FMODE_EXEC) and access control checks for AT_CHECK will stop just
> > > + * after the security_bprm_creds_for_exec() call in bprm_execve().
> > > + * Indeed, the kernel should not try to parse the content of the file
> > > + * with exec_binprm() nor change the calling thread, which means that
> > > + * the following security functions will be not called:
> > > + * - security_bprm_check()
> > > + * - security_bprm_creds_from_file()
> > > + * - security_bprm_committing_creds()
> > > + * - security_bprm_committed_creds()
> > > + */
> > > + bprm->is_check = !!(flags & AT_CHECK);
> > > +
> > > retval = bprm_mm_init(bprm);
> > > if (!retval)
> > > return bprm;
> > > @@ -1839,7 +1853,7 @@ static int bprm_execve(struct linux_binprm *bprm)
> > >
> > > /* Set the unchanging part of bprm->cred */
> > > retval = security_bprm_creds_for_exec(bprm);
> > > - if (retval)
> > > + if (retval || bprm->is_check)
> > > goto out;
> > >
> > > retval = exec_binprm(bprm);
> > > diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
> > > index e6c00e860951..8ff0eb3644a1 100644
> > > --- a/include/linux/binfmts.h
> > > +++ b/include/linux/binfmts.h
> > > @@ -42,7 +42,12 @@ struct linux_binprm {
> > > * Set when errors can no longer be returned to the
> > > * original userspace.
> > > */
> > > - point_of_no_return:1;
> > > + point_of_no_return:1,
> > > + /*
> > > + * Set by user space to check executability according to the
> > > + * caller's environment.
> > > + */
> > > + is_check:1;
> > > struct file *executable; /* Executable to pass to the interpreter */
> > > struct file *interpreter;
> > > struct file *file;
> > > diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h
> > > index 87e2dec79fea..e606815b1c5a 100644
> > > --- a/include/uapi/linux/fcntl.h
> > > +++ b/include/uapi/linux/fcntl.h
> > > @@ -154,6 +154,37 @@
> > > usable with open_by_handle_at(2). */
> > > #define AT_HANDLE_MNT_ID_UNIQUE 0x001 /* Return the u64 unique mount ID. */
> > >
> > > +/*
> > > + * AT_CHECK only performs a check on a regular file and returns 0 if execution
> > > + * of this file would be allowed, ignoring the file format and then the related
> > > + * interpreter dependencies (e.g. ELF libraries, script's shebang).
> > > + *
> > > + * Programs should always perform this check to apply kernel-level checks
> > > + * against files that are not directly executed by the kernel but passed to a
> > > + * user space interpreter instead. All files that contain executable code,
> > > + * from the point of view of the interpreter, should be checked. However the
> > > + * result of this check should only be enforced according to
> > > + * SECBIT_EXEC_RESTRICT_FILE or SECBIT_EXEC_DENY_INTERACTIVE. See securebits.h
> > > + * documentation and the samples/check-exec/inc.c example.
> > > + *
> > > + * The main purpose of this flag is to improve the security and consistency of
> > > + * an execution environment to ensure that direct file execution (e.g.
> > > + * `./script.sh`) and indirect file execution (e.g. `sh script.sh`) lead to the
> > > + * same result. For instance, this can be used to check if a file is
> > > + * trustworthy according to the caller's environment.
> > > + *
> > > + * In a secure environment, libraries and any executable dependencies should
> > > + * also be checked. For instance, dynamic linking should make sure that all
> > > + * libraries are allowed for execution to avoid trivial bypass (e.g. using
> > > + * LD_PRELOAD). For such secure execution environment to make sense, only
> > > + * trusted code should be executable, which also requires integrity guarantees.
> > > + *
> > > + * To avoid race conditions leading to time-of-check to time-of-use issues,
> > > + * AT_CHECK should be used with AT_EMPTY_PATH to check against a file
> > > + * descriptor instead of a path.
> > > + */
> >
> > If you ask me, the very elaborate comment above belongs to execveat(2)
> > man page and is way too verbose for a uapi header.
>
> OK, but since this new flags raised a lot of questions, I guess a
> dedicated Documentation/userspace-api/check-exec.rst file with thit
> AT*_CHECK and the related securebits would be useful instead of the
> related inlined documentation.
>
> >
> > > +#define AT_CHECK 0x10000
> >
> > Please see the comment "Per-syscall flags for the *at(2) family of syscalls."
> > above. If this is a per-syscall flag please use one of the per-syscall
> > flags, e.g.:
> >
> > /* Flags for execveat2(2) */
> > #define AT_EXECVE_CHECK 0x0001 /* Only perform a check if
> > execution would be allowed */
>
> I missed this part, this prefix makes sense, thanks.
>
Not only the prefix, also the overloaded value.
Thanks,
Amir.
^ permalink raw reply
* Re: [PATCH v20 2/6] security: Add EXEC_RESTRICT_FILE and EXEC_DENY_INTERACTIVE securebits
From: Mickaël Salaün @ 2024-10-14 7:40 UTC (permalink / raw)
To: Serge E. Hallyn
Cc: Al Viro, Christian Brauner, Kees Cook, Linus Torvalds, Paul Moore,
Theodore Ts'o, Adhemerval Zanella Netto, Alejandro Colomar,
Aleksa Sarai, Andrew Morton, Andy Lutomirski, Arnd Bergmann,
Casey Schaufler, Christian Heimes, Dmitry Vyukov, Elliott Hughes,
Eric Biggers, Eric Chiang, Fan Wu, Florian Weimer,
Geert Uytterhoeven, James Morris, Jan Kara, Jann Horn, Jeff Xu,
Jonathan Corbet, Jordan R Abrahams, Lakshmi Ramasubramanian,
Luca Boccassi, Luis Chamberlain, Madhavan T . Venkataraman,
Matt Bobrowski, Matthew Garrett, Matthew Wilcox, Miklos Szeredi,
Mimi Zohar, Nicolas Bouchinet, Scott Shell, Shuah Khan,
Stephen Rothwell, Steve Dower, Steve Grubb, Thibaut Sautereau,
Vincent Strubel, Xiaoming Ni, Yin Fengwei, kernel-hardening,
linux-api, linux-fsdevel, linux-integrity, linux-kernel,
linux-security-module, Andy Lutomirski
In-Reply-To: <20241013025150.GA1056399@mail.hallyn.com>
On Sat, Oct 12, 2024 at 09:51:50PM -0500, Serge E. Hallyn wrote:
> On Fri, Oct 11, 2024 at 08:44:18PM +0200, Mickaël Salaün wrote:
> > The new SECBIT_EXEC_RESTRICT_FILE, SECBIT_EXEC_DENY_INTERACTIVE, and
> > their *_LOCKED counterparts are designed to be set by processes setting
> > up an execution environment, such as a user session, a container, or a
> > security sandbox. Unlike other securebits, these ones can be set by
> > unprivileged processes. Like seccomp filters or Landlock domains, the
> > securebits are inherited across processes.
> >
> > When SECBIT_EXEC_RESTRICT_FILE is set, programs interpreting code should
> > control executable resources according to execveat(2) + AT_CHECK (see
> > previous commit).
> >
> > When SECBIT_EXEC_DENY_INTERACTIVE is set, a process should deny
> > execution of user interactive commands (which excludes executable
> > regular files).
> >
> > Being able to configure each of these securebits enables system
> > administrators or owner of image containers to gradually validate the
> > related changes and to identify potential issues (e.g. with interpreter
> > or audit logs).
> >
> > It should be noted that unlike other security bits, the
> > SECBIT_EXEC_RESTRICT_FILE and SECBIT_EXEC_DENY_INTERACTIVE bits are
> > dedicated to user space willing to restrict itself. Because of that,
> > they only make sense in the context of a trusted environment (e.g.
> > sandbox, container, user session, full system) where the process
> > changing its behavior (according to these bits) and all its parent
> > processes are trusted. Otherwise, any parent process could just execute
> > its own malicious code (interpreting a script or not), or even enforce a
> > seccomp filter to mask these bits.
> >
> > Such a secure environment can be achieved with an appropriate access
> > control (e.g. mount's noexec option, file access rights, LSM policy) and
> > an enlighten ld.so checking that libraries are allowed for execution
> > e.g., to protect against illegitimate use of LD_PRELOAD.
> >
> > Ptrace restrictions according to these securebits would not make sense
> > because of the processes' trust assumption.
> >
> > Scripts may need some changes to deal with untrusted data (e.g. stdin,
> > environment variables), but that is outside the scope of the kernel.
> >
> > See chromeOS's documentation about script execution control and the
> > related threat model:
> > https://www.chromium.org/chromium-os/developer-library/guides/security/noexec-shell-scripts/
> >
> > Cc: Al Viro <viro@zeniv.linux.org.uk>
> > Cc: Andy Lutomirski <luto@amacapital.net>
> > Cc: Christian Brauner <brauner@kernel.org>
> > Cc: Kees Cook <keescook@chromium.org>
> > Cc: Paul Moore <paul@paul-moore.com>
> > Cc: Serge Hallyn <serge@hallyn.com>
> > Signed-off-by: Mickaël Salaün <mic@digikod.net>
> > Link: https://lore.kernel.org/r/20241011184422.977903-3-mic@digikod.net
> > ---
> >
> > Changes since v19:
> > * Replace SECBIT_SHOULD_EXEC_CHECK and SECBIT_SHOULD_EXEC_RESTRICT with
> > SECBIT_EXEC_RESTRICT_FILE and SECBIT_EXEC_DENY_INTERACTIVE:
> > https://lore.kernel.org/all/20240710.eiKohpa4Phai@digikod.net/
> > * Remove the ptrace restrictions, suggested by Andy.
> > * Improve documentation according to the discussion with Jeff.
> >
> > New design since v18:
> > https://lore.kernel.org/r/20220104155024.48023-3-mic@digikod.net
> > ---
> > include/uapi/linux/securebits.h | 113 +++++++++++++++++++++++++++++++-
> > security/commoncap.c | 29 ++++++--
> > 2 files changed, 135 insertions(+), 7 deletions(-)
> >
> > diff --git a/include/uapi/linux/securebits.h b/include/uapi/linux/securebits.h
> > index d6d98877ff1a..351b6ecefc76 100644
> > --- a/include/uapi/linux/securebits.h
> > +++ b/include/uapi/linux/securebits.h
> > @@ -52,10 +52,121 @@
> > #define SECBIT_NO_CAP_AMBIENT_RAISE_LOCKED \
> > (issecure_mask(SECURE_NO_CAP_AMBIENT_RAISE_LOCKED))
> >
> > +/*
> > + * The SECBIT_EXEC_RESTRICT_FILE and SECBIT_EXEC_DENY_INTERACTIVE securebits
> > + * are intended for script interpreters and dynamic linkers to enforce a
> > + * consistent execution security policy handled by the kernel.
> > + *
> > + * Whether an interpreter should check these securebits or not depends on the
> > + * security risk of running malicious scripts with respect to the execution
> > + * environment, and whether the kernel can check if a script is trustworthy or
> > + * not. For instance, Python scripts running on a server can use arbitrary
> > + * syscalls and access arbitrary files. Such interpreters should then be
> > + * enlighten to use these securebits and let users define their security
> > + * policy. However, a JavaScript engine running in a web browser should
> > + * already be sandboxed and then should not be able to harm the user's
> > + * environment.
> > + *
> > + * When SECBIT_EXEC_RESTRICT_FILE is set, a process should only interpret or
> > + * execute a file if a call to execveat(2) with the related file descriptor and
> > + * the AT_CHECK flag succeed.
> > + *
> > + * This secure bit may be set by user session managers, service managers,
> > + * container runtimes, sandboxer tools... Except for test environments, the
> > + * related SECBIT_EXEC_RESTRICT_FILE_LOCKED bit should also be set.
> > + *
> > + * Programs should only enforce consistent restrictions according to the
> > + * securebits but without relying on any other user-controlled configuration.
> > + * Indeed, the use case for these securebits is to only trust executable code
> > + * vetted by the system configuration (through the kernel), so we should be
> > + * careful to not let untrusted users control this configuration.
> > + *
> > + * However, script interpreters may still use user configuration such as
> > + * environment variables as long as it is not a way to disable the securebits
> > + * checks. For instance, the PATH and LD_PRELOAD variables can be set by a
> > + * script's caller. Changing these variables may lead to unintended code
> > + * executions, but only from vetted executable programs, which is OK. For this
> > + * to make sense, the system should provide a consistent security policy to
> > + * avoid arbitrary code execution e.g., by enforcing a write xor execute
> > + * policy.
> > + *
> > + * SECBIT_EXEC_RESTRICT_FILE is complementary and should also be checked.
> > + */
> > +#define SECURE_EXEC_RESTRICT_FILE 8
> > +#define SECURE_EXEC_RESTRICT_FILE_LOCKED 9 /* make bit-8 immutable */
> > +
> > +#define SECBIT_EXEC_RESTRICT_FILE (issecure_mask(SECURE_EXEC_RESTRICT_FILE))
> > +#define SECBIT_EXEC_RESTRICT_FILE_LOCKED \
> > + (issecure_mask(SECURE_EXEC_RESTRICT_FILE_LOCKED))
> > +
> > +/*
> > + * When SECBIT_EXEC_DENY_INTERACTIVE is set, a process should never interpret
> > + * interactive user commands (e.g. scripts). However, if such commands are
> > + * passed through a file descriptor (e.g. stdin), its content should be
> > + * interpreted if a call to execveat(2) with the related file descriptor and
> > + * the AT_CHECK flag succeed.
> > + *
> > + * For instance, script interpreters called with a script snippet as argument
> > + * should always deny such execution if SECBIT_EXEC_DENY_INTERACTIVE is set.
> > + *
> > + * This secure bit may be set by user session managers, service managers,
> > + * container runtimes, sandboxer tools... Except for test environments, the
> > + * related SECBIT_EXEC_DENY_INTERACTIVE_LOCKED bit should also be set.
> > + *
> > + * See the SECBIT_EXEC_RESTRICT_FILE documentation.
> > + *
> > + * Here is the expected behavior for a script interpreter according to
> > + * combination of any exec securebits:
> > + *
> > + * 1. SECURE_EXEC_RESTRICT_FILE=0 SECURE_EXEC_DENY_INTERACTIVE=0 (default)
> > + * Always interpret scripts, and allow arbitrary user commands.
> > + * => No threat, everyone and everything is trusted, but we can get ahead of
> > + * potential issues thanks to the call to execveat with AT_CHECK which
> > + * should always be performed but ignored by the script interpreter.
> > + * Indeed, this check is still important to enable systems administrators
> > + * to verify requests (e.g. with audit) and prepare for migration to a
> > + * secure mode.
> > + *
> > + * 2. SECURE_EXEC_RESTRICT_FILE=1 SECURE_EXEC_DENY_INTERACTIVE=0
> > + * Deny script interpretation if they are not executable, but allow
> > + * arbitrary user commands.
> > + * => The threat is (potential) malicious scripts run by trusted (and not
> > + * fooled) users. That can protect against unintended script executions
> > + * (e.g. sh /tmp/*.sh). This makes sense for (semi-restricted) user
> > + * sessions.
> > + *
> > + * 3. SECURE_EXEC_RESTRICT_FILE=0 SECURE_EXEC_DENY_INTERACTIVE=1
> > + * Always interpret scripts, but deny arbitrary user commands.
> > + * => This use case may be useful for secure services (i.e. without
> > + * interactive user session) where scripts' integrity is verified (e.g.
> > + * with IMA/EVM or dm-verity/IPE) but where access rights might not be
> > + * ready yet. Indeed, arbitrary interactive commands would be much more
> > + * difficult to check.
> > + *
> > + * 4. SECURE_EXEC_RESTRICT_FILE=1 SECURE_EXEC_DENY_INTERACTIVE=1
> > + * Deny script interpretation if they are not executable, and also deny
> > + * any arbitrary user commands.
> > + * => The threat is malicious scripts run by untrusted users (but trusted
> > + * code). This makes sense for system services that may only execute
> > + * trusted scripts.
> > + */
> > +#define SECURE_EXEC_DENY_INTERACTIVE 10
> > +#define SECURE_EXEC_DENY_INTERACTIVE_LOCKED 11 /* make bit-10 immutable */
> > +
> > +#define SECBIT_EXEC_DENY_INTERACTIVE \
> > + (issecure_mask(SECURE_EXEC_DENY_INTERACTIVE))
> > +#define SECBIT_EXEC_DENY_INTERACTIVE_LOCKED \
> > + (issecure_mask(SECURE_EXEC_DENY_INTERACTIVE_LOCKED))
> > +
> > #define SECURE_ALL_BITS (issecure_mask(SECURE_NOROOT) | \
> > issecure_mask(SECURE_NO_SETUID_FIXUP) | \
> > issecure_mask(SECURE_KEEP_CAPS) | \
> > - issecure_mask(SECURE_NO_CAP_AMBIENT_RAISE))
> > + issecure_mask(SECURE_NO_CAP_AMBIENT_RAISE) | \
> > + issecure_mask(SECURE_EXEC_RESTRICT_FILE) | \
> > + issecure_mask(SECURE_EXEC_DENY_INTERACTIVE))
> > #define SECURE_ALL_LOCKS (SECURE_ALL_BITS << 1)
> >
> > +#define SECURE_ALL_UNPRIVILEGED (issecure_mask(SECURE_EXEC_RESTRICT_FILE) | \
> > + issecure_mask(SECURE_EXEC_DENY_INTERACTIVE))
> > +
> > #endif /* _UAPI_LINUX_SECUREBITS_H */
> > diff --git a/security/commoncap.c b/security/commoncap.c
> > index cefad323a0b1..52ea01acb453 100644
> > --- a/security/commoncap.c
> > +++ b/security/commoncap.c
> > @@ -1302,21 +1302,38 @@ int cap_task_prctl(int option, unsigned long arg2, unsigned long arg3,
> > & (old->securebits ^ arg2)) /*[1]*/
> > || ((old->securebits & SECURE_ALL_LOCKS & ~arg2)) /*[2]*/
> > || (arg2 & ~(SECURE_ALL_LOCKS | SECURE_ALL_BITS)) /*[3]*/
> > - || (cap_capable(current_cred(),
> > - current_cred()->user_ns,
> > - CAP_SETPCAP,
> > - CAP_OPT_NONE) != 0) /*[4]*/
> > /*
> > * [1] no changing of bits that are locked
> > * [2] no unlocking of locks
> > * [3] no setting of unsupported bits
> > - * [4] doing anything requires privilege (go read about
> > - * the "sendmail capabilities bug")
> > */
> > )
> > /* cannot change a locked bit */
> > return -EPERM;
> >
> > + /*
> > + * Doing anything requires privilege (go read about the
> > + * "sendmail capabilities bug"), except for unprivileged bits.
> > + * Indeed, the SECURE_ALL_UNPRIVILEGED bits are not
> > + * restrictions enforced by the kernel but by user space on
> > + * itself.
> > + */
> > + if (cap_capable(current_cred(), current_cred()->user_ns,
> > + CAP_SETPCAP, CAP_OPT_NONE) != 0) {
> > + const unsigned long unpriv_and_locks =
> > + SECURE_ALL_UNPRIVILEGED |
> > + SECURE_ALL_UNPRIVILEGED << 1;
> > + const unsigned long changed = old->securebits ^ arg2;
> > +
> > + /* For legacy reason, denies non-change. */
> > + if (!changed)
> > + return -EPERM;
>
> This is odd to me. You say for legacy reasons, but, currently, calling
> PR_SET_SECUREBITS with no changes returns 0. So you may be breaking
> a lot of programs here, unless I'm mistaken.
When we call PR_SET_SECUREBITS with 0 (and if it was 0 too), it
currently goes through the capability check and return -EPERM if the
caller doesn't have CAP_SETCAP. This is tested with
TEST_F(secbits, legacy) in tools/testing/selftests/exec/check-exec.c
(patch 3/6).
>
> > +
> > + /* Denies privileged changes. */
> > + if (changed & ~unpriv_and_locks)
> > + return -EPERM;
> > + }
> > +
> > new = prepare_creds();
> > if (!new)
> > return -ENOMEM;
> > --
> > 2.46.1
>
^ permalink raw reply
* Re: [PATCH v20 1/6] exec: Add a new AT_CHECK flag to execveat(2)
From: Mickaël Salaün @ 2024-10-14 7:39 UTC (permalink / raw)
To: Serge E. Hallyn
Cc: Al Viro, Christian Brauner, Kees Cook, Linus Torvalds, Paul Moore,
Theodore Ts'o, Adhemerval Zanella Netto, Alejandro Colomar,
Aleksa Sarai, Andrew Morton, Andy Lutomirski, Arnd Bergmann,
Casey Schaufler, Christian Heimes, Dmitry Vyukov, Elliott Hughes,
Eric Biggers, Eric Chiang, Fan Wu, Florian Weimer,
Geert Uytterhoeven, James Morris, Jan Kara, Jann Horn, Jeff Xu,
Jonathan Corbet, Jordan R Abrahams, Lakshmi Ramasubramanian,
Luca Boccassi, Luis Chamberlain, Madhavan T . Venkataraman,
Matt Bobrowski, Matthew Garrett, Matthew Wilcox, Miklos Szeredi,
Mimi Zohar, Nicolas Bouchinet, Scott Shell, Shuah Khan,
Stephen Rothwell, Steve Dower, Steve Grubb, Thibaut Sautereau,
Vincent Strubel, Xiaoming Ni, Yin Fengwei, kernel-hardening,
linux-api, linux-fsdevel, linux-integrity, linux-kernel,
linux-security-module
In-Reply-To: <20241013030416.GA1056921@mail.hallyn.com>
On Sat, Oct 12, 2024 at 10:04:16PM -0500, Serge E. Hallyn wrote:
> On Fri, Oct 11, 2024 at 08:44:17PM +0200, Mickaël Salaün wrote:
> > Add a new AT_CHECK flag to execveat(2) to check if a file would be
>
> Apologies for both bikeshedding and missing earlier discussions.
>
> But AT_CHECK sounds quite generic. How about AT_EXEC_CHECK, or
> AT_CHECK_EXEC_CREDS? (I would suggest just AT_CHECK_CREDS since
> it's for use in execveat(2), but as it's an AT_ flag, it's
> probably worth being more precise).
As Amir pointed out, we need at least to use the AT_EXECVE_CHECK_
prefix, and I agree with the AT_EXECVE_CHECK name because it's about
checking the whole execve request, not sepcifically a "creds" part.
^ permalink raw reply
* Re: [PATCH v20 1/6] exec: Add a new AT_CHECK flag to execveat(2)
From: Mickaël Salaün @ 2024-10-14 7:39 UTC (permalink / raw)
To: Amir Goldstein
Cc: Al Viro, Christian Brauner, Kees Cook, Linus Torvalds, Paul Moore,
Serge Hallyn, Theodore Ts'o, Adhemerval Zanella Netto,
Alejandro Colomar, Aleksa Sarai, Andrew Morton, Andy Lutomirski,
Arnd Bergmann, Casey Schaufler, Christian Heimes, Dmitry Vyukov,
Elliott Hughes, Eric Biggers, Eric Chiang, Fan Wu, Florian Weimer,
Geert Uytterhoeven, James Morris, Jan Kara, Jann Horn, Jeff Xu,
Jonathan Corbet, Jordan R Abrahams, Lakshmi Ramasubramanian,
Luca Boccassi, Luis Chamberlain, Madhavan T . Venkataraman,
Matt Bobrowski, Matthew Garrett, Matthew Wilcox, Miklos Szeredi,
Mimi Zohar, Nicolas Bouchinet, Scott Shell, Shuah Khan,
Stephen Rothwell, Steve Dower, Steve Grubb, Thibaut Sautereau,
Vincent Strubel, Xiaoming Ni, Yin Fengwei, kernel-hardening,
linux-api, linux-fsdevel, linux-integrity, linux-kernel,
linux-security-module
In-Reply-To: <CAOQ4uxi502PNn8eyKt9BExXVhbpx04f0XTeLg771i6wPOrLadA@mail.gmail.com>
On Sun, Oct 13, 2024 at 11:25:11AM +0200, Amir Goldstein wrote:
> On Fri, Oct 11, 2024 at 8:45 PM Mickaël Salaün <mic@digikod.net> wrote:
> >
> > Add a new AT_CHECK flag to execveat(2) to check if a file would be
> > allowed for execution. The main use case is for script interpreters and
> > dynamic linkers to check execution permission according to the kernel's
> > security policy. Another use case is to add context to access logs e.g.,
> > which script (instead of interpreter) accessed a file. As any
> > executable code, scripts could also use this check [1].
> >
> > This is different from faccessat(2) + X_OK which only checks a subset of
> > access rights (i.e. inode permission and mount options for regular
> > files), but not the full context (e.g. all LSM access checks). The main
> > use case for access(2) is for SUID processes to (partially) check access
> > on behalf of their caller. The main use case for execveat(2) + AT_CHECK
> > is to check if a script execution would be allowed, according to all the
> > different restrictions in place. Because the use of AT_CHECK follows
> > the exact kernel semantic as for a real execution, user space gets the
> > same error codes.
> >
> > An interesting point of using execveat(2) instead of openat2(2) is that
> > it decouples the check from the enforcement. Indeed, the security check
> > can be logged (e.g. with audit) without blocking an execution
> > environment not yet ready to enforce a strict security policy.
> >
> > LSMs can control or log execution requests with
> > security_bprm_creds_for_exec(). However, to enforce a consistent and
> > complete access control (e.g. on binary's dependencies) LSMs should
> > restrict file executability, or mesure executed files, with
> > security_file_open() by checking file->f_flags & __FMODE_EXEC.
> >
> > Because AT_CHECK is dedicated to user space interpreters, it doesn't
> > make sense for the kernel to parse the checked files, look for
> > interpreters known to the kernel (e.g. ELF, shebang), and return ENOEXEC
> > if the format is unknown. Because of that, security_bprm_check() is
> > never called when AT_CHECK is used.
> >
> > It should be noted that script interpreters cannot directly use
> > execveat(2) (without this new AT_CHECK flag) because this could lead to
> > unexpected behaviors e.g., `python script.sh` could lead to Bash being
> > executed to interpret the script. Unlike the kernel, script
> > interpreters may just interpret the shebang as a simple comment, which
> > should not change for backward compatibility reasons.
> >
> > Because scripts or libraries files might not currently have the
> > executable permission set, or because we might want specific users to be
> > allowed to run arbitrary scripts, the following patch provides a dynamic
> > configuration mechanism with the SECBIT_EXEC_RESTRICT_FILE and
> > SECBIT_EXEC_DENY_INTERACTIVE securebits.
> >
> > This is a redesign of the CLIP OS 4's O_MAYEXEC:
> > https://github.com/clipos-archive/src_platform_clip-patches/blob/f5cb330d6b684752e403b4e41b39f7004d88e561/1901_open_mayexec.patch
> > This patch has been used for more than a decade with customized script
> > interpreters. Some examples can be found here:
> > https://github.com/clipos-archive/clipos4_portage-overlay/search?q=O_MAYEXEC
> >
> > Cc: Al Viro <viro@zeniv.linux.org.uk>
> > Cc: Christian Brauner <brauner@kernel.org>
> > Cc: Kees Cook <keescook@chromium.org>
> > Cc: Paul Moore <paul@paul-moore.com>
> > Cc: Serge Hallyn <serge@hallyn.com>
> > Link: https://docs.python.org/3/library/io.html#io.open_code [1]
> > Signed-off-by: Mickaël Salaün <mic@digikod.net>
> > Link: https://lore.kernel.org/r/20241011184422.977903-2-mic@digikod.net
> > ---
> >
> > Changes since v19:
> > * Remove mention of "role transition" as suggested by Andy.
> > * Highlight the difference between security_bprm_creds_for_exec() and
> > the __FMODE_EXEC check for LSMs (in commit message and LSM's hooks) as
> > discussed with Jeff.
> > * Improve documentation both in UAPI comments and kernel comments
> > (requested by Kees).
> >
> > New design since v18:
> > https://lore.kernel.org/r/20220104155024.48023-3-mic@digikod.net
> > ---
> > fs/exec.c | 18 ++++++++++++++++--
> > include/linux/binfmts.h | 7 ++++++-
> > include/uapi/linux/fcntl.h | 31 +++++++++++++++++++++++++++++++
> > kernel/audit.h | 1 +
> > kernel/auditsc.c | 1 +
> > security/security.c | 10 ++++++++++
> > 6 files changed, 65 insertions(+), 3 deletions(-)
> >
> > diff --git a/fs/exec.c b/fs/exec.c
> > index 6c53920795c2..163c659d9ae6 100644
> > --- a/fs/exec.c
> > +++ b/fs/exec.c
> > @@ -891,7 +891,7 @@ static struct file *do_open_execat(int fd, struct filename *name, int flags)
> > .lookup_flags = LOOKUP_FOLLOW,
> > };
> >
> > - if ((flags & ~(AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH)) != 0)
> > + if ((flags & ~(AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH | AT_CHECK)) != 0)
> > return ERR_PTR(-EINVAL);
> > if (flags & AT_SYMLINK_NOFOLLOW)
> > open_exec_flags.lookup_flags &= ~LOOKUP_FOLLOW;
> > @@ -1545,6 +1545,20 @@ static struct linux_binprm *alloc_bprm(int fd, struct filename *filename, int fl
> > }
> > bprm->interp = bprm->filename;
> >
> > + /*
> > + * At this point, security_file_open() has already been called (with
> > + * __FMODE_EXEC) and access control checks for AT_CHECK will stop just
> > + * after the security_bprm_creds_for_exec() call in bprm_execve().
> > + * Indeed, the kernel should not try to parse the content of the file
> > + * with exec_binprm() nor change the calling thread, which means that
> > + * the following security functions will be not called:
> > + * - security_bprm_check()
> > + * - security_bprm_creds_from_file()
> > + * - security_bprm_committing_creds()
> > + * - security_bprm_committed_creds()
> > + */
> > + bprm->is_check = !!(flags & AT_CHECK);
> > +
> > retval = bprm_mm_init(bprm);
> > if (!retval)
> > return bprm;
> > @@ -1839,7 +1853,7 @@ static int bprm_execve(struct linux_binprm *bprm)
> >
> > /* Set the unchanging part of bprm->cred */
> > retval = security_bprm_creds_for_exec(bprm);
> > - if (retval)
> > + if (retval || bprm->is_check)
> > goto out;
> >
> > retval = exec_binprm(bprm);
> > diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
> > index e6c00e860951..8ff0eb3644a1 100644
> > --- a/include/linux/binfmts.h
> > +++ b/include/linux/binfmts.h
> > @@ -42,7 +42,12 @@ struct linux_binprm {
> > * Set when errors can no longer be returned to the
> > * original userspace.
> > */
> > - point_of_no_return:1;
> > + point_of_no_return:1,
> > + /*
> > + * Set by user space to check executability according to the
> > + * caller's environment.
> > + */
> > + is_check:1;
> > struct file *executable; /* Executable to pass to the interpreter */
> > struct file *interpreter;
> > struct file *file;
> > diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h
> > index 87e2dec79fea..e606815b1c5a 100644
> > --- a/include/uapi/linux/fcntl.h
> > +++ b/include/uapi/linux/fcntl.h
> > @@ -154,6 +154,37 @@
> > usable with open_by_handle_at(2). */
> > #define AT_HANDLE_MNT_ID_UNIQUE 0x001 /* Return the u64 unique mount ID. */
> >
> > +/*
> > + * AT_CHECK only performs a check on a regular file and returns 0 if execution
> > + * of this file would be allowed, ignoring the file format and then the related
> > + * interpreter dependencies (e.g. ELF libraries, script's shebang).
> > + *
> > + * Programs should always perform this check to apply kernel-level checks
> > + * against files that are not directly executed by the kernel but passed to a
> > + * user space interpreter instead. All files that contain executable code,
> > + * from the point of view of the interpreter, should be checked. However the
> > + * result of this check should only be enforced according to
> > + * SECBIT_EXEC_RESTRICT_FILE or SECBIT_EXEC_DENY_INTERACTIVE. See securebits.h
> > + * documentation and the samples/check-exec/inc.c example.
> > + *
> > + * The main purpose of this flag is to improve the security and consistency of
> > + * an execution environment to ensure that direct file execution (e.g.
> > + * `./script.sh`) and indirect file execution (e.g. `sh script.sh`) lead to the
> > + * same result. For instance, this can be used to check if a file is
> > + * trustworthy according to the caller's environment.
> > + *
> > + * In a secure environment, libraries and any executable dependencies should
> > + * also be checked. For instance, dynamic linking should make sure that all
> > + * libraries are allowed for execution to avoid trivial bypass (e.g. using
> > + * LD_PRELOAD). For such secure execution environment to make sense, only
> > + * trusted code should be executable, which also requires integrity guarantees.
> > + *
> > + * To avoid race conditions leading to time-of-check to time-of-use issues,
> > + * AT_CHECK should be used with AT_EMPTY_PATH to check against a file
> > + * descriptor instead of a path.
> > + */
>
> If you ask me, the very elaborate comment above belongs to execveat(2)
> man page and is way too verbose for a uapi header.
OK, but since this new flags raised a lot of questions, I guess a
dedicated Documentation/userspace-api/check-exec.rst file with thit
AT*_CHECK and the related securebits would be useful instead of the
related inlined documentation.
>
> > +#define AT_CHECK 0x10000
>
> Please see the comment "Per-syscall flags for the *at(2) family of syscalls."
> above. If this is a per-syscall flag please use one of the per-syscall
> flags, e.g.:
>
> /* Flags for execveat2(2) */
> #define AT_EXECVE_CHECK 0x0001 /* Only perform a check if
> execution would be allowed */
I missed this part, this prefix makes sense, thanks.
>
>
> Thanks,
> Amir.
^ permalink raw reply
* Re: [PATCH v20 1/6] exec: Add a new AT_CHECK flag to execveat(2)
From: Amir Goldstein @ 2024-10-13 9:25 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Al Viro, Christian Brauner, Kees Cook, Linus Torvalds, Paul Moore,
Serge Hallyn, Theodore Ts'o, Adhemerval Zanella Netto,
Alejandro Colomar, Aleksa Sarai, Andrew Morton, Andy Lutomirski,
Arnd Bergmann, Casey Schaufler, Christian Heimes, Dmitry Vyukov,
Elliott Hughes, Eric Biggers, Eric Chiang, Fan Wu, Florian Weimer,
Geert Uytterhoeven, James Morris, Jan Kara, Jann Horn, Jeff Xu,
Jonathan Corbet, Jordan R Abrahams, Lakshmi Ramasubramanian,
Luca Boccassi, Luis Chamberlain, Madhavan T . Venkataraman,
Matt Bobrowski, Matthew Garrett, Matthew Wilcox, Miklos Szeredi,
Mimi Zohar, Nicolas Bouchinet, Scott Shell, Shuah Khan,
Stephen Rothwell, Steve Dower, Steve Grubb, Thibaut Sautereau,
Vincent Strubel, Xiaoming Ni, Yin Fengwei, kernel-hardening,
linux-api, linux-fsdevel, linux-integrity, linux-kernel,
linux-security-module
In-Reply-To: <20241011184422.977903-2-mic@digikod.net>
On Fri, Oct 11, 2024 at 8:45 PM Mickaël Salaün <mic@digikod.net> wrote:
>
> Add a new AT_CHECK flag to execveat(2) to check if a file would be
> allowed for execution. The main use case is for script interpreters and
> dynamic linkers to check execution permission according to the kernel's
> security policy. Another use case is to add context to access logs e.g.,
> which script (instead of interpreter) accessed a file. As any
> executable code, scripts could also use this check [1].
>
> This is different from faccessat(2) + X_OK which only checks a subset of
> access rights (i.e. inode permission and mount options for regular
> files), but not the full context (e.g. all LSM access checks). The main
> use case for access(2) is for SUID processes to (partially) check access
> on behalf of their caller. The main use case for execveat(2) + AT_CHECK
> is to check if a script execution would be allowed, according to all the
> different restrictions in place. Because the use of AT_CHECK follows
> the exact kernel semantic as for a real execution, user space gets the
> same error codes.
>
> An interesting point of using execveat(2) instead of openat2(2) is that
> it decouples the check from the enforcement. Indeed, the security check
> can be logged (e.g. with audit) without blocking an execution
> environment not yet ready to enforce a strict security policy.
>
> LSMs can control or log execution requests with
> security_bprm_creds_for_exec(). However, to enforce a consistent and
> complete access control (e.g. on binary's dependencies) LSMs should
> restrict file executability, or mesure executed files, with
> security_file_open() by checking file->f_flags & __FMODE_EXEC.
>
> Because AT_CHECK is dedicated to user space interpreters, it doesn't
> make sense for the kernel to parse the checked files, look for
> interpreters known to the kernel (e.g. ELF, shebang), and return ENOEXEC
> if the format is unknown. Because of that, security_bprm_check() is
> never called when AT_CHECK is used.
>
> It should be noted that script interpreters cannot directly use
> execveat(2) (without this new AT_CHECK flag) because this could lead to
> unexpected behaviors e.g., `python script.sh` could lead to Bash being
> executed to interpret the script. Unlike the kernel, script
> interpreters may just interpret the shebang as a simple comment, which
> should not change for backward compatibility reasons.
>
> Because scripts or libraries files might not currently have the
> executable permission set, or because we might want specific users to be
> allowed to run arbitrary scripts, the following patch provides a dynamic
> configuration mechanism with the SECBIT_EXEC_RESTRICT_FILE and
> SECBIT_EXEC_DENY_INTERACTIVE securebits.
>
> This is a redesign of the CLIP OS 4's O_MAYEXEC:
> https://github.com/clipos-archive/src_platform_clip-patches/blob/f5cb330d6b684752e403b4e41b39f7004d88e561/1901_open_mayexec.patch
> This patch has been used for more than a decade with customized script
> interpreters. Some examples can be found here:
> https://github.com/clipos-archive/clipos4_portage-overlay/search?q=O_MAYEXEC
>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Cc: Christian Brauner <brauner@kernel.org>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Paul Moore <paul@paul-moore.com>
> Cc: Serge Hallyn <serge@hallyn.com>
> Link: https://docs.python.org/3/library/io.html#io.open_code [1]
> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> Link: https://lore.kernel.org/r/20241011184422.977903-2-mic@digikod.net
> ---
>
> Changes since v19:
> * Remove mention of "role transition" as suggested by Andy.
> * Highlight the difference between security_bprm_creds_for_exec() and
> the __FMODE_EXEC check for LSMs (in commit message and LSM's hooks) as
> discussed with Jeff.
> * Improve documentation both in UAPI comments and kernel comments
> (requested by Kees).
>
> New design since v18:
> https://lore.kernel.org/r/20220104155024.48023-3-mic@digikod.net
> ---
> fs/exec.c | 18 ++++++++++++++++--
> include/linux/binfmts.h | 7 ++++++-
> include/uapi/linux/fcntl.h | 31 +++++++++++++++++++++++++++++++
> kernel/audit.h | 1 +
> kernel/auditsc.c | 1 +
> security/security.c | 10 ++++++++++
> 6 files changed, 65 insertions(+), 3 deletions(-)
>
> diff --git a/fs/exec.c b/fs/exec.c
> index 6c53920795c2..163c659d9ae6 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -891,7 +891,7 @@ static struct file *do_open_execat(int fd, struct filename *name, int flags)
> .lookup_flags = LOOKUP_FOLLOW,
> };
>
> - if ((flags & ~(AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH)) != 0)
> + if ((flags & ~(AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH | AT_CHECK)) != 0)
> return ERR_PTR(-EINVAL);
> if (flags & AT_SYMLINK_NOFOLLOW)
> open_exec_flags.lookup_flags &= ~LOOKUP_FOLLOW;
> @@ -1545,6 +1545,20 @@ static struct linux_binprm *alloc_bprm(int fd, struct filename *filename, int fl
> }
> bprm->interp = bprm->filename;
>
> + /*
> + * At this point, security_file_open() has already been called (with
> + * __FMODE_EXEC) and access control checks for AT_CHECK will stop just
> + * after the security_bprm_creds_for_exec() call in bprm_execve().
> + * Indeed, the kernel should not try to parse the content of the file
> + * with exec_binprm() nor change the calling thread, which means that
> + * the following security functions will be not called:
> + * - security_bprm_check()
> + * - security_bprm_creds_from_file()
> + * - security_bprm_committing_creds()
> + * - security_bprm_committed_creds()
> + */
> + bprm->is_check = !!(flags & AT_CHECK);
> +
> retval = bprm_mm_init(bprm);
> if (!retval)
> return bprm;
> @@ -1839,7 +1853,7 @@ static int bprm_execve(struct linux_binprm *bprm)
>
> /* Set the unchanging part of bprm->cred */
> retval = security_bprm_creds_for_exec(bprm);
> - if (retval)
> + if (retval || bprm->is_check)
> goto out;
>
> retval = exec_binprm(bprm);
> diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
> index e6c00e860951..8ff0eb3644a1 100644
> --- a/include/linux/binfmts.h
> +++ b/include/linux/binfmts.h
> @@ -42,7 +42,12 @@ struct linux_binprm {
> * Set when errors can no longer be returned to the
> * original userspace.
> */
> - point_of_no_return:1;
> + point_of_no_return:1,
> + /*
> + * Set by user space to check executability according to the
> + * caller's environment.
> + */
> + is_check:1;
> struct file *executable; /* Executable to pass to the interpreter */
> struct file *interpreter;
> struct file *file;
> diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h
> index 87e2dec79fea..e606815b1c5a 100644
> --- a/include/uapi/linux/fcntl.h
> +++ b/include/uapi/linux/fcntl.h
> @@ -154,6 +154,37 @@
> usable with open_by_handle_at(2). */
> #define AT_HANDLE_MNT_ID_UNIQUE 0x001 /* Return the u64 unique mount ID. */
>
> +/*
> + * AT_CHECK only performs a check on a regular file and returns 0 if execution
> + * of this file would be allowed, ignoring the file format and then the related
> + * interpreter dependencies (e.g. ELF libraries, script's shebang).
> + *
> + * Programs should always perform this check to apply kernel-level checks
> + * against files that are not directly executed by the kernel but passed to a
> + * user space interpreter instead. All files that contain executable code,
> + * from the point of view of the interpreter, should be checked. However the
> + * result of this check should only be enforced according to
> + * SECBIT_EXEC_RESTRICT_FILE or SECBIT_EXEC_DENY_INTERACTIVE. See securebits.h
> + * documentation and the samples/check-exec/inc.c example.
> + *
> + * The main purpose of this flag is to improve the security and consistency of
> + * an execution environment to ensure that direct file execution (e.g.
> + * `./script.sh`) and indirect file execution (e.g. `sh script.sh`) lead to the
> + * same result. For instance, this can be used to check if a file is
> + * trustworthy according to the caller's environment.
> + *
> + * In a secure environment, libraries and any executable dependencies should
> + * also be checked. For instance, dynamic linking should make sure that all
> + * libraries are allowed for execution to avoid trivial bypass (e.g. using
> + * LD_PRELOAD). For such secure execution environment to make sense, only
> + * trusted code should be executable, which also requires integrity guarantees.
> + *
> + * To avoid race conditions leading to time-of-check to time-of-use issues,
> + * AT_CHECK should be used with AT_EMPTY_PATH to check against a file
> + * descriptor instead of a path.
> + */
If you ask me, the very elaborate comment above belongs to execveat(2)
man page and is way too verbose for a uapi header.
> +#define AT_CHECK 0x10000
Please see the comment "Per-syscall flags for the *at(2) family of syscalls."
above. If this is a per-syscall flag please use one of the per-syscall
flags, e.g.:
/* Flags for execveat2(2) */
#define AT_EXECVE_CHECK 0x0001 /* Only perform a check if
execution would be allowed */
Thanks,
Amir.
^ permalink raw reply
* Re: [PATCH v20 1/6] exec: Add a new AT_CHECK flag to execveat(2)
From: Serge E. Hallyn @ 2024-10-13 3:04 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Al Viro, Christian Brauner, Kees Cook, Linus Torvalds, Paul Moore,
Serge Hallyn, Theodore Ts'o, Adhemerval Zanella Netto,
Alejandro Colomar, Aleksa Sarai, Andrew Morton, Andy Lutomirski,
Arnd Bergmann, Casey Schaufler, Christian Heimes, Dmitry Vyukov,
Elliott Hughes, Eric Biggers, Eric Chiang, Fan Wu, Florian Weimer,
Geert Uytterhoeven, James Morris, Jan Kara, Jann Horn, Jeff Xu,
Jonathan Corbet, Jordan R Abrahams, Lakshmi Ramasubramanian,
Luca Boccassi, Luis Chamberlain, Madhavan T . Venkataraman,
Matt Bobrowski, Matthew Garrett, Matthew Wilcox, Miklos Szeredi,
Mimi Zohar, Nicolas Bouchinet, Scott Shell, Shuah Khan,
Stephen Rothwell, Steve Dower, Steve Grubb, Thibaut Sautereau,
Vincent Strubel, Xiaoming Ni, Yin Fengwei, kernel-hardening,
linux-api, linux-fsdevel, linux-integrity, linux-kernel,
linux-security-module
In-Reply-To: <20241011184422.977903-2-mic@digikod.net>
On Fri, Oct 11, 2024 at 08:44:17PM +0200, Mickaël Salaün wrote:
> Add a new AT_CHECK flag to execveat(2) to check if a file would be
Apologies for both bikeshedding and missing earlier discussions.
But AT_CHECK sounds quite generic. How about AT_EXEC_CHECK, or
AT_CHECK_EXEC_CREDS? (I would suggest just AT_CHECK_CREDS since
it's for use in execveat(2), but as it's an AT_ flag, it's
probably worth being more precise).
^ permalink raw reply
* Re: [PATCH v20 2/6] security: Add EXEC_RESTRICT_FILE and EXEC_DENY_INTERACTIVE securebits
From: Serge E. Hallyn @ 2024-10-13 2:51 UTC (permalink / raw)
To: Mickaël Salaün
Cc: Al Viro, Christian Brauner, Kees Cook, Linus Torvalds, Paul Moore,
Serge Hallyn, Theodore Ts'o, Adhemerval Zanella Netto,
Alejandro Colomar, Aleksa Sarai, Andrew Morton, Andy Lutomirski,
Arnd Bergmann, Casey Schaufler, Christian Heimes, Dmitry Vyukov,
Elliott Hughes, Eric Biggers, Eric Chiang, Fan Wu, Florian Weimer,
Geert Uytterhoeven, James Morris, Jan Kara, Jann Horn, Jeff Xu,
Jonathan Corbet, Jordan R Abrahams, Lakshmi Ramasubramanian,
Luca Boccassi, Luis Chamberlain, Madhavan T . Venkataraman,
Matt Bobrowski, Matthew Garrett, Matthew Wilcox, Miklos Szeredi,
Mimi Zohar, Nicolas Bouchinet, Scott Shell, Shuah Khan,
Stephen Rothwell, Steve Dower, Steve Grubb, Thibaut Sautereau,
Vincent Strubel, Xiaoming Ni, Yin Fengwei, kernel-hardening,
linux-api, linux-fsdevel, linux-integrity, linux-kernel,
linux-security-module, Andy Lutomirski
In-Reply-To: <20241011184422.977903-3-mic@digikod.net>
On Fri, Oct 11, 2024 at 08:44:18PM +0200, Mickaël Salaün wrote:
> The new SECBIT_EXEC_RESTRICT_FILE, SECBIT_EXEC_DENY_INTERACTIVE, and
> their *_LOCKED counterparts are designed to be set by processes setting
> up an execution environment, such as a user session, a container, or a
> security sandbox. Unlike other securebits, these ones can be set by
> unprivileged processes. Like seccomp filters or Landlock domains, the
> securebits are inherited across processes.
>
> When SECBIT_EXEC_RESTRICT_FILE is set, programs interpreting code should
> control executable resources according to execveat(2) + AT_CHECK (see
> previous commit).
>
> When SECBIT_EXEC_DENY_INTERACTIVE is set, a process should deny
> execution of user interactive commands (which excludes executable
> regular files).
>
> Being able to configure each of these securebits enables system
> administrators or owner of image containers to gradually validate the
> related changes and to identify potential issues (e.g. with interpreter
> or audit logs).
>
> It should be noted that unlike other security bits, the
> SECBIT_EXEC_RESTRICT_FILE and SECBIT_EXEC_DENY_INTERACTIVE bits are
> dedicated to user space willing to restrict itself. Because of that,
> they only make sense in the context of a trusted environment (e.g.
> sandbox, container, user session, full system) where the process
> changing its behavior (according to these bits) and all its parent
> processes are trusted. Otherwise, any parent process could just execute
> its own malicious code (interpreting a script or not), or even enforce a
> seccomp filter to mask these bits.
>
> Such a secure environment can be achieved with an appropriate access
> control (e.g. mount's noexec option, file access rights, LSM policy) and
> an enlighten ld.so checking that libraries are allowed for execution
> e.g., to protect against illegitimate use of LD_PRELOAD.
>
> Ptrace restrictions according to these securebits would not make sense
> because of the processes' trust assumption.
>
> Scripts may need some changes to deal with untrusted data (e.g. stdin,
> environment variables), but that is outside the scope of the kernel.
>
> See chromeOS's documentation about script execution control and the
> related threat model:
> https://www.chromium.org/chromium-os/developer-library/guides/security/noexec-shell-scripts/
>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Cc: Andy Lutomirski <luto@amacapital.net>
> Cc: Christian Brauner <brauner@kernel.org>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Paul Moore <paul@paul-moore.com>
> Cc: Serge Hallyn <serge@hallyn.com>
> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> Link: https://lore.kernel.org/r/20241011184422.977903-3-mic@digikod.net
> ---
>
> Changes since v19:
> * Replace SECBIT_SHOULD_EXEC_CHECK and SECBIT_SHOULD_EXEC_RESTRICT with
> SECBIT_EXEC_RESTRICT_FILE and SECBIT_EXEC_DENY_INTERACTIVE:
> https://lore.kernel.org/all/20240710.eiKohpa4Phai@digikod.net/
> * Remove the ptrace restrictions, suggested by Andy.
> * Improve documentation according to the discussion with Jeff.
>
> New design since v18:
> https://lore.kernel.org/r/20220104155024.48023-3-mic@digikod.net
> ---
> include/uapi/linux/securebits.h | 113 +++++++++++++++++++++++++++++++-
> security/commoncap.c | 29 ++++++--
> 2 files changed, 135 insertions(+), 7 deletions(-)
>
> diff --git a/include/uapi/linux/securebits.h b/include/uapi/linux/securebits.h
> index d6d98877ff1a..351b6ecefc76 100644
> --- a/include/uapi/linux/securebits.h
> +++ b/include/uapi/linux/securebits.h
> @@ -52,10 +52,121 @@
> #define SECBIT_NO_CAP_AMBIENT_RAISE_LOCKED \
> (issecure_mask(SECURE_NO_CAP_AMBIENT_RAISE_LOCKED))
>
> +/*
> + * The SECBIT_EXEC_RESTRICT_FILE and SECBIT_EXEC_DENY_INTERACTIVE securebits
> + * are intended for script interpreters and dynamic linkers to enforce a
> + * consistent execution security policy handled by the kernel.
> + *
> + * Whether an interpreter should check these securebits or not depends on the
> + * security risk of running malicious scripts with respect to the execution
> + * environment, and whether the kernel can check if a script is trustworthy or
> + * not. For instance, Python scripts running on a server can use arbitrary
> + * syscalls and access arbitrary files. Such interpreters should then be
> + * enlighten to use these securebits and let users define their security
> + * policy. However, a JavaScript engine running in a web browser should
> + * already be sandboxed and then should not be able to harm the user's
> + * environment.
> + *
> + * When SECBIT_EXEC_RESTRICT_FILE is set, a process should only interpret or
> + * execute a file if a call to execveat(2) with the related file descriptor and
> + * the AT_CHECK flag succeed.
> + *
> + * This secure bit may be set by user session managers, service managers,
> + * container runtimes, sandboxer tools... Except for test environments, the
> + * related SECBIT_EXEC_RESTRICT_FILE_LOCKED bit should also be set.
> + *
> + * Programs should only enforce consistent restrictions according to the
> + * securebits but without relying on any other user-controlled configuration.
> + * Indeed, the use case for these securebits is to only trust executable code
> + * vetted by the system configuration (through the kernel), so we should be
> + * careful to not let untrusted users control this configuration.
> + *
> + * However, script interpreters may still use user configuration such as
> + * environment variables as long as it is not a way to disable the securebits
> + * checks. For instance, the PATH and LD_PRELOAD variables can be set by a
> + * script's caller. Changing these variables may lead to unintended code
> + * executions, but only from vetted executable programs, which is OK. For this
> + * to make sense, the system should provide a consistent security policy to
> + * avoid arbitrary code execution e.g., by enforcing a write xor execute
> + * policy.
> + *
> + * SECBIT_EXEC_RESTRICT_FILE is complementary and should also be checked.
> + */
> +#define SECURE_EXEC_RESTRICT_FILE 8
> +#define SECURE_EXEC_RESTRICT_FILE_LOCKED 9 /* make bit-8 immutable */
> +
> +#define SECBIT_EXEC_RESTRICT_FILE (issecure_mask(SECURE_EXEC_RESTRICT_FILE))
> +#define SECBIT_EXEC_RESTRICT_FILE_LOCKED \
> + (issecure_mask(SECURE_EXEC_RESTRICT_FILE_LOCKED))
> +
> +/*
> + * When SECBIT_EXEC_DENY_INTERACTIVE is set, a process should never interpret
> + * interactive user commands (e.g. scripts). However, if such commands are
> + * passed through a file descriptor (e.g. stdin), its content should be
> + * interpreted if a call to execveat(2) with the related file descriptor and
> + * the AT_CHECK flag succeed.
> + *
> + * For instance, script interpreters called with a script snippet as argument
> + * should always deny such execution if SECBIT_EXEC_DENY_INTERACTIVE is set.
> + *
> + * This secure bit may be set by user session managers, service managers,
> + * container runtimes, sandboxer tools... Except for test environments, the
> + * related SECBIT_EXEC_DENY_INTERACTIVE_LOCKED bit should also be set.
> + *
> + * See the SECBIT_EXEC_RESTRICT_FILE documentation.
> + *
> + * Here is the expected behavior for a script interpreter according to
> + * combination of any exec securebits:
> + *
> + * 1. SECURE_EXEC_RESTRICT_FILE=0 SECURE_EXEC_DENY_INTERACTIVE=0 (default)
> + * Always interpret scripts, and allow arbitrary user commands.
> + * => No threat, everyone and everything is trusted, but we can get ahead of
> + * potential issues thanks to the call to execveat with AT_CHECK which
> + * should always be performed but ignored by the script interpreter.
> + * Indeed, this check is still important to enable systems administrators
> + * to verify requests (e.g. with audit) and prepare for migration to a
> + * secure mode.
> + *
> + * 2. SECURE_EXEC_RESTRICT_FILE=1 SECURE_EXEC_DENY_INTERACTIVE=0
> + * Deny script interpretation if they are not executable, but allow
> + * arbitrary user commands.
> + * => The threat is (potential) malicious scripts run by trusted (and not
> + * fooled) users. That can protect against unintended script executions
> + * (e.g. sh /tmp/*.sh). This makes sense for (semi-restricted) user
> + * sessions.
> + *
> + * 3. SECURE_EXEC_RESTRICT_FILE=0 SECURE_EXEC_DENY_INTERACTIVE=1
> + * Always interpret scripts, but deny arbitrary user commands.
> + * => This use case may be useful for secure services (i.e. without
> + * interactive user session) where scripts' integrity is verified (e.g.
> + * with IMA/EVM or dm-verity/IPE) but where access rights might not be
> + * ready yet. Indeed, arbitrary interactive commands would be much more
> + * difficult to check.
> + *
> + * 4. SECURE_EXEC_RESTRICT_FILE=1 SECURE_EXEC_DENY_INTERACTIVE=1
> + * Deny script interpretation if they are not executable, and also deny
> + * any arbitrary user commands.
> + * => The threat is malicious scripts run by untrusted users (but trusted
> + * code). This makes sense for system services that may only execute
> + * trusted scripts.
> + */
> +#define SECURE_EXEC_DENY_INTERACTIVE 10
> +#define SECURE_EXEC_DENY_INTERACTIVE_LOCKED 11 /* make bit-10 immutable */
> +
> +#define SECBIT_EXEC_DENY_INTERACTIVE \
> + (issecure_mask(SECURE_EXEC_DENY_INTERACTIVE))
> +#define SECBIT_EXEC_DENY_INTERACTIVE_LOCKED \
> + (issecure_mask(SECURE_EXEC_DENY_INTERACTIVE_LOCKED))
> +
> #define SECURE_ALL_BITS (issecure_mask(SECURE_NOROOT) | \
> issecure_mask(SECURE_NO_SETUID_FIXUP) | \
> issecure_mask(SECURE_KEEP_CAPS) | \
> - issecure_mask(SECURE_NO_CAP_AMBIENT_RAISE))
> + issecure_mask(SECURE_NO_CAP_AMBIENT_RAISE) | \
> + issecure_mask(SECURE_EXEC_RESTRICT_FILE) | \
> + issecure_mask(SECURE_EXEC_DENY_INTERACTIVE))
> #define SECURE_ALL_LOCKS (SECURE_ALL_BITS << 1)
>
> +#define SECURE_ALL_UNPRIVILEGED (issecure_mask(SECURE_EXEC_RESTRICT_FILE) | \
> + issecure_mask(SECURE_EXEC_DENY_INTERACTIVE))
> +
> #endif /* _UAPI_LINUX_SECUREBITS_H */
> diff --git a/security/commoncap.c b/security/commoncap.c
> index cefad323a0b1..52ea01acb453 100644
> --- a/security/commoncap.c
> +++ b/security/commoncap.c
> @@ -1302,21 +1302,38 @@ int cap_task_prctl(int option, unsigned long arg2, unsigned long arg3,
> & (old->securebits ^ arg2)) /*[1]*/
> || ((old->securebits & SECURE_ALL_LOCKS & ~arg2)) /*[2]*/
> || (arg2 & ~(SECURE_ALL_LOCKS | SECURE_ALL_BITS)) /*[3]*/
> - || (cap_capable(current_cred(),
> - current_cred()->user_ns,
> - CAP_SETPCAP,
> - CAP_OPT_NONE) != 0) /*[4]*/
> /*
> * [1] no changing of bits that are locked
> * [2] no unlocking of locks
> * [3] no setting of unsupported bits
> - * [4] doing anything requires privilege (go read about
> - * the "sendmail capabilities bug")
> */
> )
> /* cannot change a locked bit */
> return -EPERM;
>
> + /*
> + * Doing anything requires privilege (go read about the
> + * "sendmail capabilities bug"), except for unprivileged bits.
> + * Indeed, the SECURE_ALL_UNPRIVILEGED bits are not
> + * restrictions enforced by the kernel but by user space on
> + * itself.
> + */
> + if (cap_capable(current_cred(), current_cred()->user_ns,
> + CAP_SETPCAP, CAP_OPT_NONE) != 0) {
> + const unsigned long unpriv_and_locks =
> + SECURE_ALL_UNPRIVILEGED |
> + SECURE_ALL_UNPRIVILEGED << 1;
> + const unsigned long changed = old->securebits ^ arg2;
> +
> + /* For legacy reason, denies non-change. */
> + if (!changed)
> + return -EPERM;
This is odd to me. You say for legacy reasons, but, currently, calling
PR_SET_SECUREBITS with no changes returns 0. So you may be breaking
a lot of programs here, unless I'm mistaken.
> +
> + /* Denies privileged changes. */
> + if (changed & ~unpriv_and_locks)
> + return -EPERM;
> + }
> +
> new = prepare_creds();
> if (!new)
> return -ENOMEM;
> --
> 2.46.1
^ permalink raw reply
* [PATCH v20 6/6] samples/check-exec: Add an enlighten "inc" interpreter and 28 tests
From: Mickaël Salaün @ 2024-10-11 18:44 UTC (permalink / raw)
To: Al Viro, Christian Brauner, Kees Cook, Linus Torvalds, Paul Moore,
Serge Hallyn, Theodore Ts'o
Cc: Mickaël Salaün, Adhemerval Zanella Netto,
Alejandro Colomar, Aleksa Sarai, Andrew Morton, Andy Lutomirski,
Arnd Bergmann, Casey Schaufler, Christian Heimes, Dmitry Vyukov,
Elliott Hughes, Eric Biggers, Eric Chiang, Fan Wu, Florian Weimer,
Geert Uytterhoeven, James Morris, Jan Kara, Jann Horn, Jeff Xu,
Jonathan Corbet, Jordan R Abrahams, Lakshmi Ramasubramanian,
Luca Boccassi, Luis Chamberlain, Madhavan T . Venkataraman,
Matt Bobrowski, Matthew Garrett, Matthew Wilcox, Miklos Szeredi,
Mimi Zohar, Nicolas Bouchinet, Scott Shell, Shuah Khan,
Stephen Rothwell, Steve Dower, Steve Grubb, Thibaut Sautereau,
Vincent Strubel, Xiaoming Ni, Yin Fengwei, kernel-hardening,
linux-api, linux-fsdevel, linux-integrity, linux-kernel,
linux-security-module
In-Reply-To: <20241011184422.977903-1-mic@digikod.net>
Add a very simple script interpreter called "inc" that can evaluate two
different commands (one per line):
- "?" to initialize a counter from user's input;
- "+" to increment the counter (which is set to 0 by default).
It is enlighten to only interpret executable files according to AT_CHECK
and the related securebits:
# Executing a script with RESTRICT_FILE is only allowed if the script
# is exectuable:
./set-exec -f -- ./inc script-exec.inc # Allowed
./set-exec -f -- ./inc script-noexec.inc # Denied
# Executing stdin with DENY_INTERACTIVE is only allowed if stdin is an
# executable regular file:
./set-exec -i -- ./inc -i < script-exec.inc # Allowed
./set-exec -i -- ./inc -i < script-noexec.inc # Denied
# However, a pipe is not executable and it is then denied:
cat script-noexec.inc | ./set-exec -i -- ./inc -i # Denied
# Executing raw data (e.g. command argument) with DENY_INTERACTIVE is
# always denied.
./set-exec -i -- ./inc -c "+" # Denied
./inc -c "$(<script-ask.inc)" # Allowed
# To directly execute a script, we can update $PATH (used by `env`):
PATH="${PATH}:." ./script-exec.inc
# To execute several commands passed as argument:
Add a complete test suite to check the script interpreter against all
possible execution cases:
make TARGETS=exec kselftest-install
./tools/testing/selftests/kselftest_install/run_kselftest.sh
Fix ktap_helpers.sh to gracefully ignore optional argument.
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Christian Brauner <brauner@kernel.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Paul Moore <paul@paul-moore.com>
Cc: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
Link: https://lore.kernel.org/r/20241011184422.977903-7-mic@digikod.net
---
Changes since v19:
* New patch.
---
samples/Kconfig | 3 +-
samples/check-exec/.gitignore | 1 +
samples/check-exec/Makefile | 1 +
samples/check-exec/inc.c | 204 +++++++++++++++++
samples/check-exec/run-script-ask.inc | 8 +
samples/check-exec/script-ask.inc | 4 +
samples/check-exec/script-exec.inc | 3 +
samples/check-exec/script-noexec.inc | 3 +
tools/testing/selftests/exec/.gitignore | 2 +
tools/testing/selftests/exec/Makefile | 14 +-
.../selftests/exec/check-exec-tests.sh | 205 ++++++++++++++++++
.../selftests/kselftest/ktap_helpers.sh | 2 +-
12 files changed, 446 insertions(+), 4 deletions(-)
create mode 100644 samples/check-exec/inc.c
create mode 100755 samples/check-exec/run-script-ask.inc
create mode 100755 samples/check-exec/script-ask.inc
create mode 100755 samples/check-exec/script-exec.inc
create mode 100644 samples/check-exec/script-noexec.inc
create mode 100755 tools/testing/selftests/exec/check-exec-tests.sh
diff --git a/samples/Kconfig b/samples/Kconfig
index efa28ceadc42..0128cc68deed 100644
--- a/samples/Kconfig
+++ b/samples/Kconfig
@@ -296,7 +296,8 @@ config SAMPLE_CHECK_EXEC
depends on CC_CAN_LINK && HEADERS_INSTALL
help
Build a tool to easily configure SECBIT_EXEC_RESTRICT_FILE and
- SECBIT_EXEC_DENY_INTERACTIVE.
+ SECBIT_EXEC_DENY_INTERACTIVE, and a simple script interpreter to
+ demonstrate how they should be used with execveat(2) + AT_CHECK.
source "samples/rust/Kconfig"
diff --git a/samples/check-exec/.gitignore b/samples/check-exec/.gitignore
index 3f8119112ccf..cd759a19dacd 100644
--- a/samples/check-exec/.gitignore
+++ b/samples/check-exec/.gitignore
@@ -1 +1,2 @@
+/inc
/set-exec
diff --git a/samples/check-exec/Makefile b/samples/check-exec/Makefile
index d9f976e3ff98..c4f08ad0f8e3 100644
--- a/samples/check-exec/Makefile
+++ b/samples/check-exec/Makefile
@@ -1,6 +1,7 @@
# SPDX-License-Identifier: BSD-3-Clause
userprogs-always-y := \
+ inc \
set-exec
userccflags += -I usr/include
diff --git a/samples/check-exec/inc.c b/samples/check-exec/inc.c
new file mode 100644
index 000000000000..0710a860f360
--- /dev/null
+++ b/samples/check-exec/inc.c
@@ -0,0 +1,204 @@
+// SPDX-License-Identifier: BSD-3-Clause
+/*
+ * Very simple script interpreter that can evaluate two different commands (one
+ * per line):
+ * - "?" to initialize a counter from user's input;
+ * - "+" to increment the counter (which is set to 0 by default).
+ *
+ * See tools/testing/selftests/exec/check-exec-tests.sh
+ *
+ * Copyright © 2024 Microsoft Corporation
+ */
+
+#define _GNU_SOURCE
+#include <errno.h>
+#include <linux/fcntl.h>
+#include <linux/prctl.h>
+#include <linux/securebits.h>
+#include <stdbool.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/prctl.h>
+#include <unistd.h>
+
+/* Returns 1 on error, 0 otherwise. */
+static int interpret_buffer(char *buffer, size_t buffer_size)
+{
+ char *line, *saveptr = NULL;
+ long long number = 0;
+
+ /* Each command is the first character of a line. */
+ saveptr = NULL;
+ line = strtok_r(buffer, "\n", &saveptr);
+ while (line) {
+ if (*line != '#' && strlen(line) != 1) {
+ fprintf(stderr, "# ERROR: Unknown string\n");
+ return 1;
+ }
+ switch (*line) {
+ case '#':
+ /* Skips shebang and comments. */
+ break;
+ case '+':
+ /* Increments and prints the number. */
+ number++;
+ printf("%lld\n", number);
+ break;
+ case '?':
+ /* Reads integer from stdin. */
+ fprintf(stderr, "> Enter new number: \n");
+ if (scanf("%lld", &number) != 1) {
+ fprintf(stderr,
+ "# WARNING: Failed to read number from stdin\n");
+ }
+ break;
+ default:
+ fprintf(stderr, "# ERROR: Unknown character '%c'\n",
+ *line);
+ return 1;
+ }
+ line = strtok_r(NULL, "\n", &saveptr);
+ }
+ return 0;
+}
+
+/* Returns 1 on error, 0 otherwise. */
+static int interpret_stream(FILE *script, char *const script_name,
+ char *const *const envp, const bool restrict_stream)
+{
+ int err;
+ char *const script_argv[] = { script_name, NULL };
+ char buf[128] = {};
+ size_t buf_size = sizeof(buf);
+
+ /*
+ * We pass a valid argv and envp to the kernel to emulate a native
+ * script execution. We must use the script file descriptor instead of
+ * the script path name to avoid race conditions.
+ */
+ err = execveat(fileno(script), "", script_argv, envp,
+ AT_EMPTY_PATH | AT_CHECK);
+ if (err && restrict_stream) {
+ perror("ERROR: Script execution check");
+ return 1;
+ }
+
+ /* Reads script. */
+ buf_size = fread(buf, 1, buf_size - 1, script);
+ return interpret_buffer(buf, buf_size);
+}
+
+static void print_usage(const char *argv0)
+{
+ fprintf(stderr, "usage: %s <script.inc> | -i | -c <command>\n\n",
+ argv0);
+ fprintf(stderr, "Example:\n");
+ fprintf(stderr, " ./set-exec -fi -- ./inc -i < script-exec.inc\n");
+}
+
+int main(const int argc, char *const argv[], char *const *const envp)
+{
+ int opt;
+ char *cmd = NULL;
+ char *script_name = NULL;
+ bool interpret_stdin = false;
+ FILE *script_file = NULL;
+ int secbits;
+ bool deny_interactive, restrict_file;
+ size_t arg_nb;
+
+ secbits = prctl(PR_GET_SECUREBITS);
+ if (secbits == -1) {
+ /*
+ * This should never happen, except with a buggy seccomp
+ * filter.
+ */
+ perror("ERROR: Failed to get securebits");
+ return 1;
+ }
+
+ deny_interactive = !!(secbits & SECBIT_EXEC_DENY_INTERACTIVE);
+ restrict_file = !!(secbits & SECBIT_EXEC_RESTRICT_FILE);
+
+ while ((opt = getopt(argc, argv, "c:i")) != -1) {
+ switch (opt) {
+ case 'c':
+ if (cmd) {
+ fprintf(stderr, "ERROR: Command already set");
+ return 1;
+ }
+ cmd = optarg;
+ break;
+ case 'i':
+ interpret_stdin = true;
+ break;
+ default:
+ print_usage(argv[0]);
+ return 1;
+ }
+ }
+
+ /* Checks that only one argument is used, or read stdin. */
+ arg_nb = !!cmd + !!interpret_stdin;
+ if (arg_nb == 0 && argc == 2) {
+ script_name = argv[1];
+ } else if (arg_nb != 1) {
+ print_usage(argv[0]);
+ return 1;
+ }
+
+ if (cmd) {
+ /*
+ * Other kind of interactive interpretations should be denied
+ * as well (e.g. CLI arguments passing script snippets,
+ * environment variables interpreted as script). However, any
+ * way to pass script files should only be restricted according
+ * to restrict_file.
+ */
+ if (deny_interactive) {
+ fprintf(stderr,
+ "ERROR: Interactive interpretation denied.\n");
+ return 1;
+ }
+
+ return interpret_buffer(cmd, strlen(cmd));
+ }
+
+ if (interpret_stdin && !script_name) {
+ script_file = stdin;
+ /*
+ * As for any execve(2) call, this path may be logged by the
+ * kernel.
+ */
+ script_name = "/proc/self/fd/0";
+ /*
+ * When stdin is used, it can point to a regular file or a
+ * pipe. Restrict stdin execution according to
+ * SECBIT_EXEC_DENY_INTERACTIVE but always allow executable
+ * files (which are not considered as interactive inputs).
+ */
+ return interpret_stream(script_file, script_name, envp,
+ deny_interactive);
+ } else if (script_name && !interpret_stdin) {
+ /*
+ * In this sample, we don't pass any argument to scripts, but
+ * otherwise we would have to forge an argv with such
+ * arguments.
+ */
+ script_file = fopen(script_name, "r");
+ if (!script_file) {
+ perror("ERROR: Failed to open script");
+ return 1;
+ }
+ /*
+ * Restricts file execution according to
+ * SECBIT_EXEC_RESTRICT_FILE.
+ */
+ return interpret_stream(script_file, script_name, envp,
+ restrict_file);
+ }
+
+ print_usage(argv[0]);
+ return 1;
+}
diff --git a/samples/check-exec/run-script-ask.inc b/samples/check-exec/run-script-ask.inc
new file mode 100755
index 000000000000..3ea3e15fbd5a
--- /dev/null
+++ b/samples/check-exec/run-script-ask.inc
@@ -0,0 +1,8 @@
+#!/usr/bin/env sh
+
+DIR="$(dirname -- "$0")"
+
+PATH="${PATH}:${DIR}"
+
+set -x
+"${DIR}/script-ask.inc"
diff --git a/samples/check-exec/script-ask.inc b/samples/check-exec/script-ask.inc
new file mode 100755
index 000000000000..f48252ab07c1
--- /dev/null
+++ b/samples/check-exec/script-ask.inc
@@ -0,0 +1,4 @@
+#!/usr/bin/env inc
+
+?
++
diff --git a/samples/check-exec/script-exec.inc b/samples/check-exec/script-exec.inc
new file mode 100755
index 000000000000..525e958e1c20
--- /dev/null
+++ b/samples/check-exec/script-exec.inc
@@ -0,0 +1,3 @@
+#!/usr/bin/env inc
+
++
diff --git a/samples/check-exec/script-noexec.inc b/samples/check-exec/script-noexec.inc
new file mode 100644
index 000000000000..525e958e1c20
--- /dev/null
+++ b/samples/check-exec/script-noexec.inc
@@ -0,0 +1,3 @@
+#!/usr/bin/env inc
+
++
diff --git a/tools/testing/selftests/exec/.gitignore b/tools/testing/selftests/exec/.gitignore
index a32c63bb4df1..7f3d1ae762ec 100644
--- a/tools/testing/selftests/exec/.gitignore
+++ b/tools/testing/selftests/exec/.gitignore
@@ -11,9 +11,11 @@ non-regular
null-argv
/check-exec
/false
+/inc
/load_address.*
!load_address.c
/recursion-depth
+/set-exec
xxxxxxxx*
pipe
S_I*.test
diff --git a/tools/testing/selftests/exec/Makefile b/tools/testing/selftests/exec/Makefile
index 8713d1c862ae..45a3cfc435cf 100644
--- a/tools/testing/selftests/exec/Makefile
+++ b/tools/testing/selftests/exec/Makefile
@@ -10,9 +10,9 @@ ALIGN_PIES := $(patsubst %,load_address.%,$(ALIGNS))
ALIGN_STATIC_PIES := $(patsubst %,load_address.static.%,$(ALIGNS))
ALIGNMENT_TESTS := $(ALIGN_PIES) $(ALIGN_STATIC_PIES)
-TEST_PROGS := binfmt_script.py
+TEST_PROGS := binfmt_script.py check-exec-tests.sh
TEST_GEN_PROGS := execveat non-regular $(ALIGNMENT_TESTS)
-TEST_GEN_PROGS_EXTENDED := false
+TEST_GEN_PROGS_EXTENDED := false inc set-exec script-exec.inc script-noexec.inc
TEST_GEN_FILES := execveat.symlink execveat.denatured script subdir
# Makefile is a run-time dependency, since it's accessed by the execveat test
TEST_FILES := Makefile
@@ -26,6 +26,8 @@ EXTRA_CLEAN := $(OUTPUT)/subdir.moved $(OUTPUT)/execveat.moved $(OUTPUT)/xxxxx*
include ../lib.mk
+CHECK_EXEC_SAMPLES := $(top_srcdir)/samples/check-exec
+
$(OUTPUT)/subdir:
mkdir -p $@
$(OUTPUT)/script: Makefile
@@ -45,3 +47,11 @@ $(OUTPUT)/load_address.static.0x%: load_address.c
-fPIE -static-pie $< -o $@
$(OUTPUT)/false: false.c
$(CC) $(CFLAGS) $(LDFLAGS) -static $< -o $@
+$(OUTPUT)/inc: $(CHECK_EXEC_SAMPLES)/inc.c
+ $(CC) $(CFLAGS) $(LDFLAGS) $< -o $@
+$(OUTPUT)/set-exec: $(CHECK_EXEC_SAMPLES)/set-exec.c
+ $(CC) $(CFLAGS) $(LDFLAGS) $< -o $@
+$(OUTPUT)/script-exec.inc: $(CHECK_EXEC_SAMPLES)/script-exec.inc
+ cp $< $@
+$(OUTPUT)/script-noexec.inc: $(CHECK_EXEC_SAMPLES)/script-noexec.inc
+ cp $< $@
diff --git a/tools/testing/selftests/exec/check-exec-tests.sh b/tools/testing/selftests/exec/check-exec-tests.sh
new file mode 100755
index 000000000000..87102906ae3c
--- /dev/null
+++ b/tools/testing/selftests/exec/check-exec-tests.sh
@@ -0,0 +1,205 @@
+#!/usr/bin/env bash
+# SPDX-License-Identifier: GPL-2.0
+#
+# Test the "inc" interpreter.
+#
+# See include/uapi/linux/securebits.h, include/uapi/linux/fcntl.h and
+# samples/check-exec/inc.c
+#
+# Copyright © 2024 Microsoft Corporation
+
+set -u -e -o pipefail
+
+EXPECTED_OUTPUT="1"
+exec 2>/dev/null
+
+DIR="$(dirname $(readlink -f "$0"))"
+source "${DIR}"/../kselftest/ktap_helpers.sh
+
+exec_direct() {
+ local expect="$1"
+ local script="$2"
+ shift 2
+ local ret=0
+ local out
+
+ # Updates PATH for `env` to execute the `inc` interpreter.
+ out="$(PATH="." "$@" "${script}")" || ret=$?
+
+ if [[ ${ret} -ne ${expect} ]]; then
+ echo "ERROR: Wrong expectation for direct file execution: ${ret}"
+ return 1
+ fi
+ if [[ ${ret} -eq 0 && "${out}" != "${EXPECTED_OUTPUT}" ]]; then
+ echo "ERROR: Wrong output for direct file execution: ${out}"
+ return 1
+ fi
+}
+
+exec_indirect() {
+ local expect="$1"
+ local script="$2"
+ shift 2
+ local ret=0
+ local out
+
+ # Script passed as argument.
+ out="$("$@" ./inc "${script}")" || ret=$?
+
+ if [[ ${ret} -ne ${expect} ]]; then
+ echo "ERROR: Wrong expectation for indirect file execution: ${ret}"
+ return 1
+ fi
+ if [[ ${ret} -eq 0 && "${out}" != "${EXPECTED_OUTPUT}" ]]; then
+ echo "ERROR: Wrong output for indirect file execution: ${out}"
+ return 1
+ fi
+}
+
+exec_stdin_reg() {
+ local expect="$1"
+ local script="$2"
+ shift 2
+ local ret=0
+ local out
+
+ # Executing stdin must be allowed if the related file is executable.
+ out="$("$@" ./inc -i < "${script}")" || ret=$?
+
+ if [[ ${ret} -ne ${expect} ]]; then
+ echo "ERROR: Wrong expectation for stdin regular file execution: ${ret}"
+ return 1
+ fi
+ if [[ ${ret} -eq 0 && "${out}" != "${EXPECTED_OUTPUT}" ]]; then
+ echo "ERROR: Wrong output for stdin regular file execution: ${out}"
+ return 1
+ fi
+}
+
+exec_stdin_pipe() {
+ local expect="$1"
+ shift
+ local ret=0
+ local out
+
+ # A pipe is not executable.
+ out="$(cat script-exec.inc | "$@" ./inc -i)" || ret=$?
+
+ if [[ ${ret} -ne ${expect} ]]; then
+ echo "ERROR: Wrong expectation for stdin pipe execution: ${ret}"
+ return 1
+ fi
+}
+
+exec_argument() {
+ local expect="$1"
+ local ret=0
+ shift
+ local out
+
+ # Script not coming from a file must not be executed.
+ out="$("$@" ./inc -c "$(< script-exec.inc)")" || ret=$?
+
+ if [[ ${ret} -ne ${expect} ]]; then
+ echo "ERROR: Wrong expectation for arbitrary argument execution: ${ret}"
+ return 1
+ fi
+ if [[ ${ret} -eq 0 && "${out}" != "${EXPECTED_OUTPUT}" ]]; then
+ echo "ERROR: Wrong output for arbitrary argument execution: ${out}"
+ return 1
+ fi
+}
+
+exec_interactive() {
+ exec_stdin_pipe "$@"
+ exec_argument "$@"
+}
+
+ktap_test() {
+ ktap_test_result "$*" "$@"
+}
+
+ktap_print_header
+ktap_set_plan 28
+
+# Without secbit configuration, nothing is changed.
+
+ktap_print_msg "By default, executable scripts are allowed to be interpreted and executed."
+ktap_test exec_direct 0 script-exec.inc
+ktap_test exec_indirect 0 script-exec.inc
+
+ktap_print_msg "By default, executable stdin is allowed to be interpreted."
+ktap_test exec_stdin_reg 0 script-exec.inc
+
+ktap_print_msg "By default, non-executable scripts are allowed to be interpreted, but not directly executed."
+# We get 126 because of direct execution by Bash.
+ktap_test exec_direct 126 script-noexec.inc
+ktap_test exec_indirect 0 script-noexec.inc
+
+ktap_print_msg "By default, non-executable stdin is allowed to be interpreted."
+ktap_test exec_stdin_reg 0 script-noexec.inc
+
+ktap_print_msg "By default, interactive commands are allowed to be interpreted."
+ktap_test exec_interactive 0
+
+# With only file restriction: protect non-malicious users from inadvertent errors (e.g. python ~/Downloads/*.py).
+
+ktap_print_msg "With -f, executable scripts are allowed to be interpreted and executed."
+ktap_test exec_direct 0 script-exec.inc ./set-exec -f --
+ktap_test exec_indirect 0 script-exec.inc ./set-exec -f --
+
+ktap_print_msg "With -f, executable stdin is allowed to be interpreted."
+ktap_test exec_stdin_reg 0 script-exec.inc ./set-exec -f --
+
+ktap_print_msg "With -f, non-executable scripts are not allowed to be executed nor interpreted."
+# Direct execution of non-executable script is alwayse denied by the kernel.
+ktap_test exec_direct 1 script-noexec.inc ./set-exec -f --
+ktap_test exec_indirect 1 script-noexec.inc ./set-exec -f --
+
+ktap_print_msg "With -f, non-executable stdin is allowed to be interpreted."
+ktap_test exec_stdin_reg 0 script-noexec.inc ./set-exec -f --
+
+ktap_print_msg "With -f, interactive commands are allowed to be interpreted."
+ktap_test exec_interactive 0 ./set-exec -f --
+
+# With only denied interactive commands: check or monitor script content (e.g. with LSM).
+
+ktap_print_msg "With -i, executable scripts are allowed to be interpreted and executed."
+ktap_test exec_direct 0 script-exec.inc ./set-exec -i --
+ktap_test exec_indirect 0 script-exec.inc ./set-exec -i --
+
+ktap_print_msg "With -i, executable stdin is allowed to be interpreted."
+ktap_test exec_stdin_reg 0 script-exec.inc ./set-exec -i --
+
+ktap_print_msg "With -i, non-executable scripts are allowed to be interpreted, but not directly executed."
+# Direct execution of non-executable script is alwayse denied by the kernel.
+ktap_test exec_direct 1 script-noexec.inc ./set-exec -i --
+ktap_test exec_indirect 0 script-noexec.inc ./set-exec -i --
+
+ktap_print_msg "With -i, non-executable stdin is not allowed to be interpreted."
+ktap_test exec_stdin_reg 1 script-noexec.inc ./set-exec -i --
+
+ktap_print_msg "With -i, interactive commands are not allowed to be interpreted."
+ktap_test exec_interactive 1 ./set-exec -i --
+
+# With both file restriction and denied interactive commands: only allow executable scripts.
+
+ktap_print_msg "With -fi, executable scripts are allowed to be interpreted and executed."
+ktap_test exec_direct 0 script-exec.inc ./set-exec -fi --
+ktap_test exec_indirect 0 script-exec.inc ./set-exec -fi --
+
+ktap_print_msg "With -fi, executable stdin is allowed to be interpreted."
+ktap_test exec_stdin_reg 0 script-exec.inc ./set-exec -fi --
+
+ktap_print_msg "With -fi, non-executable scripts are not allowed to be interpreted nor executed."
+# Direct execution of non-executable script is alwayse denied by the kernel.
+ktap_test exec_direct 1 script-noexec.inc ./set-exec -fi --
+ktap_test exec_indirect 1 script-noexec.inc ./set-exec -fi --
+
+ktap_print_msg "With -fi, non-executable stdin is not allowed to be interpreted."
+ktap_test exec_stdin_reg 1 script-noexec.inc ./set-exec -fi --
+
+ktap_print_msg "With -fi, interactive commands are not allowed to be interpreted."
+ktap_test exec_interactive 1 ./set-exec -fi --
+
+ktap_finished
diff --git a/tools/testing/selftests/kselftest/ktap_helpers.sh b/tools/testing/selftests/kselftest/ktap_helpers.sh
index 79a125eb24c2..14e7f3ec3f84 100644
--- a/tools/testing/selftests/kselftest/ktap_helpers.sh
+++ b/tools/testing/selftests/kselftest/ktap_helpers.sh
@@ -40,7 +40,7 @@ ktap_skip_all() {
__ktap_test() {
result="$1"
description="$2"
- directive="$3" # optional
+ directive="${3:-}" # optional
local directive_str=
[ ! -z "$directive" ] && directive_str="# $directive"
--
2.46.1
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox