Received-SPF: pass (google.com: domain of linux-kernel+bounces-192797-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
MIME-Version: 1.0
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>
 <CALmYWFtedtEnfGFp5DYacHYOE7+GB8yoQC-iyw7JAxySmgQ7vw@mail.gmail.com> <f880562e-9521-4270-82e2-c6fb14dc853a@app.fastmail.com>
In-Reply-To: <f880562e-9521-4270-82e2-c6fb14dc853a@app.fastmail.com>
From: Jeff Xu <jeffxu@google.com>
Date: Tue, 28 May 2024 10:13:41 -0700
Message-ID: <CALmYWFuPBEM2DE97mQvB2eEgSO9Dvt=uO9OewMhGfhGCY66Hbw@mail.gmail.com>
Subject: Re: [PATCH v1] memfd: `MFD_NOEXEC_SEAL` should not imply `MFD_ALLOW_SEALING`
To: David Rheinsberg <david@readahead.eu>, Jeff Xu <jeffxu@chromium.org>
Cc: Aleksa Sarai <cyphar@cyphar.com>, =?UTF-8?B?QmFybmFiw6FzIFDFkWN6ZQ==?= <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>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Fri, May 24, 2024 at 7:29=E2=80=AFAM David Rheinsberg <david@readahead.e=
u> wrote:
>
> Hi
>
> On Thu, May 23, 2024, at 6:55 PM, Jeff Xu wrote:
> > On Thu, May 23, 2024 at 9:20=E2=80=AFAM Jeff Xu <jeffxu@google.com> wro=
te:
> >> On Thu, May 23, 2024 at 1:24=E2=80=AFAM David Rheinsberg <david@readah=
ead.eu> wrote:
> >> > We asked for exactly this fix before, so I very much support this. O=
ur test-suite in `dbus-broker` merely verifies what the current kernel beha=
vior is (just like the kernel selftests). I am certainly ok if the kernel b=
reaks it. I will gladly adapt the test-suite.
> >> >
> > memfd is by default not sealable, and file is by default sealable,
> > right ? that makes the memfd  semantics different from other objects
> > in linux.
> > I wonder what is the original reason to have memfd  this way?
>
> shmem-files are *not* sealable by default. This design was followed for b=
ackward compatibility reasons, since shmem-files predate sealing and silent=
ly enabling sealing on all shmem-files would have broken existing users (se=
e shmem.c which initializes seals to F_SEAL_SEAL).
>
One may ask the question: If shmem-files  need to be non-sealable by
default, does memfd need to be so as well?

> I am not sure what you mean with "makes [memfd] semantics different from =
other objects in linux". Can you elaborate?
>
The memory sealing feature - mseal() went through similar discussion
on MAP_SEALABLE flag during the RFC phase,  but everyone doesn't like
the flag, and it gets dropped.
The feedback from communities for MAP_SEALABLE were.
- such a flag will slow down the adoption of the feature, i.e.
applications on multiple layers/libraries must change in order to use
sealing, i.e.  time of construction and  time of sealing might reside
in different libraries.
- Deny of service attack is likely not a concern,  the attacker that
is able to call mseal() can probably already call mprotect() or other
calls and achieve a similar DOS attack.

> Since `memfd_create` was introduced at the same time as shmem-sealing, it=
 could certainly have enabled sealing by default. Not sure whether this wou=
ld be preferable, though.
>
I would think making memfd sealable is desirable.

Probably the same for a shmem-file too.

> > Another solution is to change memfd to be by-default sealable,
> > although that will be an api change, but what side effect  will it be
> > ?
> > If we are worried about the memfd being sealed by an attacker, the
> > malicious code could also overwrite the content since memfd is not
> > sealed.
>
> You cannot change the default-seals retrospectively. There are existing s=
hmem-users that share file-descriptors and *expect* the other party to be a=
ble to override data, but do *not* expect the other party to be able to app=
ly seals. Note that these models explicitly *want* shared, writable access =
to the buffer (e.g., render-client shares a buffer with the display server =
for scanout), so just because you can *write* to a shmem-file does not mean=
 that sharing is unsafe (e.g., using SIGBUS+mmap can safely deal with page-=
faults).
>
If the other party is controlled by an attacker, the attacker can
write garbage to the shm-file/memfd, that is already the end of the
game, at that point, sealing is no longer a concern, right?
If the threat-model is preventing attacker on the other side to write
the garbage data, then F_SEAL_WRITE|F_SEAL_SHRINK|F_SEAL_GROW can be
applied, in that case, default-sealable seems preferable because of
less code change.
If the other party needs to write to shmem/memfd anyway, then maybe
F_SEAL_EXEC needs to be applied ?

Thanks
-Jeff

> Thanks
> David