* [PATCH v20 2/6] security: Add EXEC_RESTRICT_FILE and EXEC_DENY_INTERACTIVE securebits
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, Andy Lutomirski
In-Reply-To: <20241011184422.977903-1-mic@digikod.net>
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;
+
+ /* Denies privileged changes. */
+ if (changed & ~unpriv_and_locks)
+ return -EPERM;
+ }
+
new = prepare_creds();
if (!new)
return -ENOMEM;
--
2.46.1
^ permalink raw reply related
* [PATCH v20 1/6] exec: Add a new AT_CHECK flag to execveat(2)
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 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.
+ */
+#define AT_CHECK 0x10000
+
#if defined(__KERNEL__)
#define AT_GETATTR_NOSEC 0x80000000
#endif
diff --git a/kernel/audit.h b/kernel/audit.h
index a60d2840559e..8ebdabd2ab81 100644
--- a/kernel/audit.h
+++ b/kernel/audit.h
@@ -197,6 +197,7 @@ struct audit_context {
struct open_how openat2;
struct {
int argc;
+ bool is_check;
} execve;
struct {
char *name;
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index cd57053b4a69..8d9ba5600cf2 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -2662,6 +2662,7 @@ void __audit_bprm(struct linux_binprm *bprm)
context->type = AUDIT_EXECVE;
context->execve.argc = bprm->argc;
+ context->execve.is_check = bprm->is_check;
}
diff --git a/security/security.c b/security/security.c
index 6875eb4a59fc..2f7d2c6949d7 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1248,6 +1248,12 @@ int security_vm_enough_memory_mm(struct mm_struct *mm, long pages)
* to 1 if AT_SECURE should be set to request libc enable secure mode. @bprm
* contains the linux_binprm structure.
*
+ * If execveat(2) is called with the AT_CHECK flag, bprm->is_check is set. The
+ * result must be the same as without this flag even if the execution will
+ * never really happen and @bprm will always be dropped.
+ *
+ * This hook must not change current->cred, only @bprm->cred.
+ *
* Return: Returns 0 if the hook is successful and permission is granted.
*/
int security_bprm_creds_for_exec(struct linux_binprm *bprm)
@@ -3098,6 +3104,10 @@ int security_file_receive(struct file *file)
* Save open-time permission checking state for later use upon file_permission,
* and recheck access if anything has changed since inode_permission.
*
+ * We can check if a file is opened for execution (e.g. execve(2) call), either
+ * directly or indirectly (e.g. ELF's ld.so) by checking file->f_flags &
+ * __FMODE_EXEC .
+ *
* Return: Returns 0 if permission is granted.
*/
int security_file_open(struct file *file)
--
2.46.1
^ permalink raw reply related
* [PATCH v20 0/6] Script execution control (was O_MAYEXEC)
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
Hi,
The ultimate goal of this patch series is to be able to ensure that
direct file execution (e.g. ./script.sh) and indirect file execution
(e.g. sh script.sh) lead to the same result, especially from a security
point of view.
The main changes from the previous version are new securebits with a
better semantic, an improved documentation, new user space code examples
including an enlighten interpreter and an extensive tests suite. As
discussed, I also removed the ptrace checks which doesn't make sense
anymore. You'll now find a quick FAQ at the end of this cover letter.
The current status is summarized in this article:
https://lwn.net/Articles/982085/
I also gave a talk at LPC last month:
https://lpc.events/event/18/contributions/1692/
And here is a proof of concept for Python (for now, for the previous
version: v19): https://github.com/zooba/spython/pull/12
Overview
--------
This patch series is a new approach of the initial O_MAYEXEC feature,
and a revamp of the previous patch series. Taking into account the last
reviews [1], we now stick to the kernel semantic for file executability.
One major change is the clear split between access check and policy
management.
The first patch brings the AT_CHECK flag to execveat(2). The goal is to
enable user space to check if a file could be executed (by the kernel).
Unlike stat(2) that only checks file permissions, execveat2(2) +
AT_CHECK take into account the full context, including mount points
(noexec), caller's limits, and all potential LSM extra checks (e.g.
argv, envp, credentials).
The second patch brings two new securebits used to set or get a security
policy for a set of processes. For this to be meaningful, all
executable code needs to be trusted. In practice, this means that
(malicious) users can be restricted to only run scripts provided (and
trusted) by the system.
[1] https://lore.kernel.org/r/CAHk-=wjPGNLyzeBMWdQu+kUdQLHQugznwY7CvWjmvNW47D5sog@mail.gmail.com
Script execution
----------------
One important thing to keep in mind is that the goal of this patch
series is to get the same security restrictions with these commands:
* ./script.py
* python script.py
* python < script.py
* python -m script.py
However, on secure systems, we should be able to forbid these commands
because there is no way to reliably identify the origin of the script:
* xargs -a script.py -d '\r' -- python -c
* cat script.py | python
* python
Background
----------
Compared to the previous patch series, there is no more dedicated
syscall nor sysctl configuration. This new patch series only add new
flags: one for execveat(2) and four for prctl(2).
This kind of script interpreter restriction may already be used in
hardened systems, which may need to fork interpreters and install
different versions of the binaries. This mechanism should enable to
avoid the use of duplicate binaries (and potential forked source code)
for secure interpreters (e.g. secure Python [2]) by making it possible
to dynamically enforce restrictions or not.
The ability to control script execution is also required to close a
major IMA measurement/appraisal interpreter integrity [3].
This new execveat + AT_CHECK should not be confused with the O_EXEC flag
(for open) which is intended for execute-only, which obviously doesn't
work for scripts.
I gave a talk about controlling script execution where I explain the
previous approaches [4]. The design of the WIP RFC I talked about
changed quite a bit since then.
[2] https://github.com/zooba/spython
[3] https://lore.kernel.org/lkml/20211014130125.6991-1-zohar@linux.ibm.com/
[4] https://lssna2023.sched.com/event/1K7bO
Execution policy
----------------
The "execution" usage means that the content of the file descriptor is
trusted according to the system policy to be executed by user space,
which means that it interprets the content or (try to) maps it as
executable memory.
It is important to note that this can only enable to extend access
control managed by the kernel. Hence it enables current access control
mechanism to be extended and become a superset of what they can
currently control. Indeed, the security policy could also be delegated
to an LSM, either a MAC system or an integrity system.
Complementary W^X protections can be brought by SELinux or IPE [5].
Being able to restrict execution also enables to protect the kernel by
restricting arbitrary syscalls that an attacker could perform with a
crafted binary or certain script languages. It also improves multilevel
isolation by reducing the ability of an attacker to use side channels
with specific code. These restrictions can natively be enforced for ELF
binaries (with the noexec mount option) but require this kernel
extension to properly handle scripts (e.g. Python, Perl). To get a
consistent execution policy, additional memory restrictions should also
be enforced (e.g. thanks to SELinux).
[5] https://lore.kernel.org/lkml/1716583609-21790-1-git-send-email-wufan@linux.microsoft.com/
Prerequisite for security use
-----------------------------
Because scripts might not currently have the executable permission and
still run well as is, or because we might want specific users to be
allowed to run arbitrary scripts, we also need a configuration
mechanism.
According to the threat model, to get a secure execution environment on
top of these changes, it might be required to configure and enable
existing security mechanisms such as secure boot, restrictive mount
points (e.g. with rw AND noexec), correct file permissions (including
executable libraries), IMA/EVM, SELinux policy...
The first thing to patch is the libc to check loaded libraries (e.g. see
chromeOS changes). The second thing to patch are the script
interpreters by checking direct scripts executability and by checking
their own libraries (e.g. Python's imported files or argument-passed
modules). For instance, the PEP 578 [6] (Runtime Audit Hooks) enables
Python 3.8 to be extended with policy enforcement points related to code
interpretation, which can be used to align with the PowerShell audit
features. Additional Python security improvements (e.g. a limited
interpreter without -c, stdin piping of code) are developed [2] [7].
[6] https://www.python.org/dev/peps/pep-0578/
[7] https://lore.kernel.org/lkml/0c70debd-e79e-d514-06c6-4cd1e021fa8b@python.org/
libc patch
----------
Dynamic linking needs still need to check the libraries the same way
interpreters need to check scripts.
chromeOS patches glibc with a fstatvfs check [8] [9]. This enables to
check against noexec mount points, which is OK but doesn't fit with
execve semantics. Moreover, the kernel is not aware of such check, so
all access control checks are not performed (e.g. file permission, LSMs
security policies, integrity and authenticity checks), it is not handled
with audit, and more importantly this would not work on generic
distributions because of the strict requirement and chromeOS-specific
assumptions.
[8] https://issuetracker.google.com/issues/40054993
[9] https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/6abfc9e327241a5f684b8b941c899b7ca8b6dbc1/sys-libs/glibc/files/local/glibc-2.37/0007-Deny-LD_PRELOAD-of-files-in-NOEXEC-mount.patch
Examples
--------
The initial idea comes from CLIP OS 4 and the original implementation
has been used for more than a decade:
https://github.com/clipos-archive/clipos4_doc
Chrome OS has a similar approach:
https://www.chromium.org/chromium-os/developer-library/guides/security/noexec-shell-scripts/
User space patches can be found here:
https://github.com/clipos-archive/clipos4_portage-overlay/search?q=O_MAYEXEC
There is more than the O_MAYEXEC changes (which matches this search)
e.g., to prevent Python interactive execution. There are patches for
Bash, Wine, Java (Icedtea), Busybox's ash, Perl and Python. There are
also some related patches which do not directly rely on O_MAYEXEC but
which restrict the use of browser plugins and extensions, which may be
seen as scripts too:
https://github.com/clipos-archive/clipos4_portage-overlay/tree/master/www-client
Past talks and articles
-----------------------
Closing the script execution control gap at Linux Plumbers Conference
2024: https://lpc.events/event/18/contributions/1692/
An introduction to O_MAYEXEC was given at the Linux Security Summit
Europe 2018 - Linux Kernel Security Contributions by ANSSI:
https://www.youtube.com/watch?v=chNjCRtPKQY&t=17m15s
The "write xor execute" principle was explained at Kernel Recipes 2018 -
CLIP OS: a defense-in-depth OS:
https://www.youtube.com/watch?v=PjRE0uBtkHU&t=11m14s
LWN articles:
* https://lwn.net/Articles/982085/
* https://lwn.net/Articles/832959/
* https://lwn.net/Articles/820000/
FAQ
Link: https://lore.kernel.org/r/20241011184422.977903-1-mic@digikod.net
---
Q: Why not extend open(2) or openat2(2) with a new flag like O_MAYEXEC?
A: Because it is not flexible enough:
https://lore.kernel.org/r/CAG48ez0NAV5gPgmbDaSjo=zzE=FgnYz=-OHuXwu0Vts=B5gesA@mail.gmail.com
Q: Why not only allowing file descriptor to avoid TOCTOU?
A: Because there are different use cases:
https://lore.kernel.org/r/CAHk-=whb=XuU=LGKnJWaa7LOYQz9VwHs8SLfgLbT5sf2VAbX1A@mail.gmail.com
Q: We can copy a script into a memfd and use it as an executable FD.
Wouldn't that bypass the purpose of this patch series?
A: If an attacker can create a memfd it means that a
malicious/compromised code is already running and it's too late for
script execution control to help. This patch series makes it more
difficult for an attacker to execute arbitrary code on a trusted
system in the first place:
https://lore.kernel.org/all/20240717.AGh2shahc9ee@digikod.net/
Q: What about ROP?
A: See previous answer. If ROP is exploited then the attacker already
controls some code:
https://lore.kernel.org/all/20240718.ahph4che5Shi@digikod.net/
Q: What about LD_PRELOAD environment variable?
A: The dynamic linker should be enlighten to check if libraries are
allowed to be loaded.
Q: What about The PATH environment variable?
A: All programs allowed to be executed are deemed trusted.
Q: Should we check seccomp filters too?
A: Yes, they should be considered as executable code because they can
change the behavior of processes, similarly to code injection:
https://lore.kernel.org/all/20240705.IeTheequ7Ooj@digikod.net/
Q: Could that be used for role transition?
A: That would be risky and difficult to implement correctly:
https://lore.kernel.org/all/20240723.Tae5oovie2ah@digikod.net/
Previous versions
-----------------
v19: https://lore.kernel.org/r/20240704190137.696169-1-mic@digikod.net
v18: https://lore.kernel.org/r/20220104155024.48023-1-mic@digikod.net
v17: https://lore.kernel.org/r/20211115185304.198460-1-mic@digikod.net
v16: https://lore.kernel.org/r/20211110190626.257017-1-mic@digikod.net
v15: https://lore.kernel.org/r/20211012192410.2356090-1-mic@digikod.net
v14: https://lore.kernel.org/r/20211008104840.1733385-1-mic@digikod.net
v13: https://lore.kernel.org/r/20211007182321.872075-1-mic@digikod.net
v12: https://lore.kernel.org/r/20201203173118.379271-1-mic@digikod.net
v11: https://lore.kernel.org/r/20201019164932.1430614-1-mic@digikod.net
v10: https://lore.kernel.org/r/20200924153228.387737-1-mic@digikod.net
v9: https://lore.kernel.org/r/20200910164612.114215-1-mic@digikod.net
v8: https://lore.kernel.org/r/20200908075956.1069018-1-mic@digikod.net
v7: https://lore.kernel.org/r/20200723171227.446711-1-mic@digikod.net
v6: https://lore.kernel.org/r/20200714181638.45751-1-mic@digikod.net
v5: https://lore.kernel.org/r/20200505153156.925111-1-mic@digikod.net
v4: https://lore.kernel.org/r/20200430132320.699508-1-mic@digikod.net
v3: https://lore.kernel.org/r/20200428175129.634352-1-mic@digikod.net
v2: https://lore.kernel.org/r/20190906152455.22757-1-mic@digikod.net
v1: https://lore.kernel.org/r/20181212081712.32347-1-mic@digikod.net
Regards,
Mickaël Salaün (6):
exec: Add a new AT_CHECK flag to execveat(2)
security: Add EXEC_RESTRICT_FILE and EXEC_DENY_INTERACTIVE securebits
selftests/exec: Add 32 tests for AT_CHECK and exec securebits
selftests/landlock: Add tests for execveat + AT_CHECK
samples/check-exec: Add set-exec
samples/check-exec: Add an enlighten "inc" interpreter and 28 tests
fs/exec.c | 18 +-
include/linux/binfmts.h | 7 +-
include/uapi/linux/fcntl.h | 31 ++
include/uapi/linux/securebits.h | 113 ++++-
kernel/audit.h | 1 +
kernel/auditsc.c | 1 +
samples/Kconfig | 8 +
samples/Makefile | 1 +
samples/check-exec/.gitignore | 2 +
samples/check-exec/Makefile | 15 +
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 +
samples/check-exec/set-exec.c | 85 ++++
security/commoncap.c | 29 +-
security/security.c | 10 +
tools/testing/selftests/exec/.gitignore | 4 +
tools/testing/selftests/exec/Makefile | 19 +-
.../selftests/exec/check-exec-tests.sh | 205 ++++++++
tools/testing/selftests/exec/check-exec.c | 446 ++++++++++++++++++
tools/testing/selftests/exec/config | 2 +
tools/testing/selftests/exec/false.c | 5 +
.../selftests/kselftest/ktap_helpers.sh | 2 +-
tools/testing/selftests/landlock/fs_test.c | 26 +
26 files changed, 1240 insertions(+), 12 deletions(-)
create mode 100644 samples/check-exec/.gitignore
create mode 100644 samples/check-exec/Makefile
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 100644 samples/check-exec/set-exec.c
create mode 100755 tools/testing/selftests/exec/check-exec-tests.sh
create mode 100644 tools/testing/selftests/exec/check-exec.c
create mode 100644 tools/testing/selftests/exec/config
create mode 100644 tools/testing/selftests/exec/false.c
base-commit: 8cf0b93919e13d1e8d4466eb4080a4c4d9d66d7b
--
2.46.1
^ permalink raw reply
* [PATCH v2 3/3] selftests: pidfd: add tests for PIDFD_SELF_*
From: Lorenzo Stoakes @ 2024-10-11 11:05 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
In-Reply-To: <cover.1728643714.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 v2 2/3] pidfd: add PIDFD_SELF_* sentinels to refer to own thread/process
From: Lorenzo Stoakes @ 2024-10-11 11:05 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
In-Reply-To: <cover.1728643714.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 v2 0/3] introduce PIDFD_SELF* sentinels
From: Lorenzo Stoakes @ 2024-10-11 11:05 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
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.
v2:
* Fix tests as reported by Shuah.
* Correct RFC version lore link.
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 | 22 +--
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, 341 insertions(+), 52 deletions(-)
--
2.46.2
^ permalink raw reply
* [PATCH v2 1/3] pidfd: extend pidfd_get_pid() and de-duplicate pid lookup
From: Lorenzo Stoakes @ 2024-10-11 11:05 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
In-Reply-To: <cover.1728643714.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 | 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;
}
@@ -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 related
* Re: [PATCH 3/3] selftests: pidfd: add tests for PIDFD_SELF_*
From: Lorenzo Stoakes @ 2024-10-11 8:20 UTC (permalink / raw)
To: Shuah Khan
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: <1d1190be-f74f-45ab-ac6c-2251d0bec1bc@linuxfoundation.org>
On Thu, Oct 10, 2024 at 05:16:22PM -0600, Shuah Khan wrote:
> On 10/10/24 12:15, Lorenzo Stoakes wrote:
> > 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 | 136 ++++++++++++++++++
> > .../selftests/pidfd/pidfd_setns_test.c | 11 ++
> > tools/testing/selftests/pidfd/pidfd_test.c | 67 +++++++--
> > 4 files changed, 213 insertions(+), 9 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
> > +
>
> Can't we pick these up from linux/pidfd.h - patch 2/3 adds
> them.
We're running this file in userland and it's not obvious we can correctly
import this header, it'd be some "../../" thing out of the testing root
directory and might not interact well with all scenarios in which this file
is built.
Also the existing tests do not seem to try to import that header, so it
seemed the safest way of doing this.
>
> > /*
> > * 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..10793fc845ed 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,89 @@ static int child(int sk)
> > return ret;
> > }
> > +static int __pidfd_self_thread_worker(unsigned long page_size)
> > +{
> > + int memfd;
> > + int newfd;
> > + char *ptr;
> > + int ret = 0;
> > +
> > + /*
> > + * Unshare our FDs so we have our own set. This means
> > + * PIDFD_SELF_THREAD_GROUP will fail.
> > + */
> > + if (unshare(CLONE_FILES) < 0) {
> > + ret = -errno;
> > + goto exit;
> > + }
> > +
> > + /* Truncate, map in and write to our memfd. */
> > + memfd = sys_memfd_create("test_self_child", 0);
>
> Missing eror check.
Ack, I had to rapidly change this code to not use the ASSERT_xxx() stuff
since abstracted out to helper function for pthread() to invoke and clearly
did not do so carefully enough :) thanks for pointing out will fix this +
other issues.
>
> > + if (ftruncate(memfd, page_size)) {
> > + ret = -errno;
> > + goto exit;
>
> Hmm. you probably need scoped cleanup paths. "exit" closes
> memfd and newfd which isn't open yet and sys_memfd_create()
> could fail and memfd doesn't need closing?
Yes... oops! Will fix.
>
> > + }
> > +
> > + ptr = mmap(NULL, page_size, PROT_READ | PROT_WRITE,
> > + MAP_SHARED, memfd, 0);
> > + if (ptr == MAP_FAILED) {
> > + ret = -errno;
> > + goto exit;
> > + }
> > + ptr[0] = 'y';
> > + if (munmap(ptr, page_size)) {
> > + ret = -errno;
> > + goto exit;
> > + }
> > +
> > + /* Get a thread-local duplicate of our memfd. */
> > + newfd = sys_pidfd_getfd(PIDFD_SELF_THREAD, memfd, 0);
> > + if (newfd < 0) {
> > + ret = -errno;
> > + goto exit;
>
> Same comment here - "exit" closes newfd
Ack will fix.
>
> > + }
> > +
> > + if (memfd == newfd) {
> > + ret = -EINVAL;
> > + goto exit;
> > + }
> > +
> > + /* 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) {
> > + ret = -errno;
> > + goto exit;
> > + }
> > +
> > + if (ptr[0] != 'y') {
> > + ret = -EINVAL;
> > + goto exit;
> > + }
> > +
> > + if (munmap(ptr, page_size)) {
> > + ret = -errno;
> > + goto exit;
> > + }
> > +
> > +exit:
> > + /* Cleanup. */
> > + close(newfd);
> > + close(memfd);
> > +
> > + return ret;
> > +}
> > +
> > +static void *pidfd_self_thread_worker(void *arg)
> > +{
> > + unsigned long page_size = (unsigned long)arg;
> > + int ret;
> > +
> > + ret = __pidfd_self_thread_worker(page_size);
>
> Don't you want to check error here?
We check it from callers, and it is passed to them we can't assert here as
we're not in a test.
>
> > +
> > + return (void *)(intptr_t)ret;
> > +}
> > +
> > FIXTURE(child)
> > {
> > /*
> > @@ -264,6 +349,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..ab5caa0368a1 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;
> > +
> > + ret = send_signal(pidfd);
> > +
>
> Same here - don't you have to check ret?
As in the other case, we check that in the caller rather than here. I will
add a comment to both to make it clear that we intentionally forward the
error.
>
> > + 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 res;
> > if (!have_pidfd_send_signal) {
> > ksft_test_result_skip(
> > @@ -74,17 +106,34 @@ static int test_pidfd_send_signal_simple_success(void)
> > signal(SIGUSR1, set_signal_received_on_sigusr1);
> > - ret = sys_pidfd_send_signal(pidfd, SIGUSR1, NULL, 0);
> > + send_signal(pidfd);
> > close(pidfd);
> > - if (ret < 0)
> > - ksft_exit_fail_msg("%s test: Failed to send signal\n",
> > +
> > + /* Now try the same thing only using PIDFD_SELF_THREAD_GROUP. */
> > + res = send_signal(PIDFD_SELF_THREAD_GROUP);
> > + if (res)
> > + ksft_exit_fail_msg(
> > + "%s test: Error %d on PIDFD_SELF_THREAD_GROUP signal\n",
> > + test_name, res);
> > +
> > + /*
> > + * 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);
> > + res = (int)(intptr_t)thread_res;
> > + if (res)
> > + ksft_exit_fail_msg(
> > + "%s test: Error %d on PIDFD_SELF_THREAD signal\n",
> > + test_name, res);
> > - signal_received = 0;
> > ksft_test_result_pass("%s test: Sent signal\n", test_name);
> > return 0;
> > }
>
> thanks,
> -- Shuah
Thanks for the review!
^ permalink raw reply
* Re: [PATCH 3/3] selftests: pidfd: add tests for PIDFD_SELF_*
From: Shuah Khan @ 2024-10-10 23:16 UTC (permalink / raw)
To: Lorenzo Stoakes, Christian Brauner
Cc: Shuah Khan, Liam R . Howlett, Suren Baghdasaryan, Vlastimil Babka,
pedro.falcato, linux-kselftest, linux-mm, linux-fsdevel,
linux-api, linux-kernel, Shuah Khan
In-Reply-To: <8917d809e1509c4e0bce02436a493db29e2115b3.1728578231.git.lorenzo.stoakes@oracle.com>
On 10/10/24 12:15, Lorenzo Stoakes wrote:
> 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 | 136 ++++++++++++++++++
> .../selftests/pidfd/pidfd_setns_test.c | 11 ++
> tools/testing/selftests/pidfd/pidfd_test.c | 67 +++++++--
> 4 files changed, 213 insertions(+), 9 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
> +
Can't we pick these up from linux/pidfd.h - patch 2/3 adds
them.
> /*
> * 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..10793fc845ed 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,89 @@ static int child(int sk)
> return ret;
> }
>
> +static int __pidfd_self_thread_worker(unsigned long page_size)
> +{
> + int memfd;
> + int newfd;
> + char *ptr;
> + int ret = 0;
> +
> + /*
> + * Unshare our FDs so we have our own set. This means
> + * PIDFD_SELF_THREAD_GROUP will fail.
> + */
> + if (unshare(CLONE_FILES) < 0) {
> + ret = -errno;
> + goto exit;
> + }
> +
> + /* Truncate, map in and write to our memfd. */
> + memfd = sys_memfd_create("test_self_child", 0);
Missing eror check.
> + if (ftruncate(memfd, page_size)) {
> + ret = -errno;
> + goto exit;
Hmm. you probably need scoped cleanup paths. "exit" closes
memfd and newfd which isn't open yet and sys_memfd_create()
could fail and memfd doesn't need closing?
> + }
> +
> + ptr = mmap(NULL, page_size, PROT_READ | PROT_WRITE,
> + MAP_SHARED, memfd, 0);
> + if (ptr == MAP_FAILED) {
> + ret = -errno;
> + goto exit;
> + }
> + ptr[0] = 'y';
> + if (munmap(ptr, page_size)) {
> + ret = -errno;
> + goto exit;
> + }
> +
> + /* Get a thread-local duplicate of our memfd. */
> + newfd = sys_pidfd_getfd(PIDFD_SELF_THREAD, memfd, 0);
> + if (newfd < 0) {
> + ret = -errno;
> + goto exit;
Same comment here - "exit" closes newfd
> + }
> +
> + if (memfd == newfd) {
> + ret = -EINVAL;
> + goto exit;
> + }
> +
> + /* 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) {
> + ret = -errno;
> + goto exit;
> + }
> +
> + if (ptr[0] != 'y') {
> + ret = -EINVAL;
> + goto exit;
> + }
> +
> + if (munmap(ptr, page_size)) {
> + ret = -errno;
> + goto exit;
> + }
> +
> +exit:
> + /* Cleanup. */
> + close(newfd);
> + close(memfd);
> +
> + return ret;
> +}
> +
> +static void *pidfd_self_thread_worker(void *arg)
> +{
> + unsigned long page_size = (unsigned long)arg;
> + int ret;
> +
> + ret = __pidfd_self_thread_worker(page_size);
Don't you want to check error here?
> +
> + return (void *)(intptr_t)ret;
> +}
> +
> FIXTURE(child)
> {
> /*
> @@ -264,6 +349,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..ab5caa0368a1 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;
> +
> + ret = send_signal(pidfd);
> +
Same here - don't you have to check ret?
> + 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 res;
>
> if (!have_pidfd_send_signal) {
> ksft_test_result_skip(
> @@ -74,17 +106,34 @@ static int test_pidfd_send_signal_simple_success(void)
>
> signal(SIGUSR1, set_signal_received_on_sigusr1);
>
> - ret = sys_pidfd_send_signal(pidfd, SIGUSR1, NULL, 0);
> + send_signal(pidfd);
> close(pidfd);
> - if (ret < 0)
> - ksft_exit_fail_msg("%s test: Failed to send signal\n",
> +
> + /* Now try the same thing only using PIDFD_SELF_THREAD_GROUP. */
> + res = send_signal(PIDFD_SELF_THREAD_GROUP);
> + if (res)
> + ksft_exit_fail_msg(
> + "%s test: Error %d on PIDFD_SELF_THREAD_GROUP signal\n",
> + test_name, res);
> +
> + /*
> + * 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);
> + res = (int)(intptr_t)thread_res;
> + if (res)
> + ksft_exit_fail_msg(
> + "%s test: Error %d on PIDFD_SELF_THREAD signal\n",
> + test_name, res);
>
> - signal_received = 0;
> ksft_test_result_pass("%s test: Sent signal\n", test_name);
> return 0;
> }
thanks,
-- Shuah
^ permalink raw reply
* [PATCH 3/3] selftests: pidfd: add tests for PIDFD_SELF_*
From: Lorenzo Stoakes @ 2024-10-10 18:15 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
In-Reply-To: <cover.1728578231.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 | 136 ++++++++++++++++++
.../selftests/pidfd/pidfd_setns_test.c | 11 ++
tools/testing/selftests/pidfd/pidfd_test.c | 67 +++++++--
4 files changed, 213 insertions(+), 9 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..10793fc845ed 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,89 @@ static int child(int sk)
return ret;
}
+static int __pidfd_self_thread_worker(unsigned long page_size)
+{
+ int memfd;
+ int newfd;
+ char *ptr;
+ int ret = 0;
+
+ /*
+ * Unshare our FDs so we have our own set. This means
+ * PIDFD_SELF_THREAD_GROUP will fail.
+ */
+ if (unshare(CLONE_FILES) < 0) {
+ ret = -errno;
+ goto exit;
+ }
+
+ /* Truncate, map in and write to our memfd. */
+ memfd = sys_memfd_create("test_self_child", 0);
+ if (ftruncate(memfd, page_size)) {
+ ret = -errno;
+ goto exit;
+ }
+
+ ptr = mmap(NULL, page_size, PROT_READ | PROT_WRITE,
+ MAP_SHARED, memfd, 0);
+ if (ptr == MAP_FAILED) {
+ ret = -errno;
+ goto exit;
+ }
+ ptr[0] = 'y';
+ if (munmap(ptr, page_size)) {
+ ret = -errno;
+ goto exit;
+ }
+
+ /* Get a thread-local duplicate of our memfd. */
+ newfd = sys_pidfd_getfd(PIDFD_SELF_THREAD, memfd, 0);
+ if (newfd < 0) {
+ ret = -errno;
+ goto exit;
+ }
+
+ if (memfd == newfd) {
+ ret = -EINVAL;
+ goto exit;
+ }
+
+ /* 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) {
+ ret = -errno;
+ goto exit;
+ }
+
+ if (ptr[0] != 'y') {
+ ret = -EINVAL;
+ goto exit;
+ }
+
+ if (munmap(ptr, page_size)) {
+ ret = -errno;
+ goto exit;
+ }
+
+exit:
+ /* Cleanup. */
+ close(newfd);
+ close(memfd);
+
+ return ret;
+}
+
+static void *pidfd_self_thread_worker(void *arg)
+{
+ unsigned long page_size = (unsigned long)arg;
+ int ret;
+
+ ret = __pidfd_self_thread_worker(page_size);
+
+ return (void *)(intptr_t)ret;
+}
+
FIXTURE(child)
{
/*
@@ -264,6 +349,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..ab5caa0368a1 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;
+
+ 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 res;
if (!have_pidfd_send_signal) {
ksft_test_result_skip(
@@ -74,17 +106,34 @@ static int test_pidfd_send_signal_simple_success(void)
signal(SIGUSR1, set_signal_received_on_sigusr1);
- ret = sys_pidfd_send_signal(pidfd, SIGUSR1, NULL, 0);
+ send_signal(pidfd);
close(pidfd);
- if (ret < 0)
- ksft_exit_fail_msg("%s test: Failed to send signal\n",
+
+ /* Now try the same thing only using PIDFD_SELF_THREAD_GROUP. */
+ res = send_signal(PIDFD_SELF_THREAD_GROUP);
+ if (res)
+ ksft_exit_fail_msg(
+ "%s test: Error %d on PIDFD_SELF_THREAD_GROUP signal\n",
+ test_name, res);
+
+ /*
+ * 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);
+ res = (int)(intptr_t)thread_res;
+ if (res)
+ ksft_exit_fail_msg(
+ "%s test: Error %d on PIDFD_SELF_THREAD signal\n",
+ test_name, res);
- signal_received = 0;
ksft_test_result_pass("%s test: Sent signal\n", test_name);
return 0;
}
--
2.46.2
^ permalink raw reply related
* [PATCH 2/3] pidfd: add PIDFD_SELF* sentinels to refer to own thread/process
From: Lorenzo Stoakes @ 2024-10-10 18:15 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
In-Reply-To: <cover.1728578231.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 1/3] pidfd: extend pidfd_get_pid() and de-duplicate pid lookup
From: Lorenzo Stoakes @ 2024-10-10 18:15 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
In-Reply-To: <cover.1728578231.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 | 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;
}
@@ -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 related
* [PATCH 0/3] introduce PIDFD_SELF* sentinels
From: Lorenzo Stoakes @ 2024-10-10 18:15 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
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.
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.
RFC version:
https://lore.kernel.org/linux-mm/1d19f18c-5a60-44b5-a96f-9d0e74f2b02c@lucifer.local/
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 | 22 +--
tools/testing/selftests/pidfd/pidfd.h | 8 ++
.../selftests/pidfd/pidfd_getfd_test.c | 136 ++++++++++++++++++
.../selftests/pidfd/pidfd_setns_test.c | 11 ++
tools/testing/selftests/pidfd/pidfd_test.c | 67 +++++++--
10 files changed, 330 insertions(+), 49 deletions(-)
--
2.46.2
^ permalink raw reply
* Re: (subset) [PATCH RFC v3 03/10] openat2: explicitly return -E2BIG for (usize > PAGE_SIZE)
From: Christian Brauner @ 2024-10-10 10:09 UTC (permalink / raw)
To: Aleksa Sarai
Cc: Christian Brauner, Kees Cook, Florian Weimer, Arnd Bergmann,
Mark Rutland, linux-kernel, linux-api, linux-fsdevel, linux-arch,
linux-kselftest, stable, Ingo Molnar, Peter Zijlstra, Juri Lelli,
Vincent Guittot, Dietmar Eggemann, Steven Rostedt, Ben Segall,
Mel Gorman, Valentin Schneider, Alexander Viro, Jan Kara,
Shuah Khan
In-Reply-To: <20241010-extensible-structs-check_fields-v3-3-d2833dfe6edd@cyphar.com>
On Thu, 10 Oct 2024 07:40:36 +1100, Aleksa Sarai wrote:
> While we do currently return -EFAULT in this case, it seems prudent to
> follow the behaviour of other syscalls like clone3. It seems quite
> unlikely that anyone depends on this error code being EFAULT, but we can
> always revert this if it turns out to be an issue.
>
>
Applied to the vfs.fixes branch of the vfs/vfs.git tree.
Patches in the vfs.fixes branch should appear in linux-next soon.
Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.
It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.
Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.
tree: https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.fixes
[03/10] openat2: explicitly return -E2BIG for (usize > PAGE_SIZE)
https://git.kernel.org/vfs/vfs/c/f92f0a1b0569
^ permalink raw reply
* Re: [PATCH RFC v3 00/10] extensible syscalls: CHECK_FIELDS to allow for easier feature detection
From: Florian Weimer @ 2024-10-10 6:26 UTC (permalink / raw)
To: Aleksa Sarai
Cc: Ingo Molnar, Peter Zijlstra, Juri Lelli, Vincent Guittot,
Dietmar Eggemann, Steven Rostedt, Ben Segall, Mel Gorman,
Valentin Schneider, Alexander Viro, Christian Brauner, Jan Kara,
Arnd Bergmann, Shuah Khan, Kees Cook, Florian Weimer,
Mark Rutland, linux-kernel, linux-api, linux-fsdevel, linux-arch,
linux-kselftest, stable
In-Reply-To: <20241010-extensible-structs-check_fields-v3-0-d2833dfe6edd@cyphar.com>
* Aleksa Sarai:
> This is something that I've been thinking about for a while. We had a
> discussion at LPC 2020 about this[1] but the proposals suggested there
> never materialised.
>
> In short, it is quite difficult for userspace to detect the feature
> capability of syscalls at runtime. This is something a lot of programs
> want to do, but they are forced to create elaborate scenarios to try to
> figure out if a feature is supported without causing damage to the
> system. For the vast majority of cases, each individual feature also
> needs to be tested individually (because syscall results are
> all-or-nothing), so testing even a single syscall's feature set can
> easily inflate the startup time of programs.
>
> This patchset implements the fairly minimal design I proposed in this
> talk[2] and in some old LKML threads (though I can't find the exact
> references ATM). The general flow looks like:
By the way, I have recently tried to document things from a glibc
perspective (which is a bit broader because we also have purely
userspace types):
[PATCH RFC] manual: Document how types change
<https://inbox.sourceware.org/libc-alpha/8734m4n1ij.fsf@oldenburg3.str.redhat.com/>
(This patch has not yet been reviewed.)
^ permalink raw reply
* Re: [PATCH RFC v3 03/10] openat2: explicitly return -E2BIG for (usize > PAGE_SIZE)
From: Greg KH @ 2024-10-10 6:24 UTC (permalink / raw)
To: Aleksa Sarai
Cc: Ingo Molnar, Peter Zijlstra, Juri Lelli, Vincent Guittot,
Dietmar Eggemann, Steven Rostedt, Ben Segall, Mel Gorman,
Valentin Schneider, Alexander Viro, Christian Brauner, Jan Kara,
Arnd Bergmann, Shuah Khan, Kees Cook, Florian Weimer,
Mark Rutland, linux-kernel, linux-api, linux-fsdevel, linux-arch,
linux-kselftest, stable
In-Reply-To: <20241010-extensible-structs-check_fields-v3-3-d2833dfe6edd@cyphar.com>
On Thu, Oct 10, 2024 at 07:40:36AM +1100, Aleksa Sarai wrote:
> While we do currently return -EFAULT in this case, it seems prudent to
> follow the behaviour of other syscalls like clone3. It seems quite
> unlikely that anyone depends on this error code being EFAULT, but we can
> always revert this if it turns out to be an issue.
>
> Cc: <stable@vger.kernel.org> # v5.6+
> Fixes: fddb5d430ad9 ("open: introduce openat2(2) syscall")
> Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
> ---
> fs/open.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/fs/open.c b/fs/open.c
> index 22adbef7ecc2..30bfcddd505d 100644
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -1458,6 +1458,8 @@ SYSCALL_DEFINE4(openat2, int, dfd, const char __user *, filename,
>
> if (unlikely(usize < OPEN_HOW_SIZE_VER0))
> return -EINVAL;
> + if (unlikely(usize > PAGE_SIZE))
> + return -E2BIG;
>
> err = copy_struct_from_user(&tmp, sizeof(tmp), how, usize);
> if (err)
>
> --
> 2.46.1
Why isn't this just sent as a normal fix to be included now and not
burried in a RFC series?
thanks,
greg k-h
^ permalink raw reply
* [PATCH RFC v3 10/10] selftests: mount_setattr: add CHECK_FIELDS selftest
From: Aleksa Sarai @ 2024-10-09 20:40 UTC (permalink / raw)
To: Ingo Molnar, Peter Zijlstra, Juri Lelli, Vincent Guittot,
Dietmar Eggemann, Steven Rostedt, Ben Segall, Mel Gorman,
Valentin Schneider, Alexander Viro, Christian Brauner, Jan Kara,
Arnd Bergmann, Shuah Khan
Cc: Kees Cook, Florian Weimer, Arnd Bergmann, Mark Rutland,
linux-kernel, linux-api, linux-fsdevel, linux-arch,
linux-kselftest, Aleksa Sarai
In-Reply-To: <20241010-extensible-structs-check_fields-v3-0-d2833dfe6edd@cyphar.com>
While we're at it -- to make testing for error codes with ASSERT_*
easier, make the sys_* wrappers return -errno in case of an error.
For some reason, the <sys/sysinfo.h> include doesn't correct import
<asm/posix_types.h>, leading to compilation errors -- so just import
<asm/posix_types.h> manually.
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
tools/include/uapi/asm-generic/posix_types.h | 101 +++++++++++++++++++++
tools/testing/selftests/mount_setattr/Makefile | 2 +-
.../selftests/mount_setattr/mount_setattr_test.c | 53 ++++++++++-
3 files changed, 153 insertions(+), 3 deletions(-)
diff --git a/tools/include/uapi/asm-generic/posix_types.h b/tools/include/uapi/asm-generic/posix_types.h
new file mode 100644
index 000000000000..b5f7594eee7a
--- /dev/null
+++ b/tools/include/uapi/asm-generic/posix_types.h
@@ -0,0 +1,101 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+#ifndef __ASM_GENERIC_POSIX_TYPES_H
+#define __ASM_GENERIC_POSIX_TYPES_H
+
+#include <asm/bitsperlong.h>
+/*
+ * This file is generally used by user-level software, so you need to
+ * be a little careful about namespace pollution etc.
+ *
+ * First the types that are often defined in different ways across
+ * architectures, so that you can override them.
+ */
+
+#ifndef __kernel_long_t
+typedef long __kernel_long_t;
+typedef unsigned long __kernel_ulong_t;
+#endif
+
+#ifndef __kernel_ino_t
+typedef __kernel_ulong_t __kernel_ino_t;
+#endif
+
+#ifndef __kernel_mode_t
+typedef unsigned int __kernel_mode_t;
+#endif
+
+#ifndef __kernel_pid_t
+typedef int __kernel_pid_t;
+#endif
+
+#ifndef __kernel_ipc_pid_t
+typedef int __kernel_ipc_pid_t;
+#endif
+
+#ifndef __kernel_uid_t
+typedef unsigned int __kernel_uid_t;
+typedef unsigned int __kernel_gid_t;
+#endif
+
+#ifndef __kernel_suseconds_t
+typedef __kernel_long_t __kernel_suseconds_t;
+#endif
+
+#ifndef __kernel_daddr_t
+typedef int __kernel_daddr_t;
+#endif
+
+#ifndef __kernel_uid32_t
+typedef unsigned int __kernel_uid32_t;
+typedef unsigned int __kernel_gid32_t;
+#endif
+
+#ifndef __kernel_old_uid_t
+typedef __kernel_uid_t __kernel_old_uid_t;
+typedef __kernel_gid_t __kernel_old_gid_t;
+#endif
+
+#ifndef __kernel_old_dev_t
+typedef unsigned int __kernel_old_dev_t;
+#endif
+
+/*
+ * Most 32 bit architectures use "unsigned int" size_t,
+ * and all 64 bit architectures use "unsigned long" size_t.
+ */
+#ifndef __kernel_size_t
+#if __BITS_PER_LONG != 64
+typedef unsigned int __kernel_size_t;
+typedef int __kernel_ssize_t;
+typedef int __kernel_ptrdiff_t;
+#else
+typedef __kernel_ulong_t __kernel_size_t;
+typedef __kernel_long_t __kernel_ssize_t;
+typedef __kernel_long_t __kernel_ptrdiff_t;
+#endif
+#endif
+
+#ifndef __kernel_fsid_t
+typedef struct {
+ int val[2];
+} __kernel_fsid_t;
+#endif
+
+/*
+ * anything below here should be completely generic
+ */
+typedef __kernel_long_t __kernel_off_t;
+typedef long long __kernel_loff_t;
+typedef __kernel_long_t __kernel_old_time_t;
+#ifndef __KERNEL__
+typedef __kernel_long_t __kernel_time_t;
+#endif
+typedef long long __kernel_time64_t;
+typedef __kernel_long_t __kernel_clock_t;
+typedef int __kernel_timer_t;
+typedef int __kernel_clockid_t;
+typedef char * __kernel_caddr_t;
+typedef unsigned short __kernel_uid16_t;
+typedef unsigned short __kernel_gid16_t;
+
+#endif /* __ASM_GENERIC_POSIX_TYPES_H */
diff --git a/tools/testing/selftests/mount_setattr/Makefile b/tools/testing/selftests/mount_setattr/Makefile
index 0c0d7b1234c1..3f260506fe60 100644
--- a/tools/testing/selftests/mount_setattr/Makefile
+++ b/tools/testing/selftests/mount_setattr/Makefile
@@ -1,6 +1,6 @@
# SPDX-License-Identifier: GPL-2.0
# Makefile for mount selftests.
-CFLAGS = -g $(KHDR_INCLUDES) -Wall -O2 -pthread
+CFLAGS = -g $(KHDR_INCLUDES) $(TOOLS_INCLUDES) -Wall -O2 -pthread
TEST_GEN_PROGS := mount_setattr_test
diff --git a/tools/testing/selftests/mount_setattr/mount_setattr_test.c b/tools/testing/selftests/mount_setattr/mount_setattr_test.c
index c6a8c732b802..36b8286e5f43 100644
--- a/tools/testing/selftests/mount_setattr/mount_setattr_test.c
+++ b/tools/testing/selftests/mount_setattr/mount_setattr_test.c
@@ -11,6 +11,7 @@
#include <sys/wait.h>
#include <sys/vfs.h>
#include <sys/statvfs.h>
+#include <asm/posix_types.h> /* get __kernel_* typedefs */
#include <sys/sysinfo.h>
#include <stdlib.h>
#include <unistd.h>
@@ -137,7 +138,8 @@
static inline int sys_mount_setattr(int dfd, const char *path, unsigned int flags,
struct mount_attr *attr, size_t size)
{
- return syscall(__NR_mount_setattr, dfd, path, flags, attr, size);
+ int ret = syscall(__NR_mount_setattr, dfd, path, flags, attr, size);
+ return ret >= 0 ? ret : -errno;
}
#ifndef OPEN_TREE_CLONE
@@ -152,9 +154,24 @@ static inline int sys_mount_setattr(int dfd, const char *path, unsigned int flag
#define AT_RECURSIVE 0x8000 /* Apply to the entire subtree */
#endif
+#define FSMOUNT_VALID_FLAGS \
+ (MOUNT_ATTR_RDONLY | MOUNT_ATTR_NOSUID | MOUNT_ATTR_NODEV | \
+ MOUNT_ATTR_NOEXEC | MOUNT_ATTR__ATIME | MOUNT_ATTR_NODIRATIME | \
+ MOUNT_ATTR_NOSYMFOLLOW)
+
+#define MOUNT_SETATTR_VALID_FLAGS (FSMOUNT_VALID_FLAGS | MOUNT_ATTR_IDMAP)
+
+#define MOUNT_SETATTR_PROPAGATION_FLAGS \
+ (MS_UNBINDABLE | MS_PRIVATE | MS_SLAVE | MS_SHARED)
+
+#ifndef CHECK_FIELDS
+#define CHECK_FIELDS (1ULL << 63)
+#endif
+
static inline int sys_open_tree(int dfd, const char *filename, unsigned int flags)
{
- return syscall(__NR_open_tree, dfd, filename, flags);
+ int ret = syscall(__NR_open_tree, dfd, filename, flags);
+ return ret >= 0 ? ret : -errno;
}
static ssize_t write_nointr(int fd, const void *buf, size_t count)
@@ -1497,4 +1514,36 @@ TEST_F(mount_setattr, mount_attr_nosymfollow)
ASSERT_EQ(close(fd), 0);
}
+TEST_F(mount_setattr, check_fields)
+{
+ struct mount_attr_ext {
+ struct mount_attr inner;
+ uint64_t extra1;
+ uint64_t extra2;
+ uint64_t extra3;
+ } attr;
+
+ /* Set the structure to dummy values. */
+ memset(&attr, 0xAB, sizeof(attr));
+
+ if (!mount_setattr_supported())
+ SKIP(return, "mount_setattr syscall not supported");
+
+ /* Make sure that invalid sizes are detected even with CHECK_FIELDS. */
+ EXPECT_EQ(sys_mount_setattr(-1, "", 0, (struct mount_attr *) &attr, CHECK_FIELDS), -EINVAL);
+ EXPECT_EQ(sys_mount_setattr(-1, "", 0, (struct mount_attr *) &attr, CHECK_FIELDS | 0xF000), -E2BIG);
+
+ /* Actually get the CHECK_FIELDS results. */
+ ASSERT_EQ(sys_mount_setattr(-1, "", 0, (struct mount_attr *) &attr, CHECK_FIELDS | sizeof(attr)), -EEXTSYS_NOOP);
+
+ EXPECT_EQ(attr.inner.attr_set, MOUNT_SETATTR_VALID_FLAGS);
+ EXPECT_EQ(attr.inner.attr_clr, MOUNT_SETATTR_VALID_FLAGS);
+ EXPECT_EQ(attr.inner.propagation, MOUNT_SETATTR_PROPAGATION_FLAGS);
+ EXPECT_EQ(attr.inner.userns_fd, 0xFFFFFFFFFFFFFFFF);
+ /* Trailing fields outside ksize must be zeroed. */
+ EXPECT_EQ(attr.extra1, 0);
+ EXPECT_EQ(attr.extra2, 0);
+ EXPECT_EQ(attr.extra3, 0);
+}
+
TEST_HARNESS_MAIN
--
2.46.1
^ permalink raw reply related
* [PATCH RFC v3 09/10] mount_setattr: add CHECK_FIELDS flag to usize argument
From: Aleksa Sarai @ 2024-10-09 20:40 UTC (permalink / raw)
To: Ingo Molnar, Peter Zijlstra, Juri Lelli, Vincent Guittot,
Dietmar Eggemann, Steven Rostedt, Ben Segall, Mel Gorman,
Valentin Schneider, Alexander Viro, Christian Brauner, Jan Kara,
Arnd Bergmann, Shuah Khan
Cc: Kees Cook, Florian Weimer, Arnd Bergmann, Mark Rutland,
linux-kernel, linux-api, linux-fsdevel, linux-arch,
linux-kselftest, Aleksa Sarai
In-Reply-To: <20241010-extensible-structs-check_fields-v3-0-d2833dfe6edd@cyphar.com>
As with openat2(2), this allows userspace to easily figure out what
flags and fields are supported by mount_setattr(2). As with clone3(2),
for fields which are not flag-based, we simply set every bit in the
field so that a naive bitwise-and would show that any value of the field
is valid.
The intended way of using this interface to get feature information
looks something like the following:
static bool mountattr_nosymfollow_supported;
static bool mountattr_idmap_supported;
int check_clone3_support(void)
{
int err;
struct mount_attr attr = {};
err = mount_attr(-EBADF, "", 0, &args, CHECK_FIELDS | sizeof(args));
assert(err < 0);
switch (errno) {
case EFAULT: case E2BIG:
/* Old kernel... */
check_support_the_old_way();
break;
case EEXTSYS_NOOP:
mountattr_nosymfollow_supported =
((attr.attr_clr | attr.attr_set) & MOUNT_ATTR_NOSYMFOLLOW);
mountattr_idmap_supported =
((attr.attr_clr | attr.attr_set) & MOUNT_ATTR_IDMAP) &&
(attr.userns_fd != 0);
break;
}
}
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
fs/namespace.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/fs/namespace.c b/fs/namespace.c
index 328087a4df8a..c7ae8d96b7b7 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -4771,6 +4771,7 @@ SYSCALL_DEFINE5(mount_setattr, int, dfd, const char __user *, path,
size_t, usize)
{
int err;
+ bool check_fields;
struct path target;
struct mount_attr attr;
struct mount_kattr kattr;
@@ -4783,11 +4784,27 @@ SYSCALL_DEFINE5(mount_setattr, int, dfd, const char __user *, path,
AT_NO_AUTOMOUNT))
return -EINVAL;
+ check_fields = usize & CHECK_FIELDS;
+ usize &= ~CHECK_FIELDS;
+
if (unlikely(usize > PAGE_SIZE))
return -E2BIG;
if (unlikely(usize < MOUNT_ATTR_SIZE_VER0))
return -EINVAL;
+ if (unlikely(check_fields)) {
+ memset(&attr, 0, sizeof(attr));
+ attr = (struct mount_attr) {
+ .attr_set = MOUNT_SETATTR_VALID_FLAGS,
+ .attr_clr = MOUNT_SETATTR_VALID_FLAGS,
+ .propagation = MOUNT_SETATTR_PROPAGATION_FLAGS,
+ .userns_fd = 0xFFFFFFFFFFFFFFFF,
+ };
+
+ err = copy_struct_to_user(uattr, usize, &attr, sizeof(attr), NULL);
+ return err ?: -EEXTSYS_NOOP;
+ }
+
if (!may_mount())
return -EPERM;
--
2.46.1
^ permalink raw reply related
* [PATCH RFC v3 08/10] selftests: clone3: add CHECK_FIELDS selftests
From: Aleksa Sarai @ 2024-10-09 20:40 UTC (permalink / raw)
To: Ingo Molnar, Peter Zijlstra, Juri Lelli, Vincent Guittot,
Dietmar Eggemann, Steven Rostedt, Ben Segall, Mel Gorman,
Valentin Schneider, Alexander Viro, Christian Brauner, Jan Kara,
Arnd Bergmann, Shuah Khan
Cc: Kees Cook, Florian Weimer, Arnd Bergmann, Mark Rutland,
linux-kernel, linux-api, linux-fsdevel, linux-arch,
linux-kselftest, Aleksa Sarai
In-Reply-To: <20241010-extensible-structs-check_fields-v3-0-d2833dfe6edd@cyphar.com>
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
tools/testing/selftests/clone3/.gitignore | 1 +
tools/testing/selftests/clone3/Makefile | 4 +-
.../testing/selftests/clone3/clone3_check_fields.c | 264 +++++++++++++++++++++
3 files changed, 267 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/clone3/.gitignore b/tools/testing/selftests/clone3/.gitignore
index 83c0f6246055..4ec3e1ecd273 100644
--- a/tools/testing/selftests/clone3/.gitignore
+++ b/tools/testing/selftests/clone3/.gitignore
@@ -3,3 +3,4 @@ clone3
clone3_clear_sighand
clone3_set_tid
clone3_cap_checkpoint_restore
+clone3_check_fields
diff --git a/tools/testing/selftests/clone3/Makefile b/tools/testing/selftests/clone3/Makefile
index 84832c369a2e..37141ca13f7c 100644
--- a/tools/testing/selftests/clone3/Makefile
+++ b/tools/testing/selftests/clone3/Makefile
@@ -1,8 +1,8 @@
# SPDX-License-Identifier: GPL-2.0
-CFLAGS += -g -std=gnu99 $(KHDR_INCLUDES)
+CFLAGS += -g -std=gnu99 $(KHDR_INCLUDES) $(TOOLS_INCLUDES)
LDLIBS += -lcap
TEST_GEN_PROGS := clone3 clone3_clear_sighand clone3_set_tid \
- clone3_cap_checkpoint_restore
+ clone3_cap_checkpoint_restore clone3_check_fields
include ../lib.mk
diff --git a/tools/testing/selftests/clone3/clone3_check_fields.c b/tools/testing/selftests/clone3/clone3_check_fields.c
new file mode 100644
index 000000000000..477604f9a3fb
--- /dev/null
+++ b/tools/testing/selftests/clone3/clone3_check_fields.c
@@ -0,0 +1,264 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Author: Aleksa Sarai <cyphar@cyphar.com>
+ * Copyright (C) 2024 SUSE LLC
+ */
+
+#define _GNU_SOURCE
+#include <errno.h>
+#include <inttypes.h>
+#include <linux/types.h>
+#include <linux/sched.h>
+#include <stdbool.h>
+#include <stdint.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <sys/syscall.h>
+#include <sys/types.h>
+#include <sys/un.h>
+#include <sys/wait.h>
+#include <unistd.h>
+#include <sched.h>
+
+#include "../kselftest.h"
+#include "clone3_selftests.h"
+
+#ifndef CHECK_FIELDS
+#define CHECK_FIELDS (1ULL << 63)
+#endif
+
+struct __clone_args_v0 {
+ __aligned_u64 flags;
+ __aligned_u64 pidfd;
+ __aligned_u64 child_tid;
+ __aligned_u64 parent_tid;
+ __aligned_u64 exit_signal;
+ __aligned_u64 stack;
+ __aligned_u64 stack_size;
+ __aligned_u64 tls;
+};
+
+struct __clone_args_v1 {
+ __aligned_u64 flags;
+ __aligned_u64 pidfd;
+ __aligned_u64 child_tid;
+ __aligned_u64 parent_tid;
+ __aligned_u64 exit_signal;
+ __aligned_u64 stack;
+ __aligned_u64 stack_size;
+ __aligned_u64 tls;
+ __aligned_u64 set_tid;
+ __aligned_u64 set_tid_size;
+};
+
+struct __clone_args_v2 {
+ __aligned_u64 flags;
+ __aligned_u64 pidfd;
+ __aligned_u64 child_tid;
+ __aligned_u64 parent_tid;
+ __aligned_u64 exit_signal;
+ __aligned_u64 stack;
+ __aligned_u64 stack_size;
+ __aligned_u64 tls;
+ __aligned_u64 set_tid;
+ __aligned_u64 set_tid_size;
+ __aligned_u64 cgroup;
+};
+
+static int call_clone3(void *clone_args, size_t size)
+{
+ int status;
+ pid_t pid;
+
+ pid = sys_clone3(clone_args, size);
+ if (pid < 0)
+ return -errno;
+
+ if (pid == 0) {
+ ksft_print_msg("I am the child, my PID is %d\n", getpid());
+ _exit(EXIT_SUCCESS);
+ }
+
+ ksft_print_msg("I am the parent (%d). My child's pid is %d\n",
+ getpid(), pid);
+
+ if (waitpid(-1, &status, __WALL) < 0) {
+ ksft_print_msg("waitpid() returned %s\n", strerror(errno));
+ return -errno;
+ }
+ if (!WIFEXITED(status)) {
+ ksft_print_msg("Child did not exit normally, status 0x%x\n",
+ status);
+ return EXIT_FAILURE;
+ }
+ if (WEXITSTATUS(status))
+ return WEXITSTATUS(status);
+
+ return 0;
+}
+
+static bool check(bool *failed, bool pred)
+{
+ *failed |= pred;
+ return pred;
+}
+
+static void test_clone3_check_fields(const char *test_name, size_t struct_size)
+{
+ size_t bufsize;
+ void *buffer;
+ pid_t pid;
+ bool failed = false;
+ void (*resultfn)(const char *msg, ...) = ksft_test_result_pass;
+
+ /* Allocate some bytes after clone_args to verify they are cleared. */
+ bufsize = struct_size + 16;
+ buffer = malloc(bufsize);
+ /* Set the structure to dummy values. */
+ memset(buffer, 0, bufsize);
+ memset(buffer, 0xAB, struct_size);
+
+ pid = call_clone3(buffer, CHECK_FIELDS | struct_size);
+ if (check(&failed, (pid != -EEXTSYS_NOOP)))
+ ksft_print_msg("clone3(CHECK_FIELDS) returned the wrong error code: %d (%s) != %d\n",
+ pid, strerror(-pid), -EEXTSYS_NOOP);
+
+ switch (struct_size) {
+ case sizeof(struct __clone_args_v2): {
+ struct __clone_args_v2 *args = buffer;
+
+ if (check(&failed, (args->cgroup != 0xFFFFFFFFFFFFFFFF)))
+ ksft_print_msg("clone3(CHECK_FIELDS) has wrong cgroup field: 0x%.16llx != 0x%.16llx\n",
+ args->cgroup, 0xFFFFFFFFFFFFFFFF);
+
+ /* fallthrough; */
+ }
+ case sizeof(struct __clone_args_v1): {
+ struct __clone_args_v1 *args = buffer;
+
+ if (check(&failed, (args->set_tid != 0xFFFFFFFFFFFFFFFF)))
+ ksft_print_msg("clone3(CHECK_FIELDS) has wrong set_tid field: 0x%.16llx != 0x%.16llx\n",
+ args->set_tid, 0xFFFFFFFFFFFFFFFF);
+ if (check(&failed, (args->set_tid_size != 0xFFFFFFFFFFFFFFFF)))
+ ksft_print_msg("clone3(CHECK_FIELDS) has wrong set_tid_size field: 0x%.16llx != 0x%.16llx\n",
+ args->set_tid_size, 0xFFFFFFFFFFFFFFFF);
+
+ /* fallthrough; */
+ }
+ case sizeof(struct __clone_args_v0): {
+ struct __clone_args_v0 *args = buffer;
+
+ if (check(&failed, !(args->flags & CLONE_NEWUSER)))
+ ksft_print_msg("clone3(CHECK_FIELDS) is missing CLONE_NEWUSER in flags: 0x%.16llx (0x%.16llx)\n",
+ args->flags, CLONE_NEWUSER);
+ if (check(&failed, !(args->flags & CLONE_THREAD)))
+ ksft_print_msg("clone3(CHECK_FIELDS) is missing CLONE_THREAD in flags: 0x%.16llx (0x%.16llx)\n",
+ args->flags, CLONE_THREAD);
+ /*
+ * CLONE_INTO_CGROUP was added in v2, but it will be set even
+ * with smaller structure sizes.
+ */
+ if (check(&failed, !(args->flags & CLONE_INTO_CGROUP)))
+ ksft_print_msg("clone3(CHECK_FIELDS) is missing CLONE_INTO_CGROUP in flags: 0x%.16llx (0x%.16llx)\n",
+ args->flags, CLONE_INTO_CGROUP);
+
+ if (check(&failed, (args->exit_signal != 0xFF)))
+ ksft_print_msg("clone3(CHECK_FIELDS) has wrong exit_signal field: 0x%.16llx != 0x%.16llx\n",
+ args->exit_signal, 0xFF);
+
+ if (check(&failed, (args->stack != 0xFFFFFFFFFFFFFFFF)))
+ ksft_print_msg("clone3(CHECK_FIELDS) has wrong stack field: 0x%.16llx != 0x%.16llx\n",
+ args->stack, 0xFFFFFFFFFFFFFFFF);
+ if (check(&failed, (args->stack_size != 0xFFFFFFFFFFFFFFFF)))
+ ksft_print_msg("clone3(CHECK_FIELDS) has wrong stack_size field: 0x%.16llx != 0x%.16llx\n",
+ args->stack_size, 0xFFFFFFFFFFFFFFFF);
+ if (check(&failed, (args->tls != 0xFFFFFFFFFFFFFFFF)))
+ ksft_print_msg("clone3(CHECK_FIELDS) has wrong tls field: 0x%.16llx != 0x%.16llx\n",
+ args->tls, 0xFFFFFFFFFFFFFFFF);
+
+ break;
+ }
+ default:
+ fprintf(stderr, "INVALID STRUCTURE SIZE: %d\n", struct_size);
+ abort();
+ }
+
+ /* Verify that the trailing parts of the buffer are still 0. */
+ for (size_t i = struct_size; i < bufsize; i++) {
+ char ch = ((char *)buffer)[i];
+ if (check(&failed, (ch != '\x00')))
+ ksft_print_msg("clone3(CHECK_FIELDS) touched a byte outside the size: buffer[%d] = 0x%.2x\n",
+ i, ch);
+ }
+
+ if (failed)
+ resultfn = ksft_test_result_fail;
+
+ resultfn("clone3(CHECK_FIELDS) with %s\n", test_name);
+ free(buffer);
+}
+
+struct check_fields_test {
+ const char *name;
+ size_t struct_size;
+};
+
+static struct check_fields_test check_fields_tests[] = {
+ {"struct v0", sizeof(struct __clone_args_v0)},
+ {"struct v1", sizeof(struct __clone_args_v1)},
+ {"struct v2", sizeof(struct __clone_args_v2)},
+};
+
+static int test_clone3_check_fields_badsize(const char *test_name,
+ size_t struct_size)
+{
+ void *buffer;
+ pid_t pid;
+ bool failed = false;
+ int expected_err;
+ void (*resultfn)(const char *msg, ...) = ksft_test_result_pass;
+
+ buffer = malloc(struct_size);
+ memset(buffer, 0xAB, struct_size);
+
+ if (struct_size < sizeof(struct __clone_args_v0))
+ expected_err = -EINVAL;
+ if (struct_size > 4096)
+ expected_err = -E2BIG;
+
+ pid = call_clone3(buffer, CHECK_FIELDS | struct_size);
+ if (check(&failed, (pid != expected_err)))
+ ksft_print_msg("clone3(CHECK_FIELDS) returned the wrong error code: %d (%s) != %d (%s)\n",
+ pid, strerror(-pid),
+ expected_err, strerror(-expected_err));
+
+ if (failed)
+ resultfn = ksft_test_result_fail;
+
+ resultfn("clone3(CHECK_FIELDS) with %s returns %d (%s)\n", test_name,
+ expected_err, strerror(-expected_err));
+ free(buffer);
+}
+
+static struct check_fields_test bad_size_tests[] = {
+ {"short struct (< v0)", 1},
+ {"long struct (> PAGE_SIZE)", 0xF000},
+};
+
+int main(void)
+{
+ ksft_print_header();
+ ksft_set_plan(ARRAY_SIZE(check_fields_tests) + ARRAY_SIZE(bad_size_tests));
+ test_clone3_supported();
+
+ for (int i = 0; i < ARRAY_SIZE(check_fields_tests); i++) {
+ struct check_fields_test *test = &check_fields_tests[i];
+ test_clone3_check_fields(test->name, test->struct_size);
+ }
+ for (int i = 0; i < ARRAY_SIZE(bad_size_tests); i++) {
+ struct check_fields_test *test = &bad_size_tests[i];
+ test_clone3_check_fields_badsize(test->name, test->struct_size);
+ }
+
+ ksft_finished();
+}
--
2.46.1
^ permalink raw reply related
* [PATCH RFC v3 07/10] clone3: add CHECK_FIELDS flag to usize argument
From: Aleksa Sarai @ 2024-10-09 20:40 UTC (permalink / raw)
To: Ingo Molnar, Peter Zijlstra, Juri Lelli, Vincent Guittot,
Dietmar Eggemann, Steven Rostedt, Ben Segall, Mel Gorman,
Valentin Schneider, Alexander Viro, Christian Brauner, Jan Kara,
Arnd Bergmann, Shuah Khan
Cc: Kees Cook, Florian Weimer, Arnd Bergmann, Mark Rutland,
linux-kernel, linux-api, linux-fsdevel, linux-arch,
linux-kselftest, Aleksa Sarai
In-Reply-To: <20241010-extensible-structs-check_fields-v3-0-d2833dfe6edd@cyphar.com>
As with openat2(2), this allows userspace to easily figure out what
flags and fields are supported by clone3(2). For fields which are not
flag-based, we simply set every bit in the field so that a naive
bitwise-and would show that any value of the field is valid.
For args->exit_signal, since we have an explicit bitmask for the field
defined already (CSIGNAL) we can indicate that only those bits are
supported by current kernels. If we add some extra bits to exit_signal
in the future, being able to detect them as new features would be quite
useful.
The intended way of using this interface to get feature information
looks something like the following:
static bool clone3_clear_sighand_supported;
static bool clone3_cgroup_supported;
int check_clone3_support(void)
{
int err;
struct clone_args args = {};
err = clone3(&args, CHECK_FIELDS | sizeof(args));
assert(err < 0);
switch (errno) {
case EFAULT: case E2BIG:
/* Old kernel... */
check_support_the_old_way();
break;
case EEXTSYS_NOOP:
clone3_clear_sighand_supported = (how.flags & CLONE_CLEAR_SIGHAND);
clone3_cgroup_supported = (how.flags & CLONE_INTO_CGROUP) &&
(how.cgroup != 0);
break;
}
}
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
kernel/fork.c | 30 ++++++++++++++++++++++++++++--
1 file changed, 28 insertions(+), 2 deletions(-)
diff --git a/kernel/fork.c b/kernel/fork.c
index cc760491f201..bded93f4f5f4 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -2925,11 +2925,15 @@ SYSCALL_DEFINE5(clone, unsigned long, clone_flags, unsigned long, newsp,
}
#endif
+
+#define CLONE3_VALID_FLAGS (CLONE_LEGACY_FLAGS | CLONE_CLEAR_SIGHAND | CLONE_INTO_CGROUP)
+
noinline static int copy_clone_args_from_user(struct kernel_clone_args *kargs,
struct clone_args __user *uargs,
size_t usize)
{
int err;
+ bool check_fields;
struct clone_args args;
pid_t *kset_tid = kargs->set_tid;
@@ -2941,11 +2945,34 @@ noinline static int copy_clone_args_from_user(struct kernel_clone_args *kargs,
CLONE_ARGS_SIZE_VER2);
BUILD_BUG_ON(sizeof(struct clone_args) != CLONE_ARGS_SIZE_VER2);
+ check_fields = usize & CHECK_FIELDS;
+ usize &= ~CHECK_FIELDS;
+
if (unlikely(usize > PAGE_SIZE))
return -E2BIG;
if (unlikely(usize < CLONE_ARGS_SIZE_VER0))
return -EINVAL;
+ if (unlikely(check_fields)) {
+ memset(&args, 0, sizeof(args));
+ args = (struct clone_args) {
+ .flags = CLONE3_VALID_FLAGS,
+ .pidfd = 0xFFFFFFFFFFFFFFFF,
+ .child_tid = 0xFFFFFFFFFFFFFFFF,
+ .parent_tid = 0xFFFFFFFFFFFFFFFF,
+ .exit_signal = (u64) CSIGNAL,
+ .stack = 0xFFFFFFFFFFFFFFFF,
+ .stack_size = 0xFFFFFFFFFFFFFFFF,
+ .tls = 0xFFFFFFFFFFFFFFFF,
+ .set_tid = 0xFFFFFFFFFFFFFFFF,
+ .set_tid_size = 0xFFFFFFFFFFFFFFFF,
+ .cgroup = 0xFFFFFFFFFFFFFFFF,
+ };
+
+ err = copy_struct_to_user(uargs, usize, &args, sizeof(args), NULL);
+ return err ?: -EEXTSYS_NOOP;
+ }
+
err = copy_struct_from_user(&args, sizeof(args), uargs, usize);
if (err)
return err;
@@ -3025,8 +3052,7 @@ static inline bool clone3_stack_valid(struct kernel_clone_args *kargs)
static bool clone3_args_valid(struct kernel_clone_args *kargs)
{
/* Verify that no unknown flags are passed along. */
- if (kargs->flags &
- ~(CLONE_LEGACY_FLAGS | CLONE_CLEAR_SIGHAND | CLONE_INTO_CGROUP))
+ if (kargs->flags & ~CLONE3_VALID_FLAGS)
return false;
/*
--
2.46.1
^ permalink raw reply related
* [PATCH RFC v3 06/10] selftests: openat2: add CHECK_FIELDS selftests
From: Aleksa Sarai @ 2024-10-09 20:40 UTC (permalink / raw)
To: Ingo Molnar, Peter Zijlstra, Juri Lelli, Vincent Guittot,
Dietmar Eggemann, Steven Rostedt, Ben Segall, Mel Gorman,
Valentin Schneider, Alexander Viro, Christian Brauner, Jan Kara,
Arnd Bergmann, Shuah Khan
Cc: Kees Cook, Florian Weimer, Arnd Bergmann, Mark Rutland,
linux-kernel, linux-api, linux-fsdevel, linux-arch,
linux-kselftest, Aleksa Sarai
In-Reply-To: <20241010-extensible-structs-check_fields-v3-0-d2833dfe6edd@cyphar.com>
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
tools/testing/selftests/openat2/Makefile | 2 +
tools/testing/selftests/openat2/openat2_test.c | 161 ++++++++++++++++++++++++-
2 files changed, 161 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/openat2/Makefile b/tools/testing/selftests/openat2/Makefile
index 185dc76ebb5f..239a7fd0cb35 100644
--- a/tools/testing/selftests/openat2/Makefile
+++ b/tools/testing/selftests/openat2/Makefile
@@ -12,6 +12,8 @@ ifeq ($(LLVM),)
endif
LOCAL_HDRS += helpers.h
+# Needed for EEXTSYS_NOOP definition.
+CFLAGS += $(TOOLS_INCLUDES)
include ../lib.mk
diff --git a/tools/testing/selftests/openat2/openat2_test.c b/tools/testing/selftests/openat2/openat2_test.c
index 4ca175a16ad6..9d3382bdfce9 100644
--- a/tools/testing/selftests/openat2/openat2_test.c
+++ b/tools/testing/selftests/openat2/openat2_test.c
@@ -1,11 +1,12 @@
// SPDX-License-Identifier: GPL-2.0-or-later
/*
* Author: Aleksa Sarai <cyphar@cyphar.com>
- * Copyright (C) 2018-2019 SUSE LLC.
+ * Copyright (C) 2018-2024 SUSE LLC.
*/
#define _GNU_SOURCE
#define __SANE_USERSPACE_TYPES__ // Use ll64
+#include <errno.h>
#include <fcntl.h>
#include <sched.h>
#include <sys/stat.h>
@@ -29,6 +30,10 @@
#define O_LARGEFILE 0x8000
#endif
+#ifndef CHECK_FIELDS
+#define CHECK_FIELDS (1ULL << 63)
+#endif
+
struct open_how_ext {
struct open_how inner;
uint32_t extra1;
@@ -45,6 +50,152 @@ struct struct_test {
int err;
};
+static bool check(bool *failed, bool pred)
+{
+ *failed |= pred;
+ return pred;
+}
+
+#define NUM_OPENAT2_CHECK_FIELDS_BAD_TESTS 2
+
+static void test_openat2_check_fields_bad(const char *name, size_t struct_size)
+{
+ int fd, expected_err;
+ bool failed = false;
+ struct open_how how = {};
+ void (*resultfn)(const char *msg, ...) = ksft_test_result_pass;
+
+ if (struct_size < sizeof(struct open_how))
+ expected_err = -EINVAL;
+ if (struct_size > 4096)
+ expected_err = -E2BIG;
+
+ if (!openat2_supported) {
+ ksft_print_msg("openat2(2) unsupported\n");
+ resultfn = ksft_test_result_skip;
+ goto skip;
+ }
+
+ fd = raw_openat2(AT_FDCWD, "", &how, CHECK_FIELDS | struct_size);
+ if (check(&failed, fd != expected_err))
+ ksft_print_msg("openat2(CHECK_FIELDS) returned wrong error code: %d (%s) != %d (%s)\n",
+ fd, strerror(-fd),
+ expected_err, strerror(-expected_err));
+
+ if (fd >= 0)
+ close(fd);
+
+ if (failed)
+ resultfn = ksft_test_result_fail;
+
+skip:
+ resultfn("openat2(CHECK_FIELDS) with %s returns %d (%s)\n", name,
+ expected_err, strerror(-expected_err));
+ fflush(stdout);
+}
+
+#define NUM_OPENAT2_CHECK_FIELDS_TESTS 1
+#define NUM_OPENAT2_CHECK_FIELDS_VARIATIONS 13
+
+static void test_openat2_check_fields(void)
+{
+ int misalignments[] = { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 11, 17, 87 };
+
+ for (int i = 0; i < ARRAY_LEN(misalignments); i++) {
+ int fd, misalign = misalignments[i];
+ bool failed = false;
+ char *fdpath = NULL;
+ void (*resultfn)(const char *msg, ...) = ksft_test_result_pass;
+
+ struct open_how_ext how_ext = {}, *how_copy = &how_ext;
+ void *copy = NULL;
+
+ if (!openat2_supported) {
+ ksft_print_msg("openat2(2) unsupported\n");
+ resultfn = ksft_test_result_skip;
+ goto skip;
+ }
+
+ if (misalign) {
+ /*
+ * Explicitly misalign the structure copying it with
+ * the given (mis)alignment offset. The other data is
+ * set to zero and we verify this afterwards to make
+ * sure CHECK_FIELDS doesn't write outside the buffer.
+ */
+ copy = malloc(misalign*2 + sizeof(how_ext));
+ how_copy = copy + misalign;
+ memset(copy, 0x00, misalign*2 + sizeof(how_ext));
+ memcpy(how_copy, &how_ext, sizeof(how_ext));
+ }
+
+ fd = raw_openat2(AT_FDCWD, ".", how_copy, CHECK_FIELDS | sizeof(*how_copy));
+ if (check(&failed, (fd != -EEXTSYS_NOOP)))
+ ksft_print_msg("openat2(CHECK_FIELDS) returned wrong error code: %d (%s) != %d\n",
+ fd, strerror(-fd), -EEXTSYS_NOOP);
+ if (fd >= 0) {
+ fdpath = fdreadlink(fd);
+ close(fd);
+ }
+
+ if (failed) {
+ ksft_print_msg("openat2(CHECK_FIELDS) unexpectedly returned ");
+ if (fdpath)
+ ksft_print_msg("%d['%s']\n", fd, fdpath);
+ else
+ ksft_print_msg("%d (%s)\n", fd, strerror(-fd));
+ }
+
+ if (check(&failed, !(how_copy->inner.flags & O_PATH)))
+ ksft_print_msg("openat2(CHECK_FIELDS) returned flags is missing O_PATH (0x%.16x): 0x%.16llx\n",
+ O_PATH, how_copy->inner.flags);
+
+ if (check(&failed, (how_copy->inner.mode != 07777)))
+ ksft_print_msg("openat2(CHECK_FIELDS) returned mode is invalid (0o%o): 0o%.4llo\n",
+ 07777, how_copy->inner.mode);
+
+ if (check(&failed, !(how_copy->inner.resolve & RESOLVE_IN_ROOT)))
+ ksft_print_msg("openat2(CHECK_FIELDS) returned resolve flags is missing RESOLVE_IN_ROOT (0x%.16x): 0x%.16llx\n",
+ RESOLVE_IN_ROOT, how_copy->inner.resolve);
+
+ /* Verify that the buffer space outside the struct wasn't written to. */
+ if (check(&failed, how_copy->extra1 != 0))
+ ksft_print_msg("openat2(CHECK_FIELDS) touched a byte outside open_how (extra1): 0x%x\n",
+ how_copy->extra1);
+ if (check(&failed, how_copy->extra2 != 0))
+ ksft_print_msg("openat2(CHECK_FIELDS) touched a byte outside open_how (extra2): 0x%x\n",
+ how_copy->extra2);
+ if (check(&failed, how_copy->extra3 != 0))
+ ksft_print_msg("openat2(CHECK_FIELDS) touched a byte outside open_how (extra3): 0x%x\n",
+ how_copy->extra3);
+
+ if (misalign) {
+ for (size_t i = 0; i < misalign; i++) {
+ char *p = copy + i;
+ if (check(&failed, *p != '\x00'))
+ ksft_print_msg("openat2(CHECK_FIELDS) touched a byte outside the size: buffer[%ld] = 0x%.2x\n",
+ p - (char *) copy, *p);
+ }
+ for (size_t i = 0; i < misalign; i++) {
+ char *p = copy + misalign + sizeof(how_ext) + i;
+ if (check(&failed, *p != '\x00'))
+ ksft_print_msg("openat2(CHECK_FIELDS) touched a byte outside the size: buffer[%ld] = 0x%.2x\n",
+ p - (char *) copy, *p);
+ }
+ }
+
+ if (failed)
+ resultfn = ksft_test_result_fail;
+
+skip:
+ resultfn("openat2(CHECK_FIELDS) [misalign=%d]\n", misalign);
+
+ free(copy);
+ free(fdpath);
+ fflush(stdout);
+ }
+}
+
#define NUM_OPENAT2_STRUCT_TESTS 7
#define NUM_OPENAT2_STRUCT_VARIATIONS 13
@@ -320,7 +471,9 @@ void test_openat2_flags(void)
}
}
-#define NUM_TESTS (NUM_OPENAT2_STRUCT_VARIATIONS * NUM_OPENAT2_STRUCT_TESTS + \
+#define NUM_TESTS (NUM_OPENAT2_CHECK_FIELDS_TESTS * NUM_OPENAT2_CHECK_FIELDS_VARIATIONS + \
+ NUM_OPENAT2_CHECK_FIELDS_BAD_TESTS + \
+ NUM_OPENAT2_STRUCT_VARIATIONS * NUM_OPENAT2_STRUCT_TESTS + \
NUM_OPENAT2_FLAG_TESTS)
int main(int argc, char **argv)
@@ -328,6 +481,10 @@ int main(int argc, char **argv)
ksft_print_header();
ksft_set_plan(NUM_TESTS);
+ test_openat2_check_fields();
+ test_openat2_check_fields_bad("small struct (< v0)", 0);
+ test_openat2_check_fields_bad("large struct (> PAGE_SIZE)", 0xF000);
+
test_openat2_struct();
test_openat2_flags();
--
2.46.1
^ permalink raw reply related
* [PATCH RFC v3 05/10] selftests: openat2: add 0xFF poisoned data after misaligned struct
From: Aleksa Sarai @ 2024-10-09 20:40 UTC (permalink / raw)
To: Ingo Molnar, Peter Zijlstra, Juri Lelli, Vincent Guittot,
Dietmar Eggemann, Steven Rostedt, Ben Segall, Mel Gorman,
Valentin Schneider, Alexander Viro, Christian Brauner, Jan Kara,
Arnd Bergmann, Shuah Khan
Cc: Kees Cook, Florian Weimer, Arnd Bergmann, Mark Rutland,
linux-kernel, linux-api, linux-fsdevel, linux-arch,
linux-kselftest, Aleksa Sarai
In-Reply-To: <20241010-extensible-structs-check_fields-v3-0-d2833dfe6edd@cyphar.com>
We should also verify that poisoned data after a misaligned struct is
also handled correctly by is_zeroed_user(). This test passes with no
kernel changes needed, so is_zeroed_user() was correct already.
Fixes: b28a10aedcd4 ("selftests: add openat2(2) selftests")
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
tools/testing/selftests/openat2/openat2_test.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/openat2/openat2_test.c b/tools/testing/selftests/openat2/openat2_test.c
index 5790ab446527..4ca175a16ad6 100644
--- a/tools/testing/selftests/openat2/openat2_test.c
+++ b/tools/testing/selftests/openat2/openat2_test.c
@@ -112,9 +112,9 @@ void test_openat2_struct(void)
*
* This is effectively to check that is_zeroed_user() works.
*/
- copy = malloc(misalign + sizeof(how_ext));
+ copy = malloc(misalign*2 + sizeof(how_ext));
how_copy = copy + misalign;
- memset(copy, 0xff, misalign);
+ memset(copy, 0xff, misalign*2 + sizeof(how_ext));
memcpy(how_copy, &how_ext, sizeof(how_ext));
}
--
2.46.1
^ permalink raw reply related
* [PATCH RFC v3 04/10] openat2: add CHECK_FIELDS flag to usize argument
From: Aleksa Sarai @ 2024-10-09 20:40 UTC (permalink / raw)
To: Ingo Molnar, Peter Zijlstra, Juri Lelli, Vincent Guittot,
Dietmar Eggemann, Steven Rostedt, Ben Segall, Mel Gorman,
Valentin Schneider, Alexander Viro, Christian Brauner, Jan Kara,
Arnd Bergmann, Shuah Khan
Cc: Kees Cook, Florian Weimer, Arnd Bergmann, Mark Rutland,
linux-kernel, linux-api, linux-fsdevel, linux-arch,
linux-kselftest, Aleksa Sarai
In-Reply-To: <20241010-extensible-structs-check_fields-v3-0-d2833dfe6edd@cyphar.com>
In order for userspace to be able to know what flags and fields the
kernel supports, it is currently necessary for them to do a bunch of
fairly subtle self-checks where you need to get a syscall to return a
non-EINVAL error or no-op for each flag you wish to use. If you get
-EINVAL you know the flag is unsupported, otherwise you know it is
supported.
This doesn't scale well for programs that need to check many flags, and
not all syscalls can be easily checked (how would you check for new
flags for umount2 or clone3 without side-effects?). To solve this
problem, we can take advantage of the extensible struct API used by
copy_struct_from_user() by providing a special CHECK_FIELDS flag to
extensible struct syscalls (like openat2 and clone3) which will:
1. Cause the syscall to fill the structure with every valid bit the
kernel understands. For flag arguments, this is the set of all valid
flag bits. For pointer and file descriptor arguments, this would be
all 0xFF bits (to indicate that any bits are valid). Userspace can
then easily check whether the flag they wanted is supported (by
doing a simple bitwise AND) or if a field itself is supported (by
checking if it is non-zero / all 0xFF).
2. Return a specific no-op error (-EEXTSYS_NOOP) that is not used as an
error by any other kernel code, so that userspace can be absolutely
sure that the kernel supports CHECK_FIELDS.
Rather than passing CHECK_FIELDS using the standard flags arguments for
the syscall, CHECK_FIELDS is instead the highest bit in the provided
struct size. The high bits of the size are never going to be non-zero
(we currently only allow size to be up to PAGE_SIZE, and it seems very
unlikely we will ever allow several exabyte structure arguments).
By passing the flag in the structure size, we can be sure that old
kernels will return a consistent error code (-EFAULT in openat2's case)
and that seccomp can properly filter this syscall mode (which is
guaranteed to be a no-op on all kernels -- it could even force
-EEXTSYS_NOOP to make the userspace program think the kernel doesn't
support any syscall features).
The intended way of using this interface to get feature information
looks something like the following (imagine that openat2 has gained a
new field and a new flag in the future):
static bool openat2_no_automount_supported;
static bool openat2_cwd_fd_supported;
int check_openat2_support(void)
{
int err;
struct open_how how = {};
err = openat2(AT_FDCWD, ".", &how, CHECK_FIELDS | sizeof(how));
assert(err < 0);
switch (errno) {
case EFAULT: case E2BIG:
/* Old kernel... */
check_support_the_old_way();
break;
case EEXTSYS_NOOP:
openat2_no_automount_supported = (how.flags & RESOLVE_NO_AUTOMOUNT);
openat2_cwd_fd_supported = (how.cwd_fd != 0);
break;
}
}
Link: https://youtu.be/ggD-eb3yPVs
Link: https://lwn.net/Articles/830666/
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
arch/alpha/include/uapi/asm/errno.h | 3 +++
arch/mips/include/uapi/asm/errno.h | 3 +++
arch/parisc/include/uapi/asm/errno.h | 3 +++
arch/sparc/include/uapi/asm/errno.h | 3 +++
fs/open.c | 16 ++++++++++++++++
include/uapi/asm-generic/errno.h | 3 +++
include/uapi/linux/openat2.h | 2 ++
tools/arch/alpha/include/uapi/asm/errno.h | 3 +++
tools/arch/mips/include/uapi/asm/errno.h | 3 +++
tools/arch/parisc/include/uapi/asm/errno.h | 3 +++
tools/arch/sparc/include/uapi/asm/errno.h | 3 +++
tools/include/uapi/asm-generic/errno.h | 3 +++
12 files changed, 48 insertions(+)
diff --git a/arch/alpha/include/uapi/asm/errno.h b/arch/alpha/include/uapi/asm/errno.h
index 3d265f6babaf..41157139fdad 100644
--- a/arch/alpha/include/uapi/asm/errno.h
+++ b/arch/alpha/include/uapi/asm/errno.h
@@ -125,4 +125,7 @@
#define EHWPOISON 139 /* Memory page has hardware error */
+/* For extensible syscalls. */
+#define EEXTSYS_NOOP 140 /* Extensible syscall performed no operation */
+
#endif
diff --git a/arch/mips/include/uapi/asm/errno.h b/arch/mips/include/uapi/asm/errno.h
index 2fb714e2d6d8..dd1e0ba61105 100644
--- a/arch/mips/include/uapi/asm/errno.h
+++ b/arch/mips/include/uapi/asm/errno.h
@@ -124,6 +124,9 @@
#define EHWPOISON 168 /* Memory page has hardware error */
+/* For extensible syscalls. */
+#define EEXTSYS_NOOP 169 /* Extensible syscall performed no operation */
+
#define EDQUOT 1133 /* Quota exceeded */
diff --git a/arch/parisc/include/uapi/asm/errno.h b/arch/parisc/include/uapi/asm/errno.h
index 8d94739d75c6..e2f6998ad61c 100644
--- a/arch/parisc/include/uapi/asm/errno.h
+++ b/arch/parisc/include/uapi/asm/errno.h
@@ -122,4 +122,7 @@
#define EHWPOISON 257 /* Memory page has hardware error */
+/* For extensible syscalls. */
+#define EEXTSYS_NOOP 258 /* Extensible syscall performed no operation */
+
#endif
diff --git a/arch/sparc/include/uapi/asm/errno.h b/arch/sparc/include/uapi/asm/errno.h
index 81a732b902ee..1b11e4651093 100644
--- a/arch/sparc/include/uapi/asm/errno.h
+++ b/arch/sparc/include/uapi/asm/errno.h
@@ -115,4 +115,7 @@
#define EHWPOISON 135 /* Memory page has hardware error */
+/* For extensible syscalls. */
+#define EEXTSYS_NOOP 136 /* Extensible syscall performed no operation */
+
#endif
diff --git a/fs/open.c b/fs/open.c
index 30bfcddd505d..3fbb0c82fece 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -1451,16 +1451,32 @@ SYSCALL_DEFINE4(openat2, int, dfd, const char __user *, filename,
struct open_how __user *, how, size_t, usize)
{
int err;
+ bool check_fields;
struct open_how tmp;
BUILD_BUG_ON(sizeof(struct open_how) < OPEN_HOW_SIZE_VER0);
BUILD_BUG_ON(sizeof(struct open_how) != OPEN_HOW_SIZE_LATEST);
+ check_fields = usize & CHECK_FIELDS;
+ usize &= ~CHECK_FIELDS;
+
if (unlikely(usize < OPEN_HOW_SIZE_VER0))
return -EINVAL;
if (unlikely(usize > PAGE_SIZE))
return -E2BIG;
+ if (unlikely(check_fields)) {
+ memset(&tmp, 0, sizeof(tmp));
+ tmp = (struct open_how) {
+ .flags = VALID_OPEN_FLAGS,
+ .mode = S_IALLUGO,
+ .resolve = VALID_RESOLVE_FLAGS,
+ };
+
+ err = copy_struct_to_user(how, usize, &tmp, sizeof(tmp), NULL);
+ return err ?: -EEXTSYS_NOOP;
+ }
+
err = copy_struct_from_user(&tmp, sizeof(tmp), how, usize);
if (err)
return err;
diff --git a/include/uapi/asm-generic/errno.h b/include/uapi/asm-generic/errno.h
index cf9c51ac49f9..f5bfe081e73a 100644
--- a/include/uapi/asm-generic/errno.h
+++ b/include/uapi/asm-generic/errno.h
@@ -120,4 +120,7 @@
#define EHWPOISON 133 /* Memory page has hardware error */
+/* For extensible syscalls. */
+#define EEXTSYS_NOOP 134 /* Extensible syscall performed no operation */
+
#endif
diff --git a/include/uapi/linux/openat2.h b/include/uapi/linux/openat2.h
index a5feb7604948..6052a504cfa4 100644
--- a/include/uapi/linux/openat2.h
+++ b/include/uapi/linux/openat2.h
@@ -4,6 +4,8 @@
#include <linux/types.h>
+#define CHECK_FIELDS (1ULL << 63)
+
/*
* Arguments for how openat2(2) should open the target path. If only @flags and
* @mode are non-zero, then openat2(2) operates very similarly to openat(2).
diff --git a/tools/arch/alpha/include/uapi/asm/errno.h b/tools/arch/alpha/include/uapi/asm/errno.h
index 3d265f6babaf..41157139fdad 100644
--- a/tools/arch/alpha/include/uapi/asm/errno.h
+++ b/tools/arch/alpha/include/uapi/asm/errno.h
@@ -125,4 +125,7 @@
#define EHWPOISON 139 /* Memory page has hardware error */
+/* For extensible syscalls. */
+#define EEXTSYS_NOOP 140 /* Extensible syscall performed no operation */
+
#endif
diff --git a/tools/arch/mips/include/uapi/asm/errno.h b/tools/arch/mips/include/uapi/asm/errno.h
index 2fb714e2d6d8..dd1e0ba61105 100644
--- a/tools/arch/mips/include/uapi/asm/errno.h
+++ b/tools/arch/mips/include/uapi/asm/errno.h
@@ -124,6 +124,9 @@
#define EHWPOISON 168 /* Memory page has hardware error */
+/* For extensible syscalls. */
+#define EEXTSYS_NOOP 169 /* Extensible syscall performed no operation */
+
#define EDQUOT 1133 /* Quota exceeded */
diff --git a/tools/arch/parisc/include/uapi/asm/errno.h b/tools/arch/parisc/include/uapi/asm/errno.h
index 8d94739d75c6..e2f6998ad61c 100644
--- a/tools/arch/parisc/include/uapi/asm/errno.h
+++ b/tools/arch/parisc/include/uapi/asm/errno.h
@@ -122,4 +122,7 @@
#define EHWPOISON 257 /* Memory page has hardware error */
+/* For extensible syscalls. */
+#define EEXTSYS_NOOP 258 /* Extensible syscall performed no operation */
+
#endif
diff --git a/tools/arch/sparc/include/uapi/asm/errno.h b/tools/arch/sparc/include/uapi/asm/errno.h
index 81a732b902ee..1b11e4651093 100644
--- a/tools/arch/sparc/include/uapi/asm/errno.h
+++ b/tools/arch/sparc/include/uapi/asm/errno.h
@@ -115,4 +115,7 @@
#define EHWPOISON 135 /* Memory page has hardware error */
+/* For extensible syscalls. */
+#define EEXTSYS_NOOP 136 /* Extensible syscall performed no operation */
+
#endif
diff --git a/tools/include/uapi/asm-generic/errno.h b/tools/include/uapi/asm-generic/errno.h
index cf9c51ac49f9..f5bfe081e73a 100644
--- a/tools/include/uapi/asm-generic/errno.h
+++ b/tools/include/uapi/asm-generic/errno.h
@@ -120,4 +120,7 @@
#define EHWPOISON 133 /* Memory page has hardware error */
+/* For extensible syscalls. */
+#define EEXTSYS_NOOP 134 /* Extensible syscall performed no operation */
+
#endif
--
2.46.1
^ permalink raw reply related
* [PATCH RFC v3 03/10] openat2: explicitly return -E2BIG for (usize > PAGE_SIZE)
From: Aleksa Sarai @ 2024-10-09 20:40 UTC (permalink / raw)
To: Ingo Molnar, Peter Zijlstra, Juri Lelli, Vincent Guittot,
Dietmar Eggemann, Steven Rostedt, Ben Segall, Mel Gorman,
Valentin Schneider, Alexander Viro, Christian Brauner, Jan Kara,
Arnd Bergmann, Shuah Khan
Cc: Kees Cook, Florian Weimer, Arnd Bergmann, Mark Rutland,
linux-kernel, linux-api, linux-fsdevel, linux-arch,
linux-kselftest, Aleksa Sarai, stable
In-Reply-To: <20241010-extensible-structs-check_fields-v3-0-d2833dfe6edd@cyphar.com>
While we do currently return -EFAULT in this case, it seems prudent to
follow the behaviour of other syscalls like clone3. It seems quite
unlikely that anyone depends on this error code being EFAULT, but we can
always revert this if it turns out to be an issue.
Cc: <stable@vger.kernel.org> # v5.6+
Fixes: fddb5d430ad9 ("open: introduce openat2(2) syscall")
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
fs/open.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/fs/open.c b/fs/open.c
index 22adbef7ecc2..30bfcddd505d 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -1458,6 +1458,8 @@ SYSCALL_DEFINE4(openat2, int, dfd, const char __user *, filename,
if (unlikely(usize < OPEN_HOW_SIZE_VER0))
return -EINVAL;
+ if (unlikely(usize > PAGE_SIZE))
+ return -E2BIG;
err = copy_struct_from_user(&tmp, sizeof(tmp), how, usize);
if (err)
--
2.46.1
^ permalink raw reply related
* [PATCH RFC v3 02/10] sched_getattr: port to copy_struct_to_user
From: Aleksa Sarai @ 2024-10-09 20:40 UTC (permalink / raw)
To: Ingo Molnar, Peter Zijlstra, Juri Lelli, Vincent Guittot,
Dietmar Eggemann, Steven Rostedt, Ben Segall, Mel Gorman,
Valentin Schneider, Alexander Viro, Christian Brauner, Jan Kara,
Arnd Bergmann, Shuah Khan
Cc: Kees Cook, Florian Weimer, Arnd Bergmann, Mark Rutland,
linux-kernel, linux-api, linux-fsdevel, linux-arch,
linux-kselftest, Aleksa Sarai
In-Reply-To: <20241010-extensible-structs-check_fields-v3-0-d2833dfe6edd@cyphar.com>
sched_getattr(2) doesn't care about trailing non-zero bytes in the
(ksize > usize) case, so just use copy_struct_to_user() without checking
ignored_trailing.
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
kernel/sched/syscalls.c | 42 ++----------------------------------------
1 file changed, 2 insertions(+), 40 deletions(-)
diff --git a/kernel/sched/syscalls.c b/kernel/sched/syscalls.c
index ae1b42775ef9..4ccc058bae16 100644
--- a/kernel/sched/syscalls.c
+++ b/kernel/sched/syscalls.c
@@ -1147,45 +1147,6 @@ SYSCALL_DEFINE2(sched_getparam, pid_t, pid, struct sched_param __user *, param)
return copy_to_user(param, &lp, sizeof(*param)) ? -EFAULT : 0;
}
-/*
- * Copy the kernel size attribute structure (which might be larger
- * than what user-space knows about) to user-space.
- *
- * Note that all cases are valid: user-space buffer can be larger or
- * smaller than the kernel-space buffer. The usual case is that both
- * have the same size.
- */
-static int
-sched_attr_copy_to_user(struct sched_attr __user *uattr,
- struct sched_attr *kattr,
- unsigned int usize)
-{
- unsigned int ksize = sizeof(*kattr);
-
- if (!access_ok(uattr, usize))
- return -EFAULT;
-
- /*
- * sched_getattr() ABI forwards and backwards compatibility:
- *
- * If usize == ksize then we just copy everything to user-space and all is good.
- *
- * If usize < ksize then we only copy as much as user-space has space for,
- * this keeps ABI compatibility as well. We skip the rest.
- *
- * If usize > ksize then user-space is using a newer version of the ABI,
- * which part the kernel doesn't know about. Just ignore it - tooling can
- * detect the kernel's knowledge of attributes from the attr->size value
- * which is set to ksize in this case.
- */
- kattr->size = min(usize, ksize);
-
- if (copy_to_user(uattr, kattr, kattr->size))
- return -EFAULT;
-
- return 0;
-}
-
/**
* sys_sched_getattr - similar to sched_getparam, but with sched_attr
* @pid: the pid in question.
@@ -1230,7 +1191,8 @@ SYSCALL_DEFINE4(sched_getattr, pid_t, pid, struct sched_attr __user *, uattr,
#endif
}
- return sched_attr_copy_to_user(uattr, &kattr, usize);
+ kattr.size = min(usize, sizeof(kattr));
+ return copy_struct_to_user(uattr, usize, &kattr, sizeof(kattr), NULL);
}
#ifdef CONFIG_SMP
--
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