Received-SPF: pass (google.com: domain of linux-kernel+bounces-206442-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Precedence: bulk
MIME-Version: 1.0
References: <20240524033933.135049-1-jeffxu@google.com> <20240524033933.135049-2-jeffxu@google.com>
 <79b3aa3e-bc70-410e-9646-0b6880a4a74b@app.fastmail.com> <CALmYWFu61FkbboWkXUSKBGmXeiNtBwrgfizS5kNvPMx4ByUqPQ@mail.gmail.com>
 <b8cGJnU5ofWgsiKD5z8RGlW-2ijs7IW9h4LUg1tzFBu3agFinCvdxuiSaUDG_DfVen2vCDNu-QbGfOR7DeARf4jsy3CNNTfzQGMX1HfqHdo=@protonmail.com>
 <CALmYWFv+Tsqwv96oB4rTrJ7_ZC3CoNZFjmKFYKQgGZuceqZ6vg@mail.gmail.com> <2fQi6XI7TmRN_qi9xldglgYFujpzMvr0bbkxhYNJhY6VRYjDsyTDqavR6OPU6DNBtCKAPgBVKxV0SMo7WnjUaDit-zxsBZauGavgKzqcNy8=@protonmail.com>
In-Reply-To: <2fQi6XI7TmRN_qi9xldglgYFujpzMvr0bbkxhYNJhY6VRYjDsyTDqavR6OPU6DNBtCKAPgBVKxV0SMo7WnjUaDit-zxsBZauGavgKzqcNy8=@protonmail.com>
From: Jeff Xu <jeffxu@chromium.org>
Date: Fri, 7 Jun 2024 08:59:20 -0700
Message-ID: <CABi2SkXyHegwpZKF7ZVDAOsqzaHE4BdDQOMmbuJ+51HkQ3CmJg@mail.gmail.com>
Subject: Re: [PATCH v2 1/2] memfd: fix MFD_NOEXEC_SEAL to be non-sealable by default
To: =?UTF-8?B?QmFybmFiw6FzIFDFkWN6ZQ==?= <pobrn@protonmail.com>
Cc: Jeff Xu <jeffxu@google.com>, David Rheinsberg <david@readahead.eu>, 
	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, 
	skhan@linuxfoundation.org, stable@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Barnab=C3=A1s

On Fri, May 31, 2024 at 11:56=E2=80=AFAM Barnab=C3=A1s P=C5=91cze <pobrn@pr=
otonmail.com> wrote:
>
> 2024. m=C3=A1jus 30., cs=C3=BCt=C3=B6rt=C3=B6k 0:24 keltez=C3=A9ssel, Jef=
f Xu <jeffxu@google.com> =C3=ADrta:
>
> > On Wed, May 29, 2024 at 2:46=E2=80=AFPM Barnab=C3=A1s P=C5=91cze <pobrn=
@protonmail.com> wrote:
> > >
> > > Hi
> > >
> > >
> > > 2024. m=C3=A1jus 29., szerda 23:30 keltez=C3=A9ssel, Jeff Xu <jeffxu@=
google.com> =C3=ADrta:
> > >
> > > > Hi David and Barnab=C3=A1s
> > > >
> > > > On Fri, May 24, 2024 at 7:15=E2=80=AFAM David Rheinsberg <david@rea=
dahead.eu> wrote:
> > > > >
> > > > > 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 t=
he
> > > > > > 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 no=
t set.
> > > > > > This patch changes MFD_NOEXEC_SEAL to be non-sealable by defaul=
t,
> > > > > > unless MFD_ALLOW_SEALING is explicitly set.
> > > > > >
> > > > > > This is a non-backward compatible change. However, as MFD_NOEXE=
C_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. Shou=
ldn'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 bre=
aking
> > > > >     users apart from dbus-broker unit-tests (which have a patch p=
ending
> > > > >     and explicitly support this change).
> > > > >
> > > > Actually, I think we might need to hold on to this change. With deb=
ian
> > > > code search, I found more codes that already use MFD_NOEXEC_SEAL
> > > > without MFD_ALLOW_SEALING. e.g. systemd [1], [2] [3]
> > >
> > > Yes, I have looked at those as well, and as far as I could tell,
> > > they are not affected. Have I missed something?
> > >
> > In the example, the MFD was created then passed into somewhere else
> > (safe_fork_full, open_serialization_fd, etc.), the scope and usage of
> > mfd isn't that clear to me, you might have checked all the user cases.
> > In addition, MFD_NOEXEC_SEAL  exists in libc and rust and go lib.  I
> > don't know if debian code search is sufficient to cover enough apps .
> > There is a certain risk.
> >
> > Fundamentally, I'm not convinced that making MFD default-non-sealable
> > has  meaningful benefit, especially when MFD_NOEXEC_SEAL is new.
>
> Certainly, there is always a risk, I did not mean to imply that there isn=
't.
> However, I believe this risk is low enough to at least warrant an attempt=
 at
> eliminating this inconsistency. It can always be reverted if it turns out=
 that
> the effects have been vastly underestimated by me.
>
> So I would still like to see this change merged.
>

The MFD_NOEXEC_SEAL is a new flag, technically, ABI is not broken.
The sysctl vm.memfd_noexec=3D1 or 2, is meant to help
migration/enforcement of MFD_NOEXEC_SEAL, so it will break application
if it is used pre-maturely, that is by-design.

I think the main problem here is lack of documentation, instead of a code b=
ug.
ABI change shouldn't be treated lightly, given the risk, I would like
to keep the API the same and add the documentation instead. I think
that is the best route forward.

Best Regards,
-Jeff


>
> Regards,
> Barnab=C3=A1s P=C5=91cze
>
>
> >
> >
> > >
> > > Regards,
> > > Barnab=C3=A1s
> > >
> > >
> > > >
> > > > I'm not sure if this  will break  more applications not-knowingly t=
hat
> > > > have started relying on MFD_NOEXEC_SEAL being sealable. The feature
> > > > has been out for more than a year.
> > > >
> > > > Would you consider my augments in [4] to make MFD to be sealable by=
 default ?
> > > >
> > > > At this moment, I'm willing to add a document to clarify that
> > > > MFD_NOEXEC_SEAL is sealable by default, and that an app that needs
> > > > non-sealable MFD can  set  SEAL_SEAL.  Because both MFD_NOEXEC_SEAL
> > > > and vm.memfd_noexec are new,  I don't think it breaks the existing
> > > > ABI, and vm.memfd_noexec=3D0 is there for backward compatibility
> > > > reasons. Besides, I honestly think there is little reason that MFD
> > > > needs to be non-sealable by default.  There might be few rare cases=
,
> > > > but the majority of apps don't need that.  On the flip side, the fa=
ct
> > > > that MFD is set up to be sealable by default is a nice bonus for an
> > > > app - it makes it easier for apps to use the sealing feature.
> > > >
> > > > What do you think ?
> > > >
> > > > Thanks
> > > > -Jeff
> > > >
> > > > [1] https://codesearch.debian.net/search?q=3DMFD_NOEXEC_SEAL
> > > > [2] https://codesearch.debian.net/show?file=3Dsystemd_256~rc3-5%2Fs=
rc%2Fhome%2Fhomed-home.c&line=3D1274
> > > > [3] https://sources.debian.org/src/elogind/255.5-1debian1/src/share=
d/serialize.c/?hl=3D558#L558
> > > > [4] https://lore.kernel.org/lkml/CALmYWFuPBEM2DE97mQvB2eEgSO9Dvt=3D=
uO9OewMhGfhGCY66Hbw@mail.gmail.com/
> > > >
> > > >
> > > > > > Additionally, this enhances the useability of  pid namespace sy=
sctl
> > > > > > 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 th=
e MFD
> > > > > > to be sealable. This means, any application that does not desir=
e this
> > > > > > behavior will be unable to utilize vm.memfd_noexec =3D 1 or 2 t=
o
> > > > > > migrate/enforce non-executable MFD. This adjustment ensures tha=
t
> > > > > > applications can anticipate that the sealable characteristic wi=
ll
> > > > > > remain unmodified by vm.memfd_noexec.
> > > > > >
> > > > > > This patch was initially developed by Barnab=C3=A1s P=C5=91cze,=
 and Barnab=C3=A1s
> > > > > > used Debian Code Search and GitHub to try to find potential bre=
akages
> > > > > > and could only find a single one. Dbus-broker's memfd_create() =
wrapper
> > > > > > is aware of this implicit `MFD_ALLOW_SEALING` behavior, and tri=
es to
> > > > > > work around it [1]. This workaround will break. Luckily, this o=
nly
> > > > > > 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/9eb0b7e5826fc76cad7b02=
5bc46f267d4a8784cb/src/util/misc.c#L114
> > > > > > [2]: https://github.com/bus1/dbus-broker/pull/366
> > > > > > [3]:
> > > > > > https://lore.kernel.org/lkml/20230714114753.170814-1-david@read=
ahead.eu/
> > > > > >
> > > > > > Cc: stable@vger.kernel.org
> > > > > > Fixes: 105ff5339f498a ("mm/memfd: add MFD_NOEXEC_SEAL and MFD_E=
XEC")
> > > > > > 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
> > > >