From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1755517AbcHBAS1 (ORCPT <rfc822;w@1wt.eu>);
	Mon, 1 Aug 2016 20:18:27 -0400
Received: from mail-oi0-f52.google.com ([209.85.218.52]:34373 "EHLO
	mail-oi0-f52.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1755393AbcHBASU (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Mon, 1 Aug 2016 20:18:20 -0400
MIME-Version: 1.0
In-Reply-To: <1469132667-17377-1-git-send-email-john.stultz@linaro.org>
References: <1469132667-17377-1-git-send-email-john.stultz@linaro.org>
From: John Stultz <john.stultz@linaro.org>
Date: Mon, 1 Aug 2016 17:18:18 -0700
Message-ID: <CALAqxLXBRruXfAc3kA3RgWeq-wntgUAOdOnBR9RULW8uipAXCQ@mail.gmail.com>
Subject: Re: [PATCH 1/2 v4] proc: Relax /proc/<tid>/timerslack_ns capability requirements
To: lkml <linux-kernel@vger.kernel.org>
Cc: John Stultz <john.stultz@linaro.org>, Kees Cook <keescook@chromium.org>,
        "Serge E. Hallyn" <serge@hallyn.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Arjan van de Ven <arjan@linux.intel.com>,
        Oren Laadan <orenl@cellrox.com>, Ruchi Kandoi <kandoiruchi@google.com>,
        Rom Lemarchand <romlem@android.com>, Todd Kjos <tkjos@google.com>,
        Colin Cross <ccross@android.com>, Nick Kralevich <nnk@google.com>,
        Dmitry Shmidt <dimitrysh@google.com>, Elliott Hughes <enh@google.com>,
        Android Kernel Team <kernel-team@android.com>
Content-Type: text/plain; charset=UTF-8
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Thu, Jul 21, 2016 at 1:24 PM, John Stultz <john.stultz@linaro.org> wrote:
> When an interface to allow a task to change another tasks
> timerslack was first proposed, it was suggested that something
> greater then CAP_SYS_NICE would be needed, as a task could be
> delayed further then what normally could be done with nice
> adjustments.
>
> So CAP_SYS_PTRACE was adopted instead for what became the
> /proc/<tid>/timerslack_ns interface. However, for Android (where
> this feature originates), giving the system_server
> CAP_SYS_PTRACE would allow it to observe and modify all tasks
> memory. This is considered too high a privilege level for only
> needing to change the timerslack.
>
> After some discussion, it was realized that a CAP_SYS_NICE
> process can set a task as SCHED_FIFO, so they could fork some
> spinning processes and set them all SCHED_FIFO 99, in effect
> delaying all other tasks for an infinite amount of time.
>
> So as a CAP_SYS_NICE task can already cause trouble for other
> tasks, using it as a required capability for accessing and
> modifying /proc/<tid>/timerslack_ns seems sufficient.
>
> Thus, this patch loosens the capability requirements to
> CAP_SYS_NICE and removes CAP_SYS_PTRACE, simplifying some
> of the code flow as well.
>
> This is technically an ABI change, but as the feature just
> landed in 4.6, I suspect no one is yet using it.


Ah, drat.

I just realized that I missed changing from ptrace_may_access() to
capable(CAP_SYS_NICE) means that a task cannot set its *own*
timerslack value as is possible via the PR_SET_TIMERSLACK interface.
Thus this patch, in trying to loosen the required privileges, actually
adds a unnecessary restriction.

I'm working on a patch that adds a check if  p == current and allows
the modification.

Andrew: I know you queued this in -mm late, so I didn't think you'd
send it to Linus yet, but in case you were considering it, please
wait.

thanks
-john