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:
signature.asc
Description: PGP signature
