diff mbox

[RFC,v2,6/9] Creation of "pagefault_handler_x86" LSM hook

Message ID 1497544976-7856-7-git-send-email-s.mesoraca16@gmail.com
State Superseded
Headers show

Commit Message

Salvatore Mesoraca June 15, 2017, 4:42 p.m. UTC
Creation of a new hook to let LSM modules handle user-space pagefaults on
x86.
It can be used to avoid segfaulting the originating process.
If it's the case it can modify process registers before returning.
This is not a security feature by itself, it's a way to soften some
unwanted side-effects of restrictive security features.
In particular this is used by S.A.R.A. can be used to implement what
PaX call "trampoline emulation" that, in practice, allow for some specific
code sequences to be executed even if they are in non executable memory.
This may look like a bad thing at first, but you have to consider
that:
- This allows for strict memory restrictions (e.g. W^X) to stay on even
  when they should be turned off. And, even if this emulation
  makes those features less effective, it's still better than having
  them turned off completely.
- The only code sequences emulated are trampolines used to make
  function calls. In many cases, when you have the chance to
  make arbitrary memory writes, you can already manipulate the
  control flow of the program by overwriting function pointers or
  return values. So, in many cases, the "trampoline emulation"
  doesn't introduce new exploit vectors.
- It's a feature that can be turned on only if needed, on a per
  executable file basis.

Signed-off-by: Salvatore Mesoraca <s.mesoraca16@gmail.com>
---
 arch/x86/mm/fault.c       |  6 ++++++
 include/linux/lsm_hooks.h |  9 +++++++++
 include/linux/security.h  | 11 +++++++++++
 security/security.c       | 11 +++++++++++
 4 files changed, 37 insertions(+)

Comments

Kees Cook June 27, 2017, 11:07 p.m. UTC | #1
On Thu, Jun 15, 2017 at 9:42 AM, Salvatore Mesoraca
<s.mesoraca16@gmail.com> wrote:
> Creation of a new hook to let LSM modules handle user-space pagefaults on
> x86.
> It can be used to avoid segfaulting the originating process.
> If it's the case it can modify process registers before returning.
> This is not a security feature by itself, it's a way to soften some
> unwanted side-effects of restrictive security features.
> In particular this is used by S.A.R.A. can be used to implement what
> PaX call "trampoline emulation" that, in practice, allow for some specific
> code sequences to be executed even if they are in non executable memory.
> This may look like a bad thing at first, but you have to consider
> that:
> - This allows for strict memory restrictions (e.g. W^X) to stay on even
>   when they should be turned off. And, even if this emulation
>   makes those features less effective, it's still better than having
>   them turned off completely.
> - The only code sequences emulated are trampolines used to make
>   function calls. In many cases, when you have the chance to
>   make arbitrary memory writes, you can already manipulate the
>   control flow of the program by overwriting function pointers or
>   return values. So, in many cases, the "trampoline emulation"
>   doesn't introduce new exploit vectors.
> - It's a feature that can be turned on only if needed, on a per
>   executable file basis.

