broken TASK_SIZE for ia32_aout

PATCH Applied Al Viro 4 participants 20 messages May 6, 2012 – May 7, 2012 x86

Share & tools

Al Viro

author

2012-05-06 16:20 UTC #0 lore raw

PATCH 1/1 broken TASK_SIZE for ia32_aout

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 <[email protected]>

---

arch/x86/ia32/ia32_aout.c +1 -2

index 4824fb4..07b3a68 100644
@@ -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);

Linus Torvalds

Applied

2012-05-06 17:16 UTC #1 lore raw

Quoted 3 lines · 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()...

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

Linus

H. Peter Anvin

reviewer

2012-05-06 17:37 UTC #2 lore raw

Quoted 8 lines · Linus Torvalds

Quoted 3 lines · 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()...
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

Signature · 2 lines

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

Al Viro

author

2012-05-06 17:54 UTC #3 lore raw

In reply to Linus Torvalds

Quoted 7 lines · Linus Torvalds

Quoted 3 lines · 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()...
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?

Al Viro

author

2012-05-06 17:56 UTC #4 lore raw

Quoted 2 lines · Al Viro

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...

H. Peter Anvin

reviewer

2012-05-06 17:58 UTC #5 lore raw

In reply to Al Viro

Quoted 10 lines · Al Viro

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...

Quoted 3 lines

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

Signature · 2 lines

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

Al Viro

author

2012-05-06 18:46 UTC #6 lore raw

Quoted 4 lines · H. Peter Anvin

Quoted 3 lines
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?

Quoted 9 lines

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.

H. Peter Anvin

reviewer

2012-05-06 18:48 UTC #7 lore raw

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

Quoted 35 lines · Al Viro

Quoted 1 line · H. Peter Anvin
Quoted 1 line
What kind of semantics do we want? "Thread property" one, set when
we
Quoted 1 line
Quoted 1 line
set personality on execve(), or "syscall property", like e.g. x86
TIF_IRET
Quoted 3 lines
Quoted 1 line
and TS_COMPAT?
It depends on the ABI properties of the platform. The x86 compat ABI
is
Quoted 3 lines
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
Quoted 5 lines
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.

Signature · 1 line

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

Al Viro

author

2012-05-06 19:58 UTC #8 lore raw

Quoted 13 lines · H. Peter Anvin

Quoted 12 lines · Al Viro
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?

Quoted 5 lines

Quoted 3 lines
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?

David Miller

2012-05-06 20:32 UTC #9 lore raw

In reply to H. Peter Anvin

From: "H. Peter Anvin" <[email protected]>
Date: Sun, 06 May 2012 11:48:15 -0700

Quoted 1 line

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?

H. Peter Anvin

reviewer

2012-05-06 20:51 UTC #10 lore raw

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...

Quoted 10 lines · David Miller

From: "H. Peter Anvin" <[email protected]>
Date: Sun, 06 May 2012 11:48:15 -0700
Quoted 1 line
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?

Signature · 1 line

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

David Miller

2012-05-06 23:40 UTC #11 lore raw

From: "H. Peter Anvin" <[email protected]>
Date: Sun, 06 May 2012 13:51:30 -0700

Quoted 3 lines

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.

H. Peter Anvin

reviewer

2012-05-06 23:48 UTC #12 lore raw

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

Quoted 19 lines · David Miller

From: "H. Peter Anvin" <[email protected]>
Date: Sun, 06 May 2012 13:51:30 -0700
Quoted 3 lines
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.

Signature · 1 line

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

David Miller

2012-05-07 02:56 UTC #13 lore raw

From: "H. Peter Anvin" <[email protected]>
Date: Sun, 06 May 2012 16:48:00 -0700

Quoted 2 lines

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? :-/

H. Peter Anvin

reviewer

2012-05-07 03:06 UTC #14 lore raw

Quoted 9 lines · David Miller

From: "H. Peter Anvin" <[email protected]>
Date: Sun, 06 May 2012 16:48:00 -0700
Quoted 2 lines
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

Al Viro

author

2012-05-06 23:32 UTC #15 lore raw

In reply to David Miller

Quoted 7 lines · David Miller

From: "H. Peter Anvin" <[email protected]>
Date: Sun, 06 May 2012 11:48:15 -0700
Quoted 1 line
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?

David Miller

2012-05-06 23:38 UTC #16 lore raw

From: Al Viro <[email protected]>
Date: Mon, 7 May 2012 00:32:34 +0100

Quoted 3 lines

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?

Al Viro

author

2012-05-07 00:13 UTC #17 lore raw

Quoted 7 lines · David Miller

From: Al Viro <[email protected]>
Date: Mon, 7 May 2012 00:32:34 +0100
Quoted 3 lines
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?

Quoted 5 lines

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...

Al Viro

author

2012-05-07 00:24 UTC #18 lore raw

In reply to Al Viro

Quoted 10 lines · Al Viro

Quoted 7 lines · Linus Torvalds
Quoted 3 lines · 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()...
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...

tip-bot for Al Viro

author

2012-05-06 17:23 UTC #19 lore raw

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

In reply to Al Viro

Commit-ID:  2be8e9ec677d6dfd4268b80e1ccd7d21777b86e3
Gitweb:     http://git.kernel.org/tip/2be8e9ec677d6dfd4268b80e1ccd7d21777b86e3
Author:     Al Viro <[email protected]>
AuthorDate: Sun, 6 May 2012 17:20:00 +0100
Committer:  H. Peter Anvin <[email protected]>
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 <[email protected]>

Link http://lkml.kernel.org/r/[email protected] open here

Signed-off-by H. Peter Anvin <[email protected]>

---
 arch/x86/ia32/ia32_aout.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

arch/x86/ia32/ia32_aout.c +1 -2

index d511d95..4c979cf 100644
@@ -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);