Re: [PATCH v2 seccomp 2/6] asm/syscall.h: Add syscall_arches[] array

[YiFei Zhu]

[resending this too]

On Thu, Sep 24, 2020 at 6:01 PM Kees Cook <keescook@xxxxxxxxxxxx> wrote:
> Disregarding the "how" of this, yeah, we'll certainly need something to
> tell seccomp about the arrangement of syscall tables and how to find
> them.
>
> However, I'd still prefer to do this on a per-arch basis, and include
> more detail, as I've got in my v1.
>
> Something missing from both styles, though, is a consolidation of
> values, where the AUDIT_ARCH* isn't reused in both the seccomp info and
> the syscall_get_arch() return. The problems here were two-fold:
>
> 1) putting this in syscall.h meant you do not have full NR_syscall*
>    visibility on some architectures (e.g. arm64 plays weird games with
>    header include order).

I don't get this one -- I'm not playing with NR_syscall here.

> 2) seccomp needs to handle "multiplexed" tables like x86_x32 (distros
>    haven't removed CONFIG_X86_X32 widely yet, so it is a reality that
>    it must be dealt with), which means seccomp's idea of the arch
>    "number" can't be the same as the AUDIT_ARCH.

Why so? Does anyone actually use x32 in a container? The memory cost
and analysis cost is on everyone. The worst case scenario if we don't
support it is that the syscall is not accelerated.

> So, likely a combo of approaches is needed: an array (or more likely,
> enum), declared in the per-arch seccomp.h file. And I don't see a way
> to solve #1 cleanly.
>
> Regardless, it needs to be split per architecture so that regressions
> can be bisected/reverted/isolated cleanly. And if we can't actually test
> it at runtime (or find someone who can) it's not a good idea to make the
> change. :)

You have a good point regarding tests. Don't see how it affects
regressions though. Only one file here is ever included per-build.

> > [...]
> > diff --git a/arch/x86/include/asm/syscall.h b/arch/x86/include/asm/syscall.h
> > index 7cbf733d11af..e13bb2a65b6f 100644
> > --- a/arch/x86/include/asm/syscall.h
> > +++ b/arch/x86/include/asm/syscall.h
> > @@ -97,6 +97,10 @@ static inline void syscall_set_arguments(struct task_struct *task,
> >       memcpy(&regs->bx + i, args, n * sizeof(args[0]));
> >  }
> >
> > +static __maybe_unused const int syscall_arches[] = {
> > +     AUDIT_ARCH_I386
> > +};
> > +
> >  static inline int syscall_get_arch(struct task_struct *task)
> >  {
> >       return AUDIT_ARCH_I386;
> > @@ -152,6 +156,13 @@ static inline void syscall_set_arguments(struct task_struct *task,
> >       }
> >  }
> >
> > +static __maybe_unused const int syscall_arches[] = {
> > +     AUDIT_ARCH_X86_64,
> > +#ifdef CONFIG_IA32_EMULATION
> > +     AUDIT_ARCH_I386,
> > +#endif
> > +};
>
> I'm leaving this section quoted because I'll refer to it in a later
> patch review...
>
> --
> Kees Cook