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

[Martin Radev <martin.b.radev@gmail.com>]

Thanks for the explanation and suggestion.
Is this better?

From 4ed0d9d3d3c39eb5b23b04227c3fee53b77d9aa5 Mon Sep 17 00:00:00 2001
From: Martin Radev <martin.b.radev@gmail.com>
Date: Fri, 25 Mar 2022 23:25:42 +0200
Subject: kvmtool: Have stack be not executable on x86

This patch fixes an issue of having the stack be executable
for x86 builds by ensuring that the two objects bios-rom.o
and entry.o have the section .note.GNU-stack.

Suggested-by: Alexandru Elisei <alexandru.elisei@arm.com>
Signed-off-by: Martin Radev <martin.b.radev@gmail.com>
---
 x86/bios/bios-rom.S | 5 +++++
 x86/bios/entry.S    | 5 +++++
 2 files changed, 10 insertions(+)

diff --git a/x86/bios/bios-rom.S b/x86/bios/bios-rom.S
index 3269ce9..d1c8b25 100644
--- a/x86/bios/bios-rom.S
+++ b/x86/bios/bios-rom.S
@@ -10,3 +10,8 @@
 GLOBAL(bios_rom)
 	.incbin "x86/bios/bios.bin"
 END(bios_rom)
+
+/*
+ * Add this section to ensure final binary has a non-executable stack.
+ */
+.section .note.GNU-stack,"",@progbits
diff --git a/x86/bios/entry.S b/x86/bios/entry.S
index 85056e9..1b71f89 100644
--- a/x86/bios/entry.S
+++ b/x86/bios/entry.S
@@ -90,3 +90,8 @@ GLOBAL(__locals)
 #include "local.S"
 
 END(__locals)
+
+/*
+ * Add this section to ensure final binary has a non-executable stack.
+ */
+.section .note.GNU-stack,"",@progbits
-- 
2.25.1

On Mon, Mar 14, 2022 at 05:11:08PM +0000, Alexandru Elisei wrote:
> Hi,
> 
> On Fri, Mar 11, 2022 at 11:23:21AM +0000, Andre Przywara wrote:
> > On Thu, 10 Mar 2022 14:56:30 +0000
> > Alexandru Elisei <alexandru.elisei@arm.com> 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.
> 
> Martin, would you like to pick up the diff and turn it into a proper patch? You
> don't need to credit me as the author, you can just add a Suggested-by:
> Alexandru Elisei <alexandru.elisei@arm.com> tag in the commit message. Or do you
> want me to turn this into a patch? If I do, I'll add a Reported-by: Martin Radev
> <martin.b.radev@gmail.com> tag to it.
> 
> I don't have a preference, I am asking because you were the first person who
> discovered and tried to fix this.
> 
> Thanks,
> Alex
> 
> > 
> > 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
> > > >   
> >