Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
In-Reply-To: <20181031002917.GA2180@cisco>
References: <20181029224031.29809-1-tycho@tycho.ws> <20181029224031.29809-2-tycho@tycho.ws>
 <CAGXu5jLjum=TDab67L5XdWKEmzU_5rYeLfGXB1vb2BmfZdPuHQ@mail.gmail.com>
 <20181030215404.GF7343@cisco> <CAGXu5j+ZPxu6egE1fEr+N9+zLx3N+SJ_vbS_zzj9_hrdWrrrWQ@mail.gmail.com>
 <20181030223228.GG7343@cisco> <CAGXu5j+XEYqVye7yZaYF1_-xpCQt7pA=YgsTpQJnYupE5MSa4Q@mail.gmail.com>
 <20181031002917.GA2180@cisco>
From:   Kees Cook <keescook@chromium.org>
Date:   Tue, 30 Oct 2018 18:29:01 -0700
Message-ID: <CAGXu5jJubF30FsPVJeEZ18XKN-6BJMEbgF6+8_Bs6MT1fOyv8Q@mail.gmail.com>
Subject: Re: [PATCH v8 1/2] seccomp: add a return code to trap to userspace
To:     Tycho Andersen <tycho@tycho.ws>
Cc:     Andy Lutomirski <luto@amacapital.net>,
        Oleg Nesterov <oleg@redhat.com>,
        "Eric W . Biederman" <ebiederm@xmission.com>,
        "Serge E . Hallyn" <serge@hallyn.com>,
        Christian Brauner <christian@brauner.io>,
        Tyler Hicks <tyhicks@canonical.com>,
        Akihiro Suda <suda.akihiro@lab.ntt.co.jp>,
        Aleksa Sarai <asarai@suse.de>,
        LKML <linux-kernel@vger.kernel.org>,
        Linux Containers <containers@lists.linux-foundation.org>,
        Linux API <linux-api@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, Oct 30, 2018 at 5:29 PM, Tycho Andersen <tycho@tycho.ws> wrote:
> On Tue, Oct 30, 2018 at 03:34:54PM -0700, Kees Cook wrote:
>> On Tue, Oct 30, 2018 at 3:32 PM, Tycho Andersen <tycho@tycho.ws> wrote:
>> > On Tue, Oct 30, 2018 at 03:00:17PM -0700, Kees Cook wrote:
>> >> On Tue, Oct 30, 2018 at 2:54 PM, Tycho Andersen <tycho@tycho.ws> wrot=
e:
>> >> > On Tue, Oct 30, 2018 at 02:49:21PM -0700, Kees Cook wrote:
>> >> >> On Mon, Oct 29, 2018 at 3:40 PM, Tycho Andersen <tycho@tycho.ws> w=
rote:
>> >> >> >     * switch to a flags based future-proofing mechanism for stru=
ct
>> >> >> >       seccomp_notif and seccomp_notif_resp, thus avoiding versio=
n issues
>> >> >> >       with structure length (Kees)
>> >> >> [...]
>> >> >> >
>> >> >> > +struct seccomp_notif {
>> >> >> > +       __u64 id;
>> >> >> > +       __u32 pid;
>> >> >> > +       __u32 flags;
>> >> >> > +       struct seccomp_data data;
>> >> >> > +};
>> >> >> > +
>> >> >> > +struct seccomp_notif_resp {
>> >> >> > +       __u64 id;
>> >> >> > +       __s64 val;
>> >> >> > +       __s32 error;
>> >> >> > +       __u32 flags;
>> >> >> > +};
>> >> >>
>> >> >> Hrm, so, what's the plan for when struct seccomp_data changes size=
?
>> >> >
>> >> > I guess my plan was don't ever change the size again, just use flag=
s
>> >> > and have extra state available via ioctl().
>> >> >
>> >> >> I'm realizing that it might be "too late" for userspace to discove=
r
>> >> >> it's running on a newer kernel. i.e. it gets a user notification, =
and
>> >> >> discovers flags it doesn't know how to handle. Do we actually need
>> >> >> both flags AND a length? Designing UAPI is frustrating! :)
>> >> >
>> >> > :). I don't see this as such a big problem -- in fact it's better t=
han
>> >> > the length mode, where you don't know what you don't know, because =
it
>> >> > only copied as much info as you could handle. Older userspace would
>> >> > simply not use information it didn't know how to use.
>> >> >
>> >> >> Do we need another ioctl to discover the seccomp_data size maybe?
>> >> >
>> >> > That could be an option as well, assuming we agree that size would
>> >> > work, which I thought we didn't?
>> >>
>> >> Size alone wasn't able to determine the layout of the seccomp_notif
>> >> structure since it had holes (in the prior version). seccomp_data
>> >> doesn't have holes and is likely to change in size (see the recent
>> >> thread on adding the MPK register to it...)
>> >
>> > Oh, sorry, I misread this as seccomp_notif, not seccomp_data.
>> >
>> >> I'm trying to imagine the right API for this. A portable user of
>> >> seccomp_notif expects the id/pid/flags/data to always be in the same
>> >> place, but it's the size of seccomp_data that may change. So it wants
>> >> to allocate space for seccomp_notif header and "everything else", of
>> >> which is may only understand the start of seccomp_data (and ignore an=
y
>> >> new trailing fields).
>> >>
>> >> So... perhaps the "how big are things?" ioctl would report the header
>> >> size and the seccomp_data size. Then both are flexible. And flags
>> >> would be left as a way to "version" the header?
>> >>
>> >> Any Linux API list members want to chime in here?
>> >
>> > So:
>> >
>> > struct seccomp_notify_sizes {
>> >     u16 seccomp_notify;
>> >     u16 seccomp_data;
>> > };
>> >
>> > ioctl(fd, SECCOMP_IOCTL_GET_SIZE, &sizes);
>> >
>> > This would be only one extra syscall over the lifetime of the listener
>> > process, which doesn't seem too bad. One thing that's slightly
>> > annoying is that you can't do it until you actually get an event, so
>> > maybe it could be a command on the seccomp syscall instead:
>> >
>> > seccomp(SECCOMP_GET_NOTIF_SIZES, 0, &sizes);
>>
>> Yeah, top-level makes more sense. u16 seems fine too.
>
> So one problem is this is that the third argument of the seccomp
> syscall is declared as const char, so I get:
>
> kernel/seccomp.c: In function =E2=80=98seccomp_get_notif_sizes=E2=80=99:
> kernel/seccomp.c:1401:19: warning: passing argument 1 of =E2=80=98copy_to=
_user=E2=80=99 discards =E2=80=98const=E2=80=99 qualifier from pointer targ=
et type [-Wdiscarded-qualifiers]
>   if (copy_to_user(usizes, &sizes, sizeof(sizes)))
>                    ^~~~~~
> In file included from ./include/linux/compat.h:19:0,
>                  from kernel/seccomp.c:19:
> ./include/linux/uaccess.h:152:1: note: expected =E2=80=98void *=E2=80=99 =
but argument is of type =E2=80=98const char *=E2=80=99
>  copy_to_user(void __user *to, const void *from, unsigned long n)
>  ^~~~~~~~~~~~
>
> If I drop the const it doesn't complain, but I'm not sure what the protoc=
ol is
> for changing the types of syscall declarations. In principle it doesn't r=
eally
> mean anything, but...

I think this should be fine. It's documented as "void *"...

--=20
Kees Cook