[PATCH 2/2] ARM: fix personality flag propagation across an exec

[PATCH 2/2] ARM: fix personality flag propagation across an exec

[Nicolas Pitre]

Our SET_PERSONALITY() implementation was overwriting all existing
personality flags, including ADDR_NO_RANDOMIZE, making them unavailable
to processes being exec'd after a call to personality() in user space.
This prevents the gdb test suite from running successfully.

Signed-off-by: Nicolas Pitre <nicolas.pitre@linaro.org>
---
 arch/arm/kernel/elf.c |   19 +++++++++++++------
 1 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/arch/arm/kernel/elf.c b/arch/arm/kernel/elf.c
index d4a0da1..8524d09 100644
--- a/arch/arm/kernel/elf.c
+++ b/arch/arm/kernel/elf.c
@@ -40,16 +40,23 @@ EXPORT_SYMBOL(elf_check_arch);
 void elf_set_personality(const struct elf32_hdr *x)
 {
 	unsigned int eflags = x->e_flags;
-	unsigned int personality = PER_LINUX_32BIT;
+	unsigned int personality = current->personality;
 
 	/*
+	 * Inherit most personality flags from parent, except for those
+	 * we're about to choose.  Beware: PER_LINUX_32BIT carries flag bits
+	 * outside of PER_MASK.
+	 */
+	personality &= ~(PER_MASK | PER_LINUX | PER_LINUX_32BIT);
+		
+	/*
 	 * APCS-26 is only valid for OABI executables
 	 */
-	if ((eflags & EF_ARM_EABI_MASK) == EF_ARM_EABI_UNKNOWN) {
-		if (eflags & EF_ARM_APCS_26)
-			personality = PER_LINUX;
-	}
-
+	if ((eflags & EF_ARM_EABI_MASK) == EF_ARM_EABI_UNKNOWN &&
+	    (eflags & EF_ARM_APCS_26))
+		personality |= PER_LINUX;
+	else
+		personality |= PER_LINUX_32BIT;
 	set_personality(personality);
 
 	/*
-- 
1.7.4

[PATCH 2/2] ARM: fix personality flag propagation across an exec

[Russell King - ARM Linux]

On Thu, Apr 07, 2011 at 10:52:53PM -0400, Nicolas Pitre wrote:
>  	/*
> +	 * Inherit most personality flags from parent, except for those
> +	 * we're about to choose.  Beware: PER_LINUX_32BIT carries flag bits
> +	 * outside of PER_MASK.
> +	 */
> +	personality &= ~(PER_MASK | PER_LINUX | PER_LINUX_32BIT);

PER_LINUX and PER_LINUX_32BIT aren't bitflags - the LSB is a numeric
personality ID.  So this looks wrong.

[PATCH 2/2] ARM: fix personality flag propagation across an exec

[Nicolas Pitre]

On Fri, 8 Apr 2011, Russell King - ARM Linux wrote:

> On Thu, Apr 07, 2011 at 10:52:53PM -0400, Nicolas Pitre wrote:
> >  	/*
> > +	 * Inherit most personality flags from parent, except for those
> > +	 * we're about to choose.  Beware: PER_LINUX_32BIT carries flag bits
> > +	 * outside of PER_MASK.
> > +	 */
> > +	personality &= ~(PER_MASK | PER_LINUX | PER_LINUX_32BIT);
> 
> PER_LINUX and PER_LINUX_32BIT aren't bitflags - the LSB is a numeric
> personality ID.  So this looks wrong.

[PATCH 2/2] ARM: fix personality flag propagation across an exec

[Russell King - ARM Linux]

On Fri, Apr 08, 2011 at 09:00:07AM -0400, Nicolas Pitre wrote:
> On Fri, 8 Apr 2011, Russell King - ARM Linux wrote:
> 
> > On Thu, Apr 07, 2011 at 10:52:53PM -0400, Nicolas Pitre wrote:
> > >  	/*
> > > +	 * Inherit most personality flags from parent, except for those
> > > +	 * we're about to choose.  Beware: PER_LINUX_32BIT carries flag bits
> > > +	 * outside of PER_MASK.
> > > +	 */
> > > +	personality &= ~(PER_MASK | PER_LINUX | PER_LINUX_32BIT);
> > 
> > PER_LINUX and PER_LINUX_32BIT aren't bitflags - the LSB is a numeric
> > personality ID.  So this looks wrong.
> 
> >From include/linux/personality.h:
> 
> enum {
>         PER_LINUX =             0x0000,
>         PER_LINUX_32BIT =       0x0000 | ADDR_LIMIT_32BIT,
>         PER_LINUX_FDPIC =       0x0000 | FDPIC_FUNCPTRS,
>         PER_SVR4 =              0x0001 | STICKY_TIMEOUTS | MMAP_PAGE_ZERO,
>         [...]
> 
> So this is a combination of a personality ID and flag bits.  And the 
> only difference between PER_LINUX and PER_LINUX_32BIT is one of those 
> flag bits.

Yes but its wrong to clear the bitmask using ~PER_LINUX etc.

What you want to be doing is:

	personality &= ~(PER_MASK | ADDR_LIMIT_32BIT);

so you're clearing the LSB being the personality type, and the 32-bit
address limit.

[PATCH 2/2] ARM: fix personality flag propagation across an exec

[Nicolas Pitre]

On Fri, 8 Apr 2011, Russell King - ARM Linux wrote:

> On Fri, Apr 08, 2011 at 09:00:07AM -0400, Nicolas Pitre wrote:
> > On Fri, 8 Apr 2011, Russell King - ARM Linux wrote:
> > 
> > > On Thu, Apr 07, 2011 at 10:52:53PM -0400, Nicolas Pitre wrote:
> > > >  	/*
> > > > +	 * Inherit most personality flags from parent, except for those
> > > > +	 * we're about to choose.  Beware: PER_LINUX_32BIT carries flag bits
> > > > +	 * outside of PER_MASK.
> > > > +	 */
> > > > +	personality &= ~(PER_MASK | PER_LINUX | PER_LINUX_32BIT);
> > > 
> > > PER_LINUX and PER_LINUX_32BIT aren't bitflags - the LSB is a numeric
> > > personality ID.  So this looks wrong.
> > 
> > >From include/linux/personality.h:
> > 
> > enum {
> >         PER_LINUX =             0x0000,
> >         PER_LINUX_32BIT =       0x0000 | ADDR_LIMIT_32BIT,
> >         PER_LINUX_FDPIC =       0x0000 | FDPIC_FUNCPTRS,
> >         PER_SVR4 =              0x0001 | STICKY_TIMEOUTS | MMAP_PAGE_ZERO,
> >         [...]
> > 
> > So this is a combination of a personality ID and flag bits.  And the 
> > only difference between PER_LINUX and PER_LINUX_32BIT is one of those 
> > flag bits.
> 
> Yes but its wrong to clear the bitmask using ~PER_LINUX etc.
> 
> What you want to be doing is:
> 
> 	personality &= ~(PER_MASK | ADDR_LIMIT_32BIT);
> 
> so you're clearing the LSB being the personality type, and the 32-bit
> address limit.

Sure.

However, if we're only setting the address limit flag here, wouldn't it 
be better to leave the current personality type as is and only set/clear 
the ADDR_LIMIT_32BIT flag?  Something like:

	unsigned int personality = current->personality;
        if ((eflags & EF_ARM_EABI_MASK) == EF_ARM_EABI_UNKNOWN &&
            (eflags & EF_ARM_APCS_26))
                personality &= ~ADDR_LIMIT_32BIT;
        else
                personality |= ADDR_LIMIT_32BIT;
        set_personality(personality);

Or is the actual personality type not supposed to be inherited?

I also notice that bad_syscall() is broken if extra flags such as 
ADDR_NO_RANDOMIZE are added to the current personality (will send a 
patch for that as well).


Nicolas

[PATCH 2/2] ARM: fix personality flag propagation across an exec

[Russell King - ARM Linux]

On Fri, Apr 08, 2011 at 03:50:21PM -0400, Nicolas Pitre wrote:
> However, if we're only setting the address limit flag here, wouldn't it 
> be better to leave the current personality type as is and only set/clear 
> the ADDR_LIMIT_32BIT flag?  Something like:
> 
> 	unsigned int personality = current->personality;
>         if ((eflags & EF_ARM_EABI_MASK) == EF_ARM_EABI_UNKNOWN &&
>             (eflags & EF_ARM_APCS_26))
>                 personality &= ~ADDR_LIMIT_32BIT;
>         else
>                 personality |= ADDR_LIMIT_32BIT;
>         set_personality(personality);
> 
> Or is the actual personality type not supposed to be inherited?
> 
> I also notice that bad_syscall() is broken if extra flags such as 
> ADDR_NO_RANDOMIZE are added to the current personality (will send a 
> patch for that as well).

Many architectures explicitly set a personality type on exec, so that
seems to be the thing to do.  We want it set to a PER_LINUX flavour
as the ELF executables we run tend to be Linux executables.

Also, the ARM kernel doesn't really support anything but PER_LINUX
ELF executables, so it'd be rather meaningless to set it to anything
else here.

So:

	unsigned int personality = current->personality & ~PER_MASK;

	/*
	 * We only support Linux ELF executables, so always set the
	 * personality to LINUX.
	 */
	personality |= PER_LINUX;

	/* APCS-26 is only valid for OABI executables */
	if ((eflags & EF_ARM_EABI_MASK) == EF_ARM_EABI_UNKNOWN &&
	    (eflags & EF_ARM_APCS_26))
		personality &= ~ADDR_LIMIT_32BIT;
	else
		personality |= ADDR_LIMIT_32BIT;

	set_personality(personality);

is probably what we want.

[PATCH 2/2] ARM: fix personality flag propagation across an exec

[Nicolas Pitre]

On Fri, 8 Apr 2011, Russell King - ARM Linux wrote:

> On Fri, Apr 08, 2011 at 03:50:21PM -0400, Nicolas Pitre wrote:
> 
> > Or is the actual personality type not supposed to be inherited?
> 
> Many architectures explicitly set a personality type on exec, so that
> seems to be the thing to do.  We want it set to a PER_LINUX flavour
> as the ELF executables we run tend to be Linux executables.
> 
> Also, the ARM kernel doesn't really support anything but PER_LINUX
> ELF executables, so it'd be rather meaningless to set it to anything
> else here.
> 
> So:
> 
> 	unsigned int personality = current->personality & ~PER_MASK;
> 
> 	/*
> 	 * We only support Linux ELF executables, so always set the
> 	 * personality to LINUX.
> 	 */
> 	personality |= PER_LINUX;
> 
> 	/* APCS-26 is only valid for OABI executables */
> 	if ((eflags & EF_ARM_EABI_MASK) == EF_ARM_EABI_UNKNOWN &&
> 	    (eflags & EF_ARM_APCS_26))
> 		personality &= ~ADDR_LIMIT_32BIT;
> 	else
> 		personality |= ADDR_LIMIT_32BIT;
> 
> 	set_personality(personality);
> 
> is probably what we want.

ACK.  Are you committing this directly, or do you still want me to send 
you a patch?


Nicolas