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

On 2019-12-27, Aleksa Sarai [off-list ref] wrote:

On 2019-12-26, Christian Brauner [off-list ref] wrote:

quoted

On December 26, 2019 3:32:29 PM GMT+01:00, Aleksa Sarai [off-list ref] wrote:

quoted

On 2019-12-26, Christian Brauner [off-list ref] wrote:

quoted

On Wed, Dec 25, 2019 at 09:45:33PM +0000, Sargun Dhillon wrote:

quoted

This patch is a small change in enforcement of the uapi for
SECCOMP_IOCTL_NOTIF_RECV ioctl. Specificaly, the datastructure

which is

quoted

quoted

passed (seccomp_notif), has a flags member. Previously that could

be

quoted

quoted

set to a nonsense value, and we would ignore it. This ensures that
no flags are set.

Signed-off-by: Sargun Dhillon <redacted>
Cc: Kees Cook <redacted>

I'm fine with this since we soon want to make use of the flag

argument

quoted

when we add a flag to get a pidfd from the seccomp notifier on

receive.

quoted

The major users I could identify already pass in seccomp_notif with

all

quoted

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 <redacted>

quoted

---
 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,

quoted

quoted

 	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));

quoted

	if (err)
		return err;

This way we check that the whole struct is 0 and report an error as

soon

quoted

as one of the members is non-zero. That's more drastic but it'd

ensure

quoted

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 = {};

quoted

quoted

 	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/>