[PATCH 0/3] Miscellaneous minor compat ptrace fixes

[PATCH 0/3] Miscellaneous minor compat ptrace fixes

[Dave Martin]

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

[PATCH 1/3] arm64: ptrace: Avoid setting compat FP[SC]R to garbage if get_user fails

[Dave Martin]

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.

Fixes: 478fcb2cdb23 ("arm64: Debugging support")
Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---
 arch/arm64/kernel/ptrace.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
index 35846f1..4c068dc 100644
--- a/arch/arm64/kernel/ptrace.c
+++ b/arch/arm64/kernel/ptrace.c
@@ -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

[PATCH 2/3] arm64: ptrace: Remove redundant overrun check from compat_vfp_set()

[Dave Martin]

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.

Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---
 arch/arm64/kernel/ptrace.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
index 4c068dc..949ab6b 100644
--- a/arch/arm64/kernel/ptrace.c
+++ b/arch/arm64/kernel/ptrace.c
@@ -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

[PATCH 3/3] arm64: ptrace: Fix incorrect get_user() use in compat_vfp_set()

[Dave Martin]

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().

Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---
 arch/arm64/kernel/ptrace.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
index 949ab6b..1b38c01 100644
--- a/arch/arm64/kernel/ptrace.c
+++ b/arch/arm64/kernel/ptrace.c
@@ -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

[PATCH 1/3] arm64: ptrace: Avoid setting compat FP[SC]R to garbage if get_user fails

[Will Deacon]

On Thu, Jun 29, 2017 at 03:25:47PM +0100, Dave Martin wrote:
> 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.

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

> 
> Fixes: 478fcb2cdb23 ("arm64: Debugging support")
> Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> ---
>  arch/arm64/kernel/ptrace.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
> index 35846f1..4c068dc 100644
> --- a/arch/arm64/kernel/ptrace.c
> +++ b/arch/arm64/kernel/ptrace.c
> @@ -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
> 

[PATCH 1/3] arm64: ptrace: Avoid setting compat FP[SC]R to garbage if get_user fails

[Dave Martin]

On Thu, Jun 29, 2017 at 04:37:09PM +0100, Will Deacon wrote:
> On Thu, Jun 29, 2017 at 03:25:47PM +0100, Dave Martin wrote:
> > 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.
> 
> 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.

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

> 
> Will
> 
> > 
> > Fixes: 478fcb2cdb23 ("arm64: Debugging support")
> > Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> > ---
> >  arch/arm64/kernel/ptrace.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
> > index 35846f1..4c068dc 100644
> > --- a/arch/arm64/kernel/ptrace.c
> > +++ b/arch/arm64/kernel/ptrace.c
> > @@ -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
> > 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel