On Wed, Feb 17, 2016 at 2:45 PM, Kees Cook <keesc...@chromium.org> wrote: > On Wed, Feb 17, 2016 at 2:29 PM, John Stultz <john.stu...@linaro.org> wrote: >> On Wed, Feb 17, 2016 at 12:18 PM, Andrew Morton >> <a...@linux-foundation.org> wrote: >>> On Wed, 17 Feb 2016 12:09:08 -0800 Kees Cook <keesc...@chromium.org> wrote: >>>> On Wed, Feb 17, 2016 at 11:35 AM, Andrew Morton >>>> > The procfs file's permissions are 0644, yes? So a process's >>>> > timer_slack is world-readable? hm. >>>> >>>> This should be 600, IMO. >>> >>> Sounds safer. >> >> So I've gone ahead and addressed this and the other feedback you had. >> But this bit made me realize that I may have missed a key aspect to >> the interface that Android needs. >> >> In particular, the whole point here is to allow a controlling task to >> modify other tasks' timerslack to limit background tasks' power usage >> (and to modify them back to normal when the background tasks become >> foreground tasks). Note that on android different tasks run as >> different users. >> >> Currently, the controlling process has minimally elevated privileges >> (CAP_SYS_NICE). The initial review suggested those privileges should >> be higher (PTRACE_MODE_ATTACH), which I've implemented. However, I'm >> realizing that by moving to the proc interface, the filesystem >> permissions here put yet another barrier in the way. >> >> While the 600 permissions makes initial sense, it does limit these >> controlling tasks with extra privileges (though not root) from >> modifying the timerslack, since they cannot open the file to begin >> with. >> >> So.... Does world writable (plus the PTRACE_MODE_ATTACH_FSCREDS check) >> make more sense here? Or is there a better way for a system to tweak >> the default permissions for procfs entries? (And if so, does that >> render the PTRACE_MODE_ATTACH... check unnecessary?). >> >> Apologies. I'm fighting a head-cold, so I'm not feeling particularly sharp >> here. > > Is timerslack sensitive at all? You could add the ptrace test to the > _show function too, maybe. Then 0666 would solve the open issue > without leaking the timerslack.
I don't see how timerslack would be sensitive, but probably many mistakes start out that way, so not being cavalier about it seems wise. :) Ok. Sounds like you and Andrew are on the same page wrt 666 + PTRACE_MODE_ATTACH, and that seems like it would be workable. I'll get that implemented here shortly. Thanks so much again for the feedback! Really appreciate it! -john
