Patchwork KVM: selftests: disable stack protector for all KVM tests

login
register
mail settings
Submitter Christopherson, Sean J
Date March 13, 2019, 7:43 p.m.
Message ID <20190313194314.5664-1-sean.j.christopherson@intel.com>
Download mbox | patch
Permalink /patch/748385/
State New
Headers show

Comments

Christopherson, Sean J - March 13, 2019, 7:43 p.m.
Since 4.8.3, gcc has enabled -fstack-protector by default.  This is
problematic for the KVM selftests as they do not configure fs or gs
segments (the stack canary is pulled from fs:0x28).  With the default
behavior, gcc will insert a stack canary on any function that creates
buffers of 8 bytes or more.  As a result, ucall() will hit a triple
fault shutdown due to reading a bad fs segment when inserting its
stack canary, i.e. every test fails with an unexpected SHUTDOWN.

Fixes: 14c47b7530e2d ("kvm: selftests: introduce ucall")
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 tools/testing/selftests/kvm/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Christopherson, Sean J - March 13, 2019, 10:56 p.m.
On Wed, Mar 13, 2019 at 12:43:14PM -0700, Sean Christopherson wrote:
> Since 4.8.3, gcc has enabled -fstack-protector by default.  This is
> problematic for the KVM selftests as they do not configure fs or gs
> segments (the stack canary is pulled from fs:0x28).  With the default
> behavior, gcc will insert a stack canary on any function that creates
> buffers of 8 bytes or more.  As a result, ucall() will hit a triple
> fault shutdown due to reading a bad fs segment when inserting its
> stack canary, i.e. every test fails with an unexpected SHUTDOWN.
> 
> Fixes: 14c47b7530e2d ("kvm: selftests: introduce ucall")
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>  tools/testing/selftests/kvm/Makefile | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
> index 3c1f4bdf9000..596c0864d0df 100644
> --- a/tools/testing/selftests/kvm/Makefile
> +++ b/tools/testing/selftests/kvm/Makefile
> @@ -29,7 +29,7 @@ LIBKVM += $(LIBKVM_$(UNAME_M))
>  INSTALL_HDR_PATH = $(top_srcdir)/usr
>  LINUX_HDR_PATH = $(INSTALL_HDR_PATH)/include/
>  LINUX_TOOL_INCLUDE = $(top_srcdir)/tools/include
> -CFLAGS += -O2 -g -std=gnu99 -I$(LINUX_TOOL_INCLUDE) -I$(LINUX_HDR_PATH) -Iinclude -I$(<D) -Iinclude/$(UNAME_M) -I..
> +CFLAGS += -O2 -g -std=gnu99 -fno-stack-protector -I$(LINUX_TOOL_INCLUDE) -I$(LINUX_HDR_PATH) -Iinclude -I$(<D) -Iinclude/$(UNAME_M) -I..
>  LDFLAGS += -pthread
>  
>  # After inclusion, $(OUTPUT) is defined and

Please ignore this patch, I figured out why the TLS wasn't in the elf
headers, which is a separate fix of its own.  With the TLS in hand a
better fix is to actually set fs.
Christopherson, Sean J - March 13, 2019, 11:29 p.m.
On Wed, Mar 13, 2019 at 03:56:25PM -0700, Sean Christopherson wrote:
> On Wed, Mar 13, 2019 at 12:43:14PM -0700, Sean Christopherson wrote:
> > Since 4.8.3, gcc has enabled -fstack-protector by default.  This is
> > problematic for the KVM selftests as they do not configure fs or gs
> > segments (the stack canary is pulled from fs:0x28).  With the default
> > behavior, gcc will insert a stack canary on any function that creates
> > buffers of 8 bytes or more.  As a result, ucall() will hit a triple
> > fault shutdown due to reading a bad fs segment when inserting its
> > stack canary, i.e. every test fails with an unexpected SHUTDOWN.
> > 
> > Fixes: 14c47b7530e2d ("kvm: selftests: introduce ucall")
> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > ---
> >  tools/testing/selftests/kvm/Makefile | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
> > index 3c1f4bdf9000..596c0864d0df 100644
> > --- a/tools/testing/selftests/kvm/Makefile
> > +++ b/tools/testing/selftests/kvm/Makefile
> > @@ -29,7 +29,7 @@ LIBKVM += $(LIBKVM_$(UNAME_M))
> >  INSTALL_HDR_PATH = $(top_srcdir)/usr
> >  LINUX_HDR_PATH = $(INSTALL_HDR_PATH)/include/
> >  LINUX_TOOL_INCLUDE = $(top_srcdir)/tools/include
> > -CFLAGS += -O2 -g -std=gnu99 -I$(LINUX_TOOL_INCLUDE) -I$(LINUX_HDR_PATH) -Iinclude -I$(<D) -Iinclude/$(UNAME_M) -I..
> > +CFLAGS += -O2 -g -std=gnu99 -fno-stack-protector -I$(LINUX_TOOL_INCLUDE) -I$(LINUX_HDR_PATH) -Iinclude -I$(<D) -Iinclude/$(UNAME_M) -I..
> >  LDFLAGS += -pthread
> >  
> >  # After inclusion, $(OUTPUT) is defined and
> 
> Please ignore this patch, I figured out why the TLS wasn't in the elf
> headers, which is a separate fix of its own.  With the TLS in hand a
> better fix is to actually set fs.

