From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qt0-f180.google.com ([209.85.216.180]:35842 "EHLO mail-qt0-f180.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752168AbdFMLW1 (ORCPT ); Tue, 13 Jun 2017 07:22:27 -0400 Received: by mail-qt0-f180.google.com with SMTP id u19so166744014qta.3 for ; Tue, 13 Jun 2017 04:22:27 -0700 (PDT) Message-ID: <1497352945.5762.13.camel@redhat.com> Subject: Re: [PATCH v2 2/2] fs/fcntl: f_setown, avoid undefined behaviour From: Jeff Layton To: Jiri Slaby Cc: linux-kernel@vger.kernel.org, "J. Bruce Fields" , Alexander Viro , "" Date: Tue, 13 Jun 2017 07:22:25 -0400 In-Reply-To: <935967fa-ce37-d3b2-7e7f-cb8e078abe49@suse.cz> References: <20170613092254.22235-1-jslaby@suse.cz> <20170613092254.22235-2-jslaby@suse.cz> <1497348689.4601.2.camel@redhat.com> <935967fa-ce37-d3b2-7e7f-cb8e078abe49@suse.cz> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: linux-fsdevel-owner@vger.kernel.org List-ID: 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: > > > ... > > > [] ? f_setown+0x1d8/0x200 > > > [] ? SyS_fcntl+0x999/0xf30 > > > [] ? 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 > > > Cc: Jeff Layton > > > Cc: "J. Bruce Fields" > > > Cc: Alexander Viro > > > 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..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