All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] emu: fix executable stack marking
@ 2021-02-03 13:03 Michael Chang
  2021-02-04 22:28 ` Glenn Washburn
  0 siblings, 1 reply; 6+ messages in thread
From: Michael Chang @ 2021-02-03 13:03 UTC (permalink / raw)
  To: grub-devel

The gcc by default assumes executable stack is required if the source
object file doesn't have .note.GNU-stack section in place. If any of the
source objects doesn't incorporate the GNU-stack note, the resulting
program will have executable stack flag set in PT_GNU_STACK program
header to instruct program loader or kernel to set up the exeutable
stack when program loads to memory.

Usually the .note.GNU-stack section will be generated by gcc
automatically if it finds that executable stack is not required. However
it doesn't take care of generating .note.GNU-stack section for those
object files built from assembler sources. This leads to unnecessary
risk of security of exploiting the executable stack because those
assembler sources don't actually require stack to be executable to work.

The grub-emu and grub-emu-lite are found to flag stack as executable
revealed by execstack tool.

 $ mkdir -p build-emu && cd build-emu
 $ ../configure --with-platform=emu && make
 $ execstack -q grub-core/grub-emu grub-core/grub-emu-lite
 X grub-core/grub-emu
 X grub-core/grub-emu-lite

This patch will add the missing GNU-stack note to the assembler source
used by both utilities, therefore the result doesn't count on gcc
default behavior and the executable stack is disabled.

 $ execstack -q grub-core/grub-emu grub-core/grub-emu-lite
 - grub-core/grub-emu
 - grub-core/grub-emu-lite

Signed-off-by: Michael Chang <mchang@suse.com>
---
 grub-core/kern/emu/cache_s.S | 5 +++++
 grub-core/lib/setjmp.S       | 4 ++++
 2 files changed, 9 insertions(+)

diff --git a/grub-core/kern/emu/cache_s.S b/grub-core/kern/emu/cache_s.S
index 2245cddc3..99d901ae0 100644
--- a/grub-core/kern/emu/cache_s.S
+++ b/grub-core/kern/emu/cache_s.S
@@ -2,6 +2,11 @@
 #error "This source is only meant for grub-emu platform"
 #endif
 
+/* An executable stack is not required for these functions */
+#if defined (__linux__) && defined (__ELF__)
+.section .note.GNU-stack,"",@progbits
+#endif
+
 #if defined(__i386__) || defined(__x86_64__)
 /* Nothing is necessary.  */
 #elif defined(__sparc__)
diff --git a/grub-core/lib/setjmp.S b/grub-core/lib/setjmp.S
index aa297ab0a..6128f0a9d 100644
--- a/grub-core/lib/setjmp.S
+++ b/grub-core/lib/setjmp.S
@@ -1,3 +1,7 @@
+/* An executable stack is not required for these functions */
+#if defined (__linux__) && defined (__ELF__)
+.section .note.GNU-stack,"",@progbits
+#endif
 #if defined(__i386__)
 #include "./i386/setjmp.S"
 #elif defined(__x86_64__)
-- 
2.26.2



^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH] emu: fix executable stack marking
  2021-02-03 13:03 [PATCH] emu: fix executable stack marking Michael Chang
@ 2021-02-04 22:28 ` Glenn Washburn
  2021-02-05  4:31   ` Michael Chang
  0 siblings, 1 reply; 6+ messages in thread
From: Glenn Washburn @ 2021-02-04 22:28 UTC (permalink / raw)
  To: Michael Chang via Grub-devel; +Cc: Michael Chang

Hi Michael,

On Wed,  3 Feb 2021 21:03:44 +0800
Michael Chang via Grub-devel <grub-devel@gnu.org> wrote:

> The gcc by default assumes executable stack is required if the source
> object file doesn't have .note.GNU-stack section in place. If any of
> the source objects doesn't incorporate the GNU-stack note, the
> resulting program will have executable stack flag set in PT_GNU_STACK
> program header to instruct program loader or kernel to set up the
> exeutable stack when program loads to memory.
> 
> Usually the .note.GNU-stack section will be generated by gcc
> automatically if it finds that executable stack is not required.
> However it doesn't take care of generating .note.GNU-stack section
> for those object files built from assembler sources. This leads to
> unnecessary risk of security of exploiting the executable stack
> because those assembler sources don't actually require stack to be
> executable to work.
> 
> The grub-emu and grub-emu-lite are found to flag stack as executable
> revealed by execstack tool.
> 
>  $ mkdir -p build-emu && cd build-emu
>  $ ../configure --with-platform=emu && make
>  $ execstack -q grub-core/grub-emu grub-core/grub-emu-lite
>  X grub-core/grub-emu
>  X grub-core/grub-emu-lite
> 
> This patch will add the missing GNU-stack note to the assembler source
> used by both utilities, therefore the result doesn't count on gcc
> default behavior and the executable stack is disabled.
> 
>  $ execstack -q grub-core/grub-emu grub-core/grub-emu-lite
>  - grub-core/grub-emu
>  - grub-core/grub-emu-lite

Am I correct in thinking that this isn't as useful for the bootloader
itself because, I assume, the grub linker doesn't look at that section
header or flag.  I'm wondering if it might be worthwhile to do this for
grub modules for instance.

Glenn


^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] emu: fix executable stack marking
  2021-02-04 22:28 ` Glenn Washburn
