Messages in this thread | | | Date | Fri, 17 Nov 2023 09:47:06 +0800 | Subject | Re: [PATCH] arm64: Fix 32-bit compatible userspace write size overflow error | From | Jinjie Ruan <> |
| |
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
| |