linux-kernel - Re: [PATCH] fs: fcntl, avoid undefined behaviour

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 24 Oct 2016 07:29:41 -0400
From:   Jeff Layton <jlayton@...hat.com>
To:     Jiri Slaby <jslaby@...e.cz>,
        "J. Bruce Fields" <bfields@...ldses.org>
Cc:     viro@...iv.linux.org.uk, linux-kernel@...r.kernel.org,
        linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH] fs: fcntl, avoid undefined behaviour

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:
> > > > ...
> > > >  [<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) and
> > > > return immediatelly in case it is wrong. No error is returned, the
> > > > same as in other cases.
> > > > 
> > > > Signed-off-by: Jiri Slaby <jslaby@...e.cz>
> > > > Cc: Jeff Layton <jlayton@...chiereds.net>
> > > > Cc: "J. Bruce Fields" <bfields@...ldses.org>
> > > > Cc: Alexander Viro <viro@...iv.linux.org.uk>
> > > > Cc: linux-fsdevel@...r.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 <jlayton@...hat.com>