* [PATCH 1/3] seccomp, ptrace: switch get_metadata types to arch independent
2018-02-21 2:47 [PATCH 0/3] some fixups for PTRACE_SECCOMP_GET_METADATA Tycho Andersen
@ 2018-02-21 2:47 ` Tycho Andersen
2018-02-21 3:17 ` Dmitry V. Levin
2018-02-21 2:47 ` [PATCH 2/3] ptrace, seccomp: tweak get_metadata behavior slightly Tycho Andersen
` (2 subsequent siblings)
3 siblings, 1 reply; 6+ messages in thread
From: Tycho Andersen @ 2018-02-21 2:47 UTC (permalink / raw)
To: linux-kernel, Kees Cook; +Cc: Tycho Andersen, Oleg Nesterov
Commit 26500475ac1b ("ptrace, seccomp: add support for retrieving seccomp
metadata") introduced `struct seccomp_metadata`, which contained unsigned
longs that should be arch independent. The type of the flags member was
chosen to match the corresponding argument to seccomp(), and so we need
something at least as big as unsigned long. My understanding is that __u64
should fit the bill, so let's switch both types to that.
While this is userspace facing, it was only introduced in 4.16-rc2, and so
should be safe assuming it goes in before then.
Reported-by: "Dmitry V. Levin" <ldv@altlinux.org>
Signed-off-by: Tycho Andersen <tycho@tycho.ws>
CC: Kees Cook <keescook@chromium.org>
CC: Oleg Nesterov <oleg@redhat.com>
---
include/uapi/linux/ptrace.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/uapi/linux/ptrace.h b/include/uapi/linux/ptrace.h
index e46d82b91166..d5a1b8a492b9 100644
--- a/include/uapi/linux/ptrace.h
+++ b/include/uapi/linux/ptrace.h
@@ -69,8 +69,8 @@ struct ptrace_peeksiginfo_args {
#define PTRACE_SECCOMP_GET_METADATA 0x420d
struct seccomp_metadata {
- unsigned long filter_off; /* Input: which filter */
- unsigned int flags; /* Output: filter's flags */
+ __u64 filter_off; /* Input: which filter */
+ __u64 flags; /* Output: filter's flags */
};
/* Read signals from a shared (process wide) queue */
--
2.14.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 1/3] seccomp, ptrace: switch get_metadata types to arch independent
2018-02-21 2:47 ` [PATCH 1/3] seccomp, ptrace: switch get_metadata types to arch independent Tycho Andersen
@ 2018-02-21 3:17 ` Dmitry V. Levin
0 siblings, 0 replies; 6+ messages in thread
From: Dmitry V. Levin @ 2018-02-21 3:17 UTC (permalink / raw)
To: Tycho Andersen; +Cc: Kees Cook, Oleg Nesterov, linux-api, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1635 bytes --]
On Tue, Feb 20, 2018 at 07:47:45PM -0700, Tycho Andersen wrote:
> Commit 26500475ac1b ("ptrace, seccomp: add support for retrieving seccomp
> metadata") introduced `struct seccomp_metadata`, which contained unsigned
> longs that should be arch independent. The type of the flags member was
> chosen to match the corresponding argument to seccomp(), and so we need
> something at least as big as unsigned long. My understanding is that __u64
> should fit the bill, so let's switch both types to that.
>
> While this is userspace facing, it was only introduced in 4.16-rc2, and so
> should be safe assuming it goes in before then.
>
> Reported-by: "Dmitry V. Levin" <ldv@altlinux.org>
> Signed-off-by: Tycho Andersen <tycho@tycho.ws>
> CC: Kees Cook <keescook@chromium.org>
> CC: Oleg Nesterov <oleg@redhat.com>
> ---
> include/uapi/linux/ptrace.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/include/uapi/linux/ptrace.h b/include/uapi/linux/ptrace.h
> index e46d82b91166..d5a1b8a492b9 100644
> --- a/include/uapi/linux/ptrace.h
> +++ b/include/uapi/linux/ptrace.h
> @@ -69,8 +69,8 @@ struct ptrace_peeksiginfo_args {
> #define PTRACE_SECCOMP_GET_METADATA 0x420d
>
> struct seccomp_metadata {
> - unsigned long filter_off; /* Input: which filter */
> - unsigned int flags; /* Output: filter's flags */
> + __u64 filter_off; /* Input: which filter */
> + __u64 flags; /* Output: filter's flags */
> };
>
> /* Read signals from a shared (process wide) queue */
That's much better, thanks.
Reviewed-by: "Dmitry V. Levin" <ldv@altlinux.org>
--
ldv
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/3] ptrace, seccomp: tweak get_metadata behavior slightly
2018-02-21 2:47 [PATCH 0/3] some fixups for PTRACE_SECCOMP_GET_METADATA Tycho Andersen
2018-02-21 2:47 ` [PATCH 1/3] seccomp, ptrace: switch get_metadata types to arch independent Tycho Andersen
@ 2018-02-21 2:47 ` Tycho Andersen
2018-02-21 2:47 ` [PATCH 3/3] seccomp: add a selftest for get_metadata Tycho Andersen
2018-02-22 1:05 ` [PATCH 0/3] some fixups for PTRACE_SECCOMP_GET_METADATA Kees Cook
3 siblings, 0 replies; 6+ messages in thread
From: Tycho Andersen @ 2018-02-21 2:47 UTC (permalink / raw)
To: linux-kernel, Kees Cook; +Cc: Tycho Andersen, Oleg Nesterov
Previously if users passed a small size for the input structure size, they
would get get odd behavior. It doesn't make sense to pass a structure
smaller than at least filter_off size, so let's just give -EINVAL in this
case.
This changes userspace visible behavior, but was only introduced in commit
26500475ac1b ("ptrace, seccomp: add support for retrieving seccomp
metadata") in 4.16-rc2, so should be safe to change if merged before then.
Reported-by: Eugene Syromiatnikov <esyr@redhat.com>
Signed-off-by: Tycho Andersen <tycho@tycho.ws>
CC: Kees Cook <keescook@chromium.org>
CC: Oleg Nesterov <oleg@redhat.com>
---
kernel/seccomp.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index 940fa408a288..dc77548167ef 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -1076,14 +1076,16 @@ long seccomp_get_metadata(struct task_struct *task,
size = min_t(unsigned long, size, sizeof(kmd));
- if (copy_from_user(&kmd, data, size))
+ if (size < sizeof(kmd.filter_off))
+ return -EINVAL;
+
+ if (copy_from_user(&kmd.filter_off, data, sizeof(kmd.filter_off)))
return -EFAULT;
filter = get_nth_filter(task, kmd.filter_off);
if (IS_ERR(filter))
return PTR_ERR(filter);
- memset(&kmd, 0, sizeof(kmd));
if (filter->log)
kmd.flags |= SECCOMP_FILTER_FLAG_LOG;
--
2.14.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 3/3] seccomp: add a selftest for get_metadata
2018-02-21 2:47 [PATCH 0/3] some fixups for PTRACE_SECCOMP_GET_METADATA Tycho Andersen
2018-02-21 2:47 ` [PATCH 1/3] seccomp, ptrace: switch get_metadata types to arch independent Tycho Andersen
2018-02-21 2:47 ` [PATCH 2/3] ptrace, seccomp: tweak get_metadata behavior slightly Tycho Andersen
@ 2018-02-21 2:47 ` Tycho Andersen
2018-02-22 1:05 ` [PATCH 0/3] some fixups for PTRACE_SECCOMP_GET_METADATA Kees Cook
3 siblings, 0 replies; 6+ messages in thread
From: Tycho Andersen @ 2018-02-21 2:47 UTC (permalink / raw)
To: linux-kernel, Kees Cook; +Cc: Tycho Andersen
Let's test that we get the flags correctly, and that we preserve the filter
index across the ptrace(PTRACE_SECCOMP_GET_METADATA) correctly.
Signed-off-by: Tycho Andersen <tycho@tycho.ws>
CC: Kees Cook <keescook@chromium.org>
---
tools/testing/selftests/seccomp/seccomp_bpf.c | 61 +++++++++++++++++++++++++++
1 file changed, 61 insertions(+)
diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c
index 0b457e8e0f0c..5df609950a66 100644
--- a/tools/testing/selftests/seccomp/seccomp_bpf.c
+++ b/tools/testing/selftests/seccomp/seccomp_bpf.c
@@ -141,6 +141,15 @@ struct seccomp_data {
#define SECCOMP_FILTER_FLAG_LOG 2
#endif
+#ifndef PTRACE_SECCOMP_GET_METADATA
+#define PTRACE_SECCOMP_GET_METADATA 0x420d
+
+struct seccomp_metadata {
+ __u64 filter_off; /* Input: which filter */
+ __u64 flags; /* Output: filter's flags */
+};
+#endif
+
#ifndef seccomp
int seccomp(unsigned int op, unsigned int flags, void *args)
{
@@ -2845,6 +2854,58 @@ TEST(get_action_avail)
EXPECT_EQ(errno, EOPNOTSUPP);
}
+TEST(get_metadata)
+{
+ pid_t pid;
+ int pipefd[2];
+ char buf;
+ struct seccomp_metadata md;
+
+ ASSERT_EQ(0, pipe(pipefd));
+
+ pid = fork();
+ ASSERT_GE(pid, 0);
+ if (pid == 0) {
+ struct sock_filter filter[] = {
+ BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_ALLOW),
+ };
+ struct sock_fprog prog = {
+ .len = (unsigned short)ARRAY_SIZE(filter),
+ .filter = filter,
+ };
+
+ /* one with log, one without */
+ ASSERT_EQ(0, seccomp(SECCOMP_SET_MODE_FILTER,
+ SECCOMP_FILTER_FLAG_LOG, &prog));
+ ASSERT_EQ(0, seccomp(SECCOMP_SET_MODE_FILTER, 0, &prog));
+
+ ASSERT_EQ(0, close(pipefd[0]));
+ ASSERT_EQ(1, write(pipefd[1], "1", 1));
+ ASSERT_EQ(0, close(pipefd[1]));
+
+ while (1)
+ sleep(100);
+ }
+
+ ASSERT_EQ(0, close(pipefd[1]));
+ ASSERT_EQ(1, read(pipefd[0], &buf, 1));
+
+ ASSERT_EQ(0, ptrace(PTRACE_ATTACH, pid));
+ ASSERT_EQ(pid, waitpid(pid, NULL, 0));
+
+ md.filter_off = 0;
+ ASSERT_EQ(sizeof(md), ptrace(PTRACE_SECCOMP_GET_METADATA, pid, sizeof(md), &md));
+ EXPECT_EQ(md.flags, SECCOMP_FILTER_FLAG_LOG);
+ EXPECT_EQ(md.filter_off, 0);
+
+ md.filter_off = 1;
+ ASSERT_EQ(sizeof(md), ptrace(PTRACE_SECCOMP_GET_METADATA, pid, sizeof(md), &md));
+ EXPECT_EQ(md.flags, 0);
+ EXPECT_EQ(md.filter_off, 1);
+
+ ASSERT_EQ(0, kill(pid, SIGKILL));
+}
+
/*
* TODO:
* - add microbenchmarks
--
2.14.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 0/3] some fixups for PTRACE_SECCOMP_GET_METADATA
2018-02-21 2:47 [PATCH 0/3] some fixups for PTRACE_SECCOMP_GET_METADATA Tycho Andersen
` (2 preceding siblings ...)
2018-02-21 2:47 ` [PATCH 3/3] seccomp: add a selftest for get_metadata Tycho Andersen
@ 2018-02-22 1:05 ` Kees Cook
3 siblings, 0 replies; 6+ messages in thread
From: Kees Cook @ 2018-02-22 1:05 UTC (permalink / raw)
To: Tycho Andersen; +Cc: LKML, Eugene Syromiatnikov, Dmitry V. Levin
On Tue, Feb 20, 2018 at 6:47 PM, Tycho Andersen <tycho@tycho.ws> wrote:
> Hi Kees,
>
> Here are a couple of tweaks/fixes people suggested to the get_metadata
> functionality, plus a test to ensure that things work the way they're supposed
> to and stay that way.
>
> Cheers,
>
> Tycho
>
> Tycho Andersen (3):
> seccomp, ptrace: switch get_metadata types to arch independent
> ptrace, seccomp: tweak get_metadata behavior slightly
> seccomp: add a selftest for get_metadata
Thanks! I've applied these and asked James to get them pull for 4.16.
-Kees
--
Kees Cook
Pixel Security
^ permalink raw reply [flat|nested] 6+ messages in thread