linux-kernel - RE: [PATCH v9] kallsyms: Add self-test facility

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 for Android: free password hash cracker in your pocket

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

Message-ID: <b673f98db7d14d53a6e1a1957ef81741@AcuMS.aculab.com>
Date:   Sat, 17 Dec 2022 17:37:14 +0000
From:   David Laight <David.Laight@...LAB.COM>
To:     'Geert Uytterhoeven' <geert@...ux-m68k.org>,
        "Leizhen (ThunderTown)" <thunder.leizhen@...wei.com>
CC:     Steven Rostedt <rostedt@...dmis.org>,
        Andreas Schwab <schwab@...ux-m68k.org>,
        Josh Poimboeuf <jpoimboe@...nel.org>,
        Jiri Kosina <jikos@...nel.org>,
        Miroslav Benes <mbenes@...e.cz>,
        Petr Mladek <pmladek@...e.com>,
        Joe Lawrence <joe.lawrence@...hat.com>,
        "live-patching@...r.kernel.org" <live-patching@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "Masahiro Yamada" <masahiroy@...nel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        "Jiri Olsa" <jolsa@...nel.org>, Kees Cook <keescook@...omium.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Luis Chamberlain <mcgrof@...nel.org>,
        "linux-modules@...r.kernel.org" <linux-modules@...r.kernel.org>,
        Ingo Molnar <mingo@...hat.com>,
        linux-m68k <linux-m68k@...ts.linux-m68k.org>,
        "Jason A. Donenfeld" <Jason@...c4.com>
Subject: RE: [PATCH v9] kallsyms: Add self-test facility

From: Geert Uytterhoeven
> Sent: 17 December 2022 13:37
...
> > So it's quite possible (I didn't disassemble vmlinux, because I didn't learn m68k):

Pretty easy to read it.

...
> > //binary search
> > while (low <= high) {
> >     ...
> >     ret = compare_symbol_name(name, namebuf);   ----> (1)
> >     if (!ret)
> >         break;
> > }
> >
> > low = mid;
> > while (low) {
> >     ...
> >     if (compare_symbol_name(name, namebuf))     ----> (2)
> >         break;
> >     low--;
> > }
> >
> > The pointer 'name' and 'namebuf' of (1) and (2) are the same,
> > so the 'if' statement of (2) maybe omitted by compiler.
> 
> And that is exactly what is happening: there are 3 calls to strcmp()
> with the exact same parameters, and gcc omits two of them, because it
> assumes it can predict the outcome, as the parameters haven't changed.

Additionally if kallsyms_expand_symbol() is inlined all the writes
it does to namebuf[] can get discarded - because they are used.
Which probably causes the entire function to get optimised away.
More code follows - there could easily be nothing left except
the binary chop itself that ensures the loop terminates.

> Now, why have we never noticed this before? I guess because it is very
> uncommon for a function calling strcmp() multiple times with the exact
> same pointer parameters.  Common users change the pointers before
> every call, instead of keeping the pointer, but changing the buffers'
> contents the pointers point to.
> 
> > By the way, I tried no volatile but with
> > +               : : "memory");
> > It also works well.
> 
> Indeed, gcc version 9.4.0 (Ubuntu 9.4.0-1ubuntu1~20.04) generates the
> same code for either adding the volatile or the memory clobber.
> 
> Note that strcmp() is the only function in arch/m68k/include/asm/string.h
> using inline asm without the volatile keyword.  I guess we would
> see similar issues with strnlen() (which also doesn't modify memory)
> when dropping the volatile.

The 'volatile' ensures that strcmp() doesn't get thrown away.
It doesn't ensure that the writes to namebuf[] aren't optimised away.

Only the memory clobber does that.

So all those asm functions need the memory clobber.
They probably don't need volatile - since the call has
a result.
Indeed since the memory clobber also says 'writes to memory'
it may make the volatile pointless.

I'd also add the 'early clobber' annotation to the result.
It may not matter since the pointers are in-out, but since
the pointer outputs are discarded some 'brain-damaged'
register allocation might assign the same register.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)