[PATCH] FDPIC: Ignore the loader's PT_GNU_STACK when calculating the stack size

[PATCH] FDPIC: Ignore the loader's PT_GNU_STACK when calculating the stack size

[David Howells]

Ignore the loader's PT_GNU_STACK when calculating the stack size, and only
consider the executable's PT_GNU_STACK, assuming the executable has one.

Currently the behaviour is to take the largest stack size and use that, but
that means you can't reduce the stack size in the executable.  The loader's
stack size should probably only be used when executing the loader directly.

WARNING: This patch is slightly dangerous - it may render a system inoperable
if the loader's stack size is larger than that of important executables, and
the system relies unknowingly on this increasing the size of the stack.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 fs/binfmt_elf_fdpic.c |   19 +++++++++++--------
 1 files changed, 11 insertions(+), 8 deletions(-)


diff --git a/fs/binfmt_elf_fdpic.c b/fs/binfmt_elf_fdpic.c
index 81ca047..58c49fa 100644
--- a/fs/binfmt_elf_fdpic.c
+++ b/fs/binfmt_elf_fdpic.c
@@ -283,20 +283,23 @@ static int load_elf_fdpic_binary(struct linux_binprm *bprm,
 	}
 
 	stack_size = exec_params.stack_size;
-	if (stack_size < interp_params.stack_size)
-		stack_size = interp_params.stack_size;
-
 	if (exec_params.flags & ELF_FDPIC_FLAG_EXEC_STACK)
 		executable_stack = EXSTACK_ENABLE_X;
 	else if (exec_params.flags & ELF_FDPIC_FLAG_NOEXEC_STACK)
 		executable_stack = EXSTACK_DISABLE_X;
-	else if (interp_params.flags & ELF_FDPIC_FLAG_EXEC_STACK)
-		executable_stack = EXSTACK_ENABLE_X;
-	else if (interp_params.flags & ELF_FDPIC_FLAG_NOEXEC_STACK)
-		executable_stack = EXSTACK_DISABLE_X;
-	else
+	else 
 		executable_stack = EXSTACK_DEFAULT;
 
+	if (stack_size == 0) {
+		stack_size = interp_params.stack_size;
+		if (interp_params.flags & ELF_FDPIC_FLAG_EXEC_STACK)
+			executable_stack = EXSTACK_ENABLE_X;
+		else if (interp_params.flags & ELF_FDPIC_FLAG_NOEXEC_STACK)
+			executable_stack = EXSTACK_DISABLE_X;
+		else
+			executable_stack = EXSTACK_DEFAULT;
+	}
+
 	retval = -ENOEXEC;
 	if (stack_size == 0)
 		goto error;

Re: [PATCH] FDPIC: Ignore the loader's PT_GNU_STACK when calculating the stack size

[Mike Frysinger]

On Wed, Jul 1, 2009 at 08:08, David Howells wrote:
> WARNING: This patch is slightly dangerous - it may render a system inoperable
> if the loader's stack size is larger than that of important executables, and
> the system relies unknowingly on this increasing the size of the stack.

every toolchain ive seen sets the stack size in the linker to the
default value, and the only way to change that is to manually set the
size yourself with the relevant command line option.  so it's only
going to break people who have been doing it wrong themselves --
setting the stack size to a smaller option than is acceptable.

> -       else
> +       else

this change just adds trailing whitespace ...

otherwise, just tested it and it works for me, thanks
Signed-off-by: Mike Frysinger <vapier@gentoo.org>
-mike

Re: [PATCH] FDPIC: Ignore the loader's PT_GNU_STACK when calculating the stack size

[David Howells]

Mike Frysinger <vapier.adi@gmail.com> wrote:

> this change just adds trailing whitespace ...

Fixed.

> Signed-off-by: Mike Frysinger <vapier@gentoo.org>

Oops.  Should've added that.

David

[PATCH] FDPIC: Ignore the loader's PT_GNU_STACK when calculating the stack size [ver #2]

[David Howells]

Ignore the loader's PT_GNU_STACK when calculating the stack size, and only
consider the executable's PT_GNU_STACK, assuming the executable has one.

Currently the behaviour is to take the largest stack size and use that, but
that means you can't reduce the stack size in the executable.  The loader's
stack size should probably only be used when executing the loader directly.

