From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.7 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE, SPF_PASS,URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 964BFC00A89 for ; Fri, 30 Oct 2020 16:06:46 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 429862083B for ; Fri, 30 Oct 2020 16:06:46 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726318AbgJ3QGp (ORCPT ); Fri, 30 Oct 2020 12:06:45 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:50198 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725355AbgJ3QGo (ORCPT ); Fri, 30 Oct 2020 12:06:44 -0400 Received: from smtp-42aa.mail.infomaniak.ch (smtp-42aa.mail.infomaniak.ch [IPv6:2001:1600:4:17::42aa]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 80628C0613CF for ; Fri, 30 Oct 2020 09:06:44 -0700 (PDT) Received: from smtp-2-0001.mail.infomaniak.ch (unknown [10.5.36.108]) by smtp-3-3000.mail.infomaniak.ch (Postfix) with ESMTPS id 4CN6cx2KK4zllGw2; Fri, 30 Oct 2020 17:06:41 +0100 (CET) Received: from ns3096276.ip-94-23-54.eu (unknown [94.23.54.103]) by smtp-2-0001.mail.infomaniak.ch (Postfix) with ESMTPA id 4CN6cw2Rr9zlh8TQ; Fri, 30 Oct 2020 17:06:40 +0100 (CET) Subject: Re: [PATCH v1 1/2] ptrace: Set PF_SUPERPRIV when checking capability To: Jann Horn Cc: Christian Brauner , Kees Cook , Oleg Nesterov , Eric Paris , James Morris , "Serge E . Hallyn" , Tyler Hicks , Will Drewry , kernel list , stable , =?UTF-8?Q?Micka=c3=abl_Sala=c3=bcn?= References: <20201030123849.770769-1-mic@digikod.net> <20201030123849.770769-2-mic@digikod.net> From: =?UTF-8?Q?Micka=c3=abl_Sala=c3=bcn?= Message-ID: <94a86084-5aab-4a2c-e654-f55130190c1a@digikod.net> Date: Fri, 30 Oct 2020 17:06:39 +0100 User-Agent: MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 30/10/2020 16:47, Jann Horn wrote: > On Fri, Oct 30, 2020 at 1:39 PM Mickaël Salaün 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 >> Cc: Eric Paris >> Cc: Jann Horn >> Cc: Kees Cook >> Cc: Oleg Nesterov >> Cc: Serge E. Hallyn >> Cc: Tyler Hicks >> 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 > > 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 > > 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. > >> struct mm_struct *mm; >> kuid_t caller_uid; >> kgid_t caller_gid;