Can this be made arch-agnostic? It seems a per-arch register-handling
routine would be needed, though. :(

-Kees

>
> Signed-off-by: Salvatore Mesoraca <s.mesoraca16@gmail.com>
> ---
>  arch/x86/mm/fault.c       |  6 ++++++
>  include/linux/lsm_hooks.h |  9 +++++++++
>  include/linux/security.h  | 11 +++++++++++
>  security/security.c       | 11 +++++++++++
>  4 files changed, 37 insertions(+)
>
> diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
> index 8ad91a0..b75b81a 100644
> --- a/arch/x86/mm/fault.c
> +++ b/arch/x86/mm/fault.c
> @@ -15,6 +15,7 @@
>  #include <linux/prefetch.h>            /* prefetchw                    */
>  #include <linux/context_tracking.h>    /* exception_enter(), ...       */
>  #include <linux/uaccess.h>             /* faulthandler_disabled()      */
> +#include <linux/security.h>            /* security_pagefault_handler   */
>
>  #include <asm/cpufeature.h>            /* boot_cpu_has, ...            */
>  #include <asm/traps.h>                 /* dotraplinkage, ...           */
> @@ -1358,6 +1359,11 @@ static inline bool smap_violation(int error_code, struct pt_regs *regs)
>                         local_irq_enable();
>         }
>
> +       if (unlikely(security_pagefault_handler_x86(regs,
> +                                                   error_code,
> +                                                   address)))
> +               return;
> +
>         perf_sw_event(PERF_COUNT_SW_PAGE_FAULTS, 1, regs, address);
>
>         if (error_code & PF_WRITE)
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 33dab16..da487e5 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -488,6 +488,11 @@
>   *     @vmflags contains requested the vmflags.
>   *     Return 0 if the operation is allowed to continue otherwise return
>   *     the appropriate error code.
> + * @pagefault_handler_x86:
> + *     Handle pagefaults on x86.
> + *     @regs contains process' registers.
> + *     @error_code contains error code for the pagefault.
> + *     @address contains the address that caused the pagefault.
>   * @file_lock:
>   *     Check permission before performing file locking operations.
>   *     Note: this hook mediates both flock and fcntl style locks.
> @@ -1483,6 +1488,9 @@
>         int (*file_mprotect)(struct vm_area_struct *vma, unsigned long reqprot,
>                                 unsigned long prot);
>         int (*check_vmflags)(vm_flags_t vmflags);
> +       int (*pagefault_handler_x86)(struct pt_regs *regs,
> +                                    unsigned long error_code,
> +                                    unsigned long address);
>         int (*file_lock)(struct file *file, unsigned int cmd);
>         int (*file_fcntl)(struct file *file, unsigned int cmd,
>                                 unsigned long arg);
> @@ -1754,6 +1762,7 @@ struct security_hook_heads {
>         struct list_head mmap_file;
>         struct list_head file_mprotect;
>         struct list_head check_vmflags;
> +       struct list_head pagefault_handler_x86;
>         struct list_head file_lock;
>         struct list_head file_fcntl;
>         struct list_head file_set_fowner;
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 8701872..3b91999 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -301,6 +301,9 @@ int security_mmap_file(struct file *file, unsigned long prot,
>  int security_file_mprotect(struct vm_area_struct *vma, unsigned long reqprot,
>                            unsigned long prot);
>  int security_check_vmflags(vm_flags_t vmflags);
> +int __maybe_unused security_pagefault_handler_x86(struct pt_regs *regs,
> +                                                 unsigned long error_code,
> +                                                 unsigned long address);
>  int security_file_lock(struct file *file, unsigned int cmd);
>  int security_file_fcntl(struct file *file, unsigned int cmd, unsigned long arg);
>  void security_file_set_fowner(struct file *file);
> @@ -829,6 +832,14 @@ static inline int security_check_vmflags(vm_flags_t vmflags)
>         return 0;
>  }
>
> +static inline int __maybe_unused security_pagefault_handler_x86(
> +                                               struct pt_regs *regs,
> +                                               unsigned long error_code,
> +                                               unsigned long address)
> +{
> +       return 0;
> +}
> +
>  static inline int security_file_lock(struct file *file, unsigned int cmd)
>  {
>         return 0;
> diff --git a/security/security.c b/security/security.c
> index 7e45846..f7df697 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -905,6 +905,17 @@ int security_check_vmflags(vm_flags_t vmflags)
>         return call_int_hook(check_vmflags, 0, vmflags);
>  }
>
> +int __maybe_unused security_pagefault_handler_x86(struct pt_regs *regs,
> +                                                 unsigned long error_code,
> +                                                 unsigned long address)
> +{
> +       return call_int_hook(pagefault_handler_x86,
> +                            0,
> +                            regs,
> +                            error_code,
> +                            address);
> +}
> +
>  int security_file_lock(struct file *file, unsigned int cmd)
>  {
>         return call_int_hook(file_lock, 0, file, cmd);
> --
> 1.9.1
>
Salvatore Mesoraca June 29, 2017, 7:30 p.m. UTC | #2
2017-06-28 1:07 GMT+02:00 Kees Cook <keescook@chromium.org>:
> On Thu, Jun 15, 2017 at 9:42 AM, Salvatore Mesoraca
> <s.mesoraca16@gmail.com> wrote:
>> Creation of a new hook to let LSM modules handle user-space pagefaults on
>> x86.
>> It can be used to avoid segfaulting the originating process.
>> If it's the case it can modify process registers before returning.
>> This is not a security feature by itself, it's a way to soften some
>> unwanted side-effects of restrictive security features.
>> In particular this is used by S.A.R.A. can be used to implement what
>> PaX call "trampoline emulation" that, in practice, allow for some specific
>> code sequences to be executed even if they are in non executable memory.
>> This may look like a bad thing at first, but you have to consider
>> that:
>> - This allows for strict memory restrictions (e.g. W^X) to stay on even
>>   when they should be turned off. And, even if this emulation
>>   makes those features less effective, it's still better than having
>>   them turned off completely.
>> - The only code sequences emulated are trampolines used to make
>>   function calls. In many cases, when you have the chance to
>>   make arbitrary memory writes, you can already manipulate the
>>   control flow of the program by overwriting function pointers or
>>   return values. So, in many cases, the "trampoline emulation"
>>   doesn't introduce new exploit vectors.
>> - It's a feature that can be turned on only if needed, on a per
>>   executable file basis.
>
> Can this be made arch-agnostic? It seems a per-arch register-handling
> routine would be needed, though. :(

S.A.R.A.'s "pagefault_handler_x86" implementation is fully arch specific
so it won't benefit too much from this change.
Anyway having a single hook for all archs is probably a cleaner solution,
I'll change it in the v3.
Would it be OK if I make it arch-agnostic while I actually keep it only
in arch/x86/mm/fault.c?
Thank you for your help.

Salvatore
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Kees Cook June 29, 2017, 8:20 p.m. UTC | #3
On Thu, Jun 29, 2017 at 12:30 PM, Salvatore Mesoraca
<s.mesoraca16@gmail.com> wrote:
> 2017-06-28 1:07 GMT+02:00 Kees Cook <keescook@chromium.org>:
>> On Thu, Jun 15, 2017 at 9:42 AM, Salvatore Mesoraca
>> <s.mesoraca16@gmail.com> wrote:
>>> Creation of a new hook to let LSM modules handle user-space pagefaults on
>>> x86.
>>> It can be used to avoid segfaulting the originating process.
>>> If it's the case it can modify process registers before returning.
>>> This is not a security feature by itself, it's a way to soften some
>>> unwanted side-effects of restrictive security features.
>>> In particular this is used by S.A.R.A. can be used to implement what
>>> PaX call "trampoline emulation" that, in practice, allow for some specific
>>> code sequences to be executed even if they are in non executable memory.
>>> This may look like a bad thing at first, but you have to consider
>>> that:
>>> - This allows for strict memory restrictions (e.g. W^X) to stay on even
>>>   when they should be turned off. And, even if this emulation
>>>   makes those features less effective, it's still better than having
>>>   them turned off completely.
>>> - The only code sequences emulated are trampolines used to make
>>>   function calls. In many cases, when you have the chance to
>>>   make arbitrary memory writes, you can already manipulate the
>>>   control flow of the program by overwriting function pointers or
>>>   return values. So, in many cases, the "trampoline emulation"
>>>   doesn't introduce new exploit vectors.
>>> - It's a feature that can be turned on only if needed, on a per
>>>   executable file basis.
>>
>> Can this be made arch-agnostic? It seems a per-arch register-handling
>> routine would be needed, though. :(
>
> S.A.R.A.'s "pagefault_handler_x86" implementation is fully arch specific
> so it won't benefit too much from this change.
> Anyway having a single hook for all archs is probably a cleaner solution,
> I'll change it in the v3.
> Would it be OK if I make it arch-agnostic while I actually keep it only
> in arch/x86/mm/fault.c?
> Thank you for your help.

It'd be nicer to wire it up unconditionally to all architectures, but
I'm not entirely sure if that's feasible. Perhaps SARA (or this LSM
hook) would be hidden behind some CONFIG_ARCH_HAS_LSM_PAGEFAULT or
something that each architecture could "select" in its Kconfig.

Perhaps some other LSM folks have some better ideas?

-Kees
diff mbox

Patch

diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
index 8ad91a0..b75b81a 100644
--- a/arch/x86/mm/fault.c
+++ b/arch/x86/mm/fault.c
@@ -15,6 +15,7 @@ 
 #include <linux/prefetch.h>		/* prefetchw			*/
 #include <linux/context_tracking.h>	/* exception_enter(), ...	*/
 #include <linux/uaccess.h>		/* faulthandler_disabled()	*/
+#include <linux/security.h>		/* security_pagefault_handler	*/
 
 #include <asm/cpufeature.h>		/* boot_cpu_has, ...		*/
 #include <asm/traps.h>			/* dotraplinkage, ...		*/
@@ -1358,6 +1359,11 @@  static inline bool smap_violation(int error_code, struct pt_regs *regs)
 			local_irq_enable();
 	}
 
+	if (unlikely(security_pagefault_handler_x86(regs,
+						    error_code,
+						    address)))
+		return;
+
 	perf_sw_event(PERF_COUNT_SW_PAGE_FAULTS, 1, regs, address);
 
 	if (error_code & PF_WRITE)
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 33dab16..da487e5 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -488,6 +488,11 @@ 
  *	@vmflags contains requested the vmflags.
  *	Return 0 if the operation is allowed to continue otherwise return
  *	the appropriate error code.
+ * @pagefault_handler_x86:
+ *	Handle pagefaults on x86.
+ *	@regs contains process' registers.
+ *	@error_code contains error code for the pagefault.
+ *	@address contains the address that caused the pagefault.
  * @file_lock:
  *	Check permission before performing file locking operations.
  *	Note: this hook mediates both flock and fcntl style locks.
@@ -1483,6 +1488,9 @@ 
 	int (*file_mprotect)(struct vm_area_struct *vma, unsigned long reqprot,
 				unsigned long prot);
 	int (*check_vmflags)(vm_flags_t vmflags);
+	int (*pagefault_handler_x86)(struct pt_regs *regs,
+				     unsigned long error_code,
+				     unsigned long address);
 	int (*file_lock)(struct file *file, unsigned int cmd);
 	int (*file_fcntl)(struct file *file, unsigned int cmd,
 				unsigned long arg);
@@ -1754,6 +1762,7 @@  struct security_hook_heads {
 	struct list_head mmap_file;
 	struct list_head file_mprotect;
 	struct list_head check_vmflags;
+	struct list_head pagefault_handler_x86;
 	struct list_head file_lock;
 	struct list_head file_fcntl;
 	struct list_head file_set_fowner;
diff --git a/include/linux/security.h b/include/linux/security.h
index 8701872..3b91999 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -301,6 +301,9 @@  int security_mmap_file(struct file *file, unsigned long prot,
 int security_file_mprotect(struct vm_area_struct *vma, unsigned long reqprot,
 			   unsigned long prot);
 int security_check_vmflags(vm_flags_t vmflags);
+int __maybe_unused security_pagefault_handler_x86(struct pt_regs *regs,
+						  unsigned long error_code,
+						  unsigned long address);
 int security_file_lock(struct file *file, unsigned int cmd);
 int security_file_fcntl(struct file *file, unsigned int cmd, unsigned long arg);
 void security_file_set_fowner(struct file *file);
@@ -829,6 +832,14 @@  static inline int security_check_vmflags(vm_flags_t vmflags)
 	return 0;
 }
 
+static inline int __maybe_unused security_pagefault_handler_x86(
+						struct pt_regs *regs,
+						unsigned long error_code,
+						unsigned long address)
+{
+	return 0;
+}
+
 static inline int security_file_lock(struct file *file, unsigned int cmd)
 {
 	return 0;
diff --git a/security/security.c b/security/security.c
index 7e45846..f7df697 100644
--- a/security/security.c
+++ b/security/security.c
@@ -905,6 +905,17 @@  int security_check_vmflags(vm_flags_t vmflags)
 	return call_int_hook(check_vmflags, 0, vmflags);
 }
 
+int __maybe_unused security_pagefault_handler_x86(struct pt_regs *regs,
+						  unsigned long error_code,
+						  unsigned long address)
+{
+	return call_int_hook(pagefault_handler_x86,
+			     0,
+			     regs,
+			     error_code,
+			     address);
+}
+
 int security_file_lock(struct file *file, unsigned int cmd)
 {
 	return call_int_hook(file_lock, 0, file, cmd);