[linuxsh-dev] Re: bad alignment fixup

[David W. <dw...@in...>]

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