On Fri, 8 Sep 2017, Alexander Bluhm wrote:
> On Tue, Sep 05, 2017 at 10:20:12PM -0600, Philip Guenther wrote:
> > CVSROOT: /cvs
> > Module name: src
> > Changes by: guent...@cvs.openbsd.org 2017/09/05 22:20:12
> >
> > Modified files:
> > regress/sys/kern/ptrace: ptrace.c
> >
> > Log message:
> > Fix declarations
> > ptrace(PT_IO) memory protection faults return EACCES, not EFAULT
>
> Now the test fails on i386.
>
> /usr/src/regress/sys/kern/ptrace/ptrace -Irib
> ptrace: ptrace(PT_IO): Bad address
> *** Error 1 in . (Makefile:19 'io_read_i_bad')
> FAILED
>
> Apparently i386 and amd64 return different errors. Do you know
> why? Is it worth to invesitgate or shoud we just accept both cases?
It was worth investigating and, having done so, I think my change was
incorrect and should be reverted and the amd64 kernel fixed to return
EFAULT in that case.
Currently on amd64, the copyin(9) family of calls will pass through the
return value of uvm_fault() and thus return not just EFAULT but possibly
EACCES, ENOMEM, and EIO. Since the return value of those functions is
usually passed through to userspace, that's a Bad Thing IMO and we need to
fix them to always return EFAULT when uvm_fault() fails.
The diff below does that by merging copy_fault and copy_efault, and ditto
copystr_fault and copystr_efault, and dropping the attempt to pass the
error number from trap() to the pcb_onfault callback.
While here, delete the similar attempt on arm64 to pass the error number
from trap() to the pcb_onfault callback...which already ignores it and
always return EFAULT.
ok?
Philip
Index: sys/arch/amd64/amd64/trap.c
===================================================================
RCS file: /cvs/src/sys/arch/amd64/amd64/trap.c,v
retrieving revision 1.57
diff -u -p -r1.57 trap.c
--- sys/arch/amd64/amd64/trap.c 25 Aug 2017 19:28:48 -0000 1.57
+++ sys/arch/amd64/amd64/trap.c 9 Sep 2017 23:30:19 -0000
@@ -211,10 +211,8 @@ trap(struct trapframe *frame)
goto we_re_toast;
/* Check for copyin/copyout fault. */
if (pcb->pcb_onfault != 0) {
- error = EFAULT;
copyfault:
frame->tf_rip = (u_int64_t)pcb->pcb_onfault;
- frame->tf_rax = error;
return;
}
Index: sys/arch/amd64/amd64/copy.S
===================================================================
RCS file: /cvs/src/sys/arch/amd64/amd64/copy.S,v
retrieving revision 1.10
diff -u -p -r1.10 copy.S
--- sys/arch/amd64/amd64/copy.S 25 Aug 2017 19:28:48 -0000 1.10
+++ sys/arch/amd64/amd64/copy.S 9 Sep 2017 23:30:19 -0000
@@ -107,10 +107,10 @@ ENTRY(copyout)
movq %rdi,%rdx
addq %rax,%rdx
- jc _C_LABEL(copy_efault)
+ jc _C_LABEL(copy_fault)
movq $VM_MAXUSER_ADDRESS,%r8
cmpq %r8,%rdx
- ja _C_LABEL(copy_efault)
+ ja _C_LABEL(copy_fault)
movq CPUVAR(CURPCB),%rdx
leaq _C_LABEL(copy_fault)(%rip),%r11
@@ -145,10 +145,10 @@ ENTRY(copyin)
movq %rsi,%rdx
addq %rax,%rdx
- jc _C_LABEL(copy_efault)
+ jc _C_LABEL(copy_fault)
movq $VM_MAXUSER_ADDRESS,%r8
cmpq %r8,%rdx
- ja _C_LABEL(copy_efault)
+ ja _C_LABEL(copy_fault)
3: /* bcopy(%rsi, %rdi, %rax); */
movq %rax,%rcx
@@ -171,13 +171,11 @@ ENTRY(copyin)
xorl %eax,%eax
ret
-NENTRY(copy_efault)
- movq $EFAULT,%rax
-
NENTRY(copy_fault)
SMAP_CLAC
movq CPUVAR(CURPCB),%rdx
popq PCB_ONFAULT(%rdx)
+ movl $EFAULT,%eax
ret
ENTRY(copyoutstr)
@@ -194,7 +192,7 @@ ENTRY(copyoutstr)
*/
movq $VM_MAXUSER_ADDRESS,%rax
subq %rdi,%rax
- jbe _C_LABEL(copystr_efault) /* die if CF == 1 || ZF == 1 */
+ jbe _C_LABEL(copystr_fault) /* die if CF == 1 || ZF == 1 */
cmpq %rdx,%rax
jae 1f
movq %rax,%rdx
@@ -217,8 +215,8 @@ ENTRY(copyoutstr)
2: /* rdx is zero -- return EFAULT or ENAMETOOLONG. */
movq $VM_MAXUSER_ADDRESS,%r11
cmpq %r11,%rdi
- jae _C_LABEL(copystr_efault)
- movq $ENAMETOOLONG,%rax
+ jae _C_LABEL(copystr_fault)
+ movl $ENAMETOOLONG,%eax
jmp copystr_return
ENTRY(copyinstr)
@@ -236,7 +234,7 @@ ENTRY(copyinstr)
*/
movq $VM_MAXUSER_ADDRESS,%rax
subq %rsi,%rax
- jbe _C_LABEL(copystr_efault) /* die if CF == 1 || ZF == 1 */
+ jbe _C_LABEL(copystr_fault) /* die if CF == 1 || ZF == 1 */
cmpq %rdx,%rax
jae 1f
movq %rax,%rdx
@@ -259,14 +257,12 @@ ENTRY(copyinstr)
2: /* edx is zero -- return EFAULT or ENAMETOOLONG. */
movq $VM_MAXUSER_ADDRESS,%r11
cmpq %r11,%rsi
- jae _C_LABEL(copystr_efault)
- movq $ENAMETOOLONG,%rax
+ jae _C_LABEL(copystr_fault)
+ movl $ENAMETOOLONG,%eax
jmp copystr_return
-ENTRY(copystr_efault)
- movl $EFAULT,%eax
-
ENTRY(copystr_fault)
+ movl $EFAULT,%eax
copystr_return:
SMAP_CLAC
/* Set *lencopied and return %eax. */
Index: sys/arch/arm64/arm64/trap.c
===================================================================
RCS file: /cvs/src/sys/arch/arm64/arm64/trap.c,v
retrieving revision 1.10
diff -u -p -r1.10 trap.c
--- sys/arch/arm64/arm64/trap.c 14 Aug 2017 21:53:34 -0000 1.10
+++ sys/arch/arm64/arm64/trap.c 9 Sep 2017 23:30:19 -0000
@@ -210,7 +210,6 @@ data_abort(struct trapframe *frame, uint
} else {
if (curcpu()->ci_idepth == 0 &&
pcb->pcb_onfault != 0) {
- frame->tf_x[0] = error;
frame->tf_elr = (register_t)pcb->pcb_onfault;
return;
}
Index: regress/sys/kern/ptrace/ptrace.c
===================================================================
RCS file: /cvs/src/regress/sys/kern/ptrace/ptrace.c,v
retrieving revision 1.4
diff -u -p -r1.4 ptrace.c
--- regress/sys/kern/ptrace/ptrace.c 6 Sep 2017 04:20:12 -0000 1.4
+++ regress/sys/kern/ptrace/ptrace.c 9 Sep 2017 23:30:19 -0000
@@ -132,7 +132,7 @@ main(int argc, char **argv)
if (ptrace(PT_IO, pid, (caddr_t)&piod, 0) == -1) {
warn("ptrace(PT_IO)");
- if (errno == EACCES)
+ if (errno == EFAULT)
ret = 1;
else
ret = -1;
