[PATCH] seccomp: Check flags on seccomp_notif is unset

[PATCH] seccomp: Check flags on seccomp_notif is unset

[Sargun Dhillon]

This patch is a small change in enforcement of the uapi for
SECCOMP_IOCTL_NOTIF_RECV ioctl. Specificaly, the datastructure which is
passed (seccomp_notif), has a flags member. Previously that could be
set to a nonsense value, and we would ignore it. This ensures that
no flags are set.

Signed-off-by: Sargun Dhillon <sargun@sargun.me>
Cc: Kees Cook <keescook@chromium.org>
---
 kernel/seccomp.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index 12d2227e5786..455925557490 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -1026,6 +1026,13 @@ static long seccomp_notify_recv(struct seccomp_filter *filter,
 	struct seccomp_notif unotif;
 	ssize_t ret;
 
+	if (copy_from_user(&unotif, buf, sizeof(unotif)))
+		return -EFAULT;
+
+	/* flags is reserved right now, make sure it's unset */
+	if (unotif.flags)
+		return -EINVAL;
+
 	memset(&unotif, 0, sizeof(unotif));
 
 	ret = down_interruptible(&filter->notif->request);
-- 
2.20.1

Re: [PATCH] seccomp: Check flags on seccomp_notif is unset

[Christian Brauner]

On Wed, Dec 25, 2019 at 09:45:33PM +0000, Sargun Dhillon wrote:
> This patch is a small change in enforcement of the uapi for
> SECCOMP_IOCTL_NOTIF_RECV ioctl. Specificaly, the datastructure which is
> passed (seccomp_notif), has a flags member. Previously that could be
> set to a nonsense value, and we would ignore it. This ensures that
> no flags are set.
> 
> Signed-off-by: Sargun Dhillon <sargun@sargun.me>
> Cc: Kees Cook <keescook@chromium.org>

I'm fine with this since we soon want to make use of the flag argument
when we add a flag to get a pidfd from the seccomp notifier on receive.
The major users I could identify already pass in seccomp_notif with all
fields set to 0. If we really break users we can always revert; this
seems very unlikely to me though.

One more question below, otherwise:

Reviewed-by: Christian Brauner <christian.brauner@ubuntu.com>

> ---
>  kernel/seccomp.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> index 12d2227e5786..455925557490 100644
> --- a/kernel/seccomp.c
> +++ b/kernel/seccomp.c
> @@ -1026,6 +1026,13 @@ static long seccomp_notify_recv(struct seccomp_filter *filter,
>  	struct seccomp_notif unotif;
>  	ssize_t ret;
>  
> +	if (copy_from_user(&unotif, buf, sizeof(unotif)))
> +		return -EFAULT;
> +
> +	/* flags is reserved right now, make sure it's unset */
> +	if (unotif.flags)
> +		return -EINVAL;
> +

Might it make sense to use

	err = copy_struct_from_user(&unotif, sizeof(unotif), buf, sizeof(unotif));
	if (err)
		return err;

This way we check that the whole struct is 0 and report an error as soon
as one of the members is non-zero. That's more drastic but it'd ensure
that other fields can be used in the future for whatever purposes.
It would also let us get rid of the memset() below. 

>  	memset(&unotif, 0, sizeof(unotif));
>  
>  	ret = down_interruptible(&filter->notif->request);
> -- 
> 2.20.1
> 

Re: [PATCH] seccomp: Check flags on seccomp_notif is unset

[Aleksa Sarai]

[-- Attachment #1: Type: text/plain, Size: 2562 bytes --]

On 2019-12-26, Christian Brauner <christian.brauner@ubuntu.com> wrote:
> On Wed, Dec 25, 2019 at 09:45:33PM +0000, Sargun Dhillon wrote:
> > This patch is a small change in enforcement of the uapi for
> > SECCOMP_IOCTL_NOTIF_RECV ioctl. Specificaly, the datastructure which is
> > passed (seccomp_notif), has a flags member. Previously that could be
> > set to a nonsense value, and we would ignore it. This ensures that
> > no flags are set.
> > 
> > Signed-off-by: Sargun Dhillon <sargun@sargun.me>
> > Cc: Kees Cook <keescook@chromium.org>
> 
> I'm fine with this since we soon want to make use of the flag argument
> when we add a flag to get a pidfd from the seccomp notifier on receive.
> The major users I could identify already pass in seccomp_notif with all
> fields set to 0. If we really break users we can always revert; this
> seems very unlikely to me though.
> 
> One more question below, otherwise:
> 
> Reviewed-by: Christian Brauner <christian.brauner@ubuntu.com>
> 
> > ---
> >  kernel/seccomp.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> > index 12d2227e5786..455925557490 100644
> > --- a/kernel/seccomp.c
> > +++ b/kernel/seccomp.c
> > @@ -1026,6 +1026,13 @@ static long seccomp_notify_recv(struct seccomp_filter *filter,
> >  	struct seccomp_notif unotif;
> >  	ssize_t ret;
> >  
> > +	if (copy_from_user(&unotif, buf, sizeof(unotif)))
> > +		return -EFAULT;
> > +
> > +	/* flags is reserved right now, make sure it's unset */
> > +	if (unotif.flags)
> > +		return -EINVAL;
> > +
> 
> Might it make sense to use
> 
> 	err = copy_struct_from_user(&unotif, sizeof(unotif), buf, sizeof(unotif));
> 	if (err)
> 		return err;
> 
> This way we check that the whole struct is 0 and report an error as soon
> as one of the members is non-zero. That's more drastic but it'd ensure
> that other fields can be used in the future for whatever purposes.
> It would also let us get rid of the memset() below. 

Given that this isn't an extensible struct, it would be simpler to just do
check_zeroed_user() -- copy_struct_from_user() is overkill. That would
also remove the need for any copy_from_user()s and the memset can be
dropped by just doing

  struct seccomp_notif unotif = {};

> >  	memset(&unotif, 0, sizeof(unotif));
> >  
> >  	ret = down_interruptible(&filter->notif->request);
> > -- 
> > 2.20.1
> > 


-- 
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH] seccomp: Check flags on seccomp_notif is unset

[Christian Brauner]

On December 26, 2019 3:32:29 PM GMT+01:00, Aleksa Sarai <cyphar@cyphar.com> wrote:
>On 2019-12-26, Christian Brauner <christian.brauner@ubuntu.com> wrote:
>> On Wed, Dec 25, 2019 at 09:45:33PM +0000, Sargun Dhillon wrote:
>> > This patch is a small change in enforcement of the uapi for
>> > SECCOMP_IOCTL_NOTIF_RECV ioctl. Specificaly, the datastructure
>which is
>> > passed (seccomp_notif), has a flags member. Previously that could
>be
>> > set to a nonsense value, and we would ignore it. This ensures that
>> > no flags are set.
>> > 
>> > Signed-off-by: Sargun Dhillon <sargun@sargun.me>
>> > Cc: Kees Cook <keescook@chromium.org>
>> 
>> I'm fine with this since we soon want to make use of the flag
>argument
>> when we add a flag to get a pidfd from the seccomp notifier on
>receive.
>> The major users I could identify already pass in seccomp_notif with
>all
>> fields set to 0. If we really break users we can always revert; this
>> seems very unlikely to me though.
>> 
>> One more question below, otherwise:
>> 
>> Reviewed-by: Christian Brauner <christian.brauner@ubuntu.com>
>> 
>> > ---
>> >  kernel/seccomp.c | 7 +++++++
>> >  1 file changed, 7 insertions(+)
>> > 
>> > diff --git a/kernel/seccomp.c b/kernel/seccomp.c
>> > index 12d2227e5786..455925557490 100644
>> > --- a/kernel/seccomp.c
>> > +++ b/kernel/seccomp.c
>> > @@ -1026,6 +1026,13 @@ static long seccomp_notify_recv(struct
>seccomp_filter *filter,
>> >  	struct seccomp_notif unotif;
>> >  	ssize_t ret;
>> >  
>> > +	if (copy_from_user(&unotif, buf, sizeof(unotif)))
>> > +		return -EFAULT;
>> > +
>> > +	/* flags is reserved right now, make sure it's unset */
>> > +	if (unotif.flags)
>> > +		return -EINVAL;
>> > +
>> 
>> Might it make sense to use
>> 
>> 	err = copy_struct_from_user(&unotif, sizeof(unotif), buf,
>sizeof(unotif));
>> 	if (err)
>> 		return err;
>> 
>> This way we check that the whole struct is 0 and report an error as
>soon
>> as one of the members is non-zero. That's more drastic but it'd
>ensure
>> that other fields can be used in the future for whatever purposes.
>> It would also let us get rid of the memset() below. 
>
>Given that this isn't an extensible struct, it would be simpler to just
>do
>check_zeroed_user() -- copy_struct_from_user() is overkill. That would
>also remove the need for any copy_from_user()s and the memset can be
>dropped by just doing
>
>  struct seccomp_notif unotif = {};
>
>> >  	memset(&unotif, 0, sizeof(unotif));
>> >  
>> >  	ret = down_interruptible(&filter->notif->request);
>> > -- 
>> > 2.20.1
>> > 

It is an extensible struct. That's why we have notifier size checking built in.

Re: [PATCH] seccomp: Check flags on seccomp_notif is unset

[Tycho Andersen]

On Fri, Dec 27, 2019 at 01:32:29AM +1100, Aleksa Sarai wrote:
> On 2019-12-26, Christian Brauner <christian.brauner@ubuntu.com> wrote:
> > On Wed, Dec 25, 2019 at 09:45:33PM +0000, Sargun Dhillon wrote:
> > > This patch is a small change in enforcement of the uapi for
> > > SECCOMP_IOCTL_NOTIF_RECV ioctl. Specificaly, the datastructure which is
> > > passed (seccomp_notif), has a flags member. Previously that could be
> > > set to a nonsense value, and we would ignore it. This ensures that
> > > no flags are set.
> > > 
> > > Signed-off-by: Sargun Dhillon <sargun@sargun.me>
> > > Cc: Kees Cook <keescook@chromium.org>
> > 
> > I'm fine with this since we soon want to make use of the flag argument
> > when we add a flag to get a pidfd from the seccomp notifier on receive.
> > The major users I could identify already pass in seccomp_notif with all
> > fields set to 0. If we really break users we can always revert; this
> > seems very unlikely to me though.
> > 
> > One more question below, otherwise:
> > 
> > Reviewed-by: Christian Brauner <christian.brauner@ubuntu.com>
> > 
> > > ---
> > >  kernel/seccomp.c | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > > 
> > > diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> > > index 12d2227e5786..455925557490 100644
> > > --- a/kernel/seccomp.c
> > > +++ b/kernel/seccomp.c
> > > @@ -1026,6 +1026,13 @@ static long seccomp_notify_recv(struct seccomp_filter *filter,
> > >  	struct seccomp_notif unotif;
> > >  	ssize_t ret;
> > >  
> > > +	if (copy_from_user(&unotif, buf, sizeof(unotif)))
> > > +		return -EFAULT;
> > > +
> > > +	/* flags is reserved right now, make sure it's unset */
> > > +	if (unotif.flags)
> > > +		return -EINVAL;
> > > +
> > 
> > Might it make sense to use
> > 
> > 	err = copy_struct_from_user(&unotif, sizeof(unotif), buf, sizeof(unotif));
> > 	if (err)
> > 		return err;
> > 
> > This way we check that the whole struct is 0 and report an error as soon
> > as one of the members is non-zero. That's more drastic but it'd ensure
> > that other fields can be used in the future for whatever purposes.
> > It would also let us get rid of the memset() below. 
> 
> Given that this isn't an extensible struct, it would be simpler to just do
> check_zeroed_user() -- copy_struct_from_user() is overkill. That would
> also remove the need for any copy_from_user()s and the memset can be
> dropped by just doing
> 
>   struct seccomp_notif unotif = {};

This doesn't zero the padding according to the C standard, so no, you
can't drop the memset, or you may leak kernel stack bits.

As for the rest of it, while it is an ABI change I think all of the
users are CC'd on this thread, and it's an obvious goof on my part :).
So:

Acked-by: Tycho Andersen <tycho@tycho.ws>

Tycho

Re: [PATCH] seccomp: Check flags on seccomp_notif is unset

[Aleksa Sarai]

[-- Attachment #1: Type: text/plain, Size: 3564 bytes --]

On 2019-12-26, Christian Brauner <christian.brauner@ubuntu.com> wrote:
> On December 26, 2019 3:32:29 PM GMT+01:00, Aleksa Sarai <cyphar@cyphar.com> wrote:
> >On 2019-12-26, Christian Brauner <christian.brauner@ubuntu.com> wrote:
> >> On Wed, Dec 25, 2019 at 09:45:33PM +0000, Sargun Dhillon wrote:
> >> > This patch is a small change in enforcement of the uapi for
> >> > SECCOMP_IOCTL_NOTIF_RECV ioctl. Specificaly, the datastructure
> >which is
> >> > passed (seccomp_notif), has a flags member. Previously that could
> >be
> >> > set to a nonsense value, and we would ignore it. This ensures that
> >> > no flags are set.
> >> > 
> >> > Signed-off-by: Sargun Dhillon <sargun@sargun.me>
> >> > Cc: Kees Cook <keescook@chromium.org>
> >> 
> >> I'm fine with this since we soon want to make use of the flag
> >argument
> >> when we add a flag to get a pidfd from the seccomp notifier on
> >receive.
> >> The major users I could identify already pass in seccomp_notif with
> >all
> >> fields set to 0. If we really break users we can always revert; this
> >> seems very unlikely to me though.
> >> 
> >> One more question below, otherwise:
> >> 
> >> Reviewed-by: Christian Brauner <christian.brauner@ubuntu.com>
> >> 
> >> > ---
> >> >  kernel/seccomp.c | 7 +++++++
> >> >  1 file changed, 7 insertions(+)
> >> > 
> >> > diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> >> > index 12d2227e5786..455925557490 100644
> >> > --- a/kernel/seccomp.c
> >> > +++ b/kernel/seccomp.c
> >> > @@ -1026,6 +1026,13 @@ static long seccomp_notify_recv(struct
> >seccomp_filter *filter,
> >> >  	struct seccomp_notif unotif;
> >> >  	ssize_t ret;
> >> >  
> >> > +	if (copy_from_user(&unotif, buf, sizeof(unotif)))
> >> > +		return -EFAULT;
> >> > +
> >> > +	/* flags is reserved right now, make sure it's unset */
> >> > +	if (unotif.flags)
> >> > +		return -EINVAL;
> >> > +
> >> 
> >> Might it make sense to use
> >> 
> >> 	err = copy_struct_from_user(&unotif, sizeof(unotif), buf,
> >sizeof(unotif));
> >> 	if (err)
> >> 		return err;
> >> 
> >> This way we check that the whole struct is 0 and report an error as
> >soon
> >> as one of the members is non-zero. That's more drastic but it'd
> >ensure
> >> that other fields can be used in the future for whatever purposes.
> >> It would also let us get rid of the memset() below. 
> >
> >Given that this isn't an extensible struct, it would be simpler to just
> >do
> >check_zeroed_user() -- copy_struct_from_user() is overkill. That would
> >also remove the need for any copy_from_user()s and the memset can be
> >dropped by just doing
> >
> >  struct seccomp_notif unotif = {};
> >
> >> >  	memset(&unotif, 0, sizeof(unotif));
> >> >  
> >> >  	ret = down_interruptible(&filter->notif->request);
> >> > -- 
> >> > 2.20.1
> >> > 
> 
> It is an extensible struct. That's why we have notifier size checking built in.

Ah right, NOTIF_GET_SIZES. I reckon check_zeroed_user() is still a bit
simpler since none of the fields are used right now (and really, this
patch should be checking all of them, not just ->flags, if we want to
use any of them in the future).

But sure, copy_struct_from_user() also makes sense since it is
extensible (though I personally do find the whole NOTIF_GET_SIZES thing
a bit scary -- but that's water under the bridge at this point, and as
long as userspace is clever enough it shouldn't be a problem).

-- 
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH] seccomp: Check flags on seccomp_notif is unset

[Aleksa Sarai]

[-- Attachment #1: Type: text/plain, Size: 3248 bytes --]

On 2019-12-26, Tycho Andersen <tycho@tycho.ws> wrote:
> On Fri, Dec 27, 2019 at 01:32:29AM +1100, Aleksa Sarai wrote:
> > On 2019-12-26, Christian Brauner <christian.brauner@ubuntu.com> wrote:
> > > On Wed, Dec 25, 2019 at 09:45:33PM +0000, Sargun Dhillon wrote:
> > > > This patch is a small change in enforcement of the uapi for
> > > > SECCOMP_IOCTL_NOTIF_RECV ioctl. Specificaly, the datastructure which is
> > > > passed (seccomp_notif), has a flags member. Previously that could be
> > > > set to a nonsense value, and we would ignore it. This ensures that
> > > > no flags are set.
> > > > 
> > > > Signed-off-by: Sargun Dhillon <sargun@sargun.me>
> > > > Cc: Kees Cook <keescook@chromium.org>
> > > 
> > > I'm fine with this since we soon want to make use of the flag argument
> > > when we add a flag to get a pidfd from the seccomp notifier on receive.
> > > The major users I could identify already pass in seccomp_notif with all
> > > fields set to 0. If we really break users we can always revert; this
> > > seems very unlikely to me though.
> > > 
> > > One more question below, otherwise:
> > > 
> > > Reviewed-by: Christian Brauner <christian.brauner@ubuntu.com>
> > > 
> > > > ---
> > > >  kernel/seccomp.c | 7 +++++++
> > > >  1 file changed, 7 insertions(+)
> > > > 
> > > > diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> > > > index 12d2227e5786..455925557490 100644
> > > > --- a/kernel/seccomp.c
> > > > +++ b/kernel/seccomp.c
> > > > @@ -1026,6 +1026,13 @@ static long seccomp_notify_recv(struct seccomp_filter *filter,
> > > >  	struct seccomp_notif unotif;
> > > >  	ssize_t ret;
> > > >  
> > > > +	if (copy_from_user(&unotif, buf, sizeof(unotif)))
> > > > +		return -EFAULT;
> > > > +
> > > > +	/* flags is reserved right now, make sure it's unset */
> > > > +	if (unotif.flags)
> > > > +		return -EINVAL;
> > > > +
> > > 
> > > Might it make sense to use
> > > 
> > > 	err = copy_struct_from_user(&unotif, sizeof(unotif), buf, sizeof(unotif));
> > > 	if (err)
> > > 		return err;
> > > 
> > > This way we check that the whole struct is 0 and report an error as soon
> > > as one of the members is non-zero. That's more drastic but it'd ensure
> > > that other fields can be used in the future for whatever purposes.
> > > It would also let us get rid of the memset() below. 
> > 
> > Given that this isn't an extensible struct, it would be simpler to just do
> > check_zeroed_user() -- copy_struct_from_user() is overkill. That would
> > also remove the need for any copy_from_user()s and the memset can be
> > dropped by just doing
> > 
> >   struct seccomp_notif unotif = {};
> 
> This doesn't zero the padding according to the C standard, so no, you
> can't drop the memset, or you may leak kernel stack bits.

Ah right, I didn't double-check if there was any un-named. IMHO, It's a
bit odd to have un-named padding in a struct intended for extensions
(specifically to avoid these problems -- because it means userspace will
pass garbage by accident and there's nothing we can do about it). But
it's a bit late to worry about that now. :P

-- 
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH] seccomp: Check flags on seccomp_notif is unset

[Aleksa Sarai]

[-- Attachment #1: Type: text/plain, Size: 4071 bytes --]

On 2019-12-27, Aleksa Sarai <cyphar@cyphar.com> wrote:
> On 2019-12-26, Christian Brauner <christian.brauner@ubuntu.com> wrote:
> > On December 26, 2019 3:32:29 PM GMT+01:00, Aleksa Sarai <cyphar@cyphar.com> wrote:
> > >On 2019-12-26, Christian Brauner <christian.brauner@ubuntu.com> wrote:
> > >> On Wed, Dec 25, 2019 at 09:45:33PM +0000, Sargun Dhillon wrote:
> > >> > This patch is a small change in enforcement of the uapi for
> > >> > SECCOMP_IOCTL_NOTIF_RECV ioctl. Specificaly, the datastructure
> > >which is
> > >> > passed (seccomp_notif), has a flags member. Previously that could
> > >be
> > >> > set to a nonsense value, and we would ignore it. This ensures that
> > >> > no flags are set.
> > >> > 
> > >> > Signed-off-by: Sargun Dhillon <sargun@sargun.me>
> > >> > Cc: Kees Cook <keescook@chromium.org>
> > >> 
> > >> I'm fine with this since we soon want to make use of the flag
> > >argument
> > >> when we add a flag to get a pidfd from the seccomp notifier on
> > >receive.
> > >> The major users I could identify already pass in seccomp_notif with
> > >all
> > >> fields set to 0. If we really break users we can always revert; this
> > >> seems very unlikely to me though.
> > >> 
> > >> One more question below, otherwise:
> > >> 
> > >> Reviewed-by: Christian Brauner <christian.brauner@ubuntu.com>
> > >> 
> > >> > ---
> > >> >  kernel/seccomp.c | 7 +++++++
> > >> >  1 file changed, 7 insertions(+)
> > >> > 
> > >> > diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> > >> > index 12d2227e5786..455925557490 100644
> > >> > --- a/kernel/seccomp.c
> > >> > +++ b/kernel/seccomp.c
> > >> > @@ -1026,6 +1026,13 @@ static long seccomp_notify_recv(struct
> > >seccomp_filter *filter,
> > >> >  	struct seccomp_notif unotif;
> > >> >  	ssize_t ret;
> > >> >  
> > >> > +	if (copy_from_user(&unotif, buf, sizeof(unotif)))
> > >> > +		return -EFAULT;
> > >> > +
> > >> > +	/* flags is reserved right now, make sure it's unset */
> > >> > +	if (unotif.flags)
> > >> > +		return -EINVAL;
> > >> > +
> > >> 
> > >> Might it make sense to use
> > >> 
> > >> 	err = copy_struct_from_user(&unotif, sizeof(unotif), buf,
> > >sizeof(unotif));
> > >> 	if (err)
> > >> 		return err;
> > >> 
> > >> This way we check that the whole struct is 0 and report an error as
> > >soon
> > >> as one of the members is non-zero. That's more drastic but it'd
> > >ensure
> > >> that other fields can be used in the future for whatever purposes.
> > >> It would also let us get rid of the memset() below. 
> > >
> > >Given that this isn't an extensible struct, it would be simpler to just
> > >do
> > >check_zeroed_user() -- copy_struct_from_user() is overkill. That would
> > >also remove the need for any copy_from_user()s and the memset can be
> > >dropped by just doing
> > >
> > >  struct seccomp_notif unotif = {};
> > >
> > >> >  	memset(&unotif, 0, sizeof(unotif));
> > >> >  
> > >> >  	ret = down_interruptible(&filter->notif->request);
> > >> > -- 
> > >> > 2.20.1
> > >> > 
> > 
> > It is an extensible struct. That's why we have notifier size checking built in.
> 
> Ah right, NOTIF_GET_SIZES. I reckon check_zeroed_user() is still a bit
> simpler since none of the fields are used right now (and really, this
> patch should be checking all of them, not just ->flags, if we want to
> use any of them in the future).

Scratch that -- as Tycho just mentioned, there is un-named padding in
the struct so check_zeroed_user() is the wrong thing to do. But this
also will make extensions harder to deal with because (presumably) they
will also have un-named padding, making copy_struct_from_user() the
wrong thing to do as well.

So while there's not much to be done to fix the current struct layout, I
humbly suggest that any future struct extensions should not have any
un-named padding (so that at the very least you could use
copy_struct_from_user() in some form).

-- 
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH] seccomp: Check flags on seccomp_notif is unset

[Christian Brauner]

On Fri, Dec 27, 2019 at 01:31:31PM +1100, Aleksa Sarai wrote:
> On 2019-12-27, Aleksa Sarai <cyphar@cyphar.com> wrote:
> > On 2019-12-26, Christian Brauner <christian.brauner@ubuntu.com> wrote:
> > > On December 26, 2019 3:32:29 PM GMT+01:00, Aleksa Sarai <cyphar@cyphar.com> wrote:
> > > >On 2019-12-26, Christian Brauner <christian.brauner@ubuntu.com> wrote:
> > > >> On Wed, Dec 25, 2019 at 09:45:33PM +0000, Sargun Dhillon wrote:
> > > >> > This patch is a small change in enforcement of the uapi for
> > > >> > SECCOMP_IOCTL_NOTIF_RECV ioctl. Specificaly, the datastructure
> > > >which is
> > > >> > passed (seccomp_notif), has a flags member. Previously that could
> > > >be
> > > >> > set to a nonsense value, and we would ignore it. This ensures that
> > > >> > no flags are set.
> > > >> > 
> > > >> > Signed-off-by: Sargun Dhillon <sargun@sargun.me>
> > > >> > Cc: Kees Cook <keescook@chromium.org>
> > > >> 
> > > >> I'm fine with this since we soon want to make use of the flag
> > > >argument
> > > >> when we add a flag to get a pidfd from the seccomp notifier on
> > > >receive.
> > > >> The major users I could identify already pass in seccomp_notif with
> > > >all
> > > >> fields set to 0. If we really break users we can always revert; this
> > > >> seems very unlikely to me though.
> > > >> 
> > > >> One more question below, otherwise:
> > > >> 
> > > >> Reviewed-by: Christian Brauner <christian.brauner@ubuntu.com>
> > > >> 
> > > >> > ---
> > > >> >  kernel/seccomp.c | 7 +++++++
> > > >> >  1 file changed, 7 insertions(+)
> > > >> > 
> > > >> > diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> > > >> > index 12d2227e5786..455925557490 100644
> > > >> > --- a/kernel/seccomp.c
> > > >> > +++ b/kernel/seccomp.c
> > > >> > @@ -1026,6 +1026,13 @@ static long seccomp_notify_recv(struct
> > > >seccomp_filter *filter,
> > > >> >  	struct seccomp_notif unotif;
> > > >> >  	ssize_t ret;
> > > >> >  
> > > >> > +	if (copy_from_user(&unotif, buf, sizeof(unotif)))
> > > >> > +		return -EFAULT;
> > > >> > +
> > > >> > +	/* flags is reserved right now, make sure it's unset */
> > > >> > +	if (unotif.flags)
> > > >> > +		return -EINVAL;
> > > >> > +
> > > >> 
> > > >> Might it make sense to use
> > > >> 
> > > >> 	err = copy_struct_from_user(&unotif, sizeof(unotif), buf,
> > > >sizeof(unotif));
> > > >> 	if (err)
> > > >> 		return err;
> > > >> 
> > > >> This way we check that the whole struct is 0 and report an error as
> > > >soon
> > > >> as one of the members is non-zero. That's more drastic but it'd
> > > >ensure
> > > >> that other fields can be used in the future for whatever purposes.
> > > >> It would also let us get rid of the memset() below. 
> > > >
> > > >Given that this isn't an extensible struct, it would be simpler to just
> > > >do
> > > >check_zeroed_user() -- copy_struct_from_user() is overkill. That would
> > > >also remove the need for any copy_from_user()s and the memset can be
> > > >dropped by just doing
> > > >
> > > >  struct seccomp_notif unotif = {};
> > > >
> > > >> >  	memset(&unotif, 0, sizeof(unotif));
> > > >> >  
> > > >> >  	ret = down_interruptible(&filter->notif->request);
> > > >> > -- 
> > > >> > 2.20.1
> > > >> > 
> > > 
> > > It is an extensible struct. That's why we have notifier size checking built in.
> > 
> > Ah right, NOTIF_GET_SIZES. I reckon check_zeroed_user() is still a bit
> > simpler since none of the fields are used right now (and really, this
> > patch should be checking all of them, not just ->flags, if we want to
> > use any of them in the future).
> 
> Scratch that -- as Tycho just mentioned, there is un-named padding in
> the struct so check_zeroed_user() is the wrong thing to do. But this

Hm, I don't think so.
I understood Tycho's point as _if_ there ever is padding then this would
not be zeroed.
Right now, there is no padding since the struct is correctly padded:

struct seccomp_data {
	int nr;
	__u32 arch;
	__u64 instruction_pointer;
	__u64 args[6];
};

struct seccomp_notif {
	__u64 id;
	__u32 pid;
	__u32 flags;
	struct seccomp_data data;
};

which would be - using pahole:

struct seccomp_data {
        int                        nr;                   /*     0     4 */
        __u32                      arch;                 /*     4     4 */
        __u64                      instruction_pointer;  /*     8     8 */
        __u64                      args[6];              /*    16    48 */

        /* size: 64, cachelines: 1, members: 4 */
};
struct seccomp_notif {
        __u64                      id;                   /*     0     8 */
        __u32                      pid;                  /*     8     4 */
        __u32                      flags;                /*    12     4 */
        struct seccomp_data data;                        /*    16    64 */

        /* size: 80, cachelines: 2, members: 4 */
        /* last cacheline: 16 bytes */
};

The only worry would be a 2byte int type but there's no architecture
we support which does this right now afaict.

> also will make extensions harder to deal with because (presumably) they
> will also have un-named padding, making copy_struct_from_user() the

This all will be a non-issue if we just use __u64 for extensions.

My point about using copy_struct_from_user() was that we should verify
that _all_ fields are uninitialized and not just the flags argument
since we might introduce a flags argument that requires another already
existing member in seccomp_notif to be set to a value. We should do this
change now so we don't have to risk breaking someone in the future.

I'm trying to get at least Mozilla/Firefox off of their crazy
SECCOMP_RET_TRAP way of implementing their broker onto the user notifier
and they will likely need some extensions. That includes the pidfd stuff
for seccomp that Sargun will likely be doing and the new pidfd_getfd()
syscall. So it's not unlikely that we might need other already existing
fields in that struct to be set to some value.

I don't particulary care how we do it:
- We can do a simple copy_from_user() and check each field individually.
- Use copy_struct_from_user().
  That is safe to do right now since there is no padding afaict and
  it'll automatically verify new fields as well.
  If I understand the worry correctly then the argument against
  copy_struct_from_user() here is that there might be padding introduced
  and userspace will not do an explicit memset() but rather rely on an
  empty inializer {} and will _accidently_ pass down a struct which has
  __all fields cleared__ but __uninitialized padding__ and we tell them
  EINVAL? That can only happen if we introduce padding in the struct
  which I'd argue we just don't do. That'll be in line with what we
  require from our ABIs already anyway.

Christian

Re: [PATCH] seccomp: Check flags on seccomp_notif is unset

[Sargun Dhillon]

On Fri, Dec 27, 2019 at 6:47 AM Christian Brauner
<christian.brauner@ubuntu.com> wrote:
>
> On Fri, Dec 27, 2019 at 01:31:31PM +1100, Aleksa Sarai wrote:
> > On 2019-12-27, Aleksa Sarai <cyphar@cyphar.com> wrote:
> >
> > Scratch that -- as Tycho just mentioned, there is un-named padding in
> > the struct so check_zeroed_user() is the wrong thing to do. But this
>
> Hm, I don't think so.
> I understood Tycho's point as _if_ there ever is padding then this would
> not be zeroed.
> Right now, there is no padding since the struct is correctly padded:
>
> struct seccomp_data {
>         int nr;
>         __u32 arch;
>         __u64 instruction_pointer;
>         __u64 args[6];
> };
>
> struct seccomp_notif {
>         __u64 id;
>         __u32 pid;
>         __u32 flags;
>         struct seccomp_data data;
> };
>
> which would be - using pahole:
>
> struct seccomp_data {
>         int                        nr;                   /*     0     4 */
>         __u32                      arch;                 /*     4     4 */
>         __u64                      instruction_pointer;  /*     8     8 */
>         __u64                      args[6];              /*    16    48 */
>
>         /* size: 64, cachelines: 1, members: 4 */
> };
> struct seccomp_notif {
>         __u64                      id;                   /*     0     8 */
>         __u32                      pid;                  /*     8     4 */
>         __u32                      flags;                /*    12     4 */
>         struct seccomp_data data;                        /*    16    64 */
>
>         /* size: 80, cachelines: 2, members: 4 */
>         /* last cacheline: 16 bytes */
> };
>
> The only worry would be a 2byte int type but there's no architecture
> we support which does this right now afaict.
>
> > also will make extensions harder to deal with because (presumably) they
> > will also have un-named padding, making copy_struct_from_user() the
>
> This all will be a non-issue if we just use __u64 for extensions.
>
> My point about using copy_struct_from_user() was that we should verify
> that _all_ fields are uninitialized and not just the flags argument
> since we might introduce a flags argument that requires another already
> existing member in seccomp_notif to be set to a value. We should do this
> change now so we don't have to risk breaking someone in the future.
>
> I'm trying to get at least Mozilla/Firefox off of their crazy
> SECCOMP_RET_TRAP way of implementing their broker onto the user notifier
> and they will likely need some extensions. That includes the pidfd stuff
> for seccomp that Sargun will likely be doing and the new pidfd_getfd()
> syscall. So it's not unlikely that we might need other already existing
> fields in that struct to be set to some value.
>
> I don't particulary care how we do it:
> - We can do a simple copy_from_user() and check each field individually.
Just doing a simple copy_from_user, and for now, calling memchr_inv
on the whole thing. We can drop the memset, and just leave a note to
indicate that if unpadded fields are introduced in the future, this structure
must be manually zeroed out. Although, this might be laying a trap for
ourselves.

This leaves us in a good position for introducing a flag field in the future.
All we have to do is change the memchr_inv from checking on an
entire struct basis to checking on a per-field basis.

> - Use copy_struct_from_user().
>   That is safe to do right now since there is no padding afaict and
>   it'll automatically verify new fields as well.
>   If I understand the worry correctly then the argument against
>   copy_struct_from_user() here is that there might be padding introduced
>   and userspace will not do an explicit memset() but rather rely on an
>   empty inializer {} and will _accidently_ pass down a struct which has
>   __all fields cleared__ but __uninitialized padding__ and we tell them
>   EINVAL? That can only happen if we introduce padding in the struct
>   which I'd argue we just don't do. That'll be in line with what we
>   require from our ABIs already anyway.
>
> Christian

Re: [PATCH] seccomp: Check flags on seccomp_notif is unset

[Tycho Andersen]

On Fri, Dec 27, 2019 at 09:22:20AM -0500, Sargun Dhillon wrote:
> Just doing a simple copy_from_user, and for now, calling memchr_inv
> on the whole thing. We can drop the memset, and just leave a note to
> indicate that if unpadded fields are introduced in the future, this structure
> must be manually zeroed out. Although, this might be laying a trap for
> ourselves.

Yes, please keep the memset().

Tycho

Re: [PATCH] seccomp: Check flags on seccomp_notif is unset

[Aleksa Sarai]

[-- Attachment #1: Type: text/plain, Size: 4713 bytes --]

On 2019-12-27, Sargun Dhillon <sargun@sargun.me> wrote:
> On Fri, Dec 27, 2019 at 6:47 AM Christian Brauner
> <christian.brauner@ubuntu.com> wrote:
> >
> > On Fri, Dec 27, 2019 at 01:31:31PM +1100, Aleksa Sarai wrote:
> > > On 2019-12-27, Aleksa Sarai <cyphar@cyphar.com> wrote:
> > >
> > > Scratch that -- as Tycho just mentioned, there is un-named padding in
> > > the struct so check_zeroed_user() is the wrong thing to do. But this
> >
> > Hm, I don't think so.
> > I understood Tycho's point as _if_ there ever is padding then this would
> > not be zeroed.
> > Right now, there is no padding since the struct is correctly padded:
> >
> > struct seccomp_data {
> >         int nr;
> >         __u32 arch;
> >         __u64 instruction_pointer;
> >         __u64 args[6];
> > };
> >
> > struct seccomp_notif {
> >         __u64 id;
> >         __u32 pid;
> >         __u32 flags;
> >         struct seccomp_data data;
> > };
> >
> > which would be - using pahole:
> >
> > struct seccomp_data {
> >         int                        nr;                   /*     0     4 */
> >         __u32                      arch;                 /*     4     4 */
> >         __u64                      instruction_pointer;  /*     8     8 */
> >         __u64                      args[6];              /*    16    48 */
> >
> >         /* size: 64, cachelines: 1, members: 4 */
> > };
> > struct seccomp_notif {
> >         __u64                      id;                   /*     0     8 */
> >         __u32                      pid;                  /*     8     4 */
> >         __u32                      flags;                /*    12     4 */
> >         struct seccomp_data data;                        /*    16    64 */
> >
> >         /* size: 80, cachelines: 2, members: 4 */
> >         /* last cacheline: 16 bytes */
> > };
> >
> > The only worry would be a 2byte int type but there's no architecture
> > we support which does this right now afaict.
> >
> > > also will make extensions harder to deal with because (presumably) they
> > > will also have un-named padding, making copy_struct_from_user() the
> >
> > This all will be a non-issue if we just use __u64 for extensions.
> >
> > My point about using copy_struct_from_user() was that we should verify
> > that _all_ fields are uninitialized and not just the flags argument
> > since we might introduce a flags argument that requires another already
> > existing member in seccomp_notif to be set to a value. We should do this
> > change now so we don't have to risk breaking someone in the future.
> >
> > I'm trying to get at least Mozilla/Firefox off of their crazy
> > SECCOMP_RET_TRAP way of implementing their broker onto the user notifier
> > and they will likely need some extensions. That includes the pidfd stuff
> > for seccomp that Sargun will likely be doing and the new pidfd_getfd()
> > syscall. So it's not unlikely that we might need other already existing
> > fields in that struct to be set to some value.
> >
> > I don't particulary care how we do it:
> > - We can do a simple copy_from_user() and check each field individually.
> 
> Just doing a simple copy_from_user, and for now, calling memchr_inv
> on the whole thing. We can drop the memset, and just leave a note to
> indicate that if unpadded fields are introduced in the future, this structure
> must be manually zeroed out. Although, this might be laying a trap for
> ourselves.
> 
> This leaves us in a good position for introducing a flag field in the future.
> All we have to do is change the memchr_inv from checking on an
> entire struct basis to checking on a per-field basis.

There is no need to do memchr_inv() on copy_from_user() to check for
zero-ness. That's the entire point of check_zeroed_user() -- to not need
to do it that way.

> > - Use copy_struct_from_user().
> >   That is safe to do right now since there is no padding afaict and
> >   it'll automatically verify new fields as well.
> >   If I understand the worry correctly then the argument against
> >   copy_struct_from_user() here is that there might be padding introduced
> >   and userspace will not do an explicit memset() but rather rely on an
> >   empty inializer {} and will _accidently_ pass down a struct which has
> >   __all fields cleared__ but __uninitialized padding__ and we tell them
> >   EINVAL? That can only happen if we introduce padding in the struct
> >   which I'd argue we just don't do. That'll be in line with what we
> >   require from our ABIs already anyway.


-- 
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH] seccomp: Check flags on seccomp_notif is unset

[Christian Brauner]

On December 27, 2019 4:15:01 PM GMT+01:00, Aleksa Sarai <cyphar@cyphar.com> wrote:
>On 2019-12-27, Sargun Dhillon <sargun@sargun.me> wrote:
>> On Fri, Dec 27, 2019 at 6:47 AM Christian Brauner
>> <christian.brauner@ubuntu.com> wrote:
>> >
>> > On Fri, Dec 27, 2019 at 01:31:31PM +1100, Aleksa Sarai wrote:
>> > > On 2019-12-27, Aleksa Sarai <cyphar@cyphar.com> wrote:
>> > >
>> > > Scratch that -- as Tycho just mentioned, there is un-named
>padding in
>> > > the struct so check_zeroed_user() is the wrong thing to do. But
>this
>> >
>> > Hm, I don't think so.
>> > I understood Tycho's point as _if_ there ever is padding then this
>would
>> > not be zeroed.
>> > Right now, there is no padding since the struct is correctly
>padded:
>> >
>> > struct seccomp_data {
>> >         int nr;
>> >         __u32 arch;
>> >         __u64 instruction_pointer;
>> >         __u64 args[6];
>> > };
>> >
>> > struct seccomp_notif {
>> >         __u64 id;
>> >         __u32 pid;
>> >         __u32 flags;
>> >         struct seccomp_data data;
>> > };
>> >
>> > which would be - using pahole:
>> >
>> > struct seccomp_data {
>> >         int                        nr;                   /*     0  
>  4 */
>> >         __u32                      arch;                 /*     4  
>  4 */
>> >         __u64                      instruction_pointer;  /*     8  
>  8 */
>> >         __u64                      args[6];              /*    16  
> 48 */
>> >
>> >         /* size: 64, cachelines: 1, members: 4 */
>> > };
>> > struct seccomp_notif {
>> >         __u64                      id;                   /*     0  
>  8 */
>> >         __u32                      pid;                  /*     8  
>  4 */
>> >         __u32                      flags;                /*    12  
>  4 */
>> >         struct seccomp_data data;                        /*    16  
> 64 */
>> >
>> >         /* size: 80, cachelines: 2, members: 4 */
>> >         /* last cacheline: 16 bytes */
>> > };
>> >
>> > The only worry would be a 2byte int type but there's no
>architecture
>> > we support which does this right now afaict.
>> >
>> > > also will make extensions harder to deal with because
>(presumably) they
>> > > will also have un-named padding, making copy_struct_from_user()
>the
>> >
>> > This all will be a non-issue if we just use __u64 for extensions.
>> >
>> > My point about using copy_struct_from_user() was that we should
>verify
>> > that _all_ fields are uninitialized and not just the flags argument
>> > since we might introduce a flags argument that requires another
>already
>> > existing member in seccomp_notif to be set to a value. We should do
>this
>> > change now so we don't have to risk breaking someone in the future.
>> >
>> > I'm trying to get at least Mozilla/Firefox off of their crazy
>> > SECCOMP_RET_TRAP way of implementing their broker onto the user
>notifier
>> > and they will likely need some extensions. That includes the pidfd
>stuff
>> > for seccomp that Sargun will likely be doing and the new
>pidfd_getfd()
>> > syscall. So it's not unlikely that we might need other already
>existing
>> > fields in that struct to be set to some value.
>> >
>> > I don't particulary care how we do it:
>> > - We can do a simple copy_from_user() and check each field
>individually.
>> 
>> Just doing a simple copy_from_user, and for now, calling memchr_inv
>> on the whole thing. We can drop the memset, and just leave a note to
>> indicate that if unpadded fields are introduced in the future, this
>structure
>> must be manually zeroed out. Although, this might be laying a trap
>for
>> ourselves.
>> 
>> This leaves us in a good position for introducing a flag field in the
>future.
>> All we have to do is change the memchr_inv from checking on an
>> entire struct basis to checking on a per-field basis.
>
>There is no need to do memchr_inv() on copy_from_user() to check for
>zero-ness. That's the entire point of check_zeroed_user() -- to not
>need
>to do it that way.

Right, we added that too a while ago.
Let's use it.

Christian