Jumped the gun a bit...  Turns out the other issue was due to recent gcc
versions enabling pie by default.  The "separate fix" I referred to was
to build the tests with '-static', whose side effect was to disable pie.
Another side side effect of '-static' is that the TLS section is listed
in the elf headers, which in turn allows the KVM selftests to define a
legitimate fs.base for the stack canary.

I have working code to point fs at the TLS, so we can easily go that
route if we want to.  But AFAICT, building with '-static' and accessing
TLS in the guest is unecessary (except for the stack canary).  Overall I
think disabling the stack protector is a little less ugly than building
a static binary and adding all of the TLS/fs logic.
Paolo Bonzini - March 15, 2019, 6:10 p.m.
On 13/03/19 20:43, Sean Christopherson wrote:
> Since 4.8.3, gcc has enabled -fstack-protector by default.  This is
> problematic for the KVM selftests as they do not configure fs or gs
> segments (the stack canary is pulled from fs:0x28).  With the default
> behavior, gcc will insert a stack canary on any function that creates
> buffers of 8 bytes or more.  As a result, ucall() will hit a triple
> fault shutdown due to reading a bad fs segment when inserting its
> stack canary, i.e. every test fails with an unexpected SHUTDOWN.
> 
> Fixes: 14c47b7530e2d ("kvm: selftests: introduce ucall")
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>  tools/testing/selftests/kvm/Makefile | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
> index 3c1f4bdf9000..596c0864d0df 100644
> --- a/tools/testing/selftests/kvm/Makefile
> +++ b/tools/testing/selftests/kvm/Makefile
> @@ -29,7 +29,7 @@ LIBKVM += $(LIBKVM_$(UNAME_M))
>  INSTALL_HDR_PATH = $(top_srcdir)/usr
>  LINUX_HDR_PATH = $(INSTALL_HDR_PATH)/include/
>  LINUX_TOOL_INCLUDE = $(top_srcdir)/tools/include
> -CFLAGS += -O2 -g -std=gnu99 -I$(LINUX_TOOL_INCLUDE) -I$(LINUX_HDR_PATH) -Iinclude -I$(<D) -Iinclude/$(UNAME_M) -I..
> +CFLAGS += -O2 -g -std=gnu99 -fno-stack-protector -I$(LINUX_TOOL_INCLUDE) -I$(LINUX_HDR_PATH) -Iinclude -I$(<D) -Iinclude/$(UNAME_M) -I..
>  LDFLAGS += -pthread
>  
>  # After inclusion, $(OUTPUT) is defined and
> 

Queued for after the merge window, thanks.

Paolo

Patch

diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
index 3c1f4bdf9000..596c0864d0df 100644
--- a/tools/testing/selftests/kvm/Makefile
+++ b/tools/testing/selftests/kvm/Makefile
@@ -29,7 +29,7 @@  LIBKVM += $(LIBKVM_$(UNAME_M))
 INSTALL_HDR_PATH = $(top_srcdir)/usr
 LINUX_HDR_PATH = $(INSTALL_HDR_PATH)/include/
 LINUX_TOOL_INCLUDE = $(top_srcdir)/tools/include
-CFLAGS += -O2 -g -std=gnu99 -I$(LINUX_TOOL_INCLUDE) -I$(LINUX_HDR_PATH) -Iinclude -I$(<D) -Iinclude/$(UNAME_M) -I..
+CFLAGS += -O2 -g -std=gnu99 -fno-stack-protector -I$(LINUX_TOOL_INCLUDE) -I$(LINUX_HDR_PATH) -Iinclude -I$(<D) -Iinclude/$(UNAME_M) -I..
 LDFLAGS += -pthread
 
 # After inclusion, $(OUTPUT) is defined and