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

[Andre Przywara <andre.przywara@arm.com>]

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.

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
> >