linux-kernel - Re: [PATCH] arm64: Fix 32-bit compatible userspace write size overflow error

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <f55f49d7-1003-0e98-12a2-422bf4d9318e@huawei.com>
Date:   Fri, 17 Nov 2023 09:47:06 +0800
From:   Jinjie Ruan <ruanjinjie@...wei.com>
To:     Arnd Bergmann <arnd@...db.de>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>,
        Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will@...nel.org>,
        "Eric W. Biederman" <ebiederm@...ssion.com>,
        Sam Ravnborg <sam@...nborg.org>,
        Stafford Horne <shorne@...il.com>,
        Dinh Nguyen <dinguyen@...nel.org>,
        Mark Rutland <mark.rutland@....com>
Subject: Re: [PATCH] arm64: Fix 32-bit compatible userspace write size
 overflow error



On 2023/11/16 21:39, Arnd Bergmann wrote:
> On Thu, Nov 16, 2023, at 02:47, Jinjie Ruan wrote:
>> For 32-bit compatible userspace program, write with size = -1 return not
>> -1 but unexpected other values, which is due to the __access_ok() check is
>> not right. The specified "addr + size" is greater than 32-bit limit and
>> should return -EFAULT, but TASK_SIZE_MAX still defined as UL(1) << VA_BITS
>> in U32 mode, which is much greater than "addr + size" and cannot catch the
>> overflow error.
> 
> Thank you for the detailed analysis of the change in behavior that
> resulted from my patch. As far as I can tell, this is an intentional
> change that should have been documented as part of the patch
> submission.
> 
>> 	    assert(write(fd, wbuf, 3) == 3);
>>
>> 	    ret = write (fd, wbuf, SIZE_MAX);
>> 	    pinfo("ret=%d\n", ret);
>> 	    pinfo("size_max=%d\n",SIZE_MAX);
>> 	    assert(ret==-1);
> 
> I think it is wrong to have an assert() here since the
> documentation for write() does not state what happens
> when the beginning of the buffer is addressable but the
> end is not. We were handling this inconsistently between
> architectures before my patch, which ensured we do the
> same thing on all compat architectures now.
> 
> You can argue that this behavior is inconsistent with
> native 32-bit mode, but at the time we decided that this
> was not an important distinction.
> 
>> Before applying this patch, userspace 32-bit program return 1112 if the
>> write size = -1 as below:
>> 	/root # ./test
>> 	[INFO][test.c][32][main]:ret=-1
>> 	[INFO][test.c][33][main]:size_max=-1
>> 	[INFO][test.c][36][main]:INFO: end
>> 	/root # ./test32
>> 	[INFO][test.c][32][main]:ret=1112
>> 	[INFO][test.c][33][main]:size_max=-1
>> 	test32: test.c:34: main: Assertion `ret==-1' failed.
>> 	Aborted
> 
> Here, the write() successfully gets 1112 bytes of data,
> which matches what you get for any other large size that
> does not overflow user address space in the kernel.

With a 64-bit kernel running a 32-bit user-mode program, the most
confusing behavior of writing a size of -1 is as follows when the
program is executed multiple times continuously, the return value is
different each time(4280、2776、2216、4536、856、4616、4632 or 3288 etc.)
although the same program is run each time:

/root # ./test32
[INFO][test.c][32][main]:ret=4280
[INFO][test.c][33][main]:size_max=-1
test32: test.c:34: main: Assertion `ret==-1' failed.
Aborted

/root # ./test32
[INFO][test.c][32][main]:ret=2776
[INFO][test.c][33][main]:size_max=-1
test32: test.c:34: main: Assertion `ret==-1' failed.
Aborted

/root # ./test32
[INFO][test.c][32][main]:ret=2216
[INFO][test.c][33][main]:size_max=-1
test32: test.c:34: main: Assertion `ret==-1' failed.
Aborted

/root # ./test32
[INFO][test.c][32][main]:ret=4536
[INFO][test.c][33][main]:size_max=-1
test32: test.c:34: main: Assertion `ret==-1' failed.
Aborted

/root # ./test32
[INFO][test.c][32][main]:ret=856
[INFO][test.c][33][main]:size_max=-1
test32: test.c:34: main: Assertion `ret==-1' failed.
Aborted

/root # ./test32
[INFO][test.c][32][main]:ret=4616
[INFO][test.c][33][main]:size_max=-1
test32: test.c:34: main: Assertion `ret==-1' failed.
Aborted

/root # ./test32
[INFO][test.c][32][main]:ret=4632
[INFO][test.c][33][main]:size_max=-1
test32: test.c:34: main: Assertion `ret==-1' failed.
Aborted

/root # ./test32
[INFO][test.c][32][main]:ret=3288
[INFO][test.c][33][main]:size_max=-1
test32: test.c:34: main: Assertion `ret==-1' failed.
Aborted


> 
>> Fixes: 967747bbc084 ("uaccess: remove CONFIG_SET_FS")
>>
>>  #define DEFAULT_MAP_WINDOW_64	(UL(1) << VA_BITS_MIN)
>>  #define TASK_SIZE_64		(UL(1) << vabits_actual)
>> +#ifdef CONFIG_COMPAT
>> +#define TASK_SIZE_MAX		(test_thread_flag(TIF_32BIT) ? \
>> +				UL(0x100000000) : (UL(1) << VA_BITS))
>> +#else
>>  #define TASK_SIZE_MAX		(UL(1) << VA_BITS)
>> +#endif
> 
> This adds back the cost for every user access that I was
> trying to save, and it makes arm64 behave differently from
> the other architectures.

Indeed, this adds to the cost of checking.

> 
> As far as I can tell, the current behavior was originally
> introduced on x86 with commit 9063c61fd5cb ("x86, 64-bit:
> Clean up user address masking").

Thank you!



> 
>      Arnd