On 14.02.2022 13:56, Andrew Cooper wrote:
> With altcall, we convert indirect branches into direct ones. With that
> complete, none of the potential targets need an endbr64 instruction.
>
> Furthermore, removing the endbr64 instructions is a security defence-in-depth
> improvement, because it limits the options available to an attacker who has
> managed to hijack a function pointer.
>
> Introduce new .init.{ro,}data.cf_clobber sections. Have _apply_alternatives()
> walk over this, looking for any pointers into .text, and clobber an endbr64
> instruction if found. This is some minor structure (ab)use but it works
> alarmingly well.
>
> Signed-off-by: Andrew Cooper <andrew.coop...@citrix.com>
Reviewed-by: Jan Beulich <jbeul...@suse.com>
with two remarks, which ideally would be addressed by respective
small adjustments:
> @@ -330,6 +333,41 @@ static void init_or_livepatch _apply_alternatives(struct
> alt_instr *start,
> add_nops(buf + a->repl_len, total_len - a->repl_len);
> text_poke(orig, buf, total_len);
> }
> +
> + /*
> + * Clobber endbr64 instructions now that altcall has finished optimising
> + * all indirect branches to direct ones.
> + */
> + if ( force && cpu_has_xen_ibt )
> + {
> + void *const *val;
> + unsigned int clobbered = 0;
> +
> + /*
> + * This is some minor structure (ab)use. We walk the entire contents
> + * of .init.{ro,}data.cf_clobber as if it were an array of pointers.
> + *
> + * If the pointer points into .text, and at an endbr64 instruction,
> + * nop out the endbr64. This causes the pointer to no longer be a
> + * legal indirect branch target under CET-IBT. This is a
> + * defence-in-depth measure, to reduce the options available to an
> + * adversary who has managed to hijack a function pointer.
> + */
> + for ( val = __initdata_cf_clobber_start;
> + val < __initdata_cf_clobber_end;
> + val++ )
> + {
> + void *ptr = *val;
> +
> + if ( !is_kernel_text(ptr) || !is_endbr64(ptr) )
> + continue;
> +
> + add_nops(ptr, 4);
This literal 4 would be nice to have a #define next to where the ENDBR64
encoding has its central place.
> --- a/xen/arch/x86/xen.lds.S
> +++ b/xen/arch/x86/xen.lds.S
> @@ -221,6 +221,12 @@ SECTIONS
> *(.initcall1.init)
> __initcall_end = .;
>
> + . = ALIGN(POINTER_ALIGN);
> + __initdata_cf_clobber_start = .;
> + *(.init.data.cf_clobber)
> + *(.init.rodata.cf_clobber)
> + __initdata_cf_clobber_end = .;
> +
> *(.init.data)
> *(.init.data.rel)
> *(.init.data.rel.*)
With r/o data ahead and r/w data following, may I suggest to flip the
order of the two section specifiers you add?
Jan
