Re: [PATCH v17 10/15] seccomp: add SECCOMP_RET_ERRNO

[Will Drewry <wad@chromium.org>]

On Fri, Apr 6, 2012 at 4:19 PM, Andrew Morton <akpm@linux-foundation.org> wrote:
> On Thu, 29 Mar 2012 15:01:55 -0500
> Will Drewry <wad@chromium.org> wrote:
>
>> This change adds the SECCOMP_RET_ERRNO as a valid return value from a
>> seccomp filter.  Additionally, it makes the first use of the lower
>> 16-bits for storing a filter-supplied errno.  16-bits is more than
>> enough for the errno-base.h calls.
>>
>> Returning errors instead of immediately terminating processes that
>> violate seccomp policy allow for broader use of this functionality
>> for kernel attack surface reduction.  For example, a linux container
>> could maintain a whitelist of pre-existing system calls but drop
>> all new ones with errnos.  This would keep a logically static attack
>> surface while providing errnos that may allow for graceful failure
>> without the downside of do_exit() on a bad call.
>>
>>
>> ...
>>
>> @@ -64,11 +65,17 @@ struct seccomp {
>>       struct seccomp_filter *filter;
>>  };
>>
>> -extern void __secure_computing(int);
>> -static inline void secure_computing(int this_syscall)
>> +/*
>> + * Direct callers to __secure_computing should be updated as
>> + * CONFIG_HAVE_ARCH_SECCOMP_FILTER propagates.
>
> Are there any such callers?  There's one I see in arm, but it's called
> from assembly code.

I think just arm, but I was trying to limit the patch growth as much
as I could, practically.  ARM support is relevant to my interests, and
I need to have patches out for review quite soon (once this series
settles :).

I hesitate to ask, but should I add a patch to this series for arm?

>> + */
>> +extern void __secure_computing(int) __deprecated;
>> +extern int __secure_computing_int(int);
>> +static inline int secure_computing(int this_syscall)
>>  {
>>       if (unlikely(test_thread_flag(TIF_SECCOMP)))
>> -             __secure_computing(this_syscall);
>> +             return  __secure_computing_int(this_syscall);
>> +     return 0;
>>  }
>>
>> ...
>>
>>  void __secure_computing(int this_syscall)
>>  {
>> +     /* Filter calls should never use this function. */
>> +     BUG_ON(current->seccomp.mode == SECCOMP_MODE_FILTER);
>> +     __secure_computing_int(this_syscall);
>> +}
>> +
>> +int __secure_computing_int(int this_syscall)
>
> What the heck does "_int" mean here?  I read it as "integer" but
> perhaps it's shorthand for "internal".  Give us a better name, please.
> Or a code comment.

It meant "returns an int", but its unclear.  I definitely will add a
comment, but I'm open to better naming.  Perhaps it'd make sense to
bring it inline with the other hook call styles:
  /* <proper comment here> */
  __secure_computing_enter(int this_syscall)

I can keep the other call as 'deprecated', and then remove it once all
the callers are updated.

I'll reply to the rest of the mails shortly - thanks!
will