linux-kernel - Re: [PATCH RESEND v3 4/4] perf_event_open: switch to copy_struct_from

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <201909301644.7106650E14@keescook>
Date:   Mon, 30 Sep 2019 16:44:24 -0700
From:   Kees Cook <keescook@...omium.org>
To:     Aleksa Sarai <asarai@...e.de>
Cc:     Ingo Molnar <mingo@...hat.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Jiri Olsa <jolsa@...hat.com>,
        Namhyung Kim <namhyung@...nel.org>,
        Christian Brauner <christian@...uner.io>,
        Aleksa Sarai <cyphar@...har.com>,
        Rasmus Villemoes <linux@...musvillemoes.dk>,
        Al Viro <viro@...iv.linux.org.uk>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        libc-alpha@...rceware.org, linux-api@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH RESEND v3 4/4] perf_event_open: switch to
 copy_struct_from_user()

On Tue, Oct 01, 2019 at 05:15:26AM +1000, Aleksa Sarai wrote:
> From: Aleksa Sarai <cyphar@...har.com>
> 
> The change is very straightforward, and helps unify the syscall
> interface for struct-from-userspace syscalls.
> 
> Signed-off-by: Aleksa Sarai <cyphar@...har.com>

Reviewed-by: Kees Cook <keescook@...omium.org>

-Kees

> ---
>  kernel/events/core.c | 47 +++++++++-----------------------------------
>  1 file changed, 9 insertions(+), 38 deletions(-)
> 
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 4655adbbae10..3f0cb82e4fbc 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -10586,55 +10586,26 @@ static int perf_copy_attr(struct perf_event_attr __user *uattr,
>  	u32 size;
>  	int ret;
>  
> -	if (!access_ok(uattr, PERF_ATTR_SIZE_VER0))
> -		return -EFAULT;
> -
> -	/*
> -	 * zero the full structure, so that a short copy will be nice.
> -	 */
> +	/* Zero the full structure, so that a short copy will be nice. */
>  	memset(attr, 0, sizeof(*attr));
>  
>  	ret = get_user(size, &uattr->size);
>  	if (ret)
>  		return ret;
>  
> -	if (size > PAGE_SIZE)	/* silly large */
> -		goto err_size;
> -
> -	if (!size)		/* abi compat */
> +	/* ABI compatibility quirk: */
> +	if (!size)
>  		size = PERF_ATTR_SIZE_VER0;
> -
> -	if (size < PERF_ATTR_SIZE_VER0)
> +	if (size < PERF_ATTR_SIZE_VER0 || size > PAGE_SIZE)
>  		goto err_size;
>  
> -	/*
> -	 * If we're handed a bigger struct than we know of,
> -	 * ensure all the unknown bits are 0 - i.e. new
> -	 * user-space does not rely on any kernel feature
> -	 * extensions we dont know about yet.
> -	 */
> -	if (size > sizeof(*attr)) {
> -		unsigned char __user *addr;
> -		unsigned char __user *end;
> -		unsigned char val;
> -
> -		addr = (void __user *)uattr + sizeof(*attr);
> -		end  = (void __user *)uattr + size;
> -
> -		for (; addr < end; addr++) {
> -			ret = get_user(val, addr);
> -			if (ret)
> -				return ret;
> -			if (val)
> -				goto err_size;
> -		}
> -		size = sizeof(*attr);
> +	ret = copy_struct_from_user(attr, sizeof(*attr), uattr, size);
> +	if (ret) {
> +		if (ret == -E2BIG)
> +			goto err_size;
> +		return ret;
>  	}
>  
> -	ret = copy_from_user(attr, uattr, size);
> -	if (ret)
> -		return -EFAULT;
> -
>  	attr->size = size;
>  
>  	if (attr->__reserved_1)
> -- 
> 2.23.0
> 

-- 
Kees Cook