Re: [PATCH net-next v1 7/7] bpf: Always test unprivileged programs

[Mickaël Salaün]

On 06/02/2017 17:09, Alexei Starovoitov wrote:
> On 2/5/17 3:14 PM, Mickaël Salaün wrote:
>> -if (unpriv && test->prog_type)
>> -continue;
>> +if (!test->prog_type) {
>> +if (!unpriv)
>> +set_admin(false);
>> +printf("#%d/u %s ", i, test->descr);
>> +do_test_single(test, true, , );
>> +if (!unpriv)
>> +set_admin(true);
>> +}
>>
>> -printf("#%d %s ", i, test->descr);
>> -do_test_single(test, unpriv, , );
>> +if (!unpriv) {
>> +printf("#%d/p %s ", i, test->descr);
>> +do_test_single(test, false, , );
>> +}
> 
> great idea.
> Acked-by: Alexei Starovoitov 
> 
> as far as other patches.. we need to figure out how to avoid conflicts
> between net-next and Arnaldo's tree where Joe's patches went.

A merge between this series and Arnaldo's tree works fine. The only
dependency is between patches 6 and 7.

> 
> Mickael,
> can you see some way of splitting the patch set between trees?
> Like above test_verfier.c improvement needs to go into net-next.
> The rest can go via perf
> 
> 

OK, I'll send a first series with the patches from 1 to 5 for the perf
tree and a second series with the 6th and 7th patches (touching
tools/testing/selftests/bpf only) to net-next.



signature.asc
Description: OpenPGP digital signature

Re: [PATCH net-next v1 7/7] bpf: Always test unprivileged programs

[Alexei Starovoitov]

On 2/5/17 3:14 PM, Mickaël Salaün wrote:

-   if (unpriv && test->prog_type)
-   continue;
+   if (!test->prog_type) {
+   if (!unpriv)
+   set_admin(false);
+   printf("#%d/u %s ", i, test->descr);
+   do_test_single(test, true, , );
+   if (!unpriv)
+   set_admin(true);
+   }

-   printf("#%d %s ", i, test->descr);
-   do_test_single(test, unpriv, , );
+   if (!unpriv) {
+   printf("#%d/p %s ", i, test->descr);
+   do_test_single(test, false, , );
+   }


great idea.
Acked-by: Alexei Starovoitov 

as far as other patches.. we need to figure out how to avoid conflicts
between net-next and Arnaldo's tree where Joe's patches went.

Mickael,
can you see some way of splitting the patch set between trees?
Like above test_verfier.c improvement needs to go into net-next.
The rest can go via perf

Re: [PATCH net-next v1 7/7] bpf: Always test unprivileged programs

[Daniel Borkmann]

On 02/06/2017 12:14 AM, Mickaël Salaün wrote:

If selftests are run as root, then execute the unprivileged checks as
well. This switch from 240 to 364 tests.

The test numbers are suffixed with "/u" when executed as unprivileged or
with "/p" when executed as privileged.

The geteuid() check is replaced with a capability check.

Handling capabilities require the libcap dependency.

Signed-off-by: Mickaël Salaün 
Cc: Alexei Starovoitov 
Cc: Daniel Borkmann 
Cc: Shuah Khan 


Very useful! Could probably also go as stand-alone to net-next,
but how you prefer.

Acked-by: Daniel Borkmann 

[PATCH net-next v1 7/7] bpf: Always test unprivileged programs

[Mickaël Salaün]

If selftests are run as root, then execute the unprivileged checks as
well. This switch from 240 to 364 tests.

The test numbers are suffixed with "/u" when executed as unprivileged or
with "/p" when executed as privileged.

The geteuid() check is replaced with a capability check.

Handling capabilities require the libcap dependency.

Signed-off-by: Mickaël Salaün 
Cc: Alexei Starovoitov 
Cc: Daniel Borkmann 
Cc: Shuah Khan 
---
 tools/testing/selftests/bpf/Makefile|  2 +-
 tools/testing/selftests/bpf/test_verifier.c | 68 ++---
 2 files changed, 64 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/bpf/Makefile 
