2004-03-23 23:13:33

by Kurt Garloff

[permalink] [raw]
Subject: Non-Exec stack patches

Hi,

find attached a patch to parse the elf binaries for a PT_GNU_STACK
section to set the stack non-executable if possible.
Most parts have been shamelessly stolen from Ingo Molnar's more
ambitious stackshield
http://people.redhat.com/mingo/exec-shield/exec-shield-2.6.4-C9

The toolchain has meainwhile support for marking the binaries with a
PT_GNU_STACK section with ot without x bit as needed.

If no such section is found, we leave the stack to whatever the
arch defaults to. If there is one, we explicitly disabled the VM_EXEC
bit if no x bit is found, otherwise explicitly enable.

I believe this part should be merged into official mainstream kernels.
Ingo, what do you think?

Regards,
--
Kurt Garloff <[email protected]> Cologne, DE
SUSE LINUX AG, Nuernberg, DE SUSE Labs (Head)


Attachments:
(No filename) (0.00 B)
(No filename) (189.00 B)
Download all attachments

2004-03-23 23:22:49

by Ingo Molnar

[permalink] [raw]
Subject: Re: Non-Exec stack patches


On Wed, 24 Mar 2004, Kurt Garloff wrote:

> find attached a patch to parse the elf binaries for a PT_GNU_STACK
> section to set the stack non-executable if possible.
> Most parts have been shamelessly stolen from Ingo Molnar's more
> ambitious stackshield
> http://people.redhat.com/mingo/exec-shield/exec-shield-2.6.4-C9
>
> The toolchain has meainwhile support for marking the binaries with a
> PT_GNU_STACK section with ot without x bit as needed.
>
> If no such section is found, we leave the stack to whatever the arch
> defaults to. If there is one, we explicitly disabled the VM_EXEC bit if
> no x bit is found, otherwise explicitly enable.
>
> I believe this part should be merged into official mainstream kernels.
> Ingo, what do you think?

agreed, and the patch looks good to me.

Ingo

2004-03-23 23:47:37

by Andrew Morton

[permalink] [raw]
Subject: Re: Non-Exec stack patches

Kurt Garloff <[email protected]> wrote:
>
> find attached a patch to parse the elf binaries for a PT_GNU_STACK
> section to set the stack non-executable if possible.

This patch propagates the PT_GNU_STACK setting into the vma flags, allowing
the architecture to set the stack permissions non-executable if the
architecture chooses to do that, yes?

Which architectures are currently making their pre-page execute permissions
depend upon VM_EXEC? Would additional arch patches be needed for this?

This may not get past Linus of course. It doesn't even get past me with
that magical undocumented -1/0/+1 value of the executable_stack argument.
Please replace that with a proper, commented, #defined-or-enumerated value,
thanks.

2004-03-24 00:22:20

by Kurt Garloff

[permalink] [raw]
Subject: Re: Non-Exec stack patches

On Tue, Mar 23, 2004 at 03:49:37PM -0800, Andrew Morton wrote:
> This patch propagates the PT_GNU_STACK setting into the vma flags, allowing
> the architecture to set the stack permissions non-executable if the
> architecture chooses to do that, yes?

Right.

> Which architectures are currently making their pre-page execute permissions
> depend upon VM_EXEC? Would additional arch patches be needed for this?

It works on AMD64 (not ia32e), both for 64bit and 32bit binaries.
I have not yet tested other archs.

If the values in the protection_map are different depending on bit 2,
the patch will be effecitve. (OK, the CPU/MMU needs to honour the
setting of course.) Most likely, the values for
protection_map[7] is PAGE_COPY_EXEC and of protection_map[3] is
PAGE_COPY.

> This may not get past Linus of course. It doesn't even get past me with
> that magical undocumented -1/0/+1 value of the executable_stack argument.
> Please replace that with a proper, commented, #defined-or-enumerated value,

As you wish, master.
Slightly edited and untested patch attached.

Regards,
--
Kurt Garloff <[email protected]> [Koeln, DE]
Physics:Plasma modeling <[email protected]> [TU Eindhoven, NL]
Linux: SUSE Labs (Head) <[email protected]> [SUSE Nuernberg, DE]


Attachments:
(No filename) (0.00 B)
(No filename) (189.00 B)
Download all attachments

2004-03-24 00:39:04

by David Mosberger

[permalink] [raw]
Subject: Re: Non-Exec stack patches

>>>>> On Wed, 24 Mar 2004 01:21:49 +0100, Kurt Garloff <[email protected]> said:

>> Which architectures are currently making their pre-page execute
>> permissions depend upon VM_EXEC? Would additional arch patches
>> be needed for this?

Kurt> It works on AMD64 (not ia32e), both for 64bit and 32bit
Kurt> binaries. I have not yet tested other archs.

IA64 Linux had non-executable stack/data before PT_GNU_STACK was invented.
It's keyed off a ELF header flags bit:

/* Least-significant four bits of ELF header's e_flags are
OS-specific. The bits are interpreted as follows by Linux: */
#define EF_IA_64_LINUX_EXECUTABLE_STACK 0x1

I guess I never quiet understood why an entire program header is
needed for this, but that's just me.

--david

2004-03-24 00:38:58

by Andrew Morton

[permalink] [raw]
Subject: Re: Non-Exec stack patches

Kurt Garloff <[email protected]> wrote:
>
> > Which architectures are currently making their pre-page execute permissions
> > depend upon VM_EXEC? Would additional arch patches be needed for this?
>
> It works on AMD64 (not ia32e), both for 64bit and 32bit binaries.
> I have not yet tested other archs.
>
> If the values in the protection_map are different depending on bit 2,
> the patch will be effecitve. (OK, the CPU/MMU needs to honour the
> setting of course.) Most likely, the values for
> protection_map[7] is PAGE_COPY_EXEC and of protection_map[3] is
> PAGE_COPY.

OK.

> > This may not get past Linus of course. It doesn't even get past me with
> > that magical undocumented -1/0/+1 value of the executable_stack argument.
> > Please replace that with a proper, commented, #defined-or-enumerated value,
>
> As you wish, master.
> Slightly edited and untested patch attached.

It gets rejects in arch/x86_64/ia32/ia32_binfmt.c and
arch/ia64/ia32/binfmt_elf32.c - someone has been dinking with your
put_dirty_page() prototype. I dropped those bits.

And I added the missing bit:

--- 25/include/linux/binfmts.h~noexec-stack-comments Tue Mar 23 16:35:50 2004
+++ 25-akpm/include/linux/binfmts.h Tue Mar 23 16:37:11 2004
@@ -62,9 +62,12 @@ extern int prepare_binprm(struct linux_b
extern void remove_arg_zero(struct linux_binprm *);
extern int search_binary_handler(struct linux_binprm *,struct pt_regs *);
extern int flush_old_exec(struct linux_binprm * bprm);
-#define EXSTACK_DEFAULT 0
-#define EXSTACK_DISABLE_X 1
-#define EXSTACK_ENABLE_X 2
+
+/* Stack area protections */
+#define EXSTACK_DEFAULT 0 /* Whatever the arch defaults to */
+#define EXSTACK_DISABLE_X 1 /* Disable executable stacks */
+#define EXSTACK_ENABLE_X 2 /* Enable executable stacks */
+
extern int setup_arg_pages(struct linux_binprm * bprm, int executable_stack);
extern int copy_strings(int argc,char __user * __user * argv,struct linux_binprm *bprm);
extern int copy_strings_kernel(int argc,char ** argv,struct linux_binprm *bprm);



Now, what should the kernel do if the executable requests EXSTACK_DISABLE_X
but the kernel cannot do that? Is it not a bit misleading/dangerous to
permit the executable to run anyway?

2004-03-24 00:41:36

by Ingo Molnar

[permalink] [raw]
Subject: Re: Non-Exec stack patches


On Tue, 23 Mar 2004, Andrew Morton wrote:

> Now, what should the kernel do if the executable requests
> EXSTACK_DISABLE_X but the kernel cannot do that? Is it not a bit
> misleading/dangerous to permit the executable to run anyway?

it's not really a problem. We already ignore the !X bit on x86.

Ingo

2004-03-24 01:33:15

by Ulrich Drepper

[permalink] [raw]
Subject: Re: Non-Exec stack patches

David Mosberger wrote:

> I guess I never quiet understood why an entire program header is
> needed for this, but that's just me.

This just means you haven't looked at the problem.

First, the ELF bits are limited and very crowded on some archs. There
is no central assignment so conflicts will happen.

And one single bit does not cut it. If you'd take a look, the
PT_GNU_STACK entry's permissions field specifies what permissions the
stack must have, not the presence of the field. So at least two bits
are needed which only adds to the problems of finding appropriate bits.

--
➧ Ulrich Drepper ➧ Red Hat, Inc. ➧ 444 Castro St ➧ Mountain View, CA ❖

2004-03-24 01:41:54

by David Mosberger

[permalink] [raw]
Subject: Re: Non-Exec stack patches

>>>>> On Tue, 23 Mar 2004 17:20:12 -0800, Ulrich Drepper <[email protected]> said:

Uli> David Mosberger wrote:
>> I guess I never quiet understood why an entire program header is
>> needed for this, but that's just me.

Uli> First, the ELF bits are limited and very crowded on some archs. There
Uli> is no central assignment so conflicts will happen.

Fair enough, but I don't see why this should imply that platforms that
already do have support for no-exec data/stack should be forced into
using PT_GNU_STACK. Just for uniformity's sake? Or is there a real
benefit?

Uli> And one single bit does not cut it. If you'd take a look, the
Uli> PT_GNU_STACK entry's permissions field specifies what permissions the
Uli> stack must have, not the presence of the field. So at least two bits
Uli> are needed which only adds to the problems of finding appropriate bits.

What stack protections other than RW- and RWX are useful?

--david

2004-03-24 02:14:10

by Ulrich Drepper

[permalink] [raw]
Subject: Re: Non-Exec stack patches

David Mosberger wrote:

> What stack protections other than RW- and RWX are useful?

It's not about "what protections". The three currently recognized
states are PT_GNU_STACK not present, rwx, rw-. Ingo's code documents
this. For those who need more, I'll have a paper coming up for a
conference in Toronto in April.

--
➧ Ulrich Drepper ➧ Red Hat, Inc. ➧ 444 Castro St ➧ Mountain View, CA ❖

2004-03-24 07:02:41

by Jakub Jelinek

[permalink] [raw]
Subject: Re: Non-Exec stack patches

On Tue, Mar 23, 2004 at 05:41:49PM -0800, David Mosberger wrote:
> Uli> First, the ELF bits are limited and very crowded on some archs. There
> Uli> is no central assignment so conflicts will happen.
>
> Fair enough, but I don't see why this should imply that platforms that
> already do have support for no-exec data/stack should be forced into
> using PT_GNU_STACK. Just for uniformity's sake? Or is there a real
> benefit?

Note that PT_GNU_STACK program header is not generated on EM_IA_64 and
EM_PPC64 ATM, because initially I thought it shouldn't be needed
(these 2 arches are the only ones which don't need executable stack
for GCC trampolines).
But I think we should change the toolchain and generate it on IA64 and PPC64
as well (only GCC would need changing, emitting
.section .note.GNU-stack,"",@progbits
at the end of every compile unit, ld takes care of the rest) exactly for
uniformity's sake and because you get ld.so handling free.
GLIBC dynamic linker will take care of making the stack executable if
say a binary which doesn't need executable stack depends on a shared library
which needs executable stack or even dlopens a library which needs
executable stack.

> Uli> And one single bit does not cut it. If you'd take a look, the
> Uli> PT_GNU_STACK entry's permissions field specifies what permissions the
> Uli> stack must have, not the presence of the field. So at least two bits
> Uli> are needed which only adds to the problems of finding appropriate bits.
>
> What stack protections other than RW- and RWX are useful?

At least RW- (read-write but not executable stack), RWX (rw and executable
stack) and no PT_GNU_STACK segment (unknown, not marked binary), but it is
certainly extendable.

Jakub

2004-03-24 07:09:57

by David Mosberger

[permalink] [raw]
Subject: Re: Non-Exec stack patches

>>>>> On Tue, 23 Mar 2004 18:01:14 -0800, Ulrich Drepper <[email protected]> said:

Uli> David Mosberger wrote:
>> What stack protections other than RW- and RWX are useful?

Uli> It's not about "what protections". The three currently
Uli> recognized states are PT_GNU_STACK not present, rwx, rw-.
Uli> Ingo's code documents this. For those who need more, I'll have
Uli> a paper coming up for a conference in Toronto in April.

I'd find the PT_GNU_STACK approach much more appealing if it actually
was used to get rid of the stack-special case in the kernel
altogether. Certainly it seems like it could be used to get rid of
STACK_TOP (just use the segment's p_vaddr). That would still leave
the TASK_UNMAPPED_BASE special-case, but at least it would be a step
in the right direction. Also, in the case of ia64, p_memsz could be
used to give user-level control over whether they want the
register/memory stacks to grow towards each other or apart from each
other.

--david

2004-03-24 07:16:41

by David Mosberger

[permalink] [raw]
Subject: Re: Non-Exec stack patches

>>>>> On Wed, 24 Mar 2004 02:00:20 -0500, Jakub Jelinek <[email protected]> said:

Jakub> But I think we should change the toolchain and generate it on
Jakub> IA64 and PPC64 as well (only GCC would need changing,
Jakub> emitting .section .note.GNU-stack,"",@progbits at the end of
Jakub> every compile unit, ld takes care of the rest) exactly for
Jakub> uniformity's sake and because you get ld.so handling free.

I'm not following you on the "get ld.so handling free" part. How is
that handling free?

Jakub> GLIBC dynamic linker will take care of making the stack
Jakub> executable if say a binary which doesn't need executable
Jakub> stack depends on a shared library which needs executable
Jakub> stack or even dlopens a library which needs executable stack.

Actually, that's something that worries me. Somebody just needs to
succeed in loading any shared object with the right PT_GNU_STACK
header and then the entire program will be exposed to the risk of a
writable stack. On ia64, I just don't see any need to ever implicitly
turn on execute-permission on the stack, so why allow this extra
backdoor?

--david

2004-03-24 07:31:04

by Jakub Jelinek

[permalink] [raw]
Subject: Re: Non-Exec stack patches

On Tue, Mar 23, 2004 at 11:16:36PM -0800, David Mosberger wrote:
> I'm not following you on the "get ld.so handling free" part. How is
> that handling free?

What I meant is that it is already written and tested.

> Actually, that's something that worries me. Somebody just needs to
> succeed in loading any shared object with the right PT_GNU_STACK
> header and then the entire program will be exposed to the risk of a
> writable stack. On ia64, I just don't see any need to ever implicitly
> turn on execute-permission on the stack, so why allow this extra
> backdoor?

What kind of backdoor is it? If you dlopen untrusted shared libraries
into your program you have far bigger problem than executable
stack (you can execute any code it wants in its constructors).

If there is a shared library which needs executable stack for its use
(on !IA64 !PPC64 this is e.g. any library which takes address of
a nested function and passes it to some other function and/or stores
it into some variable which cannot be optimized out, on IA64 or PPC64
this is of course much rarer, but it is still possible some language
interpreter or something builds code on the fly on the stack).

Jakub

2004-03-24 07:45:19

by David Mosberger

[permalink] [raw]
Subject: Re: Non-Exec stack patches

>>>>> On Wed, 24 Mar 2004 02:28:40 -0500, Jakub Jelinek <[email protected]> said:

Jakub> On Tue, Mar 23, 2004 at 11:16:36PM -0800, David Mosberger
Jakub> wrote:

>> I'm not following you on the "get ld.so handling free" part. How
>> is that handling free?

Jakub> What I meant is that it is already written and tested.

OK. The ELF flags bit is even more free, then... ;-)

>> Actually, that's something that worries me. Somebody just needs
>> to succeed in loading any shared object with the right
>> PT_GNU_STACK header and then the entire program will be exposed
>> to the risk of a writable stack. On ia64, I just don't see any
>> need to ever implicitly turn on execute-permission on the stack,
>> so why allow this extra backdoor?

Jakub> What kind of backdoor is it? If you dlopen untrusted shared
Jakub> libraries into your program you have far bigger problem than
Jakub> executable stack (you can execute any code it wants in its
Jakub> constructors).

Sure. Theoretically, none of this matters at all (thanks to
mprotect()), so we're justs talking about raising the barrier. With
PT_GNU_STACK, it's sufficient to tweak one bit in any dependent
library, whereas with the ELF flag bit, you need to tweak one bit in
the main executable. Not a huge difference, I'll submit, but from an
admin perspective, I find it a lot easier to check the main program to
see if it has the "executable stack/data" bit set rather than all
dependent libraries. Additionally, we could easily choose to drop
support for the ELF flag bit altogether. The only program that I know
of that ever needed executable data was XFree86 and that was only due
to an oversight---and that has long been fixed.

Jakub> If there is a shared library which needs executable stack for
Jakub> its use (on !IA64 !PPC64 this is e.g. any library which takes
Jakub> address of a nested function and passes it to some other
Jakub> function and/or stores it into some variable which cannot be
Jakub> optimized out, on IA64 or PPC64 this is of course much rarer,

For sufficiently small values of "much rarer", I agree. ;-)

Jakub> but it is still possible some language interpreter or
Jakub> something builds code on the fly on the stack).

That's why there is mprotect().

--david

2004-03-24 09:42:15

by Andi Kleen

[permalink] [raw]
Subject: Re: Non-Exec stack patches

Andrew Morton <[email protected]> writes:

> Which architectures are currently making their pre-page execute permissions
> depend upon VM_EXEC?
> Would additional arch patches be needed for this?

Yes, they would need some straight forward minor patches e.g. in the
32bit emulation. IA64 would be a candidate I guess.

i386 could do it on NX capable CPUs with PAE kernels (but it would require
backporting some fixes from x86-64). However currently it doesn't make
much sense because all x86 CPUs that support NX (AMD K8 currently only)
support 64bit kernels and people can as well run 64bit kernels.

Doing it on i386 would only make sense if non 64bit capable CPUs ever get
NX. I heard VIA may be planning that, but so far there is nothing in their
shipping CPUs, so I guess we can skip that for now.

-Andi


2004-03-24 10:23:37

by Stefan Smietanowski

[permalink] [raw]
Subject: Re: Non-Exec stack patches

Hi Andi.

>>Which architectures are currently making their pre-page execute permissions
>>depend upon VM_EXEC?
>>Would additional arch patches be needed for this?
>
>
> Yes, they would need some straight forward minor patches e.g. in the
> 32bit emulation. IA64 would be a candidate I guess.
>
> i386 could do it on NX capable CPUs with PAE kernels (but it would require
> backporting some fixes from x86-64). However currently it doesn't make
> much sense because all x86 CPUs that support NX (AMD K8 currently only)
> support 64bit kernels and people can as well run 64bit kernels.
>
> Doing it on i386 would only make sense if non 64bit capable CPUs ever get
> NX. I heard VIA may be planning that, but so far there is nothing in their
> shipping CPUs, so I guess we can skip that for now.

Well, there's also the case that (unknown if rumour or confirmed) there
will be AthlonXPs based on the K8 core that do NOT run 64bit code.

I would THINK they would include the NX bit but that's just a guess of
course.

// Stefan

2004-03-24 10:55:28

by Kurt Garloff

[permalink] [raw]
Subject: Re: Non-Exec stack patches

Hi Andrew, Ingo,

On Tue, Mar 23, 2004 at 04:41:04PM -0800, Andrew Morton wrote:
> It gets rejects in arch/x86_64/ia32/ia32_binfmt.c and
> arch/ia64/ia32/binfmt_elf32.c - someone has been dinking with your
> put_dirty_page() prototype.

That's the anon_vma stuff from Andrea, which we have merged locally.
Sorry, I forgot that it touched these places.

> I dropped those bits.

Not a good idea, as stack protection won't work in ia32 emulation of
x86_64 and ia64 then.

I readded and did a clean diff against 2.6.5-rc2 and also integrated
your comments patch.

> And I added the missing bit:

Thx!

> Now, what should the kernel do if the executable requests EXSTACK_DISABLE_X
> but the kernel cannot do that? Is it not a bit misleading/dangerous to
> permit the executable to run anyway?

Yes. It has to ...
Otherwise, most binaries would not work in i386 any more :-/

Updated patch attached.
--
Kurt Garloff <[email protected]> Cologne, DE
SUSE LINUX AG, Nuernberg, DE SUSE Labs (Head)


Attachments:
(No filename) (0.00 B)
(No filename) (189.00 B)
Download all attachments

2004-03-24 11:27:42

by Andi Kleen

[permalink] [raw]
Subject: Re: Non-Exec stack patches

> Well, there's also the case that (unknown if rumour or confirmed) there
> will be AthlonXPs based on the K8 core that do NOT run 64bit code.

Yes, you're right. Some HP laptops ship with such crippled chips.

> I would THINK they would include the NX bit but that's just a guess of
> course.

Most likely yes.

But who buys such crippled CPUs will have to live with that. Or do a patch.
NX support is mostly hype anyways, it doesn't give you much advantage.

-Andi

2004-03-24 16:39:01

by John Reiser

[permalink] [raw]
Subject: Re: Non-Exec stack patches

Jakub> but it is still possible some language interpreter or
Jakub> something builds code on the fly on the stack).

David> That's why there is mprotect().

But mprotect() costs enough (hundreds of cycles) to be a significant burden
in some cases. Generating code to a stack region that is inherently
executable is inexpensive (even allowing for restrictive alignment and
avoiding I/D cache conflicts), is thread safe, is async-signal safe, and
takes less work than other alternatives. Yes, the "black hats" do this;
so do the "white hats." Please do not increase the minimum cost for
applications that want generate-and-execute on the stack at upredictable
high frequency.

--
John Reiser, [email protected]

