linux-kernel - Re: [PATCH v2 1/2] memfd: fix MFD_NOEXEC

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Fri, 24 May 2024 16:15:06 +0200
From: "David Rheinsberg" <david@...dahead.eu>
To: "Jeff Xu" <jeffxu@...omium.org>, "Jeff Xu" <jeffxu@...gle.com>
Cc: "Andrew Morton" <akpm@...ux-foundation.org>, cyphar@...har.com,
 dmitry.torokhov@...il.com, "Daniel Verkamp" <dverkamp@...omium.org>,
 hughd@...gle.com, jorgelo@...omium.org, "Kees Cook" <keescook@...omium.org>,
 linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org,
 linux-mm@...ck.org,
 Barnabás Pőcze <pobrn@...tonmail.com>,
 skhan@...uxfoundation.org, stable@...r.kernel.org
Subject: Re: [PATCH v2 1/2] memfd: fix MFD_NOEXEC_SEAL to be non-sealable by default

Hi

On Fri, May 24, 2024, at 5:39 AM, jeffxu@...omium.org wrote:
> From: Jeff Xu <jeffxu@...gle.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 this 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 = 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ás Pőcze, and Barnabás
> 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]: 
> https://github.com/bus1/dbus-broker/blob/9eb0b7e5826fc76cad7b025bc46f267d4a8784cb/src/util/misc.c#L114
> [2]: https://github.com/bus1/dbus-broker/pull/366
> [3]: 
> https://lore.kernel.org/lkml/20230714114753.170814-1-david@readahead.eu/
>
> Cc: stable@...r.kernel.org
> Fixes: 105ff5339f498a ("mm/memfd: add MFD_NOEXEC_SEAL and MFD_EXEC")
> Signed-off-by: Barnabás Pőcze <pobrn@...tonmail.com>
> Signed-off-by: Jeff Xu <jeffxu@...gle.com>
> Reviewed-by: David Rheinsberg <david@...dahead.eu>

Looks good! Thanks!
David