On Thu, 10 Mar 2022 14:56:30 +0000 Alexandru Elisei <alexandru.elisei@xxxxxxx> wrote: Hi, > Hi Martin, > > On Fri, Mar 04, 2022 at 01:10:45AM +0200, Martin Radev wrote: > > Hello everyone, > > > [..] > > The Makefile change is kept in its original form because I didn't understand > > if there is an issue with it on aarch64. > > I'll try to explain it better. According to this blogpost about executable > stacks [1], gcc marks the stack as executable automatically for assembly > (.S) files. C files have their stack mark as non-executable by default. If > any of the object files have the stack executable, then the resulting > binary also has the stack marked as executable (obviously). > > To mark the stack as non-executable in assembly files, the empty section > .note.GNU-stack must be present in the file. This is a marking to tell > the linker that the final executable does not require an executable stack. > When the linker finds this section, it will create a PT_GNU_STACK empty > segment in the final executable. This segment tells Linux to mark the stack > as non-executable when it loads the binary. Ah, many thanks for the explanation, that makes sense. > The only assembly files that kvmtool compiles into objects are the x86 > files x86/bios/entry.S and x86/bios/bios-rom.S; the other architectures are > not affected by this. I haven't found any instances where these files (and > the other files they are including) do a call/jmp to something on the > stack, so I've added the .note.GNU-Stack section to the files: Yes, looks that the same to me, actually the assembly looks more like marshalling arguments than actual code, so we should be safe. Alex, can you send this as a proper patch. It should be somewhat independent of Martin's series, code-wise, so at least it should apply and build. Cheers, Andre > > diff --git a/x86/bios/bios-rom.S b/x86/bios/bios-rom.S > index 3269ce9793ae..571029fc157e 100644 > --- a/x86/bios/bios-rom.S > +++ b/x86/bios/bios-rom.S > @@ -10,3 +10,6 @@ > GLOBAL(bios_rom) > .incbin "x86/bios/bios.bin" > END(bios_rom) > + > +# Mark the stack as non-executable. > +.section .note.GNU-stack,"",@progbits > diff --git a/x86/bios/entry.S b/x86/bios/entry.S > index 85056e9816c4..4d5bb663a25d 100644 > --- a/x86/bios/entry.S > +++ b/x86/bios/entry.S > @@ -90,3 +90,6 @@ GLOBAL(__locals) > #include "local.S" > > END(__locals) > + > +# Mark the stack as non-executable. > +.section .note.GNU-stack,"",@progbits > > which makes the final executable have a non-executable stack. Did some very > *light* testing by booting a guest, and everything looked right to me. > > [1] https://www.airs.com/blog/archives/518 > > Thanks, > Alex > > > > > Martin Radev (5): > > kvmtool: Add WARN_ONCE macro > > virtio: Sanitize config accesses > > virtio: Check for overflows in QUEUE_NOTIFY and QUEUE_SEL > > Makefile: Mark stack as not executable > > mmio: Sanitize addr and len > > > > Makefile | 7 +++-- > > include/kvm/util.h | 10 +++++++ > > include/kvm/virtio-9p.h | 1 + > > include/kvm/virtio.h | 3 ++- > > mmio.c | 4 +++ > > virtio/9p.c | 27 ++++++++++++++----- > > virtio/balloon.c | 10 ++++++- > > virtio/blk.c | 10 ++++++- > > virtio/console.c | 10 ++++++- > > virtio/mmio.c | 44 +++++++++++++++++++++++++----- > > virtio/net.c | 12 +++++++-- > > virtio/pci.c | 59 ++++++++++++++++++++++++++++++++++++++--- > > virtio/rng.c | 8 +++++- > > virtio/scsi.c | 10 ++++++- > > virtio/vsock.c | 10 ++++++- > > 15 files changed, 199 insertions(+), 26 deletions(-) > > > > -- > > 2.25.1 > >
