Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757422AbcJXL3q (ORCPT ); Mon, 24 Oct 2016 07:29:46 -0400 Received: from mail-qt0-f169.google.com ([209.85.216.169]:33718 "EHLO mail-qt0-f169.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756129AbcJXL3n (ORCPT ); Mon, 24 Oct 2016 07:29:43 -0400 Message-ID: <1477308581.2625.20.camel@redhat.com> Subject: Re: [PATCH] fs: fcntl, avoid undefined behaviour From: Jeff Layton To: Jiri Slaby , "J. Bruce Fields" Cc: viro@zeniv.linux.org.uk, linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org Date: Mon, 24 Oct 2016 07:29:41 -0400 In-Reply-To: <60f57792-8b4a-8ae0-f0fe-42db78bc795e@suse.cz> References: <20161014092342.25546-1-jslaby@suse.cz> <1476445695.12134.19.camel@redhat.com> <20161014133846.GB16547@fieldses.org> <60f57792-8b4a-8ae0-f0fe-42db78bc795e@suse.cz> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.20.5 (3.20.5-1.fc24) Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2909 Lines: 76 On Mon, 2016-10-24 at 11:15 +0200, Jiri Slaby wrote: > On 10/14/2016, 03:38 PM, J. Bruce Fields wrote: > > > > On Fri, Oct 14, 2016 at 07:48:15AM -0400, Jeff Layton wrote: > > > > > > On Fri, 2016-10-14 at 11:23 +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) and > > > > return immediatelly in case it is wrong. No error is returned, the > > > > same as in other cases. > > > > > > > > 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 350a2c8cfd28..bfc3b040d956 100644 > > > > --- a/fs/fcntl.c > > > > +++ b/fs/fcntl.c > > > > @@ -112,6 +112,10 @@ void 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; > > > > + > > > > type = PIDTYPE_PID; > > > > if (who < 0) { > > > > type = PIDTYPE_PGID; > > > > > > Might it be better to change f_setown to return int there, so you can > > > return -EINVAL in that case? The other caller (sock_ioctl) can also > > > handle an int return there too... > > > > That might also be worth a note in the RETURN VALUE section of fcntl(2), > > which goes into surprising detail about the EINVAL cases for different > > commands. > > Yes, I checked POSIX before I sent the patch and it does not explicitly > document EINVAL, neither an error from SETOWN. So I am not sure whether > at this point we can start returning an error without breaking userspace? > > thanks, It looks like it lists this as a "may fail" case: 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. IMO, returning an error here is the right thing to do. Either the application isn't checking for errors, in which case returning one won't matter, or it is, and they probably want to be informed that their F_SETOWN didn't do what they expected. -- Jeff Layton