[PATCH] broken TASK_SIZE for ia32_aout

[PATCH] broken TASK_SIZE for ia32_aout

[Al Viro]

Setting TIF_IA32 in load_aout_binary() used to be enough; these days
TASK_SIZE is controlled by TIF_ADDR32 and that one doesn't get set
there.  Switch to use of set_personality_ia32()...

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
diff --git a/arch/x86/ia32/ia32_aout.c b/arch/x86/ia32/ia32_aout.c
index 4824fb4..07b3a68 100644
--- a/arch/x86/ia32/ia32_aout.c
+++ b/arch/x86/ia32/ia32_aout.c
@@ -294,8 +294,7 @@ static int load_aout_binary(struct linux_binprm *bprm, struct pt_regs *regs)
 
 	/* OK, This is the point of no return */
 	set_personality(PER_LINUX);
-	set_thread_flag(TIF_IA32);
-	current->mm->context.ia32_compat = 1;
+	set_personality_ia32(false);
 
 	setup_new_exec(bprm);
 

Re: [PATCH] broken TASK_SIZE for ia32_aout

[Linus Torvalds]

On Sun, May 6, 2012 at 9:20 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> Setting TIF_IA32 in load_aout_binary() used to be enough; these days
> TASK_SIZE is controlled by TIF_ADDR32 and that one doesn't get set
> there.  Switch to use of set_personality_ia32()...

Applied. Just out of curiosity, how did you notice? Just looking at
TIF_IA32 usage, or do you actually have some old app?

                   Linus

[tip:x86/urgent] x86, compat: Correct broken TASK_SIZE for ia32 a. out binaries

[tip-bot for Al Viro]

Commit-ID:  2be8e9ec677d6dfd4268b80e1ccd7d21777b86e3
Gitweb:     http://git.kernel.org/tip/2be8e9ec677d6dfd4268b80e1ccd7d21777b86e3
Author:     Al Viro <viro@ZenIV.linux.org.uk>
AuthorDate: Sun, 6 May 2012 17:20:00 +0100
Committer:  H. Peter Anvin <hpa@zytor.com>
CommitDate: Sun, 6 May 2012 09:53:15 -0700

x86, compat: Correct broken TASK_SIZE for ia32 a.out binaries

Setting TIF_IA32 in load_aout_binary() used to be enough; these days
TASK_SIZE is controlled by TIF_ADDR32 and that one doesn't get set
there.  Switch to use of set_personality_ia32()...

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
Link: http://lkml.kernel.org/r/20120506162000.GT6871@ZenIV.linux.org.uk
Signed-off-by: H. Peter Anvin <hpa@zytor.com>
---
 arch/x86/ia32/ia32_aout.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/arch/x86/ia32/ia32_aout.c b/arch/x86/ia32/ia32_aout.c
index d511d95..4c979cf 100644
--- a/arch/x86/ia32/ia32_aout.c
+++ b/arch/x86/ia32/ia32_aout.c
@@ -296,8 +296,7 @@ static int load_aout_binary(struct linux_binprm *bprm, struct pt_regs *regs)
 
 	/* OK, This is the point of no return */
 	set_personality(PER_LINUX);
-	set_thread_flag(TIF_IA32);
-	current->mm->context.ia32_compat = 1;
+	set_personality_ia32(false);
 
 	setup_new_exec(bprm);
 

Re: [PATCH] broken TASK_SIZE for ia32_aout

[H. Peter Anvin]

On 05/06/2012 10:16 AM, Linus Torvalds wrote:
> On Sun, May 6, 2012 at 9:20 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>> Setting TIF_IA32 in load_aout_binary() used to be enough; these days
>> TASK_SIZE is controlled by TIF_ADDR32 and that one doesn't get set
>> there.  Switch to use of set_personality_ia32()...
> 
> Applied. Just out of curiosity, how did you notice? Just looking at
> TIF_IA32 usage, or do you actually have some old app?
> 

OK, I just queued it up in my tree for a push to you... do you want me
to drop it back out and send the balance?

	-hpa


-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [PATCH] broken TASK_SIZE for ia32_aout

[Al Viro]

