From: David W. <dw...@in...> - 2000-11-20 14:12:19
|
prumpf said: > I think the patch leaves a security hole: if you put_user to an unaligned > invalid address, the kernel will oops. Yep. I've seen that happen. > The fix is something like (optimized for code size not speed) > > static inline void unaligned_memcpy(void *dst, void *src, int count, > struct pt_regs *regs) > { > int failed; > while(count--) { > asm volatile(" > 1: mov.b @(%2),%0 > bra 4f > 2: mov.b %0,@(%3) > 3: mov #1,%1 > 4: > .section __ex_table,"a" > .long 1b,2b > .previous" : "=r" (tmp), "=r" (failed) : > "r" (dst++), "r" (src++), "0" (0), "1" (0)); > > if (failed) { > die_if_no_fixup("invalid unaligned access", regs, ???); > return; > } > } > } Isn't that fairly much identical to __copy_user()? So a repeated application of the following ought to suffice: - memcpy(dst, src, count); + if (__copy_user(dst, src, count)) { + die_if_no_fixup("Fault in unaligned fixup", regs, 0); + return; + } > An alternative fix, and one which I think I like better, is to try the > search_exception_table before doing any unaligned handling - unaligned > userspace pointers aren`t the kernel`s job to deal with. You mean to search the exception table before attempting the fixup, and to refuse to do the fixup if there's an entry for $pc? I don't like that very much. I think we _should_ be fixing up unaligned userspace accesses. Not only when they're accessed from the kernel, but when they happen entirely in userspace. At the moment, I'm more concerned about what happens when the offending insn is in a branch delay slot. It looks like regs->pc will be pointing to the branch insn iff it was taken, so in that case, we have to fix up the insn at $pc+2 and then also emulate the branch insn follow the branch. -- dwmw2 |
From: David H. <dho...@re...> - 2000-11-22 17:32:00
|
I've attached a patch to do fixups on all instructions that can incur an unaligned access fault, barring "multiply from postinc memory operands". It also handles faults in jump, branch and return delay slots. David Howells ========== diff -uNr linuxsh/arch/sh/kernel/traps.c my-linuxsh/arch/sh/kernel/traps.c --- linuxsh/arch/sh/kernel/traps.c Mon Nov 20 13:27:44 2000 +++ my-linuxsh/arch/sh/kernel/traps.c Wed Nov 22 17:02:07 2000 @@ -29,6 +29,8 @@ #include <asm/atomic.h> #include <asm/processor.h> +#define kdebug(...) /*printk(__VA_ARGS__)*/ + static inline void console_verbose(void) { extern int console_loglevel; @@ -46,7 +48,6 @@ sti(); \ tsk->thread.error_code = error_code; \ tsk->thread.trap_no = trapnr; \ -show_regs(®s); \ force_sig(signr, tsk); \ die_if_no_fixup(str,®s,error_code); \ } @@ -77,7 +78,17 @@ die(str, regs, err); } -static void die_if_no_fixup(const char * str, struct pt_regs * regs, long err) +static int handle_unaligned_notify_count = 10; + +/*****************************************************************************/ +/* + * try and fix up kernelspace address errors + * - userspace errors just cause EFAULT to be returned, resulting in SEGV + * - kernel/userspace interfaces cause a jump to an appropriate handler + * - other kernel errors are bad + * - return 0 if fixed-up, -EFAULT if non-fatal (to the kernel) fault + */ +static int die_if_no_fixup(const char * str, struct pt_regs * regs, long err) { if (!user_mode(regs)) { @@ -85,27 +96,39 @@ fixup = search_exception_table(regs->pc); if (fixup) { regs->pc = fixup; - return; + return 0; } die(str, regs, err); } -} + return -EFAULT; +} /* end die_if_no_fixup() */ -static void handle_unaligned(u16 instruction, struct pt_regs *regs) +/*****************************************************************************/ +/* + * handle an instruction that does an unaligned memory access by emulating the + * desired behaviour + * - note that PC _may not_ point to the faulting instruction (if that instruction + * is in a branch delay slot + * - return 0 if emulation okay, -EFAULT on existential error + */ +static int handle_unaligned_ins(u16 instruction, struct pt_regs *regs) { - int index, count; + int ret, index, count; unsigned long *rm, *rn; unsigned char *src, *dst; - index = (instruction>>8)&15; + index = (instruction>>8)&15; /* 0x0F00 */ rn = ®s->regs[index]; - index = (instruction>>4)&15; + index = (instruction>>4)&15; /* 0x00F0 */ rm = ®s->regs[index]; count = 1<<(instruction&3); - switch(instruction>>12) { + kdebug("handle_unaligned_ins(%p,%04hx)\n",(u16*)regs->pc,instruction); + + ret = -EFAULT; + switch (instruction>>12) { case 0: /* mov.[bwl] to/from memory via r0+rn */ if (instruction & 8) { /* from memory */ @@ -114,102 +137,399 @@ dst = (unsigned char*) rn; *(unsigned long*)dst = 0; -#ifdef __LITTLE_ENDIAN__ - memcpy(dst, src, count); + kdebug("mov (%p),dst\n",src); - if ((count == 2) && dst[1] & 0x80) { - dst[2] = 0xff; - dst[3] = 0xff; - } -#else +#ifdef __BIG_ENDIAN dst += 4-count; - - memcpy(dst, src, count); + + /* We might be trying to fix up an access to a non-existent user page */ + if (__copy_user(dst, src, count)) + goto fetch_fault; if ((count == 2) && dst[2] & 0x80) { dst[0] = 0xff; dst[1] = 0xff; } +#else + /* We might be trying to fix up an access to a non-existent user page */ + if (__copy_user(dst, src, count)) + goto fetch_fault; + + if ((count == 2) && dst[1] & 0x80) { + dst[2] = 0xff; + dst[3] = 0xff; + } #endif } else { /* to memory */ src = (unsigned char*) rm; -#if !defined(__LITTLE_ENDIAN__) +#ifdef __BIG_ENDIAN src += 4-count; #endif dst = (unsigned char*) *rn; dst += regs->regs[0]; - memcpy(dst, src, count); + kdebug("mov src,(%p)\n",dst); + + /* We might be trying to fix up an access to a non-existent user page */ + if (__copy_user(dst, src, count)) + goto fetch_fault; } + + ret = 0; + break; + + case 1: /* mov.l Rm,@(disp,Rn) */ + src = (unsigned char*) rm; + dst = (unsigned char*) *rn; + dst += (instruction&0x000F)<<2; + + /* We might be trying to fix up an access to a non-existent user page */ + if (__copy_user(dst,src,4)) + goto fetch_fault; + ret = 0; break; case 2: /* mov.[bwl] to memory, possibly with pre-decrement */ + kdebug("mov--\n"); if(instruction & 4) *rn -= count; src = (unsigned char*) rm; dst = (unsigned char*) *rn; -#if !defined(__LITTLE_ENDIAN__) +#ifdef __BIG_ENDIAN src += 4-count; #endif - memcpy(dst, src, count); + /* We might be trying to fix up an access to a non-existent user page */ + if (__copy_user(dst, src, count)) + goto fetch_fault; + ret = 0; + break; + + case 5: /* mov.l @(disp,Rm),Rn */ + src = (unsigned char*) *rm; + src += (instruction&0x000F)<<2; + dst = (unsigned char*) rn; + *(unsigned long*)dst = 0; + /* we might be trying to fix up an access to a non-existent user page */ + if (__copy_user(dst,src,4)) + goto fetch_fault; + ret = 0; break; - case 6: - /* mov.[bwl] from memory, possibly with post-increment */ + + case 6: /* mov.[bwl] from memory, possibly with post-increment */ + kdebug("mov++\n"); src = (unsigned char*) *rm; if(instruction & 4) *rm += count; dst = (unsigned char*) rn; *(unsigned long*)dst = 0; - -#ifdef __LITTLE_ENDIAN__ - memcpy(dst, src, count); - if ((count == 2) && dst[1] & 0x80) { - dst[2] = 0xff; - dst[3] = 0xff; - } -#else +#ifdef __BIG_ENDIAN dst += 4-count; - - memcpy(dst, src, count); + + /* We might be trying to fix up an access to a non-existent user page */ + if (__copy_user(dst, src, count)) + goto fetch_fault; if ((count == 2) && dst[2] & 0x80) { dst[0] = 0xff; dst[1] = 0xff; } +#else + /* We might be trying to fix up an access to a non-existent user page */ + if (__copy_user(dst, src, count)) + goto fetch_fault; + + if ((count == 2) && dst[1] & 0x80) { + dst[2] = 0xff; + dst[3] = 0xff; + } +#endif + ret = 0; + break; + + case 8: + switch ((instruction&0xFF00)>>8) { + case 0x81: + /* mov.w R0,@(disp,Rn) */ + src = (unsigned char*) ®s->regs[0]; +#ifdef __BIG_ENDIAN + src += 2; +#endif + dst = (unsigned char*) *rm; /* called Rn in the spec */ + dst += (instruction&0x000F)<<1; + + /* we might be trying to fix up an access to a non-existent user page */ + if (__copy_user(dst, src, 2)) + goto fetch_fault; + ret = 0; + break; + + case 0x85: + /* mov.w @(disp,Rm),R0 */ + src = (unsigned char*) *rm; + src += (instruction&0x000F)<<1; + dst = (unsigned char*) ®s->regs[0]; + *(unsigned long*)dst = 0; + +#ifdef __BIG_ENDIAN + dst += 2; +#endif + + /* we might be trying to fix up an access to a non-existent user page */ + if (__copy_user(dst, src, 2)) + goto fetch_fault; + +#ifdef __BIG_ENDIAN + if (dst[2] & 0x80) { + dst[0] = 0xff; + dst[1] = 0xff; + } +#else + if (dst[1] & 0x80) { + dst[2] = 0xff; + dst[3] = 0xff; + } #endif + ret = 0; + break; + } + break; } - regs->pc += 2; -} + return ret; + + fetch_fault: + /* Argh. Address not only misaligned but also non-existent. + * Raise an EFAULT and see if it's trapped + */ + return die_if_no_fixup("Fault in unaligned fixup", regs, 0); +} /* end handle_unaligned_ins() */ + +/*****************************************************************************/ +/* + * emulate the instruction in the delay slot + * - fetches the instruction from PC+2 + */ +static inline int handle_unaligned_delayslot(struct pt_regs *regs) +{ + u16 instruction; + + if (__copy_user(&instruction, (u16 *)(regs->pc+2), 2)) { + /* the instruction-fetch faulted */ + if (user_mode(regs)) + return -EFAULT; + + /* kernel */ + die("delay-slot-insn faulting in handle_unaligned_delayslot", regs, 0); + } + + return handle_unaligned_ins(instruction,regs); + +} /* end handle_unaligned_delayslot() */ + +/*****************************************************************************/ +/* + * handle an instruction that does an unaligned memory access + * - have to be careful of branch delay-slot instructions that fault + * - if the branch would be taken PC points to the branch + * - if the branch would not be taken, PC points to delay-slot + * - return 0 if handled, -EFAULT if failed (may not return if in kernel) + */ +static int handle_unaligned_access(u16 instruction, struct pt_regs *regs) +{ + u_int rm; + int ret, index; + + index = (instruction>>8)&15; /* 0x0F00 */ + rm = regs->regs[index]; + + /* shout about the first ten userspace fixups */ + if (user_mode(regs) && handle_unaligned_notify_count>0) { + handle_unaligned_notify_count--; + + printk("Fixing up unaligned userspace access in \"%s\" pid=%d pc=0x%p ins=0x%04hx\n", + current->comm,current->pid,(u16*)regs->pc,instruction); + } + + kdebug("handle_unaligned_access(%p,%04hx)\n",(u16*)regs->pc,instruction); + + ret = -EFAULT; + switch (instruction&0xF000) { + case 0x0000: + if (instruction==0x000B) { + /* rts */ + kdebug("rts\n"); + ret = handle_unaligned_delayslot(regs); + if (ret==0) + regs->pc = regs->pr; + } + else if ((instruction&0x00FF)==0x0023) { + /* braf @Rm */ + kdebug("braf @Rm\n"); + ret = handle_unaligned_delayslot(regs); + if (ret==0) + regs->pc += rm + 4; + } + else if ((instruction&0x00FF)==0x0003) { + /* bsrf @Rm */ + kdebug("bsrf @Rm\n"); + ret = handle_unaligned_delayslot(regs); + if (ret==0) { + regs->pr = regs->pc + 4; + regs->pc += rm + 4; + } + } + else { + /* mov.[bwl] to/from memory via r0+rn */ + goto simple; + } + break; + + case 0x1000: /* mov.l Rm,@(disp,Rn) */ + goto simple; + + case 0x2000: /* mov.[bwl] to memory, possibly with pre-decrement */ + goto simple; -asmlinkage void do_address_error(struct pt_regs *regs, + case 0x4000: + if ((instruction&0x00FF)==0x002B) { + /* jmp @Rm */ + kdebug("jmp @Rm\n"); + ret = handle_unaligned_delayslot(regs); + if (ret==0) + regs->pc = rm; + } + else if ((instruction&0x00FF)==0x000B) { + /* jsr @Rm */ + kdebug("jsr @Rm\n"); + ret = handle_unaligned_delayslot(regs); + if (ret==0) { + regs->pr = regs->pc + 4; + regs->pc = rm; + } + } + else { + /* mov.[bwl] to/from memory via r0+rn */ + goto simple; + } + break; + + case 0x5000: /* mov.l @(disp,Rm),Rn */ + goto simple; + + case 0x6000: /* mov.[bwl] from memory, possibly with post-increment */ + goto simple; + + case 0x8000: /* bf lab, bf/s lab, bt lab, bt/s lab */ + switch (instruction&0x0F00) { + case 0x0100: /* mov.w R0,@(disp,Rm) */ + goto simple; + case 0x0500: /* mov.w @(disp,Rm),R0 */ + goto simple; + case 0x0B00: /* bf lab - no delayslot*/ + kdebug("bf label\n"); + break; + case 0x0F00: /* bf/s lab */ + kdebug("bf/s label\n"); + ret = handle_unaligned_delayslot(regs); + if (ret==0) + regs->pc += (instruction&0x00FF)*2 + 4; + break; + case 0x0900: /* bt lab - no delayslot */ + kdebug("bt label\n"); + break; + case 0x0D00: /* bt/s lab */ + kdebug("bt/s label\n"); + ret = handle_unaligned_delayslot(regs); + if (ret==0) + regs->pc += (instruction&0x00FF)*2 + 4; + break; + } + break; + + case 0xA000: /* bra label */ + kdebug("bra label\n"); + ret = handle_unaligned_delayslot(regs); + if (ret==0) + regs->pc += (instruction&0x0FFF)*2 + 4; + break; + + case 0xB000: /* bsr label */ + kdebug("bsr label\n"); + ret = handle_unaligned_delayslot(regs); + if (ret==0) { + regs->pr = regs->pc + 4; + regs->pc += (instruction&0x0FFF)*2 + 4; + } + break; + } + return ret; + + /* handle non-delay-slot instruction */ + simple: + ret = handle_unaligned_ins(instruction,regs); + if (ret==0) + regs->pc += 2; + return ret; +} /* end handle_unaligned_access() */ + +/*****************************************************************************/ +/* + * handle various address error exceptions + */ +asmlinkage void do_address_error(struct pt_regs *regs, unsigned long writeaccess, unsigned long address) { unsigned long error_code; + u16 instruction; + int tmp; asm volatile("stc r2_bank,%0": "=r" (error_code)); - if(user_mode(regs)) { + if (user_mode(regs)) { sti(); + current->thread.error_code = error_code; current->thread.trap_no = (writeaccess) ? 8 : 7; + + /* bad PC is not something we can fix */ + if (regs->pc & 1) + goto uspace_segv; + + if (__copy_user(&instruction, (u16 *)(regs->pc), 2)) { + /* Argh. Fault on the instruction itself. + This should never happen non-SMP + */ + goto uspace_segv; + } + + tmp = handle_unaligned_access(instruction, regs); + + if (tmp==0) + return; /* sorted */ + + uspace_segv: + printk(KERN_NOTICE "Killing process \"%s\" due to unaligned access\n", current->comm); force_sig(SIGSEGV, current); - } else { - u16 instruction; - - if(regs->pc & 1) + } + else { + if (regs->pc & 1) die("unaligned program counter", regs, error_code); - - instruction = *(u16 *)(regs->pc); - - handle_unaligned(instruction, regs); + + if (__copy_user(&instruction, (u16 *)(regs->pc), 2)) { + /* Argh. Fault on the instruction itself. + This should never happen non-SMP + */ + die("insn faulting in do_address_error", regs, 0); + } + + handle_unaligned_access(instruction, regs); } -} +} /* end do_address_error() */ DO_ERROR(12, SIGILL, "reserved instruction", reserved_inst, current) DO_ERROR(13, SIGILL, "illegal slot instruction", illegal_slot_inst, current) |
From: NIIBE Y. <gn...@ch...> - 2000-11-23 01:20:18
|
David Howells writes: > I've attached a patch to do fixups on all instructions that can incur an > unaligned access fault, barring "multiply from postinc memory operands". It > also handles faults in jump, branch and return delay slots. Thanks a lot. Installed (with no kdebug and using LITTLE_ENDIAN). Please check the CVS out. -- |
From: Philipp R. <pr...@pa...> - 2000-11-23 12:59:52
|
On Wed, Nov 22, 2000 at 05:31:41PM +0000, David Howells wrote: > > I've attached a patch to do fixups on all instructions that can incur an > unaligned access fault, barring "multiply from postinc memory operands". It > also handles faults in jump, branch and return delay slots. and adds unaligned userspace access emulation, which I still think is a terribly bad idea; it also adds at least several security holes: 1. accesses to [0x80000000,0xffffffff] generate address errors while in userspace. 2. if the faulting insn is just before 0x80000000, we must not access memory after 0x80000000 to fetch data. 3. unaligned accesses to [0x80000000,0xffffffff] get emulated > +/* > + * try and fix up kernelspace address errors > + * - userspace errors just cause EFAULT to be returned, resulting in SEGV > + * - kernel/userspace interfaces cause a jump to an appropriate handler > + * - other kernel errors are bad > + * - return 0 if fixed-up, -EFAULT if non-fatal (to the kernel) fault > + */ > +static int die_if_no_fixup(const char * str, struct pt_regs * regs, long err) > { > if (!user_mode(regs)) > { > @@ -85,27 +96,39 @@ > fixup = search_exception_table(regs->pc); > if (fixup) { > regs->pc = fixup; > - return; > + return 0; > } > die(str, regs, err); > } > -} > + return -EFAULT; > +} /* end die_if_no_fixup() */ what's the rationale for the -EFAULT return value here ? All the information is already there, and we don't know enough about the context to decide whether we're in a -EFAULT situation at all. > -static void handle_unaligned(u16 instruction, struct pt_regs *regs) > +/*****************************************************************************/ > +/* > + * handle an instruction that does an unaligned memory access by emulating the > + * desired behaviour > + * - note that PC _may not_ point to the faulting instruction (if that instruction > + * is in a branch delay slot > + * - return 0 if emulation okay, -EFAULT on existential error same here .. > + */ > +static int handle_unaligned_ins(u16 instruction, struct pt_regs *regs) > { userspace accesses to [0x80000000,0xffffffff] also get here, and must generate a SEGV rather than a memory access (which doesn't get caught by __copy_user either). > + if (__copy_user(&instruction, (u16 *)(regs->pc), 2)) { > + /* Argh. Fault on the instruction itself. > + This should never happen non-SMP > + */ > + goto uspace_segv; Isn't this what happens when you jsr to an address in the top two GB ? > - } else { > - u16 instruction; > - > - if(regs->pc & 1) > + } > + else { yuck, see CodingStyle ;) |
From: David H. <dho...@re...> - 2000-11-23 14:29:34
|
> and adds unaligned userspace access emulation, which I still think is a > terribly bad idea; insmod can do this a lot. > it also adds at least several security holes: > 1. accesses to [0x80000000,0xffffffff] generate address errors while in > userspace. > 2. if the faulting insn is just before 0x80000000, we must not access memory > after 0x80000000 to fetch data. > 3. unaligned accesses to [0x80000000,0xffffffff] get emulated Good point... I'll fix it and also add this to my test program. > what's the rationale for the -EFAULT return value here ? All the > information is already there, and we don't know enough about the context > to decide whether we're in a -EFAULT situation at all. Because otherwise the calling function would have to do call die_if_no_fixup() and then return -EFAULT in two operations. This way we can make use of gcc's tail-recursion optimisation. > same here .. Firstly, because handle_unaligned_ins() needs to tell its caller whether it succeeded or not when fixing up branch-delay-slot faults (since the branch should not be emulated if the fixup failed), and secondly so that do_address_error() can be told whether to issue a SEGV or not when userspace faults. > Isn't this what happens when you jsr to an address in the top two GB ? Sort of, but in userspace, this doesn't matter since it will be handled separately upon return. > yuck, see CodingStyle ;) I know, I know, but I generally prefer to move the else so that it isn't directly adjacent to the text on the line above (particularly so with else-if's). David |
From: Philipp R. <pr...@pa...> - 2000-11-23 14:57:58
|
On Thu, Nov 23, 2000 at 02:28:57PM +0000, David Howells wrote: > > > and adds unaligned userspace access emulation, which I still think is a > > terribly bad idea; > > insmod can do this a lot. fix insmod. it seems to work on other architectures that don't support unaligned accesses. > > it also adds at least several security holes: > > 1. accesses to [0x80000000,0xffffffff] generate address errors while in > > userspace. > > 2. if the faulting insn is just before 0x80000000, we must not access memory > > after 0x80000000 to fetch data. > > 3. unaligned accesses to [0x80000000,0xffffffff] get emulated > > Good point... I'll fix it and also add this to my test program. at which point the kernel unaligned emulation code will become larger than the kernel-only + user code combined, I think. and definitely slower. > > > what's the rationale for the -EFAULT return value here ? All the > > information is already there, and we don't know enough about the context > > to decide whether we're in a -EFAULT situation at all. > > Because otherwise the calling function would have to do call die_if_no_fixup() > and then return -EFAULT in two operations. This way we can make use of gcc's > tail-recursion optimisation. > > > same here .. > > Firstly, because handle_unaligned_ins() needs to tell its caller whether it > succeeded or not when fixing up branch-delay-slot faults (since the branch > should not be emulated if the fixup failed), and secondly so that > do_address_error() can be told whether to issue a SEGV or not when userspace > faults. why "EFAULT" ?. EFAULT means "a system call couldn't access user data necessary to complete successfully". What we have here is "we tried to access unmapped/inappropriately mapped data". In particular EFAULT is definitely inappropriate when you mean SIGSEGV. > > Isn't this what happens when you jsr to an address in the top two GB ? > > Sort of, but in userspace, this doesn't matter since it will be handled > separately upon return. it just means your comment is wrong. |
From: David M. <Dav...@st...> - 2000-11-23 15:36:51
|
On Nov 23, 2:57pm, pr...@pa... wrote: > Subject: Re: [linuxsh-dev] Re: bad alignment fixup > > On Thu, Nov 23, 2000 at 02:28:57PM +0000, David Howells wrote: > > > > > and adds unaligned userspace access emulation, which I still think is a > > > terribly bad idea; > > > > insmod can do this a lot. > > fix insmod. it seems to work on other architectures that don't support > unaligned accesses. What version of insmod are you using? I've used it a fair bit here, and have seen no problems with it. We are using 2.3.16 plus a few superH specific patches. Is the problem being caused by loading a specific module? I'm also not keen on having the kernel fix up unaligned accesses for me, as if I have a bug in my program I want to know about it. Much of the software I work with has to run under different OSes and processors, some of which silently truncate the bottom bits when you do unaligned accesses! This is not a fun bug to try to track down. The fact that the i386 handles unaligned accesses for you is the exception rather than the rule. I think the Alpha port does a printk if a program does this, so at least you know about it. Of course, it only does it for the first few, otherwise it would be terribly slow. Cheers! -- Dave McKay Software Engineer STMicroelectronics Email: dav...@st... |
From: David H. <dho...@re...> - 2000-11-23 16:53:59
|
> What version of insmod are you using? insmod v2.3.15 > Is the problem being caused by loading a specific module? ext2.o > I'm also not keen on having the kernel fix up unaligned accesses for me, as > if I have a bug in my program I want to know about it. Hence it logs the first 10 fixups it makes. David Howells |
From: Philipp R. <pr...@pa...> - 2000-11-23 18:56:55
|
On Thu, Nov 23, 2000 at 04:53:37PM +0000, David Howells wrote: > > > What version of insmod are you using? > > insmod v2.3.15 > > > Is the problem being caused by loading a specific module? > > ext2.o > > > I'm also not keen on having the kernel fix up unaligned accesses for me, as > > if I have a bug in my program I want to know about it. > > Hence it logs the first 10 fixups it makes. Insufficient for development purposes. At least you need something like alpha's UAC_SIGBUS which allow you to go on debugging without a reboot after the fist 10 unaligned accesses. |
From: Philipp R. <pr...@pa...> - 2000-11-23 18:51:14
|
On Thu, Nov 23, 2000 at 03:36:33PM +0000, David Mckay wrote: > On Nov 23, 2:57pm, pr...@pa... wrote: > > Subject: Re: [linuxsh-dev] Re: bad alignment fixup > > > > On Thu, Nov 23, 2000 at 02:28:57PM +0000, David Howells wrote: > > > > > > > and adds unaligned userspace access emulation, which I still think is a > > > > terribly bad idea; > > > > > > insmod can do this a lot. > > > > fix insmod. it seems to work on other architectures that don't support > > unaligned accesses. > > What version of insmod are you using? I've used it a fair bit here, and have > seen no problems with it. We are using 2.3.16 plus a few superH specific > patches. Is the problem being caused by loading a specific module? > > I'm also not keen on having the kernel fix up unaligned accesses for me, as > if I have a bug in my program I want to know about it. Much of the software > I work with has to run under different OSes and processors, some of which > silently truncate the bottom bits when you do unaligned accesses! This is > not a fun bug to try to track down. Many CPUs don't architecturally support unaligned accesses. We're currently lucky in that Linux doesn't run on those CPUs, or no users use weird network protocols on them. > The fact that the i386 handles unaligned accesses for you is the exception > rather than the rule. It's also not strictly speaking true; 486 and newer CPUs can generate unaligned traps if asked to, and KNI requires aligned operands. The rule for Linux definitely seems to be not to emulate unaligned user accesses. The exception is alpha, which seems to have some architectural support for unaligned accesses. SuperH doesn't. |
From: David H. <dho...@re...> - 2000-11-23 16:03:03
|
Here's another patch to traps.c. this one uses set_fs() and changes all __copy_user() to copy_from_user() or copy_to_user() to protect against userspace programs trying to be sneaky and access kernel space on unaligned addresses. David Howells ============= diff -uNr -x CVS -x .* linuxsh/arch/sh/kernel/traps.c my-linuxsh/arch/sh/kernel/traps.c --- linuxsh/arch/sh/kernel/traps.c Thu Nov 23 09:23:46 2000 +++ my-linuxsh/arch/sh/kernel/traps.c Thu Nov 23 14:59:59 2000 @@ -133,7 +133,7 @@ *(unsigned long*)dst = 0; #ifdef __LITTLE_ENDIAN__ - if (__copy_user(dst, src, count)) + if (copy_from_user(dst, src, count)) goto fetch_fault; if ((count == 2) && dst[1] & 0x80) { @@ -160,7 +160,7 @@ dst = (unsigned char*) *rn; dst += regs->regs[0]; - if (__copy_user(dst, src, count)) + if (copy_to_user(dst, src, count)) goto fetch_fault; } ret = 0; @@ -171,7 +171,7 @@ dst = (unsigned char*) *rn; dst += (instruction&0x000F)<<2; - if (__copy_user(dst,src,4)) + if (copy_to_user(dst,src,4)) goto fetch_fault; ret = 0; break; @@ -184,7 +184,7 @@ #if !defined(__LITTLE_ENDIAN__) src += 4-count; #endif - if (__copy_user(dst, src, count)) + if (copy_to_user(dst, src, count)) goto fetch_fault; ret = 0; break; @@ -195,7 +195,7 @@ dst = (unsigned char*) rn; *(unsigned long*)dst = 0; - if (__copy_user(dst,src,4)) + if (copy_from_user(dst,src,4)) goto fetch_fault; ret = 0; break; @@ -208,7 +208,7 @@ *(unsigned long*)dst = 0; #ifdef __LITTLE_ENDIAN__ - if (__copy_user(dst, src, count)) + if (copy_from_user(dst, src, count)) goto fetch_fault; if ((count == 2) && dst[1] & 0x80) { @@ -218,7 +218,7 @@ #else dst += 4-count; - if (__copy_user(dst, src, count)) + if (copy_from_user(dst, src, count)) goto fetch_fault; if ((count == 2) && dst[2] & 0x80) { @@ -239,7 +239,7 @@ dst = (unsigned char*) *rm; /* called Rn in the spec */ dst += (instruction&0x000F)<<1; - if (__copy_user(dst, src, 2)) + if (copy_to_user(dst, src, 2)) goto fetch_fault; ret = 0; break; @@ -254,7 +254,7 @@ dst += 2; #endif - if (__copy_user(dst, src, 2)) + if (copy_from_user(dst, src, 2)) goto fetch_fault; #ifdef __LITTLE_ENDIAN__ @@ -290,7 +290,7 @@ { u16 instruction; - if (__copy_user(&instruction, (u16 *)(regs->pc+2), 2)) { + if (copy_from_user(&instruction, (u16 *)(regs->pc+2), 2)) { /* the instruction-fetch faulted */ if (user_mode(regs)) return -EFAULT; @@ -442,11 +442,14 @@ unsigned long address) { unsigned long error_code; + mm_segment_t oldfs; u16 instruction; int tmp; asm volatile("stc r2_bank,%0": "=r" (error_code)); + oldfs = get_fs(); + if (user_mode(regs)) { sti(); current->thread.error_code = error_code; @@ -456,14 +459,17 @@ if (regs->pc & 1) goto uspace_segv; - if (__copy_user(&instruction, (u16 *)(regs->pc), 2)) { + set_fs(USER_DS); + if (copy_from_user(&instruction, (u16 *)(regs->pc), 2)) { /* Argh. Fault on the instruction itself. This should never happen non-SMP */ + set_fs(oldfs); goto uspace_segv; } tmp = handle_unaligned_access(instruction, regs); + set_fs(oldfs); if (tmp==0) return; /* sorted */ @@ -475,14 +481,17 @@ if (regs->pc & 1) die("unaligned program counter", regs, error_code); - if (__copy_user(&instruction, (u16 *)(regs->pc), 2)) { + set_fs(KERNEL_DS); + if (copy_from_user(&instruction, (u16 *)(regs->pc), 2)) { /* Argh. Fault on the instruction itself. This should never happen non-SMP */ + set_fs(oldfs); die("insn faulting in do_address_error", regs, 0); } handle_unaligned_access(instruction, regs); + set_fs(oldfs); } } |
From: NIIBE Y. <gn...@ch...> - 2000-11-25 01:26:42
|
David Howells wrote: > Here's another patch to traps.c. this one uses set_fs() and changes all > __copy_user() to copy_from_user() or copy_to_user() to protect against > userspace programs trying to be sneaky and access kernel space on unaligned > addresses. Thanks, installed. -- |
From: Jesper S. <js...@re...> - 2000-11-20 14:27:38
|
>>>>> "David" == David Woodhouse <dw...@in...> writes: David> You mean to search the exception table before attempting the David> fixup, and to refuse to do the fixup if there's an entry for David> $pc? I don't like that very much. I think we _should_ be fixing David> up unaligned userspace accesses. Not only when they're accessed David> from the kernel, but when they happen entirely in userspace. I don't agree. Nasty DOS you could get out of that - simply make an infinite loop read from odd addresses and the kernel will be wasting tons of time fixing up those accesses. User space apps should die if they do unaligned access. David> At the moment, I'm more concerned about what happens when the David> offending insn is in a branch delay slot. It looks like David> regs->pc will be pointing to the branch insn iff it was taken, David> so in that case, we have to fix up the insn at $pc+2 and then David> also emulate the branch insn follow the branch. Urgh. Messy. Jesper |
From: Philipp R. <pr...@pa...> - 2000-11-20 16:05:12
|
On Mon, Nov 20, 2000 at 03:27:01PM +0100, Jesper Skov wrote: > >>>>> "David" == David Woodhouse <dw...@in...> writes: > David> You mean to search the exception table before attempting the > David> fixup, and to refuse to do the fixup if there's an entry for > David> $pc? I don't like that very much. I think we _should_ be fixing > David> up unaligned userspace accesses. Not only when they're accessed > David> from the kernel, but when they happen entirely in userspace. > > I don't agree. Nasty DOS you could get out of that - simply make an > infinite loop read from odd addresses and the kernel will be wasting > tons of time fixing up those accesses. User space apps should die if > they do unaligned access. AIUI, the main cost of emulating unaligned accesses is the user->kernel-> user context switch. user->kernel->SIGBUS handler->user shouldn't be significantly more expensive but avoid all security holes I know about. > David> At the moment, I'm more concerned about what happens when the > David> offending insn is in a branch delay slot. It looks like > David> regs->pc will be pointing to the branch insn iff it was taken, > David> so in that case, we have to fix up the insn at $pc+2 and then > David> also emulate the branch insn follow the branch. > > Urgh. Messy. delay slots are :/ |
From: Bryan R. <br...@ix...> - 2000-11-21 21:29:45
|
Jesper Skov wrote: > I don't agree. Nasty DOS you could get out of that - simply make an > infinite loop read from odd addresses and the kernel will be wasting > tons of time fixing up those accesses. User space apps should die if > they do unaligned access. As a tangential question, is there a way to have the build process (gcc/as/whatever) warn on unaligned access? Obviously there are some (most?) that it can't catch because the pointer changes at run time. But I would really like some error/warning if the linker places a structure's long on a non-long boundary, for instance. I guess what I'm also asking for is a pack or balign or similar directive that works in C, to prevent the problem in the first place. About three weeks ago, one of the guys here lost two days to a structure alignment issue that caused his thread to suddenly zombie after a misaligned memory access. I was a bit shocked at how the current kernel handled this problem... no kernel message, it just terminates the damn thing? For the non-buildtime alignment issues, could we have a build option in the kernel to correct them? (and a note about the potential DoS risk, which is irrelevant to most of us using embedded systems) Or at =least= do a printk? I understand the problems with the kernel workaround, but higher level developers should not have to worry about their process suddenly dying because of some architecture specific alignment behavior. I am bothered by the current behavior of just killing the process with no squawk... That is not robust, and is not easy to debug for people who are not completely familiar with all of the LinuxSH4 internals. Thanks, Bryan --- Bryan Rittmeyer mailto:br...@ix... Ixia Communications 26601 W. Agoura Rd. Calabasas, CA 91302 |
From: Philipp R. <pr...@pa...> - 2000-11-23 13:07:09
|
On Tue, Nov 21, 2000 at 01:27:29PM -0800, Bryan Rittmeyer wrote: > Jesper Skov wrote: > > > I don't agree. Nasty DOS you could get out of that - simply make an > > infinite loop read from odd addresses and the kernel will be wasting > > tons of time fixing up those accesses. User space apps should die if > > they do unaligned access. > > As a tangential question, is there a way to have the build process > (gcc/as/whatever) warn on unaligned access? Obviously there are some gas warns about unaligned .longs. gcc should never generate unaligned accesses unless you use weird casting or assembler code. > (most?) that it can't catch because the pointer changes at run time. But > I would really like some error/warning if the linker places a > structure's long on a non-long boundary, for instance. shouldn't happen. > handled this problem... no kernel message, it just terminates the damn > thing? It sends a SIGSEGV. I'm currently looking into adding a siginfo struct with sufficient information to emulate unaligned accesses efficiently. > higher level developers should not have to worry about their process > suddenly dying because of some architecture specific alignment behavior. Write portable code and it won't. > I am bothered by the current behavior of just killing the process with > no squawk... That is not robust, and is not easy to debug for people who > are not completely familiar with all of the LinuxSH4 internals. If you don't see your process's exit status your test environment is broken. if the process doesn't die with SEGV there's another kernel bug. |
From: David W. <dw...@in...> - 2000-11-23 13:11:28
|
pr...@pa... said: > Write portable code and it won't. insmod(8) was what triggered us to do it. pr...@pa... said: > If you don't see your process's exit status your test environment is > broken. if the process doesn't die with SEGV there's another kernel > bug. You don't see the exit status of insmod when request_module() fails, do you? -- dwmw2 |
From: Philipp R. <pr...@pa...> - 2000-11-23 13:30:30
|
On Thu, Nov 23, 2000 at 01:11:20PM +0000, David Woodhouse wrote: > > pr...@pa... said: > > Write portable code and it won't. > > insmod(8) was what triggered us to do it. > > > pr...@pa... said: > > If you don't see your process's exit status your test environment is > > broken. if the process doesn't die with SEGV there's another kernel > > bug. > > You don't see the exit status of insmod when request_module() fails, do you? exec_usermodehelper is broken. not broken as in "has bugs", but broken as in "has a fundamentlly broken design". this isn't exactly news. nevertheless, there's an obvious fix (not sure whether WIF* are defined in kernelspace, but this shouldn't _be_ in kernelspace in the first place) @@ -167,6 +167,7 @@ { int pid; int waitpid_result; + int waitpid_status; sigset_t tmpsig; int i; static atomic_t kmod_concurrent = ATOMIC_INIT(0); @@ -215,7 +216,7 @@ recalc_sigpending(current); spin_unlock_irq(¤t->sigmask_lock); - waitpid_result = waitpid(pid, NULL, __WCLONE); + waitpid_result = waitpid(pid, &waitpid_status, __WCLONE); atomic_dec(&kmod_concurrent); /* Allow signals again.. */ @@ -228,6 +229,12 @@ printk(KERN_ERR "request_module[%s]: waitpid(%d,...) failed, errno %d\n", module_name, pid, -waitpid_result); } + + if (!WIFSIGNALED(waitpid_status)) { + printk(KERN_ERR "request_module[%s]: modprobe exited with signal %d\n", + module_name, WTERMSIG(waitpid_status)); + } + return 0; } #endif /* CONFIG_KMOD */ |
From: Bryan R. <br...@ix...> - 2000-11-28 20:36:35
|
Philipp Rumpf wrote: > gas warns about unaligned .longs. gcc should never generate unaligned > accesses unless you use weird casting or assembler code. Basically, I think the problem was with code like this: --- #include <stdio.h> struct structone { unsigned short foo; unsigned short bar; unsigned short moo; }; struct structtwo { unsigned char blah; unsigned long boom; }; int main(void) { static struct structone bleep = {1, 2, 3}; static struct structtwo bloop = {1, 2}; bleep.foo=1; bloop.boom=2; printf("%ld",bloop.boom); return 0; } --- I will test that exact snippet on the SH4 and see if we are still having problems. If we are, I guess it's due to the linker either not aligning structtwo or not aligning boom within structtwo? As far as I know that code is standard, valid ANSI C. Please note: we're not using the latest toolchain here, and therefore, can't use the latest kernels. I would hate to go chasing after an already fixed bug, so I will look into testing the other engineer's code with the latest stuff as soon as possible. > shouldn't happen. indeed. > It sends a SIGSEGV. I'm currently looking into adding a siginfo struct > with sufficient information to emulate unaligned accesses efficiently. Ok... in this particular case the unaligned access was coming from a child pthread. Perhaps the pthread parent caught the SIGSEGV and zombied all the children? (sounds like a b-rate horror film) > If you don't see your process's exit status your test environment is broken. Just to make sure--when a process catches, say, a SIGSEGV and hasn't installed a special handler for it, it's up to the shell to display the termination message? I am using the BusyBox shell which may not do so, in which case I will submit a patch to those guys to at least handle SIGSEGV... > if the process doesn't die with SEGV there's another kernel bug. One which may have already been fixed. I will further investigate this issue when I get some spare time (heh, in a year or two). Thanks for the reply, Bryan -- Bryan Rittmeyer mailto:br...@ix... Ixia Communications 26601 W. Agoura Rd. Calabasas, CA 91302 |
From: Philipp R. <pr...@pa...> - 2000-11-28 22:15:23
|
On Tue, Nov 28, 2000 at 12:36:12PM -0800, Bryan Rittmeyer wrote: > Philipp Rumpf wrote: > > > gas warns about unaligned .longs. gcc should never generate unaligned > > accesses unless you use weird casting or assembler code. > > Basically, I think the problem was with code like this: That code definitely should compile and work fine without causing unaligned accesses. > I will test that exact snippet on the SH4 and see if we are still having > problems. If we are, I guess it's due to the linker either not aligning > structtwo or not aligning boom within structtwo? s/linker/compiler/. It would be a GCC bug, yes. > Just to make sure--when a process catches, say, a SIGSEGV and hasn't > installed a special handler for it, it's up to the shell to display the > termination message? yes. |
From: Greg B. <gb...@po...> - 2000-11-30 07:07:32
|
G'day Bryan Rittmeyer wrote: > > Basically, I think the problem was with code like this: > > --- > #include <stdio.h> > [...] The compiler I have here (cvs snapshot from about 2 months ago) generates perfectly-well aligned assembler for this C code: .data .align 1 ! bleep aligned to sizeof(short) .type bleep.3,@object .size bleep.3,6 bleep.3: .short 1 .short 2 .short 3 .align 2 ! bloop aligned to sizeof(long) .type bloop.4,@object .size bloop.4,8 bloop.4: .byte 1 .zero 3 ! bloop.boom aligned to sizeof(long) .long 2 > Please note: we're not using the latest toolchain here, and therefore, > can't use the latest kernels. I would hate to go chasing after an > already fixed bug, so I will look into testing the other engineer's code > with the latest stuff as soon as possible. I know it's hard to keep up with these things, but in the long run it's worth the effort. > > It sends a SIGSEGV. I'm currently looking into adding a siginfo struct > > with sufficient information to emulate unaligned accesses efficiently. My two cents worth on the unaligned accesses issue. <RANT> Just Say No to unaligned accesses. Unaligned accesses should only happen as the result of bad code or compiler bugs, both of which need to have their cause fixed and *not* their symptoms covered up with bletcherous bandaids like exception handlers which decode instructions to fake unaligned accesses. Decoding instructions is a job for the CPU (and debuggers), and should *not* be part of the normal operation of the system. The fact that the Linux networking stack currently relies on unaligned accesses in rare circumstances is a bletcherous relic of its i386 heritage and should be hunted down, shot, and its head stuffed and mounted to remind people of how to write portable code. As long as the kernel has fancy exception handlers which allow the networking code to get away with this behaviour, it will only encourage the disease to spread. </RANT> Ok, I feel better now ;-) Seriously now, I'm concerned about the DoS aspect of relying on unaligned access fixups in the networking stack. Data-dependant fixups in general are fine if used to handle events generated by local processes, because they're self-limiting. Send the process SIGBUS or whatever and the problem goes away. But fixups triggered by network packets are not self-limiting in this way; it may be possible to bring a machine to its knees either maliciously or accidentally. The device I'm working on does wireless networking and might in the future be sold for law enforcement or military applications. With portable wireless devices you can't in general "protect the devices with a router"; they have to be reasonably robust. It can't just go gaga when strange packets comes along. What I'm saying is, in the long term a network stack which is DoS-vulnerable on SH4 is *not* acceptable. The only long-term solution I see is to remove the fixups from the unaligned access exception handler and fix the brokenness in the networking code Finally some practical experience which might help others. I recently had to debug Ben Reed's airo.c driver for the Aironet 4800 wireless ethernet card. The machine was responding to ARP but crashing on the first ping packet. It turns out that inserting the following line in the receive interrupt handler "fixed" it: @@ -942,6 +959,7 @@ apriv->stats.rx_dropped++; } else { char *buffer; + skb_reserve(skb,2); /* IP headers on 16 byte boundaries */ buffer = skb_put( skb, len ); bap_setup( apriv, fid, 0x36+sizeof(len), BAP0 ); bap_read( apriv, (u16*)buffer, len, BAP0 ); Quite a few of the Linux network device drivers do this. It's an example of a bandaid which works around alignment problems in a _relatively_ unbletcherous manner. Obviously its a short term solution only, but I hope it might help someone. Greg. -- These are my opinions not PPIs. |
From: Philipp R. <pr...@pa...> - 2000-11-20 16:04:25
|
On Mon, Nov 20, 2000 at 02:11:26PM +0000, David Woodhouse wrote: > > prumpf said: > > I think the patch leaves a security hole: if you put_user to an unaligned > > invalid address, the kernel will oops. > > Yep. I've seen that happen. that's still a bug _somewhere_ though. either we're passing an invalid pointer to a syscall or we're using {get,put}_user when we shouldn't. > > The fix is something like (optimized for code size not speed) > > > > static inline void unaligned_memcpy(void *dst, void *src, int count, > > struct pt_regs *regs) > > { > > int failed; > > while(count--) { > > asm volatile(" > > 1: mov.b @(%2),%0 > > bra 4f > > 2: mov.b %0,@(%3) > > 3: mov #1,%1 > > 4: > > .section __ex_table,"a" > > .long 1b,2b > > .previous" : "=r" (tmp), "=r" (failed) : > > "r" (dst++), "r" (src++), "0" (0), "1" (0)); > > > > if (failed) { > > die_if_no_fixup("invalid unaligned access", regs, ???); > > return; > > } > > } > > } > > > > Isn't that fairly much identical to __copy_user()? So a repeated > application of the following ought to suffice: > > - memcpy(dst, src, count); > + if (__copy_user(dst, src, count)) { > + die_if_no_fixup("Fault in unaligned fixup", regs, 0); > + return; > + } agreed. > > An alternative fix, and one which I think I like better, is to try the > > search_exception_table before doing any unaligned handling - unaligned > > userspace pointers aren`t the kernel`s job to deal with. > > You mean to search the exception table before attempting the fixup, and to > refuse to do the fixup if there's an entry for $pc? I don't like that very > much. I don't like that very much either, especially since we can test for userspace accesses trivially (dst&src&0x80000000)==0 for userspace accesses. > I think we _should_ be fixing up unaligned userspace accesses. Not > only when they're accessed from the kernel, but when they happen entirely in > userspace. that'd be MAC, TAS, FPU insns, and probably quite a lot of sh5 insns we don't use in the kernel but have to emulate anyway. > At the moment, I'm more concerned about what happens when the offending > insn is in a branch delay slot. It looks like regs->pc will be pointing > to the branch insn iff it was taken, so in that case, we have to fix up the > insn at $pc+2 and then also emulate the branch insn follow the branch. yikes, looks like it. Do we need -mno-mem-access-in-delay-slots for gcc ? |
From: David W. <dw...@in...> - 2000-11-20 16:51:32
|
pr...@pa... said: > that's still a bug _somewhere_ though. either we're passing an > invalid pointer to a syscall or we're using {get,put}_user when we > shouldn't. That'll probably be a bug in our hacked up no-authentication rshd. I don't care if it segfaults when it tries to handle SIGHUP. I do care if my board dies when it does so. pr...@pa... said: > that'd be MAC, TAS, FPU insns, and probably quite a lot of sh5 insns > we don't use in the kernel but have to emulate anyway. hmmm. Perhaps, yes. Unless it's really difficult, though, I think we should follow the example of the Alpha kernel - fix up as much as we can, and have a ratelimited whinge while we do so. pr...@pa... said: > yikes, looks like it. Do we need -mno-mem-access-in-delay-slots for > gcc ? Nah - it doesn't look that hard to emulate the branch too. -- dwmw2 |
From: Philipp R. <pr...@pa...> - 2000-11-20 18:05:36
|
On Mon, Nov 20, 2000 at 04:51:16PM +0000, David Woodhouse wrote: > pr...@pa... said: > > that'd be MAC, TAS, FPU insns, and probably quite a lot of sh5 insns > > we don't use in the kernel but have to emulate anyway. > > hmmm. Perhaps, yes. Unless it's really difficult, though, I think we should > follow the example of the Alpha kernel - fix up as much as we can, and have > a ratelimited whinge while we do so. AFAIK emulating unaligned accesses is part of the Alpha userspace architecture (probably as part of the VAX dynamic recompilation thing originally). That isn't true for SuperH, MIPS, ARM, or any of the other RISC-ish architectures (s390 has an "alignment check" exception as well ..) > pr...@pa... said: > > yikes, looks like it. Do we need -mno-mem-access-in-delay-slots for > > gcc ? > > Nah - it doesn't look that hard to emulate the branch too. doing it correctly certainly requires some thought. |