Bug 236857 - Fix sysctl check for some sys/audit/process-control tests
Summary: Fix sysctl check for some sys/audit/process-control tests
Status: In Progress
Alias: None
Product: Base System
Classification: Unclassified
Component: tests (show other bugs)
Version: 12.0-RELEASE
Hardware: Any Any
: --- Affects Only Me
Assignee: Olivier Cochard
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-03-28 18:25 UTC by Olivier Cochard
Modified: 2019-08-12 19:33 UTC (History)
3 users (show)

See Also:


Attachments
patch to skip tests if kernel build without CAPABILITIES (1.12 KB, patch)
2019-03-28 18:25 UTC, Olivier Cochard
no flags Details | Diff
patch (with errno) to skip tests if kernel build without CAPABILITIES (1.30 KB, patch)
2019-03-28 20:24 UTC, Olivier Cochard
no flags Details | Diff
patch to skip tests if kernel build without CAPABILITIES (1.32 KB, patch)
2019-03-28 20:46 UTC, Olivier Cochard
no flags Details | Diff
patch using ATF_REQUIRE_SYSCTL_INT (1.70 KB, patch)
2019-03-28 23:07 UTC, Olivier Cochard
no flags Details | Diff
cleanup the kernel feature check code (1.73 KB, patch)
2019-03-29 13:31 UTC, Olivier Cochard
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Olivier Cochard freebsd_committer 2019-03-28 18:25:34 UTC
Created attachment 203217 [details]
patch to skip tests if kernel build without CAPABILITIES

There are two tests that check if kern.features.security_capability_mode is correctly set (=1) , but if a kernel without CAPABILITIES these tests failed because non existing sysctl.

Before patch:

NANOBSD# kyua test sys/audit/process-control:cap_enter_success
sys/audit/process-control:cap_enter_success  ->  failed: /var/jenkins/workspace/ocafirmware_build-master/FreeBSD/tests/sys/audit/process-con
trol.c:1518: 0 != sysctlbyname(capname, &capinfo, &len, NULL, 0)  [0.003s]

Results file id is usr_tests.20190328-140620-191316
Results saved to /root/.kyua/store/results.usr_tests.20190328-140620-191316.db

0/1 passed (1 failed)

==============
After patch:

NANOBSD# kyua test sys/audit/process-control:cap_enter_success
sys/audit/process-control:cap_enter_success  ->  skipped: sysctl kern.features.security_capability_mode doesn't exist  [0.003s]

Results file id is usr_tests.20190328-181625-015778
Results saved to /root/.kyua/store/results.usr_tests.20190328-181625-015778.db

1/1 passed (0 failed)
NANOBSDx# kyua test sys/audit/process-control:cap_getmode_success
sys/audit/process-control:cap_getmode_success  ->  skipped: sysctl kern.features.security_capability_mode doesn't exist  [0.003s]

Results file id is usr_tests.20190328-181746-138037
Results saved to /root/.kyua/store/results.usr_tests.20190328-181746-138037.db

1/1 passed (0 failed)
Comment 1 Enji Cooper freebsd_committer 2019-03-28 19:10:22 UTC
This needs to test for errno in {ENOTDIR,ENOENT}.
Comment 2 Olivier Cochard freebsd_committer 2019-03-28 20:24:11 UTC
Created attachment 203222 [details]
patch (with errno) to skip tests if kernel build without CAPABILITIES

Ok, here is a patch with errno support.

But I don't understand what is the purpose of errno here, because it didn't give very useful information ?

Output with this -errno version patch applied:

# kyua test sys/audit/process-control:cap_enter_success
sys/audit/process-control:cap_enter_success  ->  skipped: sysctl for kern.features.security_capability_mode failed: No such file or directory  [0.003s]

Results file id is usr_tests.20190328-201847-910402
Results saved to /root/.kyua/store/results.usr_tests.20190328-201847-910402.db

1/1 passed (0 failed)

