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

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@arm.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

---

Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

    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(-)

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@arm.com>
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> 
> ---
> 
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
>     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
>

Hi Konrad,

On 12/06/2018 22:17, Konrad Rzeszutek Wilk wrote:
> 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@arm.com>
>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
>>
>> ---
>>
>> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>>
>>      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?

What do you mean by recognized? This new region will only be accessed to 
write instruction. The only potential fault on that region is a data abort.

So I am not sure why we would need to register it as a virtual region here.

Cheers,

On Tue, Jun 12, 2018 at 11:06:16PM +0100, Julien Grall wrote:
> Hi Konrad,
> 
> On 12/06/2018 22:17, Konrad Rzeszutek Wilk wrote:
> > 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@arm.com>
> > > Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> > > 
> > > ---
> > > 
> > > Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > > 
> > >      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?
> 
> What do you mean by recognized? This new region will only be accessed to
> write instruction. The only potential fault on that region is a data abort.

Exactly the data abort.

My recollection is that the data abort would print a stack trace. And 
the stack trace needs symbol information - which it gets from virtual_region.

But if virtual_region is not registered then the stack won't contain the
name of the function that had an data abort as the patching is done.

Hi Konrad,

On 22/06/18 14:33, Konrad Rzeszutek Wilk wrote:
> On Tue, Jun 12, 2018 at 11:06:16PM +0100, Julien Grall wrote:
>> On 12/06/2018 22:17, Konrad Rzeszutek Wilk wrote:
>>> On Tue, Jun 12, 2018 at 12:36:37PM +0100, Julien Grall wrote:
>>> Won't this mean the traps (if there are any) won't be recognized at all
>>> during this patching?
>>
>> What do you mean by recognized? This new region will only be accessed to
>> write instruction. The only potential fault on that region is a data abort.
> 
> Exactly the data abort.
> 
> My recollection is that the data abort would print a stack trace. And
> the stack trace needs symbol information - which it gets from virtual_region.
> 
> But if virtual_region is not registered then the stack won't contain the
> name of the function that had an data abort as the patching is done.

I am not sure to understand what you mean by "the name of the function 
that had a data abort". Could you give a bit more detail?

Cheers,