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@xxxxxxx>
> Cc: Jeff Layton <jlayton@xxxxxxxxxxxxxxx>
> Cc: "J. Bruce Fields" <bfields@xxxxxxxxxxxx>
> Cc: Alexander Viro <viro@xxxxxxxxxxxxxxxxxx>
> Cc: linux-fsdevel@xxxxxxxxxxxxxxx
> ---
>  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@xxxxxxxxxx>