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

[Arnd Bergmann]

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.

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

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

     Arnd