| Message ID | alpine.LFD.2.00.1104072251390.28032@xanadu.home |
|---|---|
| State | New |
| Headers | show |
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. Nicolas
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.
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
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.
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
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); /*
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(-)