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 18881C2BD09 for ; Mon, 1 Jul 2024 21:32:12 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 8CD956B0092; Mon, 1 Jul 2024 17:32:12 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 87C186B0093; Mon, 1 Jul 2024 17:32:12 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 76BF06B0095; Mon, 1 Jul 2024 17:32:12 -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 580CC6B0092 for ; Mon, 1 Jul 2024 17:32:12 -0400 (EDT) Received: from smtpin14.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay02.hostedemail.com (Postfix) with ESMTP id 11053121E46 for ; Mon, 1 Jul 2024 21:32:12 +0000 (UTC) X-FDA: 82292482104.14.7590F79 Received: from mail-qt1-f170.google.com (mail-qt1-f170.google.com [209.85.160.170]) by imf23.hostedemail.com (Postfix) with ESMTP id 3A03B140011 for ; Mon, 1 Jul 2024 21:32:10 +0000 (UTC) Authentication-Results: imf23.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=s8VVpxmV; spf=pass (imf23.hostedemail.com: domain of jeffxu@google.com designates 209.85.160.170 as permitted sender) smtp.mailfrom=jeffxu@google.com; dmarc=pass (policy=reject) header.from=google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1719869508; 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:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=J67WCzSlHWFlY+A0bs0l9CZDhvuEPnOb2uXCOASkodI=; b=27edpln/HTxRZn/hUy14jQ/iMU3mWaSmCo6qNNPvW3F+Ez1IZWqpeO0GZdHXW8C47Uu+tb tMUTCP80OGfnG2cLPgQ5RLqxiglZjnGTjC3CLXCIfymf4fw8MjTVI2bcUPHaWf3MwPGFMU pFE5eUVbUhQ0tN36RppqBXktPC/zyq4= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1719869508; a=rsa-sha256; cv=none; b=L50bOZyftR33FsiBsBQ2Tgk/lmtXyv3SldATFQkqVkuC/u93+uKcdy/4lL3JCqg0zhQ/Bg K8P85YpGjSJQUsF7ZhjbDfPlpDz3TNSH+Qgooans6tL6qtUV2bH/WQGFTtjUNfPSkfcNdL M59ST9M10lh3UcpTGWdxDjs6sKeIruM= ARC-Authentication-Results: i=1; imf23.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=s8VVpxmV; spf=pass (imf23.hostedemail.com: domain of jeffxu@google.com designates 209.85.160.170 as permitted sender) smtp.mailfrom=jeffxu@google.com; dmarc=pass (policy=reject) header.from=google.com Received: by mail-qt1-f170.google.com with SMTP id d75a77b69052e-44664ad946eso496011cf.1 for ; Mon, 01 Jul 2024 14:32:09 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1719869529; x=1720474329; darn=kvack.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=J67WCzSlHWFlY+A0bs0l9CZDhvuEPnOb2uXCOASkodI=; b=s8VVpxmViWDTI1A/h37M65n0nnsVHGQAj5zsddDU7Tpb86BjCsp4rJnoAcYOBrTL/J uDe4CtOOvubUzzrxPElgHpj66WNCFtIlReWZXzFygnn2I5NgqNumdeAi4FAgUQ1LeuMg 0Jj3lsA0WTha98OQJVB9MgdRWk0iF7Q3CZpZlLTmEoSr1tuZJJ0dq2xCNVfIvXJDuNr/ nTROPp12u35Iim4PxYjOBuDYaXI932+zQlDc8vnNzks2SZYHAi3oXZFETqaGuIibGNYF Ok9VBp/9frsxZSnArOul1p5UfwYnT9C2TK3M76iHGFYAEddHkeqSoQ+/0KlqvkUZLAmK 5rPg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1719869529; x=1720474329; h=content-transfer-encoding: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=J67WCzSlHWFlY+A0bs0l9CZDhvuEPnOb2uXCOASkodI=; b=FUJc7JvnF/8nt8eVuAfQmEmT7KcP3ClCcDl9NS6CDDI9zs9eYv7/iiN3wca5UfgEer w6ssdnlLJZzNLYpMlgya/RvOHv+wYaVW78iU7chML2fV/m8Jbkg3Rt6rX3KnFgyhn+vG MmSWsnyUYYBPJ3fwtahkUtf7aiGwHNSJcdYVyTW9U4LiW3MXvptPbqCW67mhbN43JV+s GWoEKWQ+CNEveEzp9z4y/jt8jZyPFuwHdWGOviWtkChat66M7V5YpggRlH4daP/6QhLs d+Gx8xVjHXo+BVwvZCh3JogiUx8v3aKXpjtSpyHYKrC0l5KEvZ4OirNcSjFDSKSCAfSd i19g== X-Gm-Message-State: AOJu0Yw35XSh+Ae+Ftx5K4RMkZjlnRsx9jhS51aGqJcu3rvDf1aQq5vI SIYrDgwioBebBIR43SuPEQq3izFeV5Wj7O71sF3OMl8pFkLLzIDU4zWCJ/Elcxncn8Dk40k7MIb xqfjhRiar3MXAkhpwo2XViT/wClv2ab7WDu9x X-Google-Smtp-Source: AGHT+IEH0ZITXe2UCv0cCHd5aHLEsR7t11euWOBqlLR/sVZ1A0ADj29+NfF1nQB1AvlbSYyoohooEFV9znnGPJZS9rQ= X-Received: by 2002:ac8:4d51:0:b0:444:ccfc:fd with SMTP id d75a77b69052e-4465def170emr5251491cf.3.1719869529008; Mon, 01 Jul 2024 14:32:09 -0700 (PDT) MIME-Version: 1.0 References: <20240630184912.37335-1-pobrn@protonmail.com> In-Reply-To: <20240630184912.37335-1-pobrn@protonmail.com> From: Jeff Xu Date: Mon, 1 Jul 2024 14:31:28 -0700 Message-ID: Subject: Re: [PATCH v4] memfd: `MFD_NOEXEC_SEAL` should not imply `MFD_ALLOW_SEALING` To: =?UTF-8?B?QmFybmFiw6FzIFDFkWN6ZQ==?= Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org, linux-kselftest@vger.kernel.org, akpm@linux-foundation.org, cyphar@cyphar.com, david@readahead.eu, dmitry.torokhov@gmail.com, dverkamp@chromium.org, hughd@google.com, jorgelo@chromium.org, keescook@chromium.org, skhan@linuxfoundation.org, stable@vger.kernel.org, Jeff Xu Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Stat-Signature: d1nxj3c7uuj4tfuhcjw5ekud1kxkt3rp X-Rspamd-Queue-Id: 3A03B140011 X-Rspam-User: X-Rspamd-Server: rspam08 X-HE-Tag: 1719869529-754562 X-HE-Meta: U2FsdGVkX1/cW5lDQfckbcz1+pPdBwev7e0TXIKnaiuzUV7CnEkmMKt91ngI3Sw6ACTeE/u1ePjVD1nSmPBKveCkGbedSfLxoneGvfw98w/E8lPx/gUpbiaFgkk2Px+ByuXf1v/wmCX/Zfv+0ygqAw7m7PhxtPXqQGl8GIFvV6pIn+rMDy0wzOdS0xJo42RPyx79YRFf2vFPpvFbbX6NXLbSusZ00SsAFeM4tb7Qc8inUMFXEOvHRqIZClegkqwYLNUFXHyeXF3BPeuvu15YxB7JqxhlQWChnVs7geToq3a9R1sKHdlzLyEohAIopQgkIOYtcH+xyIt97va8irQ3cjhHKCWl86pXLsRSXBPv/EBoJrYQljS4nQl84JM2pbHnNR3BDYkaMQ2vWHRhURCFnYJdRhIyw2aZayIwxJx8BKP1mYCI0oyw33wcUqKGNjWEPnYLvF0SV72aBceDbMv+Tc3p3oJkPIImxlu8LOpF4+D3uUkZi/dRnLjAx3huf3D6qz+8LlphNExjaKf1y2SzbHR+NEtANhBPPA+jNwXLgXVJUi7HEiJO1zIAr4021ZHuapyR+Gb1udbO2kqNoQt7zvzrW6Ct1Fpc8CGEiKQ/myCO0evA392aYz+ry0kTwnXOpXuF9Ady/1pD2ymGeIvRQNRKJLRNpRsxI6zORXOH/5oWMlRK+935RqwthuejdJwCHxGS+s0iFOVayzVzr2UMHoXop5gBDOWAFc1JcTqydwyFxhlr0pYt+mg0XZJaIJrpQ4jj3vHlfiT6lFhWtxJ3LnPli8UoumExwo6t1kzZcwKlNZRLkHpZnNVIT2kGdiIelbFIvA1GNDqc3aOJPGykzhYG7bwBRtkwgEV/CunSz6bYMu77EGmHulrycqGPwdAXFo/wuJ9zZY3EnvkLUlqWmne3E6ljClIfdckbfkcfokwwLkLfFfJ7D+v+4ozo32tAjTd8jlCDX3kIwEczpYb e1gi1ovq w+xSnF4FKb5OSgLILUT/EyC/v/Ub5Zner9q6dmiwqbC1D6/iRqpck5Q2XJhl9On7O1O42k2ywgNngWECao/Z6sUC6tO9r+tudLDtKqb+0uU3gXUjLkrD/Korqs7pshkWfVwu4nH+8fhz84GkHjJuvRe6lza7zJOyszLtvq7tHUwEsZuTosSRQ2ujH+PGmLelTuQjU16+/djG/TcOxCfztJh62dc82B+K9ZADW+JjOCcwL3634jhvpcGG+VdTM2B7YDcOR+xh1ivCDuj4ln06nxQAGY3lgpnyOlEpbD0c4kLC6jtTovh42zEuV7tarYXaHXcH8KdQBRBD4nNUm6NCbR2LmZYhnrrEqooocgCBQYXrlgmbSwCPEnFKlJwA8hSM2xQsc1tm4H9n4bA4o2IxJ8zetfFA3rzTB//HFE0lyM5jEUkgZ37YtFw/vf/8koydGmDmW0XyZ4QoQdCWSw/MV0p3wqlXxlO6FTboGZYbP0mW9j1O5QtuGkAMhZIaGMvXQA/BDlfKpKfCXwtwG1t5638hiVqT2twNbyv+uhjKqBi5mCpg/rxgAa+xEANB2CuZL29zw 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: Hi On Sun, Jun 30, 2024 at 11:49=E2=80=AFAM Barnab=C3=A1s P=C5=91cze wrote: > > `MFD_NOEXEC_SEAL` should remove the executable bits and set `F_SEAL_EXEC` > to prevent further modifications to the executable bits as per the commen= t > in the uapi header file: > > not executable and sealed to prevent changing to executable > > However, commit 105ff5339f498a ("mm/memfd: add MFD_NOEXEC_SEAL and MFD_EX= EC") > that introduced this feature made it so that `MFD_NOEXEC_SEAL` unsets > `F_SEAL_SEAL`, essentially acting as a superset of `MFD_ALLOW_SEALING`. > > Nothing implies that it should be so, and indeed up until the second vers= ion > of the of the patchset[0] that introduced `MFD_EXEC` and `MFD_NOEXEC_SEAL= `, > `F_SEAL_SEAL` was not removed, however, it was changed in the third revis= ion > of the patchset[1] without a clear explanation. > > This behaviour is surprising for application developers, there is no > documentation that would reveal that `MFD_NOEXEC_SEAL` has the additional > effect of `MFD_ALLOW_SEALING`. Additionally, combined with `vm.memfd_noex= ec=3D2` > it has the effect of making all memfds initially sealable. > The documentation is in linux main (653c5c75115c), I hope this gives clarity to the usage of MFD_NOEXEC_SEAL flag to application developers, furthermore I'm working on man page for memfd_create. > So do not remove `F_SEAL_SEAL` when `MFD_NOEXEC_SEAL` is requested, > thereby returning to the pre-Linux 6.3 behaviour of only allowing > sealing when `MFD_ALLOW_SEALING` is specified. > > Now, this is technically a uapi break. However, the damage is expected > to be minimal. To trigger user visible change, a program has to do the > following steps: > > - create memfd: > - with `MFD_NOEXEC_SEAL`, > - without `MFD_ALLOW_SEALING`; > - try to add seals / check the seals. > > But that seems unlikely to happen intentionally since this change > essentially reverts the kernel's behaviour to that of Linux <6.3, > so if a program worked correctly on those older kernels, it will > likely work correctly after this change. > During V3 patch discussion, I sent my reasoning, but here are summaries: - As one might have noticed, unlike other flags in memfd_create, MFD_NOEXEC_SEAL is actually a combination of multiple flags. The idea is to make it easier to use memfd in the most common way, which is NOEXEC + F_SEAL_EXEC + MFD_ALLOW_SEALING. - The new sysctl vm.noexec =3D 1 helps existing applications move to a more secure way of using memfd. IMO, MFD_ALLOW_SEALING is included by default because most applications would rather have it than not. In any case, an app can set F_SEAL_SEAL to disable the sealing. - MFD_NOEXEC_SEAL has been added for more than one year, multiple applications and distributions have backported and utilized it. Altering ABI now presents a degree of risk and may lead to disruptions. Best regards, -Jeff > I have used Debian Code Search and GitHub to try to find potential > breakages, and I could only find a single one. dbus-broker's > memfd_create() wrapper is aware of this implicit `MFD_ALLOW_SEALING` > behaviour, and tries to work around it[2]. This workaround will > break. Luckily, this only affects the test suite, it does not affect > the normal operations of dbus-broker. There is a PR with a fix[3]. > > I also carried out a smoke test by building a kernel with this change > and booting an Arch Linux system into GNOME and Plasma sessions. > > There was also a previous attempt to address this peculiarity by > introducing a new flag[4]. > > [0]: https://lore.kernel.org/lkml/20220805222126.142525-3-jeffxu@google.c= om/ > [1]: https://lore.kernel.org/lkml/20221202013404.163143-3-jeffxu@google.c= om/ > [2]: https://github.com/bus1/dbus-broker/blob/9eb0b7e5826fc76cad7b025bc46= f267d4a8784cb/src/util/misc.c#L114 > [3]: https://github.com/bus1/dbus-broker/pull/366 > [4]: https://lore.kernel.org/lkml/20230714114753.170814-1-david@readahead= .eu/ > > Cc: stable@vger.kernel.org > Signed-off-by: Barnab=C3=A1s P=C5=91cze > --- > > * v3: https://lore.kernel.org/linux-mm/20240611231409.3899809-1-jeffxu@ch= romium.org/ > * v2: https://lore.kernel.org/linux-mm/20240524033933.135049-1-jeffxu@goo= gle.com/ > * v1: https://lore.kernel.org/linux-mm/20240513191544.94754-1-pobrn@proto= nmail.com/ > > This fourth version returns to removing the inconsistency as opposed to d= ocumenting > its existence, with the same code change as v1 but with a somewhat extend= ed commit > message. This is sent because I believe it is worth at least a try; it ca= n be easily > reverted if bigger application breakages are discovered than initially im= agined. > > --- > mm/memfd.c | 9 ++++----- > tools/testing/selftests/memfd/memfd_test.c | 2 +- > 2 files changed, 5 insertions(+), 6 deletions(-) > > diff --git a/mm/memfd.c b/mm/memfd.c > index 7d8d3ab3fa37..8b7f6afee21d 100644 > --- a/mm/memfd.c > +++ b/mm/memfd.c > @@ -356,12 +356,11 @@ SYSCALL_DEFINE2(memfd_create, > > inode->i_mode &=3D ~0111; > file_seals =3D memfd_file_seals_ptr(file); > - if (file_seals) { > - *file_seals &=3D ~F_SEAL_SEAL; > + if (file_seals) > *file_seals |=3D F_SEAL_EXEC; > - } > - } else if (flags & MFD_ALLOW_SEALING) { > - /* MFD_EXEC and MFD_ALLOW_SEALING are set */ > + } > + > + if (flags & MFD_ALLOW_SEALING) { > file_seals =3D memfd_file_seals_ptr(file); > if (file_seals) > *file_seals &=3D ~F_SEAL_SEAL; > diff --git a/tools/testing/selftests/memfd/memfd_test.c b/tools/testing/s= elftests/memfd/memfd_test.c > index 95af2d78fd31..7b78329f65b6 100644 > --- a/tools/testing/selftests/memfd/memfd_test.c > +++ b/tools/testing/selftests/memfd/memfd_test.c > @@ -1151,7 +1151,7 @@ static void test_noexec_seal(void) > mfd_def_size, > MFD_CLOEXEC | MFD_NOEXEC_SEAL); > mfd_assert_mode(fd, 0666); > - mfd_assert_has_seals(fd, F_SEAL_EXEC); > + mfd_assert_has_seals(fd, F_SEAL_SEAL | F_SEAL_EXEC); > mfd_fail_chmod(fd, 0777); > close(fd); > } > -- > 2.45.2 > >