Re: [PATCH 1/2 v4] proc: Relax /proc/<tid>/timerslack_ns capability requirements

[John Stultz]

On Thu, Jul 21, 2016 at 1:24 PM, John Stultz <john.stultz@xxxxxxxxxx> 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