* Re: [PATCH v7 2/3] pid: Introduce pidfd_getfd syscall
From: kbuild test robot @ 2019-12-26 22:20 UTC (permalink / raw)
To: Sargun Dhillon
Cc: kbuild-all, linux-kernel, containers, linux-api, linux-fsdevel,
tycho, jannh, cyphar, christian.brauner, oleg, luto, viro,
gpascutto, ealvarez, fweimer, jld, arnd
In-Reply-To: <20191226180334.GA29409@ircssh-2.c.rugged-nimbus-611.internal>
[-- Attachment #1: Type: text/plain, Size: 1383 bytes --]
Hi Sargun,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on kselftest/next]
[also build test ERROR on linus/master v5.5-rc3]
[cannot apply to tip/x86/asm next-20191220]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Sargun-Dhillon/Add-pidfd_getfd-syscall/20191227-025151
base: https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git next
config: alpha-defconfig (attached as .config)
compiler: alpha-linux-gcc (GCC) 7.5.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.5.0 make.cross ARCH=alpha
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
arch/alpha/kernel/systbls.o: In function `sys_call_table':
>> (.data+0x1120): undefined reference to `sys_pidfd'
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 13327 bytes --]
^ permalink raw reply
* [PATCH v7 3/3] test: Add test for pidfd getfd
From: Sargun Dhillon @ 2019-12-26 18:04 UTC (permalink / raw)
To: linux-kernel, containers, linux-api, linux-fsdevel
Cc: tycho, jannh, cyphar, christian.brauner, oleg, luto, viro,
gpascutto, ealvarez, fweimer, jld, arnd
This adds four tests:
* Fetch FD, and then compare via kcmp
* Read data from FD to make sure it works
* Make sure getfd can be blocked by blocking ptrace_may_access
* Making sure fetching bad FDs fails
* Make sure trying to set flags to non-zero results in an
EINVAL
Signed-off-by: Sargun Dhillon <sargun@sargun.me>
---
tools/testing/selftests/pidfd/.gitignore | 1 +
tools/testing/selftests/pidfd/Makefile | 2 +-
.../selftests/pidfd/pidfd_getfd_test.c | 254 ++++++++++++++++++
3 files changed, 256 insertions(+), 1 deletion(-)
create mode 100644 tools/testing/selftests/pidfd/pidfd_getfd_test.c
diff --git a/tools/testing/selftests/pidfd/.gitignore b/tools/testing/selftests/pidfd/.gitignore
index 8d069490e17b..3a779c084d96 100644
--- a/tools/testing/selftests/pidfd/.gitignore
+++ b/tools/testing/selftests/pidfd/.gitignore
@@ -2,3 +2,4 @@ pidfd_open_test
pidfd_poll_test
pidfd_test
pidfd_wait
+pidfd_getfd_test
diff --git a/tools/testing/selftests/pidfd/Makefile b/tools/testing/selftests/pidfd/Makefile
index 43db1b98e845..75a545861375 100644
--- a/tools/testing/selftests/pidfd/Makefile
+++ b/tools/testing/selftests/pidfd/Makefile
@@ -1,7 +1,7 @@
# SPDX-License-Identifier: GPL-2.0-only
CFLAGS += -g -I../../../../usr/include/ -pthread
-TEST_GEN_PROGS := pidfd_test pidfd_fdinfo_test pidfd_open_test pidfd_poll_test pidfd_wait
+TEST_GEN_PROGS := pidfd_test pidfd_fdinfo_test pidfd_open_test pidfd_poll_test pidfd_wait pidfd_getfd_test
include ../lib.mk
diff --git a/tools/testing/selftests/pidfd/pidfd_getfd_test.c b/tools/testing/selftests/pidfd/pidfd_getfd_test.c
new file mode 100644
index 000000000000..4505e68a07ac
--- /dev/null
+++ b/tools/testing/selftests/pidfd/pidfd_getfd_test.c
@@ -0,0 +1,254 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#define _GNU_SOURCE
+#include <errno.h>
+#include <fcntl.h>
+#include <inttypes.h>
+#include <limits.h>
+#include <linux/types.h>
+#include <linux/wait.h>
+#include <sched.h>
+#include <signal.h>
+#include <stdbool.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <syscall.h>
+#include <sys/prctl.h>
+#include <sys/wait.h>
+#include <unistd.h>
+#include <sys/socket.h>
+#include <linux/kcmp.h>
+#include <linux/capability.h>
+
+#include "pidfd.h"
+#include "../kselftest.h"
+
+#define WELL_KNOWN_CHILD_FD 100
+#define UNKNOWN_FD 111
+#define SECRET_MESSAGE "secret"
+
+static int kcmp(pid_t pid1, pid_t pid2, int type, unsigned long idx1,
+ unsigned long idx2)
+{
+ errno = 0;
+ return syscall(SYS_kcmp, pid1, pid2, type, idx1, idx2);
+}
+
+/* Flags is currently reserved, so it is not exposed, and passed as 0 */
+static int pidfd_getfd(int pidfd, int fd)
+{
+ errno = 0;
+ return syscall(__NR_pidfd_getfd, pidfd, fd, 0);
+}
+
+static int child(bool disable_ptrace, int sk)
+{
+ char buf[1024];
+ int ret, fd;
+
+ ret = prctl(PR_SET_PDEATHSIG, SIGKILL);
+ if (ret)
+ ksft_exit_fail_msg("%s: Child could not set DEATHSIG\n",
+ strerror(errno));
+
+ fd = syscall(SYS_memfd_create, "test", 0);
+ if (fd < 0)
+ ksft_exit_fail_msg("%s: Child could not create memfd\n",
+ strerror(errno));
+
+ ret = write(fd, SECRET_MESSAGE, sizeof(SECRET_MESSAGE));
+ if (ret < 0)
+ ksft_exit_fail_msg("%s: Child could not write secret message\n",
+ strerror(errno));
+
+ ret = dup2(fd, WELL_KNOWN_CHILD_FD);
+ if (ret < 0)
+ ksft_exit_fail_msg("%s: Could not dup fd into well-known FD\n",
+ strerror(errno));
+
+ ret = close(fd);
+ if (ret < 0)
+ ksft_exit_fail_msg("%s: Child could close old fd\n",
+ strerror(errno));
+
+ if (disable_ptrace) {
+ ret = prctl(PR_SET_DUMPABLE, 0);
+ if (ret < 0)
+ ksft_exit_fail_msg("%s: Child failed to disable ptrace\n",
+ strerror(errno));
+ }
+ ret = send(sk, "L", 1, 0);
+ if (ret < 0)
+ ksft_exit_fail_msg("%s: Child failed to send launched message\n",
+ strerror(errno));
+ if (ret == 0)
+ ksft_exit_fail_msg("Failed to send launch message; other side is closed\n");
+
+ close(sk);
+ pause();
+
+ return EXIT_SUCCESS;
+}
+
+static int start_child(bool disable_ptrace, pid_t *childpid)
+{
+ int pidfd, ret, sk_pair[2];
+ char buf[1];
+
+ if (socketpair(PF_LOCAL, SOCK_SEQPACKET, 0, sk_pair) < 0)
+ ksft_exit_fail_msg("%s: failed to create socketpair\n",
+ strerror(errno));
+ *childpid = fork();
+ if (*childpid < 0)
+ ksft_exit_fail_msg("%s: failed to fork a child process\n",
+ strerror(errno));
+
+ if (*childpid == 0)
+ exit(child(disable_ptrace, sk_pair[1]));
+
+ close(sk_pair[1]);
+
+ pidfd = sys_pidfd_open(*childpid, 0);
+ if (pidfd < 0)
+ ksft_exit_fail_msg("%s: failed to pidfd_open\n",
+ strerror(errno));
+
+ ret = recv(sk_pair[0], &buf, 1, 0);
+ if (ret < 0)
+ ksft_exit_fail_msg("%s: failed read from launch socket\n",
+ strerror(errno));
+ if (ret == 0)
+ ksft_exit_fail_msg("Failed to read from launch socket, child failed\n");
+
+ return pidfd;
+}
+
+static void test_kcmp_and_fetch_fd(void)
+{
+ char buf[sizeof(SECRET_MESSAGE)];
+ int fd, pidfd, ret;
+ pid_t child_pid;
+
+ pidfd = start_child(false, &child_pid);
+
+ fd = pidfd_getfd(pidfd, WELL_KNOWN_CHILD_FD);
+ if (fd < 0)
+ ksft_exit_fail_msg("%s: getfd failed\n", strerror(errno));
+
+ ret = kcmp(getpid(), child_pid, KCMP_FILE, fd, WELL_KNOWN_CHILD_FD);
+ if (ret != 0)
+ ksft_exit_fail_msg("Our FD not equal to child FD\n");
+
+ ksft_test_result_pass("kcmp\n");
+
+ ret = lseek(fd, 0, SEEK_SET);
+ if (ret < 0)
+ ksft_exit_fail_msg("%s: seek failed\n", strerror(errno));
+ if (ret != 0)
+ ksft_exit_fail_msg("%d: unexpected seek position\n", ret);
+
+ ret = read(fd, buf, sizeof(buf));
+ if (ret < 0)
+ ksft_exit_fail_msg("%s: failed to read secret message\n",
+ strerror(errno));
+
+ if (strncmp(SECRET_MESSAGE, buf, sizeof(buf)) != 0)
+ ksft_exit_fail_msg("%s: Secret message not correct\n", buf);
+
+ ret = sys_pidfd_send_signal(pidfd, SIGKILL, NULL, 0);
+ close(pidfd);
+ if (ret < 0)
+ ksft_exit_fail_msg("%s: failed to send kill to child\n",
+ strerror(errno));
+
+ ksft_test_result_pass("fetch_and_read\n");
+}
+
+static void test_no_ptrace(void)
+{
+ int fd, pidfd, ret, uid;
+ pid_t child_pid;
+
+ /* turn into nobody if we're root, to avoid CAP_SYS_PTRACE */
+ uid = getuid();
+ if (uid == 0)
+ seteuid(USHRT_MAX);
+
+ pidfd = start_child(true, &child_pid);
+
+ fd = pidfd_getfd(pidfd, WELL_KNOWN_CHILD_FD);
+ if (fd != -1)
+ ksft_exit_fail_msg("%s: getfd succeeded when ptrace blocked\n",
+ strerror(errno));
+ if (errno != EPERM)
+ ksft_exit_fail_msg("%s: getfd did not get EPERM\n",
+ strerror(errno));
+
+ ret = sys_pidfd_send_signal(pidfd, SIGKILL, NULL, 0);
+ close(pidfd);
+ if (ret < 0)
+ ksft_exit_fail_msg("%s: failed to send kill to child\n",
+ strerror(errno));
+
+ if (uid == 0)
+ seteuid(0);
+
+ ksft_test_result_pass("no_ptrace\n");
+}
+
+static void test_unknown_fd(void)
+{
+ int fd, pidfd, ret;
+ pid_t child_pid;
+
+ pidfd = start_child(false, &child_pid);
+
+ fd = pidfd_getfd(pidfd, UNKNOWN_FD);
+ if (fd != -1)
+ ksft_exit_fail_msg("%s: getfd succeeded when fetching unknown FD\n",
+ strerror(errno));
+ if (errno != EBADF)
+ ksft_exit_fail_msg("%s: getfd did not get EBADF\n",
+ strerror(errno));
+
+ ret = sys_pidfd_send_signal(pidfd, SIGKILL, NULL, 0);
+ close(pidfd);
+ if (ret < 0)
+ ksft_exit_fail_msg("%s: failed to send kill to child\n",
+ strerror(errno));
+
+ ksft_test_result_pass("unknown_fd\n");
+}
+
+static void test_flags_set(void)
+{
+ int ret;
+
+ errno = 0;
+ ret = syscall(__NR_pidfd_getfd, 0, 1, 1);
+ if (ret != -1)
+ ksft_exit_fail_msg("getfd succeeded with invalid flags\n");
+ if (errno != EINVAL)
+ ksft_exit_fail_msg("%s: getfd did not get EINVAL\n",
+ strerror(errno));
+
+ ksft_test_result_pass("flags_set\n");
+}
+
+int main(int argc, char **argv)
+{
+ char buf[sizeof(SECRET_MESSAGE)];
+ int ret, status, fd, pidfd;
+ pid_t child_pid;
+
+ ksft_print_header();
+ ksft_set_plan(5);
+
+ test_kcmp_and_fetch_fd();
+ test_unknown_fd();
+ test_no_ptrace();
+ test_flags_set();
+
+ return ksft_exit_pass();
+}
--
2.20.1
^ permalink raw reply related
* [PATCH v7 2/3] pid: Introduce pidfd_getfd syscall
From: Sargun Dhillon @ 2019-12-26 18:03 UTC (permalink / raw)
To: linux-kernel, containers, linux-api, linux-fsdevel
Cc: tycho, jannh, cyphar, christian.brauner, oleg, luto, viro,
gpascutto, ealvarez, fweimer, jld, arnd
This syscall allows for the retrieval of file descriptors from other
processes, based on their pidfd. This is possible using ptrace, and
injection of parasitic code to inject code which leverages SCM_RIGHTS
to move file descriptors between a tracee and a tracer. Unfortunately,
ptrace comes with a high cost of requiring the process to be stopped,
and breaks debuggers. This does not require stopping the process under
manipulation.
One reason to use this is to allow sandboxers to take actions on file
descriptors on the behalf of another process. For example, this can be
combined with seccomp-bpf's user notification to do on-demand fd
extraction and take privileged actions. One such privileged action
is binding a socket to a privileged port.
This also adds the syscall to all architectures at the same time.
/* prototype */
/* flags is currently reserved and should be set to 0 */
int sys_pidfd_getfd(int pidfd, int fd, unsigned int flags);
/* testing */
Ran self-test suite on x86_64
Signed-off-by: Sargun Dhillon <sargun@sargun.me>
Cc: Christian Brauner <christian.brauner@ubuntu.com>
---
arch/alpha/kernel/syscalls/syscall.tbl | 1 +
arch/arm/tools/syscall.tbl | 1 +
arch/arm64/include/asm/unistd.h | 2 +-
arch/arm64/include/asm/unistd32.h | 2 +
arch/ia64/kernel/syscalls/syscall.tbl | 1 +
arch/m68k/kernel/syscalls/syscall.tbl | 1 +
arch/microblaze/kernel/syscalls/syscall.tbl | 1 +
arch/mips/kernel/syscalls/syscall_n32.tbl | 1 +
arch/mips/kernel/syscalls/syscall_n64.tbl | 1 +
arch/mips/kernel/syscalls/syscall_o32.tbl | 1 +
arch/parisc/kernel/syscalls/syscall.tbl | 1 +
arch/powerpc/kernel/syscalls/syscall.tbl | 1 +
arch/s390/kernel/syscalls/syscall.tbl | 1 +
arch/sh/kernel/syscalls/syscall.tbl | 1 +
arch/sparc/kernel/syscalls/syscall.tbl | 1 +
arch/x86/entry/syscalls/syscall_32.tbl | 1 +
arch/x86/entry/syscalls/syscall_64.tbl | 1 +
arch/xtensa/kernel/syscalls/syscall.tbl | 1 +
include/linux/syscalls.h | 1 +
include/uapi/asm-generic/unistd.h | 4 +-
kernel/pid.c | 103 ++++++++++++++++++++
21 files changed, 126 insertions(+), 2 deletions(-)
diff --git a/arch/alpha/kernel/syscalls/syscall.tbl b/arch/alpha/kernel/syscalls/syscall.tbl
index 8e13b0b2928d..d1cac0d657b7 100644
--- a/arch/alpha/kernel/syscalls/syscall.tbl
+++ b/arch/alpha/kernel/syscalls/syscall.tbl
@@ -475,3 +475,4 @@
543 common fspick sys_fspick
544 common pidfd_open sys_pidfd_open
# 545 reserved for clone3
+548 common pidfd_getfd sys_pidfd
diff --git a/arch/arm/tools/syscall.tbl b/arch/arm/tools/syscall.tbl
index 6da7dc4d79cc..ba045e2f3a60 100644
--- a/arch/arm/tools/syscall.tbl
+++ b/arch/arm/tools/syscall.tbl
@@ -449,3 +449,4 @@
433 common fspick sys_fspick
434 common pidfd_open sys_pidfd_open
435 common clone3 sys_clone3
+438 common pidfd_getfd sys_pidfd_getfd
diff --git a/arch/arm64/include/asm/unistd.h b/arch/arm64/include/asm/unistd.h
index 2629a68b8724..b722e47377a5 100644
--- a/arch/arm64/include/asm/unistd.h
+++ b/arch/arm64/include/asm/unistd.h
@@ -38,7 +38,7 @@
#define __ARM_NR_compat_set_tls (__ARM_NR_COMPAT_BASE + 5)
#define __ARM_NR_COMPAT_END (__ARM_NR_COMPAT_BASE + 0x800)
-#define __NR_compat_syscalls 436
+#define __NR_compat_syscalls 439
#endif
#define __ARCH_WANT_SYS_CLONE
diff --git a/arch/arm64/include/asm/unistd32.h b/arch/arm64/include/asm/unistd32.h
index 94ab29cf4f00..a8da97a2de41 100644
--- a/arch/arm64/include/asm/unistd32.h
+++ b/arch/arm64/include/asm/unistd32.h
@@ -879,6 +879,8 @@ __SYSCALL(__NR_fspick, sys_fspick)
__SYSCALL(__NR_pidfd_open, sys_pidfd_open)
#define __NR_clone3 435
__SYSCALL(__NR_clone3, sys_clone3)
+#define __NR_pidfd_getfd 438
+__SYSCALL(__NR_pidfd_getfd, sys_pidfd_getfd)
/*
* Please add new compat syscalls above this comment and update
diff --git a/arch/ia64/kernel/syscalls/syscall.tbl b/arch/ia64/kernel/syscalls/syscall.tbl
index 36d5faf4c86c..2b11adfc860c 100644
--- a/arch/ia64/kernel/syscalls/syscall.tbl
+++ b/arch/ia64/kernel/syscalls/syscall.tbl
@@ -356,3 +356,4 @@
433 common fspick sys_fspick
434 common pidfd_open sys_pidfd_open
# 435 reserved for clone3
+438 common pidfd_getfd sys_pidfd_getfd
diff --git a/arch/m68k/kernel/syscalls/syscall.tbl b/arch/m68k/kernel/syscalls/syscall.tbl
index a88a285a0e5f..44e879e98459 100644
--- a/arch/m68k/kernel/syscalls/syscall.tbl
+++ b/arch/m68k/kernel/syscalls/syscall.tbl
@@ -435,3 +435,4 @@
433 common fspick sys_fspick
434 common pidfd_open sys_pidfd_open
# 435 reserved for clone3
+438 common pidfd_getfd sys_pidfd_getfd
diff --git a/arch/microblaze/kernel/syscalls/syscall.tbl b/arch/microblaze/kernel/syscalls/syscall.tbl
index 09b0cd7dab0a..7afa00125cc4 100644
--- a/arch/microblaze/kernel/syscalls/syscall.tbl
+++ b/arch/microblaze/kernel/syscalls/syscall.tbl
@@ -441,3 +441,4 @@
433 common fspick sys_fspick
434 common pidfd_open sys_pidfd_open
435 common clone3 sys_clone3
+438 common pidfd_getfd sys_pidfd_getfd
diff --git a/arch/mips/kernel/syscalls/syscall_n32.tbl b/arch/mips/kernel/syscalls/syscall_n32.tbl
index e7c5ab38e403..856d5ba34461 100644
--- a/arch/mips/kernel/syscalls/syscall_n32.tbl
+++ b/arch/mips/kernel/syscalls/syscall_n32.tbl
@@ -374,3 +374,4 @@
433 n32 fspick sys_fspick
434 n32 pidfd_open sys_pidfd_open
435 n32 clone3 __sys_clone3
+438 n32 pidfd_getfd sys_pidfd_getfd
diff --git a/arch/mips/kernel/syscalls/syscall_n64.tbl b/arch/mips/kernel/syscalls/syscall_n64.tbl
index 13cd66581f3b..2db6075352f3 100644
--- a/arch/mips/kernel/syscalls/syscall_n64.tbl
+++ b/arch/mips/kernel/syscalls/syscall_n64.tbl
@@ -350,3 +350,4 @@
433 n64 fspick sys_fspick
434 n64 pidfd_open sys_pidfd_open
435 n64 clone3 __sys_clone3
+438 n64 pidfd_getfd sys_pidfd_getfd
diff --git a/arch/mips/kernel/syscalls/syscall_o32.tbl b/arch/mips/kernel/syscalls/syscall_o32.tbl
index 353539ea4140..e9f9d4a9b105 100644
--- a/arch/mips/kernel/syscalls/syscall_o32.tbl
+++ b/arch/mips/kernel/syscalls/syscall_o32.tbl
@@ -423,3 +423,4 @@
433 o32 fspick sys_fspick
434 o32 pidfd_open sys_pidfd_open
435 o32 clone3 __sys_clone3
+438 o32 pidfd_getfd sys_pidfd_getfd
diff --git a/arch/parisc/kernel/syscalls/syscall.tbl b/arch/parisc/kernel/syscalls/syscall.tbl
index 285ff516150c..c58c7eb144ca 100644
--- a/arch/parisc/kernel/syscalls/syscall.tbl
+++ b/arch/parisc/kernel/syscalls/syscall.tbl
@@ -433,3 +433,4 @@
433 common fspick sys_fspick
434 common pidfd_open sys_pidfd_open
435 common clone3 sys_clone3_wrapper
+438 common pidfd_getfd sys_pidfd_getfd
diff --git a/arch/powerpc/kernel/syscalls/syscall.tbl b/arch/powerpc/kernel/syscalls/syscall.tbl
index 43f736ed47f2..707609bfe3ea 100644
--- a/arch/powerpc/kernel/syscalls/syscall.tbl
+++ b/arch/powerpc/kernel/syscalls/syscall.tbl
@@ -517,3 +517,4 @@
433 common fspick sys_fspick
434 common pidfd_open sys_pidfd_open
435 nospu clone3 ppc_clone3
+438 common pidfd_getfd sys_pidfd_getfd
diff --git a/arch/s390/kernel/syscalls/syscall.tbl b/arch/s390/kernel/syscalls/syscall.tbl
index 3054e9c035a3..185cd624face 100644
--- a/arch/s390/kernel/syscalls/syscall.tbl
+++ b/arch/s390/kernel/syscalls/syscall.tbl
@@ -438,3 +438,4 @@
433 common fspick sys_fspick sys_fspick
434 common pidfd_open sys_pidfd_open sys_pidfd_open
435 common clone3 sys_clone3 sys_clone3
+438 common pidfd_getfd sys_pidfd_getfd sys_pidfd_getfd
diff --git a/arch/sh/kernel/syscalls/syscall.tbl b/arch/sh/kernel/syscalls/syscall.tbl
index b5ed26c4c005..88f90895aad8 100644
--- a/arch/sh/kernel/syscalls/syscall.tbl
+++ b/arch/sh/kernel/syscalls/syscall.tbl
@@ -438,3 +438,4 @@
433 common fspick sys_fspick
434 common pidfd_open sys_pidfd_open
# 435 reserved for clone3
+438 common pidfd_getfd sys_pidfd_getfd
diff --git a/arch/sparc/kernel/syscalls/syscall.tbl b/arch/sparc/kernel/syscalls/syscall.tbl
index 8c8cc7537fb2..218df6a2326e 100644
--- a/arch/sparc/kernel/syscalls/syscall.tbl
+++ b/arch/sparc/kernel/syscalls/syscall.tbl
@@ -481,3 +481,4 @@
433 common fspick sys_fspick
434 common pidfd_open sys_pidfd_open
# 435 reserved for clone3
+438 common pidfd_getfd sys_pidfd_getfd
diff --git a/arch/x86/entry/syscalls/syscall_32.tbl b/arch/x86/entry/syscalls/syscall_32.tbl
index 15908eb9b17e..9c3101b65e0f 100644
--- a/arch/x86/entry/syscalls/syscall_32.tbl
+++ b/arch/x86/entry/syscalls/syscall_32.tbl
@@ -440,3 +440,4 @@
433 i386 fspick sys_fspick __ia32_sys_fspick
434 i386 pidfd_open sys_pidfd_open __ia32_sys_pidfd_open
435 i386 clone3 sys_clone3 __ia32_sys_clone3
+438 i386 pidfd_getfd sys_pidfd_getfd __ia32_sys_pidfd_getfd
diff --git a/arch/x86/entry/syscalls/syscall_64.tbl b/arch/x86/entry/syscalls/syscall_64.tbl
index c29976eca4a8..cef85db75a62 100644
--- a/arch/x86/entry/syscalls/syscall_64.tbl
+++ b/arch/x86/entry/syscalls/syscall_64.tbl
@@ -357,6 +357,7 @@
433 common fspick __x64_sys_fspick
434 common pidfd_open __x64_sys_pidfd_open
435 common clone3 __x64_sys_clone3/ptregs
+438 common pidfd_getfd __x64_sys_pidfd_getfd
#
# x32-specific system call numbers start at 512 to avoid cache impact
diff --git a/arch/xtensa/kernel/syscalls/syscall.tbl b/arch/xtensa/kernel/syscalls/syscall.tbl
index 25f4de729a6d..ae15183def12 100644
--- a/arch/xtensa/kernel/syscalls/syscall.tbl
+++ b/arch/xtensa/kernel/syscalls/syscall.tbl
@@ -406,3 +406,4 @@
433 common fspick sys_fspick
434 common pidfd_open sys_pidfd_open
435 common clone3 sys_clone3
+438 common pidfd_getfd sys_pidfd_getfd
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index 2960dedcfde8..5edbc31af51f 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -1000,6 +1000,7 @@ asmlinkage long sys_fspick(int dfd, const char __user *path, unsigned int flags)
asmlinkage long sys_pidfd_send_signal(int pidfd, int sig,
siginfo_t __user *info,
unsigned int flags);
+asmlinkage long sys_pidfd_getfd(int pidfd, int fd, unsigned int flags);
/*
* Architecture-specific system calls
diff --git a/include/uapi/asm-generic/unistd.h b/include/uapi/asm-generic/unistd.h
index 1fc8faa6e973..d36ec3d645bd 100644
--- a/include/uapi/asm-generic/unistd.h
+++ b/include/uapi/asm-generic/unistd.h
@@ -850,9 +850,11 @@ __SYSCALL(__NR_pidfd_open, sys_pidfd_open)
#define __NR_clone3 435
__SYSCALL(__NR_clone3, sys_clone3)
#endif
+#define __NR_pidfd_getfd 438
+__SYSCALL(__NR_pidfd_getfd, sys_pidfd_getfd)
#undef __NR_syscalls
-#define __NR_syscalls 436
+#define __NR_syscalls 439
/*
* 32 bit systems traditionally used different
diff --git a/kernel/pid.c b/kernel/pid.c
index 2278e249141d..4a551f947869 100644
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -578,3 +578,106 @@ void __init pid_idr_init(void)
init_pid_ns.pid_cachep = KMEM_CACHE(pid,
SLAB_HWCACHE_ALIGN | SLAB_PANIC | SLAB_ACCOUNT);
}
+
+static struct file *__pidfd_fget(struct task_struct *task, int fd)
+{
+ struct file *file;
+ int ret;
+
+ ret = mutex_lock_killable(&task->signal->cred_guard_mutex);
+ if (ret)
+ return ERR_PTR(ret);
+
+ if (!ptrace_may_access(task, PTRACE_MODE_ATTACH_REALCREDS)) {
+ file = ERR_PTR(-EPERM);
+ goto out;
+ }
+
+ file = fget_task(task, fd);
+ if (!file)
+ file = ERR_PTR(-EBADF);
+
+out:
+ mutex_unlock(&task->signal->cred_guard_mutex);
+ return file;
+}
+
+static int pidfd_getfd(struct pid *pid, int fd)
+{
+ struct task_struct *task;
+ struct file *file;
+ int ret, retfd;
+
+ task = get_pid_task(pid, PIDTYPE_PID);
+ if (!task)
+ return -ESRCH;
+
+ file = __pidfd_fget(task, fd);
+ put_task_struct(task);
+ if (IS_ERR(file))
+ return PTR_ERR(file);
+
+ retfd = get_unused_fd_flags(O_CLOEXEC);
+ if (retfd < 0) {
+ ret = retfd;
+ goto out;
+ }
+
+ /*
+ * security_file_receive must come last since it may have side effects
+ * and cannot be reversed.
+ */
+ ret = security_file_receive(file);
+ if (ret)
+ goto out_put_fd;
+
+ fd_install(retfd, file);
+ return retfd;
+
+out_put_fd:
+ put_unused_fd(retfd);
+out:
+ fput(file);
+ return ret;
+}
+
+/**
+ * sys_pidfd_getfd() - Get a file descriptor from another process
+ *
+ * @pidfd: the pidfd file descriptor of the process
+ * @fd: the file descriptor number to get
+ * @flags: flags on how to get the fd (reserved)
+ *
+ * This syscall gets a copy of a file descriptor from another process
+ * based on the pidfd, and file descriptor number. It requires that
+ * the calling process has the ability to ptrace the process represented
+ * by the pidfd. The process which is having its file descriptor copied
+ * is otherwise unaffected.
+ *
+ * Return: On success, a cloexec file descriptor is returned.
+ * On error, a negative errno number will be returned.
+ */
+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 (!f.file)
+ return -EBADF;
+
+ pid = pidfd_pid(f.file);
+ if (IS_ERR(pid))
+ ret = PTR_ERR(pid);
+ else
+ ret = pidfd_getfd(pid, fd);
+
+ fdput(f);
+ return ret;
+}
--
2.20.1
^ permalink raw reply related
* [PATCH v7 1/3] vfs, fdtable: Add get_task_file helper
From: Sargun Dhillon @ 2019-12-26 18:02 UTC (permalink / raw)
To: linux-kernel, containers, linux-api, linux-fsdevel
Cc: tycho, jannh, cyphar, christian.brauner, oleg, luto, viro,
gpascutto, ealvarez, fweimer, jld, arnd
This introduces a function which can be used to fetch a file, given an
arbitrary task. As long as the user holds a reference (refcnt) to the
task_struct it is safe to call, and will either return NULL on failure,
or a pointer to the file, with a refcnt.
This patch is based on Oleg Nesterov's (cf. [1]) patch from September
2018.
[1]: Link: https://lore.kernel.org/r/20180915160423.GA31461@redhat.com
Signed-off-by: Sargun Dhillon <sargun@sargun.me>
Suggested-by: Oleg Nesterov <oleg@redhat.com>
Acked-by: Christian Brauner <christian.brauner@ubuntu.com>
---
fs/file.c | 21 +++++++++++++++++++--
include/linux/file.h | 2 ++
2 files changed, 21 insertions(+), 2 deletions(-)
diff --git a/fs/file.c b/fs/file.c
index 2f4fcf985079..d3bdb1717d1e 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -706,9 +706,9 @@ void do_close_on_exec(struct files_struct *files)
spin_unlock(&files->file_lock);
}
-static struct file *__fget(unsigned int fd, fmode_t mask, unsigned int refs)
+static struct file *__fget_files(struct files_struct *files, unsigned int fd,
+ fmode_t mask, unsigned int refs)
{
- struct files_struct *files = current->files;
struct file *file;
rcu_read_lock();
@@ -729,6 +729,11 @@ static struct file *__fget(unsigned int fd, fmode_t mask, unsigned int refs)
return file;
}
+static inline struct file *__fget(unsigned int fd, fmode_t mask, unsigned int refs)
+{
+ return __fget_files(current->files, fd, mask, refs);
+}
+
struct file *fget_many(unsigned int fd, unsigned int refs)
{
return __fget(fd, FMODE_PATH, refs);
@@ -746,6 +751,18 @@ struct file *fget_raw(unsigned int fd)
}
EXPORT_SYMBOL(fget_raw);
+struct file *fget_task(struct task_struct *task, unsigned int fd)
+{
+ struct file *file = NULL;
+
+ task_lock(task);
+ if (task->files)
+ file = __fget_files(task->files, fd, 0, 1);
+ task_unlock(task);
+
+ return file;
+}
+
/*
* Lightweight file lookup - no refcnt increment if fd table isn't shared.
*
diff --git a/include/linux/file.h b/include/linux/file.h
index 3fcddff56bc4..c6c7b24ea9f7 100644
--- a/include/linux/file.h
+++ b/include/linux/file.h
@@ -16,6 +16,7 @@ extern void fput(struct file *);
extern void fput_many(struct file *, unsigned int);
struct file_operations;
+struct task_struct;
struct vfsmount;
struct dentry;
struct inode;
@@ -47,6 +48,7 @@ static inline void fdput(struct fd fd)
extern struct file *fget(unsigned int fd);
extern struct file *fget_many(unsigned int fd, unsigned int refs);
extern struct file *fget_raw(unsigned int fd);
+extern struct file *fget_task(struct task_struct *task, unsigned int fd);
extern unsigned long __fdget(unsigned int fd);
extern unsigned long __fdget_raw(unsigned int fd);
extern unsigned long __fdget_pos(unsigned int fd);
--
2.20.1
^ permalink raw reply related
* [PATCH v7 0/3] Add pidfd_getfd syscall
From: Sargun Dhillon @ 2019-12-26 18:02 UTC (permalink / raw)
To: linux-kernel, containers, linux-api, linux-fsdevel
Cc: tycho, jannh, cyphar, christian.brauner, oleg, luto, viro,
gpascutto, ealvarez, fweimer, jld, arnd
This patchset introduces a mechanism (pidfd_getfd syscall) to get file
descriptors from other processes via pidfd. Although this can be achieved
using SCM_RIGHTS, and parasitic code injection, this offers a more
straightforward mechanism, with less overhead and complexity. The process
under manipulation's fd still remains valid, and unmodified by the
copy operation.
It introduces a flags field. The flags field is reserved a the moment,
but the intent is to extend it with the following capabilities:
* Close the remote FD when copying it
* Drop the cgroup data if it's a fd pointing a socket when copying it
The syscall numbers were chosen to be one greater than openat2.
Summary of history:
This initially started as a ptrace command. It did not require the process
to be stopped, and felt like kind of an awkward fit for ptrace. After that,
it moved to an ioctl on the pidfd. Given functionality, it made sense to
make it a syscall which did not require the process to be stopped.
Previous versions:
V6: https://lore.kernel.org/lkml/20191223210823.GA25083@ircssh-2.c.rugged-nimbus-611.internal/
V5: https://lore.kernel.org/lkml/20191220232746.GA20215@ircssh-2.c.rugged-nimbus-611.internal/
V4: https://lore.kernel.org/lkml/20191218235310.GA17259@ircssh-2.c.rugged-nimbus-611.internal/
V3: https://lore.kernel.org/lkml/20191217005842.GA14379@ircssh-2.c.rugged-nimbus-611.internal/
V2: https://lore.kernel.org/lkml/20191209070446.GA32336@ircssh-2.c.rugged-nimbus-611.internal/
RFC V1: https://lore.kernel.org/lkml/20191205234450.GA26369@ircssh-2.c.rugged-nimbus-611.internal/
Changes since v6:
* Proper attribution of get_task_file helper
* Move all types for syscall to int to represent fd
Changes since v5:
* Drop pidfd_getfd_options struct and replace with a flags field
Changes since v4:
* Turn into a syscall
* Move to PTRACE_MODE_ATTACH_REALCREDS from PTRACE_MODE_READ_REALCREDS
* Remove the sample code. This will come in another patchset, as the
new self-tests cover all the functionality.
Changes since v3:
* Add self-test
* Move to ioctl passing fd directly, versus args struct
* Shuffle around include files
Changes since v2:
* Move to ioctl on pidfd instead of ptrace function
* Add security check before moving file descriptor
Changes since the RFC v1:
* Introduce a new helper to fs/file.c to fetch a file descriptor from
any process. It largely uses the code suggested by Oleg, with a few
changes to fix locking
* It uses an extensible options struct to supply the FD, and option.
* I added a sample, using the code from the user-ptrace sample
Sargun Dhillon (3):
vfs, fdtable: Add get_task_file helper
pid: Introduce pidfd_getfd syscall
test: Add test for pidfd getfd
arch/alpha/kernel/syscalls/syscall.tbl | 1 +
arch/arm/tools/syscall.tbl | 1 +
arch/arm64/include/asm/unistd.h | 2 +-
arch/arm64/include/asm/unistd32.h | 2 +
arch/ia64/kernel/syscalls/syscall.tbl | 1 +
arch/m68k/kernel/syscalls/syscall.tbl | 1 +
arch/microblaze/kernel/syscalls/syscall.tbl | 1 +
arch/mips/kernel/syscalls/syscall_n32.tbl | 1 +
arch/mips/kernel/syscalls/syscall_n64.tbl | 1 +
arch/mips/kernel/syscalls/syscall_o32.tbl | 1 +
arch/parisc/kernel/syscalls/syscall.tbl | 1 +
arch/powerpc/kernel/syscalls/syscall.tbl | 1 +
arch/s390/kernel/syscalls/syscall.tbl | 1 +
arch/sh/kernel/syscalls/syscall.tbl | 1 +
arch/sparc/kernel/syscalls/syscall.tbl | 1 +
arch/x86/entry/syscalls/syscall_32.tbl | 1 +
arch/x86/entry/syscalls/syscall_64.tbl | 1 +
arch/xtensa/kernel/syscalls/syscall.tbl | 1 +
fs/file.c | 21 +-
include/linux/file.h | 2 +
include/linux/syscalls.h | 1 +
include/uapi/asm-generic/unistd.h | 4 +-
kernel/pid.c | 103 +++++++
tools/testing/selftests/pidfd/.gitignore | 1 +
tools/testing/selftests/pidfd/Makefile | 2 +-
.../selftests/pidfd/pidfd_getfd_test.c | 254 ++++++++++++++++++
26 files changed, 403 insertions(+), 5 deletions(-)
create mode 100644 tools/testing/selftests/pidfd/pidfd_getfd_test.c
--
2.20.1
^ permalink raw reply
* Re: [PATCH] seccomp: Check flags on seccomp_notif is unset
From: Tycho Andersen @ 2019-12-26 15:37 UTC (permalink / raw)
To: Aleksa Sarai
Cc: Christian Brauner, Sargun Dhillon, linux-kernel, linux-api, jannh,
keescook
In-Reply-To: <20191226143229.sbopynwut2hhsiwn@yavin.dot.cyphar.com>
On Fri, Dec 27, 2019 at 01:32:29AM +1100, Aleksa Sarai wrote:
> On 2019-12-26, Christian Brauner <christian.brauner@ubuntu.com> wrote:
> > On Wed, Dec 25, 2019 at 09:45:33PM +0000, Sargun Dhillon wrote:
> > > This patch is a small change in enforcement of the uapi for
> > > SECCOMP_IOCTL_NOTIF_RECV ioctl. Specificaly, the datastructure which is
> > > passed (seccomp_notif), has a flags member. Previously that could be
> > > set to a nonsense value, and we would ignore it. This ensures that
> > > no flags are set.
> > >
> > > Signed-off-by: Sargun Dhillon <sargun@sargun.me>
> > > Cc: Kees Cook <keescook@chromium.org>
> >
> > I'm fine with this since we soon want to make use of the flag argument
> > when we add a flag to get a pidfd from the seccomp notifier on receive.
> > The major users I could identify already pass in seccomp_notif with all
> > fields set to 0. If we really break users we can always revert; this
> > seems very unlikely to me though.
> >
> > One more question below, otherwise:
> >
> > Reviewed-by: Christian Brauner <christian.brauner@ubuntu.com>
> >
> > > ---
> > > kernel/seccomp.c | 7 +++++++
> > > 1 file changed, 7 insertions(+)
> > >
> > > diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> > > index 12d2227e5786..455925557490 100644
> > > --- a/kernel/seccomp.c
> > > +++ b/kernel/seccomp.c
> > > @@ -1026,6 +1026,13 @@ static long seccomp_notify_recv(struct seccomp_filter *filter,
> > > struct seccomp_notif unotif;
> > > ssize_t ret;
> > >
> > > + if (copy_from_user(&unotif, buf, sizeof(unotif)))
> > > + return -EFAULT;
> > > +
> > > + /* flags is reserved right now, make sure it's unset */
> > > + if (unotif.flags)
> > > + return -EINVAL;
> > > +
> >
> > Might it make sense to use
> >
> > err = copy_struct_from_user(&unotif, sizeof(unotif), buf, sizeof(unotif));
> > if (err)
> > return err;
> >
> > This way we check that the whole struct is 0 and report an error as soon
> > as one of the members is non-zero. That's more drastic but it'd ensure
> > that other fields can be used in the future for whatever purposes.
> > It would also let us get rid of the memset() below.
>
> Given that this isn't an extensible struct, it would be simpler to just do
> check_zeroed_user() -- copy_struct_from_user() is overkill. That would
> also remove the need for any copy_from_user()s and the memset can be
> dropped by just doing
>
> struct seccomp_notif unotif = {};
This doesn't zero the padding according to the C standard, so no, you
can't drop the memset, or you may leak kernel stack bits.
As for the rest of it, while it is an ABI change I think all of the
users are CC'd on this thread, and it's an obvious goof on my part :).
So:
Acked-by: Tycho Andersen <tycho@tycho.ws>
Tycho
^ permalink raw reply
* Re: [PATCH] seccomp: Check flags on seccomp_notif is unset
From: Christian Brauner @ 2019-12-26 14:34 UTC (permalink / raw)
To: Aleksa Sarai
Cc: Sargun Dhillon, linux-kernel, linux-api, tycho, jannh, keescook
In-Reply-To: <20191226143229.sbopynwut2hhsiwn@yavin.dot.cyphar.com>
On December 26, 2019 3:32:29 PM GMT+01:00, Aleksa Sarai <cyphar@cyphar.com> wrote:
>On 2019-12-26, Christian Brauner <christian.brauner@ubuntu.com> wrote:
>> On Wed, Dec 25, 2019 at 09:45:33PM +0000, Sargun Dhillon wrote:
>> > This patch is a small change in enforcement of the uapi for
>> > SECCOMP_IOCTL_NOTIF_RECV ioctl. Specificaly, the datastructure
>which is
>> > passed (seccomp_notif), has a flags member. Previously that could
>be
>> > set to a nonsense value, and we would ignore it. This ensures that
>> > no flags are set.
>> >
>> > Signed-off-by: Sargun Dhillon <sargun@sargun.me>
>> > Cc: Kees Cook <keescook@chromium.org>
>>
>> I'm fine with this since we soon want to make use of the flag
>argument
>> when we add a flag to get a pidfd from the seccomp notifier on
>receive.
>> The major users I could identify already pass in seccomp_notif with
>all
>> fields set to 0. If we really break users we can always revert; this
>> seems very unlikely to me though.
>>
>> One more question below, otherwise:
>>
>> Reviewed-by: Christian Brauner <christian.brauner@ubuntu.com>
>>
>> > ---
>> > kernel/seccomp.c | 7 +++++++
>> > 1 file changed, 7 insertions(+)
>> >
>> > diff --git a/kernel/seccomp.c b/kernel/seccomp.c
>> > index 12d2227e5786..455925557490 100644
>> > --- a/kernel/seccomp.c
>> > +++ b/kernel/seccomp.c
>> > @@ -1026,6 +1026,13 @@ static long seccomp_notify_recv(struct
>seccomp_filter *filter,
>> > struct seccomp_notif unotif;
>> > ssize_t ret;
>> >
>> > + if (copy_from_user(&unotif, buf, sizeof(unotif)))
>> > + return -EFAULT;
>> > +
>> > + /* flags is reserved right now, make sure it's unset */
>> > + if (unotif.flags)
>> > + return -EINVAL;
>> > +
>>
>> Might it make sense to use
>>
>> err = copy_struct_from_user(&unotif, sizeof(unotif), buf,
>sizeof(unotif));
>> if (err)
>> return err;
>>
>> This way we check that the whole struct is 0 and report an error as
>soon
>> as one of the members is non-zero. That's more drastic but it'd
>ensure
>> that other fields can be used in the future for whatever purposes.
>> It would also let us get rid of the memset() below.
>
>Given that this isn't an extensible struct, it would be simpler to just
>do
>check_zeroed_user() -- copy_struct_from_user() is overkill. That would
>also remove the need for any copy_from_user()s and the memset can be
>dropped by just doing
>
> struct seccomp_notif unotif = {};
>
>> > memset(&unotif, 0, sizeof(unotif));
>> >
>> > ret = down_interruptible(&filter->notif->request);
>> > --
>> > 2.20.1
>> >
It is an extensible struct. That's why we have notifier size checking built in.
^ permalink raw reply
* Re: [PATCH] seccomp: Check flags on seccomp_notif is unset
From: Aleksa Sarai @ 2019-12-26 14:32 UTC (permalink / raw)
To: Christian Brauner
Cc: Sargun Dhillon, linux-kernel, linux-api, tycho, jannh, keescook
In-Reply-To: <20191226115245.usf7z5dkui7ndp4w@wittgenstein>
[-- Attachment #1: Type: text/plain, Size: 2562 bytes --]
On 2019-12-26, Christian Brauner <christian.brauner@ubuntu.com> wrote:
> On Wed, Dec 25, 2019 at 09:45:33PM +0000, Sargun Dhillon wrote:
> > This patch is a small change in enforcement of the uapi for
> > SECCOMP_IOCTL_NOTIF_RECV ioctl. Specificaly, the datastructure which is
> > passed (seccomp_notif), has a flags member. Previously that could be
> > set to a nonsense value, and we would ignore it. This ensures that
> > no flags are set.
> >
> > Signed-off-by: Sargun Dhillon <sargun@sargun.me>
> > Cc: Kees Cook <keescook@chromium.org>
>
> I'm fine with this since we soon want to make use of the flag argument
> when we add a flag to get a pidfd from the seccomp notifier on receive.
> The major users I could identify already pass in seccomp_notif with all
> fields set to 0. If we really break users we can always revert; this
> seems very unlikely to me though.
>
> One more question below, otherwise:
>
> Reviewed-by: Christian Brauner <christian.brauner@ubuntu.com>
>
> > ---
> > kernel/seccomp.c | 7 +++++++
> > 1 file changed, 7 insertions(+)
> >
> > diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> > index 12d2227e5786..455925557490 100644
> > --- a/kernel/seccomp.c
> > +++ b/kernel/seccomp.c
> > @@ -1026,6 +1026,13 @@ static long seccomp_notify_recv(struct seccomp_filter *filter,
> > struct seccomp_notif unotif;
> > ssize_t ret;
> >
> > + if (copy_from_user(&unotif, buf, sizeof(unotif)))
> > + return -EFAULT;
> > +
> > + /* flags is reserved right now, make sure it's unset */
> > + if (unotif.flags)
> > + return -EINVAL;
> > +
>
> Might it make sense to use
>
> err = copy_struct_from_user(&unotif, sizeof(unotif), buf, sizeof(unotif));
> if (err)
> return err;
>
> This way we check that the whole struct is 0 and report an error as soon
> as one of the members is non-zero. That's more drastic but it'd ensure
> that other fields can be used in the future for whatever purposes.
> It would also let us get rid of the memset() below.
Given that this isn't an extensible struct, it would be simpler to just do
check_zeroed_user() -- copy_struct_from_user() is overkill. That would
also remove the need for any copy_from_user()s and the memset can be
dropped by just doing
struct seccomp_notif unotif = {};
> > memset(&unotif, 0, sizeof(unotif));
> >
> > ret = down_interruptible(&filter->notif->request);
> > --
> > 2.20.1
> >
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH v3 0/8] Rework random blocking
From: Theodore Y. Ts'o @ 2019-12-26 14:04 UTC (permalink / raw)
To: Stephan Mueller
Cc: Andy Lutomirski, Andy Lutomirski, LKML, Linux API, Kees Cook,
Jason A. Donenfeld, Ahmed S. Darwish, Lennart Poettering,
Eric W. Biederman, Alexander E. Patrakov, Michael Kerrisk,
Willy Tarreau, Matthew Garrett, Ext4 Developers List, linux-man
In-Reply-To: <4820831.xlnk3tY4r2@tauon.chronox.de>
On Thu, Dec 26, 2019 at 01:03:34PM +0100, Stephan Mueller wrote:
> Agreed. I was just trying to outline that the removal of the blocking_pool is
> a good thing. Even when we decide that random.c should receive a TRNG, we do
> not need to re-add a blocking pool, but can easily use the existing ChaCha20
> DRNG (most likely with its own instance).
Well, it depends on what you mean by "TRNG" --- the ChaCha20 DRNG only
has a state of 256 bits. So if you want to only depend on "true
entropy" you can't extract more than 256 bits without violating that
assumption, at least if you're using a very strict definition of TRNG.
By getting rid of the blocking pool, and making /dev/random work like
getrandom with flags set to 0, we're effectively abandoning any kind
of assertion that /dev/random is some kind of TRNG. This is not
insane; this is what the *BSD's have always done.
But once we do this, and /dev/random takes on the semantics of "block
until the CRNG has been initialized, and then it won't block after
that", if we change it so that it now has some different semantics,
such as "one you extract a 256-bit key, the read from /dev/random will
block until we can refill it, which might take seconds, minutes or
hours", will be considered a regression, and we can't do that.
Of course, we can hope that people will be using getrandom() and there
will be very few new users of the /dev/random pathname. But nothing
is ever guaranteed..
- Ted
^ permalink raw reply
* Re: [PATCH v3 0/8] Rework random blocking
From: Andy Lutomirski @ 2019-12-26 12:46 UTC (permalink / raw)
To: Stephan Mueller
Cc: Andy Lutomirski, Ted Ts'o, LKML, Linux API, Kees Cook,
Jason A. Donenfeld, Ahmed S. Darwish, Lennart Poettering,
Eric W. Biederman, Alexander E. Patrakov, Michael Kerrisk,
Willy Tarreau, Matthew Garrett, Ext4 Developers List, linux-man
In-Reply-To: <4820831.xlnk3tY4r2@tauon.chronox.de>
> On Dec 26, 2019, at 8:04 PM, Stephan Mueller <smueller@chronox.de> wrote:
>
> Am Donnerstag, 26. Dezember 2019, 12:12:29 CET schrieb Andy Lutomirski:
>
> Hi Andy,
>
>>>> On Dec 26, 2019, at 5:29 PM, Stephan Müller <smueller@chronox.de> wrote:
>>>
>>> Am Montag, 23. Dezember 2019, 09:20:43 CET schrieb Andy Lutomirski:
>>>
>>> Hi Andy,
>>>
>>>> There are some open questions and future work here:
>>>>
>>>> Should the kernel provide an interface to get software-generated
>>>> "true random" numbers? I can think of only one legitimate reason to
>>>> use such an interface: compliance with government standards. If the
>>>> kernel provides such an interface going forward, I think it should
>>>> be a brand new character device, and it should have a default mode
>>>> 0440 or similar. Software-generated "true random numbers" are a
>>>> very limited resource, and resource exhaustion is a big deal. Ask
>>>> anyone who has twiddled their thumbs while waiting for gnupg to
>>>> generate a key. If we think the kernel might do such a thing, then
>>>> patches 5-8 could be tabled for now.
>>>
>>> What about offering a compile-time option to enable or disable such code?
>>> Note, with the existing random.c code base, there is no need to have a
>>> separate blocking_pool. The ChaCha20 DRNG could be used for that very same
>>> purpose, provided that in case these true random numbers are generated
>>> when
>>> the Chacha20 DRNG received an equal amount of "unused" entropy.
>>
>> This scares me. The DRNG should be simple and easy to understand. If we’re
>> tapping extra numbers in some weird way, then I would be more comfortable
>> with some clear assurance that this doesn’t break the security. If we’re
>> tapping numbers in the same way as normal urandom, then I don’t really see
>> the point.
>
> Agreed. I was just trying to outline that the removal of the blocking_pool is
> a good thing. Even when we decide that random.c should receive a TRNG, we do
> not need to re-add a blocking pool, but can easily use the existing ChaCha20
> DRNG (most likely with its own instance).
Fair enough.
>
>>>> Alternatively, perhaps the kernel should instead provide a
>>>> privileged interface to read out raw samples from the various
>>>> entropy sources, and users who care could have a user daemon that
>>>> does something intelligent with them. This would push the mess of
>>>> trying to comply with whatever standards are involved to userspace.
>>>> Userspace could then export "true randomness" via CUSE if it is so
>>>> inclined, or could have a socket with a well-known name, or whatever
>>>> else seems appropriate.
>>>
>>> With the patch set v26 of my LRNG I offer another possible alternative
>>> avoiding any additional character device file and preventing the
>>> starvation of legitimate use cases: the LRNG has an entropy pool that
>>> leaves different levels of entropy in the pool depending on the use cases
>>> of this data.
>>>
>>> If an unprivileged caller requests true random data, at least 1024 bits of
>>> entropy is left in the pool. I.e. all entropy above that point is
>>> available
>>> for this request type. Note, even namespaces fall into this category
>>> considering that unprivileged users can create a user name space in which
>>> they can become root.
>>
>> This doesn’t solve the problem. If two different users run stupid programs
>> like gnupg, they will starve each other.
>
> But such scenario will always occur, will it not? If there are two callers for
> a limited resource, they will content if one "over-uses" the resource. My idea
> was to provide an interface where its use does not starve other more relevant
> use cases (e.g. seeding of the DRNGs). I.e. a user of a TRNG has the right to
> be DoSed - that is the price to pay when using this concept.
Maybe I’m just cynical, but I expect that, if the feature is available to everyone, then lots of user programmers will use it even though they don’t need to. If, on the other hand, there is a barrier to entry, then people will be more likely to stop and think.
Even gnupg could have been more clever — when generating a 4096-bit RSA key, there is no actual need for 4096 bits of entropy, however entropy is defined. 256 bits would have been more than adequate.
(FWIW, my personal view is that 512 bits, in the sense of “the distribution being sampled produces no output with probability greater than about 2^-512”, is a good upper limit for even the most paranoid. This is because it’s reasonable to assume that an attacker can’t do more than 2^128 operations. As djb has noted, multi-target attacks mean that you can amplify success probability in some cases by a factor that won’t exceed 2^128. Some day, quantum computers might square-root everything, giving 512 bits. Actually, quantum computers won’t square root everything, but much more complicated analysis is needed to get a believable bound.)
—Andy
^ permalink raw reply
* Re: [PATCH v3 0/8] Rework random blocking
From: Stephan Mueller @ 2019-12-26 12:03 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Andy Lutomirski, Ted Ts'o, LKML, Linux API, Kees Cook,
Jason A. Donenfeld, Ahmed S. Darwish, Lennart Poettering,
Eric W. Biederman, Alexander E. Patrakov, Michael Kerrisk,
Willy Tarreau, Matthew Garrett, Ext4 Developers List, linux-man
In-Reply-To: <888017FA-06A1-42EF-9FC0-46629138DA9E@amacapital.net>
Am Donnerstag, 26. Dezember 2019, 12:12:29 CET schrieb Andy Lutomirski:
Hi Andy,
> > On Dec 26, 2019, at 5:29 PM, Stephan Müller <smueller@chronox.de> wrote:
> >
> > Am Montag, 23. Dezember 2019, 09:20:43 CET schrieb Andy Lutomirski:
> >
> > Hi Andy,
> >
> >> There are some open questions and future work here:
> >>
> >> Should the kernel provide an interface to get software-generated
> >> "true random" numbers? I can think of only one legitimate reason to
> >> use such an interface: compliance with government standards. If the
> >> kernel provides such an interface going forward, I think it should
> >> be a brand new character device, and it should have a default mode
> >> 0440 or similar. Software-generated "true random numbers" are a
> >> very limited resource, and resource exhaustion is a big deal. Ask
> >> anyone who has twiddled their thumbs while waiting for gnupg to
> >> generate a key. If we think the kernel might do such a thing, then
> >> patches 5-8 could be tabled for now.
> >
> > What about offering a compile-time option to enable or disable such code?
> > Note, with the existing random.c code base, there is no need to have a
> > separate blocking_pool. The ChaCha20 DRNG could be used for that very same
> > purpose, provided that in case these true random numbers are generated
> > when
> > the Chacha20 DRNG received an equal amount of "unused" entropy.
>
> This scares me. The DRNG should be simple and easy to understand. If we’re
> tapping extra numbers in some weird way, then I would be more comfortable
> with some clear assurance that this doesn’t break the security. If we’re
> tapping numbers in the same way as normal urandom, then I don’t really see
> the point.
Agreed. I was just trying to outline that the removal of the blocking_pool is
a good thing. Even when we decide that random.c should receive a TRNG, we do
not need to re-add a blocking pool, but can easily use the existing ChaCha20
DRNG (most likely with its own instance).
> >> Alternatively, perhaps the kernel should instead provide a
> >> privileged interface to read out raw samples from the various
> >> entropy sources, and users who care could have a user daemon that
> >> does something intelligent with them. This would push the mess of
> >> trying to comply with whatever standards are involved to userspace.
> >> Userspace could then export "true randomness" via CUSE if it is so
> >> inclined, or could have a socket with a well-known name, or whatever
> >> else seems appropriate.
> >
> > With the patch set v26 of my LRNG I offer another possible alternative
> > avoiding any additional character device file and preventing the
> > starvation of legitimate use cases: the LRNG has an entropy pool that
> > leaves different levels of entropy in the pool depending on the use cases
> > of this data.
> >
> > If an unprivileged caller requests true random data, at least 1024 bits of
> > entropy is left in the pool. I.e. all entropy above that point is
> > available
> > for this request type. Note, even namespaces fall into this category
> > considering that unprivileged users can create a user name space in which
> > they can become root.
>
> This doesn’t solve the problem. If two different users run stupid programs
> like gnupg, they will starve each other.
But such scenario will always occur, will it not? If there are two callers for
a limited resource, they will content if one "over-uses" the resource. My idea
was to provide an interface where its use does not starve other more relevant
use cases (e.g. seeding of the DRNGs). I.e. a user of a TRNG has the right to
be DoSed - that is the price to pay when using this concept.
>
> As I see it, there are two major problems with /dev/random right now: it’s
> prone to DoS (i.e. starvation, malicious or otherwise), and, because no
> privilege is required, it’s prone to misuse. Gnupg is misuse, full stop. If
> we add a new unprivileged interface, gnupg and similar programs will use
> it, and we lose all over again.
I am under the impression that the over-using of /dev/random is that other use
cases like re-seeding of /dev/urandom are DoSed. But if there is a hog on the
TRNG that only causes a problem for itself for other equally specialized
applications, so be it.
When using a char device with rights of 440, then at least all applications
part of that special group will still content for TRNG data. The only
additional benefit I would currently see for a char device with permissions of
440 is that an admin can control which applications have access to the TRNG to
begin with. But when an attacker can fool those applications to use more
random data, we again have a DoS. So, we just pushed the issue down the road
without solving it (and I think there is no solution for the issue that TRNG
users can DoS each other).
Speaking of GnuPG and after having some discussions with Werner Koch, I think
the reason for using /dev/random was that this (used to be) the only
randomness source that has a guarantee of being seeded (Werner is not
participating in this thread, so I hope I am not misrepresenting his words).
With the presence of getrandom(2), this is now solved. IIRC libgcrypt has
received support for getrandom(2) and the issue of blocking on /dev/random
should now be a thing of the past with recent libgcrypt (and thus GnuPG)
versions.
Ciao
Stephan
^ permalink raw reply
* Re: [PATCH] seccomp: Check flags on seccomp_notif is unset
From: Christian Brauner @ 2019-12-26 11:52 UTC (permalink / raw)
To: Sargun Dhillon; +Cc: linux-kernel, linux-api, tycho, jannh, keescook
In-Reply-To: <20191225214530.GA27780@ircssh-2.c.rugged-nimbus-611.internal>
On Wed, Dec 25, 2019 at 09:45:33PM +0000, Sargun Dhillon wrote:
> This patch is a small change in enforcement of the uapi for
> SECCOMP_IOCTL_NOTIF_RECV ioctl. Specificaly, the datastructure which is
> passed (seccomp_notif), has a flags member. Previously that could be
> set to a nonsense value, and we would ignore it. This ensures that
> no flags are set.
>
> Signed-off-by: Sargun Dhillon <sargun@sargun.me>
> Cc: Kees Cook <keescook@chromium.org>
I'm fine with this since we soon want to make use of the flag argument
when we add a flag to get a pidfd from the seccomp notifier on receive.
The major users I could identify already pass in seccomp_notif with all
fields set to 0. If we really break users we can always revert; this
seems very unlikely to me though.
One more question below, otherwise:
Reviewed-by: Christian Brauner <christian.brauner@ubuntu.com>
> ---
> kernel/seccomp.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> index 12d2227e5786..455925557490 100644
> --- a/kernel/seccomp.c
> +++ b/kernel/seccomp.c
> @@ -1026,6 +1026,13 @@ static long seccomp_notify_recv(struct seccomp_filter *filter,
> struct seccomp_notif unotif;
> ssize_t ret;
>
> + if (copy_from_user(&unotif, buf, sizeof(unotif)))
> + return -EFAULT;
> +
> + /* flags is reserved right now, make sure it's unset */
> + if (unotif.flags)
> + return -EINVAL;
> +
Might it make sense to use
err = copy_struct_from_user(&unotif, sizeof(unotif), buf, sizeof(unotif));
if (err)
return err;
This way we check that the whole struct is 0 and report an error as soon
as one of the members is non-zero. That's more drastic but it'd ensure
that other fields can be used in the future for whatever purposes.
It would also let us get rid of the memset() below.
> memset(&unotif, 0, sizeof(unotif));
>
> ret = down_interruptible(&filter->notif->request);
> --
> 2.20.1
>
^ permalink raw reply
* Re: [PATCH v3 0/8] Rework random blocking
From: Stephan Mueller @ 2019-12-26 11:40 UTC (permalink / raw)
To: Matthew Garrett
Cc: Andy Lutomirski, Ted Ts'o, LKML, Linux API, Kees Cook,
Jason A. Donenfeld, Ahmed S. Darwish, Lennart Poettering,
Eric W. Biederman, Alexander E. Patrakov, Michael Kerrisk,
Willy Tarreau, Ext4 Developers List, linux-man
In-Reply-To: <20191226100334.bsh3b3dphs4j4cvx@srcf.ucam.org>
Am Donnerstag, 26. Dezember 2019, 11:03:34 CET schrieb Matthew Garrett:
Hi Matthew,
> On Thu, Dec 26, 2019 at 10:29:00AM +0100, Stephan Müller wrote:
> > What about offering a compile-time option to enable or disable such code?
> > Note, with the existing random.c code base, there is no need to have a
> > separate blocking_pool. The ChaCha20 DRNG could be used for that very same
> > purpose, provided that in case these true random numbers are generated
> > when
> > the Chacha20 DRNG received an equal amount of "unused" entropy.
>
> I think it's reasonable to offer such an option as long as it's made
> clear that it'll break userland and should only be enabled under very
> weird circumstances. We don't want to end up in a situation where
> userland developers feel that they need to code to handle such
> situations - the only people who care about this distinction should be
> in control of their userland stack and able to cope with the
> consequences.
Ok.
>
> > If an unprivileged caller requests true random data, at least 1024 bits of
> > entropy is left in the pool. I.e. all entropy above that point is
> > available
> > for this request type. Note, even namespaces fall into this category
> > considering that unprivileged users can create a user name space in which
> > they can become root.
>
> I also feel like describing any of this as "true random data" is
> misleading. Most of our entropy sources are devices that could, given
> sufficient information, be modelled accurately. We're not sampling
> quantum events here.
I am fine using any terminology that fits.
The terminology I used comes from the German AIS 31:
"""
True RNG: A device or mechanism for which the output values depend on some
unpredictable source (noise source, entropy source) that produces entropy.
Note: The class of TRNGs splits into two subclasses (PTRNGs and NPTRNGs).
"""
Bottom line, a TRNG produces random numbers at an equal rate as the underlying
noise source produces entropy. E.g. if the noise source produces 10 bits of
entropy, the RNG shall only generate 10 bits of random data.
A physical TRNG (PTRNG) uses a physical phenomenon like shot noise of a diode
in a ring oscillator. Commonly, stochastical models can be created for those
noise sources.
A non-physical TRNG (NPTRNG) uses non-physical phenomenons like timing of
events as noise source. The random.c or my LRNG are NPTRNGs. For NPTRNGs it is
unlikely that there is a stochastical model.
Ciao
Stephan
^ permalink raw reply
* Re: [musl] Re: [PATCH] uapi: Prevent redefinition of struct iphdr
From: Daniel Kolesa @ 2019-12-26 11:13 UTC (permalink / raw)
To: David Miller, musl, dalias; +Cc: AWilcox, netdev, linux-api
In-Reply-To: <20191225.194929.1465672299217213413.davem@davemloft.net>
On Thu, Dec 26, 2019, at 04:49, David Miller wrote:
> From: Rich Felker <dalias@libc.org>
> Date: Wed, 25 Dec 2019 20:05:15 -0500
>
> > On Wed, Dec 25, 2019 at 04:34:11PM -0800, David Miller wrote:
> >> I find it really strange that this, therefore, only happens for musl
> >> and we haven't had thousands of reports of this conflict with glibc
> >> over the years.
> >
> > It's possible that there's software that's including just one of the
> > headers conditional on __GLIBC__, and including both otherwise, or
> > something like that. Arguably this should be considered unsupported
> > usage; there are plenty of headers where that doesn't work and
> > shouldn't be expected to.
>
> I don't buy that, this is waaaaaay too common a header to use.
In case of net-tools, only <linux/ip.h> is included, and never <netinet/ip.h> directly. Chances are in musl the indirect include tree happens to be different and conflicting, while in glibc it is not.
>
> Please investigate.
>
Daniel
^ permalink raw reply
* Re: [PATCH v3 0/8] Rework random blocking
From: Andy Lutomirski @ 2019-12-26 11:12 UTC (permalink / raw)
To: Stephan Müller
Cc: Andy Lutomirski, Ted Ts'o, LKML, Linux API, Kees Cook,
Jason A. Donenfeld, Ahmed S. Darwish, Lennart Poettering,
Eric W. Biederman, Alexander E. Patrakov, Michael Kerrisk,
Willy Tarreau, Matthew Garrett, Ext4 Developers List, linux-man
In-Reply-To: <9872655.prSdhymlXK@positron.chronox.de>
> On Dec 26, 2019, at 5:29 PM, Stephan Müller <smueller@chronox.de> wrote:
>
> Am Montag, 23. Dezember 2019, 09:20:43 CET schrieb Andy Lutomirski:
>
> Hi Andy,
>>
>> There are some open questions and future work here:
>>
>> Should the kernel provide an interface to get software-generated
>> "true random" numbers? I can think of only one legitimate reason to
>> use such an interface: compliance with government standards. If the
>> kernel provides such an interface going forward, I think it should
>> be a brand new character device, and it should have a default mode
>> 0440 or similar. Software-generated "true random numbers" are a
>> very limited resource, and resource exhaustion is a big deal. Ask
>> anyone who has twiddled their thumbs while waiting for gnupg to
>> generate a key. If we think the kernel might do such a thing, then
>> patches 5-8 could be tabled for now.
>
> What about offering a compile-time option to enable or disable such code?
> Note, with the existing random.c code base, there is no need to have a
> separate blocking_pool. The ChaCha20 DRNG could be used for that very same
> purpose, provided that in case these true random numbers are generated when
> the Chacha20 DRNG received an equal amount of "unused" entropy.
This scares me. The DRNG should be simple and easy to understand. If we’re tapping extra numbers in some weird way, then I would be more comfortable with some clear assurance that this doesn’t break the security. If we’re tapping numbers in the same way as normal urandom, then I don’t really see the point.
>>
>> Alternatively, perhaps the kernel should instead provide a
>> privileged interface to read out raw samples from the various
>> entropy sources, and users who care could have a user daemon that
>> does something intelligent with them. This would push the mess of
>> trying to comply with whatever standards are involved to userspace.
>> Userspace could then export "true randomness" via CUSE if it is so
>> inclined, or could have a socket with a well-known name, or whatever
>> else seems appropriate.
>
> With the patch set v26 of my LRNG I offer another possible alternative
> avoiding any additional character device file and preventing the starvation of
> legitimate use cases: the LRNG has an entropy pool that leaves different
> levels of entropy in the pool depending on the use cases of this data.
>
> If an unprivileged caller requests true random data, at least 1024 bits of
> entropy is left in the pool. I.e. all entropy above that point is available
> for this request type. Note, even namespaces fall into this category
> considering that unprivileged users can create a user name space in which they
> can become root.
This doesn’t solve the problem. If two different users run stupid programs like gnupg, they will starve each other.
As I see it, there are two major problems with /dev/random right now: it’s prone to DoS (i.e. starvation, malicious or otherwise), and, because no privilege is required, it’s prone to misuse. Gnupg is misuse, full stop. If we add a new unprivileged interface, gnupg and similar programs will use it, and we lose all over again.
^ permalink raw reply
* Re: [PATCH v3 0/8] Rework random blocking
From: Matthew Garrett @ 2019-12-26 10:03 UTC (permalink / raw)
To: Stephan Müller
Cc: Andy Lutomirski, Ted Ts'o, LKML, Linux API, Kees Cook,
Jason A. Donenfeld, Ahmed S. Darwish, Lennart Poettering,
Eric W. Biederman, Alexander E. Patrakov, Michael Kerrisk,
Willy Tarreau, Ext4 Developers List, linux-man
In-Reply-To: <9872655.prSdhymlXK@positron.chronox.de>
On Thu, Dec 26, 2019 at 10:29:00AM +0100, Stephan Müller wrote:
>
> What about offering a compile-time option to enable or disable such code?
> Note, with the existing random.c code base, there is no need to have a
> separate blocking_pool. The ChaCha20 DRNG could be used for that very same
> purpose, provided that in case these true random numbers are generated when
> the Chacha20 DRNG received an equal amount of "unused" entropy.
I think it's reasonable to offer such an option as long as it's made
clear that it'll break userland and should only be enabled under very
weird circumstances. We don't want to end up in a situation where
userland developers feel that they need to code to handle such
situations - the only people who care about this distinction should be
in control of their userland stack and able to cope with the
consequences.
> If an unprivileged caller requests true random data, at least 1024 bits of
> entropy is left in the pool. I.e. all entropy above that point is available
> for this request type. Note, even namespaces fall into this category
> considering that unprivileged users can create a user name space in which they
> can become root.
I also feel like describing any of this as "true random data" is
misleading. Most of our entropy sources are devices that could, given
sufficient information, be modelled accurately. We're not sampling
quantum events here.
--
Matthew Garrett | mjg59@srcf.ucam.org
^ permalink raw reply
* Re: [PATCH v3 0/8] Rework random blocking
From: Stephan Müller @ 2019-12-26 9:29 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Ted Ts'o, LKML, Linux API, Kees Cook, Jason A. Donenfeld,
Ahmed S. Darwish, Lennart Poettering, Eric W. Biederman,
Alexander E. Patrakov, Michael Kerrisk, Willy Tarreau,
Matthew Garrett, Ext4 Developers List, linux-man
In-Reply-To: <cover.1577088521.git.luto@kernel.org>
Am Montag, 23. Dezember 2019, 09:20:43 CET schrieb Andy Lutomirski:
Hi Andy,
>
> There are some open questions and future work here:
>
> Should the kernel provide an interface to get software-generated
> "true random" numbers? I can think of only one legitimate reason to
> use such an interface: compliance with government standards. If the
> kernel provides such an interface going forward, I think it should
> be a brand new character device, and it should have a default mode
> 0440 or similar. Software-generated "true random numbers" are a
> very limited resource, and resource exhaustion is a big deal. Ask
> anyone who has twiddled their thumbs while waiting for gnupg to
> generate a key. If we think the kernel might do such a thing, then
> patches 5-8 could be tabled for now.
What about offering a compile-time option to enable or disable such code?
Note, with the existing random.c code base, there is no need to have a
separate blocking_pool. The ChaCha20 DRNG could be used for that very same
purpose, provided that in case these true random numbers are generated when
the Chacha20 DRNG received an equal amount of "unused" entropy.
>
> Alternatively, perhaps the kernel should instead provide a
> privileged interface to read out raw samples from the various
> entropy sources, and users who care could have a user daemon that
> does something intelligent with them. This would push the mess of
> trying to comply with whatever standards are involved to userspace.
> Userspace could then export "true randomness" via CUSE if it is so
> inclined, or could have a socket with a well-known name, or whatever
> else seems appropriate.
With the patch set v26 of my LRNG I offer another possible alternative
avoiding any additional character device file and preventing the starvation of
legitimate use cases: the LRNG has an entropy pool that leaves different
levels of entropy in the pool depending on the use cases of this data.
If an unprivileged caller requests true random data, at least 1024 bits of
entropy is left in the pool. I.e. all entropy above that point is available
for this request type. Note, even namespaces fall into this category
considering that unprivileged users can create a user name space in which they
can become root.
If a non-blocking DRNG serving /dev/urandom or getrandom(2) needs reseeding,
at least 512 bits of entropy is left in the pool. Each DRNG seeding operation
requires at least 128 bits and at most 256 bits of entropy. This means that at
least 2 reseed operations worth of entropy is found in the entropy pool even
though massive amount of true random numbers are requested by unprivileged
users.
If a privileged caller requests true random numbers, the entropy pool is
allowed to be exhausted.
Access to the true random number generator is provided with getrandom(2) and
the GRND_TRUERANDOM flag. If the true random number generator (TRNG) is not
compiled or not present, -EOPNOTSUPP is returned.
Entire patch set:
Reviewed-by: Stephan Müller <smueller@chronox.de>
Ciao
Stephan
^ permalink raw reply
* Re: [musl] Re: [PATCH] uapi: Prevent redefinition of struct iphdr
From: David Miller @ 2019-12-26 3:49 UTC (permalink / raw)
To: dalias; +Cc: AWilcox, netdev, linux-api, musl
In-Reply-To: <20191226010515.GD30412@brightrain.aerifal.cx>
From: Rich Felker <dalias@libc.org>
Date: Wed, 25 Dec 2019 20:05:15 -0500
> On Wed, Dec 25, 2019 at 04:34:11PM -0800, David Miller wrote:
>> I find it really strange that this, therefore, only happens for musl
>> and we haven't had thousands of reports of this conflict with glibc
>> over the years.
>
> It's possible that there's software that's including just one of the
> headers conditional on __GLIBC__, and including both otherwise, or
> something like that. Arguably this should be considered unsupported
> usage; there are plenty of headers where that doesn't work and
> shouldn't be expected to.
I don't buy that, this is waaaaaay too common a header to use.
Please investigate.
^ permalink raw reply
* Re: Re: [PATCH] uapi: Prevent redefinition of struct iphdr
From: Rich Felker @ 2019-12-26 1:05 UTC (permalink / raw)
To: David Miller; +Cc: AWilcox, netdev, linux-api, musl
In-Reply-To: <20191225.163411.1590483851343305623.davem@davemloft.net>
On Wed, Dec 25, 2019 at 04:34:11PM -0800, David Miller wrote:
> From: "A. Wilcox" <AWilcox@Wilcox-Tech.com>
> Date: Sun, 22 Dec 2019 00:02:27 -0600
>
> > @@ -83,6 +83,13 @@
> >
> > #define IPV4_BEET_PHMAXLEN 8
> >
> > +/* Allow libcs to deactivate this - musl has its own copy in <netinet/ip.h> */
> > +
> > +#ifndef __UAPI_DEF_IPHDR
> > +#define __UAPI_DEF_IPHDR 1
> > +#endif
>
> How is this a musl-only problem?
I don't think it is, unless glibc's includes linux/ip.h to get the
definition, which does not seem to be the case -- at least not on the
Debian system I had handy to check on.
> I see that glibc also defines struct iphdr
> in netinet/ip.h, so why doesn't it also suffer from this?
Maybe it does.
> I find it really strange that this, therefore, only happens for musl
> and we haven't had thousands of reports of this conflict with glibc
> over the years.
It's possible that there's software that's including just one of the
headers conditional on __GLIBC__, and including both otherwise, or
something like that. Arguably this should be considered unsupported
usage; there are plenty of headers where that doesn't work and
shouldn't be expected to.
> I want an explanation, and suitably appropriate adjustments to the commit
> message and comments of this change.
Agreed. Commit messages should not imply that something is a
musl-specific workaround when it's generally the right thing to do.
Rich
^ permalink raw reply
* Re: [PATCH] uapi: Prevent redefinition of struct iphdr
From: David Miller @ 2019-12-26 0:34 UTC (permalink / raw)
To: AWilcox; +Cc: netdev, linux-api, musl
In-Reply-To: <20191222060227.7089-1-AWilcox@Wilcox-Tech.com>
From: "A. Wilcox" <AWilcox@Wilcox-Tech.com>
Date: Sun, 22 Dec 2019 00:02:27 -0600
> @@ -83,6 +83,13 @@
>
> #define IPV4_BEET_PHMAXLEN 8
>
> +/* Allow libcs to deactivate this - musl has its own copy in <netinet/ip.h> */
> +
> +#ifndef __UAPI_DEF_IPHDR
> +#define __UAPI_DEF_IPHDR 1
> +#endif
How is this a musl-only problem? I see that glibc also defines struct iphdr
in netinet/ip.h, so why doesn't it also suffer from this?
I find it really strange that this, therefore, only happens for musl
and we haven't had thousands of reports of this conflict with glibc
over the years.
I want an explanation, and suitably appropriate adjustments to the commit
message and comments of this change.
Thank you.
^ permalink raw reply
* Re: [PATCH v6 09/10] proc: add option to mount only a pids subset
From: kbuild test robot @ 2019-12-25 23:08 UTC (permalink / raw)
Cc: kbuild-all, LKML, Kernel Hardening, Linux API, Linux FS Devel,
Linux Security Module, Akinobu Mita, Alexander Viro,
Alexey Dobriyan, Alexey Gladkov, Andrew Morton, Andy Lutomirski,
Daniel Micay, Djalal Harouni, Dmitry V . Levin,
Eric W . Biederman, Greg Kroah-Hartman, Ingo Molnar, J . Bruce
In-Reply-To: <20191225125151.1950142-10-gladkov.alexey@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2340 bytes --]
Hi Alexey,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on linux/master]
[cannot apply to lwn/docs-next linus/master v5.5-rc3 next-20191220]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Alexey-Gladkov/proc-modernize-proc-to-support-multiple-private-instances/20191226-060818
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 1522d9da40bdfe502c91163e6d769332897201fa
config: i386-tinyconfig (attached as .config)
compiler: gcc-7 (Debian 7.5.0-3) 7.5.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
In file included from include/linux/sunrpc/stats.h:13:0,
from include/linux/sunrpc/clnt.h:22,
from include/linux/nfs_fs.h:32,
from init/do_mounts.c:23:
include/linux/proc_fs.h:162:47: warning: 'struct proc_info_fs' declared inside parameter list will not be visible outside of this definition or declaration
static inline void proc_fs_set_pid_gid(struct proc_info_fs *fs_info, kgid_t gid)
^~~~~~~~~~~~
include/linux/proc_fs.h: In function 'proc_fs_pidonly':
>> include/linux/proc_fs.h:168:9: warning: 'return' with a value, in function returning void
return PROC_PIDONLY_OFF;
^~~~~~~~~~~~~~~~
include/linux/proc_fs.h:166:20: note: declared here
static inline void proc_fs_pidonly(struct proc_fs_info *fs_info)
^~~~~~~~~~~~~~~
vim +/return +168 include/linux/proc_fs.h
161
> 162 static inline void proc_fs_set_pid_gid(struct proc_info_fs *fs_info, kgid_t gid)
163 {
164 }
165
166 static inline void proc_fs_pidonly(struct proc_fs_info *fs_info)
167 {
> 168 return PROC_PIDONLY_OFF;
169 }
170
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 7285 bytes --]
^ permalink raw reply
* Re: [PATCH v6 05/10] proc: add helpers to set and get proc hidepid and gid mount options
From: kbuild test robot @ 2019-12-25 23:06 UTC (permalink / raw)
Cc: kbuild-all, LKML, Kernel Hardening, Linux API, Linux FS Devel,
Linux Security Module, Akinobu Mita, Alexander Viro,
Alexey Dobriyan, Alexey Gladkov, Andrew Morton, Andy Lutomirski,
Daniel Micay, Djalal Harouni, Dmitry V . Levin,
Eric W . Biederman, Greg Kroah-Hartman, Ingo Molnar, J . Bruce
In-Reply-To: <20191225125151.1950142-6-gladkov.alexey@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 7592 bytes --]
Hi Alexey,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on linux/master]
[also build test ERROR on lwn/docs-next linus/master v5.5-rc3 next-20191220]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Alexey-Gladkov/proc-modernize-proc-to-support-multiple-private-instances/20191226-060818
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 1522d9da40bdfe502c91163e6d769332897201fa
config: i386-tinyconfig (attached as .config)
compiler: gcc-7 (Debian 7.5.0-3) 7.5.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
All error/warnings (new ones prefixed by >>):
ld: init/do_mounts.o: in function `proc_fs_pid_gid':
>> do_mounts.c:(.text+0x5): multiple definition of `proc_fs_pid_gid'; init/main.o:main.c:(.text+0x19): first defined here
ld: arch/x86/kernel/setup.o: in function `proc_fs_pid_gid':
setup.c:(.text+0x3): multiple definition of `proc_fs_pid_gid'; init/main.o:main.c:(.text+0x19): first defined here
ld: arch/x86/kernel/e820.o: in function `proc_fs_pid_gid':
e820.c:(.text+0xb1): multiple definition of `proc_fs_pid_gid'; init/main.o:main.c:(.text+0x19): first defined here
ld: arch/x86/kernel/fpu/xstate.o: in function `proc_fs_pid_gid':
xstate.c:(.text+0x36): multiple definition of `proc_fs_pid_gid'; init/main.o:main.c:(.text+0x19): first defined here
ld: arch/x86/kernel/reboot.o: in function `proc_fs_pid_gid':
reboot.c:(.text+0x1): multiple definition of `proc_fs_pid_gid'; init/main.o:main.c:(.text+0x19): first defined here
ld: arch/x86/mm/init_32.o: in function `proc_fs_pid_gid':
init_32.c:(.text+0x0): multiple definition of `proc_fs_pid_gid'; init/main.o:main.c:(.text+0x19): first defined here
ld: arch/x86/mm/fault.o: in function `proc_fs_pid_gid':
fault.c:(.text+0x908): multiple definition of `proc_fs_pid_gid'; init/main.o:main.c:(.text+0x19): first defined here
ld: arch/x86/mm/ioremap.o: in function `proc_fs_pid_gid':
ioremap.c:(.text+0x277): multiple definition of `proc_fs_pid_gid'; init/main.o:main.c:(.text+0x19): first defined here
ld: kernel/fork.o: in function `proc_fs_pid_gid':
fork.c:(.text+0x539): multiple definition of `proc_fs_pid_gid'; init/main.o:main.c:(.text+0x19): first defined here
ld: kernel/exec_domain.o: in function `proc_fs_pid_gid':
exec_domain.c:(.text+0x0): multiple definition of `proc_fs_pid_gid'; init/main.o:main.c:(.text+0x19): first defined here
ld: kernel/cpu.o: in function `proc_fs_pid_gid':
cpu.c:(.text+0x104): multiple definition of `proc_fs_pid_gid'; init/main.o:main.c:(.text+0x19): first defined here
ld: kernel/exit.o: in function `proc_fs_pid_gid':
exit.c:(.text+0x22c): multiple definition of `proc_fs_pid_gid'; init/main.o:main.c:(.text+0x19): first defined here
ld: kernel/resource.o: in function `proc_fs_pid_gid':
resource.c:(.text+0x362): multiple definition of `proc_fs_pid_gid'; init/main.o:main.c:(.text+0x19): first defined here
ld: kernel/sysctl.o: in function `proc_fs_pid_gid':
sysctl.c:(.text+0x0): multiple definition of `proc_fs_pid_gid'; init/main.o:main.c:(.text+0x19): first defined here
ld: kernel/signal.o: in function `proc_fs_pid_gid':
signal.c:(.text+0x55b): multiple definition of `proc_fs_pid_gid'; init/main.o:main.c:(.text+0x19): first defined here
ld: kernel/sched/core.o: in function `proc_fs_pid_gid':
core.c:(.text+0x2e4): multiple definition of `proc_fs_pid_gid'; init/main.o:main.c:(.text+0x19): first defined here
ld: kernel/sched/loadavg.o: in function `proc_fs_pid_gid':
loadavg.c:(.text+0x0): multiple definition of `proc_fs_pid_gid'; init/main.o:main.c:(.text+0x19): first defined here
ld: kernel/sched/clock.o: in function `proc_fs_pid_gid':
clock.c:(.text+0x0): multiple definition of `proc_fs_pid_gid'; init/main.o:main.c:(.text+0x19): first defined here
ld: kernel/sched/cputime.o: in function `proc_fs_pid_gid':
cputime.c:(.text+0x0): multiple definition of `proc_fs_pid_gid'; init/main.o:main.c:(.text+0x19): first defined here
ld: kernel/sched/idle.o: in function `proc_fs_pid_gid':
idle.c:(.text+0x2c): multiple definition of `proc_fs_pid_gid'; init/main.o:main.c:(.text+0x19): first defined here
ld: kernel/sched/fair.o: in function `proc_fs_pid_gid':
fair.c:(.text+0x8cb): multiple definition of `proc_fs_pid_gid'; init/main.o:main.c:(.text+0x19): first defined here
ld: kernel/sched/rt.o: in function `proc_fs_pid_gid':
rt.c:(.text+0x703): multiple definition of `proc_fs_pid_gid'; init/main.o:main.c:(.text+0x19): first defined here
ld: kernel/sched/deadline.o: in function `proc_fs_pid_gid':
deadline.c:(.text+0xb02): multiple definition of `proc_fs_pid_gid'; init/main.o:main.c:(.text+0x19): first defined here
ld: kernel/sched/wait.o: in function `proc_fs_pid_gid':
wait.c:(.text+0x15c): multiple definition of `proc_fs_pid_gid'; init/main.o:main.c:(.text+0x19): first defined here
ld: kernel/sched/wait_bit.o: in function `proc_fs_pid_gid':
wait_bit.c:(.text+0x9d): multiple definition of `proc_fs_pid_gid'; init/main.o:main.c:(.text+0x19): first defined here
ld: kernel/sched/swait.o: in function `proc_fs_pid_gid':
swait.c:(.text+0x4): multiple definition of `proc_fs_pid_gid'; init/main.o:main.c:(.text+0x19): first defined here
ld: kernel/sched/completion.o: in function `proc_fs_pid_gid':
completion.c:(.text+0x4): multiple definition of `proc_fs_pid_gid'; init/main.o:main.c:(.text+0x19): first defined here
ld: kernel/time/timer_list.o: in function `proc_fs_pid_gid':
timer_list.c:(.text+0x12): multiple definition of `proc_fs_pid_gid'; init/main.o:main.c:(.text+0x19): first defined here
ld: kernel/dma.o: in function `proc_fs_pid_gid':
dma.c:(.text+0x0): multiple definition of `proc_fs_pid_gid'; init/main.o:main.c:(.text+0x19): first defined here
ld: mm/vmstat.o: in function `proc_fs_pid_gid':
vmstat.c:(.text+0x0): multiple definition of `proc_fs_pid_gid'; init/main.o:main.c:(.text+0x19): first defined here
ld: mm/slab_common.o: in function `proc_fs_pid_gid':
slab_common.c:(.text+0x0): multiple definition of `proc_fs_pid_gid'; init/main.o:main.c:(.text+0x19): first defined here
ld: mm/vmalloc.o: in function `proc_fs_pid_gid':
vmalloc.c:(.text+0x4fd): multiple definition of `proc_fs_pid_gid'; init/main.o:main.c:(.text+0x19): first defined here
ld: fs/filesystems.o: in function `proc_fs_pid_gid':
filesystems.c:(.text+0x36): multiple definition of `proc_fs_pid_gid'; init/main.o:main.c:(.text+0x19): first defined here
ld: drivers/char/misc.o: in function `proc_fs_pid_gid':
misc.c:(.text+0xc4): multiple definition of `proc_fs_pid_gid'; init/main.o:main.c:(.text+0x19): first defined here
--
In file included from init/main.c:18:0:
>> include/linux/proc_fs.h:138:47: warning: 'struct proc_info_fs' declared inside parameter list will not be visible outside of this definition or declaration
static inline void proc_fs_set_pid_gid(struct proc_info_fs *fs_info, kgid_t gid)
^~~~~~~~~~~~
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 7285 bytes --]
^ permalink raw reply
* [PATCH] seccomp: Check flags on seccomp_notif is unset
From: Sargun Dhillon @ 2019-12-25 21:45 UTC (permalink / raw)
To: linux-kernel, linux-api; +Cc: tycho, jannh, christian.brauner, keescook
This patch is a small change in enforcement of the uapi for
SECCOMP_IOCTL_NOTIF_RECV ioctl. Specificaly, the datastructure which is
passed (seccomp_notif), has a flags member. Previously that could be
set to a nonsense value, and we would ignore it. This ensures that
no flags are set.
Signed-off-by: Sargun Dhillon <sargun@sargun.me>
Cc: Kees Cook <keescook@chromium.org>
---
kernel/seccomp.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index 12d2227e5786..455925557490 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -1026,6 +1026,13 @@ static long seccomp_notify_recv(struct seccomp_filter *filter,
struct seccomp_notif unotif;
ssize_t ret;
+ if (copy_from_user(&unotif, buf, sizeof(unotif)))
+ return -EFAULT;
+
+ /* flags is reserved right now, make sure it's unset */
+ if (unotif.flags)
+ return -EINVAL;
+
memset(&unotif, 0, sizeof(unotif));
ret = down_interruptible(&filter->notif->request);
--
2.20.1
^ permalink raw reply related
* Re: [PATCH v6 2/3] pid: Introduce pidfd_getfd syscall
From: Christian Brauner @ 2019-12-25 17:05 UTC (permalink / raw)
To: Sargun Dhillon
Cc: linux-kernel, containers, linux-api, linux-fsdevel, tycho, jannh,
cyphar, oleg, luto, viro, gpascutto, ealvarez, fweimer, jld, arnd
In-Reply-To: <20191223210905.GA25110@ircssh-2.c.rugged-nimbus-611.internal>
On Mon, Dec 23, 2019 at 09:09:08PM +0000, Sargun Dhillon wrote:
> This syscall allows for the retrieval of file descriptors from other
> processes, based on their pidfd. This is possible using ptrace, and
> injection of parasitic code along with using SCM_RIGHTS to move
> file descriptors between a tracee and a tracer. Unfortunately, ptrace
> comes with a high cost of requiring the process to be stopped, and
> breaks debuggers. This does not require stopping the process under
> manipulation.
>
> One reason to use this is to allow sandboxers to take actions on file
> descriptors on the behalf of another process. For example, this can be
> combined with seccomp-bpf's user notification to do on-demand fd
> extraction and take privileged actions. One such privileged action
> can be to bind a socket to a privileged port.
>
> This also adds the syscall to all architectures at the same time.
>
> /* prototype */
> /* flags is currently reserved and should be set to 0 */
> long sys_pidfd_getfd(int pidfd, int fd, unsigned int flags);
>
> /* testing */
> Ran self-test suite on x86_64
>
> Signed-off-by: Sargun Dhillon <sargun@sargun.me>
> ---
> arch/alpha/kernel/syscalls/syscall.tbl | 1 +
> arch/arm/tools/syscall.tbl | 1 +
> arch/arm64/include/asm/unistd.h | 2 +-
> arch/arm64/include/asm/unistd32.h | 2 +
> arch/ia64/kernel/syscalls/syscall.tbl | 1 +
> arch/m68k/kernel/syscalls/syscall.tbl | 1 +
> arch/microblaze/kernel/syscalls/syscall.tbl | 1 +
> arch/mips/kernel/syscalls/syscall_n32.tbl | 1 +
> arch/mips/kernel/syscalls/syscall_n64.tbl | 1 +
> arch/mips/kernel/syscalls/syscall_o32.tbl | 1 +
> arch/parisc/kernel/syscalls/syscall.tbl | 1 +
> arch/powerpc/kernel/syscalls/syscall.tbl | 1 +
> arch/s390/kernel/syscalls/syscall.tbl | 1 +
> arch/sh/kernel/syscalls/syscall.tbl | 1 +
> arch/sparc/kernel/syscalls/syscall.tbl | 1 +
> arch/x86/entry/syscalls/syscall_32.tbl | 1 +
> arch/x86/entry/syscalls/syscall_64.tbl | 1 +
> arch/xtensa/kernel/syscalls/syscall.tbl | 1 +
> include/linux/syscalls.h | 1 +
> include/uapi/asm-generic/unistd.h | 3 +-
> kernel/pid.c | 106 ++++++++++++++++++++
> 21 files changed, 128 insertions(+), 2 deletions(-)
>
> diff --git a/arch/alpha/kernel/syscalls/syscall.tbl b/arch/alpha/kernel/syscalls/syscall.tbl
> index 8e13b0b2928d..d1cac0d657b7 100644
> --- a/arch/alpha/kernel/syscalls/syscall.tbl
> +++ b/arch/alpha/kernel/syscalls/syscall.tbl
> @@ -475,3 +475,4 @@
> 543 common fspick sys_fspick
> 544 common pidfd_open sys_pidfd_open
> # 545 reserved for clone3
> +548 common pidfd_getfd sys_pidfd
> diff --git a/arch/arm/tools/syscall.tbl b/arch/arm/tools/syscall.tbl
> index 6da7dc4d79cc..ba045e2f3a60 100644
> --- a/arch/arm/tools/syscall.tbl
> +++ b/arch/arm/tools/syscall.tbl
> @@ -449,3 +449,4 @@
> 433 common fspick sys_fspick
> 434 common pidfd_open sys_pidfd_open
> 435 common clone3 sys_clone3
> +438 common pidfd_getfd sys_pidfd_getfd
> diff --git a/arch/arm64/include/asm/unistd.h b/arch/arm64/include/asm/unistd.h
> index 2629a68b8724..b722e47377a5 100644
> --- a/arch/arm64/include/asm/unistd.h
> +++ b/arch/arm64/include/asm/unistd.h
> @@ -38,7 +38,7 @@
> #define __ARM_NR_compat_set_tls (__ARM_NR_COMPAT_BASE + 5)
> #define __ARM_NR_COMPAT_END (__ARM_NR_COMPAT_BASE + 0x800)
>
> -#define __NR_compat_syscalls 436
> +#define __NR_compat_syscalls 439
> #endif
>
> #define __ARCH_WANT_SYS_CLONE
> diff --git a/arch/arm64/include/asm/unistd32.h b/arch/arm64/include/asm/unistd32.h
> index 94ab29cf4f00..a8da97a2de41 100644
> --- a/arch/arm64/include/asm/unistd32.h
> +++ b/arch/arm64/include/asm/unistd32.h
> @@ -879,6 +879,8 @@ __SYSCALL(__NR_fspick, sys_fspick)
> __SYSCALL(__NR_pidfd_open, sys_pidfd_open)
> #define __NR_clone3 435
> __SYSCALL(__NR_clone3, sys_clone3)
> +#define __NR_pidfd_getfd 438
> +__SYSCALL(__NR_pidfd_getfd, sys_pidfd_getfd)
>
> /*
> * Please add new compat syscalls above this comment and update
> diff --git a/arch/ia64/kernel/syscalls/syscall.tbl b/arch/ia64/kernel/syscalls/syscall.tbl
> index 36d5faf4c86c..2b11adfc860c 100644
> --- a/arch/ia64/kernel/syscalls/syscall.tbl
> +++ b/arch/ia64/kernel/syscalls/syscall.tbl
> @@ -356,3 +356,4 @@
> 433 common fspick sys_fspick
> 434 common pidfd_open sys_pidfd_open
> # 435 reserved for clone3
> +438 common pidfd_getfd sys_pidfd_getfd
> diff --git a/arch/m68k/kernel/syscalls/syscall.tbl b/arch/m68k/kernel/syscalls/syscall.tbl
> index a88a285a0e5f..44e879e98459 100644
> --- a/arch/m68k/kernel/syscalls/syscall.tbl
> +++ b/arch/m68k/kernel/syscalls/syscall.tbl
> @@ -435,3 +435,4 @@
> 433 common fspick sys_fspick
> 434 common pidfd_open sys_pidfd_open
> # 435 reserved for clone3
> +438 common pidfd_getfd sys_pidfd_getfd
> diff --git a/arch/microblaze/kernel/syscalls/syscall.tbl b/arch/microblaze/kernel/syscalls/syscall.tbl
> index 09b0cd7dab0a..7afa00125cc4 100644
> --- a/arch/microblaze/kernel/syscalls/syscall.tbl
> +++ b/arch/microblaze/kernel/syscalls/syscall.tbl
> @@ -441,3 +441,4 @@
> 433 common fspick sys_fspick
> 434 common pidfd_open sys_pidfd_open
> 435 common clone3 sys_clone3
> +438 common pidfd_getfd sys_pidfd_getfd
> diff --git a/arch/mips/kernel/syscalls/syscall_n32.tbl b/arch/mips/kernel/syscalls/syscall_n32.tbl
> index e7c5ab38e403..856d5ba34461 100644
> --- a/arch/mips/kernel/syscalls/syscall_n32.tbl
> +++ b/arch/mips/kernel/syscalls/syscall_n32.tbl
> @@ -374,3 +374,4 @@
> 433 n32 fspick sys_fspick
> 434 n32 pidfd_open sys_pidfd_open
> 435 n32 clone3 __sys_clone3
> +438 n32 pidfd_getfd sys_pidfd_getfd
> diff --git a/arch/mips/kernel/syscalls/syscall_n64.tbl b/arch/mips/kernel/syscalls/syscall_n64.tbl
> index 13cd66581f3b..2db6075352f3 100644
> --- a/arch/mips/kernel/syscalls/syscall_n64.tbl
> +++ b/arch/mips/kernel/syscalls/syscall_n64.tbl
> @@ -350,3 +350,4 @@
> 433 n64 fspick sys_fspick
> 434 n64 pidfd_open sys_pidfd_open
> 435 n64 clone3 __sys_clone3
> +438 n64 pidfd_getfd sys_pidfd_getfd
> diff --git a/arch/mips/kernel/syscalls/syscall_o32.tbl b/arch/mips/kernel/syscalls/syscall_o32.tbl
> index 353539ea4140..e9f9d4a9b105 100644
> --- a/arch/mips/kernel/syscalls/syscall_o32.tbl
> +++ b/arch/mips/kernel/syscalls/syscall_o32.tbl
> @@ -423,3 +423,4 @@
> 433 o32 fspick sys_fspick
> 434 o32 pidfd_open sys_pidfd_open
> 435 o32 clone3 __sys_clone3
> +438 o32 pidfd_getfd sys_pidfd_getfd
> diff --git a/arch/parisc/kernel/syscalls/syscall.tbl b/arch/parisc/kernel/syscalls/syscall.tbl
> index 285ff516150c..c58c7eb144ca 100644
> --- a/arch/parisc/kernel/syscalls/syscall.tbl
> +++ b/arch/parisc/kernel/syscalls/syscall.tbl
> @@ -433,3 +433,4 @@
> 433 common fspick sys_fspick
> 434 common pidfd_open sys_pidfd_open
> 435 common clone3 sys_clone3_wrapper
> +438 common pidfd_getfd sys_pidfd_getfd
> diff --git a/arch/powerpc/kernel/syscalls/syscall.tbl b/arch/powerpc/kernel/syscalls/syscall.tbl
> index 43f736ed47f2..707609bfe3ea 100644
> --- a/arch/powerpc/kernel/syscalls/syscall.tbl
> +++ b/arch/powerpc/kernel/syscalls/syscall.tbl
> @@ -517,3 +517,4 @@
> 433 common fspick sys_fspick
> 434 common pidfd_open sys_pidfd_open
> 435 nospu clone3 ppc_clone3
> +438 common pidfd_getfd sys_pidfd_getfd
> diff --git a/arch/s390/kernel/syscalls/syscall.tbl b/arch/s390/kernel/syscalls/syscall.tbl
> index 3054e9c035a3..185cd624face 100644
> --- a/arch/s390/kernel/syscalls/syscall.tbl
> +++ b/arch/s390/kernel/syscalls/syscall.tbl
> @@ -438,3 +438,4 @@
> 433 common fspick sys_fspick sys_fspick
> 434 common pidfd_open sys_pidfd_open sys_pidfd_open
> 435 common clone3 sys_clone3 sys_clone3
> +438 common pidfd_getfd sys_pidfd_getfd sys_pidfd_getfd
> diff --git a/arch/sh/kernel/syscalls/syscall.tbl b/arch/sh/kernel/syscalls/syscall.tbl
> index b5ed26c4c005..88f90895aad8 100644
> --- a/arch/sh/kernel/syscalls/syscall.tbl
> +++ b/arch/sh/kernel/syscalls/syscall.tbl
> @@ -438,3 +438,4 @@
> 433 common fspick sys_fspick
> 434 common pidfd_open sys_pidfd_open
> # 435 reserved for clone3
> +438 common pidfd_getfd sys_pidfd_getfd
> diff --git a/arch/sparc/kernel/syscalls/syscall.tbl b/arch/sparc/kernel/syscalls/syscall.tbl
> index 8c8cc7537fb2..218df6a2326e 100644
> --- a/arch/sparc/kernel/syscalls/syscall.tbl
> +++ b/arch/sparc/kernel/syscalls/syscall.tbl
> @@ -481,3 +481,4 @@
> 433 common fspick sys_fspick
> 434 common pidfd_open sys_pidfd_open
> # 435 reserved for clone3
> +438 common pidfd_getfd sys_pidfd_getfd
> diff --git a/arch/x86/entry/syscalls/syscall_32.tbl b/arch/x86/entry/syscalls/syscall_32.tbl
> index 15908eb9b17e..9c3101b65e0f 100644
> --- a/arch/x86/entry/syscalls/syscall_32.tbl
> +++ b/arch/x86/entry/syscalls/syscall_32.tbl
> @@ -440,3 +440,4 @@
> 433 i386 fspick sys_fspick __ia32_sys_fspick
> 434 i386 pidfd_open sys_pidfd_open __ia32_sys_pidfd_open
> 435 i386 clone3 sys_clone3 __ia32_sys_clone3
> +438 i386 pidfd_getfd sys_pidfd_getfd __ia32_sys_pidfd_getfd
> diff --git a/arch/x86/entry/syscalls/syscall_64.tbl b/arch/x86/entry/syscalls/syscall_64.tbl
> index c29976eca4a8..cef85db75a62 100644
> --- a/arch/x86/entry/syscalls/syscall_64.tbl
> +++ b/arch/x86/entry/syscalls/syscall_64.tbl
> @@ -357,6 +357,7 @@
> 433 common fspick __x64_sys_fspick
> 434 common pidfd_open __x64_sys_pidfd_open
> 435 common clone3 __x64_sys_clone3/ptregs
> +438 common pidfd_getfd __x64_sys_pidfd_getfd
>
> #
> # x32-specific system call numbers start at 512 to avoid cache impact
> diff --git a/arch/xtensa/kernel/syscalls/syscall.tbl b/arch/xtensa/kernel/syscalls/syscall.tbl
> index 25f4de729a6d..ae15183def12 100644
> --- a/arch/xtensa/kernel/syscalls/syscall.tbl
> +++ b/arch/xtensa/kernel/syscalls/syscall.tbl
> @@ -406,3 +406,4 @@
> 433 common fspick sys_fspick
> 434 common pidfd_open sys_pidfd_open
> 435 common clone3 sys_clone3
> +438 common pidfd_getfd sys_pidfd_getfd
> diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
> index 2960dedcfde8..5edbc31af51f 100644
> --- a/include/linux/syscalls.h
> +++ b/include/linux/syscalls.h
> @@ -1000,6 +1000,7 @@ asmlinkage long sys_fspick(int dfd, const char __user *path, unsigned int flags)
> asmlinkage long sys_pidfd_send_signal(int pidfd, int sig,
> siginfo_t __user *info,
> unsigned int flags);
> +asmlinkage long sys_pidfd_getfd(int pidfd, int fd, unsigned int flags);
>
> /*
> * Architecture-specific system calls
> diff --git a/include/uapi/asm-generic/unistd.h b/include/uapi/asm-generic/unistd.h
> index 1fc8faa6e973..f358488366f6 100644
> --- a/include/uapi/asm-generic/unistd.h
> +++ b/include/uapi/asm-generic/unistd.h
> @@ -850,9 +850,10 @@ __SYSCALL(__NR_pidfd_open, sys_pidfd_open)
> #define __NR_clone3 435
> __SYSCALL(__NR_clone3, sys_clone3)
> #endif
> +#define __NR_pidfd_getfd 438
This looks wrong. This should be:
#define __NR_pidfd_getfd 438
__SYSCALL(__NR_pidfd_getfd, sys_pidfd_getfd)
>
> #undef __NR_syscalls
> -#define __NR_syscalls 436
> +#define __NR_syscalls 439
>
> /*
> * 32 bit systems traditionally used different
> diff --git a/kernel/pid.c b/kernel/pid.c
> index 2278e249141d..8f65468cd857 100644
> --- a/kernel/pid.c
> +++ b/kernel/pid.c
> @@ -578,3 +578,109 @@ void __init pid_idr_init(void)
> init_pid_ns.pid_cachep = KMEM_CACHE(pid,
> SLAB_HWCACHE_ALIGN | SLAB_PANIC | SLAB_ACCOUNT);
> }
> +
> +static struct file *__pidfd_getfd_fget_task(struct task_struct *task, u32 fd)
Hm, you're using int fd in the actual syscall, the fget_task() helper
uses an unsigned int and the new helpers you add here use u32. That's a
lot of inconsistency. :)
Looking through the syscalls we have
"unsigned long fd",
"unsigned int fd",
"int fd"
I suggest to either use "unsigned int fd" or "int fd". I don't
particularly care which it is. glibc's wrappers usually expose all fds
as ints to userspace so I'm not sure if there's any benefit for us in
using unsigned int. So I guess "int fd" everywhere up until fget_task()
is reasonable:
static struct file *__pidfd_getfd_fget_task(struct task_struct *task, int fd)
static long pidfd_getfd(struct pid *pid, int fd)
SYSCALL_DEFINE3(pidfd_getfd, int, pidfd, int, fd, unsigned int, flags)
Also, please simply name this function __pidfd_fget()
> +{
> + struct file *file;
> + int ret;
> +
> + ret = mutex_lock_killable(&task->signal->cred_guard_mutex);
> + if (ret)
> + return ERR_PTR(ret);
> +
> + if (!ptrace_may_access(task, PTRACE_MODE_ATTACH_REALCREDS)) {
> + file = ERR_PTR(-EPERM);
> + goto out;
> + }
> +
> + file = fget_task(task, fd);
> + if (!file)
> + file = ERR_PTR(-EBADF);
> +
> +out:
> + mutex_unlock(&task->signal->cred_guard_mutex);
> + return file;
> +}
> +
> +static long pidfd_getfd(struct pid *pid, u32 fd)
You're returning a long here but are using "int" everywhere. Thus I'd
make this:
static int pidfd_getfd(struct pid *pid, int fd)
which also makes it consistent with how it is used in the syscall itself.
> +{
> + struct task_struct *task;
> + struct file *file;
> + int ret, retfd;
> +
> + task = get_pid_task(pid, PIDTYPE_PID);
> + if (!task)
> + return -ESRCH;
> +
> + file = __pidfd_getfd_fget_task(task, fd);
> + put_task_struct(task);
> + if (IS_ERR(file))
> + return PTR_ERR(file);
> +
> + retfd = get_unused_fd_flags(O_CLOEXEC);
+1
> + if (retfd < 0) {
> + ret = retfd;
> + goto out;
> + }
> +
> + /*
> + * security_file_receive must come last since it may have side effects
> + * and cannot be reversed.
> + */
> + ret = security_file_receive(file);
> + if (ret)
> + goto out_put_fd;
> +
> + fd_install(retfd, file);
> + return retfd;
> +
> +out_put_fd:
> + put_unused_fd(retfd);
> +out:
> + fput(file);
> + return ret;
> +}
> +
> +/**
> + * sys_pidfd_getfd() - Get a file descriptor from another process
> + *
> + * @pidfd: the pidfd file descriptor of the process
> + * @fd: the file descriptor number to get
> + * @flags: flags on how to get the fd (reserved)
> + *
> + * This syscall gets a copy of a file descriptor from another process
> + * based on their pidfd, and file descriptor number. It requires that
> + * the calling process has the ability to ptrace the process represented
> + * by the pidfd. The process which is having its file descriptor copied
> + * is otherwise unaffected.
> + *
> + * Return: On success, a file descriptor with cloexec is returned.
"On success, a cloexec file descriptor is returned." reads better, I
think.
> + * On error, a negative errno number will be returned.
> + */
> +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 (!f.file)
> + return -EBADF;
> +
> + pid = pidfd_pid(f.file);
> + if (IS_ERR(pid)) {
> + ret = PTR_ERR(pid);
> + goto out;
> + }
> +
> + ret = pidfd_getfd(pid, fd);
> +
> +out:
> + fdput(f);
> + return ret;
This can be simplified to:
pid = pidfd_pid(f.file);
if (IS_ERR(pid))
ret = PTR_ERR(pid);
else
ret = pidfd_getfd(pid, fd);
fdput(f);
return ret;
Christian
^ permalink raw reply
* Re: [PATCH v6 1/3] vfs, fdtable: Add get_task_file helper
From: Christian Brauner @ 2019-12-25 16:19 UTC (permalink / raw)
To: Sargun Dhillon
Cc: linux-kernel, containers, linux-api, linux-fsdevel, tycho, jannh,
cyphar, oleg, luto, viro, gpascutto, ealvarez, fweimer, jld, arnd
In-Reply-To: <20191223212645.3qw7my4u4rjihxjf@wittgenstein>
On Mon, Dec 23, 2019 at 10:26:46PM +0100, Christian Brauner wrote:
> On Mon, Dec 23, 2019 at 09:08:55PM +0000, Sargun Dhillon wrote:
> > This introduces a function which can be used to fetch a file, given an
> > arbitrary task. As long as the user holds a reference (refcnt) to the
> > task_struct it is safe to call, and will either return NULL on failure,
> > or a pointer to the file, with a refcnt.
> >
> > Signed-off-by: Sargun Dhillon <sargun@sargun.me>
Could you please add:
Suggested-by: Oleg Nesterov <oleg@redhat.com>
and add a line like:
"This is based on a patch sent by Oleg (cf. [1]) a while ago."
[1]: Link: https://lore.kernel.org/r/20180915160423.GA31461@redhat.com
apart from a few nits below:
Acked-by: Christian Brauner <christian.brauner@ubuntu.com>
> > ---
> > fs/file.c | 22 ++++++++++++++++++++--
> > include/linux/file.h | 2 ++
> > 2 files changed, 22 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/file.c b/fs/file.c
> > index 2f4fcf985079..0ceeb046f4f3 100644
> > --- a/fs/file.c
> > +++ b/fs/file.c
> > @@ -706,9 +706,9 @@ void do_close_on_exec(struct files_struct *files)
> > spin_unlock(&files->file_lock);
> > }
> >
> > -static struct file *__fget(unsigned int fd, fmode_t mask, unsigned int refs)
> > +static struct file *__fget_files(struct files_struct *files, unsigned int fd,
> > + fmode_t mask, unsigned int refs)
> > {
> > - struct files_struct *files = current->files;
> > struct file *file;
> >
> > rcu_read_lock();
> > @@ -729,6 +729,11 @@ static struct file *__fget(unsigned int fd, fmode_t mask, unsigned int refs)
> > return file;
> > }
> >
> > +static struct file *__fget(unsigned int fd, fmode_t mask, unsigned int refs)
This can be:
static inline struct file *__fget(...)
> > +{
> > + return __fget_files(current->files, fd, mask, refs);
> > +}
> > +
> > struct file *fget_many(unsigned int fd, unsigned int refs)
> > {
> > return __fget(fd, FMODE_PATH, refs);
> > @@ -746,6 +751,19 @@ struct file *fget_raw(unsigned int fd)
> > }
> > EXPORT_SYMBOL(fget_raw);
> >
> > +struct file *fget_task(struct task_struct *task, unsigned int fd)
> > +{
> > + struct file *file = NULL;
> > +
> > + task_lock(task);
> > + if (task->files)
> > + file = __fget_files(task->files, fd, 0, 1);
> > +
> > + task_unlock(task);
>
> Nit: remove the \n:
>
> task_lock(task);
> if (task->files)
> file = __fget_files(task->files, fd, 0, 1);
> task_unlock(task);
>
> Christian
^ permalink raw reply
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