arch_align_stack() seems useless

arch_align_stack() seems useless

[Franck Bui-Huu]

Hello folks,

I recently pick up the implementation of arch_align_stack() from x86
architectures to make it available for mips.

But now I just realised that this function seems useless because of
the way it's used.

Currently, this function seems to be only used to randomize the stack
pointer inside a page during an execve() syscall. Only i386, x86_64
and recently mips do that. Here is the code taken from exec.c which
calls it:

	int setup_arg_pages(struct linux_binprm *bprm,
			    unsigned long stack_top,
			    int executable_stack)
	{
		[...]
	
		stack_top = arch_align_stack(stack_top);
		stack_top = PAGE_ALIGN(stack_top);
	
		[...]
	}

Since PAGE_ALIGN() is called right after arch_align_stack(), it seems
to me that the call to the latter function is useless...

Am I missing something ?

thanks

		Franck

Re: arch_align_stack() seems useless

[Arjan van de Ven]

On Mon, 27 Aug 2007 16:08:31 +0200
Franck Bui-Huu <vagabon.xyz@gmail.com> wrote:
> and recently mips do that. Here is the code taken from exec.c which
> calls it:
> 
> 	int setup_arg_pages(struct linux_binprm *bprm,
> 			    unsigned long stack_top,
> 			    int executable_stack)
> 	{
> 		[...]
> 	
> 		stack_top = arch_align_stack(stack_top);
> 		stack_top = PAGE_ALIGN(stack_top);
> 	
> 		[...]
> 	}
> 
> Since PAGE_ALIGN() is called right after arch_align_stack(), it seems
> to me that the call to the latter function is useless...
> 
> Am I missing something ?


arch_align_stack aligns, on x86, within a 2 page range (this is for
cache coloring).  The other thing you missed is that arch_align_stack()
is called in 2 locations, binfmt_elf.c is the primary location for
inside-the-page randomization.

Re: arch_align_stack() seems useless

[Franck Bui-Huu]

Hello Arjan,

Arjan van de Ven wrote:
> arch_align_stack aligns, on x86, within a 2 page range (this is for
> cache coloring).

OK, but for elf case this seems useless since the top of the stack is
already randomized.

It seems that the randomization stuff (top of the stack + stack
pointer inside a page) belongs to the elf binary format whereas it
could have been part of exec.c. Are there any reasons ?

> The other thing you missed is that arch_align_stack()
> is called in 2 locations, binfmt_elf.c is the primary location for
> inside-the-page randomization.
> 

Well not really because for mips case, we have:

	$ git grep ELF_PLATFORM include/asm-mips
	include/asm-mips/elf.h:#define ELF_PLATFORM  (NULL)

So on mips, the stack pointer won't get the inside the page
randomization. Is that correct ?

If so, I'm wondering why this randomization must depend on that string
to be defined. I must admit that I'm not sure how it's used. I guess
it's used by ld.so and it could be set to "mips" for now...

thanks
		Franck

Re: arch_align_stack() seems useless

[Franck Bui-Huu]

Arjan,

Franck Bui-Huu wrote:
> Arjan van de Ven wrote:
>> arch_align_stack aligns, on x86, within a 2 page range (this is for
>> cache coloring).
> 
> OK, but for elf case this seems useless since the top of the stack is
> already randomized.
> 
> It seems that the randomization stuff (top of the stack + stack
> pointer inside a page) belongs to the elf binary format whereas it
> could have been part of exec.c. Are there any reasons ?
> 
>> The other thing you missed is that arch_align_stack()
>> is called in 2 locations, binfmt_elf.c is the primary location for
>> inside-the-page randomization.
>>
> 
> Well not really because for mips case, we have:
> 
> 	$ git grep ELF_PLATFORM include/asm-mips
> 	include/asm-mips/elf.h:#define ELF_PLATFORM  (NULL)
> 
> So on mips, the stack pointer won't get the inside the page
> randomization. Is that correct ?
> 
> If so, I'm wondering why this randomization must depend on that string
> to be defined. I must admit that I'm not sure how it's used. I guess
> it's used by ld.so and it could be set to "mips" for now...
> 

Do you think that the diff below would be correct ? I tested it on
mips and it seems to work fine but I can't really say if it's
valid enough to submit it as a patch...

thanks
		Franck

---8<---

diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index 4482a06..024006e 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -151,6 +151,13 @@ create_elf_tables(struct linux_binprm *bprm, struct elfhdr *exec,
 	struct vm_area_struct *vma;
 
 	/*
+	 * In some cases (e.g. Hyper-Threading), we want to avoid L1
+	 * evictions by the processes running on the same package. One
+	 * thing we can do is to shuffle the initial stack for them.
+	 */
+	p = arch_align_stack(p);
+
+	/*
 	 * If this architecture has a platform capability string, copy it
 	 * to userspace.  In some cases (Sparc), this info is impossible
 	 * for userspace to get any other way, in others (i386) it is
@@ -160,14 +167,6 @@ create_elf_tables(struct linux_binprm *bprm, struct elfhdr *exec,
 	if (k_platform) {
 		size_t len = strlen(k_platform) + 1;
 
-		/*
-		 * In some cases (e.g. Hyper-Threading), we want to avoid L1
-		 * evictions by the processes running on the same package. One
-		 * thing we can do is to shuffle the initial stack for them.
-		 */
-
-		p = arch_align_stack(p);
-
 		u_platform = (elf_addr_t __user *)STACK_ALLOC(p, len);
 		if (__copy_to_user(u_platform, k_platform, len))
 			return -EFAULT;