[PATCH 0/3] some fixups for PTRACE_SECCOMP_GET_METADATA

[PATCH 0/3] some fixups for PTRACE_SECCOMP_GET_METADATA

[Tycho Andersen]

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

 include/uapi/linux/ptrace.h                   |  4 +-
 kernel/seccomp.c                              |  6 ++-
 tools/testing/selftests/seccomp/seccomp_bpf.c | 61 +++++++++++++++++++++++++++
 3 files changed, 67 insertions(+), 4 deletions(-)

-- 
2.14.1

[PATCH 1/3] seccomp, ptrace: switch get_metadata types to arch independent

[Tycho Andersen]

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

[PATCH 2/3] ptrace, seccomp: tweak get_metadata behavior slightly

[Tycho Andersen]

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

[PATCH 3/3] seccomp: add a selftest for get_metadata

[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

Re: [PATCH 1/3] seccomp, ptrace: switch get_metadata types to arch independent

[Dmitry V. Levin]

[-- 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 --]

Re: [PATCH 0/3] some fixups for PTRACE_SECCOMP_GET_METADATA

[Kees Cook]

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