2004-03-24 17:12:43

by David Mosberger

[permalink] [raw]
Subject: Re: Non-Exec stack patches

>>>>> On Wed, 24 Mar 2004 08:29:24 -0800, John Reiser <[email protected]> said:

Jakub> but it is still possible some language interpreter or
Jakub> something builds code on the fly on the stack).

David> That's why there is mprotect().

John> But mprotect() costs enough (hundreds of cycles) to be a
John> significant burden in some cases. Generating code to a stack
John> region that is inherently executable is inexpensive (even
John> allowing for restrictive alignment and avoiding I/D cache
John> conflicts), is thread safe, is async-signal safe, and takes
John> less work than other alternatives. Yes, the "black hats" do
John> this; so do the "white hats." Please do not increase the
John> minimum cost for applications that want generate-and-execute
John> on the stack at upredictable high frequency.

Huh? Only one mprotect() call is needed to make the entire stack
executable.

--david

2004-03-24 17:27:21

by Jakub Jelinek

[permalink] [raw]
Subject: Re: Non-Exec stack patches

On Wed, Mar 24, 2004 at 09:12:30AM -0800, David Mosberger wrote:
> David> That's why there is mprotect().
>
> John> But mprotect() costs enough (hundreds of cycles) to be a
> John> significant burden in some cases. Generating code to a stack
> John> region that is inherently executable is inexpensive (even
> John> allowing for restrictive alignment and avoiding I/D cache
> John> conflicts), is thread safe, is async-signal safe, and takes
> John> less work than other alternatives. Yes, the "black hats" do
> John> this; so do the "white hats." Please do not increase the
> John> minimum cost for applications that want generate-and-execute
> John> on the stack at upredictable high frequency.
>
> Huh? Only one mprotect() call is needed to make the entire stack
> executable.

Nope. Think about multithreaded apps. Furthermore, getting the exact
extents of the particular stack is difficult to find for applications,
but e.g. the threading library has to know such things.

Jakub

2004-03-24 17:49:26

by Andi Kleen

[permalink] [raw]
Subject: Re: Non-Exec stack patches

Jakub Jelinek <[email protected]> writes:
>
> Nope. Think about multithreaded apps. Furthermore, getting the exact
> extents of the particular stack is difficult to find for applications,
> but e.g. the threading library has to know such things.

It's actually not that difficult. You just have to read /proc/self/maps
and check for the mapping of your current stack pointer.
For the main stack GROWSDOWN will inherit the x or nx on growing
down.

And cache a flag about this in a TLS variable.

-Andi

2004-03-24 17:54:26

by Jakub Jelinek

[permalink] [raw]
Subject: Re: Non-Exec stack patches

On Wed, Mar 24, 2004 at 06:49:21PM +0100, Andi Kleen wrote:
> It's actually not that difficult. You just have to read /proc/self/maps
> and check for the mapping of your current stack pointer.
> For the main stack GROWSDOWN will inherit the x or nx on growing
> down.

That assumes there are always gaps around thread stacks, which doesn't
have to be true. You could make executable some other mapping
as well easily.

Jakub

2004-03-24 18:02:00

by David Mosberger

[permalink] [raw]
Subject: Re: Non-Exec stack patches

>>>>> On Wed, 24 Mar 2004 12:24:54 -0500, Jakub Jelinek <[email protected]> said:

>> Huh? Only one mprotect() call is needed to make the entire stack
>> executable.

Jakub> Nope. Think about multithreaded apps.

I said one mprotect() to make the entire stack executable. Obviously,
if you have multiple stacks, you need one call per stack. Big deal.

Jakub> Furthermore, getting the exact extents of the particular
Jakub> stack is difficult to find for applications, but e.g. the
Jakub> threading library has to know such things.

And how is this a kernel problem? There are other reasons why
applications need to know the stack extents (think garbage collector)
and it's entirely glibc's failing if it's difficult to determine the
stack extent.

--david

2004-03-24 19:09:06

by John Reiser

[permalink] [raw]
Subject: Re: Non-Exec stack patches

> Only one mprotect() call is needed to make the entire stack
> executable.

mprotect() only works on the portion that is currently allocated.
If the stack grows, then another call is needed. Tracking the high-
water mark is an expense. Forcing the allocation of the maximum
extent for the stack of the current thread, even to the same copy-
on-write all-zero page, can cause memory overcommit problems.

--
John Reiser, [email protected]

2004-03-24 19:18:12

by David Mosberger

[permalink] [raw]
Subject: Re: Non-Exec stack patches

>>>>> On Wed, 24 Mar 2004 11:02:45 -0800, John Reiser <[email protected]> said:

>> Only one mprotect() call is needed to make the entire stack
>> executable.

John> mprotect() only works on the portion that is currently allocated.
John> If the stack grows, then another call is needed.

No, mprotect() on the entire stack will mark the vm_area with the
desired protection and VM_GROWSDOWN/VM_GROWSUP will expand
automatically with the new protection. And if you want to expand the
stack in user-level, e.g., by intercepting SIGSEGV, you'll either do
an mmap() or mprotect() at any rate so there is zero extra cost there.

--david

2004-03-24 22:05:27

by Kurt Garloff

[permalink] [raw]
Subject: Re: Non-Exec stack patches

Hi Andi,

On Wed, Mar 24, 2004 at 12:27:39PM +0100, Andi Kleen wrote:
Stefan Smietanowski <[email protected]> wrote:
> > I would THINK they would include the NX bit but that's just a guess of
> > course.
>
> Most likely yes.
>
> But who buys such crippled CPUs will have to live with that. Or do a patch.

It should be straightforward. Put a different value in the
protection_map if such a CPU is detected. That should be all.

> NX support is mostly hype anyways, it doesn't give you much advantage.

It puts the hurdle somewhat higher and for some exploits does prevent
them. You can still overwrite the return address, but you need to have
put code into the data section prior to this to exploit. This may or may
not possible depending on how the daemon works. Marking the data section
non-executable would help further ...

It's always a tradeoff. Given the simplicity of the patch, I'm in favour
of having it.

Regards,
--
Kurt Garloff <[email protected]> [Koeln, DE]
Physics:Plasma modeling <[email protected]> [TU Eindhoven, NL]
Linux: SUSE Labs (Head) <[email protected]> [SUSE Nuernberg, DE]


Attachments:
(No filename) (1.12 kB)
(No filename) (189.00 B)
Download all attachments

2004-04-14 07:29:03

by Suresh Siddha

[permalink] [raw]
Subject: RE: Non-Exec stack patches

> -----Original Message-----
> From: [email protected]
> [mailto:[email protected]]On Behalf Of Andrew Morton
> Sent: Tuesday, March 23, 2004 4:41 PM
> To: Kurt Garloff
> Cc: [email protected]; [email protected]
> Subject: Re: Non-Exec stack patches
>
>
> Kurt Garloff <[email protected]> wrote:
> >
> > > Which architectures are currently making their pre-page
> execute permissions
> > > depend upon VM_EXEC? Would additional arch patches be
> needed for this?
> >
> > It works on AMD64 (not ia32e), both for 64bit and 32bit binaries.
> > I have not yet tested other archs.
> >
> > If the values in the protection_map are different depending
> on bit 2,
> > the patch will be effecitve. (OK, the CPU/MMU needs to honour the
> > setting of course.) Most likely, the values for
> > protection_map[7] is PAGE_COPY_EXEC and of protection_map[3] is
> > PAGE_COPY.
>
> OK.
>

Recent ia64 mm trees are broken because of this issue. Attached patch fixes protection_map[7] in IA64.

thanks,
suresh


Attachments:
noexec-ia64.fix (458.00 B)
noexec-ia64.fix

2004-04-14 08:24:31

by Jamie Lokier

[permalink] [raw]
Subject: Re: Non-Exec stack patches

Siddha, Suresh B wrote:
> Recent ia64 mm trees are broken because of this issue. Attached
> patch fixes protection_map[7] in IA64.

> --- linux-265mm5/include/asm-ia64/pgtable.h~ 2004-04-14 00:09:04.000000000 -0700
> +++ linux-265mm5/include/asm-ia64/pgtable.h 2004-04-13 23:45:29.000000000 -0700
> @@ -148,7 +148,7 @@
> #define __P100 __pgprot(__ACCESS_BITS | _PAGE_PL_3 | _PAGE_AR_X_RX)
> #define __P101 __pgprot(__ACCESS_BITS | _PAGE_PL_3 | _PAGE_AR_RX)
> #define __P110 PAGE_COPY
> -#define __P111 PAGE_COPY
> +#define __P111 PAGE_COPY_EXEC
>
> #define __S000 PAGE_NONE
> #define __S001 PAGE_READONLY

I'm looking at 2.6.5, and it doesn't define PAGE_COPY_EXEC. In 2.6.5,
asm-ia64/pgtable.h, PAGE_COPY is executable, and PAGE_READONLY is
non-executable.

Yes I know the naming is different from all the other asm-* files, but
that's what it says. All of those definitions have confused names on
ia64, although they seem to have the rights bits set in the end.

Has the meaning of PAGE_COPY in asm-ia64/pgtable.h changed in 2.6.5-mm5?

If so, you want to change __P110 as well as __P111.

-- Jamie

2004-04-14 08:36:22

by Jamie Lokier

[permalink] [raw]
Subject: PowerPC exec page protection

<asm-ppc/pgtable.h> and <asm-ppc64/pgtable.h> both define the
following map of protection bits:

#define __P000 PAGE_NONE
#define __P001 PAGE_READONLY_X
#define __P010 PAGE_COPY
#define __P011 PAGE_COPY_X
#define __P100 PAGE_READONLY
#define __P101 PAGE_READONLY_X
#define __P110 PAGE_COPY
#define __P111 PAGE_COPY_X

#define __S000 PAGE_NONE
#define __S001 PAGE_READONLY_X
#define __S010 PAGE_SHARED
#define __S011 PAGE_SHARED_X
#define __S100 PAGE_READONLY
#define __S101 PAGE_READONLY_X
#define __S110 PAGE_SHARED
#define __S111 PAGE_SHARED_X

The _X flags seem wrongly placed, as bit 2 is the PROT_EXEC bit, not
bit 0. Is the above intentional?

-- Jamie

2004-04-14 08:46:56

by Suresh Siddha

[permalink] [raw]
Subject: RE: Non-Exec stack patches

Jamie Lokier wrote:
> Has the meaning of PAGE_COPY in asm-ia64/pgtable.h changed in
> 2.6.5-mm5?

Yes. Kurt's recent patch for parsing PT_GNU_STACK section introduced
this change.

Here is the relevant hunk.
-#define PAGE_COPY __pgprot(__ACCESS_BITS | _PAGE_PL_3 |
_PAGE_AR_RX)
+#define PAGE_COPY __pgprot(__ACCESS_BITS | _PAGE_PL_3 |
_PAGE_AR_R)
+#define PAGE_COPY_EXEC __pgprot(__ACCESS_BITS | _PAGE_PL_3 |
_PAGE_AR_RX)

>
> If so, you want to change __P110 as well as __P111.

No. Only EXEC bit is the difference.

thanks,
suresh

2004-04-14 08:48:26

by Anton Blanchard

[permalink] [raw]
Subject: Re: PowerPC exec page protection


Hi,

> <asm-ppc/pgtable.h> and <asm-ppc64/pgtable.h> both define the
> following map of protection bits:
>
> #define __P000 PAGE_NONE
> #define __P001 PAGE_READONLY_X
> #define __P010 PAGE_COPY
> #define __P011 PAGE_COPY_X
> #define __P100 PAGE_READONLY
> #define __P101 PAGE_READONLY_X
> #define __P110 PAGE_COPY
> #define __P111 PAGE_COPY_X
>
> #define __S000 PAGE_NONE
> #define __S001 PAGE_READONLY_X
> #define __S010 PAGE_SHARED
> #define __S011 PAGE_SHARED_X
> #define __S100 PAGE_READONLY
> #define __S101 PAGE_READONLY_X
> #define __S110 PAGE_SHARED
> #define __S111 PAGE_SHARED_X
>
> The _X flags seem wrongly placed, as bit 2 is the PROT_EXEC bit, not
> bit 0. Is the above intentional?

Its backwards and we know it :) Ive got a patch to implement per page
execute on ppc64 and that did pop up.

Thanks for pointing it out, are you looking at ppc* page protection or
just chanced upon it?

Anton

2004-04-14 09:35:49

by Jamie Lokier

[permalink] [raw]
Subject: Re: PowerPC exec page protection

Anton Blanchard wrote:
> Thanks for pointing it out, are you looking at ppc* page protection or
> just chanced upon it?

Just chanced.

-- Jamie

2004-04-14 09:39:06

by Jamie Lokier

[permalink] [raw]
Subject: Re: Non-Exec stack patches

Siddha, Suresh B wrote:
> > If so, you want to change __P110 as well as __P111.
>
> No. Only EXEC bit is the difference.

Yes. __P110 means WRITE+EXEC. __P111 means READ+WRITE+EXEC.

-- Jamie

2004-04-14 09:47:23

by Jamie Lokier

[permalink] [raw]
Subject: Re: Non-Exec stack patches

People looking at PROT_EXEC page table flags might want to be aware
that <asm-um/pgtable.h> mimics the behaviour of i386: read implies and
is implied by exec, write implies read.

That might mean user-mode linux doesn't provide no-exec-stack
protection even when the underlying kernel does offer it. I'm not sure.

-- Jamie

2004-04-14 11:40:33

by Jamie Lokier

[permalink] [raw]
Subject: [PATCH] (IA64) Fix ugly __[PS]* macros in <asm-ia64/pgtable.h>

Patch: ia64-pgtable-2.6.5.patch

That mixture of PAGE_* and __pgprot() definitions for the __[PS]*
macros in asm-ia64/pgtable.h is really ugly and just makes the code
unnecessarily confusing:

#define __P000 PAGE_NONE
#define __P001 PAGE_READONLY
#define __P010 PAGE_READONLY
#define __P011 PAGE_READONLY
#define __P100 __pgprot(__ACCESS_BITS | _PAGE_PL_3 | _PAGE_AR_X_RX)
#define __P101 __pgprot(__ACCESS_BITS | _PAGE_PL_3 | _PAGE_AR_RX)
#define __P110 PAGE_COPY
#define __P111 PAGE_COPY

The PAGE_* macros which are used in __[PS]* aren't used anywhere else:
their entire reason for existing is to make the __[PS]* macros
clearer. It looks as though the people who implemented the IA-64 port
didn't realise that.

Here is a page (untested) which cleans up those definitions. It was
made from 2.6.5.

Enjoy,
-- Jamie

