Fix hppa/ia64/microblaze executable stack default (bug 22156)

classic Classic list List threaded Threaded
15 messages Options
Reply | Threaded
Open this post in threaded view
|

Fix hppa/ia64/microblaze executable stack default (bug 22156)

Joseph Myers
As per https://gcc.gnu.org/ml/gcc-patches/2017-09/msg01220.html hppa,
ia64 and microblaze default to non-executable stacks in the Linux
kernel.  glibc however defines DEFAULT_STACK_PERMS to include PF_X for
those architectures, meaning (a) elf/check-execstack fails and (b)
(from code inspection, not tested, but this is why I think this is a
user-visible bug) thread stacks are unnecessarily mapped with execute
permission.  This patch fixes the DEFAULT_STACK_PERMS definitions in
question.

Tested (compilation only) with build-many-glibcs.py for those
configurations.  This fixes the check-execstack failure (hppa still
has a check-textrel failure as the only remaining issue stopping that
architecture having clean build-many-glibcs.py results; hopefully
architecture maintainers can help resolve that).

2017-09-19  Joseph Myers  <[hidden email]>

        [BZ #22156]
        * sysdeps/hppa/stackinfo.h (DEFAULT_STACK_PERMS): Remove PF_X.
        * sysdeps/ia64/stackinfo.h (DEFAULT_STACK_PERMS): Likewise.
        * sysdeps/microblaze/stackinfo.h (DEFAULT_STACK_PERMS): Likewise.

diff --git a/sysdeps/hppa/stackinfo.h b/sysdeps/hppa/stackinfo.h
index 83b1da1..a3af38f 100644
--- a/sysdeps/hppa/stackinfo.h
+++ b/sysdeps/hppa/stackinfo.h
@@ -23,9 +23,8 @@
 
 #include <elf.h>
 
-/* Default to an executable stack.  PF_X can be overridden if PT_GNU_STACK is
- * present, but it is presumed absent.  */
-#define DEFAULT_STACK_PERMS (PF_R|PF_W|PF_X)
+/* Default to a non-executable stack.    */
+#define DEFAULT_STACK_PERMS (PF_R|PF_W)
 
 /* On PA the stack grows up.  */
 #define _STACK_GROWS_UP 1
diff --git a/sysdeps/ia64/stackinfo.h b/sysdeps/ia64/stackinfo.h
index 87e1448..a50e727 100644
--- a/sysdeps/ia64/stackinfo.h
+++ b/sysdeps/ia64/stackinfo.h
@@ -27,8 +27,7 @@
    here.  */
 #define _STACK_GROWS_DOWN 1
 
-/* Default to an executable stack.  PF_X can be overridden if PT_GNU_STACK is
- * present, but it is presumed absent.  */
-#define DEFAULT_STACK_PERMS (PF_R|PF_W|PF_X)
+/* Default to a non-executable stack.  */
+#define DEFAULT_STACK_PERMS (PF_R|PF_W)
 
 #endif /* stackinfo.h */
diff --git a/sysdeps/microblaze/stackinfo.h b/sysdeps/microblaze/stackinfo.h
index 3062b1e..05755c7 100644
--- a/sysdeps/microblaze/stackinfo.h
+++ b/sysdeps/microblaze/stackinfo.h
@@ -27,8 +27,7 @@
 /* On MicroBlaze the stack grows down.  */
 # define _STACK_GROWS_DOWN 1
 
-/* Default to an executable stack.  PF_X can be overridden if PT_GNU_STACK is
- * present, but it is presumed absent.  */
-# define DEFAULT_STACK_PERMS (PF_R|PF_W|PF_X)
+/* Default to a non-executable stack.  */
+# define DEFAULT_STACK_PERMS (PF_R|PF_W)
 
 #endif /* stackinfo.h.  */

--
Joseph S. Myers
[hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: Fix hppa/ia64/microblaze executable stack default (bug 22156)

Carlos O'Donell-6
On 09/19/2017 07:46 AM, Joseph Myers wrote:

> As per https://gcc.gnu.org/ml/gcc-patches/2017-09/msg01220.html hppa,
> ia64 and microblaze default to non-executable stacks in the Linux
> kernel.  glibc however defines DEFAULT_STACK_PERMS to include PF_X for
> those architectures, meaning (a) elf/check-execstack fails and (b)
> (from code inspection, not tested, but this is why I think this is a
> user-visible bug) thread stacks are unnecessarily mapped with execute
> permission.  This patch fixes the DEFAULT_STACK_PERMS definitions in
> question.
>
> Tested (compilation only) with build-many-glibcs.py for those
> configurations.  This fixes the check-execstack failure (hppa still
> has a check-textrel failure as the only remaining issue stopping that
> architecture having clean build-many-glibcs.py results; hopefully
> architecture maintainers can help resolve that).

I thought that PF_X was required for hppa because there was still code
generated by gcc for hppa that needed it? Otherwise *I* would have
removed it when I did the PT_GNU_STACK cleanups for all the arches.

Dave is the best positioned to answer this question.

--
Cheers,
Carlos.
Reply | Threaded
Open this post in threaded view
|

Re: Fix hppa/ia64/microblaze executable stack default (bug 22156)

Jeff Law
On 09/19/2017 07:56 AM, Carlos O'Donell wrote:

> On 09/19/2017 07:46 AM, Joseph Myers wrote:
>> As per https://gcc.gnu.org/ml/gcc-patches/2017-09/msg01220.html hppa,
>> ia64 and microblaze default to non-executable stacks in the Linux
>> kernel.  glibc however defines DEFAULT_STACK_PERMS to include PF_X for
>> those architectures, meaning (a) elf/check-execstack fails and (b)
>> (from code inspection, not tested, but this is why I think this is a
>> user-visible bug) thread stacks are unnecessarily mapped with execute
>> permission.  This patch fixes the DEFAULT_STACK_PERMS definitions in
>> question.
>>
>> Tested (compilation only) with build-many-glibcs.py for those
>> configurations.  This fixes the check-execstack failure (hppa still
>> has a check-textrel failure as the only remaining issue stopping that
>> architecture having clean build-many-glibcs.py results; hopefully
>> architecture maintainers can help resolve that).
>
> I thought that PF_X was required for hppa because there was still code
> generated by gcc for hppa that needed it? Otherwise *I* would have
> removed it when I did the PT_GNU_STACK cleanups for all the arches.
>
> Dave is the best positioned to answer this question.
I don't think the PA was ever converted to the new bits from the Adacore
guys -- meaning I think it still generates executable stack trampolines
for nested functions.

Dave would know for sure :-)

jeff
Reply | Threaded
Open this post in threaded view
|

Re: Fix hppa/ia64/microblaze executable stack default (bug 22156)

Joseph Myers
On Tue, 19 Sep 2017, Jeff Law wrote:

> > I thought that PF_X was required for hppa because there was still code
> > generated by gcc for hppa that needed it? Otherwise *I* would have
> > removed it when I did the PT_GNU_STACK cleanups for all the arches.
> >
> > Dave is the best positioned to answer this question.
> I don't think the PA was ever converted to the new bits from the Adacore
> guys -- meaning I think it still generates executable stack trampolines
> for nested functions.
>
> Dave would know for sure :-)

If Andreas's description of the kernel default is correct, I'd expect that
to mean that any attempt to call a pointer to a nested function would
already fail because the kernel had in fact mapped the stack
non-executable (so the glibc patch would accurately reflect what the
kernel does - a GCC patch would *also* be needed for pointers to nested
functions to work, but it would only need to output stack markers in the
case where trampolines had been generated, to request an executable stack
in that case).

--
Joseph S. Myers
[hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: Fix hppa/ia64/microblaze executable stack default (bug 22156)

Andreas Schwab
In reply to this post by Jeff Law
On Sep 19 2017, Jeff Law <[hidden email]> wrote:

> I don't think the PA was ever converted to the new bits from the Adacore
> guys -- meaning I think it still generates executable stack trampolines
> for nested functions.

But that should trigger emitting a stack note, overriding the default.
It's the absence of the note that is relevant here.

Andreas.

--
Andreas Schwab, SUSE Labs, [hidden email]
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
Reply | Threaded
Open this post in threaded view
|

Re: Fix hppa/ia64/microblaze executable stack default (bug 22156)

Carlos O'Donell-6
On 09/19/2017 08:54 AM, Andreas Schwab wrote:
> It's the absence of the note that is relevant here.

It can't be safe to override this?

This breaks backwards compatibility with old binaries that don't have PT_GNU_STACK
but need executable stacks?

I thought perhaps we were arguing to make old binaries
safer, at the risk of breaking some of them.

--
Cheers,
Carlos.
Reply | Threaded
Open this post in threaded view
|

Re: Fix hppa/ia64/microblaze executable stack default (bug 22156)

Joseph Myers
On Tue, 19 Sep 2017, Carlos O'Donell wrote:

> On 09/19/2017 08:54 AM, Andreas Schwab wrote:
> > It's the absence of the note that is relevant here.
>
> It can't be safe to override this?
>
> This breaks backwards compatibility with old binaries that don't have PT_GNU_STACK
> but need executable stacks?
>
> I thought perhaps we were arguing to make old binaries
> safer, at the risk of breaking some of them.

My understanding is:

(a) As per Andreas, the kernel defaults to non-executable stacks on these
three architectures.

(b) That is, if ld.so has no PT_GNU_STACK, the stack is non-executable; if
ld.so does have PT_GNU_STACK, the kernel sets the permissions on the stack
accordingly (which can include making it executable for trampoline use
when the default is non-executable).

(c) At present, these GCC ports will never create PT_GNU_STACK markings to
indicate that a trampoline needs an executable stack (if it does).

(d) glibc presumes that the kernel set the stack permissions to
DEFAULT_STACK_PERMS.  If the executable or a loaded shared library
introduces an executable stack requirement, the dynamic linker can make
the stack executable - but it will never do so if it thinks the kernel
already did so.

(e) DEFAULT_STACK_PERMS is used when creating thread stacks.

Now, if trampolines need executable stacks (and in some cases, especially
with function descriptors, they may not) but work at present on those
architectures with the default stack (rather than a thread stack), that
suggests there is something wrong with the above analysis.

--
Joseph S. Myers
[hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: Fix hppa/ia64/microblaze executable stack default (bug 22156)

John David Anglin
In reply to this post by Jeff Law
On 2017-09-19, at 10:42 AM, Jeff Law wrote:

> I don't think the PA was ever converted to the new bits from the Adacore
> guys -- meaning I think it still generates executable stack trampolines
> for nested functions.

Yes.

Dave
--
John David Anglin [hidden email]



Reply | Threaded
Open this post in threaded view
|

Re: Fix hppa/ia64/microblaze executable stack default (bug 22156)

Joseph Myers
On Fri, 22 Sep 2017, John David Anglin wrote:

> On 2017-09-19, at 10:42 AM, Jeff Law wrote:
>
> > I don't think the PA was ever converted to the new bits from the Adacore
> > guys -- meaning I think it still generates executable stack trampolines
> > for nested functions.
>
> Yes.

We now have two different contradictory assertions about hppa, here and
<https://gcc.gnu.org/ml/gcc-patches/2017-09/msg01449.html>.

Can someone with hppa-linux hardware confirm what stack permissions the
kernel in fact starts processes with in the absence of GNU-stack markers
(Andreas's analysis indicates non-executable), what if any changes glibc
makes to those permissions after process creation, and whether trampolines
work in such a process?  Testing whether my glibc patch changes the
answers to any of those questions would be a bonus.

--
Joseph S. Myers
[hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: Fix hppa/ia64/microblaze executable stack default (bug 22156)

John David Anglin
On 2017-09-22, at 12:08 PM, Joseph Myers wrote:

> On Fri, 22 Sep 2017, John David Anglin wrote:
>
>> On 2017-09-19, at 10:42 AM, Jeff Law wrote:
>>
>>> I don't think the PA was ever converted to the new bits from the Adacore
>>> guys -- meaning I think it still generates executable stack trampolines
>>> for nested functions.
>>
>> Yes.
>
> We now have two different contradictory assertions about hppa, here and
> <https://gcc.gnu.org/ml/gcc-patches/2017-09/msg01449.html>.
>
> Can someone with hppa-linux hardware confirm what stack permissions the
> kernel in fact starts processes with in the absence of GNU-stack markers
> (Andreas's analysis indicates non-executable), what if any changes glibc
> makes to those permissions after process creation, and whether trampolines
> work in such a process?  Testing whether my glibc patch changes the
> answers to any of those questions would be a bonus.


This is what I see for /bin/bash when we reach main:
f8d01000-f8d23000 rwxp 00000000 00:00 0                                  [stack]

This what I see for ld.so at _start:
f8d01000-f8d23000 rwxp 00000000 00:00 0                                  [stack]

Don't see any GNU-stack markers in either.

I'll try to test patch this weekend.

Dave
--
John David Anglin [hidden email]



Reply | Threaded
Open this post in threaded view
|

Re: Fix hppa/ia64/microblaze executable stack default (bug 22156)

Joseph Myers
On Fri, 22 Sep 2017, John David Anglin wrote:

> > Can someone with hppa-linux hardware confirm what stack permissions the
> > kernel in fact starts processes with in the absence of GNU-stack markers
> > (Andreas's analysis indicates non-executable), what if any changes glibc
> > makes to those permissions after process creation, and whether trampolines
> > work in such a process?  Testing whether my glibc patch changes the
> > answers to any of those questions would be a bonus.
>
>
> This is what I see for /bin/bash when we reach main:
> f8d01000-f8d23000 rwxp 00000000 00:00 0                                  [stack]
>
> This what I see for ld.so at _start:
> f8d01000-f8d23000 rwxp 00000000 00:00 0                                  [stack]
>
> Don't see any GNU-stack markers in either.
>
> I'll try to test patch this weekend.

Thanks.

Given what Andreas said at
<https://gcc.gnu.org/ml/gcc-patches/2017-09/msg01220.html>, does this mean
there are other causes of executable stacks in the kernel, such as
VM_STACK_DEFAULT_FLAGS?  If so, maybe hppa and microblaze do in fact need
the GCC patch rather than the glibc one?

--
Joseph S. Myers
[hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: Fix hppa/ia64/microblaze executable stack default (bug 22156)

John David Anglin
On 2017-09-22, at 4:11 PM, Joseph Myers wrote:

> Given what Andreas said at
> <https://gcc.gnu.org/ml/gcc-patches/2017-09/msg01220.html>, does this mean
> there are other causes of executable stacks in the kernel, such as
> VM_STACK_DEFAULT_FLAGS?  If so, maybe hppa and microblaze do in fact need
> the GCC patch rather than the glibc one?

VM_STACK_DEFAULT_FLAGS defaults to VM_DATA_DEFAULT_FLAGS when it is
not defined, and it is not defined on hppa.

On hppa,  VM_DATA_DEFAULT_FLAGS, and it is:

#define VM_DATA_DEFAULT_FLAGS   (VM_READ | VM_WRITE | VM_EXEC | \
                                 VM_MAYREAD | VM_MAYWRITE | VM_MAYEXEC)

Dave
--
John David Anglin [hidden email]



Reply | Threaded
Open this post in threaded view
|

Re: Fix hppa/ia64/microblaze executable stack default (bug 22156)

Joseph Myers
On Fri, 22 Sep 2017, John David Anglin wrote:

> On 2017-09-22, at 4:11 PM, Joseph Myers wrote:
>
> > Given what Andreas said at
> > <https://gcc.gnu.org/ml/gcc-patches/2017-09/msg01220.html>, does this mean
> > there are other causes of executable stacks in the kernel, such as
> > VM_STACK_DEFAULT_FLAGS?  If so, maybe hppa and microblaze do in fact need
> > the GCC patch rather than the glibc one?
>
> VM_STACK_DEFAULT_FLAGS defaults to VM_DATA_DEFAULT_FLAGS when it is
> not defined, and it is not defined on hppa.
>
> On hppa,  VM_DATA_DEFAULT_FLAGS, and it is:
>
> #define VM_DATA_DEFAULT_FLAGS   (VM_READ | VM_WRITE | VM_EXEC | \
>                                  VM_MAYREAD | VM_MAYWRITE | VM_MAYEXEC)

Given this, and your experiment showing the stack is indeed executable on
hppa, are the hppa parts of the GCC patch OK?  (I think we've established
that ia64 should use the glibc change.  Based on VM_DATA_DEFAULT_FLAGS,
microblaze would use the GCC change but hopefully there will be more
information from microblaze people soon.)

--
Joseph S. Myers
[hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: Fix hppa/ia64/microblaze executable stack default (bug 22156)

John David Anglin
On 2017-09-22, at 4:35 PM, Joseph Myers wrote:

> Given this, and your experiment showing the stack is indeed executable on
> hppa, are the hppa parts of the GCC patch OK?

The hppa parts are OK.

Thanks,
Dave
--
John David Anglin [hidden email]



Reply | Threaded
Open this post in threaded view
|

RE: Fix hppa/ia64/microblaze executable stack default (bug 22156)

Nagaraju Mekala-2
In reply to this post by Joseph Myers
> -----Original Message-----
> From: [hidden email] [mailto:libc-alpha-
> [hidden email]] On Behalf Of Joseph Myers
> Sent: Saturday, September 23, 2017 2:05 AM
> To: John David Anglin <[hidden email]>
> Cc: Jeff Law <[hidden email]>; Carlos O'Donell <[hidden email]>; GNU C
> Library <[hidden email]>; Mike Frysinger <[hidden email]>;
> Helge Deller <[hidden email]>
> Subject: Re: Fix hppa/ia64/microblaze executable stack default (bug 22156)
>
> On Fri, 22 Sep 2017, John David Anglin wrote:
>
> > On 2017-09-22, at 4:11 PM, Joseph Myers wrote:
> >
> > > Given what Andreas said at
> > > <https://gcc.gnu.org/ml/gcc-patches/2017-09/msg01220.html>, does
> > > this mean there are other causes of executable stacks in the kernel,
> > > such as VM_STACK_DEFAULT_FLAGS?  If so, maybe hppa and microblaze
> do
> > > in fact need the GCC patch rather than the glibc one?
> >
> > VM_STACK_DEFAULT_FLAGS defaults to VM_DATA_DEFAULT_FLAGS when it
> is
> > not defined, and it is not defined on hppa.
> >
> > On hppa,  VM_DATA_DEFAULT_FLAGS, and it is:
> >
> > #define VM_DATA_DEFAULT_FLAGS   (VM_READ | VM_WRITE | VM_EXEC |
> \
> >                                  VM_MAYREAD | VM_MAYWRITE |
> > VM_MAYEXEC)
>
> Given this, and your experiment showing the stack is indeed executable on
> hppa, are the hppa parts of the GCC patch OK?  (I think we've established that
> ia64 should use the glibc change.  Based on VM_DATA_DEFAULT_FLAGS,
> microblaze would use the GCC change but hopefully there will be more
> information from microblaze people soon.)
Yes, for Microblaze we need to apply GCC patch.
I have applied the patch and found no regressions with it.

Thanks,
Nagaraju
> --
> Joseph S. Myers
> [hidden email]