b/tools/testing/selftests/bpf/Makefile
index 712861492278..30bb40261692 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -1,4 +1,4 @@
-CFLAGS += -Wall -O2 -I../../../include/uapi -I../../../lib
+CFLAGS += -Wall -O2 -lcap -I../../../include/uapi -I../../../lib
 
 test_objs = test_verifier test_tag test_maps test_lru_map test_lpm_map
 
diff --git a/tools/testing/selftests/bpf/test_verifier.c 
b/tools/testing/selftests/bpf/test_verifier.c
index 04a549e54f61..aa42aef22b85 100644
--- a/tools/testing/selftests/bpf/test_verifier.c
+++ b/tools/testing/selftests/bpf/test_verifier.c
@@ -16,6 +16,7 @@
 #include 
 #include 
 
+#include 
 #include 
 
 #include 
@@ -4498,6 +4499,55 @@ static void do_test_single(struct bpf_test *test, bool 
unpriv,
goto close_fds;
 }
 
+static bool is_admin(void)
+{
+   cap_t caps;
+   cap_flag_value_t sysadmin = CAP_CLEAR;
+   const cap_value_t cap_val = CAP_SYS_ADMIN;
+
+   if (!CAP_IS_SUPPORTED(CAP_SETFCAP)) {
+   perror("cap_get_flag");
+   return false;
+   }
+   caps = cap_get_proc();
+   if (!caps) {
+   perror("cap_get_proc");
+   return false;
+   }
+   if (cap_get_flag(caps, cap_val, CAP_EFFECTIVE, ))
+   perror("cap_get_flag");
+   if (cap_free(caps) == -1)
+   perror("cap_free");
+   return (sysadmin == CAP_SET);
+}
+
+static int set_admin(bool admin)
+{
+   cap_t caps;
+   const cap_value_t cap_val = CAP_SYS_ADMIN;
+   int ret = -1;
+
+   caps = cap_get_proc();
+   if (!caps) {
+   perror("cap_get_proc");
+   return -1;
+   }
+   if (cap_set_flag(caps, CAP_EFFECTIVE, 1, _val,
+   admin ? CAP_SET : CAP_CLEAR)) {
+   perror("cap_set_flag");
+   goto out;
+   }
+   if (cap_set_proc(caps)) {
+   perror("cap_set_proc");
+   goto out;
+   }
+   ret = 0;
+out:
+   if (cap_free(caps) == -1)
+   perror("cap_free");
+   return ret;
+}
+
 static int do_test(bool unpriv, unsigned int from, unsigned int to)
 {
int i, passes = 0, errors = 0;
@@ -4508,11 +4558,19 @@ static int do_test(bool unpriv, unsigned int from, 
unsigned int to)
/* Program types that are not supported by non-root we
 * skip right away.
 */
-   if (unpriv && test->prog_type)
-   continue;
+   if (!test->prog_type) {
+   if (!unpriv)
+   set_admin(false);
+   printf("#%d/u %s ", i, test->descr);
+   do_test_single(test, true, , );
+   if (!unpriv)
+   set_admin(true);
+   }
 
-   printf("#%d %s ", i, test->descr);
-   do_test_single(test, unpriv, , );
+   if (!unpriv) {
+   printf("#%d/p %s ", i, test->descr);
+   do_test_single(test, false, , );
+   }
}
 
printf("Summary: %d PASSED, %d FAILED\n", passes, errors);
@@ -4524,7 +4582,7 @@ int main(int argc, char **argv)
struct rlimit rinf = { RLIM_INFINITY, RLIM_INFINITY };
struct rlimit rlim = { 1 << 20, 1 << 20 };
unsigned int from = 0, to = ARRAY_SIZE(tests);
-   bool unpriv = geteuid() != 0;
+   bool unpriv = !is_admin();
 
if (argc == 3) {
unsigned int l = atoi(argv[argc - 2]);
-- 
2.11.0