Re: svn commit: r345853 - head/usr.bin/rctl
On 4/4/19, 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. > > 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. > There is most likely code which always passed oversized bufs. This change would break it. -- Mateusz Guzik ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r345853 - head/usr.bin/rctl
> 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. 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. Thanks! -Enji ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r345853 - head/usr.bin/rctl
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.c11 Jun 2004 02:20:37 - 1.157
XX +++ kern_sysctl.c11 Jun 2004 07:58:23 -
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
svn commit: r345853 - head/usr.bin/rctl
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.
PR: 236714
Reported by: trasz
Diagnosed by: markj
Sponsored by: The FreeBSD Foundation
Modified:
head/usr.bin/rctl/rctl.c
Modified: head/usr.bin/rctl/rctl.c
==
--- head/usr.bin/rctl/rctl.cWed Apr 3 19:59:45 2019(r345852)
+++ head/usr.bin/rctl/rctl.cWed Apr 3 20:37:14 2019(r345853)
@@ -378,8 +378,9 @@ print_rules(char *rules, int hflag, int nflag)
static void
enosys(void)
{
- int error, racct_enable;
size_t racct_enable_len;
+ int error;
+ bool racct_enable;
racct_enable_len = sizeof(racct_enable);
error = sysctlbyname("kern.racct.enable",
@@ -392,7 +393,7 @@ enosys(void)
err(1, "sysctlbyname");
}
- if (racct_enable == 0)
+ if (!racct_enable)
errx(1, "RACCT/RCTL present, but disabled; enable using
kern.racct.enable=1 tunable");
}
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r345853 - head/usr.bin/rctl
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.c11 Jun 2004 02:20:37 - 1.157
XX +++ kern_sysctl.c11 Jun 2004 07:58:23 -
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
Re: svn commit: r345853 - head/usr.bin/rctl
On 4/4/19, 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. > > 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. > There is most likely code which always passed oversized bufs. This change would break it. -- Mateusz Guzik ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r345853 - head/usr.bin/rctl
> 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. 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. Thanks! -Enji ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
svn commit: r345853 - head/usr.bin/rctl
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.
PR: 236714
Reported by: trasz
Diagnosed by: markj
Sponsored by: The FreeBSD Foundation
Modified:
head/usr.bin/rctl/rctl.c
Modified: head/usr.bin/rctl/rctl.c
==
--- head/usr.bin/rctl/rctl.cWed Apr 3 19:59:45 2019(r345852)
+++ head/usr.bin/rctl/rctl.cWed Apr 3 20:37:14 2019(r345853)
@@ -378,8 +378,9 @@ print_rules(char *rules, int hflag, int nflag)
static void
enosys(void)
{
- int error, racct_enable;
size_t racct_enable_len;
+ int error;
+ bool racct_enable;
racct_enable_len = sizeof(racct_enable);
error = sysctlbyname("kern.racct.enable",
@@ -392,7 +393,7 @@ enosys(void)
err(1, "sysctlbyname");
}
- if (racct_enable == 0)
+ if (!racct_enable)
errx(1, "RACCT/RCTL present, but disabled; enable using
kern.racct.enable=1 tunable");
}
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
