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;