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

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

[Jinjie Ruan]

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.

Fix above error by checking 32-bit limit if it is 32-bit compatible
userspace program.

How to reproduce:

The test program is as below:

cat test.c
	#include <unistd.h>
	#include <fcntl.h>
	#include <stdio.h>
	#include <stdint.h>
	#include <stdlib.h>
	#include <assert.h>

	#define pinfo(fmt, args...) \
	    fprintf(stderr, "[INFO][%s][%d][%s]:"fmt, \
	    __FILE__,__LINE__,__func__,##args)

	#undef SIZE_MAX
	#define SIZE_MAX -1

	int main()
	{
	    char wbuf[3] = { 'x', 'y', 'z' };
	    char *path = "write.tmp";
	    int ret;

	    int fd = open(path, O_RDWR | O_CREAT);
	    if (fd<0)
	    {
	        pinfo("fd=%d\n", fd);
	        exit(-1);
	    }

	    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);
	    close(fd);
	    pinfo("INFO: end\n");

	    return 0;
	}

aarch64-linux-gnu-gcc --static test.c -o test
arm-linux-gnueabi-gcc --static test.c -o test32

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

After applying this patch, userspace 32-bit program return -1 if the write
size = -1 as expected 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=-1
	[INFO][test.c][33][main]:size_max=-1
	[INFO][test.c][36][main]:INFO: end

Fixes: 967747bbc084 ("uaccess: remove CONFIG_SET_FS")
Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
---
 arch/arm64/include/asm/processor.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm64/include/asm/processor.h b/arch/arm64/include/asm/processor.h
index e5bc54522e71..6a087d58a90a 100644
--- a/arch/arm64/include/asm/processor.h
+++ b/arch/arm64/include/asm/processor.h
@@ -52,7 +52,12 @@
 
 #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
 
 #ifdef CONFIG_COMPAT
 #if defined(CONFIG_ARM64_64K_PAGES) && defined(CONFIG_KUSER_HELPERS)
-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Mark Rutland]

On Thu, Nov 16, 2023 at 03:47:05PM +0800, 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.

Can you please explain why you believe that is unexpected?

e.g. Is that documented somewhere? Do you see a real application depending on
that somewhow?

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

The check against TASK_SIZE_MAX is not intended to catch 32-bit addr + size
overflow; it's intended to check that uaccesses never touch kernel memory. The
kernel's uaccess routines use 64-bit (or 65-bit) arithmetic, so these won't
wrap and access memory at the start of the user address space.

> Fix above error by checking 32-bit limit if it is 32-bit compatible
> userspace program.
> 
> How to reproduce:
> 
> The test program is as below:
> 
> cat test.c
> 	#include <unistd.h>
> 	#include <fcntl.h>
> 	#include <stdio.h>
> 	#include <stdint.h>
> 	#include <stdlib.h>
> 	#include <assert.h>
> 
> 	#define pinfo(fmt, args...) \
> 	    fprintf(stderr, "[INFO][%s][%d][%s]:"fmt, \
> 	    __FILE__,__LINE__,__func__,##args)
> 
> 	#undef SIZE_MAX
> 	#define SIZE_MAX -1
> 
> 	int main()
> 	{
> 	    char wbuf[3] = { 'x', 'y', 'z' };
> 	    char *path = "write.tmp";
> 	    int ret;
> 
> 	    int fd = open(path, O_RDWR | O_CREAT);
> 	    if (fd<0)
> 	    {
> 	        pinfo("fd=%d\n", fd);
> 	        exit(-1);
> 	    }
> 
> 	    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);
> 	    close(fd);
> 	    pinfo("INFO: end\n");
> 
> 	    return 0;
> 	}
> 
> aarch64-linux-gnu-gcc --static test.c -o test
> arm-linux-gnueabi-gcc --static test.c -o test32
> 
> 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
> 
> After applying this patch, userspace 32-bit program return -1 if the write
> size = -1 as expected 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=-1
> 	[INFO][test.c][33][main]:size_max=-1
> 	[INFO][test.c][36][main]:INFO: end
> 
> Fixes: 967747bbc084 ("uaccess: remove CONFIG_SET_FS")

As above, this is *not* a fix. This is the intended behaviour.

AFAICT, the behaviour didn't change on arm64 in that commit either; we were
unconditionally using TASK_SIZE_MAX many commits earlier, e.g. in commit:

  3d2403fd10a1dbb3 ("arm64: uaccess: remove set_fs()")

... so the fixes tag is bogus on both fronts.

> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
> ---
>  arch/arm64/include/asm/processor.h | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/arch/arm64/include/asm/processor.h b/arch/arm64/include/asm/processor.h
> index e5bc54522e71..6a087d58a90a 100644
> --- a/arch/arm64/include/asm/processor.h
> +++ b/arch/arm64/include/asm/processor.h
> @@ -52,7 +52,12 @@
>  
>  #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 isn't even the same as on 32-bit. On 32-bit arm, the task size split can
be 1G/3G, 2G/2G, or 3G/1G depending on configuration, and 4G/4G isn't currently
an option.