WARNING: This patch is slightly dangerous - it may render a system inoperable
if the loader's stack size is larger than that of important executables, and
the system relies unknowingly on this increasing the size of the stack.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 fs/binfmt_elf_fdpic.c |   17 ++++++++++-------
 1 files changed, 10 insertions(+), 7 deletions(-)


diff --git a/fs/binfmt_elf_fdpic.c b/fs/binfmt_elf_fdpic.c
index 81ca047..a1e6365 100644
--- a/fs/binfmt_elf_fdpic.c
+++ b/fs/binfmt_elf_fdpic.c
@@ -283,20 +283,23 @@ static int load_elf_fdpic_binary(struct linux_binprm *bprm,
 	}
 
 	stack_size = exec_params.stack_size;
-	if (stack_size < interp_params.stack_size)
-		stack_size = interp_params.stack_size;
-
 	if (exec_params.flags & ELF_FDPIC_FLAG_EXEC_STACK)
 		executable_stack = EXSTACK_ENABLE_X;
 	else if (exec_params.flags & ELF_FDPIC_FLAG_NOEXEC_STACK)
 		executable_stack = EXSTACK_DISABLE_X;
-	else if (interp_params.flags & ELF_FDPIC_FLAG_EXEC_STACK)
-		executable_stack = EXSTACK_ENABLE_X;
-	else if (interp_params.flags & ELF_FDPIC_FLAG_NOEXEC_STACK)
-		executable_stack = EXSTACK_DISABLE_X;
 	else
 		executable_stack = EXSTACK_DEFAULT;
 
+	if (stack_size == 0) {
+		stack_size = interp_params.stack_size;
+		if (interp_params.flags & ELF_FDPIC_FLAG_EXEC_STACK)
+			executable_stack = EXSTACK_ENABLE_X;
+		else if (interp_params.flags & ELF_FDPIC_FLAG_NOEXEC_STACK)
+			executable_stack = EXSTACK_DISABLE_X;
+		else
+			executable_stack = EXSTACK_DEFAULT;
+	}
+
 	retval = -ENOEXEC;
 	if (stack_size == 0)
 		goto error;

[PATCH] FDPIC: Ignore the loader's PT_GNU_STACK when calculating the stack size [ver #2]

[David Howells]

Ignore the loader's PT_GNU_STACK when calculating the stack size, and only
consider the executable's PT_GNU_STACK, assuming the executable has one.

Currently the behaviour is to take the largest stack size and use that, but
that means you can't reduce the stack size in the executable.  The loader's
stack size should probably only be used when executing the loader directly.

WARNING: This patch is slightly dangerous - it may render a system inoperable
if the loader's stack size is larger than that of important executables, and
the system relies unknowingly on this increasing the size of the stack.

Signed-off-by: David Howells <dhowells@redhat.com>
Signed-off-by: Mike Frysinger <vapier@gentoo.org>
---

 fs/binfmt_elf_fdpic.c |   17 ++++++++++-------
 1 files changed, 10 insertions(+), 7 deletions(-)


diff --git a/fs/binfmt_elf_fdpic.c b/fs/binfmt_elf_fdpic.c
index 81ca047..a1e6365 100644
--- a/fs/binfmt_elf_fdpic.c
+++ b/fs/binfmt_elf_fdpic.c
@@ -283,20 +283,23 @@ static int load_elf_fdpic_binary(struct linux_binprm *bprm,
 	}
 
 	stack_size = exec_params.stack_size;
-	if (stack_size < interp_params.stack_size)
-		stack_size = interp_params.stack_size;
-
 	if (exec_params.flags & ELF_FDPIC_FLAG_EXEC_STACK)
 		executable_stack = EXSTACK_ENABLE_X;
 	else if (exec_params.flags & ELF_FDPIC_FLAG_NOEXEC_STACK)
 		executable_stack = EXSTACK_DISABLE_X;
-	else if (interp_params.flags & ELF_FDPIC_FLAG_EXEC_STACK)
-		executable_stack = EXSTACK_ENABLE_X;
-	else if (interp_params.flags & ELF_FDPIC_FLAG_NOEXEC_STACK)
-		executable_stack = EXSTACK_DISABLE_X;
 	else
 		executable_stack = EXSTACK_DEFAULT;
 