@ 2021-02-05  4:31   ` Michael Chang
  0 siblings, 0 replies; 6+ messages in thread
From: Michael Chang @ 2021-02-05  4:31 UTC (permalink / raw)
  To: Glenn Washburn; +Cc: Michael Chang via Grub-devel

On Thu, Feb 04, 2021 at 04:28:10PM -0600, Glenn Washburn wrote:
> Hi Michael,
> 
> On Wed,  3 Feb 2021 21:03:44 +0800
> Michael Chang via Grub-devel <grub-devel@gnu.org> wrote:
> 
> > The gcc by default assumes executable stack is required if the source
> > object file doesn't have .note.GNU-stack section in place. If any of
> > the source objects doesn't incorporate the GNU-stack note, the
> > resulting program will have executable stack flag set in PT_GNU_STACK
> > program header to instruct program loader or kernel to set up the
> > exeutable stack when program loads to memory.
> > 
> > Usually the .note.GNU-stack section will be generated by gcc
> > automatically if it finds that executable stack is not required.
> > However it doesn't take care of generating .note.GNU-stack section
> > for those object files built from assembler sources. This leads to
> > unnecessary risk of security of exploiting the executable stack
> > because those assembler sources don't actually require stack to be
> > executable to work.
> > 
> > The grub-emu and grub-emu-lite are found to flag stack as executable
> > revealed by execstack tool.
> > 
> >  $ mkdir -p build-emu && cd build-emu
> >  $ ../configure --with-platform=emu && make
> >  $ execstack -q grub-core/grub-emu grub-core/grub-emu-lite
> >  X grub-core/grub-emu
> >  X grub-core/grub-emu-lite
> > 
> > This patch will add the missing GNU-stack note to the assembler source
> > used by both utilities, therefore the result doesn't count on gcc
> > default behavior and the executable stack is disabled.
> > 
> >  $ execstack -q grub-core/grub-emu grub-core/grub-emu-lite
> >  - grub-core/grub-emu
> >  - grub-core/grub-emu-lite
> 
> Am I correct in thinking that this isn't as useful for the bootloader
> itself because, I assume, the grub linker doesn't look at that section
> header or flag.  I'm wondering if it might be worthwhile to do this for
> grub modules for instance.

The .note.GNU-stack section does exist in host's .module files but gets
stripped out by genmod.sh during the build process for the target grub
modules (*.mod). It could be intentional for the good of taking less
memory footprint, as you have mentioned that bootloader itself don't use
it thus it is reasonable to do so.

But I could imagine that it is unpleasant to see scanelf to output "bad
things" of grub modules.

  !WX --- ---  ./grub-core/read.mod

I think we can attribute this to be a cosmetic issue and the "fix" may
be otherwise unfavourable to the grub itself.

Thanks,
Michael

> 
> Glenn
> 



^ permalink raw reply	[flat|nested] 6+ messages in thread
* [PATCH] emu: fix executable stack marking
@ 2021-08-02  9:40 Michael Chang
  2021-08-05 15:05 ` Daniel Kiper
  0 siblings, 1 reply; 6+ messages in thread
