Date: Thu, 4 Apr 2019 19:06:30 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Enji Cooper <yaneurabeya@gmail.com> Cc: Mateusz Guzik <mjg@freebsd.org>, src-committers <src-committers@freebsd.org>, svn-src-all <svn-src-all@freebsd.org>, svn-src-head@freebsd.org Subject: Re: svn commit: r345853 - head/usr.bin/rctl Message-ID: <20190404181545.N1160@besplex.bde.org> In-Reply-To: <EE367002-EC49-41F3-94A8-79F475CC63B8@gmail.com> References: <201904032037.x33KbEjq070604@repo.freebsd.org> <EE367002-EC49-41F3-94A8-79F475CC63B8@gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, 3 Apr 2019, Enji Cooper wrote: >> On Apr 3, 2019, at 1:37 PM, Mateusz Guzik <mjg@FreeBSD.org> wrote: >> >> Author: mjg >> Date: Wed Apr 3 20:37:14 2019 >> New Revision: 345853 >> URL: https://svnweb.freebsd.org/changeset/base/345853 >> >> Log: >> rctl: fix sysctl kern.racct.enable use after r341182 >> >> The value was changed from int to bool. Since the new type >> is smaller, the rest of the variable in the caller was left >> unitialized. Why not fix the bug? It it was breaking the ABI by changing int to bool. Churning int to bool is bad enough even when it doesn't break ABIs. > I hit a bug like this recently with capsicum-test. Do you think it makes sense to purge all of the memory or return -1/set EINVAL for reasons similar to this for newp? > > [EINVAL] A non-null newp is given and its specified length in > newlen is too large or too small. Purge? That would break correct programs to not detect ABI mismatches and kernel bugs. One direction of this was broken almost 25 years ago in phk's rewrite of sysctl. I use the following fix (edited from my 16 year old 1483 line patch for mostly style bugs in kern_sysctl.c, There are many more now). XX Index: kern_sysctl.c XX =================================================================== XX RCS file: /home/ncvs/src/sys/kern/kern_sysctl.c,v XX retrieving revision 1.157 XX diff -u -2 -r1.157 kern_sysctl.c XX --- kern_sysctl.c 11 Jun 2004 02:20:37 -0000 1.157 XX +++ kern_sysctl.c 11 Jun 2004 07:58:23 -0000 XX @@ -957,6 +949,13 @@ XX if (!req->newptr) XX return 0; XX - if (req->newlen - req->newidx < l) XX + if (req->newlen - req->newidx != l) { XX + if (req->newlen - req->newidx > l) { XX + printf( XX + "sysctl_new_kernel -- short write: %zu - %zu > %zu\n", XX + req->newlen, req->newidx, l); XX + Debugger("sysctl_new_kernel: short write"); XX + } XX return (EINVAL); XX + } XX bcopy((char *)req->newptr + req->newidx, p, l); XX req->newidx += l; XX @@ -1075,6 +1073,13 @@ XX if (!req->newptr) XX return 0; XX - if (req->newlen - req->newidx < l) XX + if (req->newlen - req->newidx != l) { XX + if (req->newlen - req->newidx > l) { XX + printf( XX + "sysctl_new_user -- short write: %zu - %zu > %zu\n", XX + req->newlen, req->newidx, l); XX + Debugger("sysctl_new_user: short write"); XX + } XX return (EINVAL); XX + } XX error = copyin((char *)req->newptr + req->newidx, p, l); XX req->newidx += l; IIRC, the error is still detected for short reads. This seems to be the case here. More recently, I tried to fix the sysctl ABI so that applications don't need to hard-code or probe for the size of integer variables to match the size used by the current kernel. Any size large enough to represent the value should work. E.g., for this bool, applications that use the unchurned ABI write an int, and everything works provided the int can be represented as a bool. In the opposite direction, larger kernel variables can easily represent smaller application variables, so there is no problem provided the kernel does the correct conversions. Currently it doesn't. For short writes, it writes to the lower bits and leaves garbage in the top bits. Or maybe the reverse for big-endian. Then it succeeds and doesn't return the documented error -1/EINVAL. My fix would break some probes. It seems to be neccessary to probe using reads, since writes would be destructive. The application might try to read into an int8_t variable. This would succeed if the kernel variable is intmax_t, provided the current value is between 0 and 127. But writing 128 or larger later won't work. The applications can start the probe with the maximum type intmax_t, but that will always work (except for unsigned values), so the application might as well hard-code use of intmax_t and waste a lot of space. Most applications are currently broken by not probing at all. This often breaks ones like systat and top which report kernel variables whose size often increases. Errror handling in systat and top is still very broken: - getsysctl() in systat detects all size mismatches but mishandles them by printing an error message and returning garbage. It mishandles even errors detected by the kernel similarly, except the garbage is less processed if sysctlbyname() fails. It also has many style bugs (function type on the same line as the function name; initialization in declaration; no blank line after declaration; excessive braces; casts to unsigned long to print size_t's since %zu has only been standard for 20 years; not using the BSD spelling u_long in these casts). - top uses the same getsysctl() wrapper as systat, but quits after an error and is missing the style bugs except for the the initialization in the declaration and the casts to unsigned long. Quitting is more technically correct, but is worse in practice. A size mismatch for a single variable breaks the display of about 25 other systl variables and many non-sysctl variables in top. Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20190404181545.N1160>