diff -ur orig-2.6.5/include/asm-ia64/pgtable.h ia64-2.6.5/include/asm-ia64/pgtable.h
--- orig-2.6.5/include/asm-ia64/pgtable.h 2004-04-12 21:45:39.000000000 +0100
+++ ia64-2.6.5/include/asm-ia64/pgtable.h 2004-04-14 12:29:01.000000000 +0100
@@ -116,13 +116,17 @@
* they are used, the page is accessed. They are cleared only by the
* page-out routines.
*/
-#define PAGE_NONE __pgprot(_PAGE_PROTNONE | _PAGE_A)
-#define PAGE_SHARED __pgprot(__ACCESS_BITS | _PAGE_PL_3 | _PAGE_AR_RW)
-#define PAGE_READONLY __pgprot(__ACCESS_BITS | _PAGE_PL_3 | _PAGE_AR_R)
-#define PAGE_COPY __pgprot(__ACCESS_BITS | _PAGE_PL_3 | _PAGE_AR_RX)
-#define PAGE_GATE __pgprot(__ACCESS_BITS | _PAGE_PL_0 | _PAGE_AR_X_RX)
-#define PAGE_KERNEL __pgprot(__DIRTY_BITS | _PAGE_PL_0 | _PAGE_AR_RWX)
-#define PAGE_KERNELRX __pgprot(__ACCESS_BITS | _PAGE_PL_0 | _PAGE_AR_RX)
+#define PAGE_NONE __pgprot(_PAGE_PROTNONE | _PAGE_A)
+#define PAGE_SHARED __pgprot(__ACCESS_BITS | _PAGE_PL_3 | _PAGE_AR_RW)
+#define PAGE_SHARED_EXEC __pgprot(__ACCESS_BITS | _PAGE_PL_3 | _PAGE_AR_RWX)
+#define PAGE_READONLY __pgprot(__ACCESS_BITS | _PAGE_PL_3 | _PAGE_AR_R)
+#define PAGE_READONLY_EXEC __pgprot(__ACCESS_BITS | _PAGE_PL_3 | _PAGE_AR_RX)
+#define PAGE_COPY __pgprot(__ACCESS_BITS | _PAGE_PL_3 | _PAGE_AR_R)
+#define PAGE_COPY_EXEC __pgprot(__ACCESS_BITS | _PAGE_PL_3 | _PAGE_AR_RX)
+#define PAGE_EXEC __pgprot(__ACCESS_BITS | _PAGE_PL_3 | _PAGE_AR_X_RX)
+#define PAGE_GATE __pgprot(__ACCESS_BITS | _PAGE_PL_0 | _PAGE_AR_X_RX)
+#define PAGE_KERNEL __pgprot(__DIRTY_BITS | _PAGE_PL_0 | _PAGE_AR_RWX)
+#define PAGE_KERNELRX __pgprot(__ACCESS_BITS | _PAGE_PL_0 | _PAGE_AR_RX)

# ifndef __ASSEMBLY__

@@ -142,21 +146,21 @@
/* xwr */
#define __P000 PAGE_NONE
#define __P001 PAGE_READONLY
-#define __P010 PAGE_READONLY /* write to priv pg -> copy & make writable */
-#define __P011 PAGE_READONLY /* ditto */
-#define __P100 __pgprot(__ACCESS_BITS | _PAGE_PL_3 | _PAGE_AR_X_RX)
-#define __P101 __pgprot(__ACCESS_BITS | _PAGE_PL_3 | _PAGE_AR_RX)
-#define __P110 PAGE_COPY
-#define __P111 PAGE_COPY
+#define __P010 PAGE_COPY
+#define __P011 PAGE_COPY
+#define __P100 PAGE_EXEC
+#define __P101 PAGE_READONLY_EXEC
+#define __P110 PAGE_COPY_EXEC
+#define __P111 PAGE_COPY_EXEC

#define __S000 PAGE_NONE
#define __S001 PAGE_READONLY
#define __S010 PAGE_SHARED /* we don't have (and don't need) write-only */
#define __S011 PAGE_SHARED
-#define __S100 __pgprot(__ACCESS_BITS | _PAGE_PL_3 | _PAGE_AR_X_RX)
-#define __S101 __pgprot(__ACCESS_BITS | _PAGE_PL_3 | _PAGE_AR_RX)
-#define __S110 __pgprot(__ACCESS_BITS | _PAGE_PL_3 | _PAGE_AR_RWX)
-#define __S111 __pgprot(__ACCESS_BITS | _PAGE_PL_3 | _PAGE_AR_RWX)
+#define __S100 PAGE_EXEC
+#define __S101 PAGE_READONLY_EXEC
+#define __S110 PAGE_SHARED_EXEC
+#define __S111 PAGE_SHARED_EXEC

#define pgd_ERROR(e) printk("%s:%d: bad pgd %016lx.\n", __FILE__, __LINE__, pgd_val(e))
#define pmd_ERROR(e) printk("%s:%d: bad pmd %016lx.\n", __FILE__, __LINE__, pmd_val(e))

2004-04-14 16:07:37

by David Mosberger

[permalink] [raw]
Subject: Re: [PATCH] (IA64) Fix ugly __[PS]* macros in <asm-ia64/pgtable.h>



Jamie> Patch: ia64-pgtable-2.6.5.patch
Jamie> That mixture of PAGE_* and __pgprot() definitions for the __[PS]*
Jamie> macros in asm-ia64/pgtable.h is really ugly and just makes the code
Jamie> unnecessarily confusing:

Jamie> #define __P000 PAGE_NONE
Jamie> #define __P001 PAGE_READONLY
Jamie> #define __P010 PAGE_READONLY
Jamie> #define __P011 PAGE_READONLY
Jamie> #define __P100 __pgprot(__ACCESS_BITS | _PAGE_PL_3 | _PAGE_AR_X_RX)
Jamie> #define __P101 __pgprot(__ACCESS_BITS | _PAGE_PL_3 | _PAGE_AR_RX)
Jamie> #define __P110 PAGE_COPY
Jamie> #define __P111 PAGE_COPY

Jamie> The PAGE_* macros which are used in __[PS]* aren't used
Jamie> anywhere else: their entire reason for existing is to make the
Jamie> __[PS]* macros clearer. It looks as though the people who
Jamie> implemented the IA-64 port didn't realise that.

Huh? You haven't actually checked, have you?
I don't pollute namespace for no good reason.

Jamie> Here is a page (untested) which cleans up those definitions.
Jamie> It was made from 2.6.5.

If the same names are adopted by the other platforms, I'm fine with it.
Otherwise, see my comment above.

--david

2004-04-14 18:30:35

by Kurt Garloff

[permalink] [raw]
Subject: Re: Non-Exec stack patches

On Wed, Apr 14, 2004 at 10:47:02AM +0100, Jamie Lokier wrote:
> People looking at PROT_EXEC page table flags might want to be aware
> that <asm-um/pgtable.h> mimics the behaviour of i386: read implies and
> is implied by exec, write implies read.
>
> That might mean user-mode linux doesn't provide no-exec-stack
> protection even when the underlying kernel does offer it. I'm not sure.

I thought UML only runs on i386.
And on i386, you have no NX feature.
You can run i386 UML on AMD64 (with 64bit kernel) though.

Regards,
--
Kurt Garloff <[email protected]> Cologne, DE
SUSE LINUX AG, Nuernberg, DE SUSE Labs (Head)


