On Fri, Oct 30, 2020 at 5:06 PM Mickaël Salaün mic@digikod.net wrote:
On 30/10/2020 16:47, Jann Horn wrote:
On Fri, Oct 30, 2020 at 1:39 PM Mickaël Salaün mic@digikod.net wrote:
Commit 69f594a38967 ("ptrace: do not audit capability check when outputing /proc/pid/stat") replaced the use of ns_capable() with has_ns_capability{,_noaudit}() which doesn't set PF_SUPERPRIV.
Commit 6b3ad6649a4c ("ptrace: reintroduce usage of subjective credentials in ptrace_has_cap()") replaced has_ns_capability{,_noaudit}() with security_capable(), which doesn't set PF_SUPERPRIV neither.
Since commit 98f368e9e263 ("kernel: Add noaudit variant of ns_capable()"), a new ns_capable_noaudit() helper is available. Let's use it!
As a result, the signature of ptrace_has_cap() is restored to its original one.
Cc: Christian Brauner christian.brauner@ubuntu.com Cc: Eric Paris eparis@redhat.com Cc: Jann Horn jannh@google.com Cc: Kees Cook keescook@chromium.org Cc: Oleg Nesterov oleg@redhat.com Cc: Serge E. Hallyn serge@hallyn.com Cc: Tyler Hicks tyhicks@linux.microsoft.com Cc: stable@vger.kernel.org Fixes: 6b3ad6649a4c ("ptrace: reintroduce usage of subjective credentials in ptrace_has_cap()") Fixes: 69f594a38967 ("ptrace: do not audit capability check when outputing /proc/pid/stat") Signed-off-by: Mickaël Salaün mic@linux.microsoft.com
Yeah... I guess this makes sense. (We'd have to undo or change it if we ever end up needing to use a different set of credentials, e.g. from ->f_cred, but I guess that's really something we should avoid anyway.)
Reviewed-by: Jann Horn jannh@google.com
with one nit:
[...]
/* Returns 0 on success, -errno on denial. */ static int __ptrace_may_access(struct task_struct *task, unsigned int mode) {
const struct cred *cred = current_cred(), *tcred;
const struct cred *const cred = current_cred(), *tcred;This is an unrelated change, and almost no kernel code marks local pointer variables as "const". I would drop this change from the patch.
This give guarantee that the cred variable will not be used for something else than current_cred(), which kinda prove that this patch doesn't change the behavior of __ptrace_may_access() by not using cred in ptrace_has_cap(). It doesn't hurt and I think it could be useful to spot issues when backporting.
And it might require an extra fixup while backporting because the next line is different and that might cause the patch to not apply.