Bug 169056
| Summary: | [patch] sysctl*(3) cannot fail with EFAULT if name contains an invalid address | ||||||
|---|---|---|---|---|---|---|---|
| Product: | Documentation | Reporter: | Enji Cooper <ngie> | ||||
| Component: | Books & Articles | Assignee: | Tom Rhodes <trhodes> | ||||
| Status: | Closed FIXED | ||||||
| Severity: | Affects Only Me | ||||||
| Priority: | Normal | ||||||
| Version: | Latest | ||||||
| Hardware: | Any | ||||||
| OS: | Any | ||||||
| Attachments: |
|
||||||
Responsible Changed From-To: freebsd-doc->trhodes Take - I spoke to Mr. Cooper about this in email. FYI, I think I said this in e-mail already, but I think the proper fix for this is to always call __sysctl() and only check for CTL_USER if the sysctl fails with ENOENT. -- John Baldwin Author: trhodes Date: Thu Sep 6 20:15:44 2012 New Revision: 240176 URL: http://svn.freebsd.org/changeset/base/240176 Log: Avoid segfault if name is invalid. Basically, only check for CTL_USER if the sysctl fails with ENOENT. PR: 169056 Reviewed by: jhb Modified: head/lib/libc/gen/sysctl.c Modified: head/lib/libc/gen/sysctl.c ============================================================================== --- head/lib/libc/gen/sysctl.c Thu Sep 6 19:26:59 2012 (r240175) +++ head/lib/libc/gen/sysctl.c Thu Sep 6 20:15:44 2012 (r240176) @@ -50,8 +50,11 @@ int sysctl(const int *name, u_int namelen, void *oldp, size_t *oldlenp, const void *newp, size_t newlen) { - if (name[0] != CTL_USER) - return (__sysctl(name, namelen, oldp, oldlenp, newp, newlen)); + int retval; + + retval = __sysctl(name, namelen, oldp, oldlenp, newp, newlen); + if (retval != -1 || errno != ENOENT || name[0] != CTL_USER) + return (retval); if (newp != NULL) { errno = EPERM; _______________________________________________ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org" > [sysctl() crashes if passes an invalid 'name' pointer]
I don't really agree with r240176. If an application passes an invalid
pointer to a function, the behaviour is undefined per POSIX and both
[EFAULT] and a segfault are valid.
A segfault seems the best response here, given that it has always been
that way. It also makes things slightly faster for well-behaved
applications because a system call is not done for CTL_USER names.
Furthermore, this situation occurs more frequently and it may not always
be possible to avoid a segfault efficiently. For example, while the
sigaction() system call will fail with [EFAULT] if passed an invalid
pointer, libthr's wrapper will read and write from userland and
therefore segfault if passed an invalid pointer.
I don't really like [EFAULT] in general because it is easily ignored and
because the system call often commits anyway (leaving the application
without important information that would have been written to the bad
pointer).
--
Jilles Tjoelker
On Fri, 7 Sep 2012 11:40:11 GMT Jilles Tjoelker <jilles@stack.nl> wrote: > The following reply was made to PR docs/169056; it has been noted by GNATS. > > From: Jilles Tjoelker <jilles@stack.nl> > To: bug-followup@FreeBSD.org, yanegomi@gmail.com, > John Baldwin <jhb@freebsd.org> > Cc: > Subject: Re: docs/169056: [patch] sysctl*(3) cannot fail with EFAULT if name > contains an invalid address > Date: Fri, 7 Sep 2012 13:35:22 +0200 > > > [sysctl() crashes if passes an invalid 'name' pointer] > > I don't really agree with r240176. If an application passes an invalid > pointer to a function, the behaviour is undefined per POSIX and both > [EFAULT] and a segfault are valid. Then why let it crash at all for the user? Why not just return EFAULT and have done with it? > > A segfault seems the best response here, given that it has always been > that way. It also makes things slightly faster for well-behaved > applications because a system call is not done for CTL_USER names. Slightly faster? I don't think an additional if statement is that damaging to performance. > > Furthermore, this situation occurs more frequently and it may not always > be possible to avoid a segfault efficiently. For example, while the > sigaction() system call will fail with [EFAULT] if passed an invalid > pointer, libthr's wrapper will read and write from userland and > therefore segfault if passed an invalid pointer. I would suggest an additional check there too; however, I'm starting to be concerned that we would spend too much time "protecting the developer from themselves" here. > > I don't really like [EFAULT] in general because it is easily ignored and > because the system call often commits anyway (leaving the application > without important information that would have been written to the bad > pointer). Noted. If you strongly about this, I'll back it out. But I would recommend Garrett's manual page patch in the backout case. Cheers, -- Tom Rhodes Closing old bug. jilles: please reopen/reassign it if you believe trhodes' commit needs to be changed. |
Based on code inspection in lib/libc/gen/sysctl.c, the requirement that states that EFAULT will be raised if name was an invalid memory address is bogus. Furthermore, it generates a segfault (or other random memory corruption issues) if the value passed in to name is invalid, due to the following line in sysctl.c: 49 int 50 sysctl(const int *name, u_int namelen, void *oldp, size_t *oldlenp, 51 const void *newp, size_t newlen) 52 { 53 if (name[0] != CTL_USER) 54 return (__sysctl(name, namelen, oldp, oldlenp, newp, newlen) ); Example: $ cat ~/bad_sysctl.c #include <sys/param.h> #include <sys/sysctl.h> int main(void) { return sysctl((int*)-1, 2, NULL, NULL, NULL, 0) == 0 ? 0 : 1; } $ gcc -Wall -o ~/bad_sysctl ~/bad_sysctl.c $ ~/bad_sysctl Segmentation fault: 11 (core dumped) $ The attached patch removes bogus requirement in sysctl(3). Whether or not the segfault in libc should be `fixed` is another topic entirely. Fix: Patch attached with submission follows: