Re: [PATCH 1/2] powerpc: ptrace CHECK_FULL_REGS
On Mon, 2007-09-24 at 17:59 -0700, Roland McGrath wrote: > > Yup, I think I ditched most of them.. for some reason I decided it > > couldn't happen, but maybe I'm wrong ? > > Well, it's a BUG_ON. It's supposed to be for something that "can't happen". > That's why it's a sanity check, not a wild assertion. ;-) > > The 2/2 patch is an example of a bug that CHECK_FULL_REGS catches. > In the status quo, using PTRACE_PEEKUSR in a bug case crashes while using > PTRACE_GETREGS in the same place might get bogus data. (In the actual bug > thus found, the data is not bogus and only the bit that FULL_REGS checks is.) Fair enough. Ben. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] powerpc: ptrace CHECK_FULL_REGS
> Yup, I think I ditched most of them.. for some reason I decided it > couldn't happen, but maybe I'm wrong ? Well, it's a BUG_ON. It's supposed to be for something that "can't happen". That's why it's a sanity check, not a wild assertion. ;-) The 2/2 patch is an example of a bug that CHECK_FULL_REGS catches. In the status quo, using PTRACE_PEEKUSR in a bug case crashes while using PTRACE_GETREGS in the same place might get bogus data. (In the actual bug thus found, the data is not bogus and only the bit that FULL_REGS checks is.) Thanks, Roland - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] powerpc: ptrace CHECK_FULL_REGS
On Mon, 2007-09-24 at 16:50 -0700, Roland McGrath wrote: > This restores the CHECK_FULL_REGS sanity check to every place that can > access the nonvolatile GPRs for ptrace. This is already done for > native-bitwidth PTRACE_PEEKUSR, but was omitted for many other cases > (32-bit ptrace, PTRACE_GETREGS, etc.); I think there may have been more > uniform checks before that were lost in the recent cleanup of GETREGS et al. Yup, I think I ditched most of them.. for some reason I decided it couldn't happen, but maybe I'm wrong ? Cheers, Ben. > Signed-off-by: Roland McGrath <[EMAIL PROTECTED]> > --- > arch/powerpc/kernel/ptrace.c |4 > arch/powerpc/kernel/ptrace32.c |8 > 2 files changed, 12 insertions(+), 0 deletions(-) > > diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c > index 8a177bd..40d34b3 100644 > --- a/arch/powerpc/kernel/ptrace.c > +++ b/arch/powerpc/kernel/ptrace.c > @@ -331,6 +331,7 @@ static long arch_ptrace_old(struct task_struct *child, > long request, long addr, > unsigned long *reg = &((unsigned long *)child->thread.regs)[0]; > unsigned long __user *tmp = (unsigned long __user *)addr; > > + CHECK_FULL_REGS(child->thread.regs); > for (i = 0; i < 32; i++) { > ret = put_user(*reg, tmp); > if (ret) > @@ -346,6 +347,7 @@ static long arch_ptrace_old(struct task_struct *child, > long request, long addr, > unsigned long *reg = &((unsigned long *)child->thread.regs)[0]; > unsigned long __user *tmp = (unsigned long __user *)addr; > > + CHECK_FULL_REGS(child->thread.regs); > for (i = 0; i < 32; i++) { > ret = get_user(*reg, tmp); > if (ret) > @@ -517,6 +519,7 @@ long arch_ptrace(struct task_struct *child, long request, > long addr, long data) > ret = -EIO; > break; > } > + CHECK_FULL_REGS(child->thread.regs); > ret = 0; > for (ui = 0; ui < PT_REGS_COUNT; ui ++) { > ret |= __put_user(ptrace_get_reg(child, ui), > @@ -537,6 +540,7 @@ long arch_ptrace(struct task_struct *child, long request, > long addr, long data) > ret = -EIO; > break; > } > + CHECK_FULL_REGS(child->thread.regs); > ret = 0; > for (ui = 0; ui < PT_REGS_COUNT; ui ++) { > ret = __get_user(tmp, (unsigned long __user *) data); > diff --git a/arch/powerpc/kernel/ptrace32.c b/arch/powerpc/kernel/ptrace32.c > index 9e6baea..6b86960 100644 > --- a/arch/powerpc/kernel/ptrace32.c > +++ b/arch/powerpc/kernel/ptrace32.c > @@ -53,6 +53,7 @@ static long compat_ptrace_old(struct task_struct *child, > long request, > unsigned long *reg = &((unsigned long *)child->thread.regs)[0]; > unsigned int __user *tmp = (unsigned int __user *)addr; > > + CHECK_FULL_REGS(child->thread.regs); > for (i = 0; i < 32; i++) { > ret = put_user(*reg, tmp); > if (ret) > @@ -68,6 +69,7 @@ static long compat_ptrace_old(struct task_struct *child, > long request, > unsigned long *reg = &((unsigned long *)child->thread.regs)[0]; > unsigned int __user *tmp = (unsigned int __user *)addr; > > + CHECK_FULL_REGS(child->thread.regs); > for (i = 0; i < 32; i++) { > ret = get_user(*reg, tmp); > if (ret) > @@ -164,6 +166,7 @@ long compat_sys_ptrace(int request, int pid, unsigned > long addr, > if ((addr & 3) || (index > PT_FPSCR32)) > break; > > + CHECK_FULL_REGS(child->thread.regs); > if (index < PT_FPR0) { > tmp = ptrace_get_reg(child, index); > } else { > @@ -210,6 +213,7 @@ long compat_sys_ptrace(int request, int pid, unsigned > long addr, > if ((addr & 3) || numReg > PT_FPSCR) > break; > > + CHECK_FULL_REGS(child->thread.regs); > if (numReg >= PT_FPR0) { > flush_fp_to_thread(child); > tmp = ((unsigned long int *)child->thread.fpr)[numReg - > PT_FPR0]; > @@ -270,6 +274,7 @@ long compat_sys_ptrace(int request, int pid, unsigned > long addr, > if ((addr & 3) || (index > PT_FPSCR32)) > break; > > + CHECK_FULL_REGS(child->thread.regs); > if (index < PT_FPR0) { > ret = ptrace_put_reg(child, index, data); > } else { > @@ -307,6 +312,7 @@ long compat_sys_ptrace(int request, int pid, unsigned > long addr, >*/ > if ((addr & 3) || (numReg > PT_FPSCR)) >
[PATCH 1/2] powerpc: ptrace CHECK_FULL_REGS
This restores the CHECK_FULL_REGS sanity check to every place that can access the nonvolatile GPRs for ptrace. This is already done for native-bitwidth PTRACE_PEEKUSR, but was omitted for many other cases (32-bit ptrace, PTRACE_GETREGS, etc.); I think there may have been more uniform checks before that were lost in the recent cleanup of GETREGS et al. Signed-off-by: Roland McGrath <[EMAIL PROTECTED]> --- arch/powerpc/kernel/ptrace.c |4 arch/powerpc/kernel/ptrace32.c |8 2 files changed, 12 insertions(+), 0 deletions(-) diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c index 8a177bd..40d34b3 100644 --- a/arch/powerpc/kernel/ptrace.c +++ b/arch/powerpc/kernel/ptrace.c @@ -331,6 +331,7 @@ static long arch_ptrace_old(struct task_struct *child, long request, long addr, unsigned long *reg = &((unsigned long *)child->thread.regs)[0]; unsigned long __user *tmp = (unsigned long __user *)addr; + CHECK_FULL_REGS(child->thread.regs); for (i = 0; i < 32; i++) { ret = put_user(*reg, tmp); if (ret) @@ -346,6 +347,7 @@ static long arch_ptrace_old(struct task_struct *child, long request, long addr, unsigned long *reg = &((unsigned long *)child->thread.regs)[0]; unsigned long __user *tmp = (unsigned long __user *)addr; + CHECK_FULL_REGS(child->thread.regs); for (i = 0; i < 32; i++) { ret = get_user(*reg, tmp); if (ret) @@ -517,6 +519,7 @@ long arch_ptrace(struct task_struct *child, long request, long addr, long data) ret = -EIO; break; } + CHECK_FULL_REGS(child->thread.regs); ret = 0; for (ui = 0; ui < PT_REGS_COUNT; ui ++) { ret |= __put_user(ptrace_get_reg(child, ui), @@ -537,6 +540,7 @@ long arch_ptrace(struct task_struct *child, long request, long addr, long data) ret = -EIO; break; } + CHECK_FULL_REGS(child->thread.regs); ret = 0; for (ui = 0; ui < PT_REGS_COUNT; ui ++) { ret = __get_user(tmp, (unsigned long __user *) data); diff --git a/arch/powerpc/kernel/ptrace32.c b/arch/powerpc/kernel/ptrace32.c index 9e6baea..6b86960 100644 --- a/arch/powerpc/kernel/ptrace32.c +++ b/arch/powerpc/kernel/ptrace32.c @@ -53,6 +53,7 @@ static long compat_ptrace_old(struct task_struct *child, long request, unsigned long *reg = &((unsigned long *)child->thread.regs)[0]; unsigned int __user *tmp = (unsigned int __user *)addr; + CHECK_FULL_REGS(child->thread.regs); for (i = 0; i < 32; i++) { ret = put_user(*reg, tmp); if (ret) @@ -68,6 +69,7 @@ static long compat_ptrace_old(struct task_struct *child, long request, unsigned long *reg = &((unsigned long *)child->thread.regs)[0]; unsigned int __user *tmp = (unsigned int __user *)addr; + CHECK_FULL_REGS(child->thread.regs); for (i = 0; i < 32; i++) { ret = get_user(*reg, tmp); if (ret) @@ -164,6 +166,7 @@ long compat_sys_ptrace(int request, int pid, unsigned long addr, if ((addr & 3) || (index > PT_FPSCR32)) break; + CHECK_FULL_REGS(child->thread.regs); if (index < PT_FPR0) { tmp = ptrace_get_reg(child, index); } else { @@ -210,6 +213,7 @@ long compat_sys_ptrace(int request, int pid, unsigned long addr, if ((addr & 3) || numReg > PT_FPSCR) break; + CHECK_FULL_REGS(child->thread.regs); if (numReg >= PT_FPR0) { flush_fp_to_thread(child); tmp = ((unsigned long int *)child->thread.fpr)[numReg - PT_FPR0]; @@ -270,6 +274,7 @@ long compat_sys_ptrace(int request, int pid, unsigned long addr, if ((addr & 3) || (index > PT_FPSCR32)) break; + CHECK_FULL_REGS(child->thread.regs); if (index < PT_FPR0) { ret = ptrace_put_reg(child, index, data); } else { @@ -307,6 +312,7 @@ long compat_sys_ptrace(int request, int pid, unsigned long addr, */ if ((addr & 3) || (numReg > PT_FPSCR)) break; + CHECK_FULL_REGS(child->thread.regs); if (numReg < PT_FPR0) { unsigned long freg = ptrace_get_reg(child, numReg); if (index % 2) @@ -342,6 +348,7 @@ long compat_sys_ptrace(int request,
[PATCH 1/2] powerpc: ptrace CHECK_FULL_REGS
This restores the CHECK_FULL_REGS sanity check to every place that can access the nonvolatile GPRs for ptrace. This is already done for native-bitwidth PTRACE_PEEKUSR, but was omitted for many other cases (32-bit ptrace, PTRACE_GETREGS, etc.); I think there may have been more uniform checks before that were lost in the recent cleanup of GETREGS et al. Signed-off-by: Roland McGrath [EMAIL PROTECTED] --- arch/powerpc/kernel/ptrace.c |4 arch/powerpc/kernel/ptrace32.c |8 2 files changed, 12 insertions(+), 0 deletions(-) diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c index 8a177bd..40d34b3 100644 --- a/arch/powerpc/kernel/ptrace.c +++ b/arch/powerpc/kernel/ptrace.c @@ -331,6 +331,7 @@ static long arch_ptrace_old(struct task_struct *child, long request, long addr, unsigned long *reg = ((unsigned long *)child-thread.regs)[0]; unsigned long __user *tmp = (unsigned long __user *)addr; + CHECK_FULL_REGS(child-thread.regs); for (i = 0; i 32; i++) { ret = put_user(*reg, tmp); if (ret) @@ -346,6 +347,7 @@ static long arch_ptrace_old(struct task_struct *child, long request, long addr, unsigned long *reg = ((unsigned long *)child-thread.regs)[0]; unsigned long __user *tmp = (unsigned long __user *)addr; + CHECK_FULL_REGS(child-thread.regs); for (i = 0; i 32; i++) { ret = get_user(*reg, tmp); if (ret) @@ -517,6 +519,7 @@ long arch_ptrace(struct task_struct *child, long request, long addr, long data) ret = -EIO; break; } + CHECK_FULL_REGS(child-thread.regs); ret = 0; for (ui = 0; ui PT_REGS_COUNT; ui ++) { ret |= __put_user(ptrace_get_reg(child, ui), @@ -537,6 +540,7 @@ long arch_ptrace(struct task_struct *child, long request, long addr, long data) ret = -EIO; break; } + CHECK_FULL_REGS(child-thread.regs); ret = 0; for (ui = 0; ui PT_REGS_COUNT; ui ++) { ret = __get_user(tmp, (unsigned long __user *) data); diff --git a/arch/powerpc/kernel/ptrace32.c b/arch/powerpc/kernel/ptrace32.c index 9e6baea..6b86960 100644 --- a/arch/powerpc/kernel/ptrace32.c +++ b/arch/powerpc/kernel/ptrace32.c @@ -53,6 +53,7 @@ static long compat_ptrace_old(struct task_struct *child, long request, unsigned long *reg = ((unsigned long *)child-thread.regs)[0]; unsigned int __user *tmp = (unsigned int __user *)addr; + CHECK_FULL_REGS(child-thread.regs); for (i = 0; i 32; i++) { ret = put_user(*reg, tmp); if (ret) @@ -68,6 +69,7 @@ static long compat_ptrace_old(struct task_struct *child, long request, unsigned long *reg = ((unsigned long *)child-thread.regs)[0]; unsigned int __user *tmp = (unsigned int __user *)addr; + CHECK_FULL_REGS(child-thread.regs); for (i = 0; i 32; i++) { ret = get_user(*reg, tmp); if (ret) @@ -164,6 +166,7 @@ long compat_sys_ptrace(int request, int pid, unsigned long addr, if ((addr 3) || (index PT_FPSCR32)) break; + CHECK_FULL_REGS(child-thread.regs); if (index PT_FPR0) { tmp = ptrace_get_reg(child, index); } else { @@ -210,6 +213,7 @@ long compat_sys_ptrace(int request, int pid, unsigned long addr, if ((addr 3) || numReg PT_FPSCR) break; + CHECK_FULL_REGS(child-thread.regs); if (numReg = PT_FPR0) { flush_fp_to_thread(child); tmp = ((unsigned long int *)child-thread.fpr)[numReg - PT_FPR0]; @@ -270,6 +274,7 @@ long compat_sys_ptrace(int request, int pid, unsigned long addr, if ((addr 3) || (index PT_FPSCR32)) break; + CHECK_FULL_REGS(child-thread.regs); if (index PT_FPR0) { ret = ptrace_put_reg(child, index, data); } else { @@ -307,6 +312,7 @@ long compat_sys_ptrace(int request, int pid, unsigned long addr, */ if ((addr 3) || (numReg PT_FPSCR)) break; + CHECK_FULL_REGS(child-thread.regs); if (numReg PT_FPR0) { unsigned long freg = ptrace_get_reg(child, numReg); if (index % 2) @@ -342,6 +348,7 @@ long compat_sys_ptrace(int request, int pid, unsigned long addr,
Re: [PATCH 1/2] powerpc: ptrace CHECK_FULL_REGS
On Mon, 2007-09-24 at 16:50 -0700, Roland McGrath wrote: This restores the CHECK_FULL_REGS sanity check to every place that can access the nonvolatile GPRs for ptrace. This is already done for native-bitwidth PTRACE_PEEKUSR, but was omitted for many other cases (32-bit ptrace, PTRACE_GETREGS, etc.); I think there may have been more uniform checks before that were lost in the recent cleanup of GETREGS et al. Yup, I think I ditched most of them.. for some reason I decided it couldn't happen, but maybe I'm wrong ? Cheers, Ben. Signed-off-by: Roland McGrath [EMAIL PROTECTED] --- arch/powerpc/kernel/ptrace.c |4 arch/powerpc/kernel/ptrace32.c |8 2 files changed, 12 insertions(+), 0 deletions(-) diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c index 8a177bd..40d34b3 100644 --- a/arch/powerpc/kernel/ptrace.c +++ b/arch/powerpc/kernel/ptrace.c @@ -331,6 +331,7 @@ static long arch_ptrace_old(struct task_struct *child, long request, long addr, unsigned long *reg = ((unsigned long *)child-thread.regs)[0]; unsigned long __user *tmp = (unsigned long __user *)addr; + CHECK_FULL_REGS(child-thread.regs); for (i = 0; i 32; i++) { ret = put_user(*reg, tmp); if (ret) @@ -346,6 +347,7 @@ static long arch_ptrace_old(struct task_struct *child, long request, long addr, unsigned long *reg = ((unsigned long *)child-thread.regs)[0]; unsigned long __user *tmp = (unsigned long __user *)addr; + CHECK_FULL_REGS(child-thread.regs); for (i = 0; i 32; i++) { ret = get_user(*reg, tmp); if (ret) @@ -517,6 +519,7 @@ long arch_ptrace(struct task_struct *child, long request, long addr, long data) ret = -EIO; break; } + CHECK_FULL_REGS(child-thread.regs); ret = 0; for (ui = 0; ui PT_REGS_COUNT; ui ++) { ret |= __put_user(ptrace_get_reg(child, ui), @@ -537,6 +540,7 @@ long arch_ptrace(struct task_struct *child, long request, long addr, long data) ret = -EIO; break; } + CHECK_FULL_REGS(child-thread.regs); ret = 0; for (ui = 0; ui PT_REGS_COUNT; ui ++) { ret = __get_user(tmp, (unsigned long __user *) data); diff --git a/arch/powerpc/kernel/ptrace32.c b/arch/powerpc/kernel/ptrace32.c index 9e6baea..6b86960 100644 --- a/arch/powerpc/kernel/ptrace32.c +++ b/arch/powerpc/kernel/ptrace32.c @@ -53,6 +53,7 @@ static long compat_ptrace_old(struct task_struct *child, long request, unsigned long *reg = ((unsigned long *)child-thread.regs)[0]; unsigned int __user *tmp = (unsigned int __user *)addr; + CHECK_FULL_REGS(child-thread.regs); for (i = 0; i 32; i++) { ret = put_user(*reg, tmp); if (ret) @@ -68,6 +69,7 @@ static long compat_ptrace_old(struct task_struct *child, long request, unsigned long *reg = ((unsigned long *)child-thread.regs)[0]; unsigned int __user *tmp = (unsigned int __user *)addr; + CHECK_FULL_REGS(child-thread.regs); for (i = 0; i 32; i++) { ret = get_user(*reg, tmp); if (ret) @@ -164,6 +166,7 @@ long compat_sys_ptrace(int request, int pid, unsigned long addr, if ((addr 3) || (index PT_FPSCR32)) break; + CHECK_FULL_REGS(child-thread.regs); if (index PT_FPR0) { tmp = ptrace_get_reg(child, index); } else { @@ -210,6 +213,7 @@ long compat_sys_ptrace(int request, int pid, unsigned long addr, if ((addr 3) || numReg PT_FPSCR) break; + CHECK_FULL_REGS(child-thread.regs); if (numReg = PT_FPR0) { flush_fp_to_thread(child); tmp = ((unsigned long int *)child-thread.fpr)[numReg - PT_FPR0]; @@ -270,6 +274,7 @@ long compat_sys_ptrace(int request, int pid, unsigned long addr, if ((addr 3) || (index PT_FPSCR32)) break; + CHECK_FULL_REGS(child-thread.regs); if (index PT_FPR0) { ret = ptrace_put_reg(child, index, data); } else { @@ -307,6 +312,7 @@ long compat_sys_ptrace(int request, int pid, unsigned long addr, */ if ((addr 3) || (numReg PT_FPSCR)) break; + CHECK_FULL_REGS(child-thread.regs); if (numReg PT_FPR0) { unsigned long freg =
Re: [PATCH 1/2] powerpc: ptrace CHECK_FULL_REGS
Yup, I think I ditched most of them.. for some reason I decided it couldn't happen, but maybe I'm wrong ? Well, it's a BUG_ON. It's supposed to be for something that can't happen. That's why it's a sanity check, not a wild assertion. ;-) The 2/2 patch is an example of a bug that CHECK_FULL_REGS catches. In the status quo, using PTRACE_PEEKUSR in a bug case crashes while using PTRACE_GETREGS in the same place might get bogus data. (In the actual bug thus found, the data is not bogus and only the bit that FULL_REGS checks is.) Thanks, Roland - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] powerpc: ptrace CHECK_FULL_REGS
On Mon, 2007-09-24 at 17:59 -0700, Roland McGrath wrote: Yup, I think I ditched most of them.. for some reason I decided it couldn't happen, but maybe I'm wrong ? Well, it's a BUG_ON. It's supposed to be for something that can't happen. That's why it's a sanity check, not a wild assertion. ;-) The 2/2 patch is an example of a bug that CHECK_FULL_REGS catches. In the status quo, using PTRACE_PEEKUSR in a bug case crashes while using PTRACE_GETREGS in the same place might get bogus data. (In the actual bug thus found, the data is not bogus and only the bit that FULL_REGS checks is.) Fair enough. Ben. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/