On Mon, Jul 12, 2021 at 08:28:55PM +0000, Qing Zhao wrote: > > On Jul 12, 2021, at 12:56 PM, Kees Cook <keesc...@chromium.org> wrote: > > On Wed, Jul 07, 2021 at 05:38:02PM +0000, Qing Zhao wrote: > >> This is the 4th version of the patch for the new security feature for GCC. > > > > It looks like padding initialization has regressed to where things where > > in version 1[1] (it was, however, working in version 2[2]). I'm seeing > > these failures again in the kernel self-test: > > > > test_stackinit: small_hole_static_all FAIL (uninit bytes: 3) > > test_stackinit: big_hole_static_all FAIL (uninit bytes: 61) > > test_stackinit: trailing_hole_static_all FAIL (uninit bytes: 7) > > test_stackinit: small_hole_dynamic_all FAIL (uninit bytes: 3) > > test_stackinit: big_hole_dynamic_all FAIL (uninit bytes: 61) > > test_stackinit: trailing_hole_dynamic_all FAIL (uninit bytes: 7) > > Are the above failures for -ftrivial-auto-var-init=zero or > -ftrivial-auto-var-init=pattern? Or both?
Yes, I was only testing =zero (the kernel test handles =pattern as well: it doesn't explicitly test for 0x00). I've verified with =pattern now, too. > For the current implementation, I believe that all paddings should be > initialized with this option, > for -ftrivial-auto-var-init=zero, the padding will be initialized to zero as > before, however, for > -ftrivial-auto-var-init=pattern, the padding will be initialized to 0xFE > byte-repeatable patterns. I've double-checked that I'm using the right gcc, with the flag. > > > > In looking at the gcc test cases, I think the wrong thing is > > being checked: we want to verify the padding itself. For example, > > in auto-init-17.c, the actual bytes after "four" need to be checked, > > rather than "four" itself. > > ******For the current auto-init-17.c > > 1 /* Verify zero initialization for array type with structure element with > 2 padding. */ > 3 /* { dg-do compile } */ > 4 /* { dg-options "-ftrivial-auto-var-init=zero" } */ > 5 > 6 struct test_trailing_hole { > 7 int one; > 8 int two; > 9 int three; > 10 char four; > 11 /* "sizeof(unsigned long) - 1" byte padding hole here. */ > 12 }; > 13 > 14 > 15 int foo () > 16 { > 17 struct test_trailing_hole var[10]; > 18 return var[2].four; > 19 } > 20 > 21 /* { dg-final { scan-assembler "movl\t\\\$0," } } */ > 22 /* { dg-final { scan-assembler "movl\t\\\$20," } } */ > 23 /* { dg-final { scan-assembler "rep stosq" } } */ > ~ > ******We have the assembly as: (-ftrivial-auto-var-init=zero) > > .file "auto-init-17.c" > .text > .globl foo > .type foo, @function > foo: > .LFB0: > .cfi_startproc > pushq %rbp > .cfi_def_cfa_offset 16 > .cfi_offset 6, -16 > movq %rsp, %rbp > .cfi_def_cfa_register 6 > subq $40, %rsp > leaq -160(%rbp), %rax > movq %rax, %rsi > movl $0, %eax > movl $20, %edx > movq %rsi, %rdi > movq %rdx, %rcx > rep stosq > movzbl -116(%rbp), %eax > movsbl %al, %eax > leave > .cfi_def_cfa 7, 8 > ret > .cfi_endproc > .LFE0: > .size foo, .-foo > .section .note.GNU-stack,"",@progbits > > From the above, we can see, “zero” will be used to initialize 8 * 20 = 16 * > 10 bytes of memory starting from the beginning of “var”, that include all the > padding holes inside > This array of structure. > > I didn’t see issue with padding initialization here. Hm, agreed -- this test does do the right thing. > > But this isn't actually sufficient because they may _accidentally_ > > be zero already. The kernel tests specifically make sure to fill the > > about-to-be-used stack with 0xff before calling a function like foo() > > above. I've extracted the kernel test to build for userspace, and it behaves the same way. See attached "stackinit.c". $ gcc-build/auto-var-init.4/installed/bin/gcc -O2 -Wall -o stackinit stackinit.c $ ./stackinit 2>&1 | grep failures: stackinit: failures: 23 $ gcc-build/auto-var-init.4/installed/bin/gcc -O2 -Wall -ftrivial-auto-var-init=zero -o stackinit stackinit.c stackinit.c: In function ‘__leaf_switch_none’: stackinit.c:326:26: warning: statement will never be executed [-Wswitch-unreachable] 326 | uint64_t var; | ^~~ $ ./stackinit 2>&1 | grep failures: stackinit: failures: 6 Same failures as seen in the kernel test (and an expected warning about the initialization that will never happen for a pre-case switch statement). > > > > (And as an aside, it seems like naming the test cases with some details > > about what is being tested in the filename would be nice -- it was > > a little weird having to dig through their numeric names to find the > > padding tests.) > > Yes, I will fix the testing names to more reflect the testing details. Great! -- Kees Cook
// SPDX-License-Identifier: GPL-2.0-or-later /* * Test cases for compiler-based stack variable zeroing via * -ftrivial-auto-var-init={zero,pattern} or CONFIG_GCC_PLUGIN_STRUCTLEAK*. * * Build example: * gcc -O2 -Wall -ftrivial-auto-var-init=zero -o stackinit stackinit.c */ /* Userspace headers. */ #include <stdio.h> #include <stdint.h> #include <string.h> #include <stdbool.h> #include <errno.h> /* Linux kernel-isms */ #define KBUILD_MODNAME "stackinit" #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt #define pr_err(fmt, ...) fprintf(stderr, pr_fmt(fmt), ##__VA_ARGS__) #define pr_warn(fmt, ...) fprintf(stderr, pr_fmt(fmt), ##__VA_ARGS__) #define pr_info(fmt, ...) fprintf(stdout, pr_fmt(fmt), ##__VA_ARGS__) #define __init /**/ #define __user /**/ #define noinline __attribute__((__noinline__)) #define __aligned(x) __attribute__((__aligned__(x))) #ifdef __clang__ # define __compiletime_error(message) /**/ #else # define __compiletime_error(message) __attribute__((__error__(message))) #endif #define __compiletime_assert(condition, msg, prefix, suffix) \ do { \ extern void prefix ## suffix(void) __compiletime_error(msg); \ if (!(condition)) \ prefix ## suffix(); \ } while (0) #define _compiletime_assert(condition, msg, prefix, suffix) \ __compiletime_assert(condition, msg, prefix, suffix) #define compiletime_assert(condition, msg) \ _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__) #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg) #define BUILD_BUG_ON(condition) \ BUILD_BUG_ON_MSG(condition, "BUILD_BUG_ON failed: " #condition) typedef uint8_t u8; typedef uint16_t u16; typedef uint32_t u32; typedef uint64_t u64; /* Exfiltration buffer. */ #define MAX_VAR_SIZE 128 static u8 check_buf[MAX_VAR_SIZE]; /* Character array to trigger stack protector in all functions. */ #define VAR_BUFFER 32 /* Volatile mask to convince compiler to copy memory with 0xff. */ static volatile u8 forced_mask = 0xff; /* Location and size tracking to validate fill and test are colocated. */ static void *fill_start, *target_start; static size_t fill_size, target_size; static bool range_contains(char *haystack_start, size_t haystack_size, char *needle_start, size_t needle_size) { if (needle_start >= haystack_start && needle_start + needle_size <= haystack_start + haystack_size) return true; return false; } #define DO_NOTHING_TYPE_SCALAR(var_type) var_type #define DO_NOTHING_TYPE_STRING(var_type) void #define DO_NOTHING_TYPE_STRUCT(var_type) void #define DO_NOTHING_RETURN_SCALAR(ptr) *(ptr) #define DO_NOTHING_RETURN_STRING(ptr) /**/ #define DO_NOTHING_RETURN_STRUCT(ptr) /**/ #define DO_NOTHING_CALL_SCALAR(var, name) \ (var) = do_nothing_ ## name(&(var)) #define DO_NOTHING_CALL_STRING(var, name) \ do_nothing_ ## name(var) #define DO_NOTHING_CALL_STRUCT(var, name) \ do_nothing_ ## name(&(var)) #define FETCH_ARG_SCALAR(var) &var #define FETCH_ARG_STRING(var) var #define FETCH_ARG_STRUCT(var) &var #define FILL_SIZE_STRING 16 #define INIT_CLONE_SCALAR /**/ #define INIT_CLONE_STRING [FILL_SIZE_STRING] #define INIT_CLONE_STRUCT /**/ #define INIT_SCALAR_none /**/ #define INIT_SCALAR_zero = 0 #define INIT_STRING_none [FILL_SIZE_STRING] /**/ #define INIT_STRING_zero [FILL_SIZE_STRING] = { } #define INIT_STRUCT_none /**/ #define INIT_STRUCT_zero = { } #define INIT_STRUCT_static_partial = { .two = 0, } #define INIT_STRUCT_static_all = { .one = arg->one, \ .two = arg->two, \ .three = arg->three, \ .four = arg->four, \ } #define INIT_STRUCT_dynamic_partial = { .two = arg->two, } #define INIT_STRUCT_dynamic_all = { .one = arg->one, \ .two = arg->two, \ .three = arg->three, \ .four = arg->four, \ } #define INIT_STRUCT_runtime_partial ; \ var.two = 0 #define INIT_STRUCT_runtime_all ; \ var.one = 0; \ var.two = 0; \ var.three = 0; \ memset(&var.four, 0, \ sizeof(var.four)) /* * @name: unique string name for the test * @var_type: type to be tested for zeroing initialization * @which: is this a SCALAR, STRING, or STRUCT type? * @init_level: what kind of initialization is performed * @xfail: is this test expected to fail? */ #define DEFINE_TEST_DRIVER(name, var_type, which, xfail) \ /* Returns 0 on success, 1 on failure. */ \ static noinline __init int test_ ## name (void) \ { \ var_type zero INIT_CLONE_ ## which; \ int ignored; \ u8 sum = 0, i; \ \ /* Notice when a new test is larger than expected. */ \ BUILD_BUG_ON(sizeof(zero) > MAX_VAR_SIZE); \ \ /* Fill clone type with zero for per-field init. */ \ memset(&zero, 0x00, sizeof(zero)); \ /* Clear entire check buffer for 0xFF overlap test. */ \ memset(check_buf, 0x00, sizeof(check_buf)); \ /* Fill stack with 0xFF. */ \ ignored = leaf_ ##name((unsigned long)&ignored, 1, \ FETCH_ARG_ ## which(zero)); \ /* Verify all bytes overwritten with 0xFF. */ \ for (sum = 0, i = 0; i < target_size; i++) \ sum += (check_buf[i] != 0xFF); \ if (sum) { \ pr_err(#name ": leaf fill was not 0xFF!?\n"); \ return 1; \ } \ /* Clear entire check buffer for later bit tests. */ \ memset(check_buf, 0x00, sizeof(check_buf)); \ /* Extract stack-defined variable contents. */ \ ignored = leaf_ ##name((unsigned long)&ignored, 0, \ FETCH_ARG_ ## which(zero)); \ \ /* Validate that compiler lined up fill and target. */ \ if (!range_contains(fill_start, fill_size, \ target_start, target_size)) { \ pr_err(#name ": stack fill missed target!?\n"); \ pr_err(#name ": fill %zu wide\n", fill_size); \ pr_err(#name ": target offset by %d\n", \ (int)((ssize_t)(uintptr_t)fill_start - \ (ssize_t)(uintptr_t)target_start)); \ return 1; \ } \ \ /* Look for any bytes still 0xFF in check region. */ \ for (sum = 0, i = 0; i < target_size; i++) \ sum += (check_buf[i] == 0xFF); \ \ if (sum == 0) { \ pr_info(#name " ok\n"); \ return 0; \ } else { \ pr_warn(#name " %sFAIL (uninit bytes: %d)\n", \ (xfail) ? "X" : "", sum); \ return (xfail) ? 0 : 1; \ } \ } #define DEFINE_TEST(name, var_type, which, init_level) \ /* no-op to force compiler into ignoring "uninitialized" vars */\ static noinline __init DO_NOTHING_TYPE_ ## which(var_type) \ do_nothing_ ## name(var_type *ptr) \ { \ /* Will always be true, but compiler doesn't know. */ \ if ((unsigned long)ptr > 0x2) \ return DO_NOTHING_RETURN_ ## which(ptr); \ else \ return DO_NOTHING_RETURN_ ## which(ptr + 1); \ } \ static noinline __init int leaf_ ## name(unsigned long sp, \ bool fill, \ var_type *arg) \ { \ char buf[VAR_BUFFER]; \ var_type var INIT_ ## which ## _ ## init_level; \ \ target_start = &var; \ target_size = sizeof(var); \ /* \ * Keep this buffer around to make sure we've got a \ * stack frame of SOME kind... \ */ \ memset(buf, (char)(sp & 0xff), sizeof(buf)); \ /* Fill variable with 0xFF. */ \ if (fill) { \ fill_start = &var; \ fill_size = sizeof(var); \ memset(fill_start, \ (char)((sp & 0xff) | forced_mask), \ fill_size); \ } \ \ /* Silence "never initialized" warnings. */ \ DO_NOTHING_CALL_ ## which(var, name); \ \ /* Exfiltrate "var". */ \ memcpy(check_buf, target_start, target_size); \ \ return (int)buf[0] | (int)buf[sizeof(buf) - 1]; \ } \ DEFINE_TEST_DRIVER(name, var_type, which, 0) /* Structure with no padding. */ struct test_packed { unsigned long one; unsigned long two; unsigned long three; unsigned long four; }; /* Simple structure with padding likely to be covered by compiler. */ struct test_small_hole { size_t one; char two; /* 3 byte padding hole here. */ int three; unsigned long four; }; /* Try to trigger unhandled padding in a structure. */ struct test_aligned { u32 internal1; u64 internal2; } __aligned(64); struct test_big_hole { u8 one; u8 two; u8 three; /* 61 byte padding hole here. */ struct test_aligned four; } __aligned(64); struct test_trailing_hole { char *one; char *two; char *three; char four; /* "sizeof(unsigned long) - 1" byte padding hole here. */ }; /* Test if STRUCTLEAK is clearing structs with __user fields. */ struct test_user { u8 one; unsigned long two; char __user *three; unsigned long four; }; #define DEFINE_SCALAR_TEST(name, init) \ DEFINE_TEST(name ## _ ## init, name, SCALAR, init) #define DEFINE_SCALAR_TESTS(init) \ DEFINE_SCALAR_TEST(u8, init); \ DEFINE_SCALAR_TEST(u16, init); \ DEFINE_SCALAR_TEST(u32, init); \ DEFINE_SCALAR_TEST(u64, init); \ DEFINE_TEST(char_array_ ## init, unsigned char, STRING, init) #define DEFINE_STRUCT_TEST(name, init) \ DEFINE_TEST(name ## _ ## init, \ struct test_ ## name, STRUCT, init) #define DEFINE_STRUCT_TESTS(init) \ DEFINE_STRUCT_TEST(small_hole, init); \ DEFINE_STRUCT_TEST(big_hole, init); \ DEFINE_STRUCT_TEST(trailing_hole, init); \ DEFINE_STRUCT_TEST(packed, init) /* These should be fully initialized all the time! */ DEFINE_SCALAR_TESTS(zero); DEFINE_STRUCT_TESTS(zero); /* Static initialization: padding may be left uninitialized. */ DEFINE_STRUCT_TESTS(static_partial); DEFINE_STRUCT_TESTS(static_all); /* Dynamic initialization: padding may be left uninitialized. */ DEFINE_STRUCT_TESTS(dynamic_partial); DEFINE_STRUCT_TESTS(dynamic_all); /* Runtime initialization: padding may be left uninitialized. */ DEFINE_STRUCT_TESTS(runtime_partial); DEFINE_STRUCT_TESTS(runtime_all); /* No initialization without compiler instrumentation. */ DEFINE_SCALAR_TESTS(none); DEFINE_STRUCT_TESTS(none); DEFINE_TEST(user, struct test_user, STRUCT, none); /* * Check two uses through a variable declaration outside either path, * which was noticed as a special case in porting earlier stack init * compiler logic. */ static int noinline __leaf_switch_none(int path, bool fill) { switch (path) { uint64_t var; case 1: target_start = &var; target_size = sizeof(var); if (fill) { fill_start = &var; fill_size = sizeof(var); memset(fill_start, forced_mask | 0x55, fill_size); } memcpy(check_buf, target_start, target_size); break; case 2: target_start = &var; target_size = sizeof(var); if (fill) { fill_start = &var; fill_size = sizeof(var); memset(fill_start, forced_mask | 0xaa, fill_size); } memcpy(check_buf, target_start, target_size); break; default: var = 5; return var & forced_mask; } return 0; } static noinline __init int leaf_switch_1_none(unsigned long sp, bool fill, uint64_t *arg) { return __leaf_switch_none(1, fill); } static noinline __init int leaf_switch_2_none(unsigned long sp, bool fill, uint64_t *arg) { return __leaf_switch_none(2, fill); } /* * These are expected to fail for most configurations because neither * GCC nor Clang have a way to perform initialization of variables in * non-code areas (i.e. in a switch statement before the first "case"). * https://bugs.llvm.org/show_bug.cgi?id=44916 */ DEFINE_TEST_DRIVER(switch_1_none, uint64_t, SCALAR, 1); DEFINE_TEST_DRIVER(switch_2_none, uint64_t, SCALAR, 1); static int __init test_stackinit_init(void) { unsigned int failures = 0; #define test_scalars(init) do { \ failures += test_u8_ ## init (); \ failures += test_u16_ ## init (); \ failures += test_u32_ ## init (); \ failures += test_u64_ ## init (); \ failures += test_char_array_ ## init (); \ } while (0) #define test_structs(init) do { \ failures += test_small_hole_ ## init (); \ failures += test_big_hole_ ## init (); \ failures += test_trailing_hole_ ## init (); \ failures += test_packed_ ## init (); \ } while (0) /* These are explicitly initialized and should always pass. */ test_scalars(zero); test_structs(zero); /* Padding here appears to be accidentally always initialized? */ test_structs(dynamic_partial); /* Padding initialization depends on compiler behaviors. */ test_structs(static_partial); test_structs(static_all); test_structs(dynamic_all); test_structs(runtime_partial); test_structs(runtime_all); /* STRUCTLEAK_BYREF_ALL should cover everything from here down. */ test_scalars(none); failures += test_switch_1_none(); failures += test_switch_2_none(); /* STRUCTLEAK_BYREF should cover from here down. */ test_structs(none); /* STRUCTLEAK will only cover this. */ failures += test_user(); if (failures == 0) pr_info("all tests passed!\n"); else pr_err("failures: %u\n", failures); return failures ? -EINVAL : 0; } int main(void) { return test_stackinit_init(); }