Re: [PATCH v7 22/26] x86/umip: Force a page fault when unable to copy emulated result to user

[Ricardo Neri]

On Fri, 2017-06-09 at 13:02 +0200, Borislav Petkov wrote:
> On Fri, May 05, 2017 at 11:17:20AM -0700, Ricardo Neri wrote:
> > fixup_umip_exception() will be called from do_general_protection. If the
> ^
> |
> Please end function names with parentheses.  ---+
> 
> > former returns false, the latter will issue a SIGSEGV with SEND_SIG_PRIV.
> > However, when emulation is successful but the emulated result cannot be
> > copied to user space memory, it is more accurate to issue a SIGSEGV with
> > SEGV_MAPERR with the offending address.
> > A new function is inspired in
> 
> That reads funny.

I will correct this.
> 
> > force_sig_info_fault is introduced to model the page fault.
> > 
> > Cc: Andy Lutomirski 
> > Cc: Andrew Morton 
> > Cc: H. Peter Anvin 
> > Cc: Borislav Petkov 
> > Cc: Brian Gerst 
> > Cc: Chen Yucong 
> > Cc: Chris Metcalf 
> > Cc: Dave Hansen 
> > Cc: Fenghua Yu 
> > Cc: Huang Rui 
> > Cc: Jiri Slaby 
> > Cc: Jonathan Corbet 
> > Cc: Michael S. Tsirkin 
> > Cc: Paul Gortmaker 
> > Cc: Peter Zijlstra 
> > Cc: Ravi V. Shankar 
> > Cc: Shuah Khan 
> > Cc: Vlastimil Babka 
> > Cc: Tony Luck 
> > Cc: Paolo Bonzini 
> > Cc: Liang Z. Li 
> > Cc: Alexandre Julliard 
> > Cc: Stas Sergeev 
> > Cc: x...@kernel.org
> > Cc: linux-msdos@vger.kernel.org
> > Signed-off-by: Ricardo Neri 
> > ---
> >  arch/x86/kernel/umip.c | 45 +++--
> >  1 file changed, 43 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/umip.c b/arch/x86/kernel/umip.c
> > index c7c5795..ff7366a 100644
> > --- a/arch/x86/kernel/umip.c
> > +++ b/arch/x86/kernel/umip.c
> > @@ -148,6 +148,41 @@ static int __emulate_umip_insn(struct insn *insn, enum 
> > umip_insn umip_inst,
> >  }
> >  
> >  /**
> > + * __force_sig_info_umip_fault() - Force a SIGSEGV with SEGV_MAPERR
> > + * @address:   Address that caused the signal
> > + * @regs:  Register set containing the instruction pointer
> > + *
> > + * Force a SIGSEGV signal with SEGV_MAPERR as the error code. This 
> > function is
> > + * intended to be used to provide a segmentation fault when the result of 
> > the
> > + * UMIP emulation could not be copied to the user space memory.
> > + *
> > + * Return: none
> > + */
> > +static void __force_sig_info_umip_fault(void __user *address,
> > +   struct pt_regs *regs)
> > +{
> > +   siginfo_t info;
> > +   struct task_struct *tsk = current;
> > +
> > +   if (show_unhandled_signals && unhandled_signal(tsk, SIGSEGV)) {
> 
> Save an indentation level:
> 
>   if (!(show_unhandled_signals && unhandled_signal(tsk, SIGSEGV)))
>   return;
> 
>   printk...
> 
I will implement like this.
> 
> 
> > +   printk_ratelimited("%s[%d] umip emulation segfault ip:%lx 
> > sp:%lx error:%x in %lx\n",
> > +  tsk->comm, task_pid_nr(tsk), regs->ip,
> > +  regs->sp, X86_PF_USER | X86_PF_WRITE,
> > +  regs->ip);
> > +   }
> > +
> > +   tsk->thread.cr2 = (unsigned long)address;
> > +   tsk->thread.error_code  = X86_PF_USER | X86_PF_WRITE;
> > +   tsk->thread.trap_nr = X86_TRAP_PF;
> > +
> > +   info.si_signo   = SIGSEGV;
> > +   info.si_errno   = 0;
> > +   info.si_code= SEGV_MAPERR;
> > +   info.si_addr= address;
> > +   force_sig_info(SIGSEGV, , tsk);
> > +}
> > +
> > +/**
> >   * fixup_umip_exception() - Fixup #GP faults caused by UMIP
> >   * @regs:  Registers as saved when entering the #GP trap
> >   *
> > @@ -235,8 +270,14 @@ bool fixup_umip_exception(struct pt_regs *regs)
> > if ((unsigned long)uaddr == -1L)
> > return false;
> > nr_copied = copy_to_user(uaddr, dummy_data, dummy_data_size);
> > -   if (nr_copied  > 0)
> > -   return false;
> > +   if (nr_copied  > 0) {
> > +   /*
> > +* If copy fails, send a signal and tell caller that
> > +* fault was fixed up
> 
> Pls end sentences in the comments with a fullstop.

I will correct this.

Thanks and BR,
Ricardo

--
To unsubscribe from this list: send the line "unsubscribe linux-msdos" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 22/26] x86/umip: Force a page fault when unable to copy emulated result to user

[Borislav Petkov]

On Fri, May 05, 2017 at 11:17:20AM -0700, Ricardo Neri wrote:
> fixup_umip_exception() will be called from do_general_protection. If the
  ^
  |
Please end function names with parentheses.---+

> former returns false, the latter will issue a SIGSEGV with SEND_SIG_PRIV.
> However, when emulation is successful but the emulated result cannot be
> copied to user space memory, it is more accurate to issue a SIGSEGV with
> SEGV_MAPERR with the offending address.
> A new function is inspired in

That reads funny.

> force_sig_info_fault is introduced to model the page fault.
> 
> Cc: Andy Lutomirski 
> Cc: Andrew Morton 
> Cc: H. Peter Anvin 
> Cc: Borislav Petkov 
> Cc: Brian Gerst 
> Cc: Chen Yucong 
> Cc: Chris Metcalf 
> Cc: Dave Hansen 
> Cc: Fenghua Yu 
> Cc: Huang Rui 
> Cc: Jiri Slaby 
> Cc: Jonathan Corbet 
> Cc: Michael S. Tsirkin 
> Cc: Paul Gortmaker 
> Cc: Peter Zijlstra 
> Cc: Ravi V. Shankar 
> Cc: Shuah Khan 
> Cc: Vlastimil Babka 
> Cc: Tony Luck 
> Cc: Paolo Bonzini 
> Cc: Liang Z. Li 
> Cc: Alexandre Julliard 
> Cc: Stas Sergeev 
> Cc: x...@kernel.org
> Cc: linux-msdos@vger.kernel.org
> Signed-off-by: Ricardo Neri 
> ---
>  arch/x86/kernel/umip.c | 45 +++--
>  1 file changed, 43 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/umip.c b/arch/x86/kernel/umip.c
> index c7c5795..ff7366a 100644
> --- a/arch/x86/kernel/umip.c
> +++ b/arch/x86/kernel/umip.c
> @@ -148,6 +148,41 @@ static int __emulate_umip_insn(struct insn *insn, enum 
> umip_insn umip_inst,
>  }
>  
>  /**
> + * __force_sig_info_umip_fault() - Force a SIGSEGV with SEGV_MAPERR
> + * @address: Address that caused the signal
> + * @regs:Register set containing the instruction pointer
> + *
> + * Force a SIGSEGV signal with SEGV_MAPERR as the error code. This function 
> is
> + * intended to be used to provide a segmentation fault when the result of the
> + * UMIP emulation could not be copied to the user space memory.
> + *
> + * Return: none
> + */
> +static void __force_sig_info_umip_fault(void __user *address,
> + struct pt_regs *regs)
> +{
> + siginfo_t info;
> + struct task_struct *tsk = current;
> +
> + if (show_unhandled_signals && unhandled_signal(tsk, SIGSEGV)) {

Save an indentation level:

if (!(show_unhandled_signals && unhandled_signal(tsk, SIGSEGV)))
return;

printk...



> + printk_ratelimited("%s[%d] umip emulation segfault ip:%lx 
> sp:%lx error:%x in %lx\n",
> +tsk->comm, task_pid_nr(tsk), regs->ip,
> +regs->sp, X86_PF_USER | X86_PF_WRITE,
> +regs->ip);
> + }
> +
> + tsk->thread.cr2 = (unsigned long)address;
> + tsk->thread.error_code  = X86_PF_USER | X86_PF_WRITE;
> + tsk->thread.trap_nr = X86_TRAP_PF;
> +
> + info.si_signo   = SIGSEGV;
> + info.si_errno   = 0;
> + info.si_code= SEGV_MAPERR;
> + info.si_addr= address;
> + force_sig_info(SIGSEGV, , tsk);
> +}
> +
> +/**
>   * fixup_umip_exception() - Fixup #GP faults caused by UMIP
>   * @regs:Registers as saved when entering the #GP trap
>   *
> @@ -235,8 +270,14 @@ bool fixup_umip_exception(struct pt_regs *regs)
>   if ((unsigned long)uaddr == -1L)
>   return false;
>   nr_copied = copy_to_user(uaddr, dummy_data, dummy_data_size);
> - if (nr_copied  > 0)
> - return false;
> + if (nr_copied  > 0) {
> + /*
> +  * If copy fails, send a signal and tell caller that
> +  * fault was fixed up

Pls end sentences in the comments with a fullstop.

> +  */
> + __force_sig_info_umip_fault(uaddr, regs);
> + return true;
> + }
>   }
>  
>   /* increase IP to let the program keep going */
> -- 
> 2.9.3
> 

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 
--
To unsubscribe from this list: send the line "unsubscribe linux-msdos" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  

