| Message ID | 1501730353-46840-2-git-send-email-keescook@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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; > >
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 <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 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 <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; >> >> > >
On Mon, Aug 7, 2017 at 6:03 PM, Tyler Hicks <tyhicks@canonical.com> wrote: >> -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. Very true, yes. I did intentionally adjust this because I wanted to keep the hot path as untouched as possible. > 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. Yeah, and while I doubt I'll want to ever check "match" for RET_ALLOW, I'll add a big comment there to explain it. > Reviewed-by: Tyler Hicks <tyhicks@canonical.com> Thanks! -Kees
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; @@ -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;
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(-)