linux-kernel - Re: [PATCH v5 5/5] powerpc/inst: Optimise copy_inst_from_kernel

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:   Tue, 07 Dec 2021 22:19:05 +1100
From:   Michael Ellerman <mpe@...erman.id.au>
To:     Nathan Chancellor <nathan@...nel.org>
Cc:     Bill Wendling <morbo@...gle.com>,
        Christophe Leroy <christophe.leroy@...roup.eu>,
        kernel test robot <lkp@...el.com>,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Paul Mackerras <paulus@...ba.org>,
        Nick Desaulniers <ndesaulniers@...gle.com>,
        llvm@...ts.linux.dev, kbuild-all@...ts.01.org,
        linuxppc-dev@...ts.ozlabs.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 5/5] powerpc/inst: Optimise
 copy_inst_from_kernel_nofault()

Nathan Chancellor <nathan@...nel.org> writes:
> On Tue, Dec 07, 2021 at 02:37:26PM +1100, Michael Ellerman wrote:
>> Bill Wendling <morbo@...gle.com> writes:
>> > On Tue, Nov 30, 2021 at 10:38 AM Bill Wendling <morbo@...gle.com> wrote:
>> >> On Tue, Nov 30, 2021 at 10:17 AM Nathan Chancellor <nathan@...nel.org> wrote:
>> >> > On Tue, Nov 30, 2021 at 10:25:43PM +1100, Michael Ellerman wrote:
>> >> > > Christophe Leroy <christophe.leroy@...roup.eu> writes:
>> >> > > > Le 29/11/2021 à 23:55, kernel test robot a écrit :
>> ...
>> >> > > >> All warnings (new ones prefixed by >>):
>> >> > > >>
>> >> > > >>     In file included from arch/powerpc/kernel/asm-offsets.c:71:
>> >> > > >>     In file included from arch/powerpc/kernel/../xmon/xmon_bpts.h:7:
>> >> > > >>>> arch/powerpc/include/asm/inst.h:165:20: warning: variable 'val' is uninitialized when used here [-Wuninitialized]
>> >> > > >>                     *inst = ppc_inst(val);
>> >> > > >>                                      ^~~
>> >> > > >>     arch/powerpc/include/asm/inst.h:53:22: note: expanded from macro 'ppc_inst'
>> >> > > >>     #define ppc_inst(x) (x)
>> >> > > >>                          ^
>> >> > > >>     arch/powerpc/include/asm/inst.h:155:18: note: initialize the variable 'val' to silence this warning
>> >> > > >>             unsigned int val, suffix;
>> >> > > >>                             ^
>> >> > > >>                              = 0
>> >> > > >
>> >> > > > I can't understand what's wrong here.
>> ...
>> >> > > >
>> >> > > > I see no possibility, no alternative path where val wouldn't be set. The
>> >> > > > asm clearly has *addr as an output param so it is always set.
>> >> > >
>> >> > > I guess clang can't convince itself of that?
>> ...
>> >> >
>> >> > It certainly looks like there is something wrong with how clang is
>> >> > tracking the initialization of the variable because it looks to me like
>> >> > val is only used in the fallthrough path, which happens after it is
>> >> > initialized via lwz.  Perhaps something is wrong with the logic of
>> >> > https://reviews.llvm.org/D71314?  I've added Bill to CC (LLVM issues are
>> >> > being migrated from Bugzilla to GitHub Issues right now so I cannot file
>> >> > this upstream at the moment).
>> >> >
>> >> If I remove the casts of "val" the warning doesn't appear. I suspect
>> >> that when I wrote that patch I forgot to remove those when checking.
>> >> #include "Captain_Picard_facepalm.h"
>> >>
>> >> I'll look into it.
>> >>
>> > Small retraction. It's the "*(<cast>)&val" that's the issue. (I.e. the "*&")
>> 
>> I guess for now I'll just squash this in as a workaround?
>> 
>> 
>> diff --git a/arch/powerpc/include/asm/inst.h b/arch/powerpc/include/asm/inst.h
>> index 631436f3f5c3..5b591c51fec9 100644
>> --- a/arch/powerpc/include/asm/inst.h
>> +++ b/arch/powerpc/include/asm/inst.h
>> @@ -157,6 +157,9 @@ static inline int copy_inst_from_kernel_nofault(ppc_inst_t *inst, u32 *src)
>>  	if (unlikely(!is_kernel_addr((unsigned long)src)))
>>  		return -ERANGE;
>
> Could we add a version check to this and a link to our bug tracker:
>
> /* https://github.com/ClangBuiltLinux/linux/issues/1521 */
> #if defined(CONFIG_CC_IS_CLANG) && CONFIG_CLANG_VERSION < 140000

Yep, thanks I'll use that.

cheers