| Message ID | 1472716599-2894-1-git-send-email-julien.grall@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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(®ion); > /* 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/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"); This is still there. Should it be "Patching %p->%p code" instead?
Hi Konrad, On 06/09/2016 20:25, Konrad Rzeszutek Wilk wrote: > 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? I don't have any idea how to write the check here. I think the only way would be a comment on __apply_alternatives as you suggested below. > > 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/ And s/text_mfn/xen_mfn/ I didn't write a separate patch because the code has been reworked quite a lot. But I am fine to move this in a separate patch if necessary. > >> >> --- >> 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. Your suggestion seems to be clearer. > >> + >> ret = __apply_alternatives(®ion); >> /* 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 >> > Regards,
Hi Konrad, On 07/09/2016 04:06, Konrad Rzeszutek Wilk wrote: >>> --- >>> 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"); > > This is still there. Should it be "Patching %p->%p code" instead? I will replace by printk(XENLOG_INFO "alternatives: alt table %p -> %p", region->begin, region->end); Regards,
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; 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); + ret = __apply_alternatives(®ion); /* 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); }
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 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 access beforehand. 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 --- xen/arch/arm/alternative.c | 58 +++++++++++++++++++++++++--------------------- 1 file changed, 31 insertions(+), 27 deletions(-)