linux-kernel - Re: [PATCH v2 2/2] seccomp: Check that seccomp

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20191228070910.qo7ahodfs2mzqw5t@wittgenstein>
Date:   Sat, 28 Dec 2019 08:09:11 +0100
From:   Christian Brauner <christian.brauner@...ntu.com>
To:     Sargun Dhillon <sargun@...gun.me>
Cc:     linux-kernel@...r.kernel.org, linux-api@...r.kernel.org,
        tycho@...ho.ws, jannh@...gle.com, keescook@...omium.org,
        cyphar@...har.com
Subject: Re: [PATCH v2 2/2] seccomp: Check that seccomp_notif is zeroed out
 by the user

On Sat, Dec 28, 2019 at 01:48:51AM +0000, Sargun Dhillon wrote:
> This patch is a small change in enforcement of the uapi for
> SECCOMP_IOCTL_NOTIF_RECV ioctl. Specifically, the datastructure which
> is passed (seccomp_notif) must be zeroed out. Previously any of its
> members could be set to nonsense values, and we would ignore it.
> 
> This ensures all fields are set to their zero value.

The upper part is correct and useful.

> 
> This relies on the seccomp_notif datastructure to not have
> any unnamed padding, as it is valid to initialize the datastructure
> as:
> 
>   struct seccomp_notif notif = {};

The interesting part here is accidently leaking kernel addresses to
userspace. For this to be an issue we'd need to do
struct seccomp_notif unotif = {};
copy_to_user(<user-buffer>, &unotif, sizeof(unotif))
_and_ seccomp_notif would need to contain unintentional padding. Even if
the latter were true we still use memset() anwyay and will likely never
remove it. So the code here sure doesn't rely or depends on correct
padding at all. 

> 
> This only initializes named members to their 0-value [1].
> 
> [1]: https://lore.kernel.org/lkml/20191227023131.klnobtlfgeqcmvbb@yavin.dot.cyphar.com/

That link isn't useful and also incorrectly claims that there is
non-intentional padding in the struct which there isn't.

Just drop that whole paragraph. The expectation is that all of our ABIs
are correctly padded anyway and this really just confuses more than it
helps.
Please resend, otherwise:

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

But see a small comment below.

> 
> Signed-off-by: Sargun Dhillon <sargun@...gun.me>
> Cc: Kees Cook <keescook@...omium.org>
> ---
>  kernel/seccomp.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> index 12d2227e5786..4fd73cbdd01e 100644
> --- a/kernel/seccomp.c
> +++ b/kernel/seccomp.c
> @@ -1026,6 +1026,12 @@ static long seccomp_notify_recv(struct seccomp_filter *filter,
>  	struct seccomp_notif unotif;
>  	ssize_t ret;
>  
> +	ret = check_zeroed_user(buf, sizeof(unotif));

It wouldn't hurt to place a small comment here so the reader can easily
spot we've ensured that this struct can be extended. But up to you...

/* Verify that we're not given garbage to keep struct extensible. */

> +	if (ret < 0)
> +		return ret;
> +	if (!ret)
> +		return -EINVAL;
> +
>  	memset(&unotif, 0, sizeof(unotif));
>  
>  	ret = down_interruptible(&filter->notif->request);
> -- 
> 2.20.1
>