Message ID | OFAEE03401.0C51FEE2-ONC1257647.002C29FE-C1257647.002C917A@transmode.se (mailing list archive) |
---|---|
State | Superseded, archived |
Headers | show |
> > No, use get_user() not __get_user() or if you use the later, also use > > access_ok(), and test the result in case it errors (if it does, you > > probably want to just goto bad access and SEGV). > > OK, lets see what this gives us: Hrm... did you change anything ? :-) Ben. > diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c > index c33c6de..1bf91d3 100644 > --- a/arch/powerpc/mm/fault.c > +++ b/arch/powerpc/mm/fault.c > @@ -153,7 +153,8 @@ int __kprobes do_page_fault(struct pt_regs *regs, unsigned long address, > #ifdef DEBUG_DCBX > const char *istr = NULL; > > - insn = *((unsigned long *)regs->nip); > + insn = 0; > + __get_user(insn, (unsigned long __user *)regs->nip); > if (((insn >> (31-5)) & 0x3f) == 31) { > if (((insn >> 1) & 0x3ff) == 1014) /* dcbz ? 0x3f6 */ > istr = "dcbz"; > @@ -171,27 +172,32 @@ int __kprobes do_page_fault(struct pt_regs *regs, unsigned long address, > dar = regs->gpr[rb]; > if (ra) > dar += regs->gpr[ra]; > - if (dar != address && address != 0x00f0 && trap == 0x300) > + if (dar != address && trap == 0x300) > printk(KERN_CRIT "%s: address:%lx, dar:%lx!\n", istr, address, dar); > if (!strcmp(istr, "dcbst") && is_write) { > printk(KERN_CRIT "dcbst R%ld,R%ld = %lx as a store, fixing!\n", > ra, rb, dar); > is_write = 0; > } > - > +#if 0 > if (trap == 0x300 && address != dar) { > __asm__ ("mtdar %0" : : "r" (dar)); > return 0; > } > +#endif > } > } > #endif > if (address == 0x00f0 && trap == 0x300) { > - pte_t *ptep; > + //pte_t *ptep; > > /* This is from a dcbX or icbi insn gone bad, these > * insn do not set DAR so we have to do it here instead */ > - insn = *((unsigned long *)regs->nip); > + if (get_user(insn, (unsigned long __user *)regs->nip)) { > + printk(KERN_CRIT "get_user failed, NIP:%lx\n", > + regs->nip); > + goto bad_area_nosemaphore; > + } > > ra = (insn >> (31-15)) & 0x1f; /* Reg RA */ > rb = (insn >> (31-20)) & 0x1f; /* Reg RB */ > @@ -206,7 +212,7 @@ int __kprobes do_page_fault(struct pt_regs *regs, unsigned long address, > trap, address, dar, error_code, istr); > #endif > address = dar; > -#if 1 > +#if 0 > if (is_write && get_pteptr(mm, dar, &ptep, NULL)) { > pte_t my_pte = *ptep; > > @@ -216,7 +222,7 @@ int __kprobes do_page_fault(struct pt_regs *regs, unsigned long address, > } > } > #else > - return 0; > + //return 0; > #endif > } > }
> > > > > No, use get_user() not __get_user() or if you use the later, also use > > > access_ok(), and test the result in case it errors (if it does, you > > > probably want to just goto bad access and SEGV). > > > > OK, lets see what this gives us: > > Hrm... did you change anything ? :-) Yes, see below > > Ben. > > > diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c > > index c33c6de..1bf91d3 100644 > > --- a/arch/powerpc/mm/fault.c > > +++ b/arch/powerpc/mm/fault.c > > @@ -153,7 +153,8 @@ int __kprobes do_page_fault(struct pt_regs *regs, > unsigned long address, > > #ifdef DEBUG_DCBX > > const char *istr = NULL; > > > > - insn = *((unsigned long *)regs->nip); > > + insn = 0; > > + __get_user(insn, (unsigned long __user *)regs->nip); Here I don't care if err. insn will be 0 if it fails and the following if will be false > > if (((insn >> (31-5)) & 0x3f) == 31) { > > if (((insn >> 1) & 0x3ff) == 1014) /* dcbz ? 0x3f6 */ > > istr = "dcbz"; > > @@ -171,27 +172,32 @@ int __kprobes do_page_fault(struct pt_regs *regs, > unsigned long address, > > dar = regs->gpr[rb]; > > if (ra) > > dar += regs->gpr[ra]; > > - if (dar != address && address != 0x00f0 && trap == 0x300) > > + if (dar != address && trap == 0x300) > > printk(KERN_CRIT "%s: address:%lx, dar:%lx!\n", istr, address, dar); > > if (!strcmp(istr, "dcbst") && is_write) { > > printk(KERN_CRIT "dcbst R%ld,R%ld = %lx as a store, fixing!\n", > > ra, rb, dar); > > is_write = 0; > > } > > - > > +#if 0 > > if (trap == 0x300 && address != dar) { > > __asm__ ("mtdar %0" : : "r" (dar)); > > return 0; > > } > > +#endif > > } > > } > > #endif > > if (address == 0x00f0 && trap == 0x300) { > > - pte_t *ptep; > > + //pte_t *ptep; > > > > /* This is from a dcbX or icbi insn gone bad, these > > * insn do not set DAR so we have to do it here instead */ > > - insn = *((unsigned long *)regs->nip); > > + if (get_user(insn, (unsigned long __user *)regs->nip)) { > > + printk(KERN_CRIT "get_user failed, NIP:%lx\n", > > + regs->nip); > > + goto bad_area_nosemaphore; > > + } and here I go to bad_area
On Tue, 2009-10-06 at 12:58 +0200, Joakim Tjernlund wrote: > Here I don't care if err. insn will be 0 if it fails and the following > if will be false I'd rather you use get_user() so it does access_ok(). Else, you can probably manufacture some code that will make the kernel access some MMIO register for example, which could be nasty. At this point, you may as well also check the result even if indeed a fault isn't going to matter. Just makes the code cleaner and avoids some random janitor coming up with a patch later on :-) Cheers, Ben. > > > if (((insn >> (31-5)) & 0x3f) == 31) { > > > if (((insn >> 1) & 0x3ff) == 1014) /* dcbz ? 0x3f6 */ > > > istr = "dcbz"; > > > @@ -171,27 +172,32 @@ int __kprobes do_page_fault(struct pt_regs *regs, > > unsigned long address, > > > dar = regs->gpr[rb]; > > > if (ra) > > > dar += regs->gpr[ra]; > > > - if (dar != address && address != 0x00f0 && trap == 0x300) > > > + if (dar != address && trap == 0x300) > > > printk(KERN_CRIT "%s: address:%lx, dar:%lx!\n", istr, address, dar); > > > if (!strcmp(istr, "dcbst") && is_write) { > > > printk(KERN_CRIT "dcbst R%ld,R%ld = %lx as a store, fixing!\n", > > > ra, rb, dar); > > > is_write = 0; > > > } > > > - > > > +#if 0 > > > if (trap == 0x300 && address != dar) { > > > __asm__ ("mtdar %0" : : "r" (dar)); > > > return 0; > > > } > > > +#endif > > > } > > > } > > > #endif > > > if (address == 0x00f0 && trap == 0x300) { > > > - pte_t *ptep; > > > + //pte_t *ptep; > > > > > > /* This is from a dcbX or icbi insn gone bad, these > > > * insn do not set DAR so we have to do it here instead */ > > > - insn = *((unsigned long *)regs->nip); > > > + if (get_user(insn, (unsigned long __user *)regs->nip)) { > > > + printk(KERN_CRIT "get_user failed, NIP:%lx\n", > > > + regs->nip); > > > + goto bad_area_nosemaphore; > > > + } > > and here I go to bad_area
Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote on 06/10/2009 13:06:26: > From: > > Benjamin Herrenschmidt <benh@kernel.crashing.org> > > To: > > Joakim Tjernlund <joakim.tjernlund@transmode.se> > > Cc: > > "linuxppc-dev@ozlabs.org" <linuxppc-dev@ozlabs.org>, Rex Feany > <RFeany@mrv.com>, Scott Wood <scottwood@freescale.com> > > Date: > > 06/10/2009 13:06 > > Subject: > > Re: [PATCH 0/6] PowerPc 8xx TLB/MMU fixes > > On Tue, 2009-10-06 at 12:58 +0200, Joakim Tjernlund wrote: > > > Here I don't care if err. insn will be 0 if it fails and the following > > if will be false > > I'd rather you use get_user() so it does access_ok(). It is only debug code so it will go away The real access is later. I need do kernel space separately, is there a better way than: if (regs->nip & 0xc0000000) /* kernel space ? */ insn = *((unsigned long *)regs->nip); else if (get_user(insn, (unsigned long *)regs->nip)) { printk(KERN_CRIT "get_user! NIP:%lx\n", regs->nip); /* DEBUG */ goto bad_area_nosemaphore; }
diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c index c33c6de..1bf91d3 100644 --- a/arch/powerpc/mm/fault.c +++ b/arch/powerpc/mm/fault.c @@ -153,7 +153,8 @@ int __kprobes do_page_fault(struct pt_regs *regs, unsigned long address, #ifdef DEBUG_DCBX const char *istr = NULL; - insn = *((unsigned long *)regs->nip); + insn = 0; + __get_user(insn, (unsigned long __user *)regs->nip); if (((insn >> (31-5)) & 0x3f) == 31) { if (((insn >> 1) & 0x3ff) == 1014) /* dcbz ? 0x3f6 */ istr = "dcbz"; @@ -171,27 +172,32 @@ int __kprobes do_page_fault(struct pt_regs *regs, unsigned long address, dar = regs->gpr[rb]; if (ra) dar += regs->gpr[ra]; - if (dar != address && address != 0x00f0 && trap == 0x300) + if (dar != address && trap == 0x300) printk(KERN_CRIT "%s: address:%lx, dar:%lx!\n", istr, address, dar); if (!strcmp(istr, "dcbst") && is_write) { printk(KERN_CRIT "dcbst R%ld,R%ld = %lx as a store, fixing!\n", ra, rb, dar); is_write = 0; } - +#if 0 if (trap == 0x300 && address != dar) { __asm__ ("mtdar %0" : : "r" (dar)); return 0; } +#endif } } #endif if (address == 0x00f0 && trap == 0x300) { - pte_t *ptep; + //pte_t *ptep; /* This is from a dcbX or icbi insn gone bad, these * insn do not set DAR so we have to do it here instead */ - insn = *((unsigned long *)regs->nip); + if (get_user(insn, (unsigned long __user *)regs->nip)) { + printk(KERN_CRIT "get_user failed, NIP:%lx\n", + regs->nip); + goto bad_area_nosemaphore; + } ra = (insn >> (31-15)) & 0x1f; /* Reg RA */ rb = (insn >> (31-20)) & 0x1f; /* Reg RB */ @@ -206,7 +212,7 @@ int __kprobes do_page_fault(struct pt_regs *regs, unsigned long address, trap, address, dar, error_code, istr); #endif address = dar; -#if 1 +#if 0 if (is_write && get_pteptr(mm, dar, &ptep, NULL)) { pte_t my_pte = *ptep; @@ -216,7 +222,7 @@ int __kprobes do_page_fault(struct pt_regs *regs, unsigned long address, } } #else - return 0; + //return 0; #endif } }