[PATCH] arm64: fix missing __user in compat_vfp_{get,set}()

[PATCH] arm64: fix missing __user in compat_vfp_{get,set}()

[Luc Van Oostenryck]

compat_vfp_get() & compat_vfp_set() are two helpers reading
or writting some values via {get,put}_user() which need a
pointer annotated with '__user'.
The buffers used by the two helpers are correctly '__user'
annotated but need to be casted to a real type before being
given to {get,put}_user().

The problem is that this cast lack a '__user' annotation.

Fix this by adding the missing '__user'.

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 arch/arm64/kernel/ptrace.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
index c142459a8..c77f425c7 100644
--- a/arch/arm64/kernel/ptrace.c
+++ b/arch/arm64/kernel/ptrace.c
@@ -908,7 +908,7 @@ static int compat_vfp_get(struct task_struct *target,
 	if (count && !ret) {
 		fpscr = (uregs->fpsr & VFP_FPSCR_STAT_MASK) |
 			(uregs->fpcr & VFP_FPSCR_CTRL_MASK);
-		ret = put_user(fpscr, (compat_ulong_t *)ubuf);
+		ret = put_user(fpscr, (compat_ulong_t __user *)ubuf);
 	}
 
 	return ret;
@@ -932,7 +932,7 @@ static int compat_vfp_set(struct task_struct *target,
 				 VFP_STATE_SIZE - sizeof(compat_ulong_t));
 
 	if (count && !ret) {
-		ret = get_user(fpscr, (compat_ulong_t *)ubuf);
+		ret = get_user(fpscr, (compat_ulong_t __user *)ubuf);
 		uregs->fpsr = fpscr & VFP_FPSCR_STAT_MASK;
 		uregs->fpcr = fpscr & VFP_FPSCR_CTRL_MASK;
 	}
-- 
2.13.0

[PATCH] arm64: fix missing __user in compat_vfp_{get,set}()

[Dave Martin]

On Wed, Jun 28, 2017 at 04:58:16PM +0200, Luc Van Oostenryck wrote:
> compat_vfp_get() & compat_vfp_set() are two helpers reading
> or writting some values via {get,put}_user() which need a
> pointer annotated with '__user'.
> The buffers used by the two helpers are correctly '__user'
> annotated but need to be casted to a real type before being
> given to {get,put}_user().
> 
> The problem is that this cast lack a '__user' annotation.
> 
> Fix this by adding the missing '__user'.

Casting __user off is something that sparse certainly should be warning
about (and it was, by the looks of it).

Reviewed-by: Dave Martin <Dave.Martin@arm.com>

(There remains a sparse warning in compat_ptrace_write_user(), where we
pass implicitly cast __user onto a pointer, under set_fs(KERNEL_DS).
I'm not sure what the most correct fix is there.)

Cheers
---Dave

> 
> Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
> ---
>  arch/arm64/kernel/ptrace.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
> index c142459a8..c77f425c7 100644
> --- a/arch/arm64/kernel/ptrace.c
> +++ b/arch/arm64/kernel/ptrace.c
> @@ -908,7 +908,7 @@ static int compat_vfp_get(struct task_struct *target,
>  	if (count && !ret) {
>  		fpscr = (uregs->fpsr & VFP_FPSCR_STAT_MASK) |
>  			(uregs->fpcr & VFP_FPSCR_CTRL_MASK);
> -		ret = put_user(fpscr, (compat_ulong_t *)ubuf);
> +		ret = put_user(fpscr, (compat_ulong_t __user *)ubuf);
>  	}
>  
>  	return ret;
> @@ -932,7 +932,7 @@ static int compat_vfp_set(struct task_struct *target,
>  				 VFP_STATE_SIZE - sizeof(compat_ulong_t));
>  
>  	if (count && !ret) {
> -		ret = get_user(fpscr, (compat_ulong_t *)ubuf);
> +		ret = get_user(fpscr, (compat_ulong_t __user *)ubuf);
>  		uregs->fpsr = fpscr & VFP_FPSCR_STAT_MASK;
>  		uregs->fpcr = fpscr & VFP_FPSCR_CTRL_MASK;
>  	}
> -- 
> 2.13.0
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH] arm64: fix missing __user in compat_vfp_{get,set}()

[Luc Van Oostenryck]

On Wed, Jun 28, 2017 at 04:22:22PM +0100, Dave Martin wrote:
> (There remains a sparse warning in compat_ptrace_write_user(), where we
> pass implicitly cast __user onto a pointer, under set_fs(KERNEL_DS).
> I'm not sure what the most correct fix is there.)

Yes, I saw.
There is no pretty solution for things like here:
you can't adjust the type, you're forced to make a cast,
explicit or implicit.


Regards,
-- Luc

[PATCH] arm64: fix missing __user in compat_vfp_{get,set}()

[Will Deacon]

On Wed, Jun 28, 2017 at 04:58:16PM +0200, Luc Van Oostenryck wrote:
> compat_vfp_get() & compat_vfp_set() are two helpers reading
> or writting some values via {get,put}_user() which need a
> pointer annotated with '__user'.
> The buffers used by the two helpers are correctly '__user'
> annotated but need to be casted to a real type before being
> given to {get,put}_user().
> 
> The problem is that this cast lack a '__user' annotation.
> 
> Fix this by adding the missing '__user'.
> 
> Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
> ---
>  arch/arm64/kernel/ptrace.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

This doesn't apply against the arm64 for-next/core branch, due to
changes from Dave reworking this to use user_regset_copyout.

Will

[PATCH v2] arm64: fix missing __user in compat_vfp_set()

[Luc Van Oostenryck]

compat_vfp_set() is a helper writting some values via put_user()
and put_user() need a pointer annotated with '__user'.
The buffer used by the helper is correctly annotated with '__user'
but need to be casted to a real type before being given to
put_user().

The problem is that this cast lack a '__user' annotation.

Fix this by adding the missing '__user'.

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>

---
Change since v1:
- rebase against arm64/for-next/core
- drop the change for compat_vfp_get() which is no more needed
---
 arch/arm64/kernel/ptrace.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
index 35846f155..3a323e2b9 100644
--- a/arch/arm64/kernel/ptrace.c
+++ b/arch/arm64/kernel/ptrace.c
@@ -946,7 +946,7 @@ static int compat_vfp_set(struct task_struct *target,
 				 VFP_STATE_SIZE - sizeof(compat_ulong_t));
 
 	if (count && !ret) {
-		ret = get_user(fpscr, (compat_ulong_t *)ubuf);
+		ret = get_user(fpscr, (compat_ulong_t __user *)ubuf);
 		uregs->fpsr = fpscr & VFP_FPSCR_STAT_MASK;
 		uregs->fpcr = fpscr & VFP_FPSCR_CTRL_MASK;
 	}
-- 
2.13.0

[PATCH v2] arm64: fix missing __user in compat_vfp_set()

[Dave Martin]

On Thu, Jun 29, 2017 at 04:24:48PM +0200, Luc Van Oostenryck wrote:
> compat_vfp_set() is a helper writting some values via put_user()
> and put_user() need a pointer annotated with '__user'.
> The buffer used by the helper is correctly annotated with '__user'
> but need to be casted to a real type before being given to
> put_user().
> 
> The problem is that this cast lack a '__user' annotation.
> 
> Fix this by adding the missing '__user'.
> 
> Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>

Apologies, I has half a reply written and then realised this issue was
less straightforward that I first thought.  I have an alternate patch,
see [1].

Cheers
---Dave

[1] [PATCH 0/3] Miscellaneous minor compat ptrace fixes
http://lists.infradead.org/pipermail/linux-arm-kernel/2017-June/516592.html

> 
> ---
> Change since v1:
> - rebase against arm64/for-next/core
> - drop the change for compat_vfp_get() which is no more needed
> ---
>  arch/arm64/kernel/ptrace.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
> index 35846f155..3a323e2b9 100644
> --- a/arch/arm64/kernel/ptrace.c
> +++ b/arch/arm64/kernel/ptrace.c
> @@ -946,7 +946,7 @@ static int compat_vfp_set(struct task_struct *target,
>  				 VFP_STATE_SIZE - sizeof(compat_ulong_t));
>  
>  	if (count && !ret) {
> -		ret = get_user(fpscr, (compat_ulong_t *)ubuf);
> +		ret = get_user(fpscr, (compat_ulong_t __user *)ubuf);
>  		uregs->fpsr = fpscr & VFP_FPSCR_STAT_MASK;
>  		uregs->fpcr = fpscr & VFP_FPSCR_CTRL_MASK;
>  	}
> -- 
> 2.13.0
> 
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2] arm64: fix missing __user in compat_vfp_set()

[Luc Van Oostenryck]

On Thu, Jun 29, 2017 at 03:34:06PM +0100, Dave Martin wrote:
> On Thu, Jun 29, 2017 at 04:24:48PM +0200, Luc Van Oostenryck wrote:
> > compat_vfp_set() is a helper writting some values via put_user()
> > and put_user() need a pointer annotated with '__user'.
> > The buffer used by the helper is correctly annotated with '__user'
> > but need to be casted to a real type before being given to
> > put_user().
> > 
> > The problem is that this cast lack a '__user' annotation.
> > 
> > Fix this by adding the missing '__user'.
> > 
> > Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
> 
> Apologies, I has half a reply written and then realised this issue was
> less straightforward that I first thought.  I have an alternate patch,
> see [1].

Ah yes.
I saw the now asymmetry between the set/copyout and the set/copyin
and I was a bit surprised by it.

Your new version look better (but I can't judge more than the look).

-- Luc