Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1664764 > unrolled thread
| Started by | Jiri Slaby <jslaby@suse.cz> |
|---|---|
| First post | 2017-06-13 13:40 +0200 |
| Last post | 2017-06-13 14:20 +0200 |
| Articles | 5 — 3 participants |
Back to article view | Back to linux.kernel
[PATCH v3 1/2] fs/fcntl: f_setown, allow returning error Jiri Slaby <jslaby@suse.cz> - 2017-06-13 13:40 +0200
[PATCH v3 2/2] fs/fcntl: f_setown, avoid undefined behaviour Jiri Slaby <jslaby@suse.cz> - 2017-06-13 13:40 +0200
Re: [PATCH v3 2/2] fs/fcntl: f_setown, avoid undefined behaviour Jeff Layton <jlayton@redhat.com> - 2017-06-13 14:20 +0200
Re: [PATCH v3 2/2] fs/fcntl: f_setown, avoid undefined behaviour zhong jiang <zhongjiang@huawei.com> - 2017-06-13 15:10 +0200
Re: [PATCH v3 1/2] fs/fcntl: f_setown, allow returning error zhong jiang <zhongjiang@huawei.com> - 2017-06-13 14:20 +0200
| From | Jiri Slaby <jslaby@suse.cz> |
|---|---|
| Date | 2017-06-13 13:40 +0200 |
| Subject | [PATCH v3 1/2] fs/fcntl: f_setown, allow returning error |
| Message-ID | <tRSkW-7nC-5@gated-at.bofh.it> |
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>
Reviewed-by: Jeff Layton <jlayton@redhat.com>
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
[toc] | [next] | [standalone]
| From | Jiri Slaby <jslaby@suse.cz> |
|---|---|
| Date | 2017-06-13 13:40 +0200 |
| Subject | [PATCH v3 2/2] fs/fcntl: f_setown, avoid undefined behaviour |
| Message-ID | <tRSkW-7nC-17@gated-at.bofh.it> |
| In reply to | #1664764 |
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
[v3] implement proper check for the bad value INT_MIN
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..693322e28751 100644
--- a/fs/fcntl.c
+++ b/fs/fcntl.c
@@ -116,6 +116,10 @@ int f_setown(struct file *filp, unsigned long arg, int force)
int who = arg;
type = PIDTYPE_PID;
if (who < 0) {
+ /* avoid overflow below */
+ if (who == INT_MIN)
+ return -EINVAL;
+
type = PIDTYPE_PGID;
who = -who;
}
--
2.13.1
[toc] | [prev] | [next] | [standalone]
| From | Jeff Layton <jlayton@redhat.com> |
|---|---|
| Date | 2017-06-13 14:20 +0200 |
| Subject | Re: [PATCH v3 2/2] fs/fcntl: f_setown, avoid undefined behaviour |
| Message-ID | <tRSXD-7Rj-1@gated-at.bofh.it> |
| In reply to | #1664768 |
On Tue, 2017-06-13 at 13:35 +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
> [v3] implement proper check for the bad value INT_MIN
>
> 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..693322e28751 100644
> --- a/fs/fcntl.c
> +++ b/fs/fcntl.c
> @@ -116,6 +116,10 @@ int f_setown(struct file *filp, unsigned long arg, int force)
> int who = arg;
> type = PIDTYPE_PID;
> if (who < 0) {
> + /* avoid overflow below */
> + if (who == INT_MIN)
> + return -EINVAL;
> +
> type = PIDTYPE_PGID;
> who = -who;
> }
Seems reasonable.
I do somewhat lean toward checking for all larger values, but there
could be userland programs that leave the upper bits set when they cast
this to unsigned long. This is probably the safer thing to do.
I'll plan to pick these up for v4.12.
On the other related note...I think we ought to return ESRCH when
find_vpid returns NULL. I'll take a look at that sometime soon too.
Thanks!
--
Jeff Layton <jlayton@redhat.com>
[toc] | [prev] | [next] | [standalone]
| From | zhong jiang <zhongjiang@huawei.com> |
|---|---|
| Date | 2017-06-13 15:10 +0200 |
| Subject | Re: [PATCH v3 2/2] fs/fcntl: f_setown, avoid undefined behaviour |
| Message-ID | <tRTK2-8oY-15@gated-at.bofh.it> |
| In reply to | #1664782 |
On 2017/6/13 20:13, Jeff Layton wrote:
> On Tue, 2017-06-13 at 13:35 +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
>> [v3] implement proper check for the bad value INT_MIN
>>
>> 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..693322e28751 100644
>> --- a/fs/fcntl.c
>> +++ b/fs/fcntl.c
>> @@ -116,6 +116,10 @@ int f_setown(struct file *filp, unsigned long arg, int force)
>> int who = arg;
>> type = PIDTYPE_PID;
>> if (who < 0) {
>> + /* avoid overflow below */
>> + if (who == INT_MIN)
>> + return -EINVAL;
>> +
>> type = PIDTYPE_PGID;
>> who = -who;
>> }
> Seems reasonable.
>
> I do somewhat lean toward checking for all larger values, but there
> could be userland programs that leave the upper bits set when they cast
> this to unsigned long. This is probably the safer thing to do.
>
> I'll plan to pick these up for v4.12.
>
> On the other related note...I think we ought to return ESRCH when
> find_vpid returns NULL. I'll take a look at that sometime soon too.
>
> Thanks!
hi, jeff
I have sent the patch about find_vpid ,and exist in linux-next branch
https://patchwork.kernel.org/patch/9766259/
Thanks
zhongjiang
[toc] | [prev] | [next] | [standalone]
| From | zhong jiang <zhongjiang@huawei.com> |
|---|---|
| Date | 2017-06-13 14:20 +0200 |
| Message-ID | <tRSXE-7Rj-19@gated-at.bofh.it> |
| In reply to | #1664764 |
On 2017/6/13 19:35, 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>
> Reviewed-by: Jeff Layton <jlayton@redhat.com>
> 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:
yes , Look good to me.
Thanks
zhongjiang
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web