| Message ID | 20161014092342.25546-1-jslaby@suse.cz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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,
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. -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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,
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> -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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
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,
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
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;
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(+)