[PATCH v4 4/4] tests: test cases for seccomp-assisted syscall filtering

Dmitry V. Levin ldv at altlinux.org
Sat Aug 31 16:26:39 UTC 2019


On Thu, Aug 29, 2019 at 04:01:13PM +0200, Paul Chaignon wrote:
> From: Chen Jingpiao <chenjingpiao at gmail.com>
> 
> Test filter_seccomp-perf checks seccomp-filter is actually enabled by
> comparing the number of syscalls performed in a time interval when
> seccomp-filter is enabled vs. disabled.  The number of syscalls should be
> one order of magnitude higher when seccomp-filter is enabled.
> 
> Test filter_seccomp-flag ensures the audit_arch_vec[].flag constants do
> not conflict with syscall numbers.  If this test fails, then the number of
> syscalls grew high enough that the code for seccomp-filter needs to be
> updated.
> 
> * tests/status-none-f.c: New file.
> * tests/pure_executables.list: Add status-none-f.
> * tests/.gitignore: Add status-none-f, filter_seccomp-perf, and
> filter_seccomp-flag.
> * tests/init.sh (test_prog_set): New function.
> * tests/filter_seccomp.in: New file.
> * tests/filter_seccomp-perf.c: New file.
> * tests/filter_seccomp-perf.test: New file.
> * tests/filter_seccomp-flag.c: New file.
> * tests/filter_seccomp-flag.test: New file.
> * tests/Makefile.am (EXTRA_DIST): Add filter_seccomp.in.
> (MISC_TESTS): Add filter_seccomp-perf.test and filter_seccomp-flag.test.
> (check_PROGRAMS): Add filter_seccomp-perf and filter_seccomp-flag.
> * tests/gen_tests.in: Add threads-execve test with -n and filter_seccomp
> test_prog_set.
> 
> Co-authored-by: Paul Chaignon <paul.chaignon at gmail.com>
> ---
>  tests/.gitignore               |  3 ++
>  tests/Makefile.am              |  5 +++
>  tests/filter_seccomp-flag.c    | 80 ++++++++++++++++++++++++++++++++++
>  tests/filter_seccomp-flag.test | 10 +++++
>  tests/filter_seccomp-perf.c    | 38 ++++++++++++++++
>  tests/filter_seccomp-perf.test | 25 +++++++++++
>  tests/filter_seccomp.in        |  4 ++
>  tests/gen_tests.in             |  2 +
>  tests/init.sh                  |  5 +++
>  tests/pure_executables.list    |  1 +
>  tests/status-none-f.c          | 19 ++++++++
>  11 files changed, 192 insertions(+)
>  create mode 100644 tests/filter_seccomp-flag.c
>  create mode 100755 tests/filter_seccomp-flag.test
>  create mode 100644 tests/filter_seccomp-perf.c
>  create mode 100755 tests/filter_seccomp-perf.test
>  create mode 100644 tests/filter_seccomp.in
>  create mode 100644 tests/status-none-f.c
> 
> diff --git a/tests/.gitignore b/tests/.gitignore
> index 4c854db8..83e525bb 100644
> --- a/tests/.gitignore
> +++ b/tests/.gitignore
> @@ -85,6 +85,7 @@ fdatasync
>  fflush
>  file_handle
>  file_ioctl
> +filter_seccomp-perf
>  filter-unavailable
>  finit_module
>  flock
> @@ -509,6 +510,7 @@ sched_yield
>  scm_rights
>  scno.h
>  seccomp-filter
> +filter_seccomp-flag
>  seccomp-filter-v
>  seccomp-strict
>  seccomp_get_action_avail
> @@ -588,6 +590,7 @@ statfs64
>  status-all
>  status-failed
>  status-none
> +status-none-f
>  status-none-threads
>  status-successful
>  status-unfinished
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index 156e359a..7f00a6a0 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -90,6 +90,8 @@ check_PROGRAMS = $(PURE_EXECUTABLES) \
>  	delay \
>  	execve-v \
>  	execveat-v \
> +	filter_seccomp-flag \
> +	filter_seccomp-perf \
>  	filter-unavailable \
>  	fork-f \
>  	fsync-y \
> @@ -313,6 +315,8 @@ MISC_TESTS = \
>  	detach-sleeping.test \
>  	detach-stopped.test \
>  	fflush.test \
> +	filter_seccomp-flag.test \
> +	filter_seccomp-perf.test \
>  	filter-unavailable.test \
>  	filtering_fd-syntax.test \
>  	filtering_syscall-syntax.test \
> @@ -395,6 +399,7 @@ EXTRA_DIST = \
>  	eventfd.expected \
>  	fadvise.h \
>  	fcntl-common.c \
> +	filter_seccomp.in \
>  	filter-unavailable.expected \
>  	fstatat.c \
>  	fstatx.c \
> diff --git a/tests/filter_seccomp-flag.c b/tests/filter_seccomp-flag.c
> new file mode 100644
> index 00000000..1b8a51f9
> --- /dev/null
> +++ b/tests/filter_seccomp-flag.c
> @@ -0,0 +1,80 @@
> +/*
> + * Check syscall numbers do not conflict with seccomp-filter flags.
> + *
> + * Copyright (c) 2019 Paul Chaignon <paul.chaignon at gmail.com>
> + * All rights reserved.
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#include <linux/audit.h>
> +#include <asm/unistd.h>
> +#include "defs.h"
> +
> +
> +/* Define these shorthand notations to simplify the syscallent files. */
> +#include "sysent_shorthand_defs.h"
> +
> +#define SEN(syscall_name) 0, 0
> +
> +const struct_sysent sysent0[] = {
> +#include "syscallent.h"
> +};
> +
> +#if SUPPORTED_PERSONALITIES > 1
> +# include PERSONALITY1_INCLUDE_FUNCS
> +static const struct_sysent sysent1[] = {
> +# include "syscallent1.h"
> +};
> +#endif
> +
> +#if SUPPORTED_PERSONALITIES > 2
> +# include PERSONALITY2_INCLUDE_FUNCS
> +static const struct_sysent sysent2[] = {
> +# include "syscallent2.h"
> +};
> +#endif
> +
> +const unsigned int nsyscall_vec[SUPPORTED_PERSONALITIES] = {
> +	ARRAY_SIZE(sysent0),
> +#if SUPPORTED_PERSONALITIES > 1
> +	ARRAY_SIZE(sysent1),
> +#endif
> +#if SUPPORTED_PERSONALITIES > 2
> +	ARRAY_SIZE(sysent2),
> +#endif
> +};
> +
> +struct audit_arch_t {
> +	unsigned int arch;
> +	unsigned int flag;
> +};
> +
> +static const struct audit_arch_t audit_arch_vec[SUPPORTED_PERSONALITIES] = {
> +#if SUPPORTED_PERSONALITIES > 1
> +	PERSONALITY0_AUDIT_ARCH,
> +	PERSONALITY1_AUDIT_ARCH,
> +# if SUPPORTED_PERSONALITIES > 2
> +	PERSONALITY2_AUDIT_ARCH,
> +# endif
> +#endif
> +};
> +
> +int
> +main(void)
> +{
> +	for (unsigned int p = 0; p < SUPPORTED_PERSONALITIES; ++p) {
> +		if (!audit_arch_vec[p].flag)
> +			continue;
> +		for (unsigned int nr = 1; nr < nsyscall_vec[p]; ++nr) {
> +			if (audit_arch_vec[p].flag & nr) {
> +				fprintf(stderr, "error: system call number %u "
> +					"of personality %u conflicts with "
> +					"seccomp-filter flag 0x%x\n", nr, p,
> +					audit_arch_vec[p].flag);
> +				return 1;

We prefer automatic %#x to manual 0x%x.

I wonder why don't you use error_msg_and_fail() instead of fprintf+return.

> +			}
> +		}
> +	}
> +	return 0;
> +}
> diff --git a/tests/filter_seccomp-flag.test b/tests/filter_seccomp-flag.test
> new file mode 100755
> index 00000000..4bb963c5
> --- /dev/null
> +++ b/tests/filter_seccomp-flag.test
> @@ -0,0 +1,10 @@
> +#!/bin/sh
> +#
> +# Check syscall numbers do not conflict with seccomp-filter flags.
> +#
> +# Copyright (c) 2019 Paul Chaignon <paul.chaignon at gmail.com>
> +# All rights reserved.
> +#
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +
> +./filter_seccomp-flag

I think this file can be replaced with a single line in tests/gen_tests.in
file:

filter_seccomp-flag	../$NAME

> diff --git a/tests/filter_seccomp-perf.c b/tests/filter_seccomp-perf.c
> new file mode 100644
> index 00000000..6ea0cab0
> --- /dev/null
> +++ b/tests/filter_seccomp-perf.c
> @@ -0,0 +1,38 @@
> +/*
> + * Check seccomp-filter is enabled.

This is not a check whether seccomp filter is enabled,
this is a check for seccomp filter performance.

> + *
> + * Copyright (c) 2019 Paul Chaignon <paul.chaignon at gmail.com>
> + * All rights reserved.
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#include "tests.h"
> +#include <signal.h>
> +#include <stdbool.h>
> +#include <stdio.h>
> +#include <unistd.h>
> +
> +static volatile bool stop = false;
> +
> +static void
> +handler(int signo)
> +{
> +    stop = true;

Strange tab stop here.

> +}
> +
> +int
> +main(void)
> +{
> +	unsigned int i;
> +	int rc = 0;
> +
> +	signal(SIGALRM, handler);
> +	alarm(1);
> +
> +	for (i = 0; !stop; i++) {
> +		rc |= chdir(".");
> +	}
> +	printf("%d\n", i);
> +	return rc;
> +}
> diff --git a/tests/filter_seccomp-perf.test b/tests/filter_seccomp-perf.test
> new file mode 100755
> index 00000000..b73c73d6
> --- /dev/null
> +++ b/tests/filter_seccomp-perf.test
> @@ -0,0 +1,25 @@
> +#!/bin/sh
> +#
> +# Check seccomp-filter is enabled.

Likewise, this is not a check whether seccomp filter is enabled,
this is a check for seccomp filter performance.

> +#
> +# Copyright (c) 2019 Paul Chaignon <paul.chaignon at gmail.com>
> +# All rights reserved.
> +#
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +
> +. "${srcdir=.}/init.sh"
> +
> +$STRACE -qfn -efchdir / > /dev/null 2> "$OUT" ||:
> +if grep "seccomp-filter is requested but unavailable" "$OUT" > /dev/null; then
> +	skip_ "seccomp-filter is unavailable"
> +fi

I suggest moving this check into a separate filter_seccomp.sh file
because it's going to be used elsewhere:

#!/bin/sh
# 
# Skip the test if seccomp filter is not available.
#
# Copyright (c) 2019 The strace developers.
# All rights reserved.
#
# SPDX-License-Identifier: GPL-2.0-or-later

$STRACE -n -f -e trace=fchdir / > /dev/null 2> "$LOG" ||:
if grep -x "[^:]*strace: seccomp-filter is requested but unavailable" \
   "$LOG" > /dev/null; then
        skip_ 'seccomp filter is unavailable'
fi

> +nb_no_seccomp=$($STRACE -qf -e signal=none -efchdir ../$NAME)
> +nb_seccomp=$($STRACE -qfn -e signal=none -efchdir ../$NAME 2> "$OUT")

Why redirecting stderr to $OUT?

What does nb_ prefix mean?  If it's number, let's use num_ prefix instead:

num_regular="$(run_strace    -f -qq -e signal=none -e trace=fchdir ../$NAME)"
mv "$LOG" "$LOG.regular"
num_seccomp="$(run_strace -n -f -qq -e signal=none -e trace=fchdir ../$NAME)"
mv "$LOG" "$LOG.seccomp"
match_diff "$LOG.regular" "$LOG.seccomp"

> +
> +# With seccomp-filter enabled, we should be able to complete at least 10 times

I've managed to find a quite slow system where this ratio is 8.

> +# more calls to chdir.
> +if [ "$nb_seccomp" -lt "$((10*nb_no_seccomp))" ]; then
> +	ratio=$((nb_seccomp / nb_no_seccomp))
> +	fail_ "Only $ratio more syscalls performed with seccomp-filter enabled"

$ratio times more

> +fi

I suggest to rewrite it this way:

min_ratio=8
# With seccomp-filter enabled, we should be able to complete
# at least $min_ratio times more chdir system calls.
ratio="$((num_seccomp / num_regular))"
if [ "$ratio" -lt "$min_ratio" ]; then
	fail_ "Only $ratio times more syscalls performed with seccomp-filter enabled"
fi

> diff --git a/tests/filter_seccomp.in b/tests/filter_seccomp.in
> new file mode 100644
> index 00000000..dc741911
> --- /dev/null
> +++ b/tests/filter_seccomp.in
> @@ -0,0 +1,4 @@
> +fork-f	-a26 -qq -f -e signal=none -e trace=chdir
> +vfork-f	-a26 -qq -f -e signal=none -e trace=chdir
> +fork-f	-a26 -qq -f -e signal=none -e trace=chdir,%memory,%ipc,%pure,%signal,%network -e status=failed
> +status-none-f	-f -e trace=!ptrace -e status=none
> diff --git a/tests/gen_tests.in b/tests/gen_tests.in
> index ab844128..c3c983fe 100644
> --- a/tests/gen_tests.in
> +++ b/tests/gen_tests.in
> @@ -65,6 +65,7 @@ fcntl64	-a8
>  fdatasync	-a14
>  file_handle	-e trace=name_to_handle_at,open_by_handle_at
>  file_ioctl	+ioctl.test
> +filter_seccomp	test_prog_set -n

This needs a prior check whether seccomp filter is unavailable, e.g.
filter_seccomp	. "${srcdir=.}/filter_seccomp.sh"; test_prog_set -n

>  finit_module	-a25
>  flock	-a19
>  fork-f	-a26 -qq -f -e signal=none -e trace=chdir
> @@ -506,6 +507,7 @@ sync_file_range2
>  sysinfo	-a14
>  syslog	-a35
>  tee
> +threads-execve	+threads-execve.test -n

I don't think this works at all.


-- 
ldv
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.strace.io/pipermail/strace-devel/attachments/20190831/136f8247/attachment.bin>


More information about the Strace-devel mailing list