+	if (stack_size == 0) {
+		stack_size = interp_params.stack_size;
+		if (interp_params.flags & ELF_FDPIC_FLAG_EXEC_STACK)
+			executable_stack = EXSTACK_ENABLE_X;
+		else if (interp_params.flags & ELF_FDPIC_FLAG_NOEXEC_STACK)
+			executable_stack = EXSTACK_DISABLE_X;
+		else
+			executable_stack = EXSTACK_DEFAULT;
+	}
+
 	retval = -ENOEXEC;
 	if (stack_size == 0)
 		goto error;

Re: [PATCH] FDPIC: Ignore the loader's PT_GNU_STACK when calculating the stack size

[Paul Mundt]

On Wed, Jul 01, 2009 at 01:08:14PM +0100, David Howells wrote:
> Ignore the loader's PT_GNU_STACK when calculating the stack size, and only
> consider the executable's PT_GNU_STACK, assuming the executable has one.
> 
> Currently the behaviour is to take the largest stack size and use that, but
> that means you can't reduce the stack size in the executable.  The loader's
> stack size should probably only be used when executing the loader directly.
> 
> WARNING: This patch is slightly dangerous - it may render a system inoperable
> if the loader's stack size is larger than that of important executables, and
> the system relies unknowingly on this increasing the size of the stack.
> 
> Signed-off-by: David Howells <dhowells@redhat.com>

Works for me too.

Acked-by: Paul Mundt <lethal@linux-sh.org>

Re: [PATCH] FDPIC: Ignore the loader's PT_GNU_STACK when calculating the stack size

[Pavel Machek]

On Wed 2009-07-01 13:08:14, David Howells wrote:
> Ignore the loader's PT_GNU_STACK when calculating the stack size, and only
> consider the executable's PT_GNU_STACK, assuming the executable has one.
> 
> Currently the behaviour is to take the largest stack size and use that, but
> that means you can't reduce the stack size in the executable.  The loader's
> stack size should probably only be used when executing the loader directly.
> 
> WARNING: This patch is slightly dangerous - it may render a system inoperable
> if the loader's stack size is larger than that of important executables, and
> the system relies unknowingly on this increasing the size of the stack.

The patch seems wrong to me; loader needs the stack, too, right?

What about making sure that the loader specifies reasonable stack
size, instead?

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] FDPIC: Ignore the loader's PT_GNU_STACK when calculating the stack size

[David Howells]

Pavel Machek <pavel@ucw.cz> wrote:

> The patch seems wrong to me; loader needs the stack, too, right?
> 
> What about making sure that the loader specifies reasonable stack
> size, instead?

The loader doesn't need its own stack, unless it _is_ the executable.  It uses
the executable's stack.  The problem is that the executable and the loader can
both specify the stack size for NOMMU, but which one do we pick, or do we
consider both.

David

Re: [PATCH] FDPIC: Ignore the loader's PT_GNU_STACK when calculating the stack size

[Pavel Machek]

On Wed 2009-07-08 12:01:40, David Howells wrote:
> Pavel Machek <pavel@ucw.cz> wrote:
> 
> > The patch seems wrong to me; loader needs the stack, too, right?
> > 
> > What about making sure that the loader specifies reasonable stack
> > size, instead?
> 
> The loader doesn't need its own stack, unless it _is_ the executable.  It uses
> the executable's stack.  The problem is that the executable and the loader can
> both specify the stack size for NOMMU, but which one do we pick, or do we
> consider both.

Imageine loader needs 64K, while executable only needs 4K. You surely
want to execute it with 64K stack, because it will not fit into 4K?

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] FDPIC: Ignore the loader's PT_GNU_STACK when calculating the stack size

[Mike Frysinger]

On Fri, Jul 3, 2009 at 23:34, Pavel Machek wrote:
> On Wed 2009-07-08 12:01:40, David Howells wrote:
>> Pavel Machek <pavel@ucw.cz> wrote:
>>
>> > The patch seems wrong to me; loader needs the stack, too, right?
>> >
>> > What about making sure that the loader specifies reasonable stack
>> > size, instead?
>>
>> The loader doesn't need its own stack, unless it _is_ the executable.  It uses
>> the executable's stack.  The problem is that the executable and the loader can
>> both specify the stack size for NOMMU, but which one do we pick, or do we
>> consider both.
>
> Imageine loader needs 64K, while executable only needs 4K. You surely
> want to execute it with 64K stack, because it will not fit into 4K?

i really dont think this is realistic.  there is exactly one ldso that
everyone uses under FDPIC ELF, and it needs a very minuscule stack.
-mike

Re: [PATCH] FDPIC: Ignore the loader's PT_GNU_STACK when calculating the stack size

