* [PATCH] mini-os: Avoid segfaults in tc{g,s}etattr
@ 2020-04-27 3:40 Jason Andryuk
2020-04-27 7:54 ` Samuel Thibault
0 siblings, 1 reply; 10+ messages in thread
From: Jason Andryuk @ 2020-04-27 3:40 UTC (permalink / raw)
To: minios-devel, xen-devel; +Cc: samuel.thibault, Jason Andryuk
Commit c96c22f1d94 "mini-os: minimal implementations of some termios
functions" introduced implementations of tcgetattr and tcsetattr.
However, they do not check if files[fildes].cons.dev is non-NULL before
dereferencing. This is not a problem for FDs allocated through
alloc_fd, but the files array pre-allocates FDs 0-2 for stdio. Those
entries have a NULL .dev, so tc{g,s}etattr on them segfault.
ioemu-stubdom segfaults when term_init() calls tcgetattr on FD 0.
Restore tcgetattr and tcsetattr behavior when .dev is NULL equivalent to
unsupported_function as it was before c96c22f1d94.
Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
---
I can't get ioemu-stubdom to start without this. With this, the guest
just reboots immediately, but it does that with a non-stubdom
device_model_version="qemu-xen-traditional" . The same guest disk image
(cirros 0.5.1) boots with a linux stubdom or non-stubdom Ubuntu
qemu-system-x86_64.
lib/sys.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/lib/sys.c b/lib/sys.c
index da434fc..c6a7b9f 100644
--- a/lib/sys.c
+++ b/lib/sys.c
@@ -1472,6 +1472,11 @@ int tcsetattr(int fildes, int action, const struct termios *tios)
return -1;
}
+ if (files[fildes].cons.dev == NULL) {
+ errno = ENOSYS;
+ return -1;
+ }
+
if (tios->c_oflag & OPOST)
files[fildes].cons.dev->is_raw = false;
else
@@ -1492,6 +1497,11 @@ int tcgetattr(int fildes, struct termios *tios)
return -1;
}
+ if (files[fildes].cons.dev == NULL) {
+ errno = ENOSYS;
+ return 0;
+ }
+
if (tios == NULL) {
errno = EINVAL;
return -1;
--
2.20.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] mini-os: Avoid segfaults in tc{g,s}etattr
2020-04-27 3:40 [PATCH] mini-os: Avoid segfaults in tc{g,s}etattr Jason Andryuk
@ 2020-04-27 7:54 ` Samuel Thibault
2020-04-27 13:30 ` Jason Andryuk
2020-04-28 11:18 ` [PATCH] mini-os: Avoid segfaults in tc{g,s}etattr Wei Liu
0 siblings, 2 replies; 10+ messages in thread
From: Samuel Thibault @ 2020-04-27 7:54 UTC (permalink / raw)
To: Jason Andryuk; +Cc: minios-devel, xen-devel
Jason Andryuk, le dim. 26 avril 2020 23:40:19 -0400, a ecrit:
> Commit c96c22f1d94 "mini-os: minimal implementations of some termios
> functions" introduced implementations of tcgetattr and tcsetattr.
> However, they do not check if files[fildes].cons.dev is non-NULL before
> dereferencing. This is not a problem for FDs allocated through
> alloc_fd, but the files array pre-allocates FDs 0-2 for stdio. Those
> entries have a NULL .dev, so tc{g,s}etattr on them segfault.
>
> ioemu-stubdom segfaults when term_init() calls tcgetattr on FD 0.
>
> Restore tcgetattr and tcsetattr behavior when .dev is NULL equivalent to
> unsupported_function as it was before c96c22f1d94.
>
> Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
Thanks!
> ---
> I can't get ioemu-stubdom to start without this. With this, the guest
> just reboots immediately, but it does that with a non-stubdom
> device_model_version="qemu-xen-traditional" . The same guest disk image
> (cirros 0.5.1) boots with a linux stubdom or non-stubdom Ubuntu
> qemu-system-x86_64.
>
> lib/sys.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/lib/sys.c b/lib/sys.c
> index da434fc..c6a7b9f 100644
> --- a/lib/sys.c
> +++ b/lib/sys.c
> @@ -1472,6 +1472,11 @@ int tcsetattr(int fildes, int action, const struct termios *tios)
> return -1;
> }
>
> + if (files[fildes].cons.dev == NULL) {
> + errno = ENOSYS;
> + return -1;
> + }
> +
> if (tios->c_oflag & OPOST)
> files[fildes].cons.dev->is_raw = false;
> else
> @@ -1492,6 +1497,11 @@ int tcgetattr(int fildes, struct termios *tios)
> return -1;
> }
>
> + if (files[fildes].cons.dev == NULL) {
> + errno = ENOSYS;
> + return 0;
> + }
> +
> if (tios == NULL) {
> errno = EINVAL;
> return -1;
> --
> 2.20.1
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] mini-os: Avoid segfaults in tc{g,s}etattr
2020-04-27 7:54 ` Samuel Thibault
@ 2020-04-27 13:30 ` Jason Andryuk
2020-04-28 11:16 ` Wei Liu
2020-04-28 11:18 ` [PATCH] mini-os: Avoid segfaults in tc{g,s}etattr Wei Liu
1 sibling, 1 reply; 10+ messages in thread
From: Jason Andryuk @ 2020-04-27 13:30 UTC (permalink / raw)
To: Samuel Thibault, Jason Andryuk, minios-devel, xen-devel
On Mon, Apr 27, 2020 at 3:54 AM Samuel Thibault
<samuel.thibault@ens-lyon.org> wrote:
>
> Jason Andryuk, le dim. 26 avril 2020 23:40:19 -0400, a ecrit:
> > Commit c96c22f1d94 "mini-os: minimal implementations of some termios
> > functions" introduced implementations of tcgetattr and tcsetattr.
> > However, they do not check if files[fildes].cons.dev is non-NULL before
> > dereferencing. This is not a problem for FDs allocated through
> > alloc_fd, but the files array pre-allocates FDs 0-2 for stdio. Those
> > entries have a NULL .dev, so tc{g,s}etattr on them segfault.
> >
> > ioemu-stubdom segfaults when term_init() calls tcgetattr on FD 0.
> >
> > Restore tcgetattr and tcsetattr behavior when .dev is NULL equivalent to
> > unsupported_function as it was before c96c22f1d94.
> >
> > Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
>
> Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
>
> Thanks!
Thank you!
> > ---
> > I can't get ioemu-stubdom to start without this. With this, the guest
> > just reboots immediately, but it does that with a non-stubdom
> > device_model_version="qemu-xen-traditional" . The same guest disk image
> > (cirros 0.5.1) boots with a linux stubdom or non-stubdom Ubuntu
> > qemu-system-x86_64.
Ubuntu gcc-9 adds -fcf-protection by default. Somehow that flag
caused rombios (I think) to restart. Setting -fcf-protection=none
like below (probably just the EMBEDDED_EXTRA_CFLAGS part) lets rombios
start properly. The hypervisor needs it as well via
EXTRA_CFLAGS_XEN_CORE=-fcf-protection=none and maybe also added to
xen/arch/x86/boot/build32.mk .
diff --git a/Config.mk b/Config.mk
index 0f303c79b2..efb3d42bc4 100644
--- a/Config.mk
+++ b/Config.mk
@@ -205,6 +205,7 @@ APPEND_CFLAGS += $(foreach i, $(APPEND_INCLUDES), -I$(i))
EMBEDDED_EXTRA_CFLAGS := -nopie -fno-stack-protector -fno-stack-protector-all
EMBEDDED_EXTRA_CFLAGS += -fno-exceptions
+EMBEDDED_EXTRA_CFLAGS += -fcf-protection=none
XEN_EXTFILES_URL ?= http://xenbits.xen.org/xen-extfiles
# All the files at that location were downloaded from elsewhere on
diff --git a/tools/firmware/Rules.mk b/tools/firmware/Rules.mk
index 26bbddccd4..0d33514d53 100644
--- a/tools/firmware/Rules.mk
+++ b/tools/firmware/Rules.mk
@@ -17,3 +17,4 @@ $(call cc-options-add,CFLAGS,CC,$(EMBEDDED_EXTRA_CFLAGS))
# Extra CFLAGS suitable for an embedded type of environment.
CFLAGS += -fno-builtin -msoft-float
+CFLAGS += -fcf-protection=none
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] mini-os: Avoid segfaults in tc{g,s}etattr
2020-04-27 13:30 ` Jason Andryuk
@ 2020-04-28 11:16 ` Wei Liu
2020-04-28 11:24 ` Andrew Cooper
2020-04-28 11:44 ` Jason Andryuk
0 siblings, 2 replies; 10+ messages in thread
From: Wei Liu @ 2020-04-28 11:16 UTC (permalink / raw)
To: Jason Andryuk
Cc: Wei Liu, Andrew Cooper, minios-devel, Jan Beulich, xen-devel,
Samuel Thibault
On Mon, Apr 27, 2020 at 09:30:50AM -0400, Jason Andryuk wrote:
> On Mon, Apr 27, 2020 at 3:54 AM Samuel Thibault
> <samuel.thibault@ens-lyon.org> wrote:
> >
> > Jason Andryuk, le dim. 26 avril 2020 23:40:19 -0400, a ecrit:
> > > Commit c96c22f1d94 "mini-os: minimal implementations of some termios
> > > functions" introduced implementations of tcgetattr and tcsetattr.
> > > However, they do not check if files[fildes].cons.dev is non-NULL before
> > > dereferencing. This is not a problem for FDs allocated through
> > > alloc_fd, but the files array pre-allocates FDs 0-2 for stdio. Those
> > > entries have a NULL .dev, so tc{g,s}etattr on them segfault.
> > >
> > > ioemu-stubdom segfaults when term_init() calls tcgetattr on FD 0.
> > >
> > > Restore tcgetattr and tcsetattr behavior when .dev is NULL equivalent to
> > > unsupported_function as it was before c96c22f1d94.
> > >
> > > Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
> >
> > Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
> >
> > Thanks!
>
> Thank you!
>
> > > ---
> > > I can't get ioemu-stubdom to start without this. With this, the guest
> > > just reboots immediately, but it does that with a non-stubdom
> > > device_model_version="qemu-xen-traditional" . The same guest disk image
> > > (cirros 0.5.1) boots with a linux stubdom or non-stubdom Ubuntu
> > > qemu-system-x86_64.
>
> Ubuntu gcc-9 adds -fcf-protection by default. Somehow that flag
> caused rombios (I think) to restart. Setting -fcf-protection=none
> like below (probably just the EMBEDDED_EXTRA_CFLAGS part) lets rombios
> start properly. The hypervisor needs it as well via
> EXTRA_CFLAGS_XEN_CORE=-fcf-protection=none and maybe also added to
> xen/arch/x86/boot/build32.mk .
Are you able to turn this into a proper patch? I suspect you will need
to test the availability of this new (?) flag.
Also Cc Jan and Andrew because it affects hypervisor build too.
>
> diff --git a/Config.mk b/Config.mk
> index 0f303c79b2..efb3d42bc4 100644
> --- a/Config.mk
> +++ b/Config.mk
> @@ -205,6 +205,7 @@ APPEND_CFLAGS += $(foreach i, $(APPEND_INCLUDES), -I$(i))
>
> EMBEDDED_EXTRA_CFLAGS := -nopie -fno-stack-protector -fno-stack-protector-all
> EMBEDDED_EXTRA_CFLAGS += -fno-exceptions
> +EMBEDDED_EXTRA_CFLAGS += -fcf-protection=none
>
> XEN_EXTFILES_URL ?= http://xenbits.xen.org/xen-extfiles
> # All the files at that location were downloaded from elsewhere on
> diff --git a/tools/firmware/Rules.mk b/tools/firmware/Rules.mk
> index 26bbddccd4..0d33514d53 100644
> --- a/tools/firmware/Rules.mk
> +++ b/tools/firmware/Rules.mk
> @@ -17,3 +17,4 @@ $(call cc-options-add,CFLAGS,CC,$(EMBEDDED_EXTRA_CFLAGS))
>
> # Extra CFLAGS suitable for an embedded type of environment.
> CFLAGS += -fno-builtin -msoft-float
> +CFLAGS += -fcf-protection=none
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] mini-os: Avoid segfaults in tc{g,s}etattr
2020-04-27 7:54 ` Samuel Thibault
2020-04-27 13:30 ` Jason Andryuk
@ 2020-04-28 11:18 ` Wei Liu
1 sibling, 0 replies; 10+ messages in thread
From: Wei Liu @ 2020-04-28 11:18 UTC (permalink / raw)
To: Samuel Thibault, Jason Andryuk, minios-devel, xen-devel; +Cc: Wei Liu
On Mon, Apr 27, 2020 at 09:54:29AM +0200, Samuel Thibault wrote:
> Jason Andryuk, le dim. 26 avril 2020 23:40:19 -0400, a ecrit:
> > Commit c96c22f1d94 "mini-os: minimal implementations of some termios
> > functions" introduced implementations of tcgetattr and tcsetattr.
> > However, they do not check if files[fildes].cons.dev is non-NULL before
> > dereferencing. This is not a problem for FDs allocated through
> > alloc_fd, but the files array pre-allocates FDs 0-2 for stdio. Those
> > entries have a NULL .dev, so tc{g,s}etattr on them segfault.
> >
> > ioemu-stubdom segfaults when term_init() calls tcgetattr on FD 0.
> >
> > Restore tcgetattr and tcsetattr behavior when .dev is NULL equivalent to
> > unsupported_function as it was before c96c22f1d94.
> >
> > Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
>
> Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
>
> Thanks!
Applied. Thanks.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] mini-os: Avoid segfaults in tc{g,s}etattr
2020-04-28 11:16 ` Wei Liu
@ 2020-04-28 11:24 ` Andrew Cooper
2020-04-28 11:44 ` Jason Andryuk
1 sibling, 0 replies; 10+ messages in thread
From: Andrew Cooper @ 2020-04-28 11:24 UTC (permalink / raw)
To: Wei Liu, Jason Andryuk
Cc: minios-devel, Samuel Thibault, Jan Beulich, xen-devel
On 28/04/2020 12:16, Wei Liu wrote:
>>>> ---
>>>> I can't get ioemu-stubdom to start without this. With this, the guest
>>>> just reboots immediately, but it does that with a non-stubdom
>>>> device_model_version="qemu-xen-traditional" . The same guest disk image
>>>> (cirros 0.5.1) boots with a linux stubdom or non-stubdom Ubuntu
>>>> qemu-system-x86_64.
>> Ubuntu gcc-9 adds -fcf-protection by default. Somehow that flag
>> caused rombios (I think) to restart. Setting -fcf-protection=none
>> like below (probably just the EMBEDDED_EXTRA_CFLAGS part) lets rombios
>> start properly.
All it does is insert ENDBR{32,64} instructions, which are nops on older
processors.
I suspect that it is not the -fcf-protection= directly, but some change
in alignment of a critical function.
>> The hypervisor needs it as well via
>> EXTRA_CFLAGS_XEN_CORE=-fcf-protection=none and maybe also added to
>> xen/arch/x86/boot/build32.mk .
> Are you able to turn this into a proper patch? I suspect you will need
> to test the availability of this new (?) flag.
>
> Also Cc Jan and Andrew because it affects hypervisor build too.
I need to chase this up. It is a GCC bug breaking the hypervisor build,
and I'm moderately disinclined to hack around it, seeing as
-fcf-protection is something we want in due course.
The bug is that GCC falsely declares that -fcf-protection is
incompatible with -mindirect-thunk=extern, despite me spending a week
during the Spectre embargo period specifically arranging for the two to
be compatible, because we knew we'd want to build retpoline-safe
binaries which could also use CET on newer hardware.
~Andrew
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] mini-os: Avoid segfaults in tc{g,s}etattr
2020-04-28 11:16 ` Wei Liu
2020-04-28 11:24 ` Andrew Cooper
@ 2020-04-28 11:44 ` Jason Andryuk
2020-04-28 11:55 ` Andrew Cooper
1 sibling, 1 reply; 10+ messages in thread
From: Jason Andryuk @ 2020-04-28 11:44 UTC (permalink / raw)
To: andrew.cooper3, Wei Liu, Jason Andryuk
Cc: minios-devel, samuel.thibault, Stefan Bader, JBeulich, xen-devel
From: Andrew Cooper <andrew.cooper3@citrix.com>
Andrew Cooper wrote:
>On 28/04/2020 12:16, Wei Liu wrote:
>>>>> ---
>>>>> I can't get ioemu-stubdom to start without this. With this, the guest
>>>>> just reboots immediately, but it does that with a non-stubdom
>>>>> device_model_version="qemu-xen-traditional" . The same guest disk image
>>>>> (cirros 0.5.1) boots with a linux stubdom or non-stubdom Ubuntu
>>>>> qemu-system-x86_64.
>>> Ubuntu gcc-9 adds -fcf-protection by default. Somehow that flag
>>> caused rombios (I think) to restart. Setting -fcf-protection=none
>>> like below (probably just the EMBEDDED_EXTRA_CFLAGS part) lets rombios
>>> start properly.
>
>All it does is insert ENDBR{32,64} instructions, which are nops on older
>processors.
>
>I suspect that it is not the -fcf-protection= directly, but some change
>in alignment of a critical function.
>
>>> The hypervisor needs it as well via
>>> EXTRA_CFLAGS_XEN_CORE=-fcf-protection=none and maybe also added to
>>> xen/arch/x86/boot/build32.mk .
>> Are you able to turn this into a proper patch? I suspect you will need
>> to test the availability of this new (?) flag.
>>
>> Also Cc Jan and Andrew because it affects hypervisor build too.
>
>I need to chase this up. It is a GCC bug breaking the hypervisor build,
>and I'm moderately disinclined to hack around it, seeing as
>-fcf-protection is something we want in due course.
>
>The bug is that GCC falsely declares that -fcf-protection is
>incompatible with -mindirect-thunk=extern, despite me spending a week
>during the Spectre embargo period specifically arranging for the two to
>be compatible, because we knew we'd want to build retpoline-safe
>binaries which could also use CET on newer hardware.
The gcc manual states:
"Note that -mindirect-branch=thunk-extern is incompatible with
-fcf-protection=branch since the external thunk cannot be modified
to disable control-flow check."
https://gcc.gnu.org/onlinedocs/gcc/x86-Options.html
Below is what I was preparing to submit as a patch. So, yes it hacks around
it, but it isn't messy.
---
Disable fcf-protection to build working binaries
Ubuntu gcc-9 enables -fcf-protection by default, which conflicts with
-mindirect-branch=extern and prevents building the hypervisor with
CONFIG_INDIRECT_THUNK:
xmalloc.h:81:1: error: ‘-mindirect-branch’ and ‘-fcf-protection’ are not
compatible
Stefan Bader also noticed that build32.mk requires -fcf-protection=none
or else the hypervisor will not boot.
https://bugs.launchpad.net/ubuntu/+source/gcc-9/+bug/1863260 Similarly,
rombios reboots almost immediately without -fcf-protection=none. Both
of those can be handled by setting it in EMBEDDED_EXTRA_CFLAGS.
CC: Stefan Bader <stefan.bader@canonical.com>
Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
---
Config.mk | 1 +
xen/arch/x86/Rules.mk | 1 +
2 files changed, 2 insertions(+)
diff --git a/Config.mk b/Config.mk
index 0f303c79b2..efb3d42bc4 100644
--- a/Config.mk
+++ b/Config.mk
@@ -205,6 +205,7 @@ APPEND_CFLAGS += $(foreach i, $(APPEND_INCLUDES), -I$(i))
EMBEDDED_EXTRA_CFLAGS := -nopie -fno-stack-protector -fno-stack-protector-all
EMBEDDED_EXTRA_CFLAGS += -fno-exceptions
+EMBEDDED_EXTRA_CFLAGS += -fcf-protection=none
XEN_EXTFILES_URL ?= http://xenbits.xen.org/xen-extfiles
# All the files at that location were downloaded from elsewhere on
diff --git a/xen/arch/x86/Rules.mk b/xen/arch/x86/Rules.mk
index 4b7ab78467..c3cbae69d2 100644
--- a/xen/arch/x86/Rules.mk
+++ b/xen/arch/x86/Rules.mk
@@ -69,6 +69,7 @@ CFLAGS += -mno-sse $(call cc-option,$(CC),-mskip-rax-setup)
ifeq ($(CONFIG_INDIRECT_THUNK),y)
CFLAGS += -mindirect-branch=thunk-extern -mindirect-branch-register
CFLAGS += -fno-jump-tables
+$(call cc-option-add,CFLAGS,CC,-fcf-protection=none)
endif
# If supported by the compiler, reduce stack alignment to 8 bytes. But allow
--
2.20.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] mini-os: Avoid segfaults in tc{g,s}etattr
2020-04-28 11:44 ` Jason Andryuk
@ 2020-04-28 11:55 ` Andrew Cooper
2020-04-29 12:34 ` Andrew Cooper
0 siblings, 1 reply; 10+ messages in thread
From: Andrew Cooper @ 2020-04-28 11:55 UTC (permalink / raw)
To: Jason Andryuk, Wei Liu
Cc: minios-devel, samuel.thibault, Stefan Bader, JBeulich, xen-devel
On 28/04/2020 12:44, Jason Andryuk wrote:
> From: Andrew Cooper <andrew.cooper3@citrix.com>
>
> Andrew Cooper wrote:
>> On 28/04/2020 12:16, Wei Liu wrote:
>>>>>> ---
>>>>>> I can't get ioemu-stubdom to start without this. With this, the guest
>>>>>> just reboots immediately, but it does that with a non-stubdom
>>>>>> device_model_version="qemu-xen-traditional" . The same guest disk image
>>>>>> (cirros 0.5.1) boots with a linux stubdom or non-stubdom Ubuntu
>>>>>> qemu-system-x86_64.
>>>> Ubuntu gcc-9 adds -fcf-protection by default. Somehow that flag
>>>> caused rombios (I think) to restart. Setting -fcf-protection=none
>>>> like below (probably just the EMBEDDED_EXTRA_CFLAGS part) lets rombios
>>>> start properly.
>> All it does is insert ENDBR{32,64} instructions, which are nops on older
>> processors.
>>
>> I suspect that it is not the -fcf-protection= directly, but some change
>> in alignment of a critical function.
>>
>>>> The hypervisor needs it as well via
>>>> EXTRA_CFLAGS_XEN_CORE=-fcf-protection=none and maybe also added to
>>>> xen/arch/x86/boot/build32.mk .
>>> Are you able to turn this into a proper patch? I suspect you will need
>>> to test the availability of this new (?) flag.
>>>
>>> Also Cc Jan and Andrew because it affects hypervisor build too.
>> I need to chase this up. It is a GCC bug breaking the hypervisor build,
>> and I'm moderately disinclined to hack around it, seeing as
>> -fcf-protection is something we want in due course.
>>
>> The bug is that GCC falsely declares that -fcf-protection is
>> incompatible with -mindirect-thunk=extern, despite me spending a week
>> during the Spectre embargo period specifically arranging for the two to
>> be compatible, because we knew we'd want to build retpoline-safe
>> binaries which could also use CET on newer hardware.
> The gcc manual states:
> "Note that -mindirect-branch=thunk-extern is incompatible with
> -fcf-protection=branch since the external thunk cannot be modified
> to disable control-flow check."
>
> https://gcc.gnu.org/onlinedocs/gcc/x86-Options.html
Yes. This is false.
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93654
but sadly tumbleweeds.
I'll start a thread on the email list.
>
> Below is what I was preparing to submit as a patch. So, yes it hacks around
> it, but it isn't messy.
>
> ---
> Disable fcf-protection to build working binaries
>
> Ubuntu gcc-9 enables -fcf-protection by default, which conflicts with
> -mindirect-branch=extern and prevents building the hypervisor with
> CONFIG_INDIRECT_THUNK:
> xmalloc.h:81:1: error: ‘-mindirect-branch’ and ‘-fcf-protection’ are not
> compatible
>
> Stefan Bader also noticed that build32.mk requires -fcf-protection=none
> or else the hypervisor will not boot.
> https://bugs.launchpad.net/ubuntu/+source/gcc-9/+bug/1863260 Similarly,
> rombios reboots almost immediately without -fcf-protection=none. Both
> of those can be handled by setting it in EMBEDDED_EXTRA_CFLAGS.
>
> CC: Stefan Bader <stefan.bader@canonical.com>
> Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
Sadly, this isn't really appropriate. We specifically do want to use
both -fcf-protection and -mindirect-branch=thunk-extern together, when
GCC isn't broken.
Overriding -fcf-protection is ok but only when we're certain we've got a
buggy GCC, so that when this bug is fixed, we can return to sensible
behaviour.
~Andrew
> ---
> Config.mk | 1 +
> xen/arch/x86/Rules.mk | 1 +
> 2 files changed, 2 insertions(+)
>
> diff --git a/Config.mk b/Config.mk
> index 0f303c79b2..efb3d42bc4 100644
> --- a/Config.mk
> +++ b/Config.mk
> @@ -205,6 +205,7 @@ APPEND_CFLAGS += $(foreach i, $(APPEND_INCLUDES), -I$(i))
>
> EMBEDDED_EXTRA_CFLAGS := -nopie -fno-stack-protector -fno-stack-protector-all
> EMBEDDED_EXTRA_CFLAGS += -fno-exceptions
> +EMBEDDED_EXTRA_CFLAGS += -fcf-protection=none
>
> XEN_EXTFILES_URL ?= http://xenbits.xen.org/xen-extfiles
> # All the files at that location were downloaded from elsewhere on
> diff --git a/xen/arch/x86/Rules.mk b/xen/arch/x86/Rules.mk
> index 4b7ab78467..c3cbae69d2 100644
> --- a/xen/arch/x86/Rules.mk
> +++ b/xen/arch/x86/Rules.mk
> @@ -69,6 +69,7 @@ CFLAGS += -mno-sse $(call cc-option,$(CC),-mskip-rax-setup)
> ifeq ($(CONFIG_INDIRECT_THUNK),y)
> CFLAGS += -mindirect-branch=thunk-extern -mindirect-branch-register
> CFLAGS += -fno-jump-tables
> +$(call cc-option-add,CFLAGS,CC,-fcf-protection=none)
> endif
>
> # If supported by the compiler, reduce stack alignment to 8 bytes. But allow
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] mini-os: Avoid segfaults in tc{g,s}etattr
2020-04-28 11:55 ` Andrew Cooper
@ 2020-04-29 12:34 ` Andrew Cooper
2020-05-12 19:31 ` rombios triple fault with -fcf-protection Jason Andryuk
0 siblings, 1 reply; 10+ messages in thread
From: Andrew Cooper @ 2020-04-29 12:34 UTC (permalink / raw)
To: Jason Andryuk, Wei Liu
Cc: minios-devel, samuel.thibault, JBeulich, Stefan Bader, xen-devel
On 28/04/2020 12:55, Andrew Cooper wrote:
>> Below is what I was preparing to submit as a patch. So, yes it hacks around
>> it, but it isn't messy.
>>
>> ---
>> Disable fcf-protection to build working binaries
>>
>> Ubuntu gcc-9 enables -fcf-protection by default, which conflicts with
>> -mindirect-branch=extern and prevents building the hypervisor with
>> CONFIG_INDIRECT_THUNK:
>> xmalloc.h:81:1: error: ‘-mindirect-branch’ and ‘-fcf-protection’ are not
>> compatible
>>
>> Stefan Bader also noticed that build32.mk requires -fcf-protection=none
>> or else the hypervisor will not boot.
>> https://bugs.launchpad.net/ubuntu/+source/gcc-9/+bug/1863260 Similarly,
>> rombios reboots almost immediately without -fcf-protection=none. Both
>> of those can be handled by setting it in EMBEDDED_EXTRA_CFLAGS.
>>
>> CC: Stefan Bader <stefan.bader@canonical.com>
>> Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
> Sadly, this isn't really appropriate. We specifically do want to use
> both -fcf-protection and -mindirect-branch=thunk-extern together, when
> GCC isn't broken.
>
> Overriding -fcf-protection is ok but only when we're certain we've got a
> buggy GCC, so that when this bug is fixed, we can return to sensible
> behaviour.
GCC has been adjusted on master
(https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=9be3bb2c0a258fd6a7d3d05d232a21930c757d3c)
and the gcc-9 branch
(https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=a03efb266fcbf4a01285fff871a5bfe5caac4944).
This should be fixed for GCC 10 and 9.4
I checked the resulting hypervisor build with both -fcf-protection and
retpolines, and it works fine.
The question now is what to do all the buggy GCCs out there. We can
either ignore the problem and it will eventually go away, or spot the
problematic compiler and clobber -fcf-protection.
We also need to see what is wrong with RomBIOS, because that is weird.
However, we should not be interfering with the HOSTCC settings.
~Andrew
^ permalink raw reply [flat|nested] 10+ messages in thread
* rombios triple fault with -fcf-protection
2020-04-29 12:34 ` Andrew Cooper
@ 2020-05-12 19:31 ` Jason Andryuk
0 siblings, 0 replies; 10+ messages in thread
From: Jason Andryuk @ 2020-05-12 19:31 UTC (permalink / raw)
To: Andrew Cooper, xen-devel
This is the output from a failed rombios boot:
(d11) Invoking ROMBIOS ...
(XEN) stdvga.c:173:d11v0 entering stdvga mode
(d11) VGABios $Id: vgabios.c,v 1.67 2008/01/27 09:44:12 vruppert Exp $
(d11) Bochs BIOS - build: 06/23/99
(d11) $Revision: 1.221 $ $Date: 2008/12/07 17:32:29 $
(d11) Options: apmbios pcibios eltorito PMM
(d11)
(d11) ata0 master: QEMU HARDDISK ATA-7 Hard-Disk ( 112 MBytes)
(d11)
(XEN) MMIO emulation failed (1): d11v0 32bit @ 0008:ffffffff ->
(XEN) d11v0 Triple fault - invoking HVM shutdown action 1
(XEN) *** Dumping Dom11 vcpu#0 state: ***
(XEN) ----[ Xen-4.14-unstable x86_64 debug=y Not tainted ]----
(XEN) CPU: 1
(XEN) RIP: 0008:[<00000000ffffffff>]
(XEN) RFLAGS: 0000000000010086 CONTEXT: hvm guest (d11v0)
(XEN) rax: 0000000043001000 rbx: 000000000009c30e rcx: 0000000000000000
(XEN) rdx: 0000000000000000 rsi: 000000000000031e rdi: 0000000000000020
(XEN) rbp: 00000000000002de rsp: 000000000009ef48 r8: 0000000000000000
(XEN) r9: 0000000000000000 r10: 0000000000000000 r11: 0000000000000000
(XEN) r12: 0000000000000000 r13: 0000000000000000 r14: 0000000000000000
(XEN) r15: 0000000000000000 cr0: 0000000000000011 cr4: 0000000000000000
(XEN) cr3: 0000000000400000 cr2: 0000000000000000
(XEN) fsb: 0000000000000000 gsb: 00000000000c9000 gss: 0000000000000002
(XEN) ds: 0018 es: 0018 fs: 0000 gs: c900 ss: 0018 cs: 0008
For a traditional stubdom with sdl enabled, I also see the init_message of iPXE
iPXE (http://ipxe.org) 00:04.0 C900 PCI2.10 PMM_
The underscore might be the cursor?
The VM reboots. If I set on_reboot="preserve" and use gdbsx:
(gdb) target remote :1234
Remote debugging using :1234
0xdeadf00d in ?? ()
(gdb) bt
#0 0xdeadf00d in ?? ()
(gdb) i r
eax 0x0 0
ecx 0x0 0
edx 0xe5a08016 -442466282
ebx 0x87 135
esp 0x23aa 0x23aa <memset+22>
ebp 0x0 0x0
esi 0x0 0
edi 0x0 0
eip 0xdeadf00d 0xdeadf00d
eflags 0xdeadbeef [ CF PF ZF SF IF DF OF RF AC VIF ID ]
cs 0xdeadf00d -559026163
ss 0xdeadbeef -559038737
ds 0x5ef890 6224016
es 0x0 0
fs 0x5ef868 6223976
gs 0x0 0
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-05-12 19:32 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-27 3:40 [PATCH] mini-os: Avoid segfaults in tc{g,s}etattr Jason Andryuk
2020-04-27 7:54 ` Samuel Thibault
2020-04-27 13:30 ` Jason Andryuk
2020-04-28 11:16 ` Wei Liu
2020-04-28 11:24 ` Andrew Cooper
2020-04-28 11:44 ` Jason Andryuk
2020-04-28 11:55 ` Andrew Cooper
2020-04-29 12:34 ` Andrew Cooper
2020-05-12 19:31 ` rombios triple fault with -fcf-protection Jason Andryuk
2020-04-28 11:18 ` [PATCH] mini-os: Avoid segfaults in tc{g,s}etattr Wei Liu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).