From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qt0-f171.google.com ([209.85.216.171]:33793 "EHLO mail-qt0-f171.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751888AbdFMKLc (ORCPT ); Tue, 13 Jun 2017 06:11:32 -0400 Received: by mail-qt0-f171.google.com with SMTP id c10so164250881qtd.1 for ; Tue, 13 Jun 2017 03:11:31 -0700 (PDT) Message-ID: <1497348689.4601.2.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 , linux-fsdevel@vger.kernel.org Date: Tue, 13 Jun 2017 06:11:29 -0400 In-Reply-To: <20170613092254.22235-2-jslaby@suse.cz> References: <20170613092254.22235-1-jslaby@suse.cz> <20170613092254.22235-2-jslaby@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 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. -- Jeff Layton