[PATCH v7 22/26] x86/umip: Force a page fault when unable to copy emulated result to user

[Ricardo Neri]

fixup_umip_exception() will be called from do_general_protection. If the
former returns false, the latter will issue a SIGSEGV with SEND_SIG_PRIV.
However, when emulation is successful but the emulated result cannot be
copied to user space memory, it is more accurate to issue a SIGSEGV with
SEGV_MAPERR with the offending address. A new function is inspired in
force_sig_info_fault is introduced to model the page fault.

Cc: Andy Lutomirski 
Cc: Andrew Morton 
Cc: H. Peter Anvin 
Cc: Borislav Petkov 
Cc: Brian Gerst 
Cc: Chen Yucong 
Cc: Chris Metcalf 
Cc: Dave Hansen 
Cc: Fenghua Yu 
Cc: Huang Rui 
Cc: Jiri Slaby 
Cc: Jonathan Corbet 
Cc: Michael S. Tsirkin 
Cc: Paul Gortmaker 
Cc: Peter Zijlstra 
Cc: Ravi V. Shankar 
Cc: Shuah Khan 
Cc: Vlastimil Babka 
Cc: Tony Luck 
Cc: Paolo Bonzini 
Cc: Liang Z. Li 
Cc: Alexandre Julliard 
Cc: Stas Sergeev 
Cc: x...@kernel.org
Cc: linux-msdos@vger.kernel.org
Signed-off-by: Ricardo Neri 
---
 arch/x86/kernel/umip.c | 45 +++--
 1 file changed, 43 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/umip.c b/arch/x86/kernel/umip.c
