Re: [PATCH v2 kvmtool 0/5] Fix few small issues in virtio code

[Alexandru Elisei <alexandru.elisei@arm.com>]

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.

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:

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
>