Re: [PATCH v4 2/4] seccomp: Add sysctl to configure actions that should be logged

On 02/15/2017 07:10 PM, Kees Cook wrote:

On Mon, Feb 13, 2017 at 7:55 PM, Tyler Hicks [off-list ref] wrote:

quoted

diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index e36dfe9..270a227 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c

@@ -509,6 +509,22 @@ static void seccomp_send_sigsys(int syscall, int reason)
 }
 #endif /* CONFIG_SECCOMP_FILTER */

+static u32 seccomp_log_max_action = SECCOMP_RET_KILL;
+
+static inline void seccomp_log(unsigned long syscall, long signr, u32 action)
+{
+       /* Force an audit message to be emitted when the action is not greater
+        * than the configured maximum action.
+        */
+       if (action <= seccomp_log_max_action)
+               return __audit_seccomp(syscall, signr, action);
+
+       /* Let the audit subsystem decide if the action should be audited based
+        * on whether the current task itself is being audited.
+        */

Nitpick on comment style, please use:

/*
 * line 1
 * line 2...
 */

No problem.

quoted

+       return audit_seccomp(syscall, signr, action);
+}
+
 /*
  * Secure computing mode 1 allows only read/write/exit/sigreturn.
  * To be fully secure this must be combined with rlimit

@@ -534,7 +550,7 @@ static void __secure_computing_strict(int this_syscall)
 #ifdef SECCOMP_DEBUG
        dump_stack();
 #endif
-       audit_seccomp(this_syscall, SIGKILL, SECCOMP_RET_KILL);
+       seccomp_log(this_syscall, SIGKILL, SECCOMP_RET_KILL);
        do_exit(SIGKILL);
 }

@@ -633,18 +649,30 @@ static int __seccomp_filter(int this_syscall, const struct seccomp_data *sd,
                return 0;

        case SECCOMP_RET_ALLOW:
+               /* Open-coded seccomp_log(), optimized for the RET_ALLOW hot
+                * path.
+                *
+                * We only want to log RET_ALLOW actions when the admin has
+                * configured them to be logged via the log_max_action sysctl.
+                * Therefore, call __audit_seccomp() directly so that RET_ALLOW
+                * actions are not audited simply because the task is being
+                * audited.
+                */
+               if (unlikely(seccomp_log_max_action == SECCOMP_RET_ALLOW))
+                       __audit_seccomp(this_syscall, 0, action);
+
                return 0;

        case SECCOMP_RET_KILL:
        default:
-               audit_seccomp(this_syscall, SIGSYS, action);
+               seccomp_log(this_syscall, SIGSYS, action);
                do_exit(SIGSYS);
        }

        unreachable();

 skip:
-       audit_seccomp(this_syscall, 0, action);
+       seccomp_log(this_syscall, 0, action);
        return -1;
 }
 #else

@@ -917,12 +945,96 @@ long seccomp_get_filter(struct task_struct *task, unsigned long filter_off,
 #define SECCOMP_RET_TRACE_NAME         "trace"
 #define SECCOMP_RET_ALLOW_NAME         "allow"

+/* Largest strlen() of all action names */
+#define SECCOMP_RET_MAX_NAME_LEN       5
+
 static char seccomp_actions_avail[] = SECCOMP_RET_KILL_NAME    " "
                                      SECCOMP_RET_TRAP_NAME     " "
                                      SECCOMP_RET_ERRNO_NAME    " "
                                      SECCOMP_RET_TRACE_NAME    " "
                                      SECCOMP_RET_ALLOW_NAME;

+struct seccomp_action_name {
+       u32             action;
+       const char      *name;
+};
+
+static struct seccomp_action_name seccomp_action_names[] = {

As long as I'm nit-picking, this can be const too. :)

I'll have to cast to a non-const pointer when assigning ctl_table.data
but I think that's fine in this case.

Tyler

-Kees