what is the right behavior of copy_to_user(0x0, ..., ...)?

what is the right behavior of copy_to_user(0x0, ..., ...)?

[Jun Sun]

When running LTP, I notice that recent kernel has a kernel access fault:

<1>Unable to handle kernel paging request at virtual address 00000000, epc
== 80273860, ra == 80205aa4
Oops in fault.c:do_page_fault, line 204:
$0 : 00000000 10001f00 00000002 00000002 00000000 86df5e98 00000001 00000040
$8 : 00000000 00000000 00000001 ffffffff 00000002 802b4864 00000001 00000001
$16: 100003d8 00000000 00000002 86df5e98 00401080 10002df8 00000000 00000097
$24: 0000000a 802e7ab6                   86df4000 86df5e60 7fff7c60 80205aa4
Hi : 00000000
Lo : 00000000
epc  : 80273860    Not tainted
Status: 10001f03
Cause : 9080800c
  ....

Tracing error reveals that user process passed a NULL buffer pointer to 
sys_getpeername() syscall, probably intentionally.  Then it goes all the way 
down to copy_to_user(0x0, ..., ...) and caused a oops as above.

As a result of oops the user process is killed.  However I am not sure if this 
is the right way to respond to an ill argument.  copy_to_user() probably 
should catch this case and return some meaningful error back to the caller.

I am not sure what is the best way to achieve this.  Any thoughts?

Jun

Re: what is the right behavior of copy_to_user(0x0, ..., ...)?

[Ralf Baechle]

On Fri, May 03, 2002 at 02:46:19PM -0700, Jun Sun wrote:

> When running LTP, I notice that recent kernel has a kernel access fault:
> 
> <1>Unable to handle kernel paging request at virtual address 00000000, epc
> == 80273860, ra == 80205aa4

Well, decode the oops message.  The question is what is at 0x80273860?

  Ralf

Re: what is the right behavior of copy_to_user(0x0, ..., ...)?

[Jun Sun]

Ralf Baechle wrote:

> On Fri, May 03, 2002 at 02:46:19PM -0700, Jun Sun wrote:
> 
> 
>>When running LTP, I notice that recent kernel has a kernel access fault:
>>
>><1>Unable to handle kernel paging request at virtual address 00000000, epc
>>== 80273860, ra == 80205aa4
>>
> 
> Well, decode the oops message.  The question is what is at 0x80273860?
> 


0x80273860 is copy_bytes in arch/mips/lib/memcpy.S, which is reached through __copy_user.

The faulting instruction, not suprisingly, is writing a byte to the 
destination at 0x0.

Anybody can try to call copy_to_user(0x0, ...) inside kernel and see the 
scene.  The question here is whether we should reach do_page_fault() and 
terminate calling process or try to catch the fault and return some meaningful 
error.

It appears earlier version of kernel does not have this problem.  I have not 
fully figured out why.

Jun

Re: what is the right behavior of copy_to_user(0x0, ..., ...)?

[Ralf Baechle]

On Fri, May 03, 2002 at 04:41:56PM -0700, Jun Sun wrote:

> It appears earlier version of kernel does not have this problem.  I have not 
> fully figured out why.

We didn't handle exceptions in branch delay slots.  Try this patch and
tell me if it helps.

  Ralf

Index: arch/mips/mm/fault.c
===================================================================
RCS file: /home/pub/cvs/linux/arch/mips/mm/fault.c,v
retrieving revision 1.25.2.2
diff -u -r1.25.2.2 fault.c
--- arch/mips/mm/fault.c	16 Jan 2002 03:49:24 -0000	1.25.2.2
+++ arch/mips/mm/fault.c	4 May 2002 01:28:34 -0000
@@ -19,6 +19,7 @@
 #include <linux/smp_lock.h>
 #include <linux/version.h>
 
+#include <asm/branch.h>
 #include <asm/hardirq.h>
 #include <asm/pgalloc.h>
 #include <asm/mmu_context.h>
@@ -77,7 +78,7 @@
 	struct vm_area_struct * vma;
 	struct task_struct *tsk = current;
 	struct mm_struct *mm = tsk->mm;
-	unsigned long fixup;
+	unsigned long epc, fixup;
 	siginfo_t info;
 
 	/*
@@ -181,7 +182,8 @@
 
 no_context:
 	/* Are we prepared to handle this kernel fault?  */
-	fixup = search_exception_table(regs->cp0_epc);
+	epc = regs->cp0_epc + delay_slot(regs) ? 4 : 0;
+	fixup = search_exception_table(epc);
 	if (fixup) {
 		long new_epc;
 
Index: arch/mips64/mm/fault.c
===================================================================
RCS file: /home/pub/cvs/linux/arch/mips64/mm/fault.c,v
retrieving revision 1.26.2.6
diff -u -r1.26.2.6 fault.c
--- arch/mips64/mm/fault.c	23 Feb 2002 02:16:42 -0000	1.26.2.6
+++ arch/mips64/mm/fault.c	4 May 2002 01:28:34 -0000
@@ -21,6 +21,7 @@
 #include <linux/smp_lock.h>
 #include <linux/version.h>
 
+#include <asm/branch.h>
 #include <asm/hardirq.h>
 #include <asm/pgalloc.h>
 #include <asm/mmu_context.h>
@@ -103,7 +104,7 @@
 	struct vm_area_struct * vma;
 	struct task_struct *tsk = current;
 	struct mm_struct *mm = tsk->mm;
-	unsigned long fixup;
+	unsigned long epc, fixup;
 	siginfo_t info;
 
 #if 0
@@ -208,7 +209,8 @@
 
 no_context:
 	/* Are we prepared to handle this kernel fault?  */
-	fixup = search_exception_table(regs->cp0_epc);
+	epc = regs->cp0_epc + delay_slot(regs) ? 4 : 0;
+	fixup = search_exception_table(epc);
 	if (fixup) {
 		long new_epc;
 

Re: what is the right behavior of copy_to_user(0x0, ..., ...)?

[Ralf Baechle]

On Tue, May 07, 2002 at 10:47:56AM +0200, Carsten Langgaard wrote:

> It fix a problem I have had for quite a while in the r4k_fpu.S. The code in
> question is:
>         jr      ra
>         .set    nomacro
>          EX(sw  t0,SC_FPC_EIR(a0))
>         .set    macro
> 
> I have fixed it locally by removing the SW from the delay-slot, but obviously
> your fix is the right one.
> But I guess we need the same fix in arch/mips/kernel/unaligned.c.

Good spotting.  I'll use a slightly different fix using the new inline
function exception_epc() in <asm/branch.h> to implement that slightly
more elegant.

Thanks,

  Ralf

Re: what is the right behavior of copy_to_user(0x0, ..., ...)?

[Jun Sun]

[-- Attachment #1: Type: text/plain, Size: 2450 bytes --]


It would help if not for the gross typo. :-)  See the attachment.

Jun


Ralf Baechle wrote:

> On Fri, May 03, 2002 at 04:41:56PM -0700, Jun Sun wrote:
> 
> 
>>It appears earlier version of kernel does not have this problem.  I have not 
>>fully figured out why.
>>
> 
> We didn't handle exceptions in branch delay slots.  Try this patch and
> tell me if it helps.
> 
>   Ralf
> 
> Index: arch/mips/mm/fault.c
> ===================================================================
> RCS file: /home/pub/cvs/linux/arch/mips/mm/fault.c,v
> retrieving revision 1.25.2.2
> diff -u -r1.25.2.2 fault.c
> --- arch/mips/mm/fault.c	16 Jan 2002 03:49:24 -0000	1.25.2.2
> +++ arch/mips/mm/fault.c	4 May 2002 01:28:34 -0000
> @@ -19,6 +19,7 @@
>  #include <linux/smp_lock.h>
>  #include <linux/version.h>
>  
> +#include <asm/branch.h>
>  #include <asm/hardirq.h>
>  #include <asm/pgalloc.h>
>  #include <asm/mmu_context.h>
> @@ -77,7 +78,7 @@
>  	struct vm_area_struct * vma;
>  	struct task_struct *tsk = current;
>  	struct mm_struct *mm = tsk->mm;
> -	unsigned long fixup;
> +	unsigned long epc, fixup;
>  	siginfo_t info;
>  
>  	/*
> @@ -181,7 +182,8 @@
>  
>  no_context:
>  	/* Are we prepared to handle this kernel fault?  */
> -	fixup = search_exception_table(regs->cp0_epc);
> +	epc = regs->cp0_epc + delay_slot(regs) ? 4 : 0;
> +	fixup = search_exception_table(epc);
>  	if (fixup) {
>  		long new_epc;
>  
> Index: arch/mips64/mm/fault.c
> ===================================================================
> RCS file: /home/pub/cvs/linux/arch/mips64/mm/fault.c,v
> retrieving revision 1.26.2.6
> diff -u -r1.26.2.6 fault.c
> --- arch/mips64/mm/fault.c	23 Feb 2002 02:16:42 -0000	1.26.2.6
> +++ arch/mips64/mm/fault.c	4 May 2002 01:28:34 -0000
> @@ -21,6 +21,7 @@
>  #include <linux/smp_lock.h>
>  #include <linux/version.h>
>  
> +#include <asm/branch.h>
>  #include <asm/hardirq.h>
>  #include <asm/pgalloc.h>
>  #include <asm/mmu_context.h>
> @@ -103,7 +104,7 @@
>  	struct vm_area_struct * vma;
>  	struct task_struct *tsk = current;
>  	struct mm_struct *mm = tsk->mm;
> -	unsigned long fixup;
> +	unsigned long epc, fixup;
>  	siginfo_t info;
>  
>  #if 0
> @@ -208,7 +209,8 @@
>  
>  no_context:
>  	/* Are we prepared to handle this kernel fault?  */
> -	fixup = search_exception_table(regs->cp0_epc);
> +	epc = regs->cp0_epc + delay_slot(regs) ? 4 : 0;
> +	fixup = search_exception_table(epc);
>  	if (fixup) {
>  		long new_epc;
>  
> 


[-- Attachment #2: junk --]
[-- Type: text/plain, Size: 896 bytes --]

diff -Nru link/arch/mips/mm/fault.c.orig link/arch/mips/mm/fault.c
--- link/arch/mips/mm/fault.c.orig	Mon May  6 11:12:41 2002
+++ link/arch/mips/mm/fault.c	Mon May  6 11:15:12 2002
@@ -182,7 +182,7 @@
 
 no_context:
 	/* Are we prepared to handle this kernel fault?  */
-	epc = regs->cp0_epc + delay_slot(regs) ? 4 : 0;
+	epc = regs->cp0_epc + (delay_slot(regs) ? 4 : 0);
 	fixup = search_exception_table(epc);
 	if (fixup) {
 		long new_epc;
diff -Nru link/arch/mips64/mm/fault.c.orig link/arch/mips64/mm/fault.c
--- link/arch/mips64/mm/fault.c.orig	Mon May  6 11:12:44 2002
+++ link/arch/mips64/mm/fault.c	Mon May  6 11:15:26 2002
@@ -209,7 +209,7 @@
 
 no_context:
 	/* Are we prepared to handle this kernel fault?  */
-	epc = regs->cp0_epc + delay_slot(regs) ? 4 : 0;
+	epc = regs->cp0_epc + (delay_slot(regs) ? 4 : 0);
 	fixup = search_exception_table(epc);
 	if (fixup) {
 		long new_epc;

Re: what is the right behavior of copy_to_user(0x0, ..., ...)?

[Ralf Baechle]

On Tue, May 07, 2002 at 10:47:56AM +0200, Carsten Langgaard wrote:

> I have fixed it locally by removing the SW from the delay-slot, but obviously
> your fix is the right one.
> But I guess we need the same fix in arch/mips/kernel/unaligned.c.

Smoke this:

Index: arch/mips64/kernel/unaligned.c
===================================================================
RCS file: /home/pub/cvs/linux/arch/mips64/kernel/unaligned.c,v
retrieving revision 1.6.2.3
diff -u -r1.6.2.3 unaligned.c
--- arch/mips64/kernel/unaligned.c	24 Apr 2002 07:58:54 -0000	1.6.2.3
+++ arch/mips64/kernel/unaligned.c	7 May 2002 10:29:05 -0000
@@ -351,7 +351,7 @@
 
 fault:
 	/* Did we have an exception handler installed? */
-	fixup = search_exception_table(regs->cp0_epc);
+	fixup = search_exception_table(exception_epc(regs));
 	if (fixup) {
 		long new_epc;
 		new_epc = fixup_exception(dpf_reg, fixup, regs->cp0_epc);
Index: arch/mips/kernel/unaligned.c
===================================================================
RCS file: /home/pub/cvs/linux/arch/mips/kernel/unaligned.c,v
retrieving revision 1.15.2.4
diff -u -r1.15.2.4 unaligned.c
--- arch/mips/kernel/unaligned.c	24 Apr 2002 07:50:26 -0000	1.15.2.4
+++ arch/mips/kernel/unaligned.c	7 May 2002 10:29:05 -0000
@@ -332,7 +332,7 @@
 
 fault:
 	/* Did we have an exception handler installed? */
-	fixup = search_exception_table(regs->cp0_epc);
+	fixup = search_exception_table(exception_epc(regs));
 	if (fixup) {
 		long new_epc;
 		new_epc = fixup_exception(dpf_reg, fixup, regs->cp0_epc);

  Ralf

Re: what is the right behavior of copy_to_user(0x0, ..., ...)?

[Carsten Langgaard]

Ralf Baechle wrote:

> On Fri, May 03, 2002 at 04:41:56PM -0700, Jun Sun wrote:
>
> > It appears earlier version of kernel does not have this problem.  I have not
> > fully figured out why.
>
> We didn't handle exceptions in branch delay slots.  Try this patch and
> tell me if it helps.

It fix a problem I have had for quite a while in the r4k_fpu.S. The code in
question is:
        jr      ra
        .set    nomacro
         EX(sw  t0,SC_FPC_EIR(a0))
        .set    macro

I have fixed it locally by removing the SW from the delay-slot, but obviously
your fix is the right one.
But I guess we need the same fix in arch/mips/kernel/unaligned.c.


>
>   Ralf
>
> Index: arch/mips/mm/fault.c
> ===================================================================
> RCS file: /home/pub/cvs/linux/arch/mips/mm/fault.c,v
> retrieving revision 1.25.2.2
> diff -u -r1.25.2.2 fault.c
> --- arch/mips/mm/fault.c        16 Jan 2002 03:49:24 -0000      1.25.2.2
> +++ arch/mips/mm/fault.c        4 May 2002 01:28:34 -0000
> @@ -19,6 +19,7 @@
>  #include <linux/smp_lock.h>
>  #include <linux/version.h>
>
> +#include <asm/branch.h>
>  #include <asm/hardirq.h>
>  #include <asm/pgalloc.h>
>  #include <asm/mmu_context.h>
> @@ -77,7 +78,7 @@
>         struct vm_area_struct * vma;
>         struct task_struct *tsk = current;
>         struct mm_struct *mm = tsk->mm;
> -       unsigned long fixup;
> +       unsigned long epc, fixup;
>         siginfo_t info;
>
>         /*
> @@ -181,7 +182,8 @@
>
>  no_context:
>         /* Are we prepared to handle this kernel fault?  */
> -       fixup = search_exception_table(regs->cp0_epc);
> +       epc = regs->cp0_epc + delay_slot(regs) ? 4 : 0;
> +       fixup = search_exception_table(epc);
>         if (fixup) {
>                 long new_epc;
>
> Index: arch/mips64/mm/fault.c
> ===================================================================
> RCS file: /home/pub/cvs/linux/arch/mips64/mm/fault.c,v
> retrieving revision 1.26.2.6
> diff -u -r1.26.2.6 fault.c
> --- arch/mips64/mm/fault.c      23 Feb 2002 02:16:42 -0000      1.26.2.6
> +++ arch/mips64/mm/fault.c      4 May 2002 01:28:34 -0000
> @@ -21,6 +21,7 @@
>  #include <linux/smp_lock.h>
>  #include <linux/version.h>
>
> +#include <asm/branch.h>
>  #include <asm/hardirq.h>
>  #include <asm/pgalloc.h>
>  #include <asm/mmu_context.h>
> @@ -103,7 +104,7 @@
>         struct vm_area_struct * vma;
>         struct task_struct *tsk = current;
>         struct mm_struct *mm = tsk->mm;
> -       unsigned long fixup;
> +       unsigned long epc, fixup;
>         siginfo_t info;
>
>  #if 0
> @@ -208,7 +209,8 @@
>
>  no_context:
>         /* Are we prepared to handle this kernel fault?  */
> -       fixup = search_exception_table(regs->cp0_epc);
> +       epc = regs->cp0_epc + delay_slot(regs) ? 4 : 0;
> +       fixup = search_exception_table(epc);
>         if (fixup) {
>                 long new_epc;
>

--
_    _ ____  ___   Carsten Langgaard   Mailto:carstenl@mips.com
|\  /|||___)(___   MIPS Denmark        Direct: +45 4486 5527
| \/ |||    ____)  Lautrupvang 4B      Switch: +45 4486 5555
  TECHNOLOGIES     2750 Ballerup       Fax...: +45 4486 5556
                   Denmark             http://www.mips.com

Re: what is the right behavior of copy_to_user(0x0, ..., ...)?

[Ralf Baechle]

On Mon, May 06, 2002 at 11:18:18AM -0700, Jun Sun wrote:

> It would help if not for the gross typo. :-)  See the attachment.

Never noticed that because I already had a slightly more elegant solution
in my tree.  It's already in CVS, check it out.

  Ralf