Re: [PATCH 1/4] seccomp: Provide matching filter for introspection

[Tyler Hicks <tyhicks@canonical.com>]

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

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 <tyhicks@canonical.com>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  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
>   *
>   * 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 <tyhicks@canonical.com>

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;
>  
> 



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