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
Attachment:
signature.asc
Description: Digital signature
