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 Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 8CF3FC369D2 for ; Wed, 25 Sep 2024 11:37:15 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 01B8B6B0092; Wed, 25 Sep 2024 07:37:15 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id F0E6C6B0095; Wed, 25 Sep 2024 07:37:14 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id DD4DB6B0096; Wed, 25 Sep 2024 07:37:14 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0012.hostedemail.com [216.40.44.12]) by kanga.kvack.org (Postfix) with ESMTP id C134E6B0092 for ; Wed, 25 Sep 2024 07:37:14 -0400 (EDT) Received: from smtpin20.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay02.hostedemail.com (Postfix) with ESMTP id 854FC120659 for ; Wed, 25 Sep 2024 11:37:14 +0000 (UTC) X-FDA: 82603059588.20.D6DE754 Received: from mail-lf1-f43.google.com (mail-lf1-f43.google.com [209.85.167.43]) by imf05.hostedemail.com (Postfix) with ESMTP id 8DFD110000C for ; Wed, 25 Sep 2024 11:37:12 +0000 (UTC) Authentication-Results: imf05.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b="eyf9/fqo"; spf=pass (imf05.hostedemail.com: domain of ebpqwerty472123@gmail.com designates 209.85.167.43 as permitted sender) smtp.mailfrom=ebpqwerty472123@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1727264112; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=M2c4QkqUSunQnHqhEYmc9PBGDMAL9urYEXHH5gg9oUE=; b=QnK+Ak8uGDci1oXU6nAoCnln71hEza27/XCj2Q9vfUa+O8cA95loC/3jHVHMneE5aaB047 YQgW7u5DKC3pYSxdxmE5cF7SM+S5H7Za8VbHclPWg2D+9YGTIo8R23GPygzDipHUl2GaLo JL6xRILYkiTx5+T5HycSFKa0eykeals= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1727264112; a=rsa-sha256; cv=none; b=kuA+oxSwGdEOCCoZHMQB91i4/nRm9cRKQ6IiA83NUgEsPLv8LCAhICNAvqtTvinUaFGc+T Dd4pXkp2W4veQRjVP3aBvL/uOyx2inRZqdtqBLGrccDQcyqG2oit0XzxshQ4zt0QV2VTQM lPRwN+KMHR7zrWFR+CoqGm8/Y95BXJA= ARC-Authentication-Results: i=1; imf05.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b="eyf9/fqo"; spf=pass (imf05.hostedemail.com: domain of ebpqwerty472123@gmail.com designates 209.85.167.43 as permitted sender) smtp.mailfrom=ebpqwerty472123@gmail.com; dmarc=pass (policy=none) header.from=gmail.com Received: by mail-lf1-f43.google.com with SMTP id 2adb3069b0e04-535be093a43so8251953e87.3 for ; Wed, 25 Sep 2024 04:37:12 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1727264231; x=1727869031; darn=kvack.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=M2c4QkqUSunQnHqhEYmc9PBGDMAL9urYEXHH5gg9oUE=; b=eyf9/fqofv6v3ZC1cBDLjiyLGSVxydEyH0ZcwTocdEO2fpuPLzrJ+QTJx4Fgb+Ngy0 PPP1ZAVLzTr4ItDmc2qrcRLd6gy8+3nXr9yQgH1FCgfs+oHhFLPI7X5PImQAwExTdor/ 9feblsGDW3QFHSkuks5XgwnBhpNWJvARTI5Hhpg9XgC4TeQXyd1Iudu5AhnVxWLhFo2O cjdaNBmth+CiZLFWRk1FSBXKXWmvsGjPlh+Ibts7SP1YDHSv9HN0vimR1VnB1dTuiQq3 sf9ClGqz4P/5ap+nnvmL4nJLCsDrjMCm7r5naOVuyIM5ItAEbyP7gpQ98HA12NrFXBUH 6k2A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1727264231; x=1727869031; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=M2c4QkqUSunQnHqhEYmc9PBGDMAL9urYEXHH5gg9oUE=; b=HJ2TrjznmL/JtPWQ8ZEIJcf2tN2ZdxnBfk4LsfWRRAfEUv5+0whDzBnWmBi5XNClH/ y8M/ICTRRfy5TBzfnTMvQyCqpAp+lK5i+8f9wbpkAQUnH/kyLUkafD8VR9cIf9Sq6LKr CLhp2cFy55CilUOeQvem3M36goWowa+QEWSLtuOsMNxw0cPOeEqDfJXn1j6EeP8oC3Pf qwzP5GpQittF/1EnmS3uU7ZHUICE7QboiQZ8PYbDftPznTz7engCoziAcnm0eYr/7ZI5 V4XdONccmVxvUHfpDuiZL9YhLztHxUTc6lzDCoKRn8tNvuYBe0brNfPpguz7biikzrcD AG0g== X-Forwarded-Encrypted: i=1; AJvYcCWvNUE30fwDUnIK066TujMqLF1fQxLzIE2mqzf+jiz9nM/gmMTvLZjRLgpA6Za5S0JTDpNeqP3yKA==@kvack.org X-Gm-Message-State: AOJu0YyvLQofyIgTzvmSnrXvR6/cuW1Ogedrbt3eMlPF9CAW0lu1FKye W4p61j3h9d5RFruBZEpA7MTS7HyY9eR8GQqCBrM1SEMOlzKauCyRaXdPD9WUmdFpL9FGTC1olwN v5QFyXrtF1X7INF3l2FSRU7P/Ud4= X-Google-Smtp-Source: AGHT+IHYrPcfkliD+Hijl1J7liKGDqIM1On5CGTajvpqJdM+yKtElYu1h+7Y/NbqHMu3+t5CBuXW2otvJhFl3+q/9Mk= X-Received: by 2002:a05:6512:68c:b0:535:6965:be30 with SMTP id 2adb3069b0e04-5387756722fmr1303526e87.50.1727264230308; Wed, 25 Sep 2024 04:37:10 -0700 (PDT) MIME-Version: 1.0 References: <20240925063034.169-1-ebpqwerty472123@gmail.com> <72050879-4546-4bc7-9983-79ad437594d4@lucifer.local> <78a854db-e8ea-475c-950d-2d9faf72f2b4@lucifer.local> In-Reply-To: <78a854db-e8ea-475c-950d-2d9faf72f2b4@lucifer.local> From: Shu Han Date: Wed, 25 Sep 2024 19:36:59 +0800 Message-ID: Subject: Re: [PATCH] mm: move the check of READ_IMPLIES_EXEC out of do_mmap() To: Lorenzo Stoakes Cc: akpm@linux-foundation.org, paul@paul-moore.com, jmorris@namei.org, serge@hallyn.com, Liam.Howlett@oracle.com, vbabka@suse.cz, linux-mm@kvack.org, linux-kernel@vger.kernel.org, linux-security-module@vger.kernel.org, Al Viro Content-Type: text/plain; charset="UTF-8" X-Rspam-User: X-Rspamd-Server: rspam04 X-Rspamd-Queue-Id: 8DFD110000C X-Stat-Signature: uukn78ewuynaah46qxumi1xnumpszqfd X-HE-Tag: 1727264232-448194 X-HE-Meta: U2FsdGVkX18IrNmBVX8u6DI/8q6SKsV4/tnPkKVHmAq5osF1V/MvM4Gr4/ZxCitpHrWOaRA3Jvs+nv1A5/oIQlJaIiFT5jBOB3+WDbYtpLwtbcbOr5cjXdLtBpEf8P+o7109TqMv/PUPnO7T4Ap9Ep31s4ZTyB1B104Lo2URH7wJX6kkDt5UMuU5C0Bes41XBgykfT3oLy/ssa5cd0ThmQA3B5VRRYPxkBoCjXwavWnGNr2U69riXh1ar4V9hjO+icKuCBzFaFPKxDL+FgQ4jUap+S99MRD+V0Y0jXhgb6c1B8pPRgnLSNx0vRKitIsQc51GlonE+Fg26HzZQb1xU8AmGY1PehJiBLtnzkRrhv6FwvDKRr8GQkI7btHnv+pg7fG6PyrgkvRxFFc4mX3ouUclwBYy99jiI4n9SaBRXGoyNqOtdyZBI9yVZ+Ch8h4NVPy7WOlIUhd2crfPKIYGfRy0ZxLAO0DDmn+4aBQNKpDimI1bJlKUhwd+PTah3/hRNPT4lsmfKpqWtS0cUkOM74b3DNXXgg3vTQ4IEbzKObr10cP8WOy/3G+n30niuBtJxIHx8lilt3AR1sLnwrXkUwa1fCIXA9mQlN4qw9BrJR5RWSxbivG0OM9yYWYu//XZZQtapE5EWgplQAKM/q50lMGgPXPLDa8cyQ7jV7vZoOdPw5pz2K8SuQ4ZTiNxtbMIj4DqrpeALwmfYo583MX+U6EdXIvuNn1HyM/cP5onba24N7sjY89G6hyhGjBXaXKO2yoH1BgSxUphotUPMWnrkUcb/YblT3fj0AG1s0BDofMNYhrijg5xeayKp1veyNhuKQcArmtWqx09EjU9X2l64zfXzixPqKx9T47nOLXW/Mg18ckAMHpaKdbdBtnbqj05mA2w5FvQN5GGIp0FG4QwRZSm//kj6PNyhuC5tsTJ+d0EBrSHLfPJI4mjNE5johBZUlw565RPDkaIfD0Xy5t iTnn0Oiz YEWuaLtw0qgT4zhwNHOYaYUQwv8SI7q9jlrFnxPioZIWzpQymkMJmWIsQcQKvDmoTN0stl0Ti3k9cia22CHoRmssTXh8S/mfGLkcJRE3nqJckmEhnCEIG8CsoyuHzU8XW5hf8thDj/3B+97LSgS6J0TFbw7T7KS2eQXog14wuaxArF4+4mDP4JMaBcgUjD6pZV2Z1bCeY2VwQwPvd4h8T3G2oebttP5lK9YnbF/a8XxywcpRm7kmOHdO4ZoAyRg6BEVVPEXmOjdHnrM8/UCYsXIeUHztyHjiLl/Lwt/jsVsfn+besyGW/IcUaLU+8UCCHwGLdnZFsOdV0sxd0+VCKulLt791j8YPMHm0pPl6xJmVbhF5sn2v8MKAG5A== 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: List-Subscribe: List-Unsubscribe: > No need to be sorry! :) Sorry if I sound harsh here - it's more a case of > trying to be as clear as I can be as that is the best approach for everyone > I think. > This code is sensitive, so we have to super careful! Thanks a lot! :) > I would disagree it's down to taste, I noted on the move the check to > do_mmap() series a number of issues and concerns, to me that seems > unworkable in it's current form, the locking thing is fatal for instance. > What you link to there seems to be neither approach (I didn't read your > second series though as that needs an RFC tag)? I mean I think perhaps what > you are doing there is the best _first step_ - simply add the checks in > each of the callsites that you feel are missing them. > This is the least controversial way and then allows maintainers of the > callers to assess whether they intended for that. > If then you end up wtih _all_ callers doing this check, we can take another > look at possibly bringing it into do_mmap() but we would absolutely have to > ensure it was done correctly, however. > 1. (If you haven't already) Submit a series that adds patches to add checks > at call sites that don't already check. > 2. If these are accepted at _all_ callsites, revisit the do_mmap() change, > properly accounting for locks (I can help with this). In fact, "mm: move the check of READ_IMPLIES_EXEC out of do_mmap()" does not have the locking issue. These two patches are quite different. This is also the modification I recommended, while another modification was suggested by LSM maintainers(Perhaps I need to add suggested-by? But that was mentioned in a non-public security mailing list, and I'm not sure if it's appropriate.). The __core__ problem is "no LSM hook" + "have logic about READ_IMPLIES_EXEC". Removing one of them is OK. The "mm: move security_file_mmap() back into do_mmap()" fixes this by adding a LSM hook. The requirement to call LSM hooks comes from the LSM modules, _not these call sites_. The issue for locks also comes from the specific implementation of LSM modules. So I send patches to LSM maintainers at the same time. The "mm: move the check of READ_IMPLIES_EXEC out of do_mmap()" fixes this by removing the logic about READ_IMPLIES_EXEC that is not needed. So no locking issues there(no changes to LSM). This will result in some minor behavioral changes for call sites mentioned in the patch. Unfortunately, due to this logic being placed in a single function do_mmap now, it is impossible to confirm it through patches one by one before change the mm module. Fortunately, these changes should clearly be fine, and here are the reasons(more specific versions): fs/aio.c, mm/util.c, ipc/shm.c: no changes arch/x86/kernel/shstk.c: Shadow Stack is stack only store return addresses, adding execute permission to shadow stack is never required. mm/mmap.c: in the history, remap_file_pages won't care about the READ_IMPLIES_EXEC. this side effect is introduced in the emulated version, after the deprecated mark exists. The patch only removes the side effects introduced. And this(mm) is the module. :) BTW, The link is the _first step_ in required(if the check is missing in that call site, there will be a bug) call sites, which has been done. > I do feel we need to better document these functions, so I will add > comments. I see you did so as part of your other series, but think maybe we > need to expand this and possibly rename both and add some asserts... it's > on the todo list! Perhaps adding sufficient comments is also a completely appropriate method as another alternative. Thanks for your kind review!