linux-kernel - Re: [PATCH 1/2] riscv: ptrace: Use the correct API for `fcsr' access

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 04 Aug 2020 19:01:01 -0700 (PDT)
From:   Palmer Dabbelt <palmer@...belt.com>
To:     macro@....com
CC:     linux-riscv@...ts.infradead.org,
        Paul Walmsley <paul.walmsley@...ive.com>,
        aou@...s.berkeley.edu, linux-kernel@...r.kernel.org,
        stable@...r.kernel.org
Subject:     Re: [PATCH 1/2] riscv: ptrace: Use the correct API for `fcsr' access

On Thu, 23 Jul 2020 16:22:15 PDT (-0700), macro@....com wrote:
> Adjust the calls to `user_regset_copyout' and `user_regset_copyin' in
> `riscv_fpr_get' and `riscv_fpr_set' respectively so as to use @start_pos
> and @end_pos according to API documentation in <linux/regset.h>, that is
> to point at the beginning and the end respectively of the data chunk to
> be copied.  Update @data accordingly, also for the first call, to make
> it clear which structure member is accessed.
>
> We currently have @start_pos fixed at 0 across all calls, which works as
> a result of the implementation, in particular because we have no padding
> between the FP general registers and the FP control and status register,
> but appears not to have been the intent of the API and is not what other
> ports do, requiring one to study the copy handlers to understand what is
> going on here.
>
> Signed-off-by: Maciej W. Rozycki <macro@....com>
> Fixes: b8c8a9590e4f ("RISC-V: Add FP register ptrace support for gdb.")
> Cc: stable@...r.kernel.org # 4.20+
> ---
>  arch/riscv/kernel/ptrace.c |   14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
>
> linux-riscv-ptrace-fcsr.diff
> Index: linux-hv/arch/riscv/kernel/ptrace.c
> ===================================================================
> --- linux-hv.orig/arch/riscv/kernel/ptrace.c
> +++ linux-hv/arch/riscv/kernel/ptrace.c
> @@ -61,10 +61,13 @@ static int riscv_fpr_get(struct task_str
>  	int ret;
>  	struct __riscv_d_ext_state *fstate = &target->thread.fstate;
>
> -	ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf, fstate, 0,
> +	ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf, &fstate->f, 0,
>  				  offsetof(struct __riscv_d_ext_state, fcsr));

As far as I can tell the current code works correctly, it just requires
knowledge of the layout of __riscv_d_ext_state to determine that it functions
correctly.  This new code still requires that knowledge: the first blob copies
the F registers, but only works if the CSR is after the registers.  If we fix
both of those the code seems easier to read, but I don't think splitting the
difference helps any.

So I guess what I'm saying is: maybe that second line should be changed to
something like "ARRAY_SIZE(fstate->f)"?

>  	if (!ret) {
> -		ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf, fstate, 0,
> +		ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
> +					  &fstate->fcsr,
> +					  offsetof(struct __riscv_d_ext_state,
> +						   fcsr),
>  					  offsetof(struct __riscv_d_ext_state, fcsr) +
>  					  sizeof(fstate->fcsr));
>  	}
> @@ -80,10 +83,13 @@ static int riscv_fpr_set(struct task_str
>  	int ret;
>  	struct __riscv_d_ext_state *fstate = &target->thread.fstate;
>
> -	ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, fstate, 0,
> +	ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, &fstate->f, 0,
>  				 offsetof(struct __riscv_d_ext_state, fcsr));
>  	if (!ret) {
> -		ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, fstate, 0,
> +		ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf,
> +					 &fstate->fcsr,
> +					 offsetof(struct __riscv_d_ext_state,
> +						  fcsr),
>  					 offsetof(struct __riscv_d_ext_state, fcsr) +
>  					 sizeof(fstate->fcsr));
>  	}