Re: [Xen-devel] [PATCH v3 07/13] xen/arm: Simplify alternative patching of non-writable region

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

On Tue, Jun 12, 2018 at 12:36:37PM +0100, Julien Grall wrote:
> During the MMU setup process, Xen will set SCTLR_EL2.WNX
> (Write-Non-eXecutable) bit. Because of that, the alternative code need
> to re-mapped the region in a difference place in order to modify the
> text section.
> 
> At the moment, the function patching the code is only aware of the
> re-mapped region. This requires the caller to mess with Xen internal in
> order to have function such as is_active_kernel_text() working.
> 
> All the interactions with Xen internal can be removed by specifying the
> offset between the region patch and the writable region for updating the
> instruction
> 
> This simplification will also make it easier to integrate dynamic patching
> in a follow-up patch. Indeed, the callback address should be in
> an original region and not re-mapped only which is writeable non-executable.
> 
> This is part of XSA-263.
> 
> Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
> Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> 
> ---
> 
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> 
>     Changes in v3:
>         - Add stefano's reviewed-by
> 
>     Changes in v2:
>         - Add commit message
>         - Remove comment in the code that does not make sense anymore
> ---
>  xen/arch/arm/alternative.c | 42 +++++++++++++-----------------------------
>  1 file changed, 13 insertions(+), 29 deletions(-)
> 
> diff --git a/xen/arch/arm/alternative.c b/xen/arch/arm/alternative.c
> index 9ffdc475d6..936cf04956 100644
> --- a/xen/arch/arm/alternative.c
> +++ b/xen/arch/arm/alternative.c
> @@ -97,12 +97,16 @@ static u32 get_alt_insn(const struct alt_instr *alt,
>  /*
>   * The region patched should be read-write to allow __apply_alternatives
>   * to replacing the instructions when necessary.
> + *
> + * @update_offset: Offset between the region patched and the writable
> + * region for the update. 0 if the patched region is writable.
>   */
> -static int __apply_alternatives(const struct alt_region *region)
> +static int __apply_alternatives(const struct alt_region *region,
> +                                paddr_t update_offset)
>  {
>      const struct alt_instr *alt;
> -    const u32 *replptr;
> -    u32 *origptr;
> +    const u32 *replptr, *origptr;
> +    u32 *updptr;
>  
>      printk(XENLOG_INFO "alternatives: Patching with alt table %p -> %p\n",
>             region->begin, region->end);
> @@ -118,6 +122,7 @@ static int __apply_alternatives(const struct alt_region 
> *region)
>          BUG_ON(alt->alt_len != alt->orig_len);
>  
>          origptr = ALT_ORIG_PTR(alt);
> +        updptr = (void *)origptr + update_offset;
>          replptr = ALT_REPL_PTR(alt);
>  
>          nr_inst = alt->alt_len / sizeof(insn);
> @@ -125,7 +130,7 @@ 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);
> -            *(origptr + i) = cpu_to_le32(insn);
> +            *(updptr + i) = cpu_to_le32(insn);
>          }
>  
>          /* Ensure the new instructions reached the memory and nuke */
> @@ -162,9 +167,6 @@ static int __apply_alternatives_multi_stop(void *unused)
>          paddr_t xen_size = _end - _start;
>          unsigned int xen_order = get_order_from_bytes(xen_size);
>          void *xenmap;
> -        struct virtual_region patch_region = {
> -            .list = LIST_HEAD_INIT(patch_region.list),
> -        };
>  
>          BUG_ON(patched);
>  
> @@ -177,31 +179,13 @@ static int __apply_alternatives_multi_stop(void *unused)
>          /* Re-mapping Xen is not expected to fail during boot. */
>          BUG_ON(!xenmap);
>  
> -        /*
> -         * If we generate a new branch instruction, the target will be
> -         * calculated in this re-mapped Xen region. So we have to register
> -         * this re-mapped Xen region as a virtual region temporarily.

What about this?

Won't this mean the traps (if there are any) won't be recognized at all
during this patching?

> -         */
> -        patch_region.start = xenmap;
> -        patch_region.end = xenmap + xen_size;
> -        register_virtual_region(&patch_region);
> +        region.begin = __alt_instructions;
> +        region.end = __alt_instructions_end;
>  
> -        /*
> -         * 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 *)__alt_instructions - (void *)_start + xenmap;
> -        region.end = (void *)__alt_instructions_end - (void *)_start + 
> xenmap;
> -
> -        ret = __apply_alternatives(&region);
> +        ret = __apply_alternatives(&region, xenmap - (void *)_start);
>          /* The patching is not expected to fail during boot. */
>          BUG_ON(ret != 0);
>  
> -        unregister_virtual_region(&patch_region);
> -
>          vunmap(xenmap);
>  
>          /* Barriers provided by the cache flushing */
> @@ -235,7 +219,7 @@ int apply_alternatives(const struct alt_instr *start, 
> const struct alt_instr *en
>          .end = end,
>      };
>  
> -    return __apply_alternatives(&region);
> +    return __apply_alternatives(&region, 0);
>  }
>  
>  /*
> -- 
> 2.11.0
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel