Received-SPF: pass (google.com: domain of linux-kernel+bounces-188919-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Fri, 24 May 2024 09:12:14 -0700
From: Aleksa Sarai <cyphar@cyphar.com>
To: Jeff Xu <jeffxu@google.com>
Cc: David Rheinsberg <david@readahead.eu>, 
	=?utf-8?Q?Barnab=C3=A1s_P=C5=91cze?= <pobrn@protonmail.com>, Andrew Morton <akpm@linux-foundation.org>, linux-mm@kvack.org, 
	linux-kernel@vger.kernel.org, linux-kselftest@vger.kernel.org, dmitry.torokhov@gmail.com, 
	Daniel Verkamp <dverkamp@chromium.org>, hughd@google.com, jorgelo@chromium.org, 
	skhan@linuxfoundation.org, Kees Cook <keescook@chromium.org>
Subject: Re: [PATCH v1] memfd: `MFD_NOEXEC_SEAL` should not imply
 `MFD_ALLOW_SEALING`
Message-ID: <20240524.160158-custard.odds.smutty.cuff-caukvmB4EWP9@cyphar.com>
References: <20240513191544.94754-1-pobrn@protonmail.com>
 <CALmYWFt7MYbWrCDVEKH4DrMQGxaXA2kK8qth-JVxzkvMd6Ohtg@mail.gmail.com>
 <20240522162324.0aeba086228eddd8aff4f628@linux-foundation.org>
 <1KDsEBw8g7ymBVpGJZp9NRH1HmCBsQ_jjQ_jKOg90gLUFhW5W6lcG-bI4-5OPkrD24RiG7G83VoZL4SXPQjfldsNFDg7bFnFFgrVZWwSWXQ=@protonmail.com>
 <08450f80-4c33-40db-886f-fee18e531545@app.fastmail.com>
 <CALmYWFv9dK5ZPzwx3WCLMXzuuDadvFxh84+8rrT7aL105+ZZAQ@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="o6rbmbukevbfurx5"
Content-Disposition: inline
In-Reply-To: <CALmYWFv9dK5ZPzwx3WCLMXzuuDadvFxh84+8rrT7aL105+ZZAQ@mail.gmail.com>


--o6rbmbukevbfurx5
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On 2024-05-23, Jeff Xu <jeffxu@google.com> wrote:
> On Thu, May 23, 2024 at 1:24=E2=80=AFAM David Rheinsberg <david@readahead=
=2Eeu> wrote:
> >
> > Hi
> >
> > On Thu, May 23, 2024, at 4:25 AM, Barnab=C3=A1s P=C5=91cze wrote:
> > > 2024. m=C3=A1jus 23., cs=C3=BCt=C3=B6rt=C3=B6k 1:23 keltez=C3=A9ssel,=
 Andrew Morton
> > > <akpm@linux-foundation.org> =C3=ADrta:
> > >> It's a change to a userspace API, yes?  Please let's have a detailed
> > >> description of why this is OK.  Why it won't affect any existing use=
rs.
> > >
> > > Yes, it is a uAPI change. To trigger user visible change, a program h=
as to
> > >
> > >  - create a memfd
> > >    - with MFD_NOEXEC_SEAL,
> > >    - without MFD_ALLOW_SEALING;
> > >  - try to add seals / check the seals.
> > >
> > > This change in essence reverts the kernel's behaviour to that of Linux
> > > <6.3, where
> > > only `MFD_ALLOW_SEALING` enabled sealing. If a program works correctly
> > > on those
> > > kernels, it will likely work correctly after this change.
> > >
> > > I have looked through Debian Code Search and GitHub, searching for
> > > `MFD_NOEXEC_SEAL`.
> > > And I could find only a single breakage that this change would case:
> > > dbus-broker
> > > has its own memfd_create() wrapper that is aware of this implicit
> > > `MFD_ALLOW_SEALING`
> > > behaviour[0], and tries to work around it. This workaround will break.
> > > Luckily,
> > > however, as far as I could tell this only affects the test suite of
> > > dbus-broker,
> > > not its normal operations, so I believe it should be fine. I have
> > > prepared a PR
> > > with a fix[1].
> >
> > We asked for exactly this fix before, so I very much support this. Our =
test-suite in `dbus-broker` merely verifies what the current kernel behavio=
r is (just like the kernel selftests). I am certainly ok if the kernel brea=
ks it. I will gladly adapt the test-suite.
> >
> > Previous discussion was in:
> >
> >     [PATCH] memfd: support MFD_NOEXEC alongside MFD_EXEC
> >     https://lore.kernel.org/lkml/20230714114753.170814-1-david@readahea=
d.eu/
> >
> > Note that this fix is particularly important in combination with `vm.me=
mfd_noexec=3D2`, since this breaks existing user-space by enabling sealing =
on all memfds unconditionally. I also encourage backporting to stable kerne=
ls.
> >
> Also with vm.memfd_noexec=3D1.
> I think that problem must be addressed either with this patch, or with
> a new flag.
>=20
> Regarding vm.memfd_noexec, on another topic.
> I think in addition to  vm.memfd_noexec =3D 1 and 2,  there still could
> be another state: 3
>=20
> =3D0. Do nothing.
> =3D1. This will add MFD_NOEXEC_SEAL if application didn't set EXEC or
> MFD_NOEXE_SEAL (to help with the migration)
> =3D2: This will reject all calls without MFD_NOEXEC_SEAL (the whole
> system doesn't allow executable memfd)
> =3D3:  Application must set MFD_EXEC or MFD_NOEXEC_SEAL explicitly, or
> else it will be rejected.
>=20
> 3 is useful because it lets applications choose what to use, and
> forces applications to migrate to new semantics (this is what 2 did
> before 9876cfe8).
> The caveat is 3 is less restrictive than 2, so must document it clearly.

As discussed at the time, "you must use this flag" is not a useful
setting for a general purpose operating system because it explicitly
disables backwards compatibility (breaking any application that was
written in the past 10 years!) for no reason other than "new is better".

As I suggested when we fixed the semantics of vm.memfd_noexec, if you
really want to block a particular flag from not being set, seccomp lets
you do this incredibly easily without acting as a footgun for admins.
Yes, vm.memfd_noexec can break programs that use executable memfds, but
that is the point of the sysctl -- making vm.memfd_noexec break programs
that don't use executable memfds (they are only guilty of being written
before mid-2023) is not useful.

In addition, making 3 less restrictive than 2 would make the original
restriction mechanism useless. A malicious process could raise the
setting to 3 and disable the "protection" (as discussed before, I really
don't understand the threat model here, but making it possible to
disable easily is pretty clearly). You could change the policy, but now
you're adding more complexity for a feature that IMO doesn't really make
sense in the first place.

> -Jeff
>=20
> > Reviewed-by: David Rheinsberg <david@readahead.eu>
> >
> > Thanks
> > David

--=20
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>

--o6rbmbukevbfurx5
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iHUEABYKAB0WIQS2TklVsp+j1GPyqQYol/rSt+lEbwUCZlC8XgAKCRAol/rSt+lE
b1u8AQCSXfCJULF5mlBI3NTwWtmfPXHkw9y5f+xKfcb1hs0lMgEAnnJpD63h0VKg
jcEGjPSfyrhbIP6TbC+mELL+Af3N7Ao=
=SpBD
-----END PGP SIGNATURE-----

--o6rbmbukevbfurx5--