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=-0.7 required=3.0 tests=FREEMAIL_FORGED_FROMDOMAIN, FREEMAIL_FROM,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS 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 16EAFC10F25 for ; Wed, 11 Mar 2020 06:12:02 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id CD19120873 for ; Wed, 11 Mar 2020 06:12:01 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org CD19120873 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=hotmail.de Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 636FB6B0003; Wed, 11 Mar 2020 02:12:01 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 5E75B6B0006; Wed, 11 Mar 2020 02:12:01 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 4AF736B0007; Wed, 11 Mar 2020 02:12:01 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0243.hostedemail.com [216.40.44.243]) by kanga.kvack.org (Postfix) with ESMTP id 346A56B0003 for ; Wed, 11 Mar 2020 02:12:01 -0400 (EDT) Received: from smtpin23.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay05.hostedemail.com (Postfix) with ESMTP id AA68C181AC9C6 for ; Wed, 11 Mar 2020 06:12:00 +0000 (UTC) X-FDA: 76582060800.23.wheel53_1173602ab482a X-HE-Tag: wheel53_1173602ab482a X-Filterd-Recvd-Size: 10534 Received: from EUR05-VI1-obe.outbound.protection.outlook.com (mail-vi1eur05olkn2025.outbound.protection.outlook.com [40.92.90.25]) by imf18.hostedemail.com (Postfix) with ESMTP for ; Wed, 11 Mar 2020 06:11:59 +0000 (UTC) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=fyeVOP+zgVlCJQbCBDx+B116RNep18NgrU9htCv7ST+MJOSVg8cwp9ZIB3hkP2NBI7ShqF90TGI5jaRfds3rKmY24FSo69jKcQZFBtREIw5PxtTjw/D+Qdhej0JG1O52zoQYNz0mvnPOJslAh6CU8P3k6tGkOMTTaf6e0o1kCgg5Y/ASDaZShUbzwVxRy2I9Hisz/yPXLhmw6kS1ihn7IgZRiH7zriqxhf4lVjbBO5n3Bhhsc4sXPLQCO/maN7MFr2tkrdMaAKAE7eZbbv/2nzOeP/NIpyoweNq2z2za/g+pHZBFycXbWKKhRFYRZDFkIzLW3ft6G+BmCvRqJeH9Bw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=CG2lVs5buT64yAQDQfrF4BRc4UVsw/DyRi3By56IKCM=; b=dXG89GsJ0Y9X6eSav4NaS4dlDwH33LKl14VfSg3sCpJfO7dhjQydJOWPkBO2yxwT7qkPT3ebDzhJ0UcAaWiTEGNLsUqTu5S5Mli726h4G0QxhIe02F+bdNVoVPraCmQ7W5Ym0HIsuQYzp3ZkJo2vNlsrTB6ZavL8lhiV22dmswkYLMs3kj6kmvN25EMgfxZwB2SLYsjJFmHPI4lc9G9CAsg3gROjWpogx9N0G8ylCv0CgG1MIyjnQVwl7NZV/+sIambLiKGPsWpg8mW5VBJKoW/SNg0VEf7YCzkiiz91vLiPnsTOuEXzSHgcdVcn4/Krr+2LHg/pVmSe2TwiYnLqyA== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=none; dmarc=none; dkim=none; arc=none Received: from DB8EUR05FT054.eop-eur05.prod.protection.outlook.com (2a01:111:e400:fc0f::38) by DB8EUR05HT078.eop-eur05.prod.protection.outlook.com (2a01:111:e400:fc0f::64) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.2793.11; Wed, 11 Mar 2020 06:11:57 +0000 Received: from AM6PR03MB5170.eurprd03.prod.outlook.com (10.233.238.51) by DB8EUR05FT054.mail.protection.outlook.com (10.233.238.111) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.2814.13 via Frontend Transport; Wed, 11 Mar 2020 06:11:57 +0000 Received: from AM6PR03MB5170.eurprd03.prod.outlook.com ([fe80::1956:d274:cab3:b4dd]) by AM6PR03MB5170.eurprd03.prod.outlook.com ([fe80::1956:d274:cab3:b4dd%6]) with mapi id 15.20.2793.013; Wed, 11 Mar 2020 06:11:57 +0000 From: Bernd Edlinger To: "jannh@google.com" , "Eric W. Biederman" CC: Christian Brauner , Kees Cook , Jonathan Corbet , Alexander Viro , Andrew Morton , "adobriyan@gmail.com" , Thomas Gleixner , Oleg Nesterov , Frederic Weisbecker , "avagin@gmail.com" , Ingo Molnar , "Peter Zijlstra (Intel)" , "duyuyang@gmail.com" , David Hildenbrand , Sebastian Andrzej Siewior , Anshuman Khandual , David Howells , James Morris , "gregkh@linuxfoundation.org" , Shakeel Butt , Jason Gunthorpe , "christian@kellner.me" , Andrea Arcangeli , Aleksa Sarai , "Dmitry V. Levin" , "linux-doc@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "linux-fsdevel@vger.kernel.org" , "linux-mm@kvack.org" , "stable@vger.kernel.org" , "linux-api@vger.kernel.org" , Arnd Bergmann , "sargun@sargun.me" Subject: Re: [PATCH] pidfd: Stop taking cred_guard_mutex Thread-Topic: [PATCH] pidfd: Stop taking cred_guard_mutex Thread-Index: AQHV9w1dmrKXzF1nwkisARGyb9pH3qhCMseAgAADrsOAAAiGgIAAAvGAgAADRYCAABelgA== Date: Wed, 11 Mar 2020 06:11:57 +0000 Message-ID: <5a8b2794-b498-af33-1327-ff2861cff83f@hotmail.de> References: <87r1y8dqqz.fsf@x220.int.ebiederm.org> <87tv32cxmf.fsf_-_@x220.int.ebiederm.org> <87v9ne5y4y.fsf_-_@x220.int.ebiederm.org> <87eeu25y14.fsf_-_@x220.int.ebiederm.org> <20200309195909.h2lv5uawce5wgryx@wittgenstein> <877dztz415.fsf@x220.int.ebiederm.org> <20200309201729.yk5sd26v4bz4gtou@wittgenstein> <87k13txnig.fsf@x220.int.ebiederm.org> <20200310085540.pztaty2mj62xt2nm@wittgenstein> <87wo7svy96.fsf_-_@x220.int.ebiederm.org> <87k13sui1p.fsf@x220.int.ebiederm.org> In-Reply-To: Accept-Language: en-US, en-GB, de-DE Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ms-exchange-imapappendstamp: AM6PR03MB5170.eurprd03.prod.outlook.com (15.20.2793.013) x-incomingtopheadermarker: OriginalChecksum:E45BB2544C586090C38E17B315A565CEFA66D1F593396D52DAAE1552D59B5643;UpperCasedChecksum:2881F61324411814D5BD39D5E91F8B9BFFC3FB3659E7CFB2B10D286FB4DA4128;SizeAsReceived:9446;Count:47 x-ms-exchange-messagesentrepresentingtype: 1 x-tmn: [eh/3gyuvAApi+J5fKmcZx0IZizg7sx9B] x-ms-publictraffictype: Email x-incomingheadercount: 47 x-eopattributedmessage: 0 x-ms-office365-filtering-correlation-id: 6ad72773-1414-481b-1f48-08d7c5831a75 x-ms-traffictypediagnostic: DB8EUR05HT078: x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: NfTADmGC1X+ro3AgRFa5jiXEPF/PZbjhVW3gyBcdaIVOtPvcqdXzCg8gJ+m3xR2dnLJ0fSskghFTCcGwt0LlrCCjGvieDSKzEKG47Ka2YwgLypxbw3Axkj+pRYJZiYDZYuJu1BEi0b9Mc13mUn44FbCSixv41dxO8slYF0XYE6i42RvEIW7WNpIZc+lb5zSY x-ms-exchange-antispam-messagedata: RlAc0obM4Rtx1hnUd47wm4nKK7N3WXtMU16xqSFI+VJJzdrEQjVPzeNO54iVuvlsGTyCas3uLCavV9d+1NnwqwRScf2SzP3TnPtLjqBrOCyk3Tt5kEeEHjNreDrWZtsQia6cDyoVRCs0DCr2cC2M9w== x-ms-exchange-transport-forked: True Content-Type: text/plain; charset="iso-8859-1" Content-ID: Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-OriginatorOrg: outlook.com X-MS-Exchange-CrossTenant-RMS-PersistedConsumerOrg: 00000000-0000-0000-0000-000000000000 X-MS-Exchange-CrossTenant-Network-Message-Id: 6ad72773-1414-481b-1f48-08d7c5831a75 X-MS-Exchange-CrossTenant-rms-persistedconsumerorg: 00000000-0000-0000-0000-000000000000 X-MS-Exchange-CrossTenant-originalarrivaltime: 11 Mar 2020 06:11:57.0825 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Internet X-MS-Exchange-CrossTenant-id: 84df9e7f-e9f6-40af-b435-aaaaaaaaaaaa X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB8EUR05HT078 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On 3/10/20 9:22 PM, Bernd Edlinger wrote:=0A= > On 3/10/20 9:10 PM, Jann Horn wrote:=0A= >> On Tue, Mar 10, 2020 at 9:00 PM Jann Horn wrote:=0A= >>> On Tue, Mar 10, 2020 at 8:29 PM Eric W. Biederman wrote:=0A= >>>> Jann Horn writes:=0A= >>>>> On Tue, Mar 10, 2020 at 7:54 PM Eric W. Biederman wrote:=0A= >>>>>> During exec some file descriptors are closed and the files struct is= =0A= >>>>>> unshared. But all of that can happen at other times and it has the= =0A= >>>>>> same protections during exec as at ordinary times. So stop taking t= he=0A= >>>>>> cred_guard_mutex as it is useless.=0A= >>>>>>=0A= >>>>>> Furthermore he cred_guard_mutex is a bad idea because it is deadlock= =0A= >>>>>> prone, as it is held in serveral while waiting possibly indefinitely= =0A= >>>>>> for userspace to do something.=0A= >> [...]=0A= >>>>> If you make this change, then if this races with execution of a setui= d=0A= >>>>> program that afterwards e.g. opens a unix domain socket, an attacker= =0A= >>>>> will be able to steal that socket and inject messages into=0A= >>>>> communication with things like DBus. procfs currently has the same=0A= >>>>> race, and that still needs to be fixed, but at least procfs doesn't= =0A= >>>>> let you open things like sockets because they don't have a working=0A= >>>>> ->open handler, and it enforces the normal permission check for=0A= >>>>> opening files.=0A= >>>>=0A= >>>> It isn't only exec that can change credentials. Do we need a lock for= =0A= >>>> changing credentials?=0A= >> [...]=0A= >>>> If we need a lock around credential change let's design and build that= .=0A= >>>> Having a mismatch between what a lock is designed to do, and what=0A= >>>> people use it for can only result in other bugs as people get confused= .=0A= >>>=0A= >>> Hmm... what benefits do we get from making it a separate lock? I guess= =0A= >>> it would allow us to make it a per-task lock instead of a=0A= >>> signal_struct-wide one? That might be helpful...=0A= >>=0A= >> But actually, isn't the core purpose of the cred_guard_mutex to guard=0A= >> against concurrent credential changes anyway? That's what almost=0A= >> everyone uses it for, and it's in the name...=0A= >>=0A= > =0A= > The main reason d'etre of exec_update_mutex is to get a consitent=0A= > view of task->mm and task credentials.=0A= > > The reason why you want the cred_guard_mutex, is that some action=0A= > is changing the resulting credentials that the execve is about=0A= > to install, and that is the data flow in the opposite direction.=0A= > =0A= =0A= So in other words, you need the exec_update_mutex when you=0A= access another thread's credentials and possibly the mmap at the=0A= same time.=0A= =0A= You need the cred_guard_mutex when you *change* the credentials=0A= of another thread. (Where you cannot be sure that the other thread=0A= just started to execve something)=0A= =0A= You need no mutex at all when you are just accessing or=0A= even changing the credentials of the current thread. (If another=0A= thread is doing execve, your task will be killed, and wether=0A= or not the credentials were changed does not matter any more)=0A= =0A= > =0A= > Bernd.=0A= > =0A=