[PATCH] seccomp: Use current_pt_regs()

[PATCH] seccomp: Use current_pt_regs()

[Denis Efremov]

Modify seccomp_do_user_notification(), __seccomp_filter(),
__secure_computing() to use current_pt_regs().

Signed-off-by: Denis Efremov <efremov@linux.com>
---
 kernel/seccomp.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index 3ee59ce0a323..dc4eaa1d6002 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -910,7 +910,7 @@ static int seccomp_do_user_notification(int this_syscall,
 	if (flags & SECCOMP_USER_NOTIF_FLAG_CONTINUE)
 		return 0;
 
-	syscall_set_return_value(current, task_pt_regs(current),
+	syscall_set_return_value(current, current_pt_regs(),
 				 err, ret);
 	return -1;
 }
@@ -943,13 +943,13 @@ static int __seccomp_filter(int this_syscall, const struct seccomp_data *sd,
 		/* Set low-order bits as an errno, capped at MAX_ERRNO. */
 		if (data > MAX_ERRNO)
 			data = MAX_ERRNO;
-		syscall_set_return_value(current, task_pt_regs(current),
+		syscall_set_return_value(current, current_pt_regs(),
 					 -data, 0);
 		goto skip;
 
 	case SECCOMP_RET_TRAP:
 		/* Show the handler the original registers. */
-		syscall_rollback(current, task_pt_regs(current));
+		syscall_rollback(current, current_pt_regs());
 		/* Let the filter pass back 16 bits of data. */
 		seccomp_send_sigsys(this_syscall, data);
 		goto skip;
@@ -962,7 +962,7 @@ static int __seccomp_filter(int this_syscall, const struct seccomp_data *sd,
 		/* ENOSYS these calls if there is no tracer attached. */
 		if (!ptrace_event_enabled(current, PTRACE_EVENT_SECCOMP)) {
 			syscall_set_return_value(current,
-						 task_pt_regs(current),
+						 current_pt_regs(),
 						 -ENOSYS, 0);
 			goto skip;
 		}
@@ -982,7 +982,7 @@ static int __seccomp_filter(int this_syscall, const struct seccomp_data *sd,
 		if (fatal_signal_pending(current))
 			goto skip;
 		/* Check if the tracer forced the syscall to be skipped. */
-		this_syscall = syscall_get_nr(current, task_pt_regs(current));
+		this_syscall = syscall_get_nr(current, current_pt_regs());
 		if (this_syscall < 0)
 			goto skip;
 
@@ -1025,7 +1025,7 @@ static int __seccomp_filter(int this_syscall, const struct seccomp_data *sd,
 			kernel_siginfo_t info;
 
 			/* Show the original registers in the dump. */
-			syscall_rollback(current, task_pt_regs(current));
+			syscall_rollback(current, current_pt_regs());
 			/* Trigger a manual coredump since do_exit skips it. */
 			seccomp_init_siginfo(&info, this_syscall, data);
 			do_coredump(&info);
@@ -1060,7 +1060,7 @@ int __secure_computing(const struct seccomp_data *sd)
 		return 0;
 
 	this_syscall = sd ? sd->nr :
-		syscall_get_nr(current, task_pt_regs(current));
+		syscall_get_nr(current, current_pt_regs());
 
 	switch (mode) {
 	case SECCOMP_MODE_STRICT:
-- 
2.26.2

Re: [PATCH] seccomp: Use current_pt_regs()

[Kees Cook]

On Mon, Aug 24, 2020 at 03:59:21PM +0300, Denis Efremov wrote:
> Modify seccomp_do_user_notification(), __seccomp_filter(),
> __secure_computing() to use current_pt_regs().

This looks okay. It seems some architectures have a separate
define for current_pt_regs(), though it's overlapped directly with
task_pt_regs(). I'm curious what the benefit of the change is?

-Kees

> 
> Signed-off-by: Denis Efremov <efremov@linux.com>
> ---
>  kernel/seccomp.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> index 3ee59ce0a323..dc4eaa1d6002 100644
> --- a/kernel/seccomp.c
> +++ b/kernel/seccomp.c
> @@ -910,7 +910,7 @@ static int seccomp_do_user_notification(int this_syscall,
>  	if (flags & SECCOMP_USER_NOTIF_FLAG_CONTINUE)
>  		return 0;
>  
> -	syscall_set_return_value(current, task_pt_regs(current),
> +	syscall_set_return_value(current, current_pt_regs(),
>  				 err, ret);
>  	return -1;
>  }
> @@ -943,13 +943,13 @@ static int __seccomp_filter(int this_syscall, const struct seccomp_data *sd,
>  		/* Set low-order bits as an errno, capped at MAX_ERRNO. */
>  		if (data > MAX_ERRNO)
>  			data = MAX_ERRNO;
> -		syscall_set_return_value(current, task_pt_regs(current),
> +		syscall_set_return_value(current, current_pt_regs(),
>  					 -data, 0);
>  		goto skip;
>  
>  	case SECCOMP_RET_TRAP:
>  		/* Show the handler the original registers. */
> -		syscall_rollback(current, task_pt_regs(current));
> +		syscall_rollback(current, current_pt_regs());
>  		/* Let the filter pass back 16 bits of data. */
>  		seccomp_send_sigsys(this_syscall, data);
>  		goto skip;
> @@ -962,7 +962,7 @@ static int __seccomp_filter(int this_syscall, const struct seccomp_data *sd,
>  		/* ENOSYS these calls if there is no tracer attached. */
>  		if (!ptrace_event_enabled(current, PTRACE_EVENT_SECCOMP)) {
>  			syscall_set_return_value(current,
> -						 task_pt_regs(current),
> +						 current_pt_regs(),
>  						 -ENOSYS, 0);
>  			goto skip;
>  		}
> @@ -982,7 +982,7 @@ static int __seccomp_filter(int this_syscall, const struct seccomp_data *sd,
>  		if (fatal_signal_pending(current))
>  			goto skip;
>  		/* Check if the tracer forced the syscall to be skipped. */
> -		this_syscall = syscall_get_nr(current, task_pt_regs(current));
> +		this_syscall = syscall_get_nr(current, current_pt_regs());
>  		if (this_syscall < 0)
>  			goto skip;
>  
> @@ -1025,7 +1025,7 @@ static int __seccomp_filter(int this_syscall, const struct seccomp_data *sd,
>  			kernel_siginfo_t info;
>  
>  			/* Show the original registers in the dump. */
> -			syscall_rollback(current, task_pt_regs(current));
> +			syscall_rollback(current, current_pt_regs());
>  			/* Trigger a manual coredump since do_exit skips it. */
>  			seccomp_init_siginfo(&info, this_syscall, data);
>  			do_coredump(&info);
> @@ -1060,7 +1060,7 @@ int __secure_computing(const struct seccomp_data *sd)
>  		return 0;
>  
>  	this_syscall = sd ? sd->nr :
> -		syscall_get_nr(current, task_pt_regs(current));
> +		syscall_get_nr(current, current_pt_regs());
>  
>  	switch (mode) {
>  	case SECCOMP_MODE_STRICT:
> -- 
> 2.26.2
> 

-- 
Kees Cook

Re: [PATCH] seccomp: Use current_pt_regs()

[Denis Efremov]

On 8/24/20 7:34 PM, Kees Cook wrote:
> On Mon, Aug 24, 2020 at 03:59:21PM +0300, Denis Efremov wrote:
>> Modify seccomp_do_user_notification(), __seccomp_filter(),
>> __secure_computing() to use current_pt_regs().
> 
> This looks okay. It seems some architectures have a separate
> define for current_pt_regs(), though it's overlapped directly with
> task_pt_regs(). I'm curious what the benefit of the change is?
> 

Generally, it's just a shorthand.
From: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=a3460a59747cfddfa7be4758e5ef08bf5d751d59
- arch versions are "optimized versions"
- some architectures have task_pt_regs() working only
  for traced tasks blocked on signal delivery. current_pt_regs()
  needs to work for *all* processes

My motivation:
I'm going to add cocci rule for using current_uid(), current_xxx(), ...
instead of raw accesses to current->cred, current->cred->uid
https://elixir.bootlin.com/linux/latest/source/include/linux/cred.h#L379
These interfaces use rcu_dereference_protected() internally for access
check.

I though that adding current_pt_regs(), current_user_stack_pointer() to
this cocci rule will be a good idea.

Thanks,
Denis

Re: [PATCH] seccomp: Use current_pt_regs()

[Kees Cook]

On Mon, 24 Aug 2020 15:59:21 +0300, Denis Efremov wrote:
> Modify seccomp_do_user_notification(), __seccomp_filter(),
> __secure_computing() to use current_pt_regs().

Applied, thanks!

[1/1] seccomp: Use current_pt_regs() instead of task_pt_regs(current)
      https://git.kernel.org/kees/c/4484dbacd7b6

I reworded your commit based on the thread and added one comment for
a weird case where task == current, hopefully that looks correct:
https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/commit/?h=for-next/seccomp&id=4484dbacd7b61eaa4e21332c0a044dedce732ebb

-- 
Kees Cook