Non-Exec stack patches

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

thanks,
suresh

<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

Hi,

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

Just chanced.

-- Jamie

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

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

Yes I have. Quite thoroughly.

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.

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

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

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

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.

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

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

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

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

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

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

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

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)

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

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)