Miscellaneous minor compat ptrace fixes

Responding to a patch from Luc [1] to fix some sparse warnings, Will reported conflicts with the arm64 tree, and looking into this I noticed that I had introduced an inconsistency [2] into the FP{,S,C}R handling in the compat VFP regset accessors.

In this case, the sparse warnings are actually alerting us to a real issue, which is why [2] squashes one of those warnings, why [1] conflicts and why the fix by [1] to the second warning is arguably incorrect (though on initial shallow inspection of the patch it looked fine).

These issues arise from the way the regset API handles user versus kernel source/destination pointers, as explained in more detail in [2].

Patch 3 of this series fixes the outstanding sparse warning in a manner more consistent with [2].

Patches 1-2 fix other minor issues that I noticed along the way, but which are not directly related.

To avoid unintentional conflicts, the patches are based on
git://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git for-next/core 3edb1dd13ce6 ("Merge branch 'aarch64/for-next/ras-apei' into aarch64/for-next/core")

Cheers
---Dave

[1] [PATCH] arm64: fix missing __user in compat_vfp_{get,set}() lists.infradead.org/pipermail/linux-arm-kernel/2017-June/516315.html

[2] [PATCH 1/3] arm64: ptrace: Fix VFP register dumping in compat coredumps lists.infradead.org/pipermail/linux-arm-kernel/2017-June/514916.html

Dave Martin (3):
arm64: ptrace: Avoid setting compat FP[SC]R to garbage if get_user
fails
arm64: ptrace: Remove redundant overrun check from compat_vfp_set()
arm64: ptrace: Fix incorrect get_user() use in compat_vfp_set()

 arch/arm64/kernel/ptrace.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

2.1.4

If get_user() fails when reading the new FPSCR value from userspace in compat_vfp_get(), then garbage* will be written to the task's FPSR and FPCR registers.

This patch prevents this by checking the return from get_user() first.

[*] Actually, zero, due to the behaviour of get_user() on error, but that's still not what userspace expects.

---
 arch/arm64/kernel/ptrace.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

index 35846f1..4c068dc 100644
@@ -947,8 +947,10 @@ static int compat_vfp_set(struct task_struct *target,
 
 	if (count && !ret) {
 		ret = get_user(fpscr, (compat_ulong_t *)ubuf);
-		uregs->fpsr = fpscr & VFP_FPSCR_STAT_MASK;
-		uregs->fpcr = fpscr & VFP_FPSCR_CTRL_MASK;
+		if (!ret) {
+			uregs->fpsr = fpscr & VFP_FPSCR_STAT_MASK;
+			uregs->fpcr = fpscr & VFP_FPSCR_CTRL_MASK;
+		}
 	}
 
 	fpsimd_flush_task_state(target);

2.1.4

On the other hand, I don't think userspace can expect that if ptrace returns an error then none of the state has been updated, can it?

Given that we don't propagate the return value from __copy_from_user, I don't see what we're really fixing here and what userspace can now rely on that it couldn't rely on before.

Will

My main concern was to avoid the apparent leak of kernel stack to userspace. The reason this is safe is not obvious from the code here. The zeroing behaviour of get_user() does not appear to be documented anywhere (at least, not that I've found).

Alternative options are to initialise fpscr to 0, or (since there is no actual bug here) to drop the patch.

Cheers
---Dave

compat_vfp_set() checks for userspace trying to write an excessive amount of data to the regset. However this check is conspicuous for its absence from every other _set() in the arm64 ptrace implementation. In fact, the core ptrace_regset() already clamps userspace's iov_len to the regset size before the individual regset .{get,set}() methods get called.

This patch removes the redundant check.

---
 arch/arm64/kernel/ptrace.c | 3 ---
 1 file changed, 3 deletions(-)

index 4c068dc..949ab6b 100644
@@ -937,9 +937,6 @@ static int compat_vfp_set(struct task_struct *target,
 	compat_ulong_t fpscr;
 	int ret;
 
-	if (pos + count > VFP_STATE_SIZE)
-		return -EIO;
-
 	uregs = &target->thread.fpsimd_state.user_fpsimd;
 
 	ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, uregs, 0,

2.1.4

Now that compat_vfp_get() uses the regset API to copy the FPSCR value out to userspace, compat_vfp_set() looks inconsistent. In particular, compat_vfp_set() will fail if called with kbuf != NULL && ubuf == NULL (which is valid usage according to the regset API).

This patch fixes compat_vfp_set() to use user_regset_copyin(), similarly to compat_vfp_get().

This also squashes a sparse warning triggered by the cast that drops __user when calling get_user().

---
 arch/arm64/kernel/ptrace.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

index 949ab6b..1b38c01 100644
@@ -935,15 +935,17 @@ static int compat_vfp_set(struct task_struct *target,
 {
 	struct user_fpsimd_state *uregs;
 	compat_ulong_t fpscr;
-	int ret;
+	int ret, vregs_end_pos;
 
 	uregs = &target->thread.fpsimd_state.user_fpsimd;
 
+	vregs_end_pos = VFP_STATE_SIZE - sizeof(compat_ulong_t);
 	ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, uregs, 0,
-				 VFP_STATE_SIZE - sizeof(compat_ulong_t));
+				 vregs_end_pos);
 
 	if (count && !ret) {
-		ret = get_user(fpscr, (compat_ulong_t *)ubuf);
+		ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, &fpscr,
+					 vregs_end_pos, VFP_STATE_SIZE);
 		if (!ret) {
 			uregs->fpsr = fpscr & VFP_FPSCR_STAT_MASK;
 			uregs->fpcr = fpscr & VFP_FPSCR_CTRL_MASK;

2.1.4