Message-ID: <ZVXvptSmmJ6MQ0dY@arm.com>
Date:   Thu, 16 Nov 2023 10:32:06 +0000
From:   "Szabolcs.Nagy@....com" <Szabolcs.Nagy@....com>
To:     "Edgecombe, Rick P" <rick.p.edgecombe@...el.com>,
        "broonie@...nel.org" <broonie@...nel.org>
Cc:     "dietmar.eggemann@....com" <dietmar.eggemann@....com>,
        "keescook@...omium.org" <keescook@...omium.org>,
        "shuah@...nel.org" <shuah@...nel.org>,
        "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>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "mingo@...hat.com" <mingo@...hat.com>,
        "hjl.tools@...il.com" <hjl.tools@...il.com>,
        "rostedt@...dmis.org" <rostedt@...dmis.org>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "linux-api@...r.kernel.org" <linux-api@...r.kernel.org>,
        "vschneid@...hat.com" <vschneid@...hat.com>,
        "catalin.marinas@....com" <catalin.marinas@....com>,
        "bristot@...hat.com" <bristot@...hat.com>,
        "will@...nel.org" <will@...nel.org>,
        "hpa@...or.com" <hpa@...or.com>,
        "peterz@...radead.org" <peterz@...radead.org>,
        "jannh@...gle.com" <jannh@...gle.com>,
        "bp@...en8.de" <bp@...en8.de>,
        "bsegall@...gle.com" <bsegall@...gle.com>,
        "linux-kselftest@...r.kernel.org" <linux-kselftest@...r.kernel.org>,
        "Pandey, Sunil K" <sunil.k.pandey@...el.com>,
        "x86@...nel.org" <x86@...nel.org>,
        "juri.lelli@...hat.com" <juri.lelli@...hat.com>
Subject: Re: [PATCH RFC RFT v2 2/5] fork: Add shadow stack support to clone3()

The 11/16/2023 00:52, Edgecombe, Rick P wrote:
> On Wed, 2023-11-15 at 18:43 +0000, Mark Brown wrote:
> >
> > > otherwise 0 size would be fine: the child may not execute
> > > a call instruction at all.
>
> It seems like a special case. Where should the SSP be for the new
> thread?

yes it is likely not an interesting case in practice so < 8
can be an error i think.

> > > > > I think for CLONE_VM we should not require a non-zero size.
> > > > > Speaking of
> > > > > CLONE_VM we should probably be clear on what the expected
> > > > > behavior is
> > > > > for situations when a new shadow stack is not usually
> > > > > allocated.
> > > > > !CLONE_VM || CLONE_VFORK will use the existing shadow stack.
> > > > > Should we
> > > > > require shadow_stack_size be zero in this case, or just ignore
> > > > > it? I'd
> > > > > lean towards requiring it to be zero so userspace doesn't pass
> > > > > garbage
> > > > > in that we have to accommodate later. What we could possibly
> > > > > need to do
> > > > > around that though, I'm not sure. What do you think?
> >
> > > > Yes, requiring it to be zero in that case makes sense I think.
> >
> > > i think the condition is "no specified separate stack for
> > > the child (stack==0 || stack==sp)".
> >
> > > CLONE_VFORK does not imply that the existing stack will be
> > > used (a stack for the child can be specified, i think both
> > > glibc and musl do this in posix_spawn).
> >
> > That also works as a check I think, though it requires the arch to
> > check
> > for the stack==sp case - I hadn't been aware of the posix_spawn()
> > usage,
> > the above checks Rick suggested just follow the handling for implicit
> > allocation we have currently.
>
> I didn't realize it was passing its own stack either. I guess the
> reason is to avoid stack overflows. But none of the specific reasons
> listed in the comments seem to applicable to shadow stacks.

while CLONE_VFORK allows the child to use the parent shadow
stack (parent and child cannot execute at the same time and
the child wont return to frames created by the parent), we
want to enable precise size accounting of the shadow stack
so requesting a new shadow stack should work if new stack
is specified.

but stack==0 can force shadow_stack_size==0.

i guess the tricky case is stack!=0 && shadow_stack_size==0:
the user may want a new shadow stack with default size logic,
or (with !CLONE_VM || CLONE_VFORK) wants to use the existing
shadow stack from the parent.

>
> What is the case for stack=sp bit of the logic?

iirc it is not documented in the clone man page what stack=0
means and of course you don't want sp==0 in the vfork child
so some targets sets stack to sp in vfork, others set it 0
and expect the kernel to do the right thing.

this likely does not apply to clone3 where the size has to be
specified so maybe stack==sp does not need special treatment.

> I need to look into this more. My first thought is, passing in a stack
> doesn't absolutely mean they want a new shadow stack allocated either.
> We are changing one heuristic, for another.

yes.

> The other thought is, the new behavior in the !CLONE_VM case doesn't
> seem optimal. fork has ->stack==0. Then we would be allocating a stack
> in only the child's MM and changing the SSP there, and for what reason?
> So I don't think we should fully move away from taking hints from the
> CLONE flags.

only stack!=0 case is tricky. stack==0 means existing shadow stack.

>
> Maybe an alternate could just be to lose the CLONE_VFORK specific stack
> sharing logic. CLONE_VM always gets a new shadow stack. I don't think
> it would disturb userspace functionally, but just involves more
> mapping/unmapping.
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Powered by blists - more mailing lists