Hi, Kees,
Thanks a lot for your testings on linux kernel.
I am happy to know that the initial implementation works fine.
I will study the padding case and the switch case to fix the issues there.
> On Feb 24, 2021, at 10:41 PM, Kees Cook <keesc...@chromium.org> wrote:
>
> (please keep me in CC, I'm not subscribed...)
Yes, I will.
>
> On Thu Feb 18, 2021 Qing Zhao said:
>> Initialize automatic variables with new first class option
>> -ftrivial-auto-var-init=[uninitialized|pattern|zero]
>
> Yay! I'm really excited to see this. Thank you for working on
> it! I've built GCC with this applied, and it works out of the box
> for a Linux kernel build, which correctly detects the availability
> of -ftrivial-auto-var-init=[pattern|zero] for the respective
> CONFIG_INIT_STACK_ALL_PATTERN and CONFIG_INIT_STACK_ALL_ZERO options.
>
> The output from the kernel's CONFIG_TEST_STACKINIT module shows coverage
> for most uninitialized cases. Yay! :)
>
> It looks like there is still some issues with padding and pre-case
> switch variables. Here's the test output, FWIW:
>
> test_stackinit: u8_zero ok
> test_stackinit: u16_zero ok
> test_stackinit: u32_zero ok
> test_stackinit: u64_zero ok
> test_stackinit: char_array_zero ok
> test_stackinit: small_hole_zero ok
> test_stackinit: big_hole_zero ok
> test_stackinit: trailing_hole_zero ok
> test_stackinit: packed_zero ok
> test_stackinit: small_hole_dynamic_partial ok
> test_stackinit: big_hole_dynamic_partial ok
> test_stackinit: trailing_hole_dynamic_partial ok
> test_stackinit: packed_dynamic_partial ok
> test_stackinit: small_hole_static_partial ok
> test_stackinit: big_hole_static_partial ok
> test_stackinit: trailing_hole_static_partial ok
> test_stackinit: packed_static_partial ok
> 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: packed_static_all ok
> 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)
> test_stackinit: packed_dynamic_all ok
> test_stackinit: small_hole_runtime_partial ok
> test_stackinit: big_hole_runtime_partial ok
> test_stackinit: trailing_hole_runtime_partial ok
> test_stackinit: packed_runtime_partial ok
> test_stackinit: small_hole_runtime_all ok
> test_stackinit: big_hole_runtime_all ok
> test_stackinit: trailing_hole_runtime_all ok
> test_stackinit: packed_runtime_all ok
> test_stackinit: u8_none ok
> test_stackinit: u16_none ok
> test_stackinit: u32_none ok
> test_stackinit: u64_none ok
> test_stackinit: char_array_none ok
> test_stackinit: switch_1_none FAIL (uninit bytes: 8)
> test_stackinit: switch_2_none FAIL (uninit bytes: 8)
> test_stackinit: small_hole_none ok
> test_stackinit: big_hole_none ok
> test_stackinit: trailing_hole_none ok
> test_stackinit: packed_none ok
> test_stackinit: user ok
> test_stackinit: failures: 8
Does the above testing include “pattern initialization” in addition to “zero
initialization”?
>
> The kernel's test for this is a mess[1] of macros I used to avoid losing
> my sanity from cut/pasting, but it makes the tests hard to read. To
> break it out, the failing cases are due to padding, as seen with the
> "test_small_hole", "test_big_hole", and "test_trailing_hole" structures:
>
> /* 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. */
> };
>
> They fail when they're statically initialized (either fully or
> partially), for example:
>
> struct test_..._hole instance = { .two = ..., };
>
> or
>
> struct test_..._hole instance = { .one = ...,
> .two = ...,
> .three = ...,
> .four = ...,
>
> };
So, when the structure variables are not statically initialized, all the
paddings are initialized correctly by the compiler?
In the current implementation, when the auto variable is explicitly
initialized, compiler will do nothing.
Looks like for structure variables we need some special handling to initialize
the paddings.
Need to study this a little bit and see how to fix it.
>
> The last case is for switch variables outside of case statements, like
> "var" here:
>
> switch (path) {
> unsigned long var;
>
> case ..:
> ...
> case ..:
> ...
> ...
> }
>
Will study this case more.
Thanks again.
Qing
> I'm really looking forward to having this available. Thanks again!
>
> -Kees
>
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/lib/test_stackinit.c
>
> --
> Kees Cook
