| Message ID | 20200427034019.6251-1-jandryuk@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mini-os: Avoid segfaults in tc{g,s}etattr | expand |
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 >
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
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 >
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.
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
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
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
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
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
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;
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(+)