* [PATCH 1/3] selftests/seccomp: Handle EINVAL on unshare(CLONE_NEWPID)
2024-01-24 14:13 [PATCH 0/3] selftests/seccomp seccomp_bpf test fixes Terry Tritton
@ 2024-01-24 14:13 ` Terry Tritton
2024-01-24 14:13 ` [PATCH 2/3] selftests/seccomp: Change the syscall used in KILL_THREAD test Terry Tritton
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Terry Tritton @ 2024-01-24 14:13 UTC (permalink / raw)
To: keescook, luto, wad, shuah
Cc: linux-kselftest, linux-kernel, peter.griffin, kernel-team,
bettyzhou, Terry Tritton
unshare(CLONE_NEWPID) can return EINVAL if the kernel does not have the
CONFIG_PID_NS option enabled.
Add a check on these calls to skip the test if we receive EINVAL.
Signed-off-by: Terry Tritton <terry.tritton@linaro.org>
---
tools/testing/selftests/seccomp/seccomp_bpf.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c
index 38f651469968..5e705674b706 100644
--- a/tools/testing/selftests/seccomp/seccomp_bpf.c
+++ b/tools/testing/selftests/seccomp/seccomp_bpf.c
@@ -3709,7 +3709,12 @@ TEST(user_notification_sibling_pid_ns)
ASSERT_GE(pid, 0);
if (pid == 0) {
- ASSERT_EQ(unshare(CLONE_NEWPID), 0);
+ ASSERT_EQ(unshare(CLONE_NEWPID), 0) {
+ if (errno == EPERM)
+ SKIP(return, "CLONE_NEWPID requires CAP_SYS_ADMIN");
+ else if (errno == EINVAL)
+ SKIP(return, "CLONE_NEWPID is invalid (missing CONFIG_PID_NS?)");
+ }
pid2 = fork();
ASSERT_GE(pid2, 0);
@@ -3727,6 +3732,8 @@ TEST(user_notification_sibling_pid_ns)
ASSERT_EQ(unshare(CLONE_NEWPID), 0) {
if (errno == EPERM)
SKIP(return, "CLONE_NEWPID requires CAP_SYS_ADMIN");
+ else if (errno == EINVAL)
+ SKIP(return, "CLONE_NEWPID is invalid (missing CONFIG_PID_NS?)");
}
ASSERT_EQ(errno, 0);
--
2.43.0.429.g432eaa2c6b-goog
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 2/3] selftests/seccomp: Change the syscall used in KILL_THREAD test
2024-01-24 14:13 [PATCH 0/3] selftests/seccomp seccomp_bpf test fixes Terry Tritton
2024-01-24 14:13 ` [PATCH 1/3] selftests/seccomp: Handle EINVAL on unshare(CLONE_NEWPID) Terry Tritton
@ 2024-01-24 14:13 ` Terry Tritton
2024-01-24 14:13 ` [PATCH 3/3] selftests/seccomp: user_notification_addfd check nextfd is available Terry Tritton
2024-01-24 16:21 ` [PATCH 0/3] selftests/seccomp seccomp_bpf test fixes Kees Cook
3 siblings, 0 replies; 5+ messages in thread
From: Terry Tritton @ 2024-01-24 14:13 UTC (permalink / raw)
To: keescook, luto, wad, shuah
Cc: linux-kselftest, linux-kernel, peter.griffin, kernel-team,
bettyzhou, Terry Tritton
The Bionic version of pthread_create used on Android calls the prctl
function to give the stack and thread local storage a useful name. This
will cause the KILL_THREAD test to fail as it will kill the thread as
soon as it is created.
change the test to use getpid instead of prctl.
Signed-off-by: Terry Tritton <terry.tritton@linaro.org>
---
tools/testing/selftests/seccomp/seccomp_bpf.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c
index 5e705674b706..da11b95b8872 100644
--- a/tools/testing/selftests/seccomp/seccomp_bpf.c
+++ b/tools/testing/selftests/seccomp/seccomp_bpf.c
@@ -784,7 +784,7 @@ void *kill_thread(void *data)
bool die = (bool)data;
if (die) {
- prctl(PR_GET_SECCOMP, 0, 0, 0, 0);
+ syscall(__NR_getpid);
return (void *)SIBLING_EXIT_FAILURE;
}
@@ -803,11 +803,11 @@ void kill_thread_or_group(struct __test_metadata *_metadata,
{
pthread_t thread;
void *status;
- /* Kill only when calling __NR_prctl. */
+ /* Kill only when calling __NR_getpid. */
struct sock_filter filter_thread[] = {
BPF_STMT(BPF_LD|BPF_W|BPF_ABS,
offsetof(struct seccomp_data, nr)),
- BPF_JUMP(BPF_JMP|BPF_JEQ|BPF_K, __NR_prctl, 0, 1),
+ BPF_JUMP(BPF_JMP|BPF_JEQ|BPF_K, __NR_getpid, 0, 1),
BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_KILL_THREAD),
BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_ALLOW),
};
@@ -819,7 +819,7 @@ void kill_thread_or_group(struct __test_metadata *_metadata,
struct sock_filter filter_process[] = {
BPF_STMT(BPF_LD|BPF_W|BPF_ABS,
offsetof(struct seccomp_data, nr)),
- BPF_JUMP(BPF_JMP|BPF_JEQ|BPF_K, __NR_prctl, 0, 1),
+ BPF_JUMP(BPF_JMP|BPF_JEQ|BPF_K, __NR_getpid, 0, 1),
BPF_STMT(BPF_RET|BPF_K, kill),
BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_ALLOW),
};
--
2.43.0.429.g432eaa2c6b-goog
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 3/3] selftests/seccomp: user_notification_addfd check nextfd is available
2024-01-24 14:13 [PATCH 0/3] selftests/seccomp seccomp_bpf test fixes Terry Tritton
2024-01-24 14:13 ` [PATCH 1/3] selftests/seccomp: Handle EINVAL on unshare(CLONE_NEWPID) Terry Tritton
2024-01-24 14:13 ` [PATCH 2/3] selftests/seccomp: Change the syscall used in KILL_THREAD test Terry Tritton
@ 2024-01-24 14:13 ` Terry Tritton
2024-01-24 16:21 ` [PATCH 0/3] selftests/seccomp seccomp_bpf test fixes Kees Cook
3 siblings, 0 replies; 5+ messages in thread
From: Terry Tritton @ 2024-01-24 14:13 UTC (permalink / raw)
To: keescook, luto, wad, shuah
Cc: linux-kselftest, linux-kernel, peter.griffin, kernel-team,
bettyzhou, Terry Tritton
Currently the user_notification_addfd test checks what the next expected
file descriptor will be by incrementing a variable nextfd. This does not
account for file descriptors that may already be open before the test is
started and will cause the test to fail if any exist.
Replace nextfd++ with a function get_next_fd which will check and return
the next available file descriptor.
Signed-off-by: Terry Tritton <terry.tritton@linaro.org>
---
tools/testing/selftests/seccomp/seccomp_bpf.c | 24 +++++++++++++++----
1 file changed, 19 insertions(+), 5 deletions(-)
diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c
index da11b95b8872..cacf6507f690 100644
--- a/tools/testing/selftests/seccomp/seccomp_bpf.c
+++ b/tools/testing/selftests/seccomp/seccomp_bpf.c
@@ -4044,6 +4044,16 @@ TEST(user_notification_filter_empty_threaded)
EXPECT_GT((pollfd.revents & POLLHUP) ?: 0, 0);
}
+
+int get_next_fd(int prev_fd)
+{
+ for (int i = prev_fd + 1; i < FD_SETSIZE; ++i) {
+ if (fcntl(i, F_GETFD) == -1)
+ return i;
+ }
+ _exit(EXIT_FAILURE);
+}
+
TEST(user_notification_addfd)
{
pid_t pid;
@@ -4060,7 +4070,7 @@ TEST(user_notification_addfd)
/* There may be arbitrary already-open fds at test start. */
memfd = memfd_create("test", 0);
ASSERT_GE(memfd, 0);
- nextfd = memfd + 1;
+ nextfd = get_next_fd(memfd);
ret = prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0);
ASSERT_EQ(0, ret) {
@@ -4071,7 +4081,8 @@ TEST(user_notification_addfd)
/* Check that the basic notification machinery works */
listener = user_notif_syscall(__NR_getppid,
SECCOMP_FILTER_FLAG_NEW_LISTENER);
- ASSERT_EQ(listener, nextfd++);
+ ASSERT_EQ(listener, nextfd);
+ nextfd = get_next_fd(nextfd);
pid = fork();
ASSERT_GE(pid, 0);
@@ -4126,14 +4137,16 @@ TEST(user_notification_addfd)
/* Verify we can set an arbitrary remote fd */
fd = ioctl(listener, SECCOMP_IOCTL_NOTIF_ADDFD, &addfd);
- EXPECT_EQ(fd, nextfd++);
+ EXPECT_EQ(fd, nextfd);
+ nextfd = get_next_fd(nextfd);
EXPECT_EQ(filecmp(getpid(), pid, memfd, fd), 0);
/* Verify we can set an arbitrary remote fd with large size */
memset(&big, 0x0, sizeof(big));
big.addfd = addfd;
fd = ioctl(listener, SECCOMP_IOCTL_NOTIF_ADDFD_BIG, &big);
- EXPECT_EQ(fd, nextfd++);
+ EXPECT_EQ(fd, nextfd);
+ nextfd = get_next_fd(nextfd);
/* Verify we can set a specific remote fd */
addfd.newfd = 42;
@@ -4171,7 +4184,8 @@ TEST(user_notification_addfd)
* Child has earlier "low" fds and now 42, so we expect the next
* lowest available fd to be assigned here.
*/
- EXPECT_EQ(fd, nextfd++);
+ EXPECT_EQ(fd, nextfd);
+ nextfd = get_next_fd(nextfd);
ASSERT_EQ(filecmp(getpid(), pid, memfd, fd), 0);
/*
--
2.43.0.429.g432eaa2c6b-goog
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 0/3] selftests/seccomp seccomp_bpf test fixes
2024-01-24 14:13 [PATCH 0/3] selftests/seccomp seccomp_bpf test fixes Terry Tritton
` (2 preceding siblings ...)
2024-01-24 14:13 ` [PATCH 3/3] selftests/seccomp: user_notification_addfd check nextfd is available Terry Tritton
@ 2024-01-24 16:21 ` Kees Cook
3 siblings, 0 replies; 5+ messages in thread
From: Kees Cook @ 2024-01-24 16:21 UTC (permalink / raw)
To: luto, wad, shuah, Terry Tritton
Cc: Kees Cook, linux-kselftest, linux-kernel, peter.griffin,
kernel-team, bettyzhou
On Wed, 24 Jan 2024 14:13:54 +0000, Terry Tritton wrote:
> Here are a few fixes for seccomp_bpf tests found when testing on
> Android:
>
> user_notification_sibling_pid_ns:
> unshare(CLONE_NEWPID) can return EINVAL so have added a check for this.
>
> KILL_THREAD:
> This one is a bit more Android specific.
> In Bionic pthread_create is calling prctl, this is causing the test to
> fail as prctl is in the filter for this test and is killed when it is
> called. I've just changed prctl to getpid in this case.
>
> [...]
Thanks for tracking all of these down. These look good to me.
Applied to for-next/seccomp, thanks!
[1/3] selftests/seccomp: Handle EINVAL on unshare(CLONE_NEWPID)
https://git.kernel.org/kees/c/18975ce05799
[2/3] selftests/seccomp: Change the syscall used in KILL_THREAD test
https://git.kernel.org/kees/c/fbcdf41167fe
[3/3] selftests/seccomp: user_notification_addfd check nextfd is available
https://git.kernel.org/kees/c/0c6f28a84431
Take care,
--
Kees Cook
^ permalink raw reply [flat|nested] 5+ messages in thread