[PATCH v3 1/2] fs/fcntl: f_setown, allow returning error

[PATCH v3 1/2] fs/fcntl: f_setown, allow returning error

[Jiri Slaby]

Allow f_setown to return an error value. We will fail in the next patch
with EINVAL for bad input to f_setown, so tile the path for the later
patch.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Reviewed-by: Jeff Layton <jlayton@redhat.com>
Cc: Jeff Layton <jlayton@poochiereds.net>
Cc: "J. Bruce Fields" <bfields@fieldses.org>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>
Cc: linux-fsdevel@vger.kernel.org
---
 fs/fcntl.c         | 7 ++++---
 include/linux/fs.h | 2 +-
 net/socket.c       | 3 +--
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/fs/fcntl.c b/fs/fcntl.c
index bbf80344c125..313eba860346 100644
--- a/fs/fcntl.c
+++ b/fs/fcntl.c
@@ -109,7 +109,7 @@ void __f_setown(struct file *filp, struct pid *pid, enum pid_type type,
 }
 EXPORT_SYMBOL(__f_setown);
 
-void f_setown(struct file *filp, unsigned long arg, int force)
+int f_setown(struct file *filp, unsigned long arg, int force)
 {
 	enum pid_type type;
 	struct pid *pid;
@@ -123,6 +123,8 @@ void f_setown(struct file *filp, unsigned long arg, int force)
 	pid = find_vpid(who);
 	__f_setown(filp, pid, type, force);
 	rcu_read_unlock();
+
+	return 0;
 }
 EXPORT_SYMBOL(f_setown);
 
