linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] fs: fcntl, avoid undefined behaviour
@ 2016-10-14  9:23 Jiri Slaby
  2016-10-14  9:23 ` [PATCH] fs: pipe, fix " Jiri Slaby
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Jiri Slaby @ 2016-10-14  9:23 UTC (permalink / raw)
  To: viro
  Cc: linux-kernel, Jiri Slaby, Jeff Layton, J. Bruce Fields, 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) and
return immediatelly in case it is wrong. No error is returned, the
same as in other cases.

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 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;
-- 
2.10.1

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH] fs: pipe, fix undefined behaviour
  2016-10-14  9:23 [PATCH] fs: fcntl, avoid undefined behaviour Jiri Slaby
@ 2016-10-14  9:23 ` Jiri Slaby
  2016-10-14  9:57   ` Vegard Nossum
  2016-10-14 11:48 ` [PATCH] fs: fcntl, avoid " Jeff Layton
  2017-06-12  5:03 ` zhong jiang
  2 siblings, 1 reply; 11+ messages in thread
From: Jiri Slaby @ 2016-10-14  9:23 UTC (permalink / raw)
  To: viro; +Cc: linux-kernel, Jiri Slaby, linux-fsdevel

echo | ./program
where ./program contains fcntl(0, F_SETPIPE_SZ, 0) this is triggered:
UBSAN: Undefined behaviour in ../include/linux/log2.h:63:13
shift exponent 64 is too large for 64-bit type 'long unsigned int'
CPU: 3 PID: 4978 Comm: syz-executor Not tainted 4.8.1-0-syzkaller #1
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.9.1-0-gb3ef39f-prebuilt.qemu-project.org 04/01/2014
...
Call Trace:
...
 [<ffffffffa18ced53>] ? pipe_fcntl+0xa3/0x7a0
 [<ffffffffa18f1940>] ? SyS_fcntl+0x930/0xf30
 [<ffffffffa2d1fb00>] ? entry_SYSCALL_64_fastpath+0x23/0xc1

Documentaion of roundup_pow_of_two explicitly mentions that passing
size 0 is undefined. So return 0 from round_pipe_size prematurely to
fix this.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>
Cc: linux-fsdevel@vger.kernel.org
---
 fs/pipe.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/pipe.c b/fs/pipe.c
index 8e0d9f26dfad..23c0c06ffc33 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -1024,6 +1024,9 @@ static inline unsigned int round_pipe_size(unsigned int size)
 {
 	unsigned long nr_pages;
 
+	if (!size)
+		return 0;
+
 	nr_pages = (size + PAGE_SIZE - 1) >> PAGE_SHIFT;
 	return roundup_pow_of_two(nr_pages) << PAGE_SHIFT;
 }
-- 
2.10.1

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] fs: pipe, fix undefined behaviour
  2016-10-14  9:23 ` [PATCH] fs: pipe, fix " Jiri Slaby
@ 2016-10-14  9:57   ` Vegard Nossum
  0 siblings, 0 replies; 11+ messages in thread
From: Vegard Nossum @ 2016-10-14  9:57 UTC (permalink / raw)
  To: Jiri Slaby; +Cc: Al Viro, LKML, linux-fsdevel

On 14 October 2016 at 11:23, Jiri Slaby <jslaby@suse.cz> wrote:
> echo | ./program
> where ./program contains fcntl(0, F_SETPIPE_SZ, 0) this is triggered:
> UBSAN: Undefined behaviour in ../include/linux/log2.h:63:13
> shift exponent 64 is too large for 64-bit type 'long unsigned int'
> CPU: 3 PID: 4978 Comm: syz-executor Not tainted 4.8.1-0-syzkaller #1
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.9.1-0-gb3ef39f-prebuilt.qemu-project.org 04/01/2014
> ...
> Call Trace:
> ...
>  [<ffffffffa18ced53>] ? pipe_fcntl+0xa3/0x7a0
>  [<ffffffffa18f1940>] ? SyS_fcntl+0x930/0xf30
>  [<ffffffffa2d1fb00>] ? entry_SYSCALL_64_fastpath+0x23/0xc1
>
> Documentaion of roundup_pow_of_two explicitly mentions that passing
> size 0 is undefined. So return 0 from round_pipe_size prematurely to
> fix this.
>
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
> Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> Cc: linux-fsdevel@vger.kernel.org
> ---
>  fs/pipe.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/fs/pipe.c b/fs/pipe.c
> index 8e0d9f26dfad..23c0c06ffc33 100644
> --- a/fs/pipe.c
> +++ b/fs/pipe.c
> @@ -1024,6 +1024,9 @@ static inline unsigned int round_pipe_size(unsigned int size)
>  {
>         unsigned long nr_pages;
>
> +       if (!size)
> +               return 0;
> +
>         nr_pages = (size + PAGE_SIZE - 1) >> PAGE_SHIFT;
>         return roundup_pow_of_two(nr_pages) << PAGE_SHIFT;
>  }

Just FYI I sent a patch for this earlier with a bit more analysis,
although it probably no longer applies cleanly:

https://lkml.org/lkml/2016/8/12/215

Personally I felt it was cleaner to limit the argument passed to
round_pipe_size() instead of relying on the implicit long -> int
truncation. Anyway, feel free to crib from the patch and/or changelog.


Vegard

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] fs: fcntl, avoid undefined behaviour
  2016-10-14  9:23 [PATCH] fs: fcntl, avoid undefined behaviour Jiri Slaby
  2016-10-14  9:23 ` [PATCH] fs: pipe, fix " Jiri Slaby
@ 2016-10-14 11:48 ` Jeff Layton
  2016-10-14 13:38   ` J. Bruce Fields
  2017-06-12  5:03 ` zhong jiang
  2 siblings, 1 reply; 11+ messages in thread
From: Jeff Layton @ 2016-10-14 11:48 UTC (permalink / raw)
  To: Jiri Slaby, viro; +Cc: linux-kernel, J. Bruce Fields, linux-fsdevel

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@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 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...

Thanks,
-- 
Jeff Layton <jlayton@redhat.com>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] fs: fcntl, avoid undefined behaviour
  2016-10-14 11:48 ` [PATCH] fs: fcntl, avoid " Jeff Layton
@ 2016-10-14 13:38   ` J. Bruce Fields
  2016-10-24  9:15     ` Jiri Slaby
  0 siblings, 1 reply; 11+ messages in thread
From: J. Bruce Fields @ 2016-10-14 13:38 UTC (permalink / raw)
  To: Jeff Layton; +Cc: Jiri Slaby, viro, linux-kernel, linux-fsdevel

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@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 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.

--b.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] fs: fcntl, avoid undefined behaviour
  2016-10-14 13:38   ` J. Bruce Fields
@ 2016-10-24  9:15     ` Jiri Slaby
  2016-10-24 11:29       ` Jeff Layton
  0 siblings, 1 reply; 11+ messages in thread
From: Jiri Slaby @ 2016-10-24  9:15 UTC (permalink / raw)
  To: J. Bruce Fields, Jeff Layton; +Cc: viro, linux-kernel, linux-fsdevel

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@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 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,
-- 
js
suse labs

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] fs: fcntl, avoid undefined behaviour
  2016-10-24  9:15     ` Jiri Slaby
@ 2016-10-24 11:29       ` Jeff Layton
  2016-10-24 11:34         ` Jiri Slaby
  0 siblings, 1 reply; 11+ messages in thread
From: Jeff Layton @ 2016-10-24 11:29 UTC (permalink / raw)
  To: Jiri Slaby, J. Bruce Fields; +Cc: viro, linux-kernel, linux-fsdevel

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@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 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@redhat.com>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] fs: fcntl, avoid undefined behaviour
  2016-10-24 11:29       ` Jeff Layton
@ 2016-10-24 11:34         ` Jiri Slaby
  0 siblings, 0 replies; 11+ messages in thread
From: Jiri Slaby @ 2016-10-24 11:34 UTC (permalink / raw)
  To: Jeff Layton, J. Bruce Fields; +Cc: viro, linux-kernel, linux-fsdevel

On 10/24/2016, 01:29 PM, Jeff Layton wrote:
> 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.

Huh, my man 3p fcntl only lists [EDEADLK] at that point. (I have 2013
edition opposing to 2016 from the link above)

> 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.

Ok, will do.

-- 
js
suse labs

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] fs: fcntl, avoid undefined behaviour
  2016-10-14  9:23 [PATCH] fs: fcntl, avoid undefined behaviour Jiri Slaby
  2016-10-14  9:23 ` [PATCH] fs: pipe, fix " Jiri Slaby
  2016-10-14 11:48 ` [PATCH] fs: fcntl, avoid " Jeff Layton
@ 2017-06-12  5:03 ` zhong jiang
  2017-06-13  9:29   ` Jiri Slaby
  2 siblings, 1 reply; 11+ messages in thread
From: zhong jiang @ 2017-06-12  5:03 UTC (permalink / raw)
  To: Jiri Slaby
  Cc: viro, linux-kernel, Jeff Layton, J. Bruce Fields, linux-fsdevel,
	Xishi Qiu, zhongjiang

On 2016/10/14 17:23, 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@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 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;
Hi, Jiri

I hit the same issue,  but I see the upstream is still not changed.  Had any problem?

Thanks
zhongjiang

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] fs: fcntl, avoid undefined behaviour
  2017-06-12  5:03 ` zhong jiang
@ 2017-06-13  9:29   ` Jiri Slaby
  2017-06-13 10:02     ` zhong jiang
  0 siblings, 1 reply; 11+ messages in thread
From: Jiri Slaby @ 2017-06-13  9:29 UTC (permalink / raw)
  To: zhong jiang
  Cc: viro, linux-kernel, Jeff Layton, J. Bruce Fields, linux-fsdevel,
	Xishi Qiu

On 06/12/2017, 07:03 AM, zhong jiang wrote:
> On 2016/10/14 17:23, 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@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 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;
> Hi, Jiri
> 
> I hit the same issue,  but I see the upstream is still not changed.  Had any problem?

Hi, it needed an update which I have just sent. So let's see if that
gets applied.

thanks,
-- 
js
suse labs

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] fs: fcntl, avoid undefined behaviour
  2017-06-13  9:29   ` Jiri Slaby
@ 2017-06-13 10:02     ` zhong jiang
  0 siblings, 0 replies; 11+ messages in thread
From: zhong jiang @ 2017-06-13 10:02 UTC (permalink / raw)
  To: Jiri Slaby
  Cc: viro, linux-kernel, Jeff Layton, J. Bruce Fields, linux-fsdevel,
	Xishi Qiu

On 2017/6/13 17:29, Jiri Slaby wrote:
> On 06/12/2017, 07:03 AM, zhong jiang wrote:
>> On 2016/10/14 17:23, 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@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 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;
>> Hi, Jiri
>>
>> I hit the same issue,  but I see the upstream is still not changed.  Had any problem?
> Hi, it needed an update which I have just sent. So let's see if that
> gets applied.
>
> thanks,
  I have updated in newest kernel version. but it fails to get the change.  Can you look at that?

Thanks
zhongjiang

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2017-06-13 10:03 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-10-14  9:23 [PATCH] fs: fcntl, avoid undefined behaviour Jiri Slaby
2016-10-14  9:23 ` [PATCH] fs: pipe, fix " Jiri Slaby
2016-10-14  9:57   ` Vegard Nossum
2016-10-14 11:48 ` [PATCH] fs: fcntl, avoid " Jeff Layton
2016-10-14 13:38   ` J. Bruce Fields
2016-10-24  9:15     ` Jiri Slaby
2016-10-24 11:29       ` Jeff Layton
2016-10-24 11:34         ` Jiri Slaby
2017-06-12  5:03 ` zhong jiang
2017-06-13  9:29   ` Jiri Slaby
2017-06-13 10:02     ` zhong jiang

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).