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 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@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?
> 
> 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.

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