I don't believe that userspace is actually dependent upon this for functional
reasons, and I don't believe that there's a security issue here. Even if
access_ok() allows addr+size to go past 4G, the kernel address calculations are
64-bit and won't wrap.

For all the reasons above, I don't beleive this is correct nor do I believe
this is necesssary. Given that, NAK to this patch.

Thanks,
Mark.

>  
>  #ifdef CONFIG_COMPAT
>  #if defined(CONFIG_ARM64_64K_PAGES) && defined(CONFIG_KUSER_HELPERS)
> -- 
> 2.34.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Jinjie Ruan]

On 2023/11/16 22:58, Mark Rutland wrote:
> On Thu, Nov 16, 2023 at 03:47:05PM +0800, 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.
> 
> Can you please explain why you believe that is unexpected?
> 
> e.g. Is that documented somewhere? Do you see a real application depending on
> that somewhow?

In my opinion, access_ok() must ensure that the access address is not
out of bounds，which must be a valid address range for 32-bit userspace
program.

> 
>> 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.
> 
> The check against TASK_SIZE_MAX is not intended to catch 32-bit addr + size
> overflow; it's intended to check that uaccesses never touch kernel memory. The
> kernel's uaccess routines use 64-bit (or 65-bit) arithmetic, so these won't
> wrap and access memory at the start of the user address space.

Thank you! My understanding of Task_SIZE_MAX is wrong, so catch 32-bit
addr + size overflow is designed by MAX_RW_COUNT?

> 
>> Fix above error by checking 32-bit limit if it is 32-bit compatible
>> userspace program.
>>
>> How to reproduce:
>>
>> The test program is as below:
>>
>> cat test.c
>> 	#include <unistd.h>
>> 	#include <fcntl.h>
>> 	#include <stdio.h>
>> 	#include <stdint.h>
>> 	#include <stdlib.h>
>> 	#include <assert.h>
>>
>> 	#define pinfo(fmt, args...) \
>> 	    fprintf(stderr, "[INFO][%s][%d][%s]:"fmt, \
>> 	    __FILE__,__LINE__,__func__,##args)
>>
>> 	#undef SIZE_MAX
>> 	#define SIZE_MAX -1
>>
>> 	int main()
>> 	{
>> 	    char wbuf[3] = { 'x', 'y', 'z' };
>> 	    char *path = "write.tmp";
>> 	    int ret;
>>
>> 	    int fd = open(path, O_RDWR | O_CREAT);
>> 	    if (fd<0)
>> 	    {
>> 	        pinfo("fd=%d\n", fd);
>> 	        exit(-1);
>> 	    }
>>
>> 	    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);
>> 	    close(fd);
>> 	    pinfo("INFO: end\n");
>>
>> 	    return 0;
>> 	}
>>
>> aarch64-linux-gnu-gcc --static test.c -o test
>> arm-linux-gnueabi-gcc --static test.c -o test32
>>
>> 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
>>
>> After applying this patch, userspace 32-bit program return -1 if the write
>> size = -1 as expected 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=-1
>> 	[INFO][test.c][33][main]:size_max=-1
>> 	[INFO][test.c][36][main]:INFO: end
>>
>> Fixes: 967747bbc084 ("uaccess: remove CONFIG_SET_FS")
> 
> As above, this is *not* a fix. This is the intended behaviour.
> 
> AFAICT, the behaviour didn't change on arm64 in that commit either; we were
> unconditionally using TASK_SIZE_MAX many commits earlier, e.g. in commit:
> 
>   3d2403fd10a1dbb3 ("arm64: uaccess: remove set_fs()")
> 
> ... so the fixes tag is bogus on both fronts.

Thank you! I'll recheck it.

> 
>> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
>> ---
>>  arch/arm64/include/asm/processor.h | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/arch/arm64/include/asm/processor.h b/arch/arm64/include/asm/processor.h
>> index e5bc54522e71..6a087d58a90a 100644
>> --- a/arch/arm64/include/asm/processor.h
>> +++ b/arch/arm64/include/asm/processor.h
>> @@ -52,7 +52,12 @@
>>  
>>  #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 isn't even the same as on 32-bit. On 32-bit arm, the task size split can
> be 1G/3G, 2G/2G, or 3G/1G depending on configuration, and 4G/4G isn't currently
> an option.
> 
> I don't believe that userspace is actually dependent upon this for functional
> reasons, and I don't believe that there's a security issue here. Even if
> access_ok() allows addr+size to go past 4G, the kernel address calculations are
> 64-bit and won't wrap.

If access_ok() is used only to distinguish between user space and kernel
space addresses, it is not the correct method to modify TASK_SIZE_MAX.

> 
> For all the reasons above, I don't beleive this is correct nor do I believe
> this is necesssary. Given that, NAK to this patch.

Thank you! I'll delve into the semantics of the "write" system call.

> 
> Thanks,
> Mark.
> 
>>  
>>  #ifdef CONFIG_COMPAT
>>  #if defined(CONFIG_ARM64_64K_PAGES) && defined(CONFIG_KUSER_HELPERS)
>> -- 
>> 2.34.1
>>
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Jinjie Ruan]

On 2023/11/16 22:58, Mark Rutland wrote:
> On Thu, Nov 16, 2023 at 03:47:05PM +0800, 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.
> 
> Can you please explain why you believe that is unexpected?
> 
> e.g. Is that documented somewhere? Do you see a real application depending on
> that somewhow?

I think access_ok() needs to ensure that the address is not out of
bounds, which guarantees that address access should not exceed the
32-bit boundary.

> 
>> 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.
> 
> The check against TASK_SIZE_MAX is not intended to catch 32-bit addr + size
> overflow; it's intended to check that uaccesses never touch kernel memory. The
> kernel's uaccess routines use 64-bit (or 65-bit) arithmetic, so these won't
> wrap and access memory at the start of the user address space.

Thank you! My understanding of TASK_SIZE_MAX is wrong.I seems that
"MAX_RW_COUNT" is designed to catch the 32-bit addr + size overflow.

> 
>> Fix above error by checking 32-bit limit if it is 32-bit compatible
>> userspace program.
>>
>> How to reproduce:
>>
>> The test program is as below:
>>
>> cat test.c
>> 	#include <unistd.h>
>> 	#include <fcntl.h>
>> 	#include <stdio.h>
>> 	#include <stdint.h>
>> 	#include <stdlib.h>
>> 	#include <assert.h>
>>
>> 	#define pinfo(fmt, args...) \
>> 	    fprintf(stderr, "[INFO][%s][%d][%s]:"fmt, \
>> 	    __FILE__,__LINE__,__func__,##args)
>>
>> 	#undef SIZE_MAX
>> 	#define SIZE_MAX -1
>>
>> 	int main()
>> 	{
>> 	    char wbuf[3] = { 'x', 'y', 'z' };
>> 	    char *path = "write.tmp";
>> 	    int ret;
>>
>> 	    int fd = open(path, O_RDWR | O_CREAT);
>> 	    if (fd<0)
>> 	    {
>> 	        pinfo("fd=%d\n", fd);
>> 	        exit(-1);
>> 	    }
>>
>> 	    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);
>> 	    close(fd);
>> 	    pinfo("INFO: end\n");
>>
>> 	    return 0;
>> 	}
>>
>> aarch64-linux-gnu-gcc --static test.c -o test
>> arm-linux-gnueabi-gcc --static test.c -o test32
>>
>> 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
>>
>> After applying this patch, userspace 32-bit program return -1 if the write
>> size = -1 as expected 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=-1
>> 	[INFO][test.c][33][main]:size_max=-1
>> 	[INFO][test.c][36][main]:INFO: end
>>
>> Fixes: 967747bbc084 ("uaccess: remove CONFIG_SET_FS")
> 
> As above, this is *not* a fix. This is the intended behaviour.
> 
> AFAICT, the behaviour didn't change on arm64 in that commit either; we were
> unconditionally using TASK_SIZE_MAX many commits earlier, e.g. in commit:
> 
>   3d2403fd10a1dbb3 ("arm64: uaccess: remove set_fs()")
> 
> ... so the fixes tag is bogus on both fronts.

Thank you!

> 
>> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
>> ---
>>  arch/arm64/include/asm/processor.h | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/arch/arm64/include/asm/processor.h b/arch/arm64/include/asm/processor.h
>> index e5bc54522e71..6a087d58a90a 100644
>> --- a/arch/arm64/include/asm/processor.h
>> +++ b/arch/arm64/include/asm/processor.h
>> @@ -52,7 +52,12 @@
>>  
>>  #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 isn't even the same as on 32-bit. On 32-bit arm, the task size split can
> be 1G/3G, 2G/2G, or 3G/1G depending on configuration, and 4G/4G isn't currently
> an option.
> 
> I don't believe that userspace is actually dependent upon this for functional
> reasons, and I don't believe that there's a security issue here. Even if
> access_ok() allows addr+size to go past 4G, the kernel address calculations are
> 64-bit and won't wrap.
> 
> For all the reasons above, I don't beleive this is correct nor do I believe
> this is necesssary. Given that, NAK to this patch.
> 
> Thanks,
> Mark.
> 
>>  
>>  #ifdef CONFIG_COMPAT
>>  #if defined(CONFIG_ARM64_64K_PAGES) && defined(CONFIG_KUSER_HELPERS)
>> -- 
>> 2.34.1
>>
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel