casting from kernel address space to user needs __force markup. Add it.
Signed-off-by: Namhyung Kim <[email protected]>
---
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
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
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 <[email protected]>
Signed-off-by: Namhyung Kim <[email protected]>
---
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
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 <[email protected]>
> Signed-off-by: Namhyung Kim <[email protected]>
> ---
> 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.
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
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
Arnd Bergmann <[email protected]> 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
On Wednesday 25 August 2010, Namhyung Kim wrote:
> Arnd Bergmann <[email protected]> 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