linux-kernel - Re: ARM: futex: Address build warning

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]

Message-ID: <20200507081924.7c77bfc9@canb.auug.org.au>
Date:   Thu, 7 May 2020 08:19:23 +1000
From:   Stephen Rothwell <sfr@...b.auug.org.au>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     Ingo Molnar <mingo@...e.hu>, "H. Peter Anvin" <hpa@...or.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Linux Next Mailing List <linux-next@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Al Viro <viro@...iv.linux.org.uk>,
        Russell King <rmk+kernel@....linux.org.uk>,
        Arnd Bergmann <arnd@...db.de>,
        linux-arm-kernel@...ts.infradead.org
Subject: Re: ARM: futex: Address build warning

Hi all,

On Tue, 14 Apr 2020 11:07:22 +0200 Thomas Gleixner <tglx@...utronix.de> wrote:
>
> Stephen reported the following build warning on a ARM multi_v7_defconfig
> build with GCC 9.2.1:
> 
> kernel/futex.c: In function 'do_futex':
> kernel/futex.c:1676:17: warning: 'oldval' may be used uninitialized in this function [-Wmaybe-uninitialized]
>  1676 |   return oldval == cmparg;
>       |          ~~~~~~~^~~~~~~~~
> kernel/futex.c:1652:6: note: 'oldval' was declared here
>  1652 |  int oldval, ret;
>       |      ^~~~~~
> 
> introduced by commit a08971e9488d ("futex: arch_futex_atomic_op_inuser()
> calling conventions change").
> 
> While that change should not make any difference it confuses GCC which
> fails to work out that oldval is not referenced when the return value is
> not zero.
> 
> GCC fails to properly analyze arch_futex_atomic_op_inuser(). It's not the
> early return, the issue is with the assembly macros. GCC fails to detect
> that those either set 'ret' to 0 and set oldval or set 'ret' to -EFAULT
> which makes oldval uninteresting. The store to the callsite supplied oldval
> pointer is conditional on ret == 0.
> 
> The straight forward way to solve this is to make the store unconditional.
> 
> Aside of addressing the build warning this makes sense anyway because it
> removes the conditional from the fastpath. In the error case the stored
> value is uninteresting and the extra store does not matter at all.
> 
> Reported-by: Stephen Rothwell <sfr@...b.auug.org.au>
> Signed-off-by: Thomas Gleixner <tglx@...utronix.de>
> Link: https://lore.kernel.org/r/874ku2q18k.fsf@nanos.tec.linutronix.de
> ---
>  arch/arm/include/asm/futex.h |    9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> --- a/arch/arm/include/asm/futex.h
> +++ b/arch/arm/include/asm/futex.h
> @@ -165,8 +165,13 @@ arch_futex_atomic_op_inuser(int op, int
>  	preempt_enable();
>  #endif
>  
> -	if (!ret)
> -		*oval = oldval;
> +	/*
> +	 * Store unconditionally. If ret != 0 the extra store is the least
> +	 * of the worries but GCC cannot figure out that __futex_atomic_op()
> +	 * is either setting ret to -EFAULT or storing the old value in
> +	 * oldval which results in a uninitialized warning at the call site.
> +	 */
> +	*oval = oldval;
>  
>  	return ret;
>  }

Any response to this?  I am still getting the warning ...

The warning was introduced by commit

  a08971e9488d ("futex: arch_futex_atomic_op_inuser() calling conventions change")

Which has been in Linus' tree since before v5.7-rc1.  Should this go in
via the tip tree, the arm tree, or will I just send ti to Linus myself?

-- 
Cheers,
Stephen Rothwell

Content of type "application/pgp-signature" skipped