Re: [PATCH v7 40/41] x86/shstk: Add ARCH_SHSTK_UNLOCK

[Borislav Petkov]

On Mon, Feb 27, 2023 at 02:29:56PM -0800, Rick Edgecombe wrote:
> From: Mike Rapoport <rppt@xxxxxxxxxxxxx>
> 
> Userspace loaders may lock features before a CRIU restore operation has
> the chance to set them to whatever state is required by the process
> being restored. Allow a way for CRIU to unlock features. Add it as an
> arch_prctl() like the other shadow stack operations, but restrict it being
> called by the ptrace arch_pctl() interface.
> 
> Tested-by: Pengfei Xu <pengfei.xu@xxxxxxxxx>
> Tested-by: John Allen <john.allen@xxxxxxx>
> Tested-by: Kees Cook <keescook@xxxxxxxxxxxx>
> Acked-by: Mike Rapoport (IBM) <rppt@xxxxxxxxxx>

That tag is kinda implicit here. Unless he doesn't ACK his own patch.
:-P

> Reviewed-by: Kees Cook <keescook@xxxxxxxxxxxx>
> Signed-off-by: Mike Rapoport <rppt@xxxxxxxxxxxxx>
> [Merged into recent API changes, added commit log and docs]
> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@xxxxxxxxx>

...

> diff --git a/arch/x86/kernel/shstk.c b/arch/x86/kernel/shstk.c
> index 2faf9b45ac72..3197ff824809 100644
> --- a/arch/x86/kernel/shstk.c
> +++ b/arch/x86/kernel/shstk.c
> @@ -451,9 +451,14 @@ long shstk_prctl(struct task_struct *task, int option, unsigned long features)
>  		return 0;
>  	}
>  
> -	/* Don't allow via ptrace */
> -	if (task != current)
> +	/* Only allow via ptrace */
> +	if (task != current) {

Is that the only case? task != current means ptrace and there's no other
way to do this from userspace?

Isn't there some flag which says that task is ptraced? I think we should
check that one too...

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette