On 06/13/2017, 12:11 PM, Jeff Layton wrote: > On Tue, 2017-06-13 at 11:22 +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 >> >> 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..db853670e22f 100644 >> --- a/fs/fcntl.c >> +++ b/fs/fcntl.c >> @@ -114,6 +114,10 @@ int f_setown(struct file *filp, unsigned long arg, int force) >> enum pid_type type; >> struct pid *pid; >> int who = arg; >> + >> + if (arg > INT_MAX) >> + return -EINVAL; >> + >> type = PIDTYPE_PID; >> if (who < 0) { >> type = PIDTYPE_PGID; > > The next part here says: > > if (who < 0) { > type = PIDTYPE_PGID; > who = -who; > } > > Won't this break the ability to pass in a pgid? Valid negative values > will end up getting back -EINVAL here, AFAICT. Of course it will. What was I thinking? So catch: a) ==== the single case? ==== if (who == INT_MIN) return -EINVAL; if (who < 0) { type = PIDTYPE_PGID; who = -who; } b) ==== or all the larger values? ==== if (who == INT_MIN || arg != (unsigned)who) return -EINVAL; if (who < 0) { type = PIDTYPE_PGID; who = -who; } ==== The former added test could be inside the "if (who < 0) { }", alternatively. thanks, -- js suse labs