Attachments:
(No filename) (675.00 B)
(No filename) (189.00 B)
Download all attachments

2004-04-14 18:37:12

by Kurt Garloff

[permalink] [raw]
Subject: Re: RE: Non-Exec stack patches

On Wed, Apr 14, 2004 at 12:28:24AM -0700, Suresh B Siddha wrote:
> Recent ia64 mm trees are broken because of this issue. Attached patch
> fixes protection_map[7] in IA64.

Ah, sorry. ia64 seems to be strangely different here.
My understanding is that it support NX page protection. And that you
don't have executable stacks in the ia64 ABI at all. But for i386
emulation you should be able to enable and disable executable stacks.
For some reason the needed defines are missing ...

Regards,
--
Kurt Garloff <[email protected]> Cologne, DE
SUSE LINUX AG, Nuernberg, DE SUSE Labs (Head)


Attachments:
(No filename) (641.00 B)
(No filename) (189.00 B)
Download all attachments

2004-04-14 18:48:21

by Jamie Lokier

[permalink] [raw]
Subject: Re: [PATCH] (IA64) Fix ugly __[PS]* macros in <asm-ia64/pgtable.h>

David Mosberger wrote:
> Huh? You haven't actually checked, have you?

Yes I have. Quite thoroughly.

> I don't pollute namespace for no good reason.

In this instance, there is a certain amount of arch variation. parisc
defines PAGE_EXECREAD and PAGE_RWX. ppc defines PAGE_COPY_X,
PAGE_SHARED_X, PAGE_READONLY_X (X is for exec). m68k defines
PAGE_COPY_C, PAGE_READONLY_C, PAGE_SHARED_C. x86_64 defines
PAGE_COPY_EXEC, PAGE_SHARED_EXEC and PAGE_READONLY_EXEC.

