From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752692AbdFMLKx (ORCPT ); Tue, 13 Jun 2017 07:10:53 -0400 Received: from mail-wr0-f196.google.com ([209.85.128.196]:32934 "EHLO mail-wr0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752033AbdFMLKw (ORCPT ); Tue, 13 Jun 2017 07:10:52 -0400 Subject: Re: [PATCH v2 2/2] fs/fcntl: f_setown, avoid undefined behaviour To: Jeff Layton Cc: linux-kernel@vger.kernel.org, "J. Bruce Fields" , Alexander Viro , linux-fsdevel@vger.kernel.org References: <20170613092254.22235-1-jslaby@suse.cz> <20170613092254.22235-2-jslaby@suse.cz> <1497348689.4601.2.camel@redhat.com> From: Jiri Slaby Message-ID: <935967fa-ce37-d3b2-7e7f-cb8e078abe49@suse.cz> Date: Tue, 13 Jun 2017 13:10:48 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.1.1 MIME-Version: 1.0 In-Reply-To: <1497348689.4601.2.camel@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-GB Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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, -- js suse labs