| Message ID | 20231005173812.966264-3-berrange@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | topic: meson: add more compiler hardening flags | expand |
On 05/10/2023 19.38, Daniel P. Berrangé wrote: > When variables are used without being initialized, there is potential > to take advantage of data that was pre-existing on the stack from an > earlier call, to drive an exploit. > > It is good practice to always initialize variables, and the compiler > can warn about flaws when -Wuninitialized is present. This warning, > however, is by no means foolproof with its output varying depending > on compiler version and which optimizations are enabled. > > The -ftrivial-auto-var-init option can be used to tell the compiler > to always initialize all variables. This increases the security and > predictability of the program, closing off certain attack vectors, > reducing the risk of unsafe memory disclosure. > > While the option takes several possible values, using 'zero' is > considered to be the option that is likely to lead to semantically > correct or safe behaviour[1]. eg sizes/indexes are not likely to > lead to out-of-bounds accesses when initialized to zero. Pointers > are less likely to point something useful if initialized to zero. > > Even with -ftrivial-auto-var-init=zero set, GCC will still issue > warnings with -Wuninitialized if it discovers a problem, so we are > not loosing diagnostics for developers, just hardening runtime > behaviour and making QEMU behave more predictably in case of hitting > bad codepaths. > > [1] https://lists.llvm.org/pipermail/cfe-dev/2020-April/065221.html > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> > --- > meson.build | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/meson.build b/meson.build > index 2003ca1ba4..19faea8d30 100644 > --- a/meson.build > +++ b/meson.build > @@ -442,6 +442,11 @@ hardening_flags = [ > # upon its return. This makes it harder to assemble > # ROP gadgets into something usable > '-fzero-call-used-regs=used-gpr', > + > + # Initialize all stack variables to zero. This makes > + # it harder to take advantage of uninitialized stack > + # data to drive exploits > + '-ftrivial-var-auto-init=zero', > ] I was a little bit torn about using =zero when I first read your patch, but after looking at [1], I tend now also tend to agree that =zero is likely the best choice. So from my side: Reviewed-by: Thomas Huth <thuth@redhat.com>
On 09/10/2023 09.44, Thomas Huth wrote: > On 05/10/2023 19.38, Daniel P. Berrangé wrote: >> When variables are used without being initialized, there is potential >> to take advantage of data that was pre-existing on the stack from an >> earlier call, to drive an exploit. >> >> It is good practice to always initialize variables, and the compiler >> can warn about flaws when -Wuninitialized is present. This warning, >> however, is by no means foolproof with its output varying depending >> on compiler version and which optimizations are enabled. >> >> The -ftrivial-auto-var-init option can be used to tell the compiler >> to always initialize all variables. This increases the security and >> predictability of the program, closing off certain attack vectors, >> reducing the risk of unsafe memory disclosure. >> >> While the option takes several possible values, using 'zero' is >> considered to be the option that is likely to lead to semantically >> correct or safe behaviour[1]. eg sizes/indexes are not likely to >> lead to out-of-bounds accesses when initialized to zero. Pointers >> are less likely to point something useful if initialized to zero. >> >> Even with -ftrivial-auto-var-init=zero set, GCC will still issue ... >> + '-ftrivial-var-auto-init=zero', >> ] There is something fishy here: In the commit description, you write about "-ftrivial-auto-var-init" but in the code you use "-ftrivial-var-auto-init" ... that looks wrong to me, please fix! > I was a little bit torn about using =zero when I first read your patch, but > after looking at [1], I tend now also tend to agree that =zero is likely the > best choice. Thinking about this twice: What about using -ftrivial-var-auto-init=pattern for --enable-debug builds, and only use the "zero" init for non-debug builds? ... that would prevent that people blindly rely on this "language extension". Thomas
On Mon, Oct 09, 2023 at 12:15:17PM +0200, Thomas Huth wrote: > On 09/10/2023 09.44, Thomas Huth wrote: > > On 05/10/2023 19.38, Daniel P. Berrangé wrote: > > > When variables are used without being initialized, there is potential > > > to take advantage of data that was pre-existing on the stack from an > > > earlier call, to drive an exploit. > > > > > > It is good practice to always initialize variables, and the compiler > > > can warn about flaws when -Wuninitialized is present. This warning, > > > however, is by no means foolproof with its output varying depending > > > on compiler version and which optimizations are enabled. > > > > > > The -ftrivial-auto-var-init option can be used to tell the compiler > > > to always initialize all variables. This increases the security and > > > predictability of the program, closing off certain attack vectors, > > > reducing the risk of unsafe memory disclosure. > > > > > > While the option takes several possible values, using 'zero' is > > > considered to be the option that is likely to lead to semantically > > > correct or safe behaviour[1]. eg sizes/indexes are not likely to > > > lead to out-of-bounds accesses when initialized to zero. Pointers > > > are less likely to point something useful if initialized to zero. > > > > > > Even with -ftrivial-auto-var-init=zero set, GCC will still issue > ... > > > + '-ftrivial-var-auto-init=zero', > > > ] > > There is something fishy here: In the commit description, you write about > "-ftrivial-auto-var-init" but in the code you use "-ftrivial-var-auto-init" > ... that looks wrong to me, please fix! Face palm, -ftrivial-auto-var-init is the correct one. > > I was a little bit torn about using =zero when I first read your patch, > > but after looking at [1], I tend now also tend to agree that =zero is > > likely the best choice. > > Thinking about this twice: What about using -ftrivial-var-auto-init=pattern > for --enable-debug builds, and only use the "zero" init for non-debug > builds? ... that would prevent that people blindly rely on this "language > extension". We can't blindly rely on it, because -Wuninitialized is still going to do static analysis and warn in most cases, which can't be ignored when -Werror is set. With regards, Daniel
diff --git a/meson.build b/meson.build index 2003ca1ba4..19faea8d30 100644 --- a/meson.build +++ b/meson.build @@ -442,6 +442,11 @@ hardening_flags = [ # upon its return. This makes it harder to assemble # ROP gadgets into something usable '-fzero-call-used-regs=used-gpr', + + # Initialize all stack variables to zero. This makes + # it harder to take advantage of uninitialized stack + # data to drive exploits + '-ftrivial-var-auto-init=zero', ] qemu_common_flags += cc.get_supported_arguments(hardening_flags)
When variables are used without being initialized, there is potential to take advantage of data that was pre-existing on the stack from an earlier call, to drive an exploit. It is good practice to always initialize variables, and the compiler can warn about flaws when -Wuninitialized is present. This warning, however, is by no means foolproof with its output varying depending on compiler version and which optimizations are enabled. The -ftrivial-auto-var-init option can be used to tell the compiler to always initialize all variables. This increases the security and predictability of the program, closing off certain attack vectors, reducing the risk of unsafe memory disclosure. While the option takes several possible values, using 'zero' is considered to be the option that is likely to lead to semantically correct or safe behaviour[1]. eg sizes/indexes are not likely to lead to out-of-bounds accesses when initialized to zero. Pointers are less likely to point something useful if initialized to zero. Even with -ftrivial-auto-var-init=zero set, GCC will still issue warnings with -Wuninitialized if it discovers a problem, so we are not loosing diagnostics for developers, just hardening runtime behaviour and making QEMU behave more predictably in case of hitting bad codepaths. [1] https://lists.llvm.org/pipermail/cfe-dev/2020-April/065221.html Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- meson.build | 5 +++++ 1 file changed, 5 insertions(+)