Re: [RFC][patch for gcc12][version 1] add -ftrivial-auto-var-init and variable attribute "uninitialized" to gcc
On Tue, Jun 01, 2021 at 04:35:53PM -0400, David Malcolm wrote: > [...] > Did this patch get reviewed/approved? It's still under review, but I think it's close. > Is the latest version still this one: > https://gcc.gnu.org/pipermail/gcc-patches/2021-February/565581.html > or is there a more recent version that should be reviewed? Yup, here's the latest (v3): https://gcc.gnu.org/pipermail/gcc-patches/2021-May/570208.html > (I don't think I'm qualified to approve the patch, I'm just a fan of > the approach. FWIW I've been experimenting with extending -fanalyzer > to detect infoleaks in the kernel, whereas AIUI this patch is about > mitigating them) Thanks for your interest! If you patch your GCC with this, it should Just Work in the kernel (i.e. you can set CONFIG_INIT_STACK_ALL_ZERO=y) > Hope this is constructive Yup! Please report back any testing; that'll help show people are interested in the feature. :) -- Kees Cook
Re: [RFC][patch for gcc12][version 1] add -ftrivial-auto-var-init and variable attribute "uninitialized" to gcc
On Mon, 2021-03-15 at 12:14 -0500, Qing Zhao via Gcc-patches wrote:
> (CC’ing gcc-patch alias).
>
> Hi, Kees,
>
>
> > On Mar 12, 2021, at 3:55 PM, Kees Cook wrote:
> >
> > On Fri, Mar 12, 2021 at 03:35:28PM -0600, Qing Zhao wrote:
> > > Hi, Kees,
> > >
> > > I am looking at the structure padding initialization issue. And
> > > also have some questions:
> > >
> > >
> > > > On Feb 24, 2021, at 10:41 PM, Kees Cook
> > > > wrote:
> > > >
> > > > It looks like there is still some issues with padding and pre-
> > > > case
> > > > switch variables. Here's the test output, FWIW:
> > > >
> > > >
> > > > 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)
> > > >
> > > > test_stackinit: switch_1_none FAIL (uninit bytes: 8)
> > > > test_stackinit: switch_2_none FAIL (uninit bytes: 8)
> > > > test_stackinit: failures: 8
> > > >
> > > >
> > > > /* 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),
> > >
> > > So, when the structure is not statically initialized, the compiler
> > > initialization is good?
> > >
> > > For the failing cases, what’s the behavior of the LLVM -ftrivial-
> > > auto-var-init?
> > >
> > > From the LLVM patch:
> > > (https://reviews.llvm.org/D54604 )
> > >
> > >
> > > To keep the patch simple, only some undef is removed for now, see
> > > replaceUndef. The padding-related infoleaks are therefore not all
> > > gone yet.
> > > This will be addressed in a follow-up, mainly because addressing
> > > padding-related
> > > leaks should be a stand-alone option which is implied by variable
> > > initialization.
> > >
> >
> > Right, padding init happened in:
> > https://github.com/llvm/llvm-project/commit/4f7bc0eee7e6099b1abd57dac3c83529944ab23c
> >
> > And was further clarified that, IIUC, padding _must be zero_
> > regardless
> > of pattern-vs-zero in:
> > https://github.com/llvm/llvm-project/commit/d39fbc7e20d84364e409ce59724ce20625637062
>
> Thanks a lot for the above information, they are very useful.
> I will take a look at the LLVM patch and try to implement this feature
> into GCC as well.
>
> >
> > > Yes, in GCC’s implementation, I think that fixing all padding-
> > > related leaks also require a
> > > separate patch.
> >
> > That's fine -- but it'll need to be tied to -ftrivial-auto-var-init,
> > since otherwise the memory isn't actually fully initialized. :)
>
> Okay, will do that.
>
> Thanks again.
>
> Qing
>
Did this patch get reviewed/approved?
Is the latest version still this one:
https://gcc.gnu.org/pipermail/gcc-patches/2021-February/565581.html
or is there a more recent version that should be reviewed?
(I don't think I'm qualified to approve the patch, I'm just a fan of
the approach. FWIW I've been experimenting with extending -fanalyzer
to detect infoleaks in the kernel, whereas AIUI this patch is about
mitigating them)
Hope this is constructive
Dave
Re: [RFC][patch for gcc12][version 1] add -ftrivial-auto-var-init and variable attribute "uninitialized" to gcc
(CC’ing gcc-patch alias).
Hi, Kees,
> On Mar 12, 2021, at 3:55 PM, Kees Cook wrote:
>
> On Fri, Mar 12, 2021 at 03:35:28PM -0600, Qing Zhao wrote:
>> Hi, Kees,
>>
>> I am looking at the structure padding initialization issue. And also have
>> some questions:
>>
>>
>>> On Feb 24, 2021, at 10:41 PM, Kees Cook wrote:
>>>
>>> It looks like there is still some issues with padding and pre-case
>>> switch variables. Here's the test output, FWIW:
>>>
>>>
>>> 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)
>>>
>>> test_stackinit: switch_1_none FAIL (uninit bytes: 8)
>>> test_stackinit: switch_2_none FAIL (uninit bytes: 8)
>>> test_stackinit: failures: 8
>>>
>>>
>>> /* 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),
>>
>> So, when the structure is not statically initialized, the compiler
>> initialization is good?
>>
>> For the failing cases, what’s the behavior of the LLVM
>> -ftrivial-auto-var-init?
>>
>> From the LLVM patch: (https://reviews.llvm.org/D54604
>> )
>>
>>
>> To keep the patch simple, only some undef is removed for now, see
>> replaceUndef. The padding-related infoleaks are therefore not all gone yet.
>> This will be addressed in a follow-up, mainly because addressing
>> padding-related
>> leaks should be a stand-alone option which is implied by variable
>> initialization.
>>
>
> Right, padding init happened in:
> https://github.com/llvm/llvm-project/commit/4f7bc0eee7e6099b1abd57dac3c83529944ab23c
>
> And was further clarified that, IIUC, padding _must be zero_ regardless
> of pattern-vs-zero in:
> https://github.com/llvm/llvm-project/commit/d39fbc7e20d84364e409ce59724ce20625637062
Thanks a lot for the above information, they are very useful.
I will take a look at the LLVM patch and try to implement this feature into GCC
as well.
>
>> Yes, in GCC’s implementation, I think that fixing all padding-related leaks
>> also require a
>> separate patch.
>
> That's fine -- but it'll need to be tied to -ftrivial-auto-var-init,
> since otherwise the memory isn't actually fully initialized. :)
Okay, will do that.
Thanks again.
Qing
>
> -Kees
>
> --
> Kees Cook
Re: [RFC][patch for gcc12][version 1] add -ftrivial-auto-var-init and variable attribute "uninitialized" to gcc
On Fri, Mar 12, 2021 at 03:35:28PM -0600, Qing Zhao wrote:
> Hi, Kees,
>
> I am looking at the structure padding initialization issue. And also have
> some questions:
>
>
> > On Feb 24, 2021, at 10:41 PM, Kees Cook wrote:
> >
> > It looks like there is still some issues with padding and pre-case
> > switch variables. Here's the test output, FWIW:
> >
> >
> > 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)
> >
> > test_stackinit: switch_1_none FAIL (uninit bytes: 8)
> > test_stackinit: switch_2_none FAIL (uninit bytes: 8)
> > test_stackinit: failures: 8
> >
> >
> > /* 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),
>
> So, when the structure is not statically initialized, the compiler
> initialization is good?
>
> For the failing cases, what’s the behavior of the LLVM
> -ftrivial-auto-var-init?
>
> From the LLVM patch: (https://reviews.llvm.org/D54604
> )
>
>
> To keep the patch simple, only some undef is removed for now, see
> replaceUndef. The padding-related infoleaks are therefore not all gone yet.
> This will be addressed in a follow-up, mainly because addressing
> padding-related
> leaks should be a stand-alone option which is implied by variable
> initialization.
>
Right, padding init happened in:
https://github.com/llvm/llvm-project/commit/4f7bc0eee7e6099b1abd57dac3c83529944ab23c
And was further clarified that, IIUC, padding _must be zero_ regardless
of pattern-vs-zero in:
https://github.com/llvm/llvm-project/commit/d39fbc7e20d84364e409ce59724ce20625637062
> Yes, in GCC’s implementation, I think that fixing all padding-related leaks
> also require a
> separate patch.
That's fine -- but it'll need to be tied to -ftrivial-auto-var-init,
since otherwise the memory isn't actually fully initialized. :)
-Kees
--
Kees Cook
Re: [RFC][patch for gcc12][version 1] add -ftrivial-auto-var-init and variable attribute "uninitialized" to gcc
Hi, Kees,
I am looking at the structure padding initialization issue. And also have some
questions:
> On Feb 24, 2021, at 10:41 PM, Kees Cook wrote:
>
> It looks like there is still some issues with padding and pre-case
> switch variables. Here's the test output, FWIW:
>
>
> 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)
>
> test_stackinit: switch_1_none FAIL (uninit bytes: 8)
> test_stackinit: switch_2_none FAIL (uninit bytes: 8)
> test_stackinit: failures: 8
>
>
> /* 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),
So, when the structure is not statically initialized, the compiler
initialization is good?
For the failing cases, what’s the behavior of the LLVM -ftrivial-auto-var-init?
From the LLVM patch: (https://reviews.llvm.org/D54604
)
To keep the patch simple, only some undef is removed for now, see
replaceUndef. The padding-related infoleaks are therefore not all gone yet.
This will be addressed in a follow-up, mainly because addressing padding-related
leaks should be a stand-alone option which is implied by variable
initialization.
Yes, in GCC’s implementation, I think that fixing all padding-related leaks
also require a
separate patch.
Qing
> for example:
>
> struct test_..._hole instance = { .two = ..., };
>
> or
>
> struct test_..._hole instance = { .one = ...,
> .two = ...,
> .three = ...,
> .four = ...,
> };
>
> The last case is for switch variables outside of case statements, like
> "var" here:
>
> switch (path) {
> unsigned long var;
>
> case ..:
> ...
> case ..:
> ...
> ...
> }
>
>
> 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
Re: [RFC][patch for gcc12][version 1] add -ftrivial-auto-var-init and variable attribute "uninitialized" to gcc
> On Mar 11, 2021, at 6:46 PM, Kees Cook wrote:
>
> On Thu, Mar 11, 2021 at 03:47:17PM -0600, Qing Zhao wrote:
>> Hi, Kees,
>>
>> Sorry for the late reply (I have been busy with other work recently).
>>
>> Currently, I am working on the issue of flexible length array as the last
>> field of the structure.
>>
>> In order to fix it correctly, I have the following question:
>>
>>
>>> On Feb 26, 2021, at 3:42 PM, Kees Cook wrote:
>>>
>>> On Thu, Feb 25, 2021 at 05:56:38PM -0600, Qing Zhao wrote:
Just noticed that you didn’t add -fauto-var-init-approach=D to the command
line.
>>>
>>> Ah-ha! I didn't realize that was needed; thanks. However, now some of the
>>> sources crash in a different way. Here's the reproducer:
>>>
>>> $ cat poc.i
>>> struct a {
>>> int b;
>>> int array[];
>>> };
>>> void c() {
>>> struct a d;
>>> }
>>>
>>
>> For such variable length array as the last field of the structure, static
>> initialization is not allowed,
>> User needs to explicitly allocate memory and initialize the allocated array
>> manually in the source code.
>>
>> So, if the compiler has to initialize this structure when requested by
>> -ftrivial-auto-var-init, I think that
>> only the fields before the last fields need to be initialized, Is this the
>> correct behavior you expected?
>
> Right, that would be my expectation as well. Putting such a struct on
> the stack tends to be nonsensical, but maybe happens if part of a union,
> which would get initialized correctly, etc:
>
> union {
> struct a {
> int b;
> int array[];
> };
> char buf[32];
> };
>
Okay, thanks. This issue has been fixed in my local repository.
Qing
> --
> Kees Cook
Re: [RFC][patch for gcc12][version 1] add -ftrivial-auto-var-init and variable attribute "uninitialized" to gcc
On Thu, Mar 11, 2021 at 03:47:17PM -0600, Qing Zhao wrote:
> Hi, Kees,
>
> Sorry for the late reply (I have been busy with other work recently).
>
> Currently, I am working on the issue of flexible length array as the last
> field of the structure.
>
> In order to fix it correctly, I have the following question:
>
>
> > On Feb 26, 2021, at 3:42 PM, Kees Cook wrote:
> >
> > On Thu, Feb 25, 2021 at 05:56:38PM -0600, Qing Zhao wrote:
> >> Just noticed that you didn’t add -fauto-var-init-approach=D to the command
> >> line.
> >
> > Ah-ha! I didn't realize that was needed; thanks. However, now some of the
> > sources crash in a different way. Here's the reproducer:
> >
> > $ cat poc.i
> > struct a {
> > int b;
> > int array[];
> > };
> > void c() {
> > struct a d;
> > }
> >
>
> For such variable length array as the last field of the structure, static
> initialization is not allowed,
> User needs to explicitly allocate memory and initialize the allocated array
> manually in the source code.
>
> So, if the compiler has to initialize this structure when requested by
> -ftrivial-auto-var-init, I think that
> only the fields before the last fields need to be initialized, Is this the
> correct behavior you expected?
Right, that would be my expectation as well. Putting such a struct on
the stack tends to be nonsensical, but maybe happens if part of a union,
which would get initialized correctly, etc:
union {
struct a {
int b;
int array[];
};
char buf[32];
};
--
Kees Cook
Re: [RFC][patch for gcc12][version 1] add -ftrivial-auto-var-init and variable attribute "uninitialized" to gcc
Hi, Kees,
Sorry for the late reply (I have been busy with other work recently).
Currently, I am working on the issue of flexible length array as the last field
of the structure.
In order to fix it correctly, I have the following question:
> On Feb 26, 2021, at 3:42 PM, Kees Cook wrote:
>
> On Thu, Feb 25, 2021 at 05:56:38PM -0600, Qing Zhao wrote:
>> Just noticed that you didn’t add -fauto-var-init-approach=D to the command
>> line.
>
> Ah-ha! I didn't realize that was needed; thanks. However, now some of the
> sources crash in a different way. Here's the reproducer:
>
> $ cat poc.i
> struct a {
> int b;
> int array[];
> };
> void c() {
> struct a d;
> }
>
For such variable length array as the last field of the structure, static
initialization is not allowed,
User needs to explicitly allocate memory and initialize the allocated array
manually in the source code.
So, if the compiler has to initialize this structure when requested by
-ftrivial-auto-var-init, I think that
only the fields before the last fields need to be initialized, Is this the
correct behavior you expected?
Thanks.
Qing
> $ gcc -ftrivial-auto-var-init=pattern -fauto-var-init-approach=D -c /dev/null
> poc.i
> during RTL pass: expand
> poc.i: In function ‘c’:
> poc.i:6:12: internal compiler error: in build_pattern_cst, at tree.c:2652
>6 | struct a d;
> |^
> 0x75b572 build_pattern_cst(tree_node*)
>../../../gcc/gcc/tree.c:2652
> 0x10db116 build_pattern_cst(tree_node*)
>../../../gcc/gcc/tree.c:2612
> 0xb8a230 expand_DEFERRED_INIT
>../../../gcc/gcc/internal-fn.c:2980
> 0x970e17 expand_call_stmt
>../../../gcc/gcc/cfgexpand.c:2749
> 0x970e17 expand_gimple_stmt_1
>../../../gcc/gcc/cfgexpand.c:3844
> 0x970e17 expand_gimple_stmt
>../../../gcc/gcc/cfgexpand.c:4008
> 0x9766b3 expand_gimple_basic_block
>../../../gcc/gcc/cfgexpand.c:6045
> 0x9780d6 execute
>../../../gcc/gcc/cfgexpand.c:6729
> Please submit a full bug report,
> with preprocessed source if appropriate.
> Please include the complete backtrace with any bug report.
> See
Re: [RFC][patch for gcc12][version 1] add -ftrivial-auto-var-init and variable attribute "uninitialized" to gcc
Thanks. I will take a look and fix this issue.
BTW, could you please also re-test -ftrivial-auto-var-init=zero
-fauto-var-init-approach=D too?
And let me know are there new issues for -ftrivial-auto-var-init=zero?
(FYI, I have tested -ftrivial-auto-var-init=zero -fauto-var-init-approach=D and
also -ftrivial-auto-var-init=pattern -fauto-var-init-approach=D on cpu2017,
without any issue).
Thanks a lot for your help.
Qing
On Feb 26, 2021, at 3:42 PM, Kees Cook wrote:
>
> On Thu, Feb 25, 2021 at 05:56:38PM -0600, Qing Zhao wrote:
>> Just noticed that you didn’t add -fauto-var-init-approach=D to the command
>> line.
>
> Ah-ha! I didn't realize that was needed; thanks. However, now some of the
> sources crash in a different way. Here's the reproducer:
>
> $ cat poc.i
> struct a {
> int b;
> int array[];
> };
> void c() {
> struct a d;
> }
>
> $ gcc -ftrivial-auto-var-init=pattern -fauto-var-init-approach=D -c /dev/null
> poc.i
> during RTL pass: expand
> poc.i: In function ‘c’:
> poc.i:6:12: internal compiler error: in build_pattern_cst, at tree.c:2652
>6 | struct a d;
> |^
> 0x75b572 build_pattern_cst(tree_node*)
>../../../gcc/gcc/tree.c:2652
> 0x10db116 build_pattern_cst(tree_node*)
>../../../gcc/gcc/tree.c:2612
> 0xb8a230 expand_DEFERRED_INIT
>../../../gcc/gcc/internal-fn.c:2980
> 0x970e17 expand_call_stmt
>../../../gcc/gcc/cfgexpand.c:2749
> 0x970e17 expand_gimple_stmt_1
>../../../gcc/gcc/cfgexpand.c:3844
> 0x970e17 expand_gimple_stmt
>../../../gcc/gcc/cfgexpand.c:4008
> 0x9766b3 expand_gimple_basic_block
>../../../gcc/gcc/cfgexpand.c:6045
> 0x9780d6 execute
>../../../gcc/gcc/cfgexpand.c:6729
> Please submit a full bug report,
> with preprocessed source if appropriate.
> Please include the complete backtrace with any bug report.
> See
Re: [RFC][patch for gcc12][version 1] add -ftrivial-auto-var-init and variable attribute "uninitialized" to gcc
On Thu, Feb 25, 2021 at 05:56:38PM -0600, Qing Zhao wrote:
> Just noticed that you didn’t add -fauto-var-init-approach=D to the command
> line.
Ah-ha! I didn't realize that was needed; thanks. However, now some of the
sources crash in a different way. Here's the reproducer:
$ cat poc.i
struct a {
int b;
int array[];
};
void c() {
struct a d;
}
$ gcc -ftrivial-auto-var-init=pattern -fauto-var-init-approach=D -c /dev/null
poc.i
during RTL pass: expand
poc.i: In function ‘c’:
poc.i:6:12: internal compiler error: in build_pattern_cst, at tree.c:2652
6 | struct a d;
|^
0x75b572 build_pattern_cst(tree_node*)
../../../gcc/gcc/tree.c:2652
0x10db116 build_pattern_cst(tree_node*)
../../../gcc/gcc/tree.c:2612
0xb8a230 expand_DEFERRED_INIT
../../../gcc/gcc/internal-fn.c:2980
0x970e17 expand_call_stmt
../../../gcc/gcc/cfgexpand.c:2749
0x970e17 expand_gimple_stmt_1
../../../gcc/gcc/cfgexpand.c:3844
0x970e17 expand_gimple_stmt
../../../gcc/gcc/cfgexpand.c:4008
0x9766b3 expand_gimple_basic_block
../../../gcc/gcc/cfgexpand.c:6045
0x9780d6 execute
../../../gcc/gcc/cfgexpand.c:6729
Please submit a full bug report,
with preprocessed source if appropriate.
Please include the complete backtrace with any bug report.
See
Re: [RFC][patch for gcc12][version 1] add -ftrivial-auto-var-init and variable attribute "uninitialized" to gcc
Hi, Kees,
Just noticed that you didn’t add -fauto-var-init-approach=D to the command line.
[qinzhao@localhost uninit]$ cat t8.c
a() { char b[1]; }
[qinzhao@localhost uninit]$ sh t
/home/qinzhao/Install/latest/bin/gcc -ftrivial-auto-var-init=pattern
-fauto-var-init-approach=D t8.c -S
t8.c:1:1: warning: return type defaults to ‘int’ [-Wimplicit-int]
1 | a() { char b[1]; }
| ^
Without -fauto-var-init-approach=D, I have the same error as yours.
(This option is just temporary, its purpose is to compare two different
implementations for “zero” initialization,
Since “pattern” initialization is added later after the comparison, “pattern”
initialization does not support the
Default “A” approach. I plan to make the default as “D” in the final version of
the patch).
So, please add “-fauto-var-init-approach=D” along with
“-ftrivial-auto-var-init=pattern/zero” for the testing.
Sorry for the confusion.
> On Feb 25, 2021, at 2:00 PM, Kees Cook wrote:
>
> On Thu, Feb 25, 2021 at 12:15:01PM -0600, Qing Zhao wrote:
>>> On Feb 24, 2021, at 10:41 PM, Kees Cook wrote:
>>> [...]
>>> 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”?
>
> This was from the zero-init case. I've just tested pattern-init now and
> it actually crashes GCC. I minimized the test case to this:
>
> $ cat main.i
> a() { char b[1]; }
> $ gcc -ftrivial-auto-var-init=pattern -c /dev/null main.i
> main.i:1:1: warning: return type defaults to ‘int’ [-Wimplicit-int]
>1 | a() { char b[1]; }
> | ^
> main.i: In function ‘a’:
> main.i:1:12: internal compiler error: in gimplify_init_ctor_eval, at
> gimplify.c:4873
>1 | a() { char b[1]; }
> |^
> 0x69740d gimplify_init_ctor_eval
>../../../gcc/gcc/gimplify.c:4873
> 0xb5ac8f gimplify_init_constructor
>../../../gcc/gcc/gimplify.c:5320
> 0xb6b68a gimplify_modify_expr
>../../../gcc/gcc/gimplify.c:5952
> 0xb533ba gimplify_expr(tree_node**, gimple**, gimple**, bool (*)(tree_node*),
> int)
>../../../gcc/gcc/gimplify.c:14262
> 0xb56b26 gimplify_stmt(tree_node**, gimple**)
>../../../gcc/gcc/gimplify.c:7056
> 0xb68e6e gimplify_and_add(tree_node*, gimple**)
>../../../gcc/gcc/gimplify.c:489
> 0xb68e6e gimple_add_init_for_auto_var
>../../../gcc/gcc/gimplify.c:1892
> 0xb68e6e gimplify_decl_expr
>../../../gcc/gcc/gimplify.c:2010
> 0xb53bd6 gimplify_expr(tree_node**, gimple**, gimple**, bool (*)(tree_node*),
> int)
>../../../gcc/gcc/gimplify.c:14459
> 0xb56b26 gimplify_stmt(tree_node**, gimple**)
>../../../gcc/gcc/gimplify.c:7056
> 0xb5727d gimplify_bind_expr
>../../../gcc/gcc/gimplify.c:1421
> 0xb536f0 gimplify_expr(tree_node**, gimple**, gimple**, bool (*)(tree_node*),
> int)
>../../../gcc/gcc/gimplify.c:14463
> 0xb6ccc9 gimplify_stmt(tree_node**, gimple**)
>../../../gcc/gcc/gimplify.c:7056
> 0xb6ccc9 gimplify_body(tree_node*, bool)
>../../../gcc/gcc/gimplify.c:15498
> 0xb6d0ed gimplify_function_tree(tree_node*)
>../../../gcc/gcc/gimplify.c:15652
> 0x9ae8d7 cgraph_node::analyze()
>../../../gcc/gcc/cgraphunit.c:670
> 0x9b13a7 analyze_functions
>../../../gcc/gcc/cgraphunit.c:1233
> 0x9b1f9d symbol_table::finalize_compilation_unit()
>../../../gcc/gcc/cgraphunit.c:2511
> Please submit a full bug report,
> with preprocessed source if appropriate.
> Please include the complete backtrace with any bug report.
> See
Re: [RFC][patch for gcc12][version 1] add -ftrivial-auto-var-init and variable attribute "uninitialized" to gcc
On Thu, Feb 25, 2021 at 12:15:01PM -0600, Qing Zhao wrote:
> > On Feb 24, 2021, at 10:41 PM, Kees Cook wrote:
> > [...]
> > 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”?
This was from the zero-init case. I've just tested pattern-init now and
it actually crashes GCC. I minimized the test case to this:
$ cat main.i
a() { char b[1]; }
$ gcc -ftrivial-auto-var-init=pattern -c /dev/null main.i
main.i:1:1: warning: return type defaults to ‘int’ [-Wimplicit-int]
1 | a() { char b[1]; }
| ^
main.i: In function ‘a’:
main.i:1:12: internal compiler error: in gimplify_init_ctor_eval, at
gimplify.c:4873
1 | a() { char b[1]; }
|^
0x69740d gimplify_init_ctor_eval
../../../gcc/gcc/gimplify.c:4873
0xb5ac8f gimplify_init_constructor
../../../gcc/gcc/gimplify.c:5320
0xb6b68a gimplify_modify_expr
../../../gcc/gcc/gimplify.c:5952
0xb533ba gimplify_expr(tree_node**, gimple**, gimple**, bool (*)(tree_node*),
int)
../../../gcc/gcc/gimplify.c:14262
0xb56b26 gimplify_stmt(tree_node**, gimple**)
../../../gcc/gcc/gimplify.c:7056
0xb68e6e gimplify_and_add(tree_node*, gimple**)
../../../gcc/gcc/gimplify.c:489
0xb68e6e gimple_add_init_for_auto_var
../../../gcc/gcc/gimplify.c:1892
0xb68e6e gimplify_decl_expr
../../../gcc/gcc/gimplify.c:2010
0xb53bd6 gimplify_expr(tree_node**, gimple**, gimple**, bool (*)(tree_node*),
int)
../../../gcc/gcc/gimplify.c:14459
0xb56b26 gimplify_stmt(tree_node**, gimple**)
../../../gcc/gcc/gimplify.c:7056
0xb5727d gimplify_bind_expr
../../../gcc/gcc/gimplify.c:1421
0xb536f0 gimplify_expr(tree_node**, gimple**, gimple**, bool (*)(tree_node*),
int)
../../../gcc/gcc/gimplify.c:14463
0xb6ccc9 gimplify_stmt(tree_node**, gimple**)
../../../gcc/gcc/gimplify.c:7056
0xb6ccc9 gimplify_body(tree_node*, bool)
../../../gcc/gcc/gimplify.c:15498
0xb6d0ed gimplify_function_tree(tree_node*)
../../../gcc/gcc/gimplify.c:15652
0x9ae8d7 cgraph_node::analyze()
../../../gcc/gcc/cgraphunit.c:670
0x9b13a7 analyze_functions
../../../gcc/gcc/cgraphunit.c:1233
0x9b1f9d symbol_table::finalize_compilation_unit()
../../../gcc/gcc/cgraphunit.c:2511
Please submit a full bug report,
with preprocessed source if appropriate.
Please include the complete backtrace with any bug report.
See
Re: [RFC][patch for gcc12][version 1] add -ftrivial-auto-var-init and variable attribute "uninitialized" to gcc
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 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
Re: [RFC][patch for gcc12][version 1] add -ftrivial-auto-var-init and variable attribute "uninitialized" to gcc
(please keep me in CC, I'm not subscribed...)
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
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 = ...,
};
The last case is for switch variables outside of case statements, like
"var" here:
switch (path) {
unsigned long var;
case ..:
...
case ..:
...
...
}
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
Re: [RFC][patch for gcc12][version 1] add -ftrivial-auto-var-init and variable attribute "uninitialized" to gcc
On Thu, Feb 18 2021, Qing Zhao via Gcc-patches wrote: > (CC’ing Kees Cook on this topic) > > Hi, > > This is the first version of the complete patch for the new security feature > for GCC: > > Initialize automatic variables with new first class option > -ftrivial-auto-var-init=[uninitialized|pattern|zero] > and a new variable attribute “uninitialized” to exclude some variables from > automatical initialization to > Control runtime overhead. > [...] > > **the complete patch: > > See the attachment. I think you forgot to attach the patch (or at least I do not see any attachment). Martin
[RFC][patch for gcc12][version 1] add -ftrivial-auto-var-init and variable attribute "uninitialized" to gcc
(CC’ing Kees Cook on this topic) Hi, This is the first version of the complete patch for the new security feature for GCC: Initialize automatic variables with new first class option -ftrivial-auto-var-init=[uninitialized|pattern|zero] and a new variable attribute “uninitialized” to exclude some variables from automatical initialization to Control runtime overhead. '-ftrivial-auto-var-init=CHOICE' Initialize automatic variables with either a pattern or with zeroes to increase program security by preventing uninitialized memory disclosure and use. The three values of CHOICE are: * 'uninitialized' doesn't initialize any automatic variables. This is C and C++'s default. * 'pattern' Initialize automatic variables with values which will likely transform logic bugs into crashes down the line, are easily recognized in a crash dump and without being values that programmers can rely on for useful program semantics. The values used for pattern initialization might be changed in the future. * 'zero' Initialize automatic variables with zeroes. The default is 'uninitialized'. You can control this behavior for a specific variable by using the variable attribute 'uninitialized' (*note Variable Attributes::). 'uninitialized' This attribute, attached to a variable with automatic storage, means that the variable should not be automatically initialized by the compiler when the option '-ftrivial-auto-var-init' presents. With the option '-ftrivial-auto-var-init', all the automatic variables that do not have explicit initializers will be initialized by the compiler. These additional compiler initializations might incur run-time overhead, sometimes dramatically. This attribute can be used to mark some variables to be excluded from such automatical initialization in order to reduce runtime overhead. This attribute has no effect when the option '-ftrivial-auto-var-init' does not present. **This implementation has the following features: 1. A new internal function .DEFERRED_INIT is introduced; 2. During gimplification phase, calls to .DEFERRED_INIT will be inserted for the initialization; 3. During expand phase, calls to .DEFERRED_INIT are expanded to real “zero” or “pattern” initialization; 4. In order to maintain the -Wuninitialized warning, uninitialized warning phase is adjusted to specially handling the calls to .DEFERRED_INIT; 5. In order to reduce the stack usage per strength reduction transformation, the strength reduction phase Is adjusted to specially handle the calls to .DEFERRED_INIT; 6. Point to analysis also is adjusted to specially handle the calls to .DEFERRED_INIT; 7. VLA auto variables are initialized specially, for “zero” initialization, add a call to MEMSET to initialize it; for “pattern” initialization, add a loop of calls to MEMCPY to initialize it; 8. For “pattern” initialization, used the similar patten as LLVM implementation for integers and pointers: + /* The following value is a guaranteed unmappable pointer value and has a + repeated byte-pattern which makes it easier to synthesize. We use it for + pointers as well as integers so that aggregates are likely to be + initialized with this repeated value. */ + uint64_t largevalue = 0xull; + /* For 32-bit platforms it's a bit trickier because, across systems, only the + zero page can reasonably be expected to be unmapped, and even then we need + a very low address. We use a smaller value, and that value sadly doesn't + have a repeated byte-pattern. We don't use it for integers. */ + uint32_t smallvalue = 0x00AA; Use quiet NAN for real type. **testing cases: 1. In c-c++-common, verification at gimple level for all types, all combination of options, new attributes; 2. In gcc.target, (for x86 and aarch64) verification at RTL or assembly level for all types, all combination of options, new attributes; 3. In gcc.dg and g++.dg, verification of the new option still keep the -Wuninitialized warnings. Copy the existing uninit-*.c to auto-init-uninit-*.c, add the new option -ftrivial-auto-var-init=zero to it. **NOTE for the review: In this version implementation, I still keep the implementation to the following two approaches for your references: A. Adding real initialization during gimplification, not maintain the uninitialized warnings. D. Adding .DEFFERED_INIT during gimplification, expand the .DEFFERED_INIT during expand to real initialization. Adjusting uninitialized pass with the new refs with “.DEFFERED_INIT” A is the approach that I will delete in the next version. D will be the one we choose. (The reason I kept both implementation is, please check the implementation for both A and D since the performance, code size, stack