On Sun, May 06, 2012 at 10:16:11AM -0700, Linus Torvalds wrote:
> On Sun, May 6, 2012 at 9:20 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> > Setting TIF_IA32 in load_aout_binary() used to be enough; these days
> > TASK_SIZE is controlled by TIF_ADDR32 and that one doesn't get set
> > there. ?Switch to use of set_personality_ia32()...
> 
> Applied. Just out of curiosity, how did you notice? Just looking at
> TIF_IA32 usage, or do you actually have some old app?

Putting together an idiot's guide to thread flags ;-)  Other very similar
bug, AFAICS, is in mips elf.h:
#define __SET_PERSONALITY32_N32()                                       \
        do {                                                            \
                set_thread_flag(TIF_32BIT_ADDR);                        \
                current->thread.abi = &mips_abi_n32;                    \
        } while (0)
probably should clear_thread_flag(TIF_32BIT_REGS); as well.  Ralf?

Another catch on mips, and that one is downright funny:
#ifdef CONFIG_MIPS32_O32
#define TIF_32BIT TIF_32BIT_REGS
#elif defined(CONFIG_MIPS32_N32)
#define TIF_32BIT _TIF_32BIT_ADDR
#endif /* CONFIG_MIPS32_O32 */

Spot the breakage.  Note that TIF_32BIT is used only in is_compat_task()
there, so that went unnoticed for a long time.  BTW, I really wonder if
the logics here is correct, nevermind the obvious typo...  For kernels
with O32, but not N32 support we could bloody well use TIF_32BIT_ADDR -
O32 sets both flags.  And for kernels that support both (O32 and N32 are
not mutually exclusive), we probably want both of o32 and n32 processes
match is_compat_task().

Note that we have different semantics for is_compat_task() on x86 and
everywhere (AFAICS) else: "is this a 32bit syscall" vs. "is this a biarch
syscall with 32bit pointers, etc.".  On architectures where 32bit syscall
can't be issued by 64bit task and vice versa there's no difference, but for
e.g. sparc there definitely is one.  is_compat_task() there goes by what
the task is, not what kind of syscall is it trying to make.  It mostly
doesn't matter (is_compat_task() has very few users), but I suspect that
for e.g. ext4 is_32bit_api() it does matter and is currently broken...

What kind of semantics do we want?  "Thread property" one, set when we
set personality on execve(), or "syscall property", like e.g. x86 TIF_IRET
and TS_COMPAT?

Re: [PATCH] broken TASK_SIZE for ia32_aout

[Al Viro]

On Sun, May 06, 2012 at 06:54:51PM +0100, Al Viro wrote:
> everywhere (AFAICS) else: "is this a 32bit syscall" vs. "is this a biarch
> syscall with 32bit pointers, etc.".  On architectures where 32bit syscall
  ^^^^^^^
 "process", of course...

Re: [PATCH] broken TASK_SIZE for ia32_aout

[H. Peter Anvin]

On 05/06/2012 10:54 AM, Al Viro wrote:
> 
> Note that we have different semantics for is_compat_task() on x86 and
> everywhere (AFAICS) else: "is this a 32bit syscall" vs. "is this a biarch
> syscall with 32bit pointers, etc.".  On architectures where 32bit syscall
> can't be issued by 64bit task and vice versa there's no difference, but for
> e.g. sparc there definitely is one.  is_compat_task() there goes by what
> the task is, not what kind of syscall is it trying to make.  It mostly
> doesn't matter (is_compat_task() has very few users), but I suspect that
> for e.g. ext4 is_32bit_api() it does matter and is currently broken...
> 
> What kind of semantics do we want?  "Thread property" one, set when we
> set personality on execve(), or "syscall property", like e.g. x86 TIF_IRET
> and TS_COMPAT?

It depends on the ABI properties of the platform.  The x86 compat ABI is
that any task can issue a compat ABI request and get a compat ABI
response (a 64-bit task can call int $0x80 for an ia32 syscall
invocation, or use syscall with either an x86-64 or and x32 system call
number.)  So is_compat_task() returns the current system call mode of
the task, because that is what downstream users need.  One of the
biggest users is the input subsystem, which earns the black mark for
worst possible ABI design, and that definitely depends on the system
call type being invoked.

	-hpa


-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [PATCH] broken TASK_SIZE for ia32_aout

[Al Viro]

On Sun, May 06, 2012 at 10:58:49AM -0700, H. Peter Anvin wrote:

> > What kind of semantics do we want?  "Thread property" one, set when we
> > set personality on execve(), or "syscall property", like e.g. x86 TIF_IRET
> > and TS_COMPAT?
> 
> It depends on the ABI properties of the platform.  The x86 compat ABI is
> that any task can issue a compat ABI request and get a compat ABI
> response (a 64-bit task can call int $0x80 for an ia32 syscall
> invocation, or use syscall with either an x86-64 or and x32 system call
> number.)  So is_compat_task() returns the current system call mode of
> the task, because that is what downstream users need.  One of the
> biggest users is the input subsystem, which earns the black mark for
> worst possible ABI design, and that definitely depends on the system
> call type being invoked.

Umm...  Let me restate that question: is there ever a case when it would
_not_ be a syscall property?  I.e. when both 64bit and 32bit syscalls are
possible for a given process *and* callers of is_compat_task() care about
the kind of process and not the kind of syscall?

Is e.g. sparc behaviour ("what kind of process it is, regardless of whether
we are issuing a 32bit or a 64bit syscall") correct?

Sure, on a platform where the possible kind of syscall is a function of
process' personality, a thread property can be a used to tell which
kind of syscall we are in.

Re: [PATCH] broken TASK_SIZE for ia32_aout

[H. Peter Anvin]

I would argue Sparc is not correct here but I am not a Sparc expert.

Al Viro <viro@ZenIV.linux.org.uk> wrote:

>On Sun, May 06, 2012 at 10:58:49AM -0700, H. Peter Anvin wrote:
>
>> > What kind of semantics do we want?  "Thread property" one, set when
>we
>> > set personality on execve(), or "syscall property", like e.g. x86
>TIF_IRET
>> > and TS_COMPAT?
>> 
>> It depends on the ABI properties of the platform.  The x86 compat ABI
>is
>> that any task can issue a compat ABI request and get a compat ABI
>> response (a 64-bit task can call int $0x80 for an ia32 syscall
>> invocation, or use syscall with either an x86-64 or and x32 system
>call
>> number.)  So is_compat_task() returns the current system call mode of
>> the task, because that is what downstream users need.  One of the
>> biggest users is the input subsystem, which earns the black mark for
>> worst possible ABI design, and that definitely depends on the system
>> call type being invoked.
>
>Umm...  Let me restate that question: is there ever a case when it
>would
>_not_ be a syscall property?  I.e. when both 64bit and 32bit syscalls
>are
>possible for a given process *and* callers of is_compat_task() care
>about
>the kind of process and not the kind of syscall?
>
>Is e.g. sparc behaviour ("what kind of process it is, regardless of
>whether
>we are issuing a 32bit or a 64bit syscall") correct?
>
>Sure, on a platform where the possible kind of syscall is a function of
>process' personality, a thread property can be a used to tell which
>kind of syscall we are in.

-- 
Sent from my mobile phone. Please excuse brevity and lack of formatting.

Re: [PATCH] broken TASK_SIZE for ia32_aout

[Al Viro]

On Sun, May 06, 2012 at 11:48:15AM -0700, H. Peter Anvin wrote:
> Al Viro <viro@ZenIV.linux.org.uk> wrote:
> >Umm...  Let me restate that question: is there ever a case when it
> >would
> >_not_ be a syscall property?  I.e. when both 64bit and 32bit syscalls
> >are
> >possible for a given process *and* callers of is_compat_task() care
> >about
> >the kind of process and not the kind of syscall?
> >
> >Is e.g. sparc behaviour ("what kind of process it is, regardless of
> >whether
> >we are issuing a 32bit or a 64bit syscall") correct?
> >
> >Sure, on a platform where the possible kind of syscall is a function of
> >process' personality, a thread property can be a used to tell which
> >kind of syscall we are in.

> I would argue Sparc is not correct here but I am not a Sparc expert.

Dave?

Re: [PATCH] broken TASK_SIZE for ia32_aout

[David Miller]

From: "H. Peter Anvin" <hpa@zytor.com>
Date: Sun, 06 May 2012 11:48:15 -0700

> I would argue Sparc is not correct here but I am not a Sparc expert.

I can easily make sparc take on the x86 behavior if things like the
input layer really need it.

However, isn't the input layer case a case where compat==native
on anything other than x86 because it's a u64 alignment issue?

Re: [PATCH] broken TASK_SIZE for ia32_aout

[H. Peter Anvin]

No.  The input layer exports pointers in structures on the read/write paths, and bitmasks encoded in ASCII strings which depend on sizeof(long).  The stupid... it burns...

David Miller <davem@davemloft.net> wrote:

>From: "H. Peter Anvin" <hpa@zytor.com>
>Date: Sun, 06 May 2012 11:48:15 -0700
>
>> I would argue Sparc is not correct here but I am not a Sparc expert.
>
>I can easily make sparc take on the x86 behavior if things like the
>input layer really need it.
>
>However, isn't the input layer case a case where compat==native
>on anything other than x86 because it's a u64 alignment issue?

-- 
Sent from my mobile phone. Please excuse brevity and lack of formatting.

Re: [PATCH] broken TASK_SIZE for ia32_aout

[Al Viro]

On Sun, May 06, 2012 at 04:32:43PM -0400, David Miller wrote:
> From: "H. Peter Anvin" <hpa@zytor.com>
> Date: Sun, 06 May 2012 11:48:15 -0700
> 
> > I would argue Sparc is not correct here but I am not a Sparc expert.
> 
> I can easily make sparc take on the x86 behavior if things like the
> input layer really need it.

BTW, in related area: take a look at syscall_get_arguments().  AFAICS,
it's rather odd -
#ifdef CONFIG_SPARC64
        if (test_tsk_thread_flag(task, TIF_32BIT))
                zero_extend = 1;
#endif
bit looks dubious.  What happens when 32bit task makes a 64bit syscall?
Do we really want to drop upper 32 bits of all arguments in the copy we
are building?

Re: [PATCH] broken TASK_SIZE for ia32_aout

[David Miller]

From: Al Viro <viro@ZenIV.linux.org.uk>
Date: Mon, 7 May 2012 00:32:34 +0100

> bit looks dubious.  What happens when 32bit task makes a 64bit syscall?
> Do we really want to drop upper 32 bits of all arguments in the copy we
> are building?

I basically never intended to support 32-bit tasks making 64-bit
system calls except in extremely limited situations such as ptrace()
so that a 32-bit gdb could debug 64-bit tasks.

Do we really have to support crap like this?

Re: [PATCH] broken TASK_SIZE for ia32_aout

[David Miller]

From: "H. Peter Anvin" <hpa@zytor.com>
Date: Sun, 06 May 2012 13:51:30 -0700

> No.  The input layer exports pointers in structures on the
> read/write paths, and bitmasks encoded in ASCII strings which depend
> on sizeof(long).  The stupid... it burns...

So there has to be a hack in every userland application wherein we
teach it no only about this issue, but add special code to teach it
how to make 64-bit system call traps.

FOR EVERY CPU TYPE?

I'd rather wait for a newer less broken input API to be added, rather
than go through the pain of having to support 64-bit system calls in
32-bit apps.

Nobody on sparc has noticed this yet, and I doubt anyone is going to
care meanwhile.

Re: [PATCH] broken TASK_SIZE for ia32_aout

[H. Peter Anvin]

Why not just make it illegal to make the system calls you don't care about then, to prevent any security issue?

David Miller <davem@davemloft.net> wrote:

>From: "H. Peter Anvin" <hpa@zytor.com>
>Date: Sun, 06 May 2012 13:51:30 -0700
>
>> No.  The input layer exports pointers in structures on the
>> read/write paths, and bitmasks encoded in ASCII strings which depend
>> on sizeof(long).  The stupid... it burns...
>
>So there has to be a hack in every userland application wherein we
>teach it no only about this issue, but add special code to teach it
>how to make 64-bit system call traps.
>
>FOR EVERY CPU TYPE?
>
>I'd rather wait for a newer less broken input API to be added, rather
>than go through the pain of having to support 64-bit system calls in
>32-bit apps.
>
>Nobody on sparc has noticed this yet, and I doubt anyone is going to
>care meanwhile.

-- 
Sent from my mobile phone. Please excuse brevity and lack of formatting.

Re: [PATCH] broken TASK_SIZE for ia32_aout

[Al Viro]

On Sun, May 06, 2012 at 07:38:14PM -0400, David Miller wrote:
> From: Al Viro <viro@ZenIV.linux.org.uk>
> Date: Mon, 7 May 2012 00:32:34 +0100
> 
> > bit looks dubious.  What happens when 32bit task makes a 64bit syscall?
> > Do we really want to drop upper 32 bits of all arguments in the copy we
> > are building?
> 
> I basically never intended to support 32-bit tasks making 64-bit
> system calls except in extremely limited situations such as ptrace()
> so that a 32-bit gdb could debug 64-bit tasks.
> 
> Do we really have to support crap like this?

Hey, I'm all for getting rid of that...

Re: [PATCH] broken TASK_SIZE for ia32_aout

[Al Viro]

On Sun, May 06, 2012 at 06:54:51PM +0100, Al Viro wrote:
> On Sun, May 06, 2012 at 10:16:11AM -0700, Linus Torvalds wrote:
> > On Sun, May 6, 2012 at 9:20 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> > > Setting TIF_IA32 in load_aout_binary() used to be enough; these days
> > > TASK_SIZE is controlled by TIF_ADDR32 and that one doesn't get set
> > > there. ?Switch to use of set_personality_ia32()...
> > 
> > Applied. Just out of curiosity, how did you notice? Just looking at
> > TIF_IA32 usage, or do you actually have some old app?
> 
> Putting together an idiot's guide to thread flags ;-)

BTW, in process something else got found:
---
TIF_IRET(i386) is used to force return to userland via iret, even
if we'd entered the kernel via sysenter.  This, to put it mildly,
is a fucking ugly hack.  Essentially, it acts as a silent duplicate of
NOTIFY_RESUME.  do_notify_resume() clears it (as it clears NOTIFY_RESUME
itself).  And i386 has glue treat it as something indistinguishable from
NOTIFY_RESUME, never mentioning either explicitly.  _Any_ path going
through do_notify_resume() there will leave via iret - sysret is for fast
path only.  So setting TIF_IRET (or, for that matter, TIF_NOTIFY_RESUME)
has an effect of forcing exit via iret.

That had been obfuscated just enough to have it copied to hexagon, m32r,
microblaze and xtensa.  None of those ever set it.  Or have an analog of
the x86 issues that make forcing exit via iret needed there.  Or give
a flying fuck for all that crap anyway.  Pure and simple cargo-cult
programming.

IMO we should kill it everywhere, starting at i386.  There we can simply
set TIF_NOTIFY_RESUME instead.
---

Comments?  It's _not_ 3.4.0 material, obviously, but unless I'm missing
something, killing that thing would be a a Good Thing(tm), especially
since reconstructing the reasons it works at all really takes a non-trivial
amount of crawling throught entry_32.S...

Re: [PATCH] broken TASK_SIZE for ia32_aout

[David Miller]

From: "H. Peter Anvin" <hpa@zytor.com>
Date: Sun, 06 May 2012 16:48:00 -0700

> Why not just make it illegal to make the system calls you don't care
> about then, to prevent any security issue?

I'll take a closer look tomorrow.

BTW, when did you start top-posting? :-/

Re: [PATCH] broken TASK_SIZE for ia32_aout

[H. Peter Anvin]

On 05/06/2012 07:56 PM, David Miller wrote:
> From: "H. Peter Anvin" <hpa@zytor.com>
> Date: Sun, 06 May 2012 16:48:00 -0700
> 
>> Why not just make it illegal to make the system calls you don't care
>> about then, to prevent any security issue?
> 
> I'll take a closer look tomorrow.
> 
> BTW, when did you start top-posting? :-/

Just when on a cell phone...

	-hpa