linux-kernel - Re: [RFC PATCH] sys_read: add a compat_sys

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:	Wed, 08 Jun 2016 00:33:54 -0700
From:	"H. Peter Anvin" <hpa@...or.com>
To:	"Zhangjian (Bamvor)" <bamvor.zhangjian@...wei.com>,
	Weidong Wang <wangweidong1@...wei.com>, tglx@...utronix.de,
	mingo@...hat.com, viro@...iv.linux.org.uk, arnd@...db.de
CC:	linux-kernel@...r.kernel.org, x86@...nel.org,
	Hanjun Guo <guohanjun@...wei.com>,
	Linux-Api <linux-api@...r.kernel.org>
Subject: Re: [RFC PATCH] sys_read: add a compat_sys_read for 64bit system

On June 7, 2016 7:14:41 PM PDT, "Zhangjian (Bamvor)" <bamvor.zhangjian@...wei.com> wrote:
>Hi,
>
>On 2016/6/8 9:33, Weidong Wang wrote:
>> Test 32 progress and 64 progress on the 64bit system with
>> this progress:
>>
>> int main(int argc, char **argv)
>> {
>>          int fd = 0;
>>          int i, ret = 0;
>>          char buf[512];
>>          unsigned long count = -1;
>>
>>          fd = open("/tmp", O_RDONLY);
>>          if (fd < -1) {
>>                  printf("Pls check the directory is exist?\n");
>>                  return -1;
>>          }
>>          errno = 0;
>>          ret = read(fd, NULL, count);
>>          printf("Ret is %d errno %d\n", ret, errno);
>>          close(fd);
>>
>>          return 0;
>> }
>>
>> we get the different errno. The 64 progress we get errno is -14 while
>> the 32 progress is -21.
>>
>> The reason is that, the user progress would use a 32bit count, while
>> the sys_read size_t in kernel is 64bit.  When the uesrspace count is
>> -1(0xffffffff), it goes to the sys_read, it would be change to a
>positive
>> number.
>>
>> So I think we should add a compat_sys_read for the read syscall. I
>test it
>> on x86 or arm64 platform. The patch works well.
>As weidong said, we tested on x86, x86_64, aarch64 ilp32, aarch64 lp64.
>We do not familiar with other architecture, cc linux-api, hope could
>get more
>input.
>
>Regards
>
>Bamvor
>>
>> As well this patch may do work for the 'tile' 64 system.
>> I think it may enter the same result on mips/parisc/powerpc/sparc.
>> The s390 do the compat_sys_s390_read for the compat sys_read.
>>
>> Signed-off-by: Weidong Wang <wangweidong1@...wei.com>
>> ---
>>   arch/x86/entry/syscalls/syscall_32.tbl | 2 +-
>>   fs/read_write.c                        | 8 ++++++++
>>   include/linux/compat.h                 | 2 ++
>>   include/uapi/asm-generic/unistd.h      | 2 +-
>>   4 files changed, 12 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/entry/syscalls/syscall_32.tbl
>b/arch/x86/entry/syscalls/syscall_32.tbl
>> index 4cddd17..ebc24e3 100644
>> --- a/arch/x86/entry/syscalls/syscall_32.tbl
>> +++ b/arch/x86/entry/syscalls/syscall_32.tbl
>> @@ -9,7 +9,7 @@
>>   0	i386	restart_syscall		sys_restart_syscall
>>   1	i386	exit			sys_exit
>>   2	i386	fork			sys_fork			sys_fork
>> -3	i386	read			sys_read
>> +3	i386	read			sys_read			compat_sys_read
>>   4	i386	write			sys_write
>>   5	i386	open			sys_open			compat_sys_open
>>   6	i386	close			sys_close
>> diff --git a/fs/read_write.c b/fs/read_write.c
>> index 933b53a..d244848 100644
>> --- a/fs/read_write.c
>> +++ b/fs/read_write.c
>> @@ -613,6 +613,14 @@ SYSCALL_DEFINE3(write, unsigned int, fd, const
>char __user *, buf,
>>   	return ret;
>>   }
>>
>> +#ifdef CONFIG_COMPAT
>> +COMPAT_SYSCALL_DEFINE3(read, unsigned int, fd, char __user *, buf,
>> +		compat_size_t, count)
>> +{
>> +        return sys_read(fd, buf, (compat_ssize_t)count);
>> +}
>> +#endif
>> +
>>   SYSCALL_DEFINE4(pread64, unsigned int, fd, char __user *, buf,
>>   			size_t, count, loff_t, pos)
>>   {
>> diff --git a/include/linux/compat.h b/include/linux/compat.h
>> index f964ef7..d88ccad 100644
>> --- a/include/linux/compat.h
>> +++ b/include/linux/compat.h
>> @@ -332,6 +332,8 @@ asmlinkage long compat_sys_keyctl(u32 option,
>>   			      u32 arg2, u32 arg3, u32 arg4, u32 arg5);
>>   asmlinkage long compat_sys_ustat(unsigned dev, struct compat_ustat
>__user *u32);
>>
>> +asmlinkage ssize_t compat_sys_read(unsigned int fd,
>> +		char __user * buf, compat_size_t count);
>>   asmlinkage ssize_t compat_sys_readv(compat_ulong_t fd,
>>   		const struct compat_iovec __user *vec, compat_ulong_t vlen);
>>   asmlinkage ssize_t compat_sys_writev(compat_ulong_t fd,
>> diff --git a/include/uapi/asm-generic/unistd.h
>b/include/uapi/asm-generic/unistd.h
>> index a26415b..745818a 100644
>> --- a/include/uapi/asm-generic/unistd.h
>> +++ b/include/uapi/asm-generic/unistd.h
>> @@ -201,7 +201,7 @@ __SC_COMP(__NR_getdents64, sys_getdents64,
>compat_sys_getdents64)
>>   #define __NR3264_lseek 62
>>   __SC_3264(__NR3264_lseek, sys_llseek, sys_lseek)
>>   #define __NR_read 63
>> -__SYSCALL(__NR_read, sys_read)
>> +__SC_COMP(__NR_read, sys_read, compat_sys_read)
>>   #define __NR_write 64
>>   __SYSCALL(__NR_write, sys_write)
>>   #define __NR_readv 65
>>

Does this cause any actual problems?  Also, it seems extremely unlikely read() would be the only system call so affected.
-- 
Sent from my Android device with K-9 Mail. Please excuse brevity and formatting.