Re: [Xen-devel] [PATCH v5 18/28] xsplice: Add support for alternatives

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 24.03.16 at 21:00, <konrad.wilk@xxxxxxxxxx> wrote:
> --- a/xen/arch/x86/alternative.c
> +++ b/xen/arch/x86/alternative.c
> @@ -28,7 +28,7 @@
>  extern struct alt_instr __alt_instructions[], __alt_instructions_end[];
>  
>  #ifdef K8_NOP1
> -static const unsigned char k8nops[] __initconst = {
> +static const unsigned char k8nops[] = {

Just like in Linux these init annotations should become conditional
upon CONFIG_XSPLICE (and I realize this applies to at least the
previous patch too).

> @@ -127,7 +127,7 @@ static void __init add_nops(void *insns, unsigned int len)
>   *
>   * This routine is called with local interrupt disabled.
>   */
> -static void *__init text_poke_early(void *addr, const void *opcode, size_t 
> len)
> +static void *text_poke_early(void *addr, const void *opcode, size_t len)

I'm afraid this function's name as well as the comment preceding it
need to change.

> -static void __init apply_alternatives(struct alt_instr *start, struct 
> alt_instr *end)
> +void apply_alternatives_nocheck(struct alt_instr *start, struct alt_instr 
> *end)

Same here - the preceding comment needs adjustment.

> --- a/xen/arch/x86/test/xen_hello_world_func.c
> +++ b/xen/arch/x86/test/xen_hello_world_func.c
> @@ -5,10 +5,13 @@
>  
>  #include <xen/config.h>
>  #include <xen/types.h>
> +#include <asm/nops.h>
> +#include <asm/alternative.h>
>  
>  /* Our replacement function for xen_extra_version. */
>  const char *xen_hello_world(void)
>  {
> +    alternative(ASM_NOP1, ASM_NOP1, 1);

Above you say the code is being exercised by this: How can you be
sure that whatever feature has value 1 is actually present? The
pending SMEP/SMAP patches add X86_FEATURE_ALWAYS for such
a purpose.

> --- a/xen/common/xsplice.c
> +++ b/xen/common/xsplice.c
> @@ -590,6 +590,22 @@ static int prepare_payload(struct payload *payload,
>          region->ex_end = (struct exception_table_entry *)(sec->load_addr + 
> sec->sec->sh_size);
>  
>          sort_exception_table(region->ex, region->ex_end);
> +
> +    }

These two lines want to be swapped.

> +    sec = xsplice_elf_sec_by_name(elf, ".altinstructions");
> +    if ( sec )
> +    {
> +        if ( !sec->sec->sh_size ||
> +             (sec->sec->sh_size % sizeof (struct alt_instr)) )
> +        {
> +            dprintk(XENLOG_DEBUG, "%s%s: Wrong size of .alt_instr (exp:%lu 
> vs %lu)!\n",
> +                    XSPLICE, elf->name, sizeof (struct alt_instr),
> +                    sec->sec->sh_size);
> +            return -EINVAL;
> +        }
> +        apply_alternatives_nocheck((struct alt_instr *)sec->load_addr,
> +                                   (struct alt_instr *)(sec->load_addr +
> +                                   sec->sec->sh_size));

I think alternative patching needs to enforce that only code/data
within the owning image gets patched, to avoid abuse.

> --- a/xen/include/asm-x86/alternative.h
> +++ b/xen/include/asm-x86/alternative.h
> @@ -23,6 +23,12 @@ struct alt_instr {
>      u8  replacementlen;     /* length of new instruction, <= instrlen */
>  };
>  
> +/*
> + * An variant to be used on code that can be patched without many checks.
> + */

"A variant", comment style, and - what does "many" mean?

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel