ELF: warn if process starts with executable stack

PT_GNU_STACK is fail open design, at least warn people that something isn't right.

Signed-off-by Alexey Dobriyan <[email protected]>

---

  fs/exec.c |    7 +++++++
  1 file changed, 7 insertions(+)

fs/exec.c +7 -0

+#ifdef CONFIG_MMU
+	if (vm_flags & VM_EXEC) {
+		pr_warn_once("process '%s'/%u started with executable stack\n",
+			     current->comm, current->pid);
+	}
+#endif
+
  	/* Move stack pages down in memory. */
  	if (stack_shift) {
  		ret = shift_arg_pages(vma, stack_shift);

PT_GNU_STACK is fail open design,

at least warn people that something
isn't right.

...

fs/exec.c +6 -0

@@ -762,6 +762,13 @@ int setup_arg_pages(struct linux_binprm *bprm,
 		goto out_unlock;
 	BUG_ON(prev != vma);
 
+#ifdef CONFIG_MMU
+	if (vm_flags & VM_EXEC) {
+		pr_warn_once("process '%s'/%u started with executable stack\n",
+			     current->comm, current->pid);
+	}
+#endif
 
 	/* Move stack pages down in memory. */
 	if (stack_shift) {
 		ret = shift_arg_pages(vma, stack_shift);

Quoted 1 line · On Mon, 18 Nov 2019 17:51:15 +0300 Alexey Dobriyan <[email protected]> wrote:

PT_GNU_STACK is fail open design,

Not sure what this means. Please expand on the motivation for this change.

Quoted 2 lines

at least warn people that something
isn't right.

People who use an executable stack get a kernel splat. How is that useful?

fs/exec.c int setup_arg_pages(struct linux_binprm *bprm, · line 765

Quoted 7 lines

fs/exec.c +1 -0

@@ -762,6 +762,13 @@ int setup_arg_pages(struct linux_binprm *bprm,
 		goto out_unlock;
 	BUG_ON(prev != vma);
 
+#ifdef CONFIG_MMU

There were few episodes of silent downgrade to an executable stack:

1) linking innocent looking assembly file

	$ cat f.S
	.intel_syntax noprefix
	.text
	.globl f
	f:
	        ret

	$ cat main.c
	void f(void);
	int main(void)
	{
	        f();
	        return 0;
	}

	$ gcc main.c f.S
	$ readelf -l ./a.out
	  GNU_STACK      0x0000000000000000 0x0000000000000000 0x0000000000000000
                         0x0000000000000000 0x0000000000000000  RWE    0x10

2) converting C99 nested function into a closure
https://nullprogram.com/blog/2019/11/15/

	void intsort2(int *base, size_t nmemb, _Bool invert)
	{
	    int cmp(const void *a, const void *b)
	    {
	        int r = *(int *)a - *(int *)b;
	        return invert ? -r : r;
	    }
	    qsort(base, nmemb, sizeof(*base), cmp);
	}

will silently require stack trampolines while non-closure version will not.

While without a double this behaviour is documented somewhere, add a warning

so that developers and users can at least notice. After so many years of x86_64 having proper executable stack support it should not cause too much problems.

If the system is old or CPU is old, then there will be an early warning against init and/or support personnel will write that "uh-oh, our Enterprise Software absolutely requires executable stack" and close tickets and customers will nod heads and life moves on.

There were few episodes of silent downgrade to an executable stack:

1) linking innocent looking assembly file

	$ cat f.S
	.intel_syntax noprefix
	.text
	.globl f
	f:
	        ret

	$ cat main.c
	void f(void);
	int main(void)
	{
	        f();
	        return 0;
	}

	$ gcc main.c f.S
	$ readelf -l ./a.out
	  GNU_STACK      0x0000000000000000 0x0000000000000000 0x0000000000000000
                         0x0000000000000000 0x0000000000000000  RWE    0x10

2) converting C99 nested function into a closure
https://nullprogram.com/blog/2019/11/15/

	void intsort2(int *base, size_t nmemb, _Bool invert)
	{
	    int cmp(const void *a, const void *b)
	    {
	        int r = *(int *)a - *(int *)b;
	        return invert ? -r : r;
	    }
	    qsort(base, nmemb, sizeof(*base), cmp);
	}

will silently require stack trampolines while non-closure version will not.

While without a double this behaviour is documented somewhere, add a warning so that developers and users can at least notice. After so many years of x86_64 having proper executable stack support it should not cause too much problems.

If the system is old or CPU is old, then there will be an early warning against init and/or support personnel will write that "uh-oh, our Enterprise Software absolutely requires executable stack" and close tickets and customers will nod heads and life moves on.

Signed-off-by Alexey Dobriyan <[email protected]>

---

 fs/exec.c |    5 +++++
 1 file changed, 5 insertions(+)

Quoted 60 lines · Alexey Dobriyan

There were few episodes of silent downgrade to an executable stack:

1) linking innocent looking assembly file

	$ cat f.S
	.intel_syntax noprefix
	.text
	.globl f
	f:
	        ret

	$ cat main.c
	void f(void);
	int main(void)
	{
	        f();
	        return 0;
	}

	$ gcc main.c f.S
	$ readelf -l ./a.out
	  GNU_STACK      0x0000000000000000 0x0000000000000000 0x0000000000000000
                         0x0000000000000000 0x0000000000000000  RWE    0x10

2) converting C99 nested function into a closure
https://nullprogram.com/blog/2019/11/15/

	void intsort2(int *base, size_t nmemb, _Bool invert)
	{
	    int cmp(const void *a, const void *b)
	    {
	        int r = *(int *)a - *(int *)b;
	        return invert ? -r : r;
	    }
	    qsort(base, nmemb, sizeof(*base), cmp);
	}

will silently require stack trampolines while non-closure version will not.

While without a double this behaviour is documented somewhere, add a warning so that developers and users can at least notice. After so many years of x86_64 having proper executable stack support it should not cause too much problems.

If the system is old or CPU is old, then there will be an early warning against init and/or support personnel will write that "uh-oh, our Enterprise Software absolutely requires executable stack" and close tickets and customers will nod heads and life moves on.

Signed-off-by Alexey Dobriyan <[email protected]>

---

 fs/exec.c |    5 +++++
 1 file changed, 5 insertions(+)

fs/exec.c +0 -0

@@ -762,6 +762,11 @@ int setup_arg_pages(struct linux_binprm *bprm,
 		goto out_unlock;
 	BUG_ON(prev != vma);
 

fs/exec.c int setup_arg_pages(struct linux_binprm *bprm, · line 768

Quoted 4 lines

fs/exec.c +4 -0

+	if (vm_flags & VM_EXEC) {
+		pr_warn_once("process '%s'/%u started with executable stack\n",
+			     current->comm, current->pid);
+	}

Given that this is triggerable by userspace, is there a concern about PID namespaces here?

Quoted 61 lines · Will Deacon

Quoted 60 lines · Alexey Dobriyan

There were few episodes of silent downgrade to an executable stack:

1) linking innocent looking assembly file

	$ cat f.S
	.intel_syntax noprefix
	.text
	.globl f
	f:
	        ret

	$ cat main.c
	void f(void);
	int main(void)
	{
	        f();
	        return 0;
	}

	$ gcc main.c f.S
	$ readelf -l ./a.out
	  GNU_STACK      0x0000000000000000 0x0000000000000000 0x0000000000000000
                         0x0000000000000000 0x0000000000000000  RWE    0x10

2) converting C99 nested function into a closure
https://nullprogram.com/blog/2019/11/15/

	void intsort2(int *base, size_t nmemb, _Bool invert)
	{
	    int cmp(const void *a, const void *b)
	    {
	        int r = *(int *)a - *(int *)b;
	        return invert ? -r : r;
	    }
	    qsort(base, nmemb, sizeof(*base), cmp);
	}

will silently require stack trampolines while non-closure version will not.

While without a double this behaviour is documented somewhere, add a warning so that developers and users can at least notice. After so many years of x86_64 having proper executable stack support it should not cause too much problems.

If the system is old or CPU is old, then there will be an early warning against init and/or support personnel will write that "uh-oh, our Enterprise Software absolutely requires executable stack" and close tickets and customers will nod heads and life moves on.

Signed-off-by Alexey Dobriyan <[email protected]>

---

 fs/exec.c |    5 +++++
 1 file changed, 5 insertions(+)

fs/exec.c +0 -0

@@ -762,6 +762,11 @@ int setup_arg_pages(struct linux_binprm *bprm,
 		goto out_unlock;
 	BUG_ON(prev != vma);
 

Quoted 7 lines

fs/exec.c int setup_arg_pages(struct linux_binprm *bprm, · line 768

Quoted 4 lines

fs/exec.c +4 -0

+	if (vm_flags & VM_EXEC) {
+		pr_warn_once("process '%s'/%u started with executable stack\n",
+			     current->comm, current->pid);
+	}

Given that this is triggerable by userspace, is there a concern about PID namespaces here?

In what sense? Are you thinking about the printing of the pid?

Pretty much by fiat and by definition the kernel log always print things in the initial pid namespace. Which this printk does.

Quoted 71 lines · Eric W. Biederman

Quoted 68 lines · Will Deacon

Quoted 57 lines · Alexey Dobriyan

There were few episodes of silent downgrade to an executable stack:

1) linking innocent looking assembly file

	$ cat f.S
	.intel_syntax noprefix
	.text
	.globl f
	f:
	        ret

	$ cat main.c
	void f(void);
	int main(void)
	{
	        f();
	        return 0;
	}

	$ gcc main.c f.S
	$ readelf -l ./a.out
	  GNU_STACK      0x0000000000000000 0x0000000000000000 0x0000000000000000
                         0x0000000000000000 0x0000000000000000  RWE    0x10

2) converting C99 nested function into a closure
https://nullprogram.com/blog/2019/11/15/

	void intsort2(int *base, size_t nmemb, _Bool invert)
	{
	    int cmp(const void *a, const void *b)
	    {
	        int r = *(int *)a - *(int *)b;
	        return invert ? -r : r;
	    }
	    qsort(base, nmemb, sizeof(*base), cmp);
	}

will silently require stack trampolines while non-closure version will not.

While without a double this behaviour is documented somewhere, add a warning so that developers and users can at least notice. After so many years of x86_64 having proper executable stack support it should not cause too much problems.

If the system is old or CPU is old, then there will be an early warning against init and/or support personnel will write that "uh-oh, our Enterprise Software absolutely requires executable stack" and close tickets and customers will nod heads and life moves on.

Signed-off-by Alexey Dobriyan <[email protected]>

---

 fs/exec.c |    5 +++++
 1 file changed, 5 insertions(+)

fs/exec.c int setup_arg_pages(struct linux_binprm *bprm, · line 768

Quoted 7 lines

fs/exec.c +4 -0

@@ -762,6 +762,11 @@ int setup_arg_pages(struct linux_binprm *bprm,
 		goto out_unlock;
 	BUG_ON(prev != vma);
 
+	if (vm_flags & VM_EXEC) {
+		pr_warn_once("process '%s'/%u started with executable stack\n",
+			     current->comm, current->pid);
+	}

Given that this is triggerable by userspace, is there a concern about PID namespaces here?

Quoted 4 lines

In what sense? Are you thinking about the printing of the pid?

Pretty much by fiat and by definition the kernel log always print things in the initial pid namespace. Which this printk does.

Ok, fair enough. Just wanted to make sure it was ok, since we're not using a task_pid_nr*() accessor and it might have been overlooked.