On 14/02/2022 13:06, Jan Beulich wrote:
> 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>
Thanks,
> 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.
We don't have an encoding of ENDBR64 in a central place.
The best you can probably have is
#define ENDBR64_LEN 4
in endbr.h ?
>
>> --- 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?
I don't follow. This is all initdata which is merged together into a
single section.
The only reason const data is split out in the first place is to appease
the toolchains, not because it makes a difference.
~Andrew
