Received-SPF: pass (google.com: domain of linux-kernel+bounces-188781-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Feedback-ID: id2994666:Fastmail
User-Agent: Cyrus-JMAP/3.11.0-alpha0-480-g515a2f54a-fm-20240515.001-g515a2f54
Precedence: bulk
MIME-Version: 1.0
Message-Id: <79b3aa3e-bc70-410e-9646-0b6880a4a74b@app.fastmail.com>
In-Reply-To: <20240524033933.135049-2-jeffxu@google.com>
References: <20240524033933.135049-1-jeffxu@google.com>
 <20240524033933.135049-2-jeffxu@google.com>
Date: Fri, 24 May 2024 16:15:06 +0200
From: "David Rheinsberg" <david@readahead.eu>
To: "Jeff Xu" <jeffxu@chromium.org>, "Jeff Xu" <jeffxu@google.com>
Cc: "Andrew Morton" <akpm@linux-foundation.org>, cyphar@cyphar.com,
 dmitry.torokhov@gmail.com, "Daniel Verkamp" <dverkamp@chromium.org>,
 hughd@google.com, jorgelo@chromium.org, "Kees Cook" <keescook@chromium.org>,
 linux-kernel@vger.kernel.org, linux-kselftest@vger.kernel.org,
 linux-mm@kvack.org,
 =?UTF-8?Q?Barnab=C3=A1s_P=C5=91cze?= <pobrn@protonmail.com>,
 skhan@linuxfoundation.org, stable@vger.kernel.org
Subject: Re: [PATCH v2 1/2] memfd: fix MFD_NOEXEC_SEAL to be non-sealable by default
Content-Type: text/plain;charset=utf-8
Content-Transfer-Encoding: quoted-printable

Hi

On Fri, May 24, 2024, at 5:39 AM, jeffxu@chromium.org wrote:
> From: Jeff Xu <jeffxu@google.com>
>
> By default, memfd_create() creates a non-sealable MFD, unless the
> MFD_ALLOW_SEALING flag is set.
>
> When the MFD_NOEXEC_SEAL flag is initially introduced, the MFD created
> with that flag is sealable, even though MFD_ALLOW_SEALING is not set.
> This patch changes MFD_NOEXEC_SEAL to be non-sealable by default,
> unless MFD_ALLOW_SEALING is explicitly set.
>
> This is a non-backward compatible change. However, as MFD_NOEXEC_SEAL
> is new, we expect not many applications will rely on the nature of
> MFD_NOEXEC_SEAL being sealable. In most cases, the application already
> sets MFD_ALLOW_SEALING if they need a sealable MFD.

This does not really reflect the effort that went into this. Shouldn't t=
his be something along the lines of:

    This is a non-backward compatible change. However, MFD_NOEXEC_SEAL
    was only recently introduced and a codesearch revealed no breaking
    users apart from dbus-broker unit-tests (which have a patch pending
    and explicitly support this change).

> Additionally, this enhances the useability of  pid namespace sysctl
> vm.memfd_noexec. When vm.memfd_noexec equals 1 or 2, the kernel will
> add MFD_NOEXEC_SEAL if mfd_create does not specify MFD_EXEC or
> MFD_NOEXEC_SEAL, and the addition of MFD_NOEXEC_SEAL enables the MFD
> to be sealable. This means, any application that does not desire this
> behavior will be unable to utilize vm.memfd_noexec =3D 1 or 2 to
> migrate/enforce non-executable MFD. This adjustment ensures that
> applications can anticipate that the sealable characteristic will
> remain unmodified by vm.memfd_noexec.
>
> This patch was initially developed by Barnab=C3=A1s P=C5=91cze, and Ba=
rnab=C3=A1s
> used Debian Code Search and GitHub to try to find potential breakages
> and could only find a single one. Dbus-broker's memfd_create() wrapper
> is aware of this implicit `MFD_ALLOW_SEALING` behavior, and tries to
> work around it [1]. 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[2]. In
> addition, David Rheinsberg also raised similar fix in [3]
>
> [1]:=20
> https://github.com/bus1/dbus-broker/blob/9eb0b7e5826fc76cad7b025bc46f2=
67d4a8784cb/src/util/misc.c#L114
> [2]: https://github.com/bus1/dbus-broker/pull/366
> [3]:=20
> https://lore.kernel.org/lkml/20230714114753.170814-1-david@readahead.e=
u/
>
> Cc: stable@vger.kernel.org
> Fixes: 105ff5339f498a ("mm/memfd: add MFD_NOEXEC_SEAL and MFD_EXEC")
> Signed-off-by: Barnab=C3=A1s P=C5=91cze <pobrn@protonmail.com>
> Signed-off-by: Jeff Xu <jeffxu@google.com>
> Reviewed-by: David Rheinsberg <david@readahead.eu>

Looks good! Thanks!
David