Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230504170942.822147-1-revest@chromium.org> <ZFQQSKijXQHWlYaI@x1n>
In-Reply-To: <ZFQQSKijXQHWlYaI@x1n>
From:   Florent Revest <revest@chromium.org>
Date:   Fri, 5 May 2023 18:42:08 +0200
Message-ID: <CABRcYmJFcUs=3QYXz8iq7qvu2orJ4HL-cHdBKg9o7=Ma=nfPLw@mail.gmail.com>
Subject: Re: [PATCH 0/4] MDWE without inheritance
To:     Peter Xu <peterx@redhat.com>
Cc:     linux-kernel@vger.kernel.org, linux-mm@kvack.org,
        akpm@linux-foundation.org, catalin.marinas@arm.com,
        anshuman.khandual@arm.com, joey.gouly@arm.com, mhocko@suse.com,
        keescook@chromium.org, david@redhat.com, izbyshev@ispras.ru,
        nd@arm.com, broonie@kernel.org, szabolcs.nagy@arm.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Thu, May 4, 2023 at 10:06=E2=80=AFPM Peter Xu <peterx@redhat.com> wrote:
>
> On Thu, May 04, 2023 at 07:09:38PM +0200, Florent Revest wrote:
> > Joey recently introduced a Memory-Deny-Write-Executable (MDWE) prctl wh=
ich tags
> > current with a flag that prevents pages that were previously not execut=
able from
> > becoming executable.
> > This tag always gets inherited by children tasks. (it's in MMF_INIT_MAS=
K)
> >
> > At Google, we've been using a somewhat similar downstream patch for a f=
ew years
> > now. To make the adoption of this feature easier, we've had it support =
a mode in
> > which the W^X flag does not propagate to children. For example, this is=
 handy if
> > a C process which wants W^X protection suspects it could start children
> > processes that would use a JIT.
> >
> > I'd like to align our features with the upstream prctl. This series pro=
poses a
> > new NO_INHERIT flag to the MDWE prctl to make this kind of adoption eas=
ier. It
> > sets a different flag in current that is not in MMF_INIT_MASK and which=
 does not
> > propagate.
>
> I don't think I have enough context, so sorry if I'm going to ask a naive
> question..

Not at all! :) You're absolutely right, it's important to address these poi=
nts.

> I can understand how current MDWE helps on not allowing any modifi-able
> content from becoming executable.  How could NO_INHERIT help if it won't
> inherit and not in MMF_INIT_MASK?

The way I see it, enabling MDWE is just a small step towards hardening
a binary anyway. It can possibly make exploitation a bit harder in the
case where the attacker has _just_: a write primitive they can use to
write a shellcode somewhere and a primitive to make that page
executable later. It's a fairly narrow protection already and I think
it only really helps as part of a broader "defense in depth" strategy.

> IIUC it means the restriction will only apply to the current process.  Th=
en
> I assume the process can escape from this rule simply by a fork().  If so=
,
> what's the point to protect at all?

If we assume enough control from the attacker, then MDWE is already
useless since it can be bypassed by writing to a file and then
mmapping that file with PROT_EXEC. I think that's a good example of
how "perfect can be the enemy of good" in security hardening. MDWE
isn't a silver-bullet but it's a cheap trick and it makes a small dent
in reducing the attack surface so it seems worth having anyway ?

But indeed, to address your question, if you choose to use this
NO_INHERIT flag: you're no longer protected if the attacker can fork()
as part of their exploitation. I think it's been a useful trade-off
for our internal users since, on the other hand, it also makes
adoption a lot easier: our C++ services developers can trivially opt
into a potpourri of hardening features without having to think too
much about how they work under-the-hood. The default behavior has been
to use a NO_INHERIT strategy so users don't get bad surprises the day
when they try to spawn a JITted subcommand. In the meantime, their C++
service still gets a little bit of extra protection.

> And, what's the difference of this comparing to disabling MDWE after bein=
g
> enabled (which seems to be forbidden for now, but it seems fork() can pla=
y
> a similar role of disabling it)?

That would be functionally somewhat similar, yes. I think it mostly
comes down to ease of adoption. I imagine that users who would opt
into NO_INHERIT are those who are interested in MDWE for the binary
they are writing but aren't 100% confident in what subprocesses they
will run and so they don't have to think about disabling it after
every fork.