Fix compatible mmap() return pointer over 4Gb

I need those arch_{native,compat}_rnd() to compute separately random factor for mmap() in compat syscalls for 64-bit binaries and vice-versa for native syscall in 32-bit compat binaries. They will be used in the following patches.

Signed-off-by Dmitry Safonov <[email protected]>

---
 arch/x86/mm/mmap.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

arch/x86/mm/mmap.c +10 -3

index d2dc0438d654..42063e787717 100644
@@ -65,20 +65,27 @@ static int mmap_is_legacy(void)
 	return sysctl_legacy_va_layout;
 }
 
-unsigned long arch_mmap_rnd(void)
+#ifdef CONFIG_COMPAT
+static unsigned long arch_compat_rnd(void)
 {
-	unsigned long rnd;
+	return (get_random_long() & ((1UL << mmap_rnd_compat_bits) - 1))
+		<< PAGE_SHIFT;
+}
+#endif
 
-	if (mmap_is_ia32())
+static unsigned long arch_native_rnd(void)
+{
+	return (get_random_long() & ((1UL << mmap_rnd_bits) - 1)) << PAGE_SHIFT;
+}

Quoted 33 lines · Dmitry Safonov

I need those arch_{native,compat}_rnd() to compute separately random factor for mmap() in compat syscalls for 64-bit binaries and vice-versa for native syscall in 32-bit compat binaries. They will be used in the following patches.

Signed-off-by Dmitry Safonov <[email protected]>

---
 arch/x86/mm/mmap.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

arch/x86/mm/mmap.c +10 -3

index d2dc0438d654..42063e787717 100644
@@ -65,20 +65,27 @@ static int mmap_is_legacy(void)
      return sysctl_legacy_va_layout;
 }
 
-unsigned long arch_mmap_rnd(void)
+#ifdef CONFIG_COMPAT
+static unsigned long arch_compat_rnd(void)
 {
-     unsigned long rnd;
+     return (get_random_long() & ((1UL << mmap_rnd_compat_bits) - 1))
+             << PAGE_SHIFT;
+}
+#endif
 
-     if (mmap_is_ia32())
+static unsigned long arch_native_rnd(void)
+{
+     return (get_random_long() & ((1UL << mmap_rnd_bits) - 1)) << PAGE_SHIFT;
+}

arch/x86/mm/mmap.c static int mmap_is_legacy(void) · line 86

Quoted 10 lines

arch/x86/mm/mmap.c +5 -3