=> The 'No such file or directory' seems misleading for me, because the message is about a sysctl.
Comment 3 Enji Cooper freebsd_committer 2019-03-28 20:30:48 UTC
(In reply to Olivier Cochard from comment #2)

Let me rephrase for clarity; the test should be something of the form:

if (sysctlbyname(...) == -1) {
    if (errno == ENOTDIR || errno == ENOENT) {
        atf_tc_skip("not testing; : %s", capname, strerror(errno));
    } else {
        /* something happened with the sysctlbyname call; report the error */
    }
}

Also, I realize that there's already a macro for this, called `ATF_REQUIRE_FEATURE(..)`.

If you change the call to `ATF_REQUIRE_FEATURE("security_capability_mode");`, it will "do the right thing".

You'll need to add...

`#include <freebsd_test_suite/macros.h>`

... to the source file and also add `CFLAGS+= -I${SRCTOP}/tests` to the Makefile.
Comment 4 Olivier Cochard freebsd_committer 2019-03-28 20:46:50 UTC
Created attachment 203223 [details]
patch to skip tests if kernel build without CAPABILITIES

Thanks for the tips!
The code a lot's cleaner now and the error message too:

# kyua test sys/audit/process-control:cap_enter_success
sys/audit/process-control:cap_enter_success  ->  skipped: kernel feature (security_capability_mode) not present  [0.003s]

Results file id is usr_tests.20190328-204336-320096
Results saved to /root/.kyua/store/results.usr_tests.20190328-204336-320096.db

1/1 passed (0 failed)
Comment 5 Alan Somers freebsd_committer 2019-03-28 21:54:15 UTC
LGTM!
Comment 6 Alan Somers freebsd_committer 2019-03-28 22:05:33 UTC
Actually, I take that back.  You should remove the old sysctlbyname(3) call and only use ATF_REQUIRE_FEATURE.
Comment 7 Olivier Cochard freebsd_committer 2019-03-28 23:07:49 UTC
Created attachment 203226 [details]
patch using ATF_REQUIRE_SYSCTL_INT

Good remark.

The original code was:

```
const char *capname = "kern.features.security_capability_mode";
ATF_REQUIRE_EQ(0, sysctlbyname(capname, &capinfo, &len, NULL, 0))
/* Without CAPABILITY_MODE enabled, cap_enter() returns ENOSYS */
if (!capinfo)
   atf_tc_skip("Capsicum is not enabled in the system");
```

So, the purpose was to check if kern.features.security_capability_mode==1.
Which mean "Check if capability is enabled".

My patch was adding a "Check if this capability exist", before to check its status.

If I remove the old sysctlbyname(3) call, I will not catch this condition:
- Capability exist
- But capability is disabled

So I've read freebsd_test_suite/macros.h, and I've found these functions:
- ATF_REQUIRE_FEATURE(_feature_name)
- ATF_REQUIRE_KERNEL_MODULE(_mod_name)
- ATF_REQUIRE_SYSCTL_INT(_mib_name, _required_value)

And I think the best way should using ATF_REQUIRE_SYSCTL_INT(capname,1) that manage both condition.

With this new patch:
# kyua test sys/audit/process-control:cap_enter_success
sys/audit/process-control:cap_enter_success  ->  skipped: sysctl for kern.features.security_capability_mode failed: No such file or director
y  [0.003s]

Results file id is usr_tests.20190328-230635-632822
Results saved to /root/.kyua/store/results.usr_tests.20190328-230635-632822.db

1/1 passed (0 failed)
Comment 8 Alan Somers freebsd_committer 2019-03-29 01:02:22 UTC
Actually, ATF_REQUIRE_FEATURE is correct, it just isn't obvious.  You have to look at the source for feature_present(3) in lib/libc/gen/feature_present.c to see that it does just what you want.  It returns true if the sysctl exists and its value is unequal to 0.
Comment 9 Enji Cooper freebsd_committer 2019-03-29 01:03:47 UTC
(In reply to Alan Somers from comment #8)

Bingo :)...

$ man feature_present
FEATURE_PRESENT(3)     FreeBSD Library Functions Manual     FEATURE_PRESENT(3)

...

DESCRIPTION
     The feature_present() function provides a method for an application to
     determine if a specific kernel feature is present in the currently
     running kernel.  The feature argument specifies the name of the feature
     to check.  The feature_present() function will return 1 if the specified
     feature is present, otherwise it will return 0.  If the feature_present()
     function is not able to determine the presence of feature due to an
     internal error it will return 0.

RETURN VALUES
     If feature is present then 1 is returned; otherwise 0 is returned.
Comment 10 Olivier Cochard freebsd_committer 2019-03-29 13:31:45 UTC
Created attachment 203241 [details]
cleanup the kernel feature check code

New version according to your explanations (thanks you for your patience).
Comment 11 Enji Cooper freebsd_committer 2019-03-29 14:56:21 UTC
LGTM!
Comment 12 Ed Maste freebsd_committer 2019-04-01 13:46:38 UTC
If you need a src committer approval, 
Approved by: emaste
Comment 13 commit-hook freebsd_committer 2019-04-01 14:21:54 UTC
A commit references this bug:

Author: olivier
Date: Mon Apr  1 14:21:32 UTC 2019
New revision: 345765
URL: https://svnweb.freebsd.org/changeset/base/345765

Log:
  Fix and simplify code by using ATF_REQUIRE_FEATURE macro

  PR:		236857
  Reviewed by:	asomers, ngie
  Approved by:	emaste
  MFC after:	 1 month
  Sponsored by:	Netflix

Changes:
  head/tests/sys/audit/Makefile
  head/tests/sys/audit/process-control.c
Comment 14 Ed Maste freebsd_committer 2019-08-12 19:33:43 UTC
Waiting for MFC