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

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]

Date:	Tue, 12 Nov 2013 12:32:10 -0500
From:	Steven Rostedt <rostedt@...dmis.org>
To:	Juri Lelli <juri.lelli@...il.com>
Cc:	peterz@...radead.org, tglx@...utronix.de, mingo@...hat.com,
	oleg@...hat.com, fweisbec@...il.com, darren@...art.com,
	johan.eker@...csson.com, p.faure@...tech.ch,
	linux-kernel@...r.kernel.org, claudio@...dence.eu.com,
	michael@...rulasolutions.com, fchecconi@...il.com,
	tommaso.cucinotta@...up.it, nicola.manica@...i.unitn.it,
	luca.abeni@...tn.it, dhaval.giani@...il.com, hgu1972@...il.com,
	paulmck@...ux.vnet.ibm.com, raistlin@...ux.it,
	insop.song@...il.com, liming.wang@...driver.com, jkacur@...hat.com,
	harald.gustafsson@...csson.com, vincent.guittot@...aro.org,
	bruce.ashfield@...driver.com
Subject: Re: [PATCH 02/14] sched: add extended scheduling interface.

On Thu,  7 Nov 2013 14:43:36 +0100
Juri Lelli <juri.lelli@...il.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.

-- Steve



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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/