* [PATCH v2 1/2] fs/fcntl: f_setown, allow returning error
@ 2017-06-13 9:22 Jiri Slaby
2017-06-13 9:22 ` [PATCH v2 2/2] fs/fcntl: f_setown, avoid undefined behaviour Jiri Slaby
2017-06-13 10:02 ` [PATCH v2 1/2] fs/fcntl: f_setown, allow returning error Jeff Layton
0 siblings, 2 replies; 6+ messages in thread
From: Jiri Slaby @ 2017-06-13 9:22 UTC (permalink / raw)
To: jlayton
Cc: linux-kernel, Jiri Slaby, Jeff Layton, J. Bruce Fields,
Alexander Viro, linux-fsdevel
Allow f_setown to return an error value. We will fail in the next patch
with EINVAL for bad input to f_setown, so tile the path for the later
patch.
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 | 7 ++++---
include/linux/fs.h | 2 +-
net/socket.c | 3 +--
3 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/fs/fcntl.c b/fs/fcntl.c
index bbf80344c125..313eba860346 100644
--- a/fs/fcntl.c
+++ b/fs/fcntl.c
@@ -109,7 +109,7 @@ void __f_setown(struct file *filp, struct pid *pid, enum pid_type type,
}
EXPORT_SYMBOL(__f_setown);
-void f_setown(struct file *filp, unsigned long arg, int force)
+int f_setown(struct file *filp, unsigned long arg, int force)
{
enum pid_type type;
struct pid *pid;
@@ -123,6 +123,8 @@ void f_setown(struct file *filp, unsigned long arg, int force)
pid = find_vpid(who);
__f_setown(filp, pid, type, force);
rcu_read_unlock();
+
+ return 0;
}
EXPORT_SYMBOL(f_setown);
@@ -305,8 +307,7 @@ static long do_fcntl(int fd, unsigned int cmd, unsigned long arg,
force_successful_syscall_return();
break;
case F_SETOWN:
- f_setown(filp, arg, 1);
- err = 0;
+ err = f_setown(filp, arg, 1);
break;
case F_GETOWN_EX:
err = f_getown_ex(filp, arg);
diff --git a/include/linux/fs.h b/include/linux/fs.h
index ecc301043abf..6dd215a339d4 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1250,7 +1250,7 @@ extern void fasync_free(struct fasync_struct *);
extern void kill_fasync(struct fasync_struct **, int, int);
extern void __f_setown(struct file *filp, struct pid *, enum pid_type, int force);
-extern void f_setown(struct file *filp, unsigned long arg, int force);
+extern int f_setown(struct file *filp, unsigned long arg, int force);
extern void f_delown(struct file *filp);
extern pid_t f_getown(struct file *filp);
extern int send_sigurg(struct fown_struct *fown);
diff --git a/net/socket.c b/net/socket.c
index 8f9dab330d57..59e902b9df09 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -991,8 +991,7 @@ static long sock_ioctl(struct file *file, unsigned cmd, unsigned long arg)
err = -EFAULT;
if (get_user(pid, (int __user *)argp))
break;
- f_setown(sock->file, pid, 1);
- err = 0;
+ err = f_setown(sock->file, pid, 1);
break;
case FIOGETOWN:
case SIOCGPGRP:
--
2.13.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v2 2/2] fs/fcntl: f_setown, avoid undefined behaviour
2017-06-13 9:22 [PATCH v2 1/2] fs/fcntl: f_setown, allow returning error Jiri Slaby
@ 2017-06-13 9:22 ` Jiri Slaby
2017-06-13 10:11 ` Jeff Layton
2017-06-13 10:02 ` [PATCH v2 1/2] fs/fcntl: f_setown, allow returning error Jeff Layton
1 sibling, 1 reply; 6+ messages in thread
From: Jiri Slaby @ 2017-06-13 9:22 UTC (permalink / raw)
To: jlayton
Cc: linux-kernel, Jiri Slaby, Jeff Layton, J. Bruce Fields,
Alexander Viro, linux-fsdevel
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;
--
2.13.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/2] fs/fcntl: f_setown, allow returning error
2017-06-13 9:22 [PATCH v2 1/2] fs/fcntl: f_setown, allow returning error Jiri Slaby
2017-06-13 9:22 ` [PATCH v2 2/2] fs/fcntl: f_setown, avoid undefined behaviour Jiri Slaby
@ 2017-06-13 10:02 ` Jeff Layton
1 sibling, 0 replies; 6+ messages in thread
From: Jeff Layton @ 2017-06-13 10:02 UTC (permalink / raw)
To: Jiri Slaby; +Cc: linux-kernel, J. Bruce Fields, Alexander Viro, linux-fsdevel
On Tue, 2017-06-13 at 11:22 +0200, Jiri Slaby wrote:
> Allow f_setown to return an error value. We will fail in the next patch
> with EINVAL for bad input to f_setown, so tile the path for the later
> patch.
>
> 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 | 7 ++++---
> include/linux/fs.h | 2 +-
> net/socket.c | 3 +--
> 3 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/fs/fcntl.c b/fs/fcntl.c
> index bbf80344c125..313eba860346 100644
> --- a/fs/fcntl.c
> +++ b/fs/fcntl.c
> @@ -109,7 +109,7 @@ void __f_setown(struct file *filp, struct pid *pid, enum pid_type type,
> }
> EXPORT_SYMBOL(__f_setown);
>
> -void f_setown(struct file *filp, unsigned long arg, int force)
> +int f_setown(struct file *filp, unsigned long arg, int force)
> {
> enum pid_type type;
> struct pid *pid;
> @@ -123,6 +123,8 @@ void f_setown(struct file *filp, unsigned long arg, int force)
> pid = find_vpid(who);
> __f_setown(filp, pid, type, force);
> rcu_read_unlock();
> +
> + return 0;
> }
> EXPORT_SYMBOL(f_setown);
>
> @@ -305,8 +307,7 @@ static long do_fcntl(int fd, unsigned int cmd, unsigned long arg,
> force_successful_syscall_return();
> break;
> case F_SETOWN:
> - f_setown(filp, arg, 1);
> - err = 0;
> + err = f_setown(filp, arg, 1);
> break;
> case F_GETOWN_EX:
> err = f_getown_ex(filp, arg);
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index ecc301043abf..6dd215a339d4 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -1250,7 +1250,7 @@ extern void fasync_free(struct fasync_struct *);
> extern void kill_fasync(struct fasync_struct **, int, int);
>
> extern void __f_setown(struct file *filp, struct pid *, enum pid_type, int force);
> -extern void f_setown(struct file *filp, unsigned long arg, int force);
> +extern int f_setown(struct file *filp, unsigned long arg, int force);
> extern void f_delown(struct file *filp);
> extern pid_t f_getown(struct file *filp);
> extern int send_sigurg(struct fown_struct *fown);
> diff --git a/net/socket.c b/net/socket.c
> index 8f9dab330d57..59e902b9df09 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -991,8 +991,7 @@ static long sock_ioctl(struct file *file, unsigned cmd, unsigned long arg)
> err = -EFAULT;
> if (get_user(pid, (int __user *)argp))
> break;
> - f_setown(sock->file, pid, 1);
> - err = 0;
> + err = f_setown(sock->file, pid, 1);
> break;
> case FIOGETOWN:
> case SIOCGPGRP:
Looks good:
Reviewed-by: Jeff Layton <jlayton@redhat.com>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] fs/fcntl: f_setown, avoid undefined behaviour
2017-06-13 9:22 ` [PATCH v2 2/2] fs/fcntl: f_setown, avoid undefined behaviour Jiri Slaby
@ 2017-06-13 10:11 ` Jeff Layton
2017-06-13 11:10 ` Jiri Slaby
0 siblings, 1 reply; 6+ messages in thread
From: Jeff Layton @ 2017-06-13 10:11 UTC (permalink / raw)
To: Jiri Slaby; +Cc: linux-kernel, J. Bruce Fields, Alexander Viro, linux-fsdevel
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.
--
Jeff Layton <jlayton@redhat.com>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] fs/fcntl: f_setown, avoid undefined behaviour
2017-06-13 10:11 ` Jeff Layton
@ 2017-06-13 11:10 ` Jiri Slaby
2017-06-13 11:22 ` Jeff Layton
0 siblings, 1 reply; 6+ messages in thread
From: Jiri Slaby @ 2017-06-13 11:10 UTC (permalink / raw)
To: Jeff Layton; +Cc: linux-kernel, J. Bruce Fields, Alexander Viro, linux-fsdevel
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,
--
js
suse labs
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] fs/fcntl: f_setown, avoid undefined behaviour
2017-06-13 11:10 ` Jiri Slaby
@ 2017-06-13 11:22 ` Jeff Layton
0 siblings, 0 replies; 6+ messages in thread
From: Jeff Layton @ 2017-06-13 11:22 UTC (permalink / raw)
To: Jiri Slaby
Cc: linux-kernel, J. Bruce Fields, Alexander Viro,
<linux-fsdevel@vger.kernel.org>
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>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-06-13 11:22 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-06-13 9:22 [PATCH v2 1/2] fs/fcntl: f_setown, allow returning error Jiri Slaby
2017-06-13 9:22 ` [PATCH v2 2/2] fs/fcntl: f_setown, avoid undefined behaviour Jiri Slaby
2017-06-13 10:11 ` Jeff Layton
2017-06-13 11:10 ` Jiri Slaby
2017-06-13 11:22 ` Jeff Layton
2017-06-13 10:02 ` [PATCH v2 1/2] fs/fcntl: f_setown, allow returning error Jeff Layton
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).