index c7c5795..ff7366a 100644
--- a/arch/x86/kernel/umip.c
+++ b/arch/x86/kernel/umip.c
@@ -148,6 +148,41 @@ static int __emulate_umip_insn(struct insn *insn, enum 
umip_insn umip_inst,
 }
 
 /**
+ * __force_sig_info_umip_fault() - Force a SIGSEGV with SEGV_MAPERR
+ * @address:   Address that caused the signal
+ * @regs:  Register set containing the instruction pointer
+ *
+ * Force a SIGSEGV signal with SEGV_MAPERR as the error code. This function is
+ * intended to be used to provide a segmentation fault when the result of the
+ * UMIP emulation could not be copied to the user space memory.
+ *
+ * Return: none
+ */
+static void __force_sig_info_umip_fault(void __user *address,
+   struct pt_regs *regs)
+{
+   siginfo_t info;
+   struct task_struct *tsk = current;
+
+   if (show_unhandled_signals && unhandled_signal(tsk, SIGSEGV)) {
+   printk_ratelimited("%s[%d] umip emulation segfault ip:%lx 
sp:%lx error:%x in %lx\n",
+  tsk->comm, task_pid_nr(tsk), regs->ip,
+  regs->sp, X86_PF_USER | X86_PF_WRITE,
+  regs->ip);
+   }
+
+   tsk->thread.cr2 = (unsigned long)address;
+   tsk->thread.error_code  = X86_PF_USER | X86_PF_WRITE;
+   tsk->thread.trap_nr = X86_TRAP_PF;
+
+   info.si_signo   = SIGSEGV;
+   info.si_errno   = 0;
+   info.si_code= SEGV_MAPERR;
+   info.si_addr= address;
+   force_sig_info(SIGSEGV, , tsk);
+}
+
+/**
  * fixup_umip_exception() - Fixup #GP faults caused by UMIP
  * @regs:  Registers as saved when entering the #GP trap
  *
@@ -235,8 +270,14 @@ bool fixup_umip_exception(struct pt_regs *regs)
if ((unsigned long)uaddr == -1L)
return false;
nr_copied = copy_to_user(uaddr, dummy_data, dummy_data_size);
-   if (nr_copied  > 0)
-   return false;
+   if (nr_copied  > 0) {
+   /*
+* If copy fails, send a signal and tell caller that
+* fault was fixed up
+*/
+   __force_sig_info_umip_fault(uaddr, regs);
+   return true;
+   }
}
 
/* increase IP to let the program keep going */
-- 
2.9.3

--
To unsubscribe from this list: send the line "unsubscribe linux-msdos" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html