Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Sun, 17 May 2020 00:17:14 -0700
From:   Kees Cook <keescook@chromium.org>
To:     Sargun Dhillon <sargun@sargun.me>
Cc:     linux-kernel@vger.kernel.org,
        containers@lists.linux-foundation.org, linux-api@vger.kernel.org,
        christian.brauner@ubuntu.com, tycho@tycho.ws, cyphar@cyphar.com
Subject: Re: [PATCH] seccomp: Add group_leader pid to seccomp_notif
Message-ID: <202005162344.74A02C2D@keescook>
References: <20200515234005.32370-1-sargun@sargun.me>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200515234005.32370-1-sargun@sargun.me>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, May 15, 2020 at 04:40:05PM -0700, Sargun Dhillon wrote:
> This includes the thread group leader ID in the seccomp_notif. This is
> immediately useful for opening up a pidfd for the group leader, as
> pidfds only work on group leaders.
> 
> Previously, it was considered to include an actual pidfd in the
> seccomp_notif structure[1], but it was suggested to avoid proliferating
> mechanisms to create pidfds[2].
> 
> [1]: https://lkml.org/lkml/2020/1/24/133
> [2]: https://lkml.org/lkml/2020/5/15/481

nit: please use lore.kernel.org/lkml/ URLs

> Suggested-by: Christian Brauner <christian.brauner@ubuntu.com>
> Signed-off-by: Sargun Dhillon <sargun@sargun.me>
> ---
>  include/uapi/linux/seccomp.h                  |  2 +
>  kernel/seccomp.c                              |  1 +
>  tools/testing/selftests/seccomp/seccomp_bpf.c | 50 +++++++++++++++++++
>  3 files changed, 53 insertions(+)
> 
> diff --git a/include/uapi/linux/seccomp.h b/include/uapi/linux/seccomp.h
> index c1735455bc53..f0c272ef0f1e 100644
> --- a/include/uapi/linux/seccomp.h
> +++ b/include/uapi/linux/seccomp.h
> @@ -75,6 +75,8 @@ struct seccomp_notif {
>  	__u32 pid;
>  	__u32 flags;
>  	struct seccomp_data data;
> +	__u32 tgid;
> +	__u8 pad0[4];
>  };

I think we need to leave off padding and instead use __packed. If we
don't then userspace can't tell when "pad0" changes its "meaning" (i.e.
the size of seccomp_notif becomes 88 bytes with above -- either via
explicit padding like you've got or via implicit by the compiler. If
some other u32 gets added in the future, user space will still see "88"
as the size.

So I *think* the right change here is:

-};
+	__u32 tgid;
+} __packed;

Though tgid may need to go above seccomp_data... for when it grows.
Agh...

_However_, unfortunately, I appear to have no thought this through very
well, and there is actually no sanity-checking in the kernel for dealing
with an old userspace when sizes change. :( For example, if a userspace
doesn't check sizes and calls an ioctl, etc, the kernel will clobber the
user buffer if it's too small.

Even the SECCOMP_GET_NOTIF_SIZES command lacks a buffer size argument.
:(

So:

- should we just declare such userspace as "wrong"? I don't think
  that'll work, especially since what if we ever change the size of
  seccomp_data...  that predated the ..._SIZES command.

- should we add a SECCOMP_SET_SIZES command to tell the kernel what
  we're expecting? There's no "state" associated across seccomp(2)
  calls, but maybe that doesn't matter because only user_notif writes
  back to userspace. For the ioctl, the state could be part of the
  private file data? Sending seccomp_data back to userspace only
  happens here, and any changes in seccomp_data size will just be seen
  as allowing a filter to query further into it.

- should GET_SIZES report "useful" size? (i.e. exclude padding?)

> diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c

Yay test updates! :)

> +TEST(user_notification_groupleader)

In my first pass of review I was going to say "can you please also check
the sizes used by the ioctl?" But that triggered the above size checking
mess in my mind.

Let me look at this more closely on Monday, and I'll proposed something.
:P

Thanks!

-Kees

-- 
Kees Cook