Re: [PATCH] xen/arm: alternative: Make it possible to patch outside of the hypervisor

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Thu, Sep 01, 2016 at 08:56:39AM +0100, Julien Grall wrote:
> With livepatch the alternatives that should be patched are outside of
> the Xen hypervisor _start -> _end. The current code is assuming that
> only Xen could be patched and therefore will explode when a payload
> contains alternatives.
> 
> Given that alt_instr contains a relative offset, the function
> __apply_alternatives could directly take in parameter the virtual
                                             ^- the    ^- for
> address of the alt_instr set of the re-mapped region. So we can mandate
> the callers of __apply_alternatives to ensure the region has read-write

s/ensure the region/provide us with a region that/
> access beforehand.

How do we mandate it?

Should the __apply_alternatives have an comment about it?

> 
> The only caller that will patch directly the Xen binary is the function
> __apply_alternatives_multi_stop. The other caller apply_alternatives
> will work on the payload which will still have read-write access at that
> time.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> 
> ---
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
>     This is an alternative of the patch suggested by Konrad [1] to fix
>     alternatives patching with livepatching.
> 
>     [1] https://lists.xenproject.org/archives/html/xen-devel/2016-08/msg02880.html

You also did

 s/_stext/_start/

> 
> ---
>  xen/arch/arm/alternative.c | 58 +++++++++++++++++++++++++---------------------
>  1 file changed, 31 insertions(+), 27 deletions(-)
> 
> diff --git a/xen/arch/arm/alternative.c b/xen/arch/arm/alternative.c
> index 8ee5a11..db4bd0f 100644
> --- a/xen/arch/arm/alternative.c
> +++ b/xen/arch/arm/alternative.c
> @@ -97,26 +97,11 @@ static u32 get_alt_insn(const struct alt_instr *alt,
>  static int __apply_alternatives(const struct alt_region *region)
>  {
>      const struct alt_instr *alt;
> -    const u32 *origptr, *replptr;
> -    u32 *writeptr, *writemap;
> -    mfn_t text_mfn = _mfn(virt_to_mfn(_stext));
> -    unsigned int text_order = get_order_from_bytes(_end - _start);
> +    const u32 *replptr;
> +    u32 *origptr;
>  
>      printk(XENLOG_INFO "alternatives: Patching kernel code\n");
>  
> -    /*
> -     * The text section is read-only. So re-map Xen to be able to patch
> -     * the code.
> -     */
> -    writemap = __vmap(&text_mfn, 1 << text_order, 1, 1, PAGE_HYPERVISOR,
> -                      VMAP_DEFAULT);
> -    if ( !writemap )
> -    {
> -        printk(XENLOG_ERR "alternatives: Unable to map the text section (size %u)\n",
> -               1 << text_order);
> -        return -ENOMEM;
> -    }
> -
>      for ( alt = region->begin; alt < region->end; alt++ )
>      {
>          u32 insn;
> @@ -128,7 +113,6 @@ static int __apply_alternatives(const struct alt_region *region)
>          BUG_ON(alt->alt_len != alt->orig_len);
>  
>          origptr = ALT_ORIG_PTR(alt);
> -        writeptr = origptr - (u32 *)_start + writemap;
>          replptr = ALT_REPL_PTR(alt);
>  
>          nr_inst = alt->alt_len / sizeof(insn);
> @@ -136,19 +120,17 @@ static int __apply_alternatives(const struct alt_region *region)
>          for ( i = 0; i < nr_inst; i++ )
>          {
>              insn = get_alt_insn(alt, origptr + i, replptr + i);
> -            *(writeptr + i) = cpu_to_le32(insn);
> +            *(origptr + i) = cpu_to_le32(insn);
>          }
>  
>          /* Ensure the new instructions reached the memory and nuke */
> -        clean_and_invalidate_dcache_va_range(writeptr,
> -                                             (sizeof (*writeptr) * nr_inst));
> +        clean_and_invalidate_dcache_va_range(origptr,
> +                                             (sizeof (*origptr) * nr_inst));
>      }
>  
>      /* Nuke the instruction cache */
>      invalidate_icache();
>  
> -    vunmap(writemap);
> -
>      return 0;
>  }
>  
> @@ -159,10 +141,6 @@ static int __apply_alternatives(const struct alt_region *region)
>  static int __apply_alternatives_multi_stop(void *unused)
>  {
>      static int patched = 0;
> -    const struct alt_region region = {
> -        .begin = __alt_instructions,
> -        .end = __alt_instructions_end,
> -    };
>  
>      /* We always have a CPU 0 at this point (__init) */
>      if ( smp_processor_id() )
> @@ -174,12 +152,38 @@ static int __apply_alternatives_multi_stop(void *unused)
>      else
>      {
>          int ret;
> +        struct alt_region region;
> +        mfn_t xen_mfn = _mfn(virt_to_mfn(_start));
> +        unsigned int xen_order = get_order_from_bytes(_end - _start);
> +        char *xenmap;

Could this be 'void *'?
>  
>          BUG_ON(patched);
> +
> +        /*
> +         * The text and inittext section is read-only. So re-map Xen to
> +         * be able to patch the code.
> +         */
> +        xenmap = __vmap(&xen_mfn, 1U << xen_order, 1, 1, PAGE_HYPERVISOR,
> +                        VMAP_DEFAULT);
> +        /* Re-mapping Xen is not expected to fail during boot. */
> +        BUG_ON(!xenmap);
> +
> +        /*
> +         * Find the virtual address of the alternative region in the new
> +         * mapping.
> +         * alt_instr contains relative offset, so the function
> +         * __apply_alternatives will patch in the re-mapped version of
> +         * Xen.
> +         */
> +        region.begin = (void *)((char *)__alt_instructions - _start + xenmap);
> +        region.end = (void *)((char *)__alt_instructions_end - _start + xenmap);

Which I think would make this have less casts?

Oh wait. _start is a char *, so we would have to have:

  region.begin = (void *)__alt_instructions - (void *)_start + xenmap;
  region.end = (void *)__alt_instructions_end - (void *)_start + xenmap;

Julien, you are the maintainer, so your call.

> +
>          ret = __apply_alternatives(&region);
>          /* The patching is not expected to fail during boot. */
>          BUG_ON(ret != 0);
>  
> +        vunmap(xenmap);
> +
>          /* Barriers provided by the cache flushing */
>          write_atomic(&patched, 1);
>      }
> -- 
> 1.9.1
> 

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