Re: [PATCH] lib/test_stackinit: move a local outside the switch statement

[Kees Cook <keescook@chromium.org>]

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