@@ -305,8 +307,7 @@ static long do_fcntl(int fd, unsigned int cmd, unsigned long arg,
 		force_successful_syscall_return();
 		break;
 	case F_SETOWN:
-		f_setown(filp, arg, 1);
-		err = 0;
+		err = f_setown(filp, arg, 1);
 		break;
 	case F_GETOWN_EX:
 		err = f_getown_ex(filp, arg);
diff --git a/include/linux/fs.h b/include/linux/fs.h
index ecc301043abf..6dd215a339d4 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1250,7 +1250,7 @@ extern void fasync_free(struct fasync_struct *);
 extern void kill_fasync(struct fasync_struct **, int, int);
 
 extern void __f_setown(struct file *filp, struct pid *, enum pid_type, int force);
-extern void f_setown(struct file *filp, unsigned long arg, int force);
+extern int f_setown(struct file *filp, unsigned long arg, int force);
 extern void f_delown(struct file *filp);
 extern pid_t f_getown(struct file *filp);
 extern int send_sigurg(struct fown_struct *fown);
diff --git a/net/socket.c b/net/socket.c
index 8f9dab330d57..59e902b9df09 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -991,8 +991,7 @@ static long sock_ioctl(struct file *file, unsigned cmd, unsigned long arg)
 			err = -EFAULT;
 			if (get_user(pid, (int __user *)argp))
 				break;
-			f_setown(sock->file, pid, 1);
-			err = 0;
+			err = f_setown(sock->file, pid, 1);
 			break;
 		case FIOGETOWN:
 		case SIOCGPGRP:
-- 
2.13.1

[PATCH v3 2/2] fs/fcntl: f_setown, avoid undefined behaviour

[Jiri Slaby]

fcntl(0, F_SETOWN, 0x80000000) triggers:
UBSAN: Undefined behaviour in fs/fcntl.c:118:7
negation of -2147483648 cannot be represented in type 'int':
CPU: 1 PID: 18261 Comm: syz-executor Not tainted 4.8.1-0-syzkaller #1
...
Call Trace:
...
 [<ffffffffad8f0868>] ? f_setown+0x1d8/0x200
 [<ffffffffad8f19a9>] ? SyS_fcntl+0x999/0xf30
 [<ffffffffaed1fb00>] ? entry_SYSCALL_64_fastpath+0x23/0xc1

Fix that by checking the arg parameter properly (against INT_MAX) before
"who = -who". And return immediatelly with -EINVAL in case it is wrong.
Note that according to POSIX we can return EINVAL:
    http://pubs.opengroup.org/onlinepubs/9699919799/functions/fcntl.html

    [EINVAL]
        The cmd argument is F_SETOWN and the value of the argument
        is not valid as a process or process group identifier.

[v2] returns an error, v1 used to fail silently
[v3] implement proper check for the bad value INT_MIN

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Cc: Jeff Layton <jlayton@poochiereds.net>
Cc: "J. Bruce Fields" <bfields@fieldses.org>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>
Cc: linux-fsdevel@vger.kernel.org
---
 fs/fcntl.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/fs/fcntl.c b/fs/fcntl.c
index 313eba860346..693322e28751 100644
--- a/fs/fcntl.c
+++ b/fs/fcntl.c
@@ -116,6 +116,10 @@ int f_setown(struct file *filp, unsigned long arg, int force)
 	int who = arg;
 	type = PIDTYPE_PID;
 	if (who < 0) {
+		/* avoid overflow below */
+		if (who == INT_MIN)
+			return -EINVAL;
+
 		type = PIDTYPE_PGID;
 		who = -who;
 	}
-- 
2.13.1

Re: [PATCH v3 2/2] fs/fcntl: f_setown, avoid undefined behaviour

[Jeff Layton]

On Tue, 2017-06-13 at 13:35 +0200, Jiri Slaby wrote:
> fcntl(0, F_SETOWN, 0x80000000) triggers:
> UBSAN: Undefined behaviour in fs/fcntl.c:118:7
> negation of -2147483648 cannot be represented in type 'int':
> CPU: 1 PID: 18261 Comm: syz-executor Not tainted 4.8.1-0-syzkaller #1
> ...
> Call Trace:
> ...
>  [<ffffffffad8f0868>] ? f_setown+0x1d8/0x200
>  [<ffffffffad8f19a9>] ? SyS_fcntl+0x999/0xf30
>  [<ffffffffaed1fb00>] ? entry_SYSCALL_64_fastpath+0x23/0xc1
> 
> Fix that by checking the arg parameter properly (against INT_MAX) before
> "who = -who". And return immediatelly with -EINVAL in case it is wrong.
> Note that according to POSIX we can return EINVAL:
>     http://pubs.opengroup.org/onlinepubs/9699919799/functions/fcntl.html
> 
>     [EINVAL]
>         The cmd argument is F_SETOWN and the value of the argument
>         is not valid as a process or process group identifier.
> 
> [v2] returns an error, v1 used to fail silently
> [v3] implement proper check for the bad value INT_MIN
> 
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
> Cc: Jeff Layton <jlayton@poochiereds.net>
> Cc: "J. Bruce Fields" <bfields@fieldses.org>
> Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> Cc: linux-fsdevel@vger.kernel.org
> ---
>  fs/fcntl.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/fs/fcntl.c b/fs/fcntl.c
> index 313eba860346..693322e28751 100644
> --- a/fs/fcntl.c
> +++ b/fs/fcntl.c
> @@ -116,6 +116,10 @@ int f_setown(struct file *filp, unsigned long arg, int force)
>  	int who = arg;
>  	type = PIDTYPE_PID;
>  	if (who < 0) {
> +		/* avoid overflow below */
> +		if (who == INT_MIN)
> +			return -EINVAL;
> +
>  		type = PIDTYPE_PGID;
>  		who = -who;
>  	}

Seems reasonable.

I do somewhat lean toward checking for all larger values, but there
could be userland programs that leave the upper bits set when they cast
this to unsigned long. This is probably the safer thing to do.

I'll plan to pick these up for v4.12.

On the other related note...I think we ought to return ESRCH when
find_vpid returns NULL. I'll take a look at that sometime soon too.

Thanks!
-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH v3 2/2] fs/fcntl: f_setown, avoid undefined behaviour

[zhong jiang]

On 2017/6/13 20:13, Jeff Layton wrote:
> On Tue, 2017-06-13 at 13:35 +0200, Jiri Slaby wrote:
>> fcntl(0, F_SETOWN, 0x80000000) triggers:
>> UBSAN: Undefined behaviour in fs/fcntl.c:118:7
>> negation of -2147483648 cannot be represented in type 'int':
>> CPU: 1 PID: 18261 Comm: syz-executor Not tainted 4.8.1-0-syzkaller #1
>> ...
>> Call Trace:
>> ...
>>  [<ffffffffad8f0868>] ? f_setown+0x1d8/0x200
>>  [<ffffffffad8f19a9>] ? SyS_fcntl+0x999/0xf30
>>  [<ffffffffaed1fb00>] ? entry_SYSCALL_64_fastpath+0x23/0xc1
>>
>> Fix that by checking the arg parameter properly (against INT_MAX) before
>> "who = -who". And return immediatelly with -EINVAL in case it is wrong.
>> Note that according to POSIX we can return EINVAL:
>>     http://pubs.opengroup.org/onlinepubs/9699919799/functions/fcntl.html
>>
>>     [EINVAL]
>>         The cmd argument is F_SETOWN and the value of the argument
>>         is not valid as a process or process group identifier.
>>
>> [v2] returns an error, v1 used to fail silently
>> [v3] implement proper check for the bad value INT_MIN
>>
>> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
>> Cc: Jeff Layton <jlayton@poochiereds.net>
>> Cc: "J. Bruce Fields" <bfields@fieldses.org>
>> Cc: Alexander Viro <viro@zeniv.linux.org.uk>
>> Cc: linux-fsdevel@vger.kernel.org
>> ---
>>  fs/fcntl.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/fs/fcntl.c b/fs/fcntl.c
>> index 313eba860346..693322e28751 100644
>> --- a/fs/fcntl.c
>> +++ b/fs/fcntl.c
>> @@ -116,6 +116,10 @@ int f_setown(struct file *filp, unsigned long arg, int force)
>>  	int who = arg;
>>  	type = PIDTYPE_PID;
>>  	if (who < 0) {
>> +		/* avoid overflow below */
>> +		if (who == INT_MIN)
>> +			return -EINVAL;
>> +
>>  		type = PIDTYPE_PGID;
>>  		who = -who;
>>  	}
> Seems reasonable.
>
> I do somewhat lean toward checking for all larger values, but there
> could be userland programs that leave the upper bits set when they cast
> this to unsigned long. This is probably the safer thing to do.
>
> I'll plan to pick these up for v4.12.
>
> On the other related note...I think we ought to return ESRCH when
> find_vpid returns NULL. I'll take a look at that sometime soon too.
>
> Thanks!
 hi, jeff

 I have sent the patch about find_vpid ,and exist in linux-next branch

 https://patchwork.kernel.org/patch/9766259/

 Thanks
zhongjiang

Re: [PATCH v3 1/2] fs/fcntl: f_setown, allow returning error

[zhong jiang]

On 2017/6/13 19:35, Jiri Slaby wrote:
> Allow f_setown to return an error value. We will fail in the next patch
> with EINVAL for bad input to f_setown, so tile the path for the later
> patch.
>
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
> Reviewed-by: Jeff Layton <jlayton@redhat.com>
> Cc: Jeff Layton <jlayton@poochiereds.net>
> Cc: "J. Bruce Fields" <bfields@fieldses.org>
> Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> Cc: linux-fsdevel@vger.kernel.org
> ---
>  fs/fcntl.c         | 7 ++++---
>  include/linux/fs.h | 2 +-
>  net/socket.c       | 3 +--
>  3 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/fs/fcntl.c b/fs/fcntl.c
> index bbf80344c125..313eba860346 100644
> --- a/fs/fcntl.c
> +++ b/fs/fcntl.c
> @@ -109,7 +109,7 @@ void __f_setown(struct file *filp, struct pid *pid, enum pid_type type,
>  }
>  EXPORT_SYMBOL(__f_setown);
>  
> -void f_setown(struct file *filp, unsigned long arg, int force)
> +int f_setown(struct file *filp, unsigned long arg, int force)
>  {
>  	enum pid_type type;
>  	struct pid *pid;
> @@ -123,6 +123,8 @@ void f_setown(struct file *filp, unsigned long arg, int force)
>  	pid = find_vpid(who);
>  	__f_setown(filp, pid, type, force);
>  	rcu_read_unlock();
> +
> +	return 0;
>  }
>  EXPORT_SYMBOL(f_setown);
>  
> @@ -305,8 +307,7 @@ static long do_fcntl(int fd, unsigned int cmd, unsigned long arg,
>  		force_successful_syscall_return();
>  		break;
>  	case F_SETOWN:
> -		f_setown(filp, arg, 1);
> -		err = 0;
> +		err = f_setown(filp, arg, 1);
>  		break;
>  	case F_GETOWN_EX:
>  		err = f_getown_ex(filp, arg);
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index ecc301043abf..6dd215a339d4 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -1250,7 +1250,7 @@ extern void fasync_free(struct fasync_struct *);
>  extern void kill_fasync(struct fasync_struct **, int, int);
>  
>  extern void __f_setown(struct file *filp, struct pid *, enum pid_type, int force);
> -extern void f_setown(struct file *filp, unsigned long arg, int force);
> +extern int f_setown(struct file *filp, unsigned long arg, int force);
>  extern void f_delown(struct file *filp);
>  extern pid_t f_getown(struct file *filp);
>  extern int send_sigurg(struct fown_struct *fown);
> diff --git a/net/socket.c b/net/socket.c
> index 8f9dab330d57..59e902b9df09 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -991,8 +991,7 @@ static long sock_ioctl(struct file *file, unsigned cmd, unsigned long arg)
>  			err = -EFAULT;
>  			if (get_user(pid, (int __user *)argp))
>  				break;
> -			f_setown(sock->file, pid, 1);
> -			err = 0;
> +			err = f_setown(sock->file, pid, 1);
>  			break;
>  		case FIOGETOWN:
>  		case SIOCGPGRP:
  yes , Look good to me.  

 Thanks
 zhongjiang