On Tue, 2017-06-13 at 13:10 +0200, Jiri Slaby wrote: > 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, It's up to you how you want to phrase it. This is not something that is called very often the context of a program, so I'd aim to make it very obvious what the check does. Also, something not directly related to your patch, but someone could pass in a pid that's in the right range but that doesn't actually exist. If someone does that today, I think we end up calling __f_setown with a NULL struct pid pointer, which looks like it'll just clear out the f_owner struct. The POSIX fcntl page says we're supposed to return this in that case: [ESRCH] The cmd argument is F_SETOWN and no process or process group can be found corresponding to that specified by arg. Should we return that if find_vpid returns NULL? -- Jeff Layton <jlayton@xxxxxxxxxx>
