Messages in this thread Patch in this message | | | Date | Wed, 5 Jul 2017 11:24:33 +0100 | From | James Hogan <> | Subject | Re: [PATCH 8/9] RISC-V: User-facing API |
| |
On Tue, Jul 04, 2017 at 12:51:01PM -0700, Palmer Dabbelt wrote: > diff --git a/arch/riscv/kernel/ptrace.c b/arch/riscv/kernel/ptrace.c > new file mode 100644 > index 000000000000..2720d5e97354 > --- /dev/null > +++ b/arch/riscv/kernel/ptrace.c > @@ -0,0 +1,138 @@
> +/* Put registers back to task. */ > +static void putregs(struct task_struct *child, struct pt_regs *uregs) > +{ > + struct pt_regs *regs = task_pt_regs(child); > + *regs = *uregs; > +} > + > +static int riscv_gpr_get(struct task_struct *target, > + const struct user_regset *regset, > + unsigned int pos, unsigned int count, > + void *kbuf, void __user *ubuf) > +{ > + struct pt_regs *regs; > + > + regs = task_pt_regs(target); > + return user_regset_copyout(&pos, &count, &kbuf, &ubuf, regs, 0, > + sizeof(*regs));
sizeof(struct pt_regs) > sizeof(struct user_regs_struct), which allows supervisor registers to be copied too. I think you should be using sizeof(struct user_regs_struct) instead.
> +} > + > +static int riscv_gpr_set(struct task_struct *target, > + const struct user_regset *regset, > + unsigned int pos, unsigned int count, > + const void *kbuf, const void __user *ubuf) > +{ > + int ret; > + struct pt_regs regs; > + > + ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, ®s, 0, > + sizeof(regs));
same
> + if (ret) > + return ret; > + > + putregs(target, ®s);
you're still copying via the stack without initialising the non-written fields. If userland does a short PTRACE_SETREGSET the remaining fields will be copied from the uninitialised kernel stack and accessible to userland via PTRACE_GETREGSET.
Even if the user does a full sizeof(struct user_regs_struct) (not pt_regs) PTRACE_SETREGSET the supervisor registers will be overwritten with uninitialised stack content.
> diff --git a/arch/riscv/kernel/sys_riscv.c b/arch/riscv/kernel/sys_riscv.c > new file mode 100644 > index 000000000000..4419604ff46c > --- /dev/null > +++ b/arch/riscv/kernel/sys_riscv.c
> +SYSCALL_DEFINE3(sysriscv_cmpxchg32, u32 __user *, ptr, u32, new, u32, old) > +{ > + u32 prev; > + unsigned int err; > + > + if (!access_ok(VERIFY_WRITE, ptr, sizeof(*ptr))) > + return -EFAULT; > + > +#ifdef CONFIG_ISA_A > + err = 0; > + prev = cmpxchg32(ptr, old, new);
I think this needs a special version of cmpxchg (or for cmpxchg to be modified) with fixup protection to return -EFAULT in case the page isn't mapped or is paged out/read only.
> +#else > + preempt_disable(); > + err = __get_user(prev, ptr); > + if (likely(!err && prev == old)) > + err = __put_user(new, ptr); > + preempt_enable(); > +#endif > + > + return unlikely(err) ? err : prev; > +} > + > +SYSCALL_DEFINE3(sysriscv_cmpxchg64, u64 __user *, ptr, u64, new, u64, old) > +{ > +#ifdef CONFIG_64BIT > + u64 prev; > + unsigned int err; > + > + if (!access_ok(VERIFY_WRITE, ptr, sizeof(*ptr))) > + return -EFAULT; > + > +#ifdef CONFIG_ISA_A > + err = 0; > + prev = cmpxchg64(ptr, old, new);
Likewise
> +#else > + preempt_disable(); > + err = __get_user(prev, ptr); > + if (likely(!err && prev == old)) > + err = __put_user(new, ptr); > + preempt_enable(); > +#endif > + return unlikely(err) ? err : prev; > +#else > + return -ENOTSUPP;
I think -ENOSYS is more standard for missing/unimplemented system calls.
A better way IMO would be to #ifdef the definitions in unistd.h, then the __NR_* definitions could also be more accurately extracted from the kernel headers, and you could just ifdef CONFIG_64BIT the whole syscall implementation, i.e.:
> diff --git a/arch/riscv/include/uapi/asm/unistd.h b/arch/riscv/include/uapi/asm/unistd.h > new file mode 100644 > index 000000000000..37a5429cc896 > --- /dev/null > +++ b/arch/riscv/include/uapi/asm/unistd.h > @@ -0,0 +1,23 @@ > +/* > + * These system calls add support for AMOs on RISC-V systems without support > + * for the A extension. > + */ > +#define __NR_sysriscv_cmpxchg32 (__NR_arch_specific_syscall + 0) > +__SYSCALL(__NR_sysriscv_cmpxchg32, sys_sysriscv_cmpxchg32)
+ifdef WHATEVER_BUILTIN_GCC_DEFINES_FOR_64BIT_RISCV_ABI
In case its helpful the following should list builtin preprocessor defines for given CFLAGS: ${CROSS_COMPILE}gcc ${CFLAGS} -dM -E -</dev/null > +#define __NR_sysriscv_cmpxchg64 (__NR_arch_specific_syscall + 1) > +__SYSCALL(__NR_sysriscv_cmpxchg64, sys_sysriscv_cmpxchg64)
+endif
Cheers James [unhandled content-type:application/pgp-signature] | |