Skip to contentSkip to conversation

xtensa: prevent arbitrary read in ptrace

PATCH In review Dan Rosenberg 4 participants 4 messages Jul 8, 2011

Subsystems (2)
ptrace support Tensilica Xtensa port (xtensa)
lore
Share & tools
lore

Dan Rosenberg

author
#0 lore raw

PATCH 1/1 xtensa: prevent arbitrary read in ptrace

Prevent an arbitrary kernel read. Check the user pointer with access_ok() before copying data in.

Signed-off-by Dan Rosenberg <[email protected]>
Cc [email protected]
---
 arch/xtensa/kernel/ptrace.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)
arch/xtensa/kernel/ptrace.c +3 -0
index c72c947..ddce75e 100644
@@ -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);
#1 lore raw
Quoted 14 lines · On Thu, 07 Jul 2011 20:03:54 -0400 Dan Rosenberg <[email protected]>…

Prevent an arbitrary kernel read. Check the user pointer with access_ok() before copying data in.

Signed-off-by Dan Rosenberg <[email protected]>
Cc [email protected]
---
 arch/xtensa/kernel/ptrace.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)
arch/xtensa/kernel/ptrace.c +0 -0
index c72c947..ddce75e 100644

arch/xtensa/kernel/ptrace.c int ptrace_setxregs(struct task_struct *child, void __user *uregs) · line 151

Quoted 5 lines
arch/xtensa/kernel/ptrace.c +2 -0
@@ -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?

Quoted 4 lines
arch/xtensa/kernel/ptrace.c +1 -0
+
 #if XTENSA_HAVE_COPROCESSORS
 	/* Flush all coprocessors before we overwrite them. */
 	coprocessor_flush_all(ti);
maintainer
#2 lore raw
Quoted 21 lines · Andrew Morton
Quoted 17 lines · On Thu, 07 Jul 2011 20:03:54 -0400 Dan Rosenberg <[email protected]>…

Prevent an arbitrary kernel read. Check the user pointer with access_ok() before copying data in.

Signed-off-by Dan Rosenberg <[email protected]>
Cc [email protected]
---
 arch/xtensa/kernel/ptrace.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)
arch/xtensa/kernel/ptrace.c +0 -0
index c72c947..ddce75e 100644
@@ -147,6 +147,9 @@ int ptrace_setxregs(struct task_struct *child, void __user *uregs)
 	elf_xtregs_t *xtregs = uregs;
 	int ret = 0;
Quoted 4 lines

arch/xtensa/kernel/ptrace.c int ptrace_setxregs(struct task_struct *child, void __user *uregs) · line 151

Quoted 2 lines
arch/xtensa/kernel/ptrace.c +2 -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.

#3 lore raw

Hi,

I'll try to run a build this weekend and will look if that code builds at all.

Thanks,
-Chris

Quoted 37 lines · Oleg Nesterov
Quoted 21 lines · Andrew Morton
Quoted 17 lines · On Thu, 07 Jul 2011 20:03:54 -0400 Dan Rosenberg<[email protected]>…

Prevent an arbitrary kernel read. Check the user pointer with access_ok() before copying data in.

Signed-off-by Dan Rosenberg<[email protected]>
Cc [email protected]
---
  arch/xtensa/kernel/ptrace.c |    3 +++
  1 files changed, 3 insertions(+), 0 deletions(-)
arch/xtensa/kernel/ptrace.c +0 -0
index c72c947..ddce75e 100644
@@ -147,6 +147,9 @@ int ptrace_setxregs(struct task_struct *child, void __user *uregs)
  	elf_xtregs_t *xtregs = uregs;
  	int ret = 0;
Quoted 4 lines

arch/xtensa/kernel/ptrace.c int ptrace_setxregs(struct task_struct *child, void __user *uregs) · line 151

Quoted 2 lines
arch/xtensa/kernel/ptrace.c +2 -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.