Bug 236714 - Clang problem with rctl(8)
Summary: Clang problem with rctl(8)
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: bin (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Only Me
Assignee: Mateusz Guzik
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-03-22 12:16 UTC by Edward Tomasz Napierala
Modified: 2019-04-03 20:38 UTC (History)
5 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Edward Tomasz Napierala freebsd_committer freebsd_triage 2019-03-22 12:16:09 UTC
I've stumbled upon something that seems to me to be a miscompilation issue in rctl(8).  Basically, this code in usr.bin/rctl/rctl.c:

        racct_enable_len = sizeof(racct_enable);
        error = sysctlbyname("kern.racct.enable",
            &racct_enable, &racct_enable_len, NULL, 0);

        if (error != 0) {
                if (errno == ENOENT)
                        errx(1, "RACCT/RCTL support not present in kernel; see rctl(8) for details");

                err(1, "sysctlbyname");
        }

        if (racct_enable == 0)
                errx(1, "RACCT/RCTL present, but disabled; enable using kern.racct.enable=1 tunable");

Doesn't work as expected unless the racct_enable is initialized before calling sysctlbyname(3).  Without this change:

% doas rctl
rctl: failed to show rules for '::': Function not implemented

With the change:

% doas rctl
rctl: RACCT/RCTL present, but disabled; enable using kern.racct.enable=1 tunable
Comment 1 Mark Johnston freebsd_committer freebsd_triage 2019-03-22 18:34:25 UTC
It's a regression from r341182.  The kernel is exporting racct_enable as a bool.
Comment 2 Edward Tomasz Napierala freebsd_committer freebsd_triage 2019-03-22 20:02:22 UTC
Thanks!  I wonder if we could perhaps return an error from sysctl(3) in this case?
Comment 3 Mark Johnston freebsd_committer freebsd_triage 2019-03-22 20:06:21 UTC
(In reply to Edward Tomasz Napierala from comment #2)
It would probably be better to detect the case where the old buffer is larger than 1 byte, and zero the full buffer before writing out the value.
Comment 4 Dimitry Andric freebsd_committer freebsd_triage 2019-03-22 21:21:58 UTC
(In reply to Mark Johnston from comment #1)
> It's a regression from r341182.  The kernel is exporting racct_enable as a
> bool.

Yes indeed, I did a bit of debugging, and it turns out that if you pass a pointer to an int (and the length of an int, obviousl), the sysctl does not modify the variable at all...
Comment 5 Mateusz Guzik freebsd_committer freebsd_triage 2019-04-01 20:13:07 UTC
(In reply to Mark Johnston from comment #3)

I don't think that's feasible since consumers can pass arbitrary sizes, and some probably pass more than they intended in a bogus manner where they would not be prepared to sudden zeroing of the area passed.

Looks like the bug at hand can be easily taken care of with just resizing the var to bool in the userspace tool. While the previous binary remains broken I don't think it's a big deal since this is current-only.

that is:
diff --git a/usr.bin/rctl/rctl.c b/usr.bin/rctl/rctl.c
index 74073c13207e..f8b5115a4507 100644
--- a/usr.bin/rctl/rctl.c
+++ b/usr.bin/rctl/rctl.c
@@ -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",
Comment 6 Mark Johnston freebsd_committer freebsd_triage 2019-04-01 20:26:52 UTC
(In reply to Mateusz Guzik from comment #5)
Well, it's fine in the (probably more common) case where the kernel changed a type from int to bool and userspace applications are not immediately updated to match.  But I agree that it could cause some unpleasant surprises, and your diff looks fine to me.
Comment 7 Konstantin Belousov freebsd_committer freebsd_triage 2019-04-02 08:06:35 UTC
What is the problem checking for exactly old or new buffer size ?  Look e.g. at the sysctl_bufspace() in vfs_bio.c.
Comment 8 Mateusz Guzik freebsd_committer freebsd_triage 2019-04-02 18:12:08 UTC
I thought markj wanted to handle a general case.

I did not check if anything depends on the affected sysctl in userspace, so that's my bad. However, since the change is only in current and the tool can be modified I don't think there is much use of adding aforementioned support there.
Comment 9 Konstantin Belousov freebsd_committer freebsd_triage 2019-04-02 18:58:28 UTC
(In reply to Mateusz Guzik from comment #8)
Is rctl(8) useful in any way for non-root user, or for root user in a jail ?
If answer to any of this question is yes, we should provide ABI compat.  Otherwise, indeed, it does not matter.
Comment 10 Mateusz Guzik freebsd_committer freebsd_triage 2019-04-02 19:25:12 UTC
It's all gated with priv_check calls defaulting to not allowing jailed root to do anything. So someone would have to have a dedicated mac module granting this.

So if trasz is fine with the patch, I'll commit it.
Comment 11 Edward Tomasz Napierala freebsd_committer freebsd_triage 2019-04-03 10:56:04 UTC
Patch looks good, thank you!

What about the underlying issue, which is sysctl(3) silently failing when it receives an improperly sized buffer?  Would it be feasible to make it return an error, so it's easier to fix next time it happens?
Comment 12 Mateusz Guzik freebsd_committer freebsd_triage 2019-04-03 20:31:21 UTC
It is not being ignored. There is non-zero garbage in an area not written to by sysctl, which makes the zero comparison fail.

diff --git a/usr.bin/rctl/rctl.c b/usr.bin/rctl/rctl.c
index 74073c13207e..8fb9e862aee8 100644
--- a/usr.bin/rctl/rctl.c
+++ b/usr.bin/rctl/rctl.c
@@ -378,12 +378,15 @@ print_rules(char *rules, int hflag, int nflag)
 static void
 enosys(void)
 {
-       int error, racct_enable;
+       int error, racct_enable = ~(0);
        size_t racct_enable_len;
 
+
+       printf("%x\n", racct_enable);
        racct_enable_len = sizeof(racct_enable);
        error = sysctlbyname("kern.racct.enable",
            &racct_enable, &racct_enable_len, NULL, 0);
+       printf("%x\n", racct_enable);
 
        if (error != 0) {
                if (errno == ENOENT)

Results in:
ffffffff
ffffff00
a.out: failed to show rules for '::': Function not implemented
Comment 13 commit-hook freebsd_committer freebsd_triage 2019-04-03 20:38:01 UTC
A commit references this bug:

Author: mjg
Date: Wed Apr  3 20:37:14 UTC 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

Changes:
  head/usr.bin/rctl/rctl.c