| Message ID | 20220303231050.2146621-1-martin.b.radev@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Fix few small issues in virtio code | expand |
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 >
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 > >
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 > > > >
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
Hi, On Sun, Mar 27, 2022 at 03:46:51PM +0300, Martin Radev wrote: > Thanks for the explanation and suggestion. > Is this better? Looks good to me, please send it as a separate patch in the next iteration of the series. Thanks, Alex > > 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 > > > > > > > >
On Fri, Mar 04, 2022 at 01:10:45AM +0200, Martin Radev wrote: > Hello everyone, > > Thanks for the reviews in the first patch set. > > This is the second version of the original patch set which addresses > few found overflows in the common virtio code. Since the first version, > the following changes were made: > - the virtio_net warning patch was removed. > - a WARN_ONCE macro is added to help signal that an issue was observed, > but without polluting the log. > - a couple of improvements in sanitization and style. > - TODO comment for missing handling of multi-byte PCI accesses. > > The Makefile change is kept in its original form because I didn't understand > if there is an issue with it on aarch64. It looks like we're nearly there with this series. Martin, do you plan to spin a v3, please? Will