On 08/07/2017 08:03 PM, Tyler Hicks wrote: > On 08/02/2017 10:19 PM, Kees Cook wrote: >> Both the upcoming logging improvements and changes to RET_KILL will need >> to know which filter a given seccomp return value originated from. In >> order to delay logic processing of result until after the seccomp loop, >> this adds a single pointer assignment on matches. This will allow both >> log and RET_KILL logic to work off the filter rather than doing more >> expensive tests inside the time-critical run_filters loop. >> >> Running tight cycles of getpid() with filters attached shows no measurable >> difference in speed. >> >> Suggested-by: Tyler Hicks <[email protected]> >> Signed-off-by: Kees Cook <[email protected]> >> --- >> kernel/seccomp.c | 11 ++++++++--- >> 1 file changed, 8 insertions(+), 3 deletions(-) >> >> diff --git a/kernel/seccomp.c b/kernel/seccomp.c >> index 98b59b5db90b..8bdcf01379e4 100644 >> --- a/kernel/seccomp.c >> +++ b/kernel/seccomp.c >> @@ -171,10 +171,12 @@ static int seccomp_check_filter(struct sock_filter >> *filter, unsigned int flen) >> /** >> * seccomp_run_filters - evaluates all seccomp filters against @sd >> * @sd: optional seccomp data to be passed to filters >> + * @match: stores struct seccomp_filter that resulted in the return value
Thinking just a bit more about this patch, can you document that @match
may be NULL upon return?
Tyler
>> *
>> * Returns valid seccomp BPF response codes.
>> */
>> -static u32 seccomp_run_filters(const struct seccomp_data *sd)
>> +static u32 seccomp_run_filters(const struct seccomp_data *sd,
>> + struct seccomp_filter **match)
>> {
>> struct seccomp_data sd_local;
>> u32 ret = SECCOMP_RET_ALLOW;
>
> My version of this patch initialized *match to f here. The reason I did
> that is because if BPF_PROG_RUN() returns RET_ALLOW for all
> filters, I didn't want *match to remain NULL when seccomp_run_filters()
> returns. FILTER_FLAG_LOG nor FILTER_FLAG_KILL_PROCESS would be affected
> by this because they don't care about RET_ALLOW actions but there could
> conceivably be a filter flag in the future that cares about RET_ALLOW
> and not initializing *match to the first filter could result in a latent
> bug for that filter flag.
>
> I'm fine with not adding the initialization since this is a hot path and
> it doesn't help any of the currently existing/planned filter flags but I
> wanted to at least mention it.
>
> Reviewed-by: Tyler Hicks <[email protected]>
>
> Tyler
>
>> @@ -198,8 +200,10 @@ static u32 seccomp_run_filters(const struct
>> seccomp_data *sd)
>> for (; f; f = f->prev) {
>> u32 cur_ret = BPF_PROG_RUN(f->prog, sd);
>>
>> - if ((cur_ret & SECCOMP_RET_ACTION) < (ret & SECCOMP_RET_ACTION))
>> + if ((cur_ret & SECCOMP_RET_ACTION) < (ret &
>> SECCOMP_RET_ACTION)) {
>> ret = cur_ret;
>> + *match = f;
>> + }
>> }
>> return ret;
>> }
>> @@ -566,6 +570,7 @@ static int __seccomp_filter(int this_syscall, const
>> struct seccomp_data *sd,
>> const bool recheck_after_trace)
>> {
>> u32 filter_ret, action;
>> + struct seccomp_filter *match = NULL;
>> int data;
>>
>> /*
>> @@ -574,7 +579,7 @@ static int __seccomp_filter(int this_syscall, const
>> struct seccomp_data *sd,
>> */
>> rmb();
>>
>> - filter_ret = seccomp_run_filters(sd);
>> + filter_ret = seccomp_run_filters(sd, &match);
>> data = filter_ret & SECCOMP_RET_DATA;
>> action = filter_ret & SECCOMP_RET_ACTION;
>>
>>
>
>
signature.asc
Description: OpenPGP digital signature
