Re: [PATCH 02/14] sched: add extended scheduling interface.

[Juri Lelli <juri.lelli@gmail.com>]

On 11/12/2013 06:32 PM, Steven Rostedt wrote:
> On Thu,  7 Nov 2013 14:43:36 +0100
> Juri Lelli <juri.lelli@gmail.com> wrote:
> 
>   
>> +static int
>> +do_sched_setscheduler2(pid_t pid, int policy,
>> +			 struct sched_param2 __user *param2)
>> +{
>> +	struct sched_param2 lparam2;
>> +	struct task_struct *p;
>> +	int retval;
>> +
>> +	if (!param2 || pid < 0)
>> +		return -EINVAL;
>> +
>> +	memset(&lparam2, 0, sizeof(struct sched_param2));
>> +	if (copy_from_user(&lparam2, param2, sizeof(struct sched_param2)))
>> +		return -EFAULT;
> 
> Why the memset() before the copy_from_user()? We are copying
> sizeof(sched_param2) anyway, and should overwrite anything that was on
> the stack. I'm not aware of any possible leak from copying from
> userspace. I could understand it if we were copying to userspace.
> 
> do_sched_setscheduler() doesn't do that either.
> 
>> +
>> +	rcu_read_lock();
>> +	retval = -ESRCH;
>> +	p = find_process_by_pid(pid);
>> +	if (p != NULL)
>> +		retval = sched_setscheduler2(p, policy, &lparam2);
>> +	rcu_read_unlock();
>> +
>> +	return retval;
>> +}
>> +
>>  /**
>>   * sys_sched_setscheduler - set/change the scheduler policy and RT priority
>>   * @pid: the pid in question.
>> @@ -3514,6 +3553,21 @@ SYSCALL_DEFINE3(sched_setscheduler, pid_t, pid, int, policy,
>>  }
>>  
>>  /**
>> + * sys_sched_setscheduler2 - same as above, but with extended sched_param
>> + * @pid: the pid in question.
>> + * @policy: new policy (could use extended sched_param).
>> + * @param: structure containg the extended parameters.
>> + */
>> +SYSCALL_DEFINE3(sched_setscheduler2, pid_t, pid, int, policy,
>> +		struct sched_param2 __user *, param2)
>> +{
>> +	if (policy < 0)
>> +		return -EINVAL;
>> +
>> +	return do_sched_setscheduler2(pid, policy, param2);
>> +}
>> +
>> +/**
>>   * sys_sched_setparam - set/change the RT priority of a thread
>>   * @pid: the pid in question.
>>   * @param: structure containing the new RT priority.
>> @@ -3526,6 +3580,17 @@ SYSCALL_DEFINE2(sched_setparam, pid_t, pid, struct sched_param __user *, param)
>>  }
>>  
>>  /**
>> + * sys_sched_setparam2 - same as above, but with extended sched_param
>> + * @pid: the pid in question.
>> + * @param2: structure containing the extended parameters.
>> + */
>> +SYSCALL_DEFINE2(sched_setparam2, pid_t, pid,
>> +		struct sched_param2 __user *, param2)
>> +{
>> +	return do_sched_setscheduler2(pid, -1, param2);
>> +}
>> +
>> +/**
>>   * sys_sched_getscheduler - get the policy (scheduling class) of a thread
>>   * @pid: the pid in question.
>>   *
>> @@ -3595,6 +3660,45 @@ out_unlock:
>>  	return retval;
>>  }
>>  
>> +/**
>> + * sys_sched_getparam2 - same as above, but with extended sched_param
>> + * @pid: the pid in question.
>> + * @param2: structure containing the extended parameters.
>> + */
>> +SYSCALL_DEFINE2(sched_getparam2, pid_t, pid,
>> +		struct sched_param2 __user *, param2)
>> +{
>> +	struct sched_param2 lp;
>> +	struct task_struct *p;
>> +	int retval;
>> +
>> +	if (!param2 || pid < 0)
>> +		return -EINVAL;
>> +
>> +	rcu_read_lock();
>> +	p = find_process_by_pid(pid);
>> +	retval = -ESRCH;
>> +	if (!p)
>> +		goto out_unlock;
>> +
>> +	retval = security_task_getscheduler(p);
>> +	if (retval)
>> +		goto out_unlock;
>> +
>> +	lp.sched_priority = p->rt_priority;
>> +	rcu_read_unlock();
>> +
> 
> OK, now we are missing the memset(). This does leak info, as lp never
> was set to zero, it just contains anything on the stack, and the only
> value you updated was sched_priority. We just copied to user memory
> from the kernel stack.

Right! memset() moved:

@@ -3779,7 +3779,6 @@ do_sched_setscheduler2(pid_t pid, int policy,
        if (!param2 || pid < 0)
                return -EINVAL;

-       memset(&lparam2, 0, sizeof(struct sched_param2));
        if (copy_from_user(&lparam2, param2, sizeof(struct sched_param2)))
                return -EFAULT;

@@ -3937,6 +3936,8 @@ SYSCALL_DEFINE2(sched_getparam2, pid_t, pid,
        if (!param2 || pid < 0)
                return -EINVAL;

+       memset(&lp, 0, sizeof(struct sched_param2));
+
        rcu_read_lock();
        p = find_process_by_pid(pid);
        retval = -ESRCH;

Thanks,

- Juri

> 
>> +	retval = copy_to_user(param2, &lp,
>> +			sizeof(struct sched_param2)) ? -EFAULT : 0;
>> +
>> +	return retval;
>> +
>> +out_unlock:
>> +	rcu_read_unlock();
>> +	return retval;
>> +
>> +}
>> +
>>  long sched_setaffinity(pid_t pid, const struct cpumask *in_mask)
>>  {
>>  	cpumask_var_t cpus_allowed, new_mask;
>