2023-11-15 10:57:15

by Jinjie Ruan

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

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
insufficient. The specified "ptr + size" is greater than 32-bit limit and
should return -EFAULT, but it is not checked and can not 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:

root@syzkaller:~# 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;
}

Before applying this patch, userspace 32-bit program return 3799 if the
write size = -1 as below:
root@syzkaller:~# gcc test.c -o test
root@syzkaller:~# gcc -m32 test.c -o test32
root@syzkaller:~# ./test
[INFO][test.c][32][main]:ret=-1
[INFO][test.c][33][main]:size_max=-1
[INFO][test.c][36][main]:INFO: end
root@syzkaller:~# ./test32
[INFO][test.c][32][main]:ret=3799
[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@syzkaller:~# gcc test.c -o test
root@syzkaller:~# gcc -m32 test.c -o test32
root@syzkaller:~# ./test
[INFO][test.c][32][main]:ret=-1
[INFO][test.c][33][main]:size_max=-1
[INFO][test.c][36][main]:INFO: end
root@syzkaller:~# ./test32
[INFO][test.c][32][main]:ret=-1
[INFO][test.c][33][main]:size_max=-1
[INFO][test.c][36][main]:INFO: end

Fixes: b9bd9f605c4a ("x86: uaccess: move 32-bit and 64-bit parts into proper <asm/uaccess_N.h> header")
Signed-off-by: Jinjie Ruan <[email protected]>
---
arch/x86/include/asm/uaccess_64.h | 6 ++++++
1 file changed, 6 insertions(+)

diff --git a/arch/x86/include/asm/uaccess_64.h b/arch/x86/include/asm/uaccess_64.h
index f2c02e4469cc..9febfbca3a88 100644
--- a/arch/x86/include/asm/uaccess_64.h
+++ b/arch/x86/include/asm/uaccess_64.h
@@ -88,6 +88,12 @@ static inline bool __access_ok(const void __user *ptr, unsigned long size)
return valid_user_address(ptr);
} else {
unsigned long sum = size + (unsigned long)ptr;
+
+#ifdef CONFIG_COMPAT
+ if (in_ia32_syscall())
+ return valid_user_address(sum) &&
+ sum >= (unsigned long)ptr && sum < UL(0x100000000);
+#endif
return valid_user_address(sum) && sum >= (unsigned long)ptr;
}
}
--
2.34.1

2023-11-16 04:32:38

by H. Peter Anvin

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

It's not clear to me that this is actually the correct behavior; the
important thing is that it faults (as nothing is mapped above 4 GB anyway.)

-hpa


On 11/15/23 02:56, 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
> insufficient. The specified "ptr + size" is greater than 32-bit limit and
> should return -EFAULT, but it is not checked and can not catch the overflow
> error.
>
> Fix above error by checking 32-bit limit if it is 32-bit compatible
> userspace program.

-hpa