From: Michael Chang @ 2021-08-02  9:40 UTC (permalink / raw)
  To: The development of GNU GRUB

The gcc by default assumes executable stack is required if the source
object file doesn't have .note.GNU-stack section in place. If any of the
source objects doesn't incorporate the GNU-stack note, the resulting
program will have executable stack flag set in PT_GNU_STACK program
header to instruct program loader or kernel to set up the exeutable
stack when program loads to memory.

Usually the .note.GNU-stack section will be generated by gcc
automatically if it finds that executable stack is not required. However
it doesn't take care of generating .note.GNU-stack section for those
object files built from assembler sources. This leads to unnecessary
risk of security of exploiting the executable stack because those
assembler sources don't actually require stack to be executable to work.

The grub-emu and grub-emu-lite are found to flag stack as executable
revealed by execstack tool.

 $ mkdir -p build-emu && cd build-emu
 $ ../configure --with-platform=emu && make
 $ execstack -q grub-core/grub-emu grub-core/grub-emu-lite
 X grub-core/grub-emu
 X grub-core/grub-emu-lite

This patch will add the missing GNU-stack note to the assembler source
used by both utilities, therefore the result doesn't count on gcc
default behavior and the executable stack is disabled.

 $ execstack -q grub-core/grub-emu grub-core/grub-emu-lite
 - grub-core/grub-emu
 - grub-core/grub-emu-lite

Signed-off-by: Michael Chang <mchang@suse.com>
---
 grub-core/kern/emu/cache_s.S | 5 +++++
 grub-core/lib/setjmp.S       | 4 ++++
 2 files changed, 9 insertions(+)

diff --git a/grub-core/kern/emu/cache_s.S b/grub-core/kern/emu/cache_s.S
index 2245cddc3..c35926f6b 100644
--- a/grub-core/kern/emu/cache_s.S
+++ b/grub-core/kern/emu/cache_s.S
@@ -2,6 +2,11 @@
 #error "This source is only meant for grub-emu platform"
 #endif
 
+/* An executable stack is not required for these functions */
+#if defined (__linux__) && defined (__ELF__)
+.section .note.GNU-stack,"",%progbits
+#endif
+
 #if defined(__i386__) || defined(__x86_64__)
 /* Nothing is necessary.  */
 #elif defined(__sparc__)
diff --git a/grub-core/lib/setjmp.S b/grub-core/lib/setjmp.S
index aa297ab0a..e6c3608e1 100644
--- a/grub-core/lib/setjmp.S
+++ b/grub-core/lib/setjmp.S
@@ -1,3 +1,7 @@
+/* An executable stack is not required for these functions */
+#if defined (__linux__) && defined (__ELF__)
+.section .note.GNU-stack,"",%progbits
+#endif
 #if defined(__i386__)
 #include "./i386/setjmp.S"
 #elif defined(__x86_64__)
-- 
2.31.1



^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH] emu: fix executable stack marking
  2021-08-02  9:40 Michael Chang
@ 2021-08-05 15:05 ` Daniel Kiper
  2021-08-09  5:05   ` Michael Chang
  0 siblings, 1 reply; 6+ messages in thread
From: Daniel Kiper @ 2021-08-05 15:05 UTC (permalink / raw)
  To: Michael Chang via Grub-devel; +Cc: Michael Chang

On Mon, Aug 02, 2021 at 05:40:57PM +0800, Michael Chang via Grub-devel wrote:
> The gcc by default assumes executable stack is required if the source
> object file doesn't have .note.GNU-stack section in place. If any of the
> source objects doesn't incorporate the GNU-stack note, the resulting
> program will have executable stack flag set in PT_GNU_STACK program
> header to instruct program loader or kernel to set up the exeutable
> stack when program loads to memory.
>
> Usually the .note.GNU-stack section will be generated by gcc
> automatically if it finds that executable stack is not required. However
> it doesn't take care of generating .note.GNU-stack section for those
> object files built from assembler sources. This leads to unnecessary
> risk of security of exploiting the executable stack because those
> assembler sources don't actually require stack to be executable to work.
>
> The grub-emu and grub-emu-lite are found to flag stack as executable
> revealed by execstack tool.

Did you test all executables for all supported architectures and platforms?

>  $ mkdir -p build-emu && cd build-emu
>  $ ../configure --with-platform=emu && make
>  $ execstack -q grub-core/grub-emu grub-core/grub-emu-lite
>  X grub-core/grub-emu
>  X grub-core/grub-emu-lite
>
> This patch will add the missing GNU-stack note to the assembler source
> used by both utilities, therefore the result doesn't count on gcc
> default behavior and the executable stack is disabled.
>
>  $ execstack -q grub-core/grub-emu grub-core/grub-emu-lite
>  - grub-core/grub-emu
>  - grub-core/grub-emu-lite
>
> Signed-off-by: Michael Chang <mchang@suse.com>

Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

Daniel


^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] emu: fix executable stack marking
  2021-08-05 15:05 ` Daniel Kiper
@ 2021-08-09  5:05   ` Michael Chang
  0 siblings, 0 replies; 6+ messages in thread
From: Michael Chang @ 2021-08-09  5:05 UTC (permalink / raw)
  To: The development of GNU GRUB

On Thu, Aug 05, 2021 at 05:05:05PM +0200, Daniel Kiper wrote:
> On Mon, Aug 02, 2021 at 05:40:57PM +0800, Michael Chang via Grub-devel wrote:
> > The gcc by default assumes executable stack is required if the source
> > object file doesn't have .note.GNU-stack section in place. If any of the
> > source objects doesn't incorporate the GNU-stack note, the resulting
> > program will have executable stack flag set in PT_GNU_STACK program
> > header to instruct program loader or kernel to set up the exeutable
> > stack when program loads to memory.
> >
> > Usually the .note.GNU-stack section will be generated by gcc
> > automatically if it finds that executable stack is not required. However
> > it doesn't take care of generating .note.GNU-stack section for those
> > object files built from assembler sources. This leads to unnecessary
> > risk of security of exploiting the executable stack because those
> > assembler sources don't actually require stack to be executable to work.
> >
> > The grub-emu and grub-emu-lite are found to flag stack as executable
> > revealed by execstack tool.
> 
> Did you test all executables for all supported architectures and platforms?

No. But the test should cover grub executables on as many plarforms as
possbile as follow.

 i386-pc
 i386-efi
 x86_64-efi
 powerpc-ieee1275
 arm64-efi
 x86_64-xen
 x86_64-emu
 arm64-emu

> >  $ mkdir -p build-emu && cd build-emu
> >  $ ../configure --with-platform=emu && make
> >  $ execstack -q grub-core/grub-emu grub-core/grub-emu-lite
> >  X grub-core/grub-emu
> >  X grub-core/grub-emu-lite
> >
> > This patch will add the missing GNU-stack note to the assembler source
> > used by both utilities, therefore the result doesn't count on gcc
> > default behavior and the executable stack is disabled.
> >
> >  $ execstack -q grub-core/grub-emu grub-core/grub-emu-lite
> >  - grub-core/grub-emu
> >  - grub-core/grub-emu-lite
> >
> > Signed-off-by: Michael Chang <mchang@suse.com>
> 
> Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

Thanks for reviewing the patch.

Regards,
Michael

> 
> Daniel
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel



^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-08-09  5:05 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-02-03 13:03 [PATCH] emu: fix executable stack marking Michael Chang
2021-02-04 22:28 ` Glenn Washburn
2021-02-05  4:31   ` Michael Chang
  -- strict thread matches above, loose matches on Subject: below --
2021-08-02  9:40 Michael Chang
2021-08-05 15:05 ` Daniel Kiper
2021-08-09  5:05   ` Michael Chang

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.