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>
 <CABRcYmJFcUs=3QYXz8iq7qvu2orJ4HL-cHdBKg9o7=Ma=nfPLw@mail.gmail.com> <ZFhQcwDBFWcRCC4N@x1n>
In-Reply-To: <ZFhQcwDBFWcRCC4N@x1n>
From:   Florent Revest <revest@chromium.org>
Date:   Mon, 8 May 2023 14:12:21 +0200
Message-ID: <CABRcYm+RquNGYUyfCEmLrcaoMSKagzwXgto-24ZCaLAdFUKUjg@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,
        toiwoton@gmail.com, lennart@poettering.net
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Mon, May 8, 2023 at 3:29=E2=80=AFAM Peter Xu <peterx@redhat.com> wrote:
>
> On Fri, May 05, 2023 at 06:42:08PM +0200, Florent Revest wrote:
> > On Thu, May 4, 2023 at 10:06=E2=80=AFPM Peter Xu <peterx@redhat.com> wr=
ote:
> > >
> > > On Thu, May 04, 2023 at 07:09:38PM +0200, Florent Revest wrote:
> > > > Joey recently introduced a Memory-Deny-Write-Executable (MDWE) prct=
l which tags
> > > > current with a flag that prevents pages that were previously not ex=
ecutable from
> > > > becoming executable.
> > > > This tag always gets inherited by children tasks. (it's in MMF_INIT=
_MASK)
> > > >
> > > > At Google, we've been using a somewhat similar downstream patch for=
 a few years
> > > > now. To make the adoption of this feature easier, we've had it supp=
ort a mode in
> > > > which the W^X flag does not propagate to children. For example, thi=
s is handy if
> > > > a C process which wants W^X protection suspects it could start chil=
dren
> > > > processes that would use a JIT.
> > > >
> > > > I'd like to align our features with the upstream prctl. This series=
 proposes a
> > > > new NO_INHERIT flag to the MDWE prctl to make this kind of adoption=
 easier. It
> > > > sets a different flag in current that is not in MMF_INIT_MASK and w=
hich does not
> > > > propagate.
> > >
> > > I don't think I have enough context, so sorry if I'm going to ask a n=
aive
> > > question..
> >
> > Not at all! :) You're absolutely right, it's important to address these=
 points.
> >
> > > I can understand how current MDWE helps on not allowing any modifi-ab=
le
> > > content from becoming executable.  How could NO_INHERIT help if it wo=
n'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.=
  Then
> > > I assume the process can escape from this rule simply by a fork().  I=
f 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 =
being
> > > enabled (which seems to be forbidden for now, but it seems fork() can=
 play
> > > 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.
>
> Okay, that makes sense to me.  Thanks.
>
> Since the original MDWE was for systemd, I'm wondering what will happen i=
f
> some program like what you said is invoked by systemd and with MDWE enabl=
ed
> already.

Good question

> Currently in your patch IIUC MDWE_NO_INHERIT will fail directly on MDWE
> enabled process,

Yes, I tried to stay close to the spirit of the existing logic (which
doesn't allow any sort of privilege gains) but this is not
particularly a requirement on our side so I'm quite flexible here.
Maybe Joey has an input here ?

> but then it makes me think whether it makes more sense to
> allow converting MDWE->MDWE_NO_INHERIT in this case.  It seems to provide=
 a
> most broad coverage on system daemons using MDWE starting from systemd
> initial process, meanwhile allows specific daemons to fork into anything
> like a JIT process so it can make itself NO_INHERIT.  Attackers won't
> leverage this because MDWE_NO_INHERIT also means MDWE enabled.

I should have cc-ed systemd folks who could have opinions on this. I
will for v2.

+ cc Topi & Lennart