[Pavel Machek]

On Wed 2009-07-08 14:48:51, Mike Frysinger wrote:
> On Fri, Jul 3, 2009 at 23:34, Pavel Machek wrote:
> > On Wed 2009-07-08 12:01:40, David Howells wrote:
> >> Pavel Machek <pavel@ucw.cz> wrote:
> >>
> >> > The patch seems wrong to me; loader needs the stack, too, right?
> >> >
> >> > What about making sure that the loader specifies reasonable stack
> >> > size, instead?
> >>
> >> The loader doesn't need its own stack, unless it _is_ the executable.  It uses
> >> the executable's stack.  The problem is that the executable and the loader can
> >> both specify the stack size for NOMMU, but which one do we pick, or do we
> >> consider both.
> >
> > Imageine loader needs 64K, while executable only needs 4K. You surely
> > want to execute it with 64K stack, because it will not fit into 4K?
> 
> i really dont think this is realistic.  there is exactly one ldso that
> everyone uses under FDPIC ELF, and it needs a very minuscule stack.

Not very realistic; but that argues that the patch is NOP.

And if it _is_ realistic, the patch adds a bug.

=> patch is bad idea.
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] FDPIC: Ignore the loader's PT_GNU_STACK when calculating the stack size

[David Howells]

Pavel Machek <pavel@ucw.cz> wrote:

> > i really dont think this is realistic.  there is exactly one ldso that
> > everyone uses under FDPIC ELF, and it needs a very minuscule stack.
> 
> Not very realistic; but that argues that the patch is NOP.
>
> And if it _is_ realistic, the patch adds a bug.

No, it doesn't.  The problem is that the loader, when it is linked, is given a
sillyly large default stack size, and this causes the application to be given a
much larger stack than is strictly necessary - a stack that is drawn from a
limited pool of non-pageable RAM and that must be allocated as a contiguous
lump.

The executables should take into account the amount of stack space needed to
call into all their libraries - and that includes the loader.  You can argue
that the loader should specify the amount of extra overhead it will need, and
that this should be _added_ to the executable's stack estimate, but using the
loader's stack estimate instead of the executable's does not really make sense.

You can also argue that the executable doesn't know how much space the loader
will use, because the loader can be independently replaced; but this isn't a
good argument because the loader doesn't know how much the executable will use,
and neither know how much the various shared libs will use.  The only way to do
that is to add up all the stack estimates.

Now, it *is* perfectly reasonable to use the loader's stack estimate in the
case that the loader is run as an executable.  It could, for example, behave as
the GLIBC loader and have some functions available for shared library query and
suchlike.

David

Re: [PATCH] FDPIC: Ignore the loader's PT_GNU_STACK when calculating the stack size

[Pavel Machek]

On Thu 2009-07-09 11:59:11, David Howells wrote:
> Pavel Machek <pavel@ucw.cz> wrote:
> 
> > > i really dont think this is realistic.  there is exactly one ldso that
> > > everyone uses under FDPIC ELF, and it needs a very minuscule stack.
> > 
> > Not very realistic; but that argues that the patch is NOP.
> >
> > And if it _is_ realistic, the patch adds a bug.
> 
> No, it doesn't.  The problem is that the loader, when it is linked, is given a
> sillyly large default stack size, and this causes the application to be given a
> much larger stack than is strictly necessary - a stack that is drawn from a
> limited pool of non-pageable RAM and that must be allocated as a contiguous
> lump.

Fix the loader to only request as big stack as it needs?
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] FDPIC: Ignore the loader's PT_GNU_STACK when calculating the stack size

[Mike Frysinger]

On Sat, Jul 11, 2009 at 17:30, Pavel Machek wrote:
> On Thu 2009-07-09 11:59:11, David Howells wrote:
>> Pavel Machek wrote:
>> > > i really dont think this is realistic.  there is exactly one ldso that
>> > > everyone uses under FDPIC ELF, and it needs a very minuscule stack.
>> >
>> > Not very realistic; but that argues that the patch is NOP.
>> >
>> > And if it _is_ realistic, the patch adds a bug.
>>
>> No, it doesn't.  The problem is that the loader, when it is linked, is given a
>> sillyly large default stack size, and this causes the application to be given a
>> much larger stack than is strictly necessary - a stack that is drawn from a
>> limited pool of non-pageable RAM and that must be allocated as a contiguous
>> lump.
>
> Fix the loader to only request as big stack as it needs?

