Bug 236857

Summary: Fix sysctl check for some sys/audit/process-control tests
Product: Base System Reporter: Olivier Cochard <olivier>
Component: testsAssignee: Olivier Cochard <olivier>
Status: In Progress ---    
Severity: Affects Only Me CC: asomers, emaste, ngie
Priority: ---    
Version: 12.0-RELEASE   
Hardware: Any   
OS: Any   
Attachments:
Description Flags
patch to skip tests if kernel build without CAPABILITIES
none
patch (with errno) to skip tests if kernel build without CAPABILITIES
none
patch to skip tests if kernel build without CAPABILITIES
none
patch using ATF_REQUIRE_SYSCTL_INT
none
cleanup the kernel feature check code none

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