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 <[email protected]>
Cc: Jeff Layton <[email protected]>
Cc: "J. Bruce Fields" <[email protected]>
Cc: Alexander Viro <[email protected]>
Cc: [email protected]
---
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
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 <[email protected]>
Cc: Alexander Viro <[email protected]>
Cc: [email protected]
---
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
On 14 October 2016 at 11:23, Jiri Slaby <[email protected]> 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 <[email protected]>
> Cc: Alexander Viro <[email protected]>
> Cc: [email protected]
> ---
> 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
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 <[email protected]>
> Cc: Jeff Layton <[email protected]>
> Cc: "J. Bruce Fields" <[email protected]>
> Cc: Alexander Viro <[email protected]>
> Cc: [email protected]
> ---
> 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 <[email protected]>
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 <[email protected]>
> > Cc: Jeff Layton <[email protected]>
> > Cc: "J. Bruce Fields" <[email protected]>
> > Cc: Alexander Viro <[email protected]>
> > Cc: [email protected]
> > ---
> > 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.
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 <[email protected]>
>>> Cc: Jeff Layton <[email protected]>
>>> Cc: "J. Bruce Fields" <[email protected]>
>>> Cc: Alexander Viro <[email protected]>
>>> Cc: [email protected]
>>> ---
>>> 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
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 <[email protected]>
> > > > Cc: Jeff Layton <[email protected]>
> > > > Cc: "J. Bruce Fields" <[email protected]>
> > > > Cc: Alexander Viro <[email protected]>
> > > > Cc: [email protected]
> > > > ---
> > > > 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 <[email protected]>
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
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 <[email protected]>
> Cc: Jeff Layton <[email protected]>
> Cc: "J. Bruce Fields" <[email protected]>
> Cc: Alexander Viro <[email protected]>
> Cc: [email protected]
> ---
> 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
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 <[email protected]>
>> Cc: Jeff Layton <[email protected]>
>> Cc: "J. Bruce Fields" <[email protected]>
>> Cc: Alexander Viro <[email protected]>
>> Cc: [email protected]
>> ---
>> 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
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 <[email protected]>
>>> Cc: Jeff Layton <[email protected]>
>>> Cc: "J. Bruce Fields" <[email protected]>
>>> Cc: Alexander Viro <[email protected]>
>>> Cc: [email protected]
>>> ---
>>> 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