Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Wed, 19 Feb 2020 09:36:04 -0800
From:   Kees Cook <keescook@chromium.org>
To:     glider@google.com
Cc:     jannh@google.com, ard.biesheuvel@linaro.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] lib/test_stackinit: move a local outside the switch
 statement
Message-ID: <202002190916.EFA74B50C@keescook>
References: <20200218094815.233387-1-glider@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20200218094815.233387-1-glider@google.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, Feb 18, 2020 at 10:48:15AM +0100, glider@google.com wrote:
> Right now CONFIG_INIT_STACK_ALL is unable to initialize locals declared
> in switch statements, see http://llvm.org/PR44916.
> Move the variable declaration outside the switch in lib/test_stackinit.c
> to prevent potential test failures until this is sorted out.
> 
> Cc: Kees Cook <keescook@chromium.org>
> Signed-off-by: Alexander Potapenko <glider@google.com>

Er, no. This test is specifically to catch that case. (i.e. the proposed
GCC version of this feature misses this case too.) We absolutely want
this test to continue to fail until it's fixed:

[   65.546670] test_stackinit: switch_1_none FAIL (uninit bytes: 8)
[   65.547478] test_stackinit: switch_2_none FAIL (uninit bytes: 8)

What would be nice is if Clang could at least _warn_ about these
conditions. GCC does this in the same situation:

fs/fcntl.c: In function ‘send_sigio_to_task’:
fs/fcntl.c:738:13: warning: statement will never be executed [-Wswitch-unreachable]
   siginfo_t si;
             ^~

I have a patch to fix all the switch statement variables, though:

https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/commit/?h=kspp/gcc-plugin/stackinit&id=35ed32e16e13a86370e4b70991db8d5f771ba898

-Kees

> ---
>  lib/test_stackinit.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/test_stackinit.c b/lib/test_stackinit.c
> index 2d7d257a430e..41e2a6e0cdaa 100644
> --- a/lib/test_stackinit.c
> +++ b/lib/test_stackinit.c
> @@ -282,9 +282,9 @@ DEFINE_TEST(user, struct test_user, STRUCT, none);
>   */
>  static int noinline __leaf_switch_none(int path, bool fill)
>  {
> -	switch (path) {
> -		uint64_t var;
> +	uint64_t var;
>  
> +	switch (path) {
>  	case 1:
>  		target_start = &var;
>  		target_size = sizeof(var);
> -- 
> 2.25.0.265.gbab2e86ba0-goog
> 

-- 
Kees Cook