* [PATCH] xtensa: prevent arbitrary read in ptrace
@ 2011-07-08 0:03 Dan Rosenberg
2011-07-08 18:27 ` [Security] " Andrew Morton
0 siblings, 1 reply; 6+ messages in thread
From: Dan Rosenberg @ 2011-07-08 0:03 UTC (permalink / raw)
To: chris; +Cc: linux-kernel, security
Prevent an arbitrary kernel read. Check the user pointer with
access_ok() before copying data in.
Signed-off-by: Dan Rosenberg <drosenberg@vsecurity.com>
Cc: stable@kernel.org
---
arch/xtensa/kernel/ptrace.c | 3 +++
1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/arch/xtensa/kernel/ptrace.c b/arch/xtensa/kernel/ptrace.c
index c72c947..ddce75e 100644
--- a/arch/xtensa/kernel/ptrace.c
+++ b/arch/xtensa/kernel/ptrace.c
@@ -147,6 +147,9 @@ int ptrace_setxregs(struct task_struct *child, void __user *uregs)
elf_xtregs_t *xtregs = uregs;
int ret = 0;
+ if (!access_ok(VERIFY_READ, uregs, sizeof(elf_xtregs_t)))
+ return -EIO;
+
#if XTENSA_HAVE_COPROCESSORS
/* Flush all coprocessors before we overwrite them. */
coprocessor_flush_all(ti);
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Security] [PATCH] xtensa: prevent arbitrary read in ptrace
2011-07-08 0:03 [PATCH] xtensa: prevent arbitrary read in ptrace Dan Rosenberg
@ 2011-07-08 18:27 ` Andrew Morton
2011-07-08 18:42 ` Oleg Nesterov
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Morton @ 2011-07-08 18:27 UTC (permalink / raw)
To: Dan Rosenberg; +Cc: chris, security, linux-kernel, Oleg Nesterov
On Thu, 07 Jul 2011 20:03:54 -0400
Dan Rosenberg <drosenberg@vsecurity.com> wrote:
> Prevent an arbitrary kernel read. Check the user pointer with
> access_ok() before copying data in.
>
> Signed-off-by: Dan Rosenberg <drosenberg@vsecurity.com>
> Cc: stable@kernel.org
> ---
> arch/xtensa/kernel/ptrace.c | 3 +++
> 1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/arch/xtensa/kernel/ptrace.c b/arch/xtensa/kernel/ptrace.c
> index c72c947..ddce75e 100644
> --- a/arch/xtensa/kernel/ptrace.c
> +++ b/arch/xtensa/kernel/ptrace.c
> @@ -147,6 +147,9 @@ int ptrace_setxregs(struct task_struct *child, void __user *uregs)
> elf_xtregs_t *xtregs = uregs;
> int ret = 0;
>
> + if (!access_ok(VERIFY_READ, uregs, sizeof(elf_xtregs_t)))
> + return -EIO;
This should be -EFAULT, methinks?
> +
> #if XTENSA_HAVE_COPROCESSORS
> /* Flush all coprocessors before we overwrite them. */
> coprocessor_flush_all(ti);
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Security] [PATCH] xtensa: prevent arbitrary read in ptrace
2011-07-08 18:27 ` [Security] " Andrew Morton
@ 2011-07-08 18:42 ` Oleg Nesterov
2011-07-08 19:29 ` Chris Zankel
0 siblings, 1 reply; 6+ messages in thread
From: Oleg Nesterov @ 2011-07-08 18:42 UTC (permalink / raw)
To: Andrew Morton; +Cc: Dan Rosenberg, chris, security, linux-kernel
On 07/08, Andrew Morton wrote:
>
> On Thu, 07 Jul 2011 20:03:54 -0400
> Dan Rosenberg <drosenberg@vsecurity.com> wrote:
>
> > Prevent an arbitrary kernel read. Check the user pointer with
> > access_ok() before copying data in.
> >
> > Signed-off-by: Dan Rosenberg <drosenberg@vsecurity.com>
> > Cc: stable@kernel.org
> > ---
> > arch/xtensa/kernel/ptrace.c | 3 +++
> > 1 files changed, 3 insertions(+), 0 deletions(-)
> >
> > diff --git a/arch/xtensa/kernel/ptrace.c b/arch/xtensa/kernel/ptrace.c
> > index c72c947..ddce75e 100644
> > --- a/arch/xtensa/kernel/ptrace.c
> > +++ b/arch/xtensa/kernel/ptrace.c
> > @@ -147,6 +147,9 @@ int ptrace_setxregs(struct task_struct *child, void __user *uregs)
> > elf_xtregs_t *xtregs = uregs;
> > int ret = 0;
> >
> > + if (!access_ok(VERIFY_READ, uregs, sizeof(elf_xtregs_t)))
> > + return -EIO;
>
> This should be -EFAULT, methinks?
Also, it seems that ptrace_setxregs/ptrace_getxregs could be static?
The patch looks "obviously correct" but I don't understand this code.
Hmm. We don't read/write the XTENSA_HAVE_COPROCESSORS data, but use
sizeof(elf_xtregs_t) anyway. This looks a bit strange but I guess
this doesn't matter.
Oleg.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Security] [PATCH] xtensa: prevent arbitrary read in ptrace
2011-07-08 18:42 ` Oleg Nesterov
@ 2011-07-08 19:29 ` Chris Zankel
0 siblings, 0 replies; 6+ messages in thread
From: Chris Zankel @ 2011-07-08 19:29 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: Andrew Morton, Dan Rosenberg, security, linux-kernel
Hi,
I'll try to run a build this weekend and will look if that code builds
at all.
Thanks,
-Chris
On 7/8/11 11:42 AM, Oleg Nesterov wrote:
> On 07/08, Andrew Morton wrote:
>>
>> On Thu, 07 Jul 2011 20:03:54 -0400
>> Dan Rosenberg<drosenberg@vsecurity.com> wrote:
>>
>>> Prevent an arbitrary kernel read. Check the user pointer with
>>> access_ok() before copying data in.
>>>
>>> Signed-off-by: Dan Rosenberg<drosenberg@vsecurity.com>
>>> Cc: stable@kernel.org
>>> ---
>>> arch/xtensa/kernel/ptrace.c | 3 +++
>>> 1 files changed, 3 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/arch/xtensa/kernel/ptrace.c b/arch/xtensa/kernel/ptrace.c
>>> index c72c947..ddce75e 100644
>>> --- a/arch/xtensa/kernel/ptrace.c
>>> +++ b/arch/xtensa/kernel/ptrace.c
>>> @@ -147,6 +147,9 @@ int ptrace_setxregs(struct task_struct *child, void __user *uregs)
>>> elf_xtregs_t *xtregs = uregs;
>>> int ret = 0;
>>>
>>> + if (!access_ok(VERIFY_READ, uregs, sizeof(elf_xtregs_t)))
>>> + return -EIO;
>>
>> This should be -EFAULT, methinks?
>
> Also, it seems that ptrace_setxregs/ptrace_getxregs could be static?
>
> The patch looks "obviously correct" but I don't understand this code.
>
> Hmm. We don't read/write the XTENSA_HAVE_COPROCESSORS data, but use
> sizeof(elf_xtregs_t) anyway. This looks a bit strange but I guess
> this doesn't matter.
>
> Oleg.
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Security] [PATCH] xtensa: prevent arbitrary read in ptrace
@ 2011-07-08 18:35 Dan Rosenberg
2011-07-08 18:42 ` Andrew Morton
0 siblings, 1 reply; 6+ messages in thread
From: Dan Rosenberg @ 2011-07-08 18:35 UTC (permalink / raw)
To: Andrew Morton; +Cc: chris, security, linux-kernel, Oleg Nesterov
Sorry for the top post and any email mangling (mobile).
I only used EIO to mirror the existing behavior in ptrace_getxregs(). EFAULT seems better.
-Dan
------Original Message------
From: Andrew Morton
To: Dan Rosenberg
Cc: chris@zankel.net
Cc: security@kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: Oleg Nesterov
Subject: Re: [Security] [PATCH] xtensa: prevent arbitrary read in ptrace
Sent: Jul 8, 2011 2:27 PM
On Thu, 07 Jul 2011 20:03:54 -0400
Dan Rosenberg <drosenberg@vsecurity.com> wrote:
> Prevent an arbitrary kernel read. Check the user pointer with
> access_ok() before copying data in.
>
> Signed-off-by: Dan Rosenberg <drosenberg@vsecurity.com>
> Cc: stable@kernel.org
> ---
> arch/xtensa/kernel/ptrace.c | 3 +++
> 1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/arch/xtensa/kernel/ptrace.c b/arch/xtensa/kernel/ptrace.c
> index c72c947..ddce75e 100644
> --- a/arch/xtensa/kernel/ptrace.c
> +++ b/arch/xtensa/kernel/ptrace.c
> @@ -147,6 +147,9 @@ int ptrace_setxregs(struct task_struct *child, void __user *uregs)
> elf_xtregs_t *xtregs = uregs;
> int ret = 0;
>
> + if (!access_ok(VERIFY_READ, uregs, sizeof(elf_xtregs_t)))
> + return -EIO;
This should be -EFAULT, methinks?
> +
> #if XTENSA_HAVE_COPROCESSORS
> /* Flush all coprocessors before we overwrite them. */
> coprocessor_flush_all(ti);ÿôèº{.nÇ+‰·Ÿ®‰†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a¢f£¢·hšïêÿ‘êçz_è®\x03(階ŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨èÚ&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Security] [PATCH] xtensa: prevent arbitrary read in ptrace
2011-07-08 18:35 Dan Rosenberg
@ 2011-07-08 18:42 ` Andrew Morton
0 siblings, 0 replies; 6+ messages in thread
From: Andrew Morton @ 2011-07-08 18:42 UTC (permalink / raw)
To: drosenberg; +Cc: chris, security, linux-kernel, Oleg Nesterov
On Fri, 8 Jul 2011 18:35:56 +0000
"Dan Rosenberg" <drosenberg@vsecurity.com> wrote:
> Sorry for the top post and any email mangling (mobile).
>
> I only used EIO to mirror the existing behavior in ptrace_getxregs(). EFAULT seems better.
ptrace_getxregs() is busted ;)
int ptrace_getxregs(struct task_struct *child, void __user *uregs)
{
...
if (!access_ok(VERIFY_WRITE, uregs, sizeof(elf_xtregs_t)))
return -EIO;
...
ret |= __copy_to_user(&xtregs->user,&ti->xtregs_user,
sizeof(xtregs->user));
return ret ? -EFAULT : 0;
}
that makes no sense.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-07-08 19:29 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-07-08 0:03 [PATCH] xtensa: prevent arbitrary read in ptrace Dan Rosenberg
2011-07-08 18:27 ` [Security] " Andrew Morton
2011-07-08 18:42 ` Oleg Nesterov
2011-07-08 19:29 ` Chris Zankel
-- strict thread matches above, loose matches on Subject: below --
2011-07-08 18:35 Dan Rosenberg
2011-07-08 18:42 ` Andrew Morton
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox