Re: [PATCH 1/2] compat32: translate userland PT_* request values into kernel

2019-05-29 Thread Michał Górny
On Thu, 2019-05-30 at 03:15 +1000, matthew green wrote:
> thanks for working on this.
> 
> > +   case PT_FIRSTMACH + 0:
> > +   return PT_STEP;
> > +   case PT_FIRSTMACH + 1:
> > +   return PT_GETREGS;
> [ ... ]
> 
> these magic numbers are a little ugly.  can you avoid them?

I know they are, however I don't think it's really possible.  One option
is to declare additional set of constants in the amd64 headers but that
doesn't really gain us anything, and ends up in kinda-absurd:

  case PT32_STEP: return PT_STEP;

or alike.

> is there a way to have amd64 have direct access to the i386
> values?

I don't think so.

> > --- a/sys/sys/ptrace.h
> > +++ b/sys/sys/ptrace.h
> > @@ -283,6 +283,10 @@ intptrace_machdep_dorequest(struct lwp *, struct 
> > lwp *, int,
> >  #define FIX_SSTEP(p)
> >  #endif
> >  
> > +#ifndef PTRACE_TRANSLATE_REQUEST32
> > +#define PTRACE_TRANSLATE_REQUEST32(x) x
> > +#endif
> > +
> 
> can this part live in sys/compat(/netbsd32)?  no need for the
> public ptrace header to get this, right?
> 

I'll try and see.

-- 
Best regards,
Michał Górny



signature.asc
Description: This is a digitally signed message part


re: [PATCH 1/2] compat32: translate userland PT_* request values into kernel

2019-05-29 Thread matthew green
thanks for working on this.

> + case PT_FIRSTMACH + 0:
> + return PT_STEP;
> + case PT_FIRSTMACH + 1:
> + return PT_GETREGS;
[ ... ]

these magic numbers are a little ugly.  can you avoid them?
is there a way to have amd64 have direct access to the i386
values?

> --- a/sys/sys/ptrace.h
> +++ b/sys/sys/ptrace.h
> @@ -283,6 +283,10 @@ int  ptrace_machdep_dorequest(struct lwp *, struct 
> lwp *, int,
>  #define FIX_SSTEP(p)
>  #endif
>  
> +#ifndef PTRACE_TRANSLATE_REQUEST32
> +#define PTRACE_TRANSLATE_REQUEST32(x) x
> +#endif
> +

can this part live in sys/compat(/netbsd32)?  no need for the
public ptrace header to get this, right?


.mrg.


Re: [PATCH 1/2] compat32: translate userland PT_* request values into kernel

2019-05-28 Thread Kamil Rytarowski
On 27.05.2019 21:03, Michał Górny wrote:
> Currently, the compat32 passes PT_* request values to kernel functions
> without translation.  This works fine for low PT_* requests that happen
> to have the same values both on i386 and amd64.  However, for requests
> higher than PT_SETFPREGS, the value passed from userland (matching i386
> const) does not match the correct kernel (amd64) request.  As a result,
> e.g. when compat32 process calls PT_GETDBREGS, kernel actually processes
> it as PT_SETSTEP.
> 
> To resolve this, introduce support for compat32 PT_* request
> translation.  The interface is based on PTRACE_TRANSLATE_REQUEST32 macro
> that is defined to a mapping function on architectures needing it.
> In case of amd64, this function maps userland i386 PT_* values into
> appropriate amd64 PT_* values.
> 
> For the time being, the two additional PT_GETXMMREGS and PT_SETXMMREGS
> requests are unsupported due to lack of matching free amd64 constant.


Both patches look good to me.



signature.asc
Description: OpenPGP digital signature


[PATCH 1/2] compat32: translate userland PT_* request values into kernel

2019-05-27 Thread Michał Górny
Currently, the compat32 passes PT_* request values to kernel functions
without translation.  This works fine for low PT_* requests that happen
to have the same values both on i386 and amd64.  However, for requests
higher than PT_SETFPREGS, the value passed from userland (matching i386
const) does not match the correct kernel (amd64) request.  As a result,
e.g. when compat32 process calls PT_GETDBREGS, kernel actually processes
it as PT_SETSTEP.

To resolve this, introduce support for compat32 PT_* request
translation.  The interface is based on PTRACE_TRANSLATE_REQUEST32 macro
that is defined to a mapping function on architectures needing it.
In case of amd64, this function maps userland i386 PT_* values into
appropriate amd64 PT_* values.

For the time being, the two additional PT_GETXMMREGS and PT_SETXMMREGS
requests are unsupported due to lack of matching free amd64 constant.
---
 sys/arch/amd64/amd64/netbsd32_machdep.c   | 37 +++
 sys/arch/amd64/include/netbsd32_machdep.h |  3 ++
 sys/arch/amd64/include/ptrace.h   |  2 ++
 sys/compat/netbsd32/netbsd32_ptrace.c |  8 -
 sys/sys/ptrace.h  |  4 +++
 5 files changed, 53 insertions(+), 1 deletion(-)

diff --git a/sys/arch/amd64/amd64/netbsd32_machdep.c 
b/sys/arch/amd64/amd64/netbsd32_machdep.c
index 39d671a4b9b6..06f6cbdf3703 100644
--- a/sys/arch/amd64/amd64/netbsd32_machdep.c
+++ b/sys/arch/amd64/amd64/netbsd32_machdep.c
@@ -335,6 +335,43 @@ cpu_coredump32(struct lwp *l, struct coredump_iostate 
*iocookie,
 }
 #endif
 
+int
+netbsd32_ptrace_translate_request(int req)
+{
+
+   switch (req)
+   {
+   case 0 ... PT_FIRSTMACH - 1:
+   return req;
+   case PT_FIRSTMACH + 0:
+   return PT_STEP;
+   case PT_FIRSTMACH + 1:
+   return PT_GETREGS;
+   case PT_FIRSTMACH + 2:
+   return PT_SETREGS;
+   case PT_FIRSTMACH + 3:
+   return PT_GETFPREGS;
+   case PT_FIRSTMACH + 4:
+   return PT_SETFPREGS;
+   case PT_FIRSTMACH + 5:
+   /* PT_GETXMMREGS */
+   return -1;
+   case PT_FIRSTMACH + 6:
+   /* PT_SETXMMREGS */
+   return -1;
+   case PT_FIRSTMACH + 7:
+   return PT_GETDBREGS;
+   case PT_FIRSTMACH + 8:
+   return PT_SETDBREGS;
+   case PT_FIRSTMACH + 9:
+   return PT_SETSTEP;
+   case PT_FIRSTMACH + 10:
+   return PT_CLEARSTEP;
+   default:
+   return -1;
+   }
+}
+
 int
 netbsd32_process_read_regs(struct lwp *l, struct reg32 *regs)
 {
diff --git a/sys/arch/amd64/include/netbsd32_machdep.h 
b/sys/arch/amd64/include/netbsd32_machdep.h
index 51691eb7c90f..37019d29134d 100644
--- a/sys/arch/amd64/include/netbsd32_machdep.h
+++ b/sys/arch/amd64/include/netbsd32_machdep.h
@@ -151,6 +151,9 @@ struct x86_64_set_mtrr_args32 {
 
 #define NETBSD32_MID_MACHINE MID_I386
 
+/* Translate ptrace() PT_* request from 32-bit userland to kernel. */
+int netbsd32_ptrace_translate_request(int);
+
 int netbsd32_process_read_regs(struct lwp *, struct reg32 *);
 int netbsd32_process_read_fpregs(struct lwp *, struct fpreg32 *, size_t *);
 int netbsd32_process_read_dbregs(struct lwp *, struct dbreg32 *, size_t *);
diff --git a/sys/arch/amd64/include/ptrace.h b/sys/arch/amd64/include/ptrace.h
index c5cef941fd16..8d0f54dbad24 100644
--- a/sys/arch/amd64/include/ptrace.h
+++ b/sys/arch/amd64/include/ptrace.h
@@ -87,6 +87,8 @@
 #define process_reg32  struct reg32
 #define process_fpreg32struct fpreg32
 #define process_dbreg32struct dbreg32
+
+#define PTRACE_TRANSLATE_REQUEST32(x) netbsd32_ptrace_translate_request(x)
 #endif /* COMPAT_NETBSD32 */
 #endif /* _KERNEL_OPT */
 
diff --git a/sys/compat/netbsd32/netbsd32_ptrace.c 
b/sys/compat/netbsd32/netbsd32_ptrace.c
index 614d363c3126..feb2b99c8d54 100644
--- a/sys/compat/netbsd32/netbsd32_ptrace.c
+++ b/sys/compat/netbsd32/netbsd32_ptrace.c
@@ -243,6 +243,8 @@ int
 netbsd32_ptrace(struct lwp *l, const struct netbsd32_ptrace_args *uap,
 register_t *retval)
 {
+   int req;
+
/* {
syscallarg(int) req;
syscallarg(pid_t) pid;
@@ -250,7 +252,11 @@ netbsd32_ptrace(struct lwp *l, const struct 
netbsd32_ptrace_args *uap,
syscallarg(int) data;
} */
 
-   return do_ptrace(_ptm, l, SCARG(uap, req), SCARG(uap, pid),
+   req = PTRACE_TRANSLATE_REQUEST32(SCARG(uap, req));
+   if (req == -1)
+   return EOPNOTSUPP;
+
+   return do_ptrace(_ptm, l, req, SCARG(uap, pid),
SCARG_P32(uap, addr), SCARG(uap, data), retval);
 }
 
diff --git a/sys/sys/ptrace.h b/sys/sys/ptrace.h
index b213b38f5c6b..484f0ff6d268 100644
--- a/sys/sys/ptrace.h
+++ b/sys/sys/ptrace.h
@@ -283,6 +283,10 @@ intptrace_machdep_dorequest(struct lwp *, struct 
lwp *, int,
 #define FIX_SSTEP(p)
 #endif
 
+#ifndef PTRACE_TRANSLATE_REQUEST32