+
+unsigned long arch_mmap_rnd(void)
+{
 #ifdef CONFIG_COMPAT
-             rnd = get_random_long() & ((1UL << mmap_rnd_compat_bits) - 1);
-#else
-             rnd = get_random_long() & ((1UL << mmap_rnd_bits) - 1);
+     if (mmap_is_ia32())
+             return arch_compat_rnd();
 #endif

I can't say that I'm thrilled about the ifdeffery this is adding.

But I can't think of a cleaner approach at a quick glance, though - that's generic and arch-specific code intertwined muck. Sad face.

--
Regards/Gruss,
Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

I can't say that I'm thrilled about the ifdeffery this is adding.

But I can't think of a cleaner approach at a quick glance, though - that's generic and arch-specific code intertwined muck. Sad face.

Quoted 4 lines · Borislav Petkov

I can't say that I'm thrilled about the ifdeffery this is adding.

But I can't think of a cleaner approach at a quick glance, though - that's generic and arch-specific code intertwined muck. Sad face.

It's trivial enough to do ....

Thanks,

tglx

---
 arch/x86/mm/mmap.c |   22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

arch/x86/mm/mmap.c +5 -9

@@ -55,6 +55,10 @@ static unsigned long stack_maxrandom_siz
 #define MIN_GAP (128*1024*1024UL + stack_maxrandom_size())
 #define MAX_GAP (TASK_SIZE/6*5)
 
+#ifndef CONFIG_COMPAT
+# define mmap_rnd_compat_bits	mmap_rnd_bits
+#endif
+
 static int mmap_is_legacy(void)
 {
 	if (current->personality & ADDR_COMPAT_LAYOUT)

@@ -66,20 +70,14 @@ static int mmap_is_legacy(void)
 	return sysctl_legacy_va_layout;
 }
 
-unsigned long arch_mmap_rnd(void)
+static unsigned long arch_rnd(unsigned int rndbits)
 {
-	unsigned long rnd;
-
-	if (mmap_is_ia32())
-#ifdef CONFIG_COMPAT
-		rnd = get_random_long() & ((1UL << mmap_rnd_compat_bits) - 1);
-#else
-		rnd = get_random_long() & ((1UL << mmap_rnd_bits) - 1);
-#endif

Quoted 4 lines

I can't say that I'm thrilled about the ifdeffery this is adding.

But I can't think of a cleaner approach at a quick glance, though - that's generic and arch-specific code intertwined muck. Sad face.

It's trivial enough to do ....

Thanks,

tglx

---
 arch/x86/mm/mmap.c |   22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

arch/x86/mm/mmap.c +4 -0

@@ -55,6 +55,10 @@ static unsigned long stack_maxrandom_siz
 #define MIN_GAP (128*1024*1024UL + stack_maxrandom_size())
 #define MAX_GAP (TASK_SIZE/6*5)
 
+#ifndef CONFIG_COMPAT
+# define mmap_rnd_compat_bits  mmap_rnd_bits
+#endif
+
 

From my POV, I can't say that it's clearer to shadow mmap_compat_bits

static int mmap_is_legacy(void)

 {
        if (current->personality & ADDR_COMPAT_LAYOUT)

arch/x86/mm/mmap.c +6 -12

@@ -66,20 +70,14 @@ static int mmap_is_legacy(void)
        return sysctl_legacy_va_layout;
 }
 
-unsigned long arch_mmap_rnd(void)
+static unsigned long arch_rnd(unsigned int rndbits)
 {
-       unsigned long rnd;
-
-       if (mmap_is_ia32())
-#ifdef CONFIG_COMPAT
-               rnd = get_random_long() & ((1UL << mmap_rnd_compat_bits) - 1);
-#else
-               rnd = get_random_long() & ((1UL << mmap_rnd_bits) - 1);
-#endif
-       else
-               rnd = get_random_long() & ((1UL << mmap_rnd_bits) - 1);
+       return (get_random_long() & ((1UL << rndbits) - 1)) << PAGE_SHIFT;
+}
 
-       return rnd << PAGE_SHIFT;
+unsigned long arch_mmap_rnd(void)
+{
+       return arch_rnd(mmap_is_ia32() ? mmap_rnd_compat_bits : mmap_rnd_bits);
 }
 
 static unsigned long mmap_base(unsigned long rnd)

2017-02-10 23:10 GMT+03:00 Thomas Gleixner <[email protected]>:

Quoted 22 lines · Borislav Petkov

Quoted 4 lines

I can't say that I'm thrilled about the ifdeffery this is adding.

But I can't think of a cleaner approach at a quick glance, though - that's generic and arch-specific code intertwined muck. Sad face.

It's trivial enough to do ....

Thanks,

tglx

---
 arch/x86/mm/mmap.c |   22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

arch/x86/mm/mmap.c +0 -0

@@ -55,6 +55,10 @@ static unsigned long stack_maxrandom_siz
 #define MIN_GAP (128*1024*1024UL + stack_maxrandom_size())
 #define MAX_GAP (TASK_SIZE/6*5)
 

Quoted 6 lines

arch/x86/mm/mmap.c +4 -0

+#ifndef CONFIG_COMPAT
+# define mmap_rnd_compat_bits  mmap_rnd_bits
+#endif
+
 

From my POV, I can't say that it's clearer to shadow mmap_compat_bits

like that then to have two functions with native/compat names. But if you insist, I'll resend patches set with your version.

Quoted 23 lines · Dmitry Safonov

2017-02-10 23:10 GMT+03:00 Thomas Gleixner <[email protected]>:

Quoted 22 lines · Borislav Petkov

Quoted 4 lines

I can't say that I'm thrilled about the ifdeffery this is adding.

But I can't think of a cleaner approach at a quick glance, though - that's generic and arch-specific code intertwined muck. Sad face.

It's trivial enough to do ....

Thanks,

tglx

---
 arch/x86/mm/mmap.c |   22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

arch/x86/mm/mmap.c +0 -0

@@ -55,6 +55,10 @@ static unsigned long stack_maxrandom_siz
 #define MIN_GAP (128*1024*1024UL + stack_maxrandom_size())
 #define MAX_GAP (TASK_SIZE/6*5)
 

Quoted 8 lines

Quoted 6 lines

arch/x86/mm/mmap.c +4 -0

+#ifndef CONFIG_COMPAT
+# define mmap_rnd_compat_bits  mmap_rnd_bits
+#endif
+
 

From my POV, I can't say that it's clearer to shadow mmap_compat_bits

like that then to have two functions with native/compat names. But if you insist, I'll resend patches set with your version.

You can make that

#ifdef CONFIG_64BIT

# define mmap32_rnd_bits  mmap_compat_rnd_bits
# define mmap64_rnd_bits  mmap_rnd_bits
#else
# define mmap32_rnd_bits  mmap_rnd_bits
# define mmap64_rnd_bits  mmap_rnd_bits
#endif

and use that. That's still way more readable than the unholy ifdef mess.

Thanks,

tglx

Quoted 44 lines · Thomas Gleixner

Quoted 23 lines · Dmitry Safonov

2017-02-10 23:10 GMT+03:00 Thomas Gleixner <[email protected]>:

Quoted 22 lines · Borislav Petkov

Quoted 4 lines

I can't say that I'm thrilled about the ifdeffery this is adding.

But I can't think of a cleaner approach at a quick glance, though - that's generic and arch-specific code intertwined muck. Sad face.

It's trivial enough to do ....

Thanks,

tglx

---
 arch/x86/mm/mmap.c |   22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

arch/x86/mm/mmap.c +0 -0

@@ -55,6 +55,10 @@ static unsigned long stack_maxrandom_siz
 #define MIN_GAP (128*1024*1024UL + stack_maxrandom_size())
 #define MAX_GAP (TASK_SIZE/6*5)
 

Quoted 8 lines

Quoted 6 lines

arch/x86/mm/mmap.c +4 -0

+#ifndef CONFIG_COMPAT
+# define mmap_rnd_compat_bits  mmap_rnd_bits
+#endif
+
 

From my POV, I can't say that it's clearer to shadow mmap_compat_bits

like that then to have two functions with native/compat names. But if you insist, I'll resend patches set with your version.

You can make that

#ifdef CONFIG_64BIT

# define mmap32_rnd_bits  mmap_compat_rnd_bits
# define mmap64_rnd_bits  mmap_rnd_bits
#else
# define mmap32_rnd_bits  mmap_rnd_bits
# define mmap64_rnd_bits  mmap_rnd_bits
#endif

Quoted 1 line

and use that. That's still way more readable than the unholy ifdef mess.

Ok, will send this version in v5.
Ping me if you mind me using your SOB for this patch.

In the following patch they will be used to compute:
- mmap{,_legacy}_base for 64-bit mmap()
- mmap_compat{,_legacy}_base for 32-bit mmap()

This patch makes it possible to calculate mmap bases for any specified task_size, which is needed to correctly choose the base address for mmap in 32-bit syscalls and 64-bit syscalls.

arch/x86/include/asm/processor.h +0 -0

 #define KSTK_EIP(task)		(task_pt_regs(task)->ip)
 

arch/x86/mm/mmap.c +0 -0

index 42063e787717..98be520fd270 100644
@@ -35,12 +35,14 @@ struct va_alignment __read_mostly va_align = {
 	.flags = -1,
 };

arch/x86/mm/mmap.c +1 -4

-	if (gap < MIN_GAP)
-		gap = MIN_GAP;
-	else if (gap > MAX_GAP)
-		gap = MAX_GAP;
+	if (gap < MIN_GAP(task_size))

Quoted 4 lines · Dmitry Safonov

In the following patch they will be used to compute:
- mmap{,_legacy}_base for 64-bit mmap()
- mmap_compat{,_legacy}_base for 32-bit mmap()

Quoted 3 lines

This patch makes it possible to calculate mmap bases for any specified task_size, which is needed to correctly choose the base address for mmap in 32-bit syscalls and 64-bit syscalls.

Please rework your changelogs so they follow the requirements in Documentation.... Look for 'this patch' there.

Also order it in a way which makes it clear what the problem is and how it's solved.

arch/x86/include/asm/elf.h do { \ · line 300

Quoted 11 lines

arch/x86/include/asm/elf.h +3 -1

+#define STACK_RND_MASK_MODE(native) (0x7ff)
 #define STACK_RND_MASK (0x7ff)
 
 #define ARCH_DLINFO		ARCH_DLINFO_IA32

@@ -295,7 +296,8 @@ do {									\
 #else /* CONFIG_X86_32 */
 
 /* 1GB for 64bit, 8MB for 32bit */
-#define STACK_RND_MASK (test_thread_flag(TIF_ADDR32) ? 0x7ff : 0x3fffff)
+#define STACK_RND_MASK_MODE(native) ((native) ? 0x3fffff : 0x7ff)
+#define STACK_RND_MASK STACK_RND_MASK_MODE(!test_thread_flag(TIF_ADDR32))

I had to look twice what MODE means. The macro name suggests that it returns the mode, while it actually returns the mask. That's confusing at best. And 'native' is not intuitive either.

#define __STACK_RND_MASK(is64bit) ((is64bit) ? 0x3fffff : 0x7ff)
#define STACK_RND_MASK __STACK_RND_MASK(!mmap_is_ia32())

That is pretty much clear and it uses mmap_is_ia32() as we do in other places. As a consequemce you can use the same macros for 64 and 32 bit.

arch/x86/include/asm/processor.h extern void start_thread(struct pt_regs *regs, unsigned long new_ip, · line 866

Quoted 12 lines

arch/x86/include/asm/processor.h +2 -1

index 1be64da0384e..52086e65b422 100644
@@ -862,7 +862,8 @@ extern void start_thread(struct pt_regs *regs, unsigned long new_ip,
  * This decides where the kernel will search for a free chunk of vm
  * space during mmap's.
  */
-#define TASK_UNMAPPED_BASE	(PAGE_ALIGN(TASK_SIZE / 3))
+#define _TASK_UNMAPPED_BASE(task_size)	(PAGE_ALIGN(task_size / 3))
+#define TASK_UNMAPPED_BASE	_TASK_UNMAPPED_BASE(TASK_SIZE)

Please use two underscores and align the macros in tabular fashion.

#define __TASK_UNMAPPED_BASE(task_size) (PAGE_ALIGN(task_size / 3))
#define TASK_UNMAPPED_BASE __TASK_UNMAPPED_BASE(TASK_SIZE)

That way stuff becomes easy to read.

Quoted 10 lines

arch/x86/include/asm/processor.h +0 -0

 #define KSTK_EIP(task)		(task_pt_regs(task)->ip)
 

arch/x86/mm/mmap.c +0 -0

index 42063e787717..98be520fd270 100644
@@ -35,12 +35,14 @@ struct va_alignment __read_mostly va_align = {
 	.flags = -1,
 };

arch/x86/mm/mmap.c struct va_alignment __read_mostly va_align = { · line 44

Quoted 10 lines

arch/x86/mm/mmap.c +3 -2

 
-static unsigned long stack_maxrandom_size(void)
+static unsigned long stack_maxrandom_size(unsigned long task_size)
 {
 	unsigned long max = 0;
 	if ((current->flags & PF_RANDOMIZE) &&
 		!(current->personality & ADDR_NO_RANDOMIZE)) {
-		max = ((-1UL) & STACK_RND_MASK) << PAGE_SHIFT;
+		max = (-1UL);
+		max &= STACK_RND_MASK_MODE(task_size == TASK_SIZE_MAX);

That just makes me barf, really. I have to go and lookup how TASK_SIZE_MAX is defined in order to read that code. TASK_SIZE_MAX as is does not give a hint at all that it means TASK_SIZE_MAX of 64 bit tasks.

You just explained me that you want stuff proper for clarity reasons. So what's so wrong with adding a helper inline tasksize_64bit() or such?

arch/x86/mm/mmap.c static unsigned long stack_maxrandom_size(void) · line 57

Quoted 13 lines

arch/x86/mm/mmap.c +3 -2

+		max <<= PAGE_SHIFT;
 	}
 
 	return max;

@@ -51,8 +53,8 @@ static unsigned long stack_maxrandom_size(void)
  *
  * Leave an at least ~128 MB hole with possible stack randomization.
  */
-#define MIN_GAP (128*1024*1024UL + stack_maxrandom_size())
-#define MAX_GAP (TASK_SIZE/6*5)
+#define MIN_GAP(task_size) (128*1024*1024UL + stack_maxrandom_size(task_size))
+#define MAX_GAP(task_size) (task_size/6*5)

Eew. Yes it's existing macro mess, but there is not point in proliferating that. That macro crap is only used in mmap_base() and there is no justification for these unreadable macros at all. It just makes stuff obfuscated for no reason. Just blindly making it more obfuscated does not make it any better.

arch/x86/mm/mmap.c unsigned long arch_mmap_rnd(void) · line 91

Quoted 5 lines

arch/x86/mm/mmap.c +0 -0

 static int mmap_is_legacy(void)
 {

@@ -88,16 +90,22 @@ unsigned long arch_mmap_rnd(void)
 	return arch_native_rnd();
 }

#define SIZE_128M (128 * 1024 * 1024UL)

arch/x86/mm/mmap.c unsigned long arch_mmap_rnd(void) · line 95

Quoted 4 lines

arch/x86/mm/mmap.c +1 -1

-static unsigned long mmap_base(unsigned long rnd)
+static unsigned long mmap_base(unsigned long rnd, unsigned long task_size)
 {
 	unsigned long gap = rlimit(RLIMIT_STACK);

	unsigned long gap_min, gap_max;

	/* Add comment what this means */
	gap_min = SIZE_128M + stack_maxrandom_size(task_size);
	/* Explain that ' /6 * 5' magic */
	gap_max = (task_size / 6) * 5;

and use gap_min/gap_max below. That would be too intuitive and readable, right?

Quoted 6 lines

arch/x86/mm/mmap.c +1 -4

-	if (gap < MIN_GAP)
-		gap = MIN_GAP;
-	else if (gap > MAX_GAP)
-		gap = MAX_GAP;
+	if (gap < MIN_GAP(task_size))

arch/x86/mm/mmap.c unsigned long arch_mmap_rnd(void) · line 106

Quoted 10 lines

arch/x86/mm/mmap.c +8 -1

+		gap = MIN_GAP(task_size);
+	else if (gap > MAX_GAP(task_size))
+		gap = MAX_GAP(task_size);
 
-	return PAGE_ALIGN(TASK_SIZE - gap - rnd);
+	return PAGE_ALIGN(task_size - gap - rnd);
+}
+
+static unsigned long mmap_legacy_base(unsigned long rnd,
+		unsigned long task_size)

Please align the argument in the second line with the argument in the first one

static unsigned long mmap_legacy_base(unsigned long rnd,
unsigned long task_size)

Thanks,

tglx

Quoted 4 lines

That just makes me barf, really. I have to go and lookup how TASK_SIZE_MAX is defined in order to read that code. TASK_SIZE_MAX as is does not give a hint at all that it means TASK_SIZE_MAX of 64 bit tasks.

Quoted 2 lines

You just explained me that you want stuff proper for clarity reasons. So what's so wrong with adding a helper inline tasksize_64bit() or such?

I thought about that, but I'll need to redefine it under ifdefs :-/ I mean, for 32-bit native code.
Hmm, I think, if I use is32bit parameter for __STACK_RND_MASK(), will it be more readable if I just compare to IA32_PAGE_OFFSET here? Or does it makes sence to introduce tasksize_32bit()?

Quoted 4 lines

arch/x86/mm/mmap.c +1 -1

-static unsigned long mmap_base(unsigned long rnd)
+static unsigned long mmap_base(unsigned long rnd, unsigned long task_size)
 {

	unsigned long gap = rlimit(RLIMIT_STACK);

	unsigned long gap_min, gap_max;

	/* Add comment what this means */
	gap_min = SIZE_128M + stack_maxrandom_size(task_size);
	/* Explain that ' /6 * 5' magic */
	gap_max = (task_size / 6) * 5;

Quoted 11 lines · Thomas Gleixner

Quoted 5 lines

(quoted diff) +1 -1

-static unsigned long mmap_base(unsigned long rnd)
+static unsigned long mmap_base(unsigned long rnd, unsigned long

task_size)

 {
	unsigned long gap = rlimit(RLIMIT_STACK);

	unsigned long gap_min, gap_max;

	/* Add comment what this means */
	gap_min = SIZE_128M + stack_maxrandom_size(task_size);
	/* Explain that ' /6 * 5' magic */
	gap_max = (task_size / 6) * 5;

So, I can't find about those limits on a gap size:
They were introduced by commit 8913d55b6c58 ("i386 virtual memory layout rework").
All I could find is that 128Mb limit was more limit on virtual adress space than on a memory available those days.
And 5/6 of task_size looks like heuristic value.
So I'm not sure, what to write in comments:
that rlimit on stack can't be bigger than 5/6 of task_size? That looks obvious from the code.

Fix 32-bit compat_sys_mmap() mapping VMA over 4Gb in 64-bit binaries and 64-bit sys_mmap() mapping VMA only under 4Gb in 32-bit binaries. Introduced new bases for compat syscalls in mm_struct:
mmap_compat_base and mmap_compat_legacy_base for top-down and bottom-up allocations accordingly.
Taught arch_get_unmapped_area{,_topdown}() to use the new mmap_bases in compat syscalls for high/low limits in vm_unmapped_area().

I discovered that bug on ZDTM tests for compat 32-bit C/R. Working compat sys_mmap() in 64-bit binaries is really needed for that purpose, as 32-bit applications are restored from 64-bit CRIU binary.

Quoted 11 lines · Dmitry Safonov

Fix 32-bit compat_sys_mmap() mapping VMA over 4Gb in 64-bit binaries and 64-bit sys_mmap() mapping VMA only under 4Gb in 32-bit binaries. Introduced new bases for compat syscalls in mm_struct:
mmap_compat_base and mmap_compat_legacy_base for top-down and bottom-up allocations accordingly.
Taught arch_get_unmapped_area{,_topdown}() to use the new mmap_bases in compat syscalls for high/low limits in vm_unmapped_area().

I discovered that bug on ZDTM tests for compat 32-bit C/R. Working compat sys_mmap() in 64-bit binaries is really needed for that purpose, as 32-bit applications are restored from 64-bit CRIU binary.

Again that changelog sucks.

Explain the problem/bug first. Then explain the way to fix it and do not tell fairy tales about what you did without explaing the bug in the first place.

Documentation....SubittingPatches explains that very well.

arch/Kconfig config ARCH_MMAP_RND_COMPAT_BITS · line 679

Quoted 6 lines

arch/Kconfig +6 -0

+config HAVE_ARCH_COMPAT_MMAP_BASES
+	bool
+	help
+	  If this is set, one program can do native and compatible syscall
+	  mmap() on architecture. Thus kernel has different bases to
+	  compute high and low virtual address limits for allocation.

Sigh. How is a user supposed to decode this?

This allows 64bit applications to invoke syscalls in 64bit and 32bit mode. Required for ....

arch/x86/kernel/sys_x86_64.c static void find_start_end(unsigned long flags, unsigned long *begin, · line 120

Quoted 12 lines

arch/x86/kernel/sys_x86_64.c +3 -3

@@ -113,10 +114,19 @@ static void find_start_end(unsigned long flags, unsigned long *begin,
 		if (current->flags & PF_RANDOMIZE) {
 			*begin = randomize_page(*begin, 0x02000000);
 		}
-	} else {
-		*begin = current->mm->mmap_legacy_base;
-		*end = TASK_SIZE;
+		return;
 	}
+
+#ifdef CONFIG_COMPAT

Can you please find a solution which does not create that ifdef horror in the code? Just a few accessors to those compat fields are required to do that.

arch/x86/mm/mmap.c static unsigned long mmap_legacy_base(unsigned long rnd, · line 142

Quoted 5 lines

arch/x86/mm/mmap.c +5 -0

+
+#ifdef CONFIG_COMPAT
+	arch_pick_mmap_base(&mm->mmap_compat_base, &mm->mmap_compat_legacy_base,
+			arch_compat_rnd(), IA32_PAGE_OFFSET);
+#endif

Ditto

Thanks,

tglx

At this momet, logic in arch_get_unmapped_area{,_topdown} for mmaps with MAP_32BIT flag checks TIF_ADDR32 which means:
o if 32-bit ELF changes mode to 64-bit on x86_64 and then tries to mmap() with MAP_32BIT it'll result in addr over 4Gb (as default is top-down allocation)
o if 64-bit ELF changes mode to 32-bit and tries mmap() with MAP_32BIT, it'll allocate only memory in 1GB space: [0x40000000, 0x80000000).

Fix it by handeling MAP_32BIT in 64-bit syscalls only.

As a little bonus it'll make thread flag a little less used.

arch/x86/kernel/sys_x86_64.c +2 -2

@@ -101,7 +101,7 @@ SYSCALL_DEFINE6(mmap, unsigned long, addr, unsigned long, len,
 static void find_start_end(unsigned long flags, unsigned long *begin,
 			   unsigned long *end)
 {
-	if (!test_thread_flag(TIF_ADDR32) && (flags & MAP_32BIT)) {
+	if (!in_compat_syscall() && (flags & MAP_32BIT)) {
 		/* This is usually used needed to map code in small
 		   model, so it needs to be in the first 31bit. Limit
 		   it to that.  This means we need to move the

@@ -195,7 +195,7 @@ arch_get_unmapped_area_topdown(struct file *filp, const unsigned long addr0,
 		return addr;
 
 	/* for MAP_32BIT mappings we force the legacy mmap base */
-	if (!test_thread_flag(TIF_ADDR32) && (flags & MAP_32BIT))
+	if (!in_compat_syscall() && (flags & MAP_32BIT))
 		goto bottomup;
 
 	/* requesting a specific address */

Signature · 1 line

2.11.0

Quoted 8 lines · Dmitry Safonov

At this momet, logic in arch_get_unmapped_area{,_topdown} for mmaps with MAP_32BIT flag checks TIF_ADDR32 which means:
o if 32-bit ELF changes mode to 64-bit on x86_64 and then tries to mmap() with MAP_32BIT it'll result in addr over 4Gb (as default is top-down allocation)
o if 64-bit ELF changes mode to 32-bit and tries mmap() with MAP_32BIT, it'll allocate only memory in 1GB space: [0x40000000, 0x80000000).

Quoted 1 line

Fix it by handeling MAP_32BIT in 64-bit syscalls only.

I really have a hard time to understand what is fixed and how that is related to the $subject.

Again. Please explain the problem first properly so one can understand the issue immediately.

Quoted 2 lines

As a little bonus it'll make thread flag a little less used.

I really do not understand the bonus part here. You replace the thread flag check with a different one and AFAICT this looks like oart of the 'fix'.

Thanks,

tglx

Quoted 22 lines

arch/x86/kernel/sys_x86_64.c +2 -3

@@ -101,7 +101,7 @@ SYSCALL_DEFINE6(mmap, unsigned long, addr, unsigned long, len,
 static void find_start_end(unsigned long flags, unsigned long *begin,
 			   unsigned long *end)
 {
-	if (!test_thread_flag(TIF_ADDR32) && (flags & MAP_32BIT)) {
+	if (!in_compat_syscall() && (flags & MAP_32BIT)) {
 		/* This is usually used needed to map code in small
 		   model, so it needs to be in the first 31bit. Limit
 		   it to that.  This means we need to move the

@@ -195,7 +195,7 @@ arch_get_unmapped_area_topdown(struct file *filp, const unsigned long addr0,
 		return addr;
 
 	/* for MAP_32BIT mappings we force the legacy mmap base */
-	if (!test_thread_flag(TIF_ADDR32) && (flags & MAP_32BIT))
+	if (!in_compat_syscall() && (flags & MAP_32BIT))
 		goto bottomup;
 
 	/* requesting a specific address */
--

2.11.0

Quoted 10 lines · Thomas Gleixner

Quoted 8 lines · Dmitry Safonov

At this momet, logic in arch_get_unmapped_area{,_topdown} for mmaps with MAP_32BIT flag checks TIF_ADDR32 which means:
o if 32-bit ELF changes mode to 64-bit on x86_64 and then tries to mmap() with MAP_32BIT it'll result in addr over 4Gb (as default is top-down allocation)
o if 64-bit ELF changes mode to 32-bit and tries mmap() with MAP_32BIT, it'll allocate only memory in 1GB space: [0x40000000, 0x80000000).

Quoted 8 lines

Quoted 1 line

Fix it by handeling MAP_32BIT in 64-bit syscalls only.

I really have a hard time to understand what is fixed and how that is related to the $subject.

Again. Please explain the problem first properly so one can understand the
issue immediately.

Ok, rewrote the changes log.

Quoted 6 lines

Quoted 2 lines

As a little bonus it'll make thread flag a little less used.

I really do not understand the bonus part here. You replace the thread flag
check with a different one and AFAICT this looks like oart of the 'fix'.

It's a part of the fix, right.
What I meant here is that after those patches TIF_ADDR32 is no more used after exec() time. That's bonus as if we manage to change exec() code in some way (i.e., pass address restriction as a parameter), we'll have additional free thread info flag.

Quoted 29 lines

Thanks,

tglx

Quoted 24 lines

arch/x86/kernel/sys_x86_64.c +1 -1

@@ -101,7 +101,7 @@ SYSCALL_DEFINE6(mmap, unsigned long, addr,
 unsigned long, len,
 static void find_start_end(unsigned long flags, unsigned long *begin,
                unsigned long *end)
 {
-    if (!test_thread_flag(TIF_ADDR32) && (flags & MAP_32BIT)) {
+    if (!in_compat_syscall() && (flags & MAP_32BIT)) {
         /* This is usually used needed to map code in small
            model, so it needs to be in the first 31bit. Limit

it to that. This means we need to move the

arch/x86/kernel/sys_x86_64.c +1 -1

@@ -195,7 +195,7 @@ arch_get_unmapped_area_topdown(struct file *filp,
 const unsigned long addr0,
         return addr;
 
     /* for MAP_32BIT mappings we force the legacy mmap base */
-    if (!test_thread_flag(TIF_ADDR32) && (flags & MAP_32BIT))
+    if (!in_compat_syscall() && (flags & MAP_32BIT))
         goto bottomup;
 

     /* requesting a specific address */

--
2.11.0

Changes since v3:
- fixed usage of 64-bit random mask for 32-bit mm->mmap_compat_base, during introducing mmap_compat{_legacy,}_base

Changes since v2:
- don't distinguish native and compat tasks by TIF_ADDR32, introduced mmap_compat{_legacy,}_base which allows to treat them the same
- fixed kbuild errors

Changes since v1:
- Recalculate mmap_base instead of using max possible virtual address for compat/native syscall. That will make policy for allocation the same in 32-bit binaries and in 32-bit syscalls in 64-bit binaries. I need this because sys_mmap() in restored 32-bit process shouldn't hit the stack area.
- Fixed mmap() with MAP_32BIT flag in the same usecases
- used in_compat_syscall() helper rather TS_COMPAT check (Andy noticed)
- introduced find_top() helper as suggested by Andy to simplify code
- fixed test error-handeling: it checked the result of sys_mmap() with MMAP_FAILED, which is not correct, as it calls raw syscall - now checks return value to be aligned to PAGE_SIZE.

Description from v1 [2]:

A fix for bug in mmap() that I referenced in [1]. Also selftest for it.

[1]: https://marc.info/?l=linux-kernel&m=148311451525315
[2]: https://marc.info/?l=linux-kernel&m=148415888707662

Cc Thomas Gleixner <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc "H. Peter Anvin" <[email protected]>

Cc Andy Lutomirski <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc [email protected]

Cc [email protected]

Dmitry Safonov (5):
x86/mm: split arch_mmap_rnd() on compat/native versions x86/mm: introduce mmap{,_legacy}_base
x86/mm: fix 32-bit mmap() for 64-bit ELF x86/mm: check in_compat_syscall() instead TIF_ADDR32 for

    mmap(MAP_32BIT)

selftests/x86: add test to check compat mmap() return addr

 arch/Kconfig                                   |   7 +
 arch/x86/Kconfig                               |   1 +
 arch/x86/include/asm/elf.h                     |   4 +-
 arch/x86/include/asm/processor.h               |   3 +-
 arch/x86/kernel/sys_x86_64.c                   |  32 +++-
 arch/x86/mm/mmap.c                             |  89 +++++++----
 include/linux/mm_types.h                       |   5 +
 tools/testing/selftests/x86/Makefile           |   2 +-
 tools/testing/selftests/x86/test_compat_mmap.c | 208 +++++++++++++++++++++++++
 9 files changed, 311 insertions(+), 40 deletions(-)
 create mode 100644 tools/testing/selftests/x86/test_compat_mmap.c