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 <keesc...@chromium.org> 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 <keesc...@chromium.org>
> > > > 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 <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
