[prev in list] [next in list] [prev in thread] [next in thread] 

List:       linux-api
Subject:    Re: [PATCH RESEND v3 3/4] sched_setattr: switch to copy_struct_from_user()
From:       Kees Cook <keescook () chromium ! org>
Date:       2019-09-30 23:43:57
Message-ID: 201909301643.570C02E () keescook
[Download RAW message or body]

On Tue, Oct 01, 2019 at 05:15:25AM +1000, Aleksa Sarai wrote:
> From: Aleksa Sarai <cyphar@cyphar.com>
> 
> The change is very straightforward, and helps unify the syscall
> interface for struct-from-userspace syscalls. Ideally we could also
> unify sched_getattr(2)-style syscalls as well, but unfortunately the
> correct semantics for such syscalls are much less clear (see [1] for
> more detail). In future we could come up with a more sane idea for how
> the syscall interface should look.
> 
> [1]: commit 1251201c0d34 ("sched/core: Fix uclamp ABI bug, clean up and
> robustify sched_read_attr() ABI logic and code")
> 
> Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>

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

-Kees

> ---
> kernel/sched/core.c | 43 +++++++------------------------------------
> 1 file changed, 7 insertions(+), 36 deletions(-)
> 
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 7880f4f64d0e..dd05a378631a 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -5106,9 +5106,6 @@ static int sched_copy_attr(struct sched_attr __user *uattr, \
> struct sched_attr *a  u32 size;
> 	int ret;
> 
> -	if (!access_ok(uattr, SCHED_ATTR_SIZE_VER0))
> -		return -EFAULT;
> -
> 	/* Zero the full structure, so that a short copy will be nice: */
> 	memset(attr, 0, sizeof(*attr));
> 
> @@ -5116,45 +5113,19 @@ static int sched_copy_attr(struct sched_attr __user *uattr, \
> struct sched_attr *a  if (ret)
> 		return ret;
> 
> -	/* Bail out on silly large: */
> -	if (size > PAGE_SIZE)
> -		goto err_size;
> -
> 	/* ABI compatibility quirk: */
> 	if (!size)
> 		size = SCHED_ATTR_SIZE_VER0;
> -
> -	if (size < SCHED_ATTR_SIZE_VER0)
> +	if (size < SCHED_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;
> -
> 	if ((attr->sched_flags & SCHED_FLAG_UTIL_CLAMP) &&
> 	    size < SCHED_ATTR_SIZE_VER1)
> 		return -EINVAL;
> @@ -5354,7 +5325,7 @@ sched_attr_copy_to_user(struct sched_attr __user *uattr,
> * sys_sched_getattr - similar to sched_getparam, but with sched_attr
> * @pid: the pid in question.
> * @uattr: structure containing the extended parameters.
> - * @usize: sizeof(attr) that user-space knows about, for forwards and backwards \
> compatibility. + * @usize: sizeof(attr) for fwd/bwd comp.
> * @flags: for future extension.
> */
> SYSCALL_DEFINE4(sched_getattr, pid_t, pid, struct sched_attr __user *, uattr,
> -- 
> 2.23.0
> 

-- 
Kees Cook


[prev in list] [next in list] [prev in thread] [next in thread]