linux-kernel - Re: Runtime failure running sh:qemu in -next due to 'sh: fix copy_from

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 16 Sep 2016 22:31:41 +0100
From:   Al Viro <viro@...IV.linux.org.uk>
To:     Guenter Roeck <linux@...ck-us.net>
Cc:     linux-kernel@...r.kernel.org,
        Yoshinori Sato <ysato@...rs.sourceforge.jp>,
        linux-sh@...r.kernel.org, Rich Felker <dalias@...c.org>
Subject: Re: Runtime failure running sh:qemu in -next due to 'sh: fix
 copy_from_user()'

On Fri, Sep 16, 2016 at 01:59:38PM -0700, Guenter Roeck wrote:
> Yes, reverting 6e050503a150 fixes the problem.
> 
> I added a BUG() into the "if (unlikely())" below, but it doesn't catch,
> and I still get the ip: OVERRUN errors. Which leaves me a bit puzzled.
> 
> Guenter
> 
> > The change in question is
> >         if (__copy_size && __access_ok(__copy_from, __copy_size))
> > -               return __copy_user(to, from, __copy_size);
> > +               __copy_size = __copy_user(to, from, __copy_size);
> > +
> > +       if (unlikely(__copy_size))
> > +               memset(to + (n - __copy_size), 0, __copy_size);
> >  
> >         return __copy_size;

So we don't even hit that memset()?  What the hell?  __copy_user() is
declared as
__kernel_size_t __copy_user(void *to, const void *from, __kernel_size_t n);

and __copy_size copy_from_user() is

        __kernel_size_t __copy_size = (__kernel_size_t) n;

So
	return __copy_user(to, from, __copy_size);
and
	__copy_size = __copy_user(to, from, __copy_size);
	return __copy_size;
ought to be doing exactly the same thing.  At that point it's starting to
smell like a compiler bug somewhere in there.

Try to remove that (not triggered) if (unlikely(__copy_size)) memset(...)
and see if that's enough to recover.  And it would be nice to see what
all three variants (as it is, with commit reverted and with just that if
removed) generate in e.g. sys_utimensat() (fs/utimes.s)