Re: [PATCH v2 2/2] fs/fcntl: f_setown, avoid undefined behaviour

[Jeff Layton <jlayton@redhat.com>]

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:
> > > ...
> > >  [<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) 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 <jslaby@suse.cz>
> > > Cc: Jeff Layton <jlayton@poochiereds.net>
> > > Cc: "J. Bruce Fields" <bfields@fieldses.org>
> > > Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> > > 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 <jlayton@redhat.com>