Re: [5.2 REGRESSION] Generic vDSO breaks seccomp-enabled userspace on i386

[Kees Cook]

On Mon, Jul 22, 2019 at 04:47:36PM -0700, Andy Lutomirski wrote:
> On Mon, Jul 22, 2019 at 4:28 PM Kees Cook <keescook@xxxxxxxxxxxx> wrote:
> > I've built a straw-man for this idea... but I have to say I don't
> > like it. This can lead to really unexpected behaviors if someone
> > were to have differing filters for the two syscalls. For example,
> > let's say someone was doing a paranoid audit of 2038-unsafe clock usage
> > and marked clock_gettime() with RET_KILL and marked clock_gettime64()
> > with RET_LOG. This aliasing would make clock_gettime64() trigger with
> > RET_KILL...
> 
> This particular issue is solvable:
> 
> > +       /* Handle syscall aliases when result is not SECCOMP_RET_ALLOW. */
> > +       if (unlikely(action != SECCOMP_RET_ALLOW)) {
> > +               int alias;
> > +
> > +               alias = seccomp_syscall_alias(sd->arch, sd->nr);
> > +               if (unlikely(alias != -1)) {
> > +                       /* Use sd_local for an aliased syscall. */
> > +                       if (sd != &sd_local) {
> > +                               sd_local = *sd;
> > +                               sd = &sd_local;
> > +                       }
> > +                       sd_local.nr = alias;
> > +
> > +                       /* Run again, with the alias, accepting the results. */
> > +                       filter_ret = seccomp_run_filters(sd, &match);
> > +                       data = filter_ret & SECCOMP_RET_DATA;
> > +                       action = filter_ret & SECCOMP_RET_ACTION_FULL;
> 
> How about:
> 
> new_data = ...;
> new_action = ...;
> if (new_action == SECCOMP_RET_ALLOW) {
>   data = new_data;
>   action = new_action;
> }

Spelling it out for myself: this means that if both syscalls have
non-RET_ALLOW results, the original result is kept. But if the alias
is allowed, allow it. That solves my particular example, but I don't
think it's enough. (And it might be just as bad.) What if someone wants
to RET_TRACE clock_gettime64 and other syscalls are RET_ALLOWed. Now
clock_gettime64 cannot be traced since clock_gettime gets RET_ALLOW and
replaces the results.

> It might also be nice to allow a filter to say "hey, I want to set
> this result and I do *not* want compatibility aliases applied", but
> I'm not quite sure how to express that.

Especially since we're working on "old" filters.

> I don't love this whole concept, but I also don't have a better idea.

How about we revert the vDSO change? :P

I keep coming back to using the vDSO return address as an indicator.
Most vDSO calls don't make syscalls, yes? So they're normally
unfilterable by seccomp.

What was the prior vDSO behavior?

-- 
Kees Cook