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

[Tyler Hicks <tyhicks@canonical.com>]

[-- Attachment #1.1: Type: text/plain, Size: 4311 bytes --]

On 02/16/2017 12:40 PM, Tyler Hicks wrote:
> On 02/15/2017 07:10 PM, Kees Cook wrote:
>> On Mon, Feb 13, 2017 at 7:55 PM, Tyler Hicks <tyhicks@canonical.com> wrote:
>>> 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.
> 
>>
>>> +       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.

I was confused about which array you were talking about. I thought you
were talking about seccomp_actions_avail[].

No casts are needed when making seccomp_action_names[] const.

Tyler


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 801 bytes --]