[PATCHv4,4/5] x86/mm: check in_compat_syscall() instead TIF_ADDR32 for mmap(MAP_32BIT)
diff mbox series

Message ID 20170130120432.6716-5-dsafonov@virtuozzo.com
State New, archived
Headers show
Series
  • Fix compatible mmap() return pointer over 4Gb
Related show

Commit Message

Dmitry Safonov Jan. 30, 2017, 12:04 p.m. UTC
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.

Signed-off-by: Dmitry Safonov <dsafonov@virtuozzo.com>
---
 arch/x86/kernel/sys_x86_64.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Thomas Gleixner Feb. 11, 2017, 8:13 p.m. UTC | #1
On Mon, 30 Jan 2017, Dmitry Safonov wrote:

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

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.

> 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

> @@ -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
> 
>
Dmitry Safonov Feb. 14, 2017, 4:11 p.m. UTC | #2
On 02/11/2017 11:13 PM, Thomas Gleixner wrote:
> On Mon, 30 Jan 2017, Dmitry Safonov wrote:
>
>> 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.
>
> 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.

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

> Thanks,
>
> 	tglx
>
>> @@ -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
>>
>>
Dmitry Safonov Feb. 14, 2017, 4:14 p.m. UTC | #3
On 02/14/2017 07:11 PM, Dmitry Safonov wrote:
> On 02/11/2017 11:13 PM, Thomas Gleixner wrote:
>> On Mon, 30 Jan 2017, Dmitry Safonov wrote:
>>
>>> 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.
>>
>> 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.
>
>>
>>> 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.

Oh, I lied here: it'll be still used for TASK_SIZE macro and etc.
But, I don't see why in generic code we can't use TASK_SIZE_MAX,
at least at this moment.

>
>> Thanks,
>>
>>     tglx
>>
>>> @@ -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
>>>
>>>
>
>

Patch
diff mbox series

diff --git a/arch/x86/kernel/sys_x86_64.c b/arch/x86/kernel/sys_x86_64.c
index 90be0839441d..533e879e9cdf 100644
--- a/arch/x86/kernel/sys_x86_64.c
+++ b/arch/x86/kernel/sys_x86_64.c
@@ -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 */