[PATCH 2/3] ptrace: add __force markup

[PATCH 2/3] ptrace: add __force markup

[Namhyung Kim]

casting from kernel address space to user needs __force markup. Add it.

Signed-off-by: Namhyung Kim <namhyung@gmail.com>
---
 kernel/ptrace.c |   13 +++++++------
 1 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index 4afd9b8..1abbb4d 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -580,18 +580,19 @@ int ptrace_request(struct task_struct *child, long request,
 		ret = ptrace_setoptions(child, data);
 		break;
 	case PTRACE_GETEVENTMSG:
-		ret = put_user(child->ptrace_message, (unsigned long __user *) data);
+		ret = put_user(child->ptrace_message,
+			       (unsigned long __user __force *) data);
 		break;
 
 	case PTRACE_GETSIGINFO:
 		ret = ptrace_getsiginfo(child, &siginfo);
 		if (!ret)
-			ret = copy_siginfo_to_user((siginfo_t __user *) data,
+			ret = copy_siginfo_to_user((siginfo_t __user __force *) data,
 						   &siginfo);
 		break;
 
 	case PTRACE_SETSIGINFO:
-		if (copy_from_user(&siginfo, (siginfo_t __user *) data,
+		if (copy_from_user(&siginfo, (siginfo_t __user __force *) data,
 				   sizeof siginfo))
 			ret = -EFAULT;
 		else
@@ -623,7 +624,7 @@ int ptrace_request(struct task_struct *child, long request,
 		}
 		mmput(mm);
 
-		ret = put_user(tmp, (unsigned long __user *) data);
+		ret = put_user(tmp, (unsigned long __user __force *) data);
 		break;
 	}
 #endif
@@ -652,7 +653,7 @@ int ptrace_request(struct task_struct *child, long request,
 	case PTRACE_SETREGSET:
 	{
 		struct iovec kiov;
-		struct iovec __user *uiov = (struct iovec __user *) data;
+		struct iovec __user *uiov = (struct iovec __user __force *) data;
 
 		if (!access_ok(VERIFY_WRITE, uiov, sizeof(*uiov)))
 			return -EFAULT;
@@ -742,7 +743,7 @@ int generic_ptrace_peekdata(struct task_struct *tsk, long addr, long data)
 	copied = access_process_vm(tsk, addr, &tmp, sizeof(tmp), 0);
 	if (copied != sizeof(tmp))
 		return -EIO;
-	return put_user(tmp, (unsigned long __user *)data);
+	return put_user(tmp, (unsigned long __user __force *)data);
 }
 
 int generic_ptrace_pokedata(struct task_struct *tsk, long addr, long data)
-- 
1.7.0.4

Re: [PATCH 2/3] ptrace: add __force markup

[Arnd Bergmann]

On Sunday 22 August 2010 12:22:15 Namhyung Kim wrote:
> index 4afd9b8..1abbb4d 100644
> --- a/kernel/ptrace.c
> +++ b/kernel/ptrace.c
> @@ -580,18 +580,19 @@ int ptrace_request(struct task_struct *child, long request,
>                 ret = ptrace_setoptions(child, data);
>                 break;
>         case PTRACE_GETEVENTMSG:
> -               ret = put_user(child->ptrace_message, (unsigned long __user *) data);
> +               ret = put_user(child->ptrace_message,
> +                              (unsigned long __user __force *) data);
>                 break;

Maybe you should introduce a new temporary variable

	void __user *p = (void __user __force*)data;

and get rid of all the other casts instead.

	Arnd

[PATCH 2/3 RESEND] ptrace: cleanup address space conversion on ptrace_request()

[Namhyung Kim]

In some cases 'data' should be considered as a user pointer but it requires
cast with additional __force markup which was missing. Rather than add the
markups wherever needed, make use of a temporary variable of user pointer.
This patch is based on compat_ptrace_request() implementation.

Suggested-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Namhyung Kim <namhyung@gmail.com>
---
 kernel/ptrace.c |   13 +++++++------
 1 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index 4afd9b8..7547b1c 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -564,6 +564,7 @@ int ptrace_request(struct task_struct *child, long request,
 {
 	int ret = -EIO;
 	siginfo_t siginfo;
+	unsigned long __user *datap = (unsigned long __user __force *) data;
 
 	switch (request) {
 	case PTRACE_PEEKTEXT:
@@ -580,18 +581,18 @@ int ptrace_request(struct task_struct *child, long request,
 		ret = ptrace_setoptions(child, data);
 		break;
 	case PTRACE_GETEVENTMSG:
-		ret = put_user(child->ptrace_message, (unsigned long __user *) data);
+		ret = put_user(child->ptrace_message, datap);
 		break;
 
 	case PTRACE_GETSIGINFO:
 		ret = ptrace_getsiginfo(child, &siginfo);
 		if (!ret)
-			ret = copy_siginfo_to_user((siginfo_t __user *) data,
+			ret = copy_siginfo_to_user((siginfo_t __user *) datap,
 						   &siginfo);
 		break;
 
 	case PTRACE_SETSIGINFO:
-		if (copy_from_user(&siginfo, (siginfo_t __user *) data,
+		if (copy_from_user(&siginfo, (siginfo_t __user *) datap,
 				   sizeof siginfo))
 			ret = -EFAULT;
 		else
@@ -623,7 +624,7 @@ int ptrace_request(struct task_struct *child, long request,
 		}
 		mmput(mm);
 
-		ret = put_user(tmp, (unsigned long __user *) data);
+		ret = put_user(tmp, datap);
 		break;
 	}
 #endif
@@ -652,7 +653,7 @@ int ptrace_request(struct task_struct *child, long request,
 	case PTRACE_SETREGSET:
 	{
 		struct iovec kiov;
-		struct iovec __user *uiov = (struct iovec __user *) data;
+		struct iovec __user *uiov = (struct iovec __user *) datap;
 
 		if (!access_ok(VERIFY_WRITE, uiov, sizeof(*uiov)))
 			return -EFAULT;
@@ -742,7 +743,7 @@ int generic_ptrace_peekdata(struct task_struct *tsk, long addr, long data)
 	copied = access_process_vm(tsk, addr, &tmp, sizeof(tmp), 0);
 	if (copied != sizeof(tmp))
 		return -EIO;
-	return put_user(tmp, (unsigned long __user *)data);
+	return put_user(tmp, (unsigned long __user __force *)data);
 }
 
 int generic_ptrace_pokedata(struct task_struct *tsk, long addr, long data)
-- 
1.7.0.4

Re: [PATCH 2/3 RESEND] ptrace: cleanup address space conversion on ptrace_request()

[Christoph Hellwig]

On Tue, Aug 24, 2010 at 01:06:15AM +0900, Namhyung Kim wrote:
> In some cases 'data' should be considered as a user pointer but it requires
> cast with additional __force markup which was missing. Rather than add the
> markups wherever needed, make use of a temporary variable of user pointer.
> This patch is based on compat_ptrace_request() implementation.
> 
> Suggested-by: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Namhyung Kim <namhyung@gmail.com>
> ---
>  kernel/ptrace.c |   13 +++++++------
>  1 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/kernel/ptrace.c b/kernel/ptrace.c
> index 4afd9b8..7547b1c 100644
> --- a/kernel/ptrace.c
> +++ b/kernel/ptrace.c
> @@ -564,6 +564,7 @@ int ptrace_request(struct task_struct *child, long request,
>  {
>  	int ret = -EIO;
>  	siginfo_t siginfo;
> +	unsigned long __user *datap = (unsigned long __user __force *) data;

Just make this a

	void __user *datap = (void __user *)data;

and you can pass it around withjout additional casts.  Also no need
for the force here I think.

You'll still need a cast for the put_user calls, or you could just
convert them to copy_to_user which is fine with a void pointer.

Re: [PATCH 2/3 RESEND] ptrace: cleanup address space conversion on ptrace_request()

[Namhyung Kim]

2010-08-23 (Mon), 12:21 -0400, Christoph Hellwig:
> Just make this a
> 
> 	void __user *datap = (void __user *)data;
> 
> and you can pass it around withjout additional casts.  Also no need
> for the force here I think.
> 

But removing __force results in following sparse warning:

 ptrace.c:567:40: warning: cast adds address space to expression
(<asn:1>)


-- 
Regards,
Namhyung Kim

Re: [PATCH 2/3 RESEND] ptrace: cleanup address space conversion on ptrace_request()

[Arnd Bergmann]

On Monday 23 August 2010, Namhyung Kim wrote:
> But removing __force results in following sparse warning:
> 
>  ptrace.c:567:40: warning: cast adds address space to expression
> (<asn:1>)

The problem is that ptrace takes a 'long' argument, not 'unsigned long'.
Sparse complains about adding address spaces in all casts except those
from/to unsigned long, which we often use as an opaque token that can
fit anything.

I don't think we ever rely on 'data' being signed, and user space
sees it as 'void *' anyway, so it should be possible to just turn
it into 'unsigned long'.

	Arnd

Re: [PATCH 2/3 RESEND] ptrace: cleanup address space conversion on ptrace_request()

[Namhyung Kim]

Arnd Bergmann <arnd@arndb.de> writes:
> I don't think we ever rely on 'data' being signed, and user space
> sees it as 'void *' anyway, so it should be possible to just turn
> it into 'unsigned long'.
>

After I've checked all arch_ptrace() code, I can think there is no specific
reason @addr and @data to be signed long types. Most of archs use them
as if they were unsigned. So I'll prepare a patch series to change this
and some cleanups of ptrace_request() and all of arch_ptrace() if no
objection exists.

Thanks.

-- 
Regards,
Namhyung Kim

Re: [PATCH 2/3 RESEND] ptrace: cleanup address space conversion on ptrace_request()

[Arnd Bergmann]

On Wednesday 25 August 2010, Namhyung Kim wrote:
> Arnd Bergmann <arnd@arndb.de> writes:
> > I don't think we ever rely on 'data' being signed, and user space
> > sees it as 'void *' anyway, so it should be possible to just turn
> > it into 'unsigned long'.
> >
> 
> After I've checked all arch_ptrace() code, I can think there is no specific
> reason @addr and @data to be signed long types. Most of archs use them
> as if they were unsigned. So I'll prepare a patch series to change this
> and some cleanups of ptrace_request() and all of arch_ptrace() if no
> objection exists.

Sounds good to me.

	Arnd