diff mbox

xen/arm: alternative: Make it possible to patch outside of the hypervisor

Message ID 1472716599-2894-1-git-send-email-julien.grall@arm.com (mailing list archive)
State New, archived
Headers show

Commit Message

Julien Grall Sept. 1, 2016, 7:56 a.m. UTC
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(-)

Comments

Konrad Rzeszutek Wilk Sept. 6, 2016, 7:25 p.m. UTC | #1
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
>
Konrad Rzeszutek Wilk Sept. 7, 2016, 3:06 a.m. UTC | #2
> > ---
> >  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?
Julien Grall Sept. 7, 2016, 6:52 a.m. UTC | #3
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(&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
>>
>

Regards,
Julien Grall Sept. 7, 2016, 11:36 a.m. UTC | #4
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 mbox

Patch

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(&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);
     }