Re: [PATCH] prctl: Add PR_SET_TIMERSLACK_PID for setting timer slack of an arbitrary thread.

[Kees Cook]

On Fri, Feb 5, 2016 at 10:08 AM, John Stultz <john.stultz@xxxxxxxxxx> wrote:
> From: Ruchi Kandoi <kandoiruchi@xxxxxxxxxx>
>
> This allows power/performance management software to set timer
> slack for other threads according to its policy for the thread
> (such as when the thread is designated foreground vs. background
> activity)
>
> Second argument is similar to PR_SET_TIMERSLACK, if non-zero
> then the slack is set to that value otherwise sets it to the
> default for the thread.
>
> Takes PID of the thread as the third argument.
>
> This interface checks that the calling task has permissions to
> to use PTRACE_MODE_ATTACH on the target task, so that we can
> ensure arbitrary apps do not change the timer slack for other
> apps.
>
> Additional fixes from Ruchi and Micha Kalfon <micha@xxxxxxxxxxx>
> have been folded into this patch to make it easier to reivew.
>
> Cc: Arjan van de Ven <arjan@xxxxxxxxxxxxxxx>
> Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Cc: Oren Laadan <orenl@xxxxxxxxxxx>
> Cc: Ruchi Kandoi <kandoiruchi@xxxxxxxxxx>
> Cc: Rom Lemarchand <romlem@xxxxxxxxxxx>
> Cc: Kees Cook <keescook@xxxxxxxxxxxx>
> Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> Cc: Android Kernel Team <kernel-team@xxxxxxxxxxx>
> Signed-off-by: Ruchi Kandoi <kandoiruchi@xxxxxxxxxx>
> [jstultz:
>  * Folded in CAP_SYS_NICE check from Ruchi.
>  * Folded in fix misplaced PR_SET_TIMERSLACK_PID case fix from
>    Micha.
>  * Folded in make PR_SET_TIMERSLACK_PID pid namespace aware fix
>    from Micha.
>  * Changed PR_SET_TIMERSLACK_PID so it didn't collide with
>    already upstream prctrl values.
>  * Reworked commit message.
>  * Moved from CAP_SYS_NICE to PTRACE_MODE_ATTACH for permissions
>    checks]
> Acked-by: Arjan van de Ven <arjan@xxxxxxxxxxxxxxx>
> Reviewed-by: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Signed-off-by: John Stultz <john.stultz@xxxxxxxxxx>
> ---
>  include/uapi/linux/prctl.h |  7 +++++++
>  kernel/sys.c               | 25 +++++++++++++++++++++++++
>  2 files changed, 32 insertions(+)
>
> diff --git a/include/uapi/linux/prctl.h b/include/uapi/linux/prctl.h
> index a8d0759..1a13c2b 100644
> --- a/include/uapi/linux/prctl.h
> +++ b/include/uapi/linux/prctl.h
> @@ -187,6 +187,13 @@ struct prctl_mm_map {
>
>  #define PR_SET_FP_MODE         45
>  #define PR_GET_FP_MODE         46
> +
> +/* Sets the timerslack for arbitrary threads

Multiline comments should start with a blank:

/*
 * Sets the timerslack...
 * ...
 */

> + * arg2 slack value, 0 means "use default"
> + * arg3 pid of the thread whose timer slack needs to be set
> + */
> +#define PR_SET_TIMERSLACK_PID  47
> +
>  # define PR_FP_MODE_FR         (1 << 0)        /* 64b FP registers */
>  # define PR_FP_MODE_FRE                (1 << 1)        /* 32b compatibility */
>
> diff --git a/kernel/sys.c b/kernel/sys.c
> index 78947de..5189378 100644
> --- a/kernel/sys.c
> +++ b/kernel/sys.c
> @@ -41,6 +41,9 @@
>  #include <linux/syscore_ops.h>
>  #include <linux/version.h>
>  #include <linux/ctype.h>
> +#include <linux/mm.h>
> +#include <linux/mempolicy.h>
> +#include <linux/sched.h>
>
>  #include <linux/compat.h>
>  #include <linux/syscalls.h>
> @@ -2076,6 +2079,7 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
>                 unsigned long, arg4, unsigned long, arg5)
>  {
>         struct task_struct *me = current;
> +       struct task_struct *tsk;
>         unsigned char comm[sizeof(me->comm)];
>         long error;
>
> @@ -2218,6 +2222,27 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
>         case PR_GET_TID_ADDRESS:
>                 error = prctl_get_tid_address(me, (int __user **)arg2);
>                 break;
> +       case PR_SET_TIMERSLACK_PID:

You should reject unused params that are non-zero to allow future
extensions. (Which begs the question, are there other timer-related
things that might need setting too?)

> +               rcu_read_lock();
> +               tsk = find_task_by_vpid((pid_t)arg3);
> +               if (tsk == NULL) {
> +                       rcu_read_unlock();
> +                       return -EINVAL;

Traditionally, unable to look up a pid results in ESRCH.

> +               }
> +               get_task_struct(tsk);
> +               rcu_read_unlock();
> +               if (ptrace_may_access(tsk, PTRACE_MODE_ATTACH)) {
> +                       put_task_struct(tsk);
> +                       return -EPERM;
> +               }
> +               if (arg2 <= 0)
> +                       tsk->timer_slack_ns =
> +                               tsk->default_timer_slack_ns;
> +               else
> +                       tsk->timer_slack_ns = arg2;
> +               put_task_struct(tsk);
> +               error = 0;
> +               break;
>         case PR_SET_CHILD_SUBREAPER:
>                 me->signal->is_child_subreaper = !!arg2;
>                 break;
> --
> 1.9.1
>

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security