On Wed, Nov 08, 2017 at 01:55:06PM +0800, Greentime Hu wrote:

> +#define __range_ok(addr, size) (size <= get_fs() && addr <= (get_fs() -size))
> +
> +#define access_ok(type, addr, size)                 \
> +     __range_ok((unsigned long)addr, (unsigned long)size)

> +#define __get_user_x(__r2,__p,__e,__s,__i...)                                
> \
> +        __asm__ __volatile__ (                                       \
> +             __asmeq("%0", "$r0") __asmeq("%1", "$r2")               \
> +             "bal    __get_user_" #__s                               \

... which does not check access_ok() or do any visible equivalents; OK...

> +#define get_user(x,p)                                                        
> \
> +     ({                                                              \
> +             const register typeof(*(p)) __user *__p asm("$r0") = (p);\
> +             register unsigned long __r2 asm("$r2");                 \
> +             register int __e asm("$r0");                            \
> +             switch (sizeof(*(__p))) {                               \
> +             case 1:                                                 \
> +                     __get_user_x(__r2, __p, __e, 1, "$lp");         \

... and neither does this, which is almost certainly *not* OK.

> +#define put_user(x,p)                                                        
> \

Same here, AFAICS.

> +extern unsigned long __arch_copy_from_user(void *to, const void __user * 
> from,
> +                                        unsigned long n);

> +static inline unsigned long raw_copy_from_user(void *to,
> +                                            const void __user * from,
> +                                            unsigned long n)
> +{
> +     return __arch_copy_from_user(to, from, n);
> +}

Er...  Why not call your __arch_... raw_... and be done with that?

> +#define INLINE_COPY_FROM_USER
> +#define INLINE_COPY_TO_USER

Are those actually worth bothering?  IOW, have you compared behaviour
with and without them?

> +ENTRY(__arch_copy_to_user)
> +     push    $r0
> +     push    $r2
> +     beqz    $r2, ctu_exit
> +     srli    $p0, $r2, #2            ! $p0 = number of word to clear
> +     andi    $r2, $r2, #3            ! Bytes less than a word to copy
> +     beqz    $p0, byte_ctu           ! Only less than a word to copy
> +word_ctu:
> +     lmw.bim $p1, [$r1], $p1         ! Load the next word
> +USER(        smw.bim,$p1, [$r0], $p1)        ! Store the next word

Umm...  It's that happy with unaligned loads and stores?  Your memcpy seems
to be trying to avoid those...

> +9001:
> +     pop     $p1                     ! Original $r2, n
> +     pop     $p0                     ! Original $r0, void *to
> +     sub     $r1, $r0, $p0           ! Bytes copied
> +     sub     $r2, $p1, $r1           ! Bytes left to copy
> +     push    $lp
> +     move    $r0, $p0
> +     bal     memzero                 ! Clean up the memory

Just what memory are you zeroing here?  The one you had been
unable to store into in the first place?

> +ENTRY(__arch_copy_from_user)

> +9001:
> +     pop     $p1                     ! Original $r2, n
> +     pop     $p0                     ! Original $r0, void *to
> +     sub     $r1, $r1, $p0           ! Bytes copied
> +     sub     $r2, $p1, $r1           ! Bytes left to copy
> +     push    $lp
> +     bal     memzero                 ! Clean up the memory

Ditto, only this one is even worse - instead of just oopsing on
you, it will quietly destroy data past the area you've copied
into.  raw_copy_..._user() MUST NOT ZERO ANYTHING.  Ever.

Reply via email to