From owner-svn-src-all@freebsd.org Tue Sep 3 14:06:34 2019 Return-Path: Delivered-To: svn-src-all@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id 0276ADC489; Tue, 3 Sep 2019 14:06:10 +0000 (UTC) (envelope-from yuripv@freebsd.org) Received: from freefall.freebsd.org (freefall.freebsd.org [96.47.72.132]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) server-signature RSA-PSS (4096 bits) client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "freefall.freebsd.org", Issuer "Let's Encrypt Authority X3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 46N7z54Bnlz4PG5; Tue, 3 Sep 2019 14:06:09 +0000 (UTC) (envelope-from yuripv@freebsd.org) Received: by freefall.freebsd.org (Postfix, from userid 1452) id B66451A242; Tue, 3 Sep 2019 14:06:00 +0000 (UTC) X-Original-To: yuripv@localmail.freebsd.org Delivered-To: yuripv@localmail.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) (Client CN "mx1.freebsd.org", Issuer "Let's Encrypt Authority X3" (verified OK)) by freefall.freebsd.org (Postfix) with ESMTPS id 1ECFDD94E; Thu, 4 Apr 2019 08:06:47 +0000 (UTC) (envelope-from owner-src-committers@freebsd.org) Received: from freefall.freebsd.org (freefall.freebsd.org [96.47.72.132]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) server-signature RSA-PSS (4096 bits) client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "freefall.freebsd.org", Issuer "Let's Encrypt Authority X3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 725CF8B59E; Thu, 4 Apr 2019 08:06:46 +0000 (UTC) (envelope-from owner-src-committers@freebsd.org) Received: by freefall.freebsd.org (Postfix, from userid 538) id 1E4E9D91D; Thu, 4 Apr 2019 08:06:46 +0000 (UTC) Delivered-To: src-committers@localmail.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [96.47.72.80]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) (Client CN "mx1.freebsd.org", Issuer "Let's Encrypt Authority X3" (verified OK)) by freefall.freebsd.org (Postfix) with ESMTPS id C9A0FD918; Thu, 4 Apr 2019 08:06:43 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail106.syd.optusnet.com.au (mail106.syd.optusnet.com.au [211.29.132.42]) by mx1.freebsd.org (Postfix) with ESMTP id 099978B596; Thu, 4 Apr 2019 08:06:43 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from [192.168.0.102] (c110-21-101-228.carlnfd1.nsw.optusnet.com.au [110.21.101.228]) by mail106.syd.optusnet.com.au (Postfix) with ESMTPS id 7D5E33DCAE5; Thu, 4 Apr 2019 19:06:31 +1100 (AEDT) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Enji Cooper cc: Mateusz Guzik , src-committers , svn-src-all , svn-src-head@freebsd.org Subject: Re: svn commit: r345853 - head/usr.bin/rctl In-Reply-To: Message-ID: <20190404181545.N1160@besplex.bde.org> References: <201904032037.x33KbEjq070604@repo.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.2 cv=P6RKvmIu c=1 sm=1 tr=0 cx=a_idp_d a=PalzARQSbocsUSjMRkwAPg==:117 a=PalzARQSbocsUSjMRkwAPg==:17 a=jpOVt7BSZ2e4Z31A5e1TngXxSK0=:19 a=kj9zAlcOel0A:10 a=6I5d2MoRAAAA:8 a=KUmEUG2a7xQWqrzlUCUA:9 a=KRTGSK9FuyMgZ-wV:21 a=Zd5fP2Ss_TbmmEtX:21 a=CjuIK1q_8ugA:10 a=IjZwj45LgO3ly-622nXo:22 Precedence: bulk X-Loop: FreeBSD.org Sender: owner-src-committers@freebsd.org X-Rspamd-Queue-Id: 725CF8B59E X-Spamd-Bar: ------ Authentication-Results: mx1.freebsd.org X-Spamd-Result: default: False [-6.99 / 15.00]; NEURAL_HAM_MEDIUM(-1.00)[-1.000,0]; NEURAL_HAM_SHORT(-0.99)[-0.992,0]; REPLY(-4.00)[]; NEURAL_HAM_LONG(-1.00)[-1.000,0] Status: O X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.29 List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Date: Tue, 03 Sep 2019 14:06:34 -0000 X-Original-Date: Thu, 4 Apr 2019 19:06:30 +1100 (EST) X-List-Received-Date: Tue, 03 Sep 2019 14:06:34 -0000 On Wed, 3 Apr 2019, Enji Cooper wrote: >> On Apr 3, 2019, at 1:37 PM, Mateusz Guzik 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