After my changes to mmap(), its code now relies on the bitness of
performing syscall. According to that, it chooses the base of allocation:
mmap_base for 64-bit mmap() and mmap_compat_base for 32-bit syscall.
It was done by:
commit 1b028f784e8c ("x86/mm: Introduce mmap_compat_base() for
32-bit mmap()").
The code afterwards relies on in_compat_syscall() returning true for
32-bit syscalls. It's usually so while we're in context of application
that does 32-bit syscalls. But during exec() it is not valid for x32 ELF.
The reason is that the application hasn't yet done any syscall, so x32
bit has not being set.
That results in -ENOMEM for x32 ELF files as there fired BAD_ADDR()
in elf_map(), that is called from do_execve()->load_elf_binary().
For i386 ELFs it works as SET_PERSONALITY() sets TS_COMPAT flag.
Set x32 bit before first return to userspace, during setting personality
at exec(). This way we can rely on in_compat_syscall() during exec().
Do also the reverse: drop x32 syscall bit at SET_PERSONALITY for 64-bits.
Fixes: commit 1b028f784e8c ("x86/mm: Introduce mmap_compat_base() for
32-bit mmap()")
Cc: [email protected]
Cc: [email protected]
Cc: Andrei Vagin <[email protected]>
Cc: Cyrill Gorcunov <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: "Kirill A. Shutemov" <[email protected]>
Cc: [email protected]
Cc: H. Peter Anvin <[email protected]>
Cc: Andy Lutomirski <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Reported-by: Adam Borowski <[email protected]>
Signed-off-by: Dmitry Safonov <[email protected]>
---
v2:
- specifying mmap() allocation path which failed during exec()
- fix comment style
v3:
- clear x32 syscall flag during x32 -> x86-64 exec() (thanks, HPA).
arch/x86/kernel/process_64.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
index d6b784a5520d..b03f186369eb 100644
--- a/arch/x86/kernel/process_64.c
+++ b/arch/x86/kernel/process_64.c
@@ -493,6 +493,8 @@ void set_personality_64bit(void)
clear_thread_flag(TIF_IA32);
clear_thread_flag(TIF_ADDR32);
clear_thread_flag(TIF_X32);
+ /* Drop x32 syscall bit, so in_compat_syscall() will return false. */
+ task_pt_regs(current)->orig_ax &= ~__X32_SYSCALL_BIT;
/* Ensure the corresponding mm is not marked. */
if (current->mm)
@@ -519,8 +521,14 @@ void set_personality_ia32(bool x32)
if (current->mm)
current->mm->context.ia32_compat = TIF_X32;
current->personality &= ~READ_IMPLIES_EXEC;
- /* in_compat_syscall() uses the presence of the x32
- syscall bit flag to determine compat status */
+ /*
+ * in_compat_syscall() uses the presence of the x32
+ * syscall bit flag to determine compat status.
+ * On the bitness of syscall relies x86 mmap() code,
+ * so set x32 syscall bit right here to make
+ * in_compat_syscall() work during exec().
+ */
+ task_pt_regs(current)->orig_ax |= __X32_SYSCALL_BIT;
current->thread.status &= ~TS_COMPAT;
} else {
set_thread_flag(TIF_IA32);
--
2.12.0
On Tue, Mar 21, 2017 at 08:47:11PM +0300, Dmitry Safonov wrote:
> After my changes to mmap(), its code now relies on the bitness of
> performing syscall. According to that, it chooses the base of allocation:
> mmap_base for 64-bit mmap() and mmap_compat_base for 32-bit syscall.
> It was done by:
> commit 1b028f784e8c ("x86/mm: Introduce mmap_compat_base() for
> 32-bit mmap()").
>
> The code afterwards relies on in_compat_syscall() returning true for
> 32-bit syscalls. It's usually so while we're in context of application
> that does 32-bit syscalls. But during exec() it is not valid for x32 ELF.
> The reason is that the application hasn't yet done any syscall, so x32
> bit has not being set.
> That results in -ENOMEM for x32 ELF files as there fired BAD_ADDR()
> in elf_map(), that is called from do_execve()->load_elf_binary().
> For i386 ELFs it works as SET_PERSONALITY() sets TS_COMPAT flag.
>
> Set x32 bit before first return to userspace, during setting personality
> at exec(). This way we can rely on in_compat_syscall() during exec().
> Do also the reverse: drop x32 syscall bit at SET_PERSONALITY for 64-bits.
>
> Fixes: commit 1b028f784e8c ("x86/mm: Introduce mmap_compat_base() for
> 32-bit mmap()")
Tested:
with bash:x32, mksh:amd64, posh:i386, zsh:armhf (binfmt:qemu), fork+exec
works for every parent-child combination.
Contrary to my naive initial reading of your fix, mixing syscalls from a
process of the wrong ABI also works as it did before. While using a glibc
wrapper will call the right version, x32 processes calling amd64 syscalls is
surprisingly common -- this brings seccomp joy.
I've attached a freestanding test case for write() and mmap(); it's
freestanding asm as most of you don't have an x32 toolchain at hand, sorry
for unfriendly error messages.
So with these two patches:
x86/tls: Forcibly set the accessed bit in TLS segments
x86/mm: set x32 syscall bit in SET_PERSONALITY()
everything appears to be fine.
--
⢀⣴⠾⠻⢶⣦⠀ Meow!
⣾⠁⢠⠒⠀⣿⡁
⢿⡄⠘⠷⠚⠋⠀ Collisions shmolisions, let's see them find a collision or second
⠈⠳⣄⠀⠀⠀⠀ preimage for double rot13!
On March 21, 2017 2:16:48 PM PDT, Adam Borowski <[email protected]> wrote:
>On Tue, Mar 21, 2017 at 08:47:11PM +0300, Dmitry Safonov wrote:
>> After my changes to mmap(), its code now relies on the bitness of
>> performing syscall. According to that, it chooses the base of
>allocation:
>> mmap_base for 64-bit mmap() and mmap_compat_base for 32-bit syscall.
>> It was done by:
>> commit 1b028f784e8c ("x86/mm: Introduce mmap_compat_base() for
>> 32-bit mmap()").
>>
>> The code afterwards relies on in_compat_syscall() returning true for
>> 32-bit syscalls. It's usually so while we're in context of
>application
>> that does 32-bit syscalls. But during exec() it is not valid for x32
>ELF.
>> The reason is that the application hasn't yet done any syscall, so
>x32
>> bit has not being set.
>> That results in -ENOMEM for x32 ELF files as there fired BAD_ADDR()
>> in elf_map(), that is called from do_execve()->load_elf_binary().
>> For i386 ELFs it works as SET_PERSONALITY() sets TS_COMPAT flag.
>>
>> Set x32 bit before first return to userspace, during setting
>personality
>> at exec(). This way we can rely on in_compat_syscall() during exec().
>> Do also the reverse: drop x32 syscall bit at SET_PERSONALITY for
>64-bits.
>>
>> Fixes: commit 1b028f784e8c ("x86/mm: Introduce mmap_compat_base() for
>> 32-bit mmap()")
>
>Tested:
>with bash:x32, mksh:amd64, posh:i386, zsh:armhf (binfmt:qemu),
>fork+exec
>works for every parent-child combination.
>
>Contrary to my naive initial reading of your fix, mixing syscalls from
>a
>process of the wrong ABI also works as it did before. While using a
>glibc
>wrapper will call the right version, x32 processes calling amd64
>syscalls is
>surprisingly common -- this brings seccomp joy.
>
>I've attached a freestanding test case for write() and mmap(); it's
>freestanding asm as most of you don't have an x32 toolchain at hand,
>sorry
>for unfriendly error messages.
>
>So with these two patches:
>x86/tls: Forcibly set the accessed bit in TLS segments
>x86/mm: set x32 syscall bit in SET_PERSONALITY()
>everything appears to be fine.
What userspace is that? Is this syscall(3) (ab)users or incorrectly ported to x32 software?
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
2017-03-22 0:16 GMT+03:00 Adam Borowski <[email protected]>:
> On Tue, Mar 21, 2017 at 08:47:11PM +0300, Dmitry Safonov wrote:
>> After my changes to mmap(), its code now relies on the bitness of
>> performing syscall. According to that, it chooses the base of allocation:
>> mmap_base for 64-bit mmap() and mmap_compat_base for 32-bit syscall.
>> It was done by:
>> commit 1b028f784e8c ("x86/mm: Introduce mmap_compat_base() for
>> 32-bit mmap()").
>>
>> The code afterwards relies on in_compat_syscall() returning true for
>> 32-bit syscalls. It's usually so while we're in context of application
>> that does 32-bit syscalls. But during exec() it is not valid for x32 ELF.
>> The reason is that the application hasn't yet done any syscall, so x32
>> bit has not being set.
>> That results in -ENOMEM for x32 ELF files as there fired BAD_ADDR()
>> in elf_map(), that is called from do_execve()->load_elf_binary().
>> For i386 ELFs it works as SET_PERSONALITY() sets TS_COMPAT flag.
>>
>> Set x32 bit before first return to userspace, during setting personality
>> at exec(). This way we can rely on in_compat_syscall() during exec().
>> Do also the reverse: drop x32 syscall bit at SET_PERSONALITY for 64-bits.
>>
>> Fixes: commit 1b028f784e8c ("x86/mm: Introduce mmap_compat_base() for
>> 32-bit mmap()")
>
> Tested:
> with bash:x32, mksh:amd64, posh:i386, zsh:armhf (binfmt:qemu), fork+exec
> works for every parent-child combination.
>
> Contrary to my naive initial reading of your fix, mixing syscalls from a
> process of the wrong ABI also works as it did before. While using a glibc
> wrapper will call the right version, x32 processes calling amd64 syscalls is
> surprisingly common -- this brings seccomp joy.
JFI: x32 mmap() syscall in 64 ELF should work even better - it has returned
addresses over 4Gb in ia32 mmap()s, so I expect it did the same in x32 top-down
allocation. (I guess you've mentioned the fixes-for patch).
So the thing to check not also that mmap() returned address, but at least
verify-dereference it with `mov' e.g. (or better - to parse /proc/self/maps)
> I've attached a freestanding test case for write() and mmap(); it's
> freestanding asm as most of you don't have an x32 toolchain at hand, sorry
> for unfriendly error messages.
>
> So with these two patches:
> x86/tls: Forcibly set the accessed bit in TLS segments
> x86/mm: set x32 syscall bit in SET_PERSONALITY()
> everything appears to be fine.
Big thanks for the testing work, Adam!
--
Dmitry
On Tue, 21 Mar 2017, Dmitry Safonov wrote:
> v3:
> - clear x32 syscall flag during x32 -> x86-64 exec() (thanks, HPA).
For correctness sake, this wants to be cleared in the IA32 path as
well. It's not causing any harm, but ....
I'll amend the patch.
Thanks,
tglx
On March 21, 2017 3:21:13 PM PDT, Thomas Gleixner <[email protected]> wrote:
>On Tue, 21 Mar 2017, Dmitry Safonov wrote:
>> v3:
>> - clear x32 syscall flag during x32 -> x86-64 exec() (thanks, HPA).
>
>For correctness sake, this wants to be cleared in the IA32 path as
>well. It's not causing any harm, but ....
>
>I'll amend the patch.
>
>Thanks,
>
> tglx
Since the i386 syscall namespace is totally separate (and different), should we simply change the system call number to the appropriate sys_execve number?
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
On Tue, 21 Mar 2017, [email protected] wrote:
> On March 21, 2017 3:21:13 PM PDT, Thomas Gleixner <[email protected]> wrote:
> >On Tue, 21 Mar 2017, Dmitry Safonov wrote:
> >> v3:
> >> - clear x32 syscall flag during x32 -> x86-64 exec() (thanks, HPA).
> >
> >For correctness sake, this wants to be cleared in the IA32 path as
> >well. It's not causing any harm, but ....
> >
> >I'll amend the patch.
> >
> >Thanks,
> >
> > tglx
>
> Since the i386 syscall namespace is totally separate (and different),
> should we simply change the system call number to the appropriate
> sys_execve number?
That should work as well and would be more intuitive.
Thanks,
tglx
On 03/22/2017 01:34 AM, Thomas Gleixner wrote:
> On Tue, 21 Mar 2017, [email protected] wrote:
>
>> On March 21, 2017 3:21:13 PM PDT, Thomas Gleixner <[email protected]> wrote:
>>> On Tue, 21 Mar 2017, Dmitry Safonov wrote:
>>>> v3:
>>>> - clear x32 syscall flag during x32 -> x86-64 exec() (thanks, HPA).
>>>
>>> For correctness sake, this wants to be cleared in the IA32 path as
>>> well. It's not causing any harm, but ....
>>>
>>> I'll amend the patch.
Indeed, thanks!
>> Since the i386 syscall namespace is totally separate (and different),
>> should we simply change the system call number to the appropriate
>> sys_execve number?
>
> That should work as well and would be more intuitive.
Not sure that I got the idea correctly, something like this?
I haven't find any easy way to get compat syscall nr like
__NR_compat_execve, so I defined it there.
I'll resend v4 with the fixup if that's what was expected.
--->8---
diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
index b03f186369eb..c58ac0bff2f1 100644
--- a/arch/x86/kernel/process_64.c
+++ b/arch/x86/kernel/process_64.c
@@ -507,6 +507,8 @@ void set_personality_64bit(void)
current->personality &= ~READ_IMPLIES_EXEC;
}
+#define __NR_ia32_execve 11
+
void set_personality_ia32(bool x32)
{
/* inherit personality from parent */
@@ -537,6 +539,7 @@ void set_personality_ia32(bool x32)
current->mm->context.ia32_compat = TIF_IA32;
current->personality |= force_personality32;
/* Prepare the first "return" to user space */
+ task_pt_regs(current)->orig_ax = __NR_ia32_execve;
current->thread.status |= TS_COMPAT;
}
}
On 03/22/2017 01:21 AM, Thomas Gleixner wrote:
> On Tue, 21 Mar 2017, Dmitry Safonov wrote:
>> v3:
>> - clear x32 syscall flag during x32 -> x86-64 exec() (thanks, HPA).
>
> For correctness sake, this wants to be cleared in the IA32 path as
> well. It's not causing any harm, but ....
>
> I'll amend the patch.
So, just a gentle reminder about this problem.
Should I resend v4 with clearing x32 bit in ia32 path?
Or should I resend with this fixup:
https://lkml.org/lkml/2017/3/22/343
The fixup doesn't look as simple as clearing x32 syscall bit, but I may
be wrong.
--
Dmitry
On Tue, 28 Mar 2017, Dmitry Safonov wrote:
> On 03/22/2017 01:21 AM, Thomas Gleixner wrote:
> > On Tue, 21 Mar 2017, Dmitry Safonov wrote:
> > > v3:
> > > - clear x32 syscall flag during x32 -> x86-64 exec() (thanks, HPA).
> >
> > For correctness sake, this wants to be cleared in the IA32 path as
> > well. It's not causing any harm, but ....
> >
> > I'll amend the patch.
>
> So, just a gentle reminder about this problem.
> Should I resend v4 with clearing x32 bit in ia32 path?
> Or should I resend with this fixup:
> https://lkml.org/lkml/2017/3/22/343
>
> The fixup doesn't look as simple as clearing x32 syscall bit, but I may
> be wrong.
Something like the below should set it correctly for all possible
scenarios.
Thanks,
tglx
8<------------------
arch/x86/kernel/process_64.c | 63 ++++++++++++++++++++++++++++---------------
1 file changed, 42 insertions(+), 21 deletions(-)
--- a/arch/x86/kernel/process_64.c
+++ b/arch/x86/kernel/process_64.c
@@ -494,6 +494,8 @@ void set_personality_64bit(void)
clear_thread_flag(TIF_IA32);
clear_thread_flag(TIF_ADDR32);
clear_thread_flag(TIF_X32);
+ /* Pretend that this comes from a 64bit execve */
+ task_pt_regs(current)->orig_ax = __NR_execve;
/* Ensure the corresponding mm is not marked. */
if (current->mm)
@@ -506,32 +508,51 @@ void set_personality_64bit(void)
current->personality &= ~READ_IMPLIES_EXEC;
}
-void set_personality_ia32(bool x32)
+static void __set_personality_x32(void)
+{
+#ifdef CONFIG_X86_X32
+ clear_thread_flag(TIF_IA32);
+ set_thread_flag(TIF_X32);
+ if (current->mm)
+ current->mm->context.ia32_compat = TIF_X32;
+ current->personality &= ~READ_IMPLIES_EXEC;
+ /*
+ * in_compat_syscall() uses the presence of the x32
+ * syscall bit flag to determine compat status.
+ * The x86 mmap() code relies on the syscall bitness
+ * so set x32 syscall bit right here to make
+ * in_compat_syscall() work during exec().
+ *
+ * Pretend to come from a x32 execve.
+ */
+ task_pt_regs(current)->orig_ax = __NR_x32_execve | __X32_SYSCALL_BIT;
+ current->thread.status &= ~TS_COMPAT;
+#endif
+}
+
+static void __set_personality_ia32(void)
{
- /* inherit personality from parent */
+#ifdef CONFIG_COMPAT_32
+ set_thread_flag(TIF_IA32);
+ clear_thread_flag(TIF_X32);
+ if (current->mm)
+ current->mm->context.ia32_compat = TIF_IA32;
+ current->personality |= force_personality32;
+ /* Prepare the first "return" to user space */
+ task_pt_regs(current)->orig_ax = __NR_ia32_execve;
+ current->thread.status |= TS_COMPAT;
+#endif
+}
+void set_personality_ia32(bool x32)
+{
/* Make sure to be in 32bit mode */
set_thread_flag(TIF_ADDR32);
- /* Mark the associated mm as containing 32-bit tasks. */
- if (x32) {
- clear_thread_flag(TIF_IA32);
- set_thread_flag(TIF_X32);
- if (current->mm)
- current->mm->context.ia32_compat = TIF_X32;
- current->personality &= ~READ_IMPLIES_EXEC;
- /* in_compat_syscall() uses the presence of the x32
- syscall bit flag to determine compat status */
- current->thread.status &= ~TS_COMPAT;
- } else {
- set_thread_flag(TIF_IA32);
- clear_thread_flag(TIF_X32);
- if (current->mm)
- current->mm->context.ia32_compat = TIF_IA32;
- current->personality |= force_personality32;
- /* Prepare the first "return" to user space */
- current->thread.status |= TS_COMPAT;
- }
+ if (x32)
+ __set_personality_x32();
+ else
+ __set_personality_ia32();
}
EXPORT_SYMBOL_GPL(set_personality_ia32);
On 03/28/2017 03:51 PM, Thomas Gleixner wrote:
> On Tue, 28 Mar 2017, Dmitry Safonov wrote:
>> On 03/22/2017 01:21 AM, Thomas Gleixner wrote:
>>> On Tue, 21 Mar 2017, Dmitry Safonov wrote:
>>>> v3:
>>>> - clear x32 syscall flag during x32 -> x86-64 exec() (thanks, HPA).
>>>
>>> For correctness sake, this wants to be cleared in the IA32 path as
>>> well. It's not causing any harm, but ....
>>>
>>> I'll amend the patch.
>>
>> So, just a gentle reminder about this problem.
>> Should I resend v4 with clearing x32 bit in ia32 path?
>> Or should I resend with this fixup:
>> https://lkml.org/lkml/2017/3/22/343
>>
>> The fixup doesn't look as simple as clearing x32 syscall bit, but I may
>> be wrong.
>
> Something like the below should set it correctly for all possible
> scenarios.
Ok, I'll check the ifdeffery, define __NR_{x32_,ia32_}execve,
test it and resend v4 today or tomorrow.
Thanks.
>
> Thanks,
>
> tglx
>
> 8<------------------
>
> arch/x86/kernel/process_64.c | 63 ++++++++++++++++++++++++++++---------------
> 1 file changed, 42 insertions(+), 21 deletions(-)
>
> --- a/arch/x86/kernel/process_64.c
> +++ b/arch/x86/kernel/process_64.c
> @@ -494,6 +494,8 @@ void set_personality_64bit(void)
> clear_thread_flag(TIF_IA32);
> clear_thread_flag(TIF_ADDR32);
> clear_thread_flag(TIF_X32);
> + /* Pretend that this comes from a 64bit execve */
> + task_pt_regs(current)->orig_ax = __NR_execve;
>
> /* Ensure the corresponding mm is not marked. */
> if (current->mm)
> @@ -506,32 +508,51 @@ void set_personality_64bit(void)
> current->personality &= ~READ_IMPLIES_EXEC;
> }
>
> -void set_personality_ia32(bool x32)
> +static void __set_personality_x32(void)
> +{
> +#ifdef CONFIG_X86_X32
> + clear_thread_flag(TIF_IA32);
> + set_thread_flag(TIF_X32);
> + if (current->mm)
> + current->mm->context.ia32_compat = TIF_X32;
> + current->personality &= ~READ_IMPLIES_EXEC;
> + /*
> + * in_compat_syscall() uses the presence of the x32
> + * syscall bit flag to determine compat status.
> + * The x86 mmap() code relies on the syscall bitness
> + * so set x32 syscall bit right here to make
> + * in_compat_syscall() work during exec().
> + *
> + * Pretend to come from a x32 execve.
> + */
> + task_pt_regs(current)->orig_ax = __NR_x32_execve | __X32_SYSCALL_BIT;
> + current->thread.status &= ~TS_COMPAT;
> +#endif
> +}
> +
> +static void __set_personality_ia32(void)
> {
> - /* inherit personality from parent */
> +#ifdef CONFIG_COMPAT_32
> + set_thread_flag(TIF_IA32);
> + clear_thread_flag(TIF_X32);
> + if (current->mm)
> + current->mm->context.ia32_compat = TIF_IA32;
> + current->personality |= force_personality32;
> + /* Prepare the first "return" to user space */
> + task_pt_regs(current)->orig_ax = __NR_ia32_execve;
> + current->thread.status |= TS_COMPAT;
> +#endif
> +}
>
> +void set_personality_ia32(bool x32)
> +{
> /* Make sure to be in 32bit mode */
> set_thread_flag(TIF_ADDR32);
>
> - /* Mark the associated mm as containing 32-bit tasks. */
> - if (x32) {
> - clear_thread_flag(TIF_IA32);
> - set_thread_flag(TIF_X32);
> - if (current->mm)
> - current->mm->context.ia32_compat = TIF_X32;
> - current->personality &= ~READ_IMPLIES_EXEC;
> - /* in_compat_syscall() uses the presence of the x32
> - syscall bit flag to determine compat status */
> - current->thread.status &= ~TS_COMPAT;
> - } else {
> - set_thread_flag(TIF_IA32);
> - clear_thread_flag(TIF_X32);
> - if (current->mm)
> - current->mm->context.ia32_compat = TIF_IA32;
> - current->personality |= force_personality32;
> - /* Prepare the first "return" to user space */
> - current->thread.status |= TS_COMPAT;
> - }
> + if (x32)
> + __set_personality_x32();
> + else
> + __set_personality_ia32();
> }
> EXPORT_SYMBOL_GPL(set_personality_ia32);
>
>
--
Dmitry