Re: [PATCH RFC RFT v2 2/5] fork: Add shadow stack support to clone3()

Mark Brown
2023-11-15T12:37:00Z

[-- Attachment #1: Type: text/plain, Size: 1456 bytes --]

On Wed, Nov 15, 2023 at 12:45:45AM +0000, Edgecombe, Rick P wrote:
> On Tue, 2023-11-14 at 20:05 +0000, Mark Brown wrote:

> > +               if (size < 8)
> > +                       return (unsigned long)ERR_PTR(-EINVAL);

> What is the intention here? The check in map_shadow_stack is to leave
> space for the token, but here there is no token.

It was to ensure that there is sufficient space for at least one entry
on the stack.

> 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.

> > +++ b/include/linux/sched/task.h
> > @@ -41,6 +41,8 @@ struct kernel_clone_args {
> >         void *fn_arg;
> >         struct cgroup *cgrp;
> >         struct css_set *cset;
> > +       unsigned long shadow_stack;
>
> Was this ^ left in accidentally? Elsewhere in this patch it is getting
> checked too.

Yes, it's just bitrot from removing the pointer.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]