Patchwork [RFC,v6,3/6] x86/entry: Erase kernel stack in syscall_trace_enter()

login
register
mail settings
Submitter Alexander Popov
Date Dec. 5, 2017, 11:33 p.m.
Message ID <1512516827-29797-4-git-send-email-alex.popov@linux.com>
Download mbox | patch
Permalink /patch/10094145/
State New
Headers show

Comments

Alexander Popov - Dec. 5, 2017, 11:33 p.m.
Make STACKLEAK erase kernel stack after ptrace/seccomp/auditing
not to leave any sensitive information on the stack for the syscall code.

This code is modified from Brad Spengler/PaX Team's code in the last
public patch of grsecurity/PaX based on our understanding of the code.
Changes or omissions from the original code are ours and don't reflect
the original grsecurity/PaX code.

Signed-off-by: Alexander Popov <alex.popov@linux.com>
---
 arch/x86/entry/common.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)
Dmitry V. Levin - Dec. 6, 2017, 9:12 p.m.
Hi,

On Wed, Dec 06, 2017 at 02:33:44AM +0300, Alexander Popov wrote:
> Make STACKLEAK erase kernel stack after ptrace/seccomp/auditing
> not to leave any sensitive information on the stack for the syscall code.
> 
> This code is modified from Brad Spengler/PaX Team's code in the last
> public patch of grsecurity/PaX based on our understanding of the code.
> Changes or omissions from the original code are ours and don't reflect
> the original grsecurity/PaX code.
> 
> Signed-off-by: Alexander Popov <alex.popov@linux.com>
> ---
>  arch/x86/entry/common.c | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c
> index d7d3cc2..d45b7cf 100644
> --- a/arch/x86/entry/common.c
> +++ b/arch/x86/entry/common.c
> @@ -45,6 +45,12 @@ __visible inline void enter_from_user_mode(void)
>  static inline void enter_from_user_mode(void) {}
>  #endif
>  
> +#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
> +asmlinkage void erase_kstack(void);
> +#else
> +static void erase_kstack(void) {}
> +#endif
> +
>  static void do_audit_syscall_entry(struct pt_regs *regs, u32 arch)
>  {
>  #ifdef CONFIG_X86_64
> @@ -81,11 +87,15 @@ static long syscall_trace_enter(struct pt_regs *regs)
>  		emulated = true;
>  
>  	if ((emulated || (work & _TIF_SYSCALL_TRACE)) &&
> -	    tracehook_report_syscall_entry(regs))
> +	    tracehook_report_syscall_entry(regs)) {
> +		erase_kstack();
>  		return -1L;
> +	}
>  
> -	if (emulated)
> +	if (emulated) {
> +		erase_kstack();
>  		return -1L;
> +	}
>  
>  #ifdef CONFIG_SECCOMP
>  	/*
> @@ -117,8 +127,10 @@ static long syscall_trace_enter(struct pt_regs *regs)
>  		}
>  
>  		ret = __secure_computing(&sd);
> -		if (ret == -1)
> +		if (ret == -1) {
> +			erase_kstack();
>  			return ret;
> +		}
>  	}
>  #endif
>  
> @@ -127,6 +139,7 @@ static long syscall_trace_enter(struct pt_regs *regs)
>  
>  	do_audit_syscall_entry(regs, arch);
>  
> +	erase_kstack();
>  	return ret ?: regs->orig_ax;
>  }

wrt adding erase_kstack() calls to syscall_trace_enter(), I think the only
case where this would be appropriate is that still has a chance of
executing syscall code.  In all cases where syscall_trace_enter() returns
-1 no syscall code is going to be executed and the stack will be erased on
exiting syscall anyway.

In other words, only the last hunk of this patch seems to be useful,
all others look redundant.

P.S.  I've trimmed the Cc list to those who took part in earlier rounds
of this discussion.
Alexander Popov - Dec. 11, 2017, 10:38 p.m.
On 07.12.2017 00:12, Dmitry V. Levin wrote:
> On Wed, Dec 06, 2017 at 02:33:44AM +0300, Alexander Popov wrote:
>> Make STACKLEAK erase kernel stack after ptrace/seccomp/auditing
>> not to leave any sensitive information on the stack for the syscall code.
>>
>> This code is modified from Brad Spengler/PaX Team's code in the last
>> public patch of grsecurity/PaX based on our understanding of the code.
>> Changes or omissions from the original code are ours and don't reflect
>> the original grsecurity/PaX code.
>>
>> Signed-off-by: Alexander Popov <alex.popov@linux.com>
>> ---
>>  arch/x86/entry/common.c | 19 ++++++++++++++++---
>>  1 file changed, 16 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c
>> index d7d3cc2..d45b7cf 100644
>> --- a/arch/x86/entry/common.c
>> +++ b/arch/x86/entry/common.c
>> @@ -45,6 +45,12 @@ __visible inline void enter_from_user_mode(void)
>>  static inline void enter_from_user_mode(void) {}
>>  #endif
>>  
>> +#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
>> +asmlinkage void erase_kstack(void);
>> +#else
>> +static void erase_kstack(void) {}
>> +#endif
>> +
>>  static void do_audit_syscall_entry(struct pt_regs *regs, u32 arch)
>>  {
>>  #ifdef CONFIG_X86_64
>> @@ -81,11 +87,15 @@ static long syscall_trace_enter(struct pt_regs *regs)
>>  		emulated = true;
>>  
>>  	if ((emulated || (work & _TIF_SYSCALL_TRACE)) &&
>> -	    tracehook_report_syscall_entry(regs))
>> +	    tracehook_report_syscall_entry(regs)) {
>> +		erase_kstack();
>>  		return -1L;
>> +	}
>>  
>> -	if (emulated)
>> +	if (emulated) {
>> +		erase_kstack();
>>  		return -1L;
>> +	}
>>  
>>  #ifdef CONFIG_SECCOMP
>>  	/*
>> @@ -117,8 +127,10 @@ static long syscall_trace_enter(struct pt_regs *regs)
>>  		}
>>  
>>  		ret = __secure_computing(&sd);
>> -		if (ret == -1)
>> +		if (ret == -1) {
>> +			erase_kstack();
>>  			return ret;
>> +		}
>>  	}
>>  #endif
>>  
>> @@ -127,6 +139,7 @@ static long syscall_trace_enter(struct pt_regs *regs)
>>  
>>  	do_audit_syscall_entry(regs, arch);
>>  
>> +	erase_kstack();
>>  	return ret ?: regs->orig_ax;
>>  }

Hello Dmitry,

Thanks for the review.

> wrt adding erase_kstack() calls to syscall_trace_enter(), I think the only
> case where this would be appropriate is that still has a chance of
> executing syscall code.  In all cases where syscall_trace_enter() returns
> -1 no syscall code is going to be executed and the stack will be erased on
> exiting syscall anyway.
> 
> In other words, only the last hunk of this patch seems to be useful,
> all others look redundant.

I agree with your point. I'll fix it in v7.

> P.S.  I've trimmed the Cc list to those who took part in earlier rounds
> of this discussion.

Excuse me, I've returned everybody back to CC list again :)

Best regards,
Alexander

Patch

diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c
index d7d3cc2..d45b7cf 100644
--- a/arch/x86/entry/common.c
+++ b/arch/x86/entry/common.c
@@ -45,6 +45,12 @@  __visible inline void enter_from_user_mode(void)
 static inline void enter_from_user_mode(void) {}
 #endif
 
+#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
+asmlinkage void erase_kstack(void);
+#else
+static void erase_kstack(void) {}
+#endif
+
 static void do_audit_syscall_entry(struct pt_regs *regs, u32 arch)
 {
 #ifdef CONFIG_X86_64
@@ -81,11 +87,15 @@  static long syscall_trace_enter(struct pt_regs *regs)
 		emulated = true;
 
 	if ((emulated || (work & _TIF_SYSCALL_TRACE)) &&
-	    tracehook_report_syscall_entry(regs))
+	    tracehook_report_syscall_entry(regs)) {
+		erase_kstack();
 		return -1L;
+	}
 
-	if (emulated)
+	if (emulated) {
+		erase_kstack();
 		return -1L;
+	}
 
 #ifdef CONFIG_SECCOMP
 	/*
@@ -117,8 +127,10 @@  static long syscall_trace_enter(struct pt_regs *regs)
 		}
 
 		ret = __secure_computing(&sd);
-		if (ret == -1)
+		if (ret == -1) {
+			erase_kstack();
 			return ret;
+		}
 	}
 #endif
 
@@ -127,6 +139,7 @@  static long syscall_trace_enter(struct pt_regs *regs)
 
 	do_audit_syscall_entry(regs, arch);
 
+	erase_kstack();
 	return ret ?: regs->orig_ax;
 }