Re: [RFC PATCH v2 20/27] x86/cet/shstk: ELF header parsing of CET

[Yu-cheng Yu]

On Wed, 2018-07-11 at 12:37 -0700, Jann Horn wrote:
> On Tue, Jul 10, 2018 at 3:31 PM Yu-cheng Yu <yu-cheng.yu@xxxxxxxxx>
> wrote:
> > 
> > 
> > Look in .note.gnu.property of an ELF file and check if shadow stack
> > needs
> > to be enabled for the task.
> > 
> > Signed-off-by: H.J. Lu <hjl.tools@xxxxxxxxx>
> > Signed-off-by: Yu-cheng Yu <yu-cheng.yu@xxxxxxxxx>
> [...]
> > 
> > diff --git a/arch/x86/kernel/elf.c b/arch/x86/kernel/elf.c
> > new file mode 100644
> > index 000000000000..233f6dad9c1f
> > --- /dev/null
> > +++ b/arch/x86/kernel/elf.c
> [...]
> > 
> > +#define NOTE_SIZE_BAD(n, align, max) \
> > +ÂÂÂÂÂÂÂ((n->n_descsz < 8) || ((n->n_descsz % align) != 0) || \
> > +ÂÂÂÂÂÂÂÂ(((u8 *)(n + 1) + 4 + n->n_descsz) > (max)))
> Please do not compute out-of-bounds pointers and then compare them
> against an expected maximum pointer. Computing an out-of-bounds
> pointer is undefined behavior according to the C99 specification,
> section "6.5.6 Additive operators", paragraph 8; and in this case,
> n->n_descsz is 32 bits wide, which means that even if the compiler
> isn't doing anything funny, if you're operating on addresses in the
> last 4GiB of virtual memory and the pointer wraps around, this could
> break.
> In particular, if anyone ever uses this code in a 32-bit kernel, this
> is going to blow up.
> Please use size comparisons instead of pointer comparisons.

I will fix it.

> [...]
> > 
> > diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> > index 0ac456b52bdd..3395f6a631d5 100644
> > --- a/fs/binfmt_elf.c
> > +++ b/fs/binfmt_elf.c
> > @@ -1081,6 +1081,22 @@ static int load_elf_binary(struct
> > linux_binprm *bprm)
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂgoto out_free_dentry;
> > ÂÂÂÂÂÂÂÂ}
> > 
> > +#ifdef CONFIG_ARCH_HAS_PROGRAM_PROPERTIES
> > +
> > +ÂÂÂÂÂÂÂif (interpreter) {
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂretval = arch_setup_features(&loc->interp_elf_ex,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂinterp_elf_phdata,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂinterpreter, true);
> > +ÂÂÂÂÂÂÂ} else {
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂretval = arch_setup_features(&loc->elf_ex,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂelf_phdata,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂbprm->file, false);
> > +ÂÂÂÂÂÂÂ}
> So for non-static binaries, the ELF headers of ld.so determine
> whether
> CET will be on or off for the entire system, right? Is the intent
> here
> that ld.so should start with CET enabled, and then either use the
> compatibility bitmap or turn CET off at runtime if the executable or
> one of the libraries doesn't actually work with CET?


The kernel command-line options "no_cet_shstk" and "no_cet_ibt" turn
off CET features for the whole system. ÂThe GLIBC tunable
"glibc.tune.hwcap=-SHSTK,-IBT" turns off CET features for the current
shell. ÂAnother GLIBC tunable "glibc.tune.x86_shstk=<on, permissive>"
determines, in the current shell, how dlopen() deals with SHSTK legacy
lib's.

So, if ld.so's ELF header has SHSTK/IBT, and CET is enabled in the
current shell, it will run with CET enabled. ÂIf the application
executable and all its dependent libraries have CET, ld.so runs the
application with CET enabled. ÂOtherwise ld.so turns off SHSTK (and/or
sets up legacy bitmap for IBT) before passing to the application.

Yu-cheng