lkml.org 
[lkml]   [2017]   [Jun]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v2 2/2] fs/fcntl: f_setown, avoid undefined behaviour
From
Date
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>

\
 
 \ /
  Last update: 2017-06-13 13:23    [W:0.041 / U:0.072 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site