Messages in this thread | | | From | YiFei Zhu <> | Date | Thu, 24 Sep 2020 20:27:40 -0500 | Subject | Re: [PATCH v2 seccomp 2/6] asm/syscall.h: Add syscall_arches[] array |
| |
[resending this too]
On Thu, Sep 24, 2020 at 6:01 PM Kees Cook <keescook@chromium.org> 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(®s->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
| |