lkml.org 
[lkml]   [2017]   [Jul]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [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, &regs, 0,
> + sizeof(regs));

same

> + if (ret)
> + return ret;
> +
> + putregs(target, &regs);

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]
\
 
 \ /
  Last update: 2017-07-05 12:25    [W:0.319 / U:2.072 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site