linux-kernel - Re: [PATCH RFT v8 4/9] fork: Add shadow stack support to clone3()

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAG48ez2z5bRdKNddG+kEGz9A_m=66r38OHjyg6CapFTcjT9aRg@mail.gmail.com>
Date: Fri, 16 Aug 2024 19:08:09 +0200
From: Jann Horn <jannh@...gle.com>
To: "Edgecombe, Rick P" <rick.p.edgecombe@...el.com>
Cc: "dietmar.eggemann@....com" <dietmar.eggemann@....com>, "broonie@...nel.org" <broonie@...nel.org>, 
	"Szabolcs.Nagy@....com" <Szabolcs.Nagy@....com>, "brauner@...nel.org" <brauner@...nel.org>, 
	"dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>, "debug@...osinc.com" <debug@...osinc.com>, 
	"mgorman@...e.de" <mgorman@...e.de>, "vincent.guittot@...aro.org" <vincent.guittot@...aro.org>, 
	"fweimer@...hat.com" <fweimer@...hat.com>, "mingo@...hat.com" <mingo@...hat.com>, 
	"rostedt@...dmis.org" <rostedt@...dmis.org>, "hjl.tools@...il.com" <hjl.tools@...il.com>, 
	"tglx@...utronix.de" <tglx@...utronix.de>, "vschneid@...hat.com" <vschneid@...hat.com>, 
	"shuah@...nel.org" <shuah@...nel.org>, "hpa@...or.com" <hpa@...or.com>, 
	"peterz@...radead.org" <peterz@...radead.org>, "bp@...en8.de" <bp@...en8.de>, 
	"bsegall@...gle.com" <bsegall@...gle.com>, "x86@...nel.org" <x86@...nel.org>, 
	"juri.lelli@...hat.com" <juri.lelli@...hat.com>, 
	"linux-kselftest@...r.kernel.org" <linux-kselftest@...r.kernel.org>, "kees@...nel.org" <kees@...nel.org>, 
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, 
	"catalin.marinas@....com" <catalin.marinas@....com>, 
	"linux-api@...r.kernel.org" <linux-api@...r.kernel.org>, "will@...nel.org" <will@...nel.org>
Subject: Re: [PATCH RFT v8 4/9] fork: Add shadow stack support to clone3()

On Thu, Aug 15, 2024 at 2:18 AM Edgecombe, Rick P
<rick.p.edgecombe@...el.com> wrote:
> On Thu, 2024-08-08 at 09:15 +0100, Mark Brown wrote:
> > +       if (access_remote_vm(mm, addr, &val, sizeof(val),
> > +                            FOLL_FORCE | FOLL_WRITE) != sizeof(val))
> > +               goto out;
>
> The GUPs still seem a bit unfortunate for a couple reasons:
>  - We could do a CMPXCHG version and are just not (I see ARM has identical code
> in gcs_consume_token()). It's not the only race like this though FWIW.
>  - I *think* this is the only unprivileged FOLL_FORCE that can write to the
> current process in the kernel. As is, it could be used on normal RO mappings, at
> least in a limited way. Maybe another point for the VMA check. We'd want to
> check that it is normal shadow stack?

Yeah, having a FOLL_FORCE write in clone3 would be a weakness for
userspace CFI and probably make it possible to violate mseal()
restrictions that are supposed to enforce that address space regions
are read-only.

>  - Lingering doubts about the wisdom of doing GUPs during task creation.
>
> I don't think they are show stoppers, but the VMA check would be nice to have in
> the first upstream support.
[...]
> > +static void shstk_post_fork(struct task_struct *p,
> > +                           struct kernel_clone_args *args)
> > +{
> > +       if (!IS_ENABLED(CONFIG_ARCH_HAS_USER_SHADOW_STACK))
> > +               return;
> > +
> > +       if (!args->shadow_stack)
> > +               return;
> > +
> > +       if (arch_shstk_post_fork(p, args) != 0)
> > +               force_sig_fault_to_task(SIGSEGV, SEGV_CPERR, NULL, p);
> > +}
> > +
>
> Hmm, is this forcing the signal on the new task, which is set up on a user
> provided shadow stack that failed the token check? It would handle the signal
> with an arbitrary SSP then I think. We should probably fail the clone call in
> the parent instead, which can be done by doing the work in copy_process(). Do
> you see a problem with doing it at the end of copy_process()? I don't know if
> there could be ordering constraints.

FWIW I think we have things like force_fatal_sig() and
force_exit_sig() to send signals that userspace can't catch with
signal handlers - if you have to do the copying after the new task has
been set up, something along those lines might be the right way to
kill the child.

Though, did anyone in the thread yet suggest that you could do this
before the child process has fully materialized but after the child MM
has been set up? Somewhere in copy_process() between copy_mm() and the
"/* No more failure paths after this point. */" comment?