Those are all arch-private names, some of them used in code in arch/*,
some just to define the __[PS]* constants.

> Jamie> Here is a page (untested) which cleans up those definitions.
> Jamie> It was made from 2.6.5.
>
> If the same names are adopted by the other platforms, I'm fine with it.
> Otherwise, see my comment above.

I copied <asm-x86_64/pgtable.h>, which closely matches ia64, except
for PAGE_EXEC which I named because nothing else has it. That name
isn't used anywhere. On reflection, a better name for it is
PAGE_EXECONLY (like PAGE_READONLY).

In theory the Alpha can do exec-only pages, but it's __[PS]* map
always gives read permission when there's execute permission. I'm not
sure if there's a reason for that, or if it just historically copied
the i386 behaviour (Alpha was the first port).

The constants PAGE_KERNEL and PAGE_READONLY are used in
arch-independent code with a common meaning. Otherwise, the constants
are used only to defined __[PS]* and a few in arch-dependent
initialisation code.

I agree it is best to avoid namespace pollution. However this is one
area where ia64 sticks out because it's approach is different from
other ports. All of them except Alpha used PAGE_* names to clarify
the __[PS]* map, defining additional names as needed.

The Alpha is quite clean in a different way, or looks like it until
you realise the _PAGE_P() macro is equivalent to identity so just
obfuscates the code. (The _PAGE_P() macro which is absurd because
it's a complicated expression that's equivalent to identity).

The thing I don't like about the ia64 file is the mixing of two
different styles of definition in the same table. When I had to read
all the arch pgtable.h files to discover what is Linux's mmap()
behaviour on each one, ia64 stood out awkwardly.

-- Jamie

2004-04-14 19:03:11

by David Mosberger

[permalink] [raw]
Subject: Re: [PATCH] (IA64) Fix ugly __[PS]* macros in <asm-ia64/pgtable.h>

>>>>> On Wed, 14 Apr 2004 19:46:03 +0100, Jamie Lokier <[email protected]> said:

Jamie> David Mosberger wrote:
>> Huh? You haven't actually checked, have you?

Jamie> Yes I have. Quite thoroughly.

Then you should have noticed that drivers/char/mem.c is using PAGE_COPY.
Various architecture-dependent code is also using PAGE_foo macros.

Jamie> In theory the Alpha can do exec-only pages, but it's __[PS]*
Jamie> map always gives read permission when there's execute
Jamie> permission. I'm not sure if there's a reason for that, or if
Jamie> it just historically copied the i386 behaviour (Alpha was the
Jamie> first port).

I know why: back in those days, GCC emitted code for nested C
functions that assumed an executable stack. Also, Linus wasn't
terribly eager to turn off execute-permission on data/stacks. Even on
ia64 we started out that way, until I saw the error in my ways.

Jamie> I agree it is best to avoid namespace pollution. However
Jamie> this is one area where ia64 sticks out because it's approach
Jamie> is different from other ports. All of them except Alpha used
Jamie> PAGE_* names to clarify the __[PS]* map, defining additional
Jamie> names as needed.

The reality is that whenever you introduce a globally visible name
that is not used on x86, there is a very definite risk that someone
will use that same name and cause a conflict. We have had that happen
several times and that's the reason I'm normally religious about
prefixing all ia64-specific names with a "ia64_" or "IA64_" (yes, this
makes code a bit uglier, but you can't have it both ways). Your
argument that the Alpha and other ports are doing something different
doesn't buy me anything. If the ia64 break builds, the Alpha
maintainer won't fix it up for me, after all.

--david

2004-04-14 19:15:08

by Suresh Siddha

[permalink] [raw]
Subject: RE: Non-Exec stack patches

Jamie Lokier wrote:
> Siddha, Suresh B wrote:
> > > If so, you want to change __P110 as well as __P111.
> >
> > No. Only EXEC bit is the difference.
>
> Yes. __P110 means WRITE+EXEC. __P111 means READ+WRITE+EXEC.
>

Thanks Jamie. Attached the updated patch. Andrew please apply.

thanks,
suresh


Attachments:
noexec-ia64.fix (531.00 B)
noexec-ia64.fix

2004-04-14 19:16:30

by Jamie Lokier

[permalink] [raw]
Subject: Re: [PATCH] (IA64) Fix ugly __[PS]* macros in <asm-ia64/pgtable.h>

David Mosberger wrote:
> If the ia64 break builds, the Alpha maintainer won't fix it up for
> me, after all.

Ok. In this case PAGE_COPY_EXEC, PAGE_SHARED_EXEC and
PAGE_READONLY_EXEC are in x86_64, so those names are fairly safe.

You could write the other one as IA64_PAGE_EXECONLY to be very safe.

-- Jamie

2004-04-14 19:30:59

by Jamie Lokier

[permalink] [raw]
Subject: Re: [PATCH] (IA64) Fix ugly __[PS]* macros in <asm-ia64/pgtable.h>

David Mosberger wrote:
> Jamie> Yes I have. Quite thoroughly.
>
> Then you should have noticed that drivers/char/mem.c is using PAGE_COPY.

That's an interesting bug in drivers/char/mem.c. PAGE_COPY is wrong,
for archs with separate execute permission or write-only mappings.

The correct argument in drivers/char/mem.c should be vma->vm_page_prot.

> Jamie> In theory the Alpha can do exec-only pages, but it's __[PS]*
> Jamie> map always gives read permission when there's execute
> Jamie> permission. I'm not sure if there's a reason for that, or if
> Jamie> it just historically copied the i386 behaviour (Alpha was the
> Jamie> first port).
>
> I know why: back in those days, GCC emitted code for nested C
> functions that assumed an executable stack. Also, Linus wasn't
> terribly eager to turn off execute-permission on data/stacks. Even on
> ia64 we started out that way, until I saw the error in my ways.

We're both wrong. I misread the Alpha code: it does have exec-only
pages. What it doesn't have is write-only. And exec-only pages
aren't relevant to GCC's requirements, which used to be
read-implies-exec (exec-only breaks exec-implies-read).

-- Jamie

2004-04-14 20:05:43

by David Mosberger

[permalink] [raw]
Subject: Re: [PATCH] (IA64) Fix ugly __[PS]* macros in <asm-ia64/pgtable.h>

>>>>> On Wed, 14 Apr 2004 20:28:44 +0100, Jamie Lokier <[email protected]> said:

>> I know why: back in those days, GCC emitted code for nested C
>> functions that assumed an executable stack. Also, Linus wasn't
>> terribly eager to turn off execute-permission on data/stacks.
>> Even on ia64 we started out that way, until I saw the error in my
>> ways.

Jamie> We're both wrong.

No, Alpha Linux didn't map data without execute permission.

Jamie> What it doesn't have is write-only.

Yes, that one is because earlier Alphas couldn't do subword stores, so
WRITE-permission necessitated READ-permission.

--david

2004-04-14 20:15:23

by Jeff Dike

[permalink] [raw]
Subject: Re: Non-Exec stack patches

[email protected] said:
> I thought UML only runs on i386. And on i386, you have no NX feature.
> You can run i386 UML on AMD64 (with 64bit kernel) though.

It also runs natively on and64 (i.e. a 64 bit UML). The UML page table stuff
is going to need to be fixed for NX, but there's no reason that it won't work.

Jeff

2004-04-14 21:08:17

by Jamie Lokier

[permalink] [raw]
Subject: Re: [PATCH] (IA64) Fix ugly __[PS]* macros in <asm-ia64/pgtable.h>

David Mosberger wrote:
> No, Alpha Linux didn't map data without execute permission.

That was true from Linux 1.1.67 (when Alpha was introduced) to 1.1.84
(when __[PS]* was introduced). I'm not sure the Alpha target even
worked during those versions. Since Linux 1.1.84, it has mapped pages
on the Alpha without execute permission: the _PAGE_FOE (fault on exec)
bit is set for mappings which don't have PROT_EXEC.

Btw, they used PAGE_EXECONLY in those days :)

-- Jamie

2004-04-14 23:13:20

by David Mosberger

[permalink] [raw]
Subject: Re: [PATCH] (IA64) Fix ugly __[PS]* macros in <asm-ia64/pgtable.h>

>>>>> On Wed, 14 Apr 2004 22:05:38 +0100, Jamie Lokier <[email protected]> said:

Jamie> David Mosberger wrote:

>> No, Alpha Linux didn't map data without execute permission.

Jamie> That was true from Linux 1.1.67 (when Alpha was introduced)
Jamie> to 1.1.84 (when __[PS]* was introduced). I'm not sure the
Jamie> Alpha target even worked during those versions. Since Linux
Jamie> 1.1.84, it has mapped pages on the Alpha without execute
Jamie> permission: the _PAGE_FOE (fault on exec) bit is set for
Jamie> mappings which don't have PROT_EXEC.

$ uname -a
Linux hostname 2.2.20 #2 Wed Mar 20 19:57:28 EST 2002 alpha GNU/Linux
$ cat /proc/self/maps | grep cat
0000000120000000-0000000120006000 r-xp 0000000000000000 08:01 309740 /bin/cat
0000000120014000-0000000120016000 rwxp 0000000000004000 08:01 309740 /bin/cat

As you can see, the data-segment is mapped with EXEC bit turned on.
Ditto for the stack segment, which I think is this one:

000000011fffe000-0000000120000000 rwxp 0000000000000000 00:00 0

--david

2004-04-15 15:28:40

by Jamie Lokier

[permalink] [raw]
Subject: Re: [PATCH] (IA64) Fix ugly __[PS]* macros in <asm-ia64/pgtable.h>

David Mosberger wrote:
> >> No, Alpha Linux didn't map data without execute permission.
>
> Jamie> That was true from Linux 1.1.67 (when Alpha was introduced)
> Jamie> to 1.1.84 (when __[PS]* was introduced). I'm not sure the
> Jamie> Alpha target even worked during those versions. Since Linux
> Jamie> 1.1.84, it has mapped pages on the Alpha without execute
> Jamie> permission: the _PAGE_FOE (fault on exec) bit is set for
> Jamie> mappings which don't have PROT_EXEC.
>
> $ uname -a
> Linux hostname 2.2.20 #2 Wed Mar 20 19:57:28 EST 2002 alpha GNU/Linux
> $ cat /proc/self/maps | grep cat
> 0000000120000000-0000000120006000 r-xp 0000000000000000 08:01 309740 /bin/cat
> 0000000120014000-0000000120016000 rwxp 0000000000004000 08:01 309740 /bin/cat
>
> As you can see, the data-segment is mapped with EXEC bit turned on.
> Ditto for the stack segment, which I think is this one:
>
> 000000011fffe000-0000000120000000 rwxp 0000000000000000 00:00 0

Lest we get more wires crossed, the "x" in /proc/self/maps indicates
that userspace _requested_ executable mapping with PROT_EXEC.

It is independent of whether the kernel and hardware can grant a
non-executable mapping. On my Athlon:

08048000-0804c000 r-xp 00000000 03:02 95153 /bin/cat
0804c000-0804d000 rw-p 00003000 03:02 95153 /bin/cat
bfffe000-c0000000 rwxp fffff000 00:00 0

The stack is mapped executable, and the data segment is mapped
non-executable, according to /proc/self/maps which reflects the
PROT_EXEC request. But in fact because of hardware limitations,
despite the "rw-p" my data segment is actually executable.

If you map a segment without PROT_EXEC on Alpha, using a kernel newer
than 1.1.84, you'll get a non-executable mapping. That's what I mean
when I say the Alpha maps data without executable permission. The ELF
loader appears to ask for exec permission anyway, which is another
thing entirely.

-- Jamie

2004-04-15 17:47:52

by David Mosberger

[permalink] [raw]
Subject: Re: [PATCH] (IA64) Fix ugly __[PS]* macros in <asm-ia64/pgtable.h>

>>>>> On Thu, 15 Apr 2004 16:26:00 +0100, Jamie Lokier <[email protected]> said:

>> As you can see, the data-segment is mapped with EXEC bit turned on.
>> Ditto for the stack segment, which I think is this one:

>> 000000011fffe000-0000000120000000 rwxp 0000000000000000 00:00 0

Jamie> The stack is mapped executable, and the data segment is mapped
Jamie> non-executable, according to /proc/self/maps which reflects the
Jamie> PROT_EXEC request. But in fact because of hardware limitations,
Jamie> despite the "rw-p" my data segment is actually executable.

Yes, but Alpha doesn't have this limitation.

Jamie> If you map a segment without PROT_EXEC on Alpha, using a kernel newer
Jamie> than 1.1.84, you'll get a non-executable mapping.

Sure.

Jamie> That's what I mean when I say the Alpha maps data without
Jamie> executable permission. The ELF loader appears to ask for
Jamie> exec permission anyway, which is another thing entirely.

It's not the ELF loader, it's the kernel. See VM_DATA_DEFAULT_FLAGS
in asm-alpha/page.h.

--david