and what if the loader needs a larger stack when run as an application
?  you could make the same exact argument for every library that an
application has a DT_NEEDED tag for, or that it dlopen()'s.  but for
the same reasons, it doesnt fly.

the only stack that should be checked is what the application itself
says it needs.  the ldso has no way of knowing what functions exactly
the application in question will be using (whether in the ldso itself
or in any library), thus only the application itself knows what the
stack usage will look like.
-mike

Re: [PATCH] FDPIC: Ignore the loader's PT_GNU_STACK when calculating the stack size

[Pavel Machek]

On Tue 2009-07-14 08:15:03, Mike Frysinger wrote:
> On Sat, Jul 11, 2009 at 17:30, Pavel Machek wrote:
> > On Thu 2009-07-09 11:59:11, David Howells wrote:
> >> Pavel Machek wrote:
> >> > > i really dont think this is realistic.  there is exactly one ldso that
> >> > > everyone uses under FDPIC ELF, and it needs a very minuscule stack.
> >> >
> >> > Not very realistic; but that argues that the patch is NOP.
> >> >
> >> > And if it _is_ realistic, the patch adds a bug.
> >>
> >> No, it doesn't.  The problem is that the loader, when it is linked, is given a
> >> sillyly large default stack size, and this causes the application to be given a
> >> much larger stack than is strictly necessary - a stack that is drawn from a
> >> limited pool of non-pageable RAM and that must be allocated as a contiguous
> >> lump.
> >
> > Fix the loader to only request as big stack as it needs?
> 
> and what if the loader needs a larger stack when run as an application
> ?  you could make the same exact argument for every library that an
> application has a DT_NEEDED tag for, or that it dlopen()'s.  but for
> the same reasons, it doesnt fly.
> 
> the only stack that should be checked is what the application itself
> says it needs.  the ldso has no way of knowing what functions exactly
> the application in question will be using (whether in the ldso itself
> or in any library), thus only the application itself knows what the
> stack usage will look like.

And the application has no way of knowing how much stack this
particular ldso needs. Too bad, it is all broken.

(And the solution of taking maximum of all involved parties still
looks best to me.)
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] FDPIC: Ignore the loader's PT_GNU_STACK when calculating the stack size

[Mike Frysinger]

On Sat, Jul 18, 2009 at 06:39, Pavel Machek wrote:
> On Tue 2009-07-14 08:15:03, Mike Frysinger wrote:
>> On Sat, Jul 11, 2009 at 17:30, Pavel Machek wrote:
>> > On Thu 2009-07-09 11:59:11, David Howells wrote:
>> >> Pavel Machek wrote:
>> >> > > i really dont think this is realistic.  there is exactly one ldso that
>> >> > > everyone uses under FDPIC ELF, and it needs a very minuscule stack.
>> >> >
>> >> > Not very realistic; but that argues that the patch is NOP.
>> >> >
>> >> > And if it _is_ realistic, the patch adds a bug.
>> >>
>> >> No, it doesn't.  The problem is that the loader, when it is linked, is given a
>> >> sillyly large default stack size, and this causes the application to be given a
>> >> much larger stack than is strictly necessary - a stack that is drawn from a
>> >> limited pool of non-pageable RAM and that must be allocated as a contiguous
>> >> lump.
>> >
>> > Fix the loader to only request as big stack as it needs?
>>
>> and what if the loader needs a larger stack when run as an application
>> ?  you could make the same exact argument for every library that an
>> application has a DT_NEEDED tag for, or that it dlopen()'s.  but for
>> the same reasons, it doesnt fly.
>>
>> the only stack that should be checked is what the application itself
>> says it needs.  the ldso has no way of knowing what functions exactly
>> the application in question will be using (whether in the ldso itself
>> or in any library), thus only the application itself knows what the
>> stack usage will look like.
>
> And the application has no way of knowing how much stack this
> particular ldso needs. Too bad, it is all broken.

you're still wrongly assuming the ldso has any idea of what functions
the application will be invoking.  in the nommu embedded world (which
is the *only* place this change matters), reduced stack sizes are not
picked out of a hat.  they're taken based on actual testing/review for
a particular setup.  along that same line, upgrading of complete
systems (kernel/userspace/toolchain) arent dropped in casually -- this
kind of reduced stack review would occur again.  your concern is not
realistic in any way nor applicable in any scenario that matters.
-mike