[PATCH 1/2] xen/livepatch: Clean up arch relocation handling

[PATCH 1/2] xen/livepatch: Clean up arch relocation handling

[Andrew Cooper]

 * Reduce symbol scope and initalisation as much as possible
 * Annotate a fallthrough case in arm64
 * Fix switch statement style in arm32

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
CC: Ross Lagerwall <ross.lagerwall@citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>

The purpose of this patch is simply to make the following patch easier to
review.
---
 xen/arch/arm/arm32/livepatch.c | 27 ++++++++++++---------------
 xen/arch/arm/arm64/livepatch.c | 19 +++++++------------
 xen/arch/x86/livepatch.c       | 13 +++++--------
 3 files changed, 24 insertions(+), 35 deletions(-)

diff --git a/xen/arch/arm/arm32/livepatch.c b/xen/arch/arm/arm32/livepatch.c
index a7fd5e2..a328179 100644
--- a/xen/arch/arm/arm32/livepatch.c
+++ b/xen/arch/arm/arm32/livepatch.c
@@ -224,21 +224,21 @@ int arch_livepatch_perform(struct livepatch_elf *elf,
                            const struct livepatch_elf_sec *rela,
                            bool use_rela)
 {
-    const Elf_RelA *r_a;
-    const Elf_Rel *r;
-    unsigned int symndx, i;
-    uint32_t val;
-    void *dest;
+    unsigned int i;
     int rc = 0;
 
     for ( i = 0; i < (rela->sec->sh_size / rela->sec->sh_entsize); i++ )
     {
+        unsigned int symndx;
+        uint32_t val;
+        void *dest;
         unsigned char type;
-        s32 addend = 0;
+        s32 addend;
 
         if ( use_rela )
         {
-            r_a = rela->data + i * rela->sec->sh_entsize;
+            const Elf_RelA *r_a = rela->data + i * rela->sec->sh_entsize;
+
             symndx = ELF32_R_SYM(r_a->r_info);
             type = ELF32_R_TYPE(r_a->r_info);
             dest = base->load_addr + r_a->r_offset; /* P */
@@ -246,10 +246,12 @@ int arch_livepatch_perform(struct livepatch_elf *elf,
         }
         else
         {
-            r = rela->data + i * rela->sec->sh_entsize;
+            const Elf_Rel *r = rela->data + i * rela->sec->sh_entsize;
+
             symndx = ELF32_R_SYM(r->r_info);
             type = ELF32_R_TYPE(r->r_info);
             dest = base->load_addr + r->r_offset; /* P */
+            addend = get_addend(type, dest);
         }
 
         if ( symndx > elf->nsym )
@@ -259,13 +261,11 @@ int arch_livepatch_perform(struct livepatch_elf *elf,
             return -EINVAL;
         }
 
-        if ( !use_rela )
-            addend = get_addend(type, dest);
-
         val = elf->sym[symndx].sym->st_value; /* S */
 
         rc = perform_rel(type, dest, val, addend);
-        switch ( rc ) {
+        switch ( rc )
+        {
         case -EOVERFLOW:
             dprintk(XENLOG_ERR, LIVEPATCH "%s: Overflow in relocation %u in %s for %s!\n",
                     elf->name, i, rela->name, base->name);
@@ -275,9 +275,6 @@ int arch_livepatch_perform(struct livepatch_elf *elf,
             dprintk(XENLOG_ERR, LIVEPATCH "%s: Unhandled relocation #%x\n",
                     elf->name, type);
             break;
-
-        default:
-            break;
         }
 
         if ( rc )
diff --git a/xen/arch/arm/arm64/livepatch.c b/xen/arch/arm/arm64/livepatch.c
index dae64f5..63929b1 100644
--- a/xen/arch/arm/arm64/livepatch.c
+++ b/xen/arch/arm/arm64/livepatch.c
@@ -241,19 +241,16 @@ int arch_livepatch_perform_rela(struct livepatch_elf *elf,
                                 const struct livepatch_elf_sec *base,
                                 const struct livepatch_elf_sec *rela)
 {
-    const Elf_RelA *r;
-    unsigned int symndx, i;
-    uint64_t val;
-    void *dest;
-    bool_t overflow_check;
+    unsigned int i;
 
     for ( i = 0; i < (rela->sec->sh_size / rela->sec->sh_entsize); i++ )
     {
+        const Elf_RelA *r = rela->data + i * rela->sec->sh_entsize;
+        unsigned int symndx = ELF64_R_SYM(r->r_info);
+        void *dest = base->load_addr + r->r_offset; /* P */
+        bool overflow_check = true;
         int ovf = 0;
-
-        r = rela->data + i * rela->sec->sh_entsize;
-
-        symndx = ELF64_R_SYM(r->r_info);
+        uint64_t val;
 
         if ( symndx > elf->nsym )
         {
@@ -262,11 +259,8 @@ int arch_livepatch_perform_rela(struct livepatch_elf *elf,
             return -EINVAL;
         }
 
-        dest = base->load_addr + r->r_offset; /* P */
         val = elf->sym[symndx].sym->st_value +  r->r_addend; /* S+A */
 
-        overflow_check = true;
-
         /* ARM64 operations at minimum are always 32-bit. */
         if ( r->r_offset >= base->sec->sh_size ||
             (r->r_offset + sizeof(uint32_t)) > base->sec->sh_size )
@@ -403,6 +397,7 @@ int arch_livepatch_perform_rela(struct livepatch_elf *elf,
 
         case R_AARCH64_ADR_PREL_PG_HI21_NC:
             overflow_check = false;
+            /* Fallthrough. */
         case R_AARCH64_ADR_PREL_PG_HI21:
             ovf = reloc_insn_imm(RELOC_OP_PAGE, dest, val, 12, 21,
                                  AARCH64_INSN_IMM_ADR);
diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c
index dd50dd1..7917610 100644
--- a/xen/arch/x86/livepatch.c
+++ b/xen/arch/x86/livepatch.c
@@ -161,16 +161,14 @@ int arch_livepatch_perform_rela(struct livepatch_elf *elf,
                                 const struct livepatch_elf_sec *base,
                                 const struct livepatch_elf_sec *rela)
 {
-    const Elf_RelA *r;
-    unsigned int symndx, i;
-    uint64_t val;
-    uint8_t *dest;
+    unsigned int i;
 
     for ( i = 0; i < (rela->sec->sh_size / rela->sec->sh_entsize); i++ )
     {
-        r = rela->data + i * rela->sec->sh_entsize;
-
-        symndx = ELF64_R_SYM(r->r_info);
+        const Elf_RelA *r = rela->data + i * rela->sec->sh_entsize;
+        unsigned int symndx = ELF64_R_SYM(r->r_info);
+        uint8_t *dest = base->load_addr + r->r_offset;
+        uint64_t val;
 
         if ( symndx > elf->nsym )
         {
@@ -179,7 +177,6 @@ int arch_livepatch_perform_rela(struct livepatch_elf *elf,
             return -EINVAL;
         }
 
-        dest = base->load_addr + r->r_offset;
         val = r->r_addend + elf->sym[symndx].sym->st_value;
 
         switch ( ELF64_R_TYPE(r->r_info) )
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 2/2] xen/livepatch: Don't crash on encountering STN_UNDEF relocations

[Andrew Cooper]

A symndx of STN_UNDEF is special, and means a symbol value of 0.

There is no real symbol data for it, so avoid tripping over a NULL pointer
with "elf->sym[symndx].sym->st_value".

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
CC: Ross Lagerwall <ross.lagerwall@citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>

Functionally tested on x86, but both arm variants look to suffer from the same
issue.  Compile tested on all architectures.

TODO: Figure out how my livepatch has a STN_UNDEF relocation...
---
 xen/arch/arm/arm32/livepatch.c | 14 +++++++++++---
 xen/arch/arm/arm64/livepatch.c | 14 +++++++++++---
 xen/arch/x86/livepatch.c       | 14 +++++++++++---
 3 files changed, 33 insertions(+), 9 deletions(-)

diff --git a/xen/arch/arm/arm32/livepatch.c b/xen/arch/arm/arm32/livepatch.c
index a328179..0f7990a 100644
--- a/xen/arch/arm/arm32/livepatch.c
+++ b/xen/arch/arm/arm32/livepatch.c
@@ -254,14 +254,22 @@ int arch_livepatch_perform(struct livepatch_elf *elf,
             addend = get_addend(type, dest);
         }
 
-        if ( symndx > elf->nsym )
+        if ( symndx == STN_UNDEF )
+            val = 0;
+        else if ( symndx > elf->nsym )
         {
             dprintk(XENLOG_ERR, LIVEPATCH "%s: Relative symbol wants symbol@%u which is past end!\n",
                     elf->name, symndx);
             return -EINVAL;
         }
-
-        val = elf->sym[symndx].sym->st_value; /* S */
+        else if ( !elf->sym[symndx].sym )
+        {
+            dprintk(XENLOG_ERR, LIVEPATCH "%s: No relative symbol@%u\n",
+                    elf->name, symndx);
+            return -EINVAL;
+        }
+        else
+            val = elf->sym[symndx].sym->st_value; /* S */
 
         rc = perform_rel(type, dest, val, addend);
         switch ( rc )
diff --git a/xen/arch/arm/arm64/livepatch.c b/xen/arch/arm/arm64/livepatch.c
index 63929b1..476e238 100644
--- a/xen/arch/arm/arm64/livepatch.c
+++ b/xen/arch/arm/arm64/livepatch.c
@@ -252,14 +252,22 @@ int arch_livepatch_perform_rela(struct livepatch_elf *elf,
         int ovf = 0;
         uint64_t val;
 
-        if ( symndx > elf->nsym )
+        if ( symndx == STN_UNDEF )
+            val = 0;
+        else if ( symndx > elf->nsym )
         {
             dprintk(XENLOG_ERR, LIVEPATCH "%s: Relative relocation wants symbol@%u which is past end!\n",
                     elf->name, symndx);
             return -EINVAL;
         }
-
-        val = elf->sym[symndx].sym->st_value +  r->r_addend; /* S+A */
+        else if ( !elf->sym[symndx].sym )
+        {
+            dprintk(XENLOG_ERR, LIVEPATCH "%s: No relative symbol@%u\n",
+                    elf->name, symndx);
+            return -EINVAL;
+        }
+        else
+            val = elf->sym[symndx].sym->st_value + r->r_addend; /* S+A */
 
         /* ARM64 operations at minimum are always 32-bit. */
         if ( r->r_offset >= base->sec->sh_size ||
diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c
index 7917610..6f44128 100644
--- a/xen/arch/x86/livepatch.c
+++ b/xen/arch/x86/livepatch.c
@@ -170,14 +170,22 @@ int arch_livepatch_perform_rela(struct livepatch_elf *elf,
         uint8_t *dest = base->load_addr + r->r_offset;
         uint64_t val;
 
-        if ( symndx > elf->nsym )
+        if ( symndx == STN_UNDEF )
+            val = 0;
+        else if ( symndx > elf->nsym )
         {
             dprintk(XENLOG_ERR, LIVEPATCH "%s: Relative relocation wants symbol@%u which is past end!\n",
                     elf->name, symndx);
             return -EINVAL;
         }
-
-        val = r->r_addend + elf->sym[symndx].sym->st_value;
+        else if ( !elf->sym[symndx].sym )
+        {
+            dprintk(XENLOG_ERR, LIVEPATCH "%s: No symbol@%u\n",
+                    elf->name, symndx);
+            return -EINVAL;
+        }
+        else
+            val = r->r_addend + elf->sym[symndx].sym->st_value;
 
         switch ( ELF64_R_TYPE(r->r_info) )
         {
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 2/2] xen/livepatch: Don't crash on encountering STN_UNDEF relocations

[Andrew Cooper]

On 13/06/17 21:51, Andrew Cooper wrote:
> A symndx of STN_UNDEF is special, and means a symbol value of 0.
>
> There is no real symbol data for it, so avoid tripping over a NULL pointer
> with "elf->sym[symndx].sym->st_value".
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> CC: Ross Lagerwall <ross.lagerwall@citrix.com>
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien.grall@arm.com>
>
> Functionally tested on x86, but both arm variants look to suffer from the same
> issue.  Compile tested on all architectures.
>
> TODO: Figure out how my livepatch has a STN_UNDEF relocation...

On second thoughts, maybe STN_UNDEF symbols should be a hard failure.

(XEN) livepatch: live: Applying 1 functions
(XEN) ----[ Xen-4.10-unstable  x86_64  debug=y   Not tainted ]----
(XEN) CPU:    3
(XEN) RIP:    e008:[<0000000000000000>] 0000000000000000

In this case, the hook function hasn't been wired up correctly, which
causes apply_payload() to fall over a NULL data->load_funcs[i]();

As for why the STN_UNDEF symbol, it comes from an assembly hook function
missing a .type attribute.  Still, Xen shouldn't crash when it
encounters one.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 1/2] xen/livepatch: Clean up arch relocation handling

[Jan Beulich]

 >>> On 13.06.17 at 22:51, <andrew.cooper3@citrix.com> wrote:
> * Reduce symbol scope and initalisation as much as possible
>  * Annotate a fallthrough case in arm64
>  * Fix switch statement style in arm32
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 2/2] xen/livepatch: Don't crash on encountering STN_UNDEF relocations

[Jan Beulich]

>>> On 13.06.17 at 23:13, <andrew.cooper3@citrix.com> wrote:
> On 13/06/17 21:51, Andrew Cooper wrote:
>> A symndx of STN_UNDEF is special, and means a symbol value of 0.
>>
>> There is no real symbol data for it, so avoid tripping over a NULL pointer
>> with "elf->sym[symndx].sym->st_value".
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>> CC: Ross Lagerwall <ross.lagerwall@citrix.com>
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Stefano Stabellini <sstabellini@kernel.org>
>> CC: Julien Grall <julien.grall@arm.com>
>>
>> Functionally tested on x86, but both arm variants look to suffer from the 
> same
>> issue.  Compile tested on all architectures.
>>
>> TODO: Figure out how my livepatch has a STN_UNDEF relocation...
> 
> On second thoughts, maybe STN_UNDEF symbols should be a hard failure.

Perhaps, but I still don't see where it was coming from in the first
place.

> (XEN) livepatch: live: Applying 1 functions
> (XEN) ----[ Xen-4.10-unstable  x86_64  debug=y   Not tainted ]----
> (XEN) CPU:    3
> (XEN) RIP:    e008:[<0000000000000000>] 0000000000000000
> 
> In this case, the hook function hasn't been wired up correctly, which
> causes apply_payload() to fall over a NULL data->load_funcs[i]();
> 
> As for why the STN_UNDEF symbol, it comes from an assembly hook function
> missing a .type attribute.  Still, Xen shouldn't crash when it
> encounters one.

I don't understand - a missing .type would lead to STT_NOTYPE in
the symbol table, but not a relocation targeting STN_UNDEF.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 2/2] xen/livepatch: Don't crash on encountering STN_UNDEF relocations

[Jan Beulich]

>>> On 13.06.17 at 22:51, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/arch/x86/livepatch.c
> +++ b/xen/arch/x86/livepatch.c
> @@ -170,14 +170,22 @@ int arch_livepatch_perform_rela(struct livepatch_elf *elf,
>          uint8_t *dest = base->load_addr + r->r_offset;
>          uint64_t val;
>  
> -        if ( symndx > elf->nsym )
> +        if ( symndx == STN_UNDEF )
> +            val = 0;
> +        else if ( symndx > elf->nsym )
>          {
>              dprintk(XENLOG_ERR, LIVEPATCH "%s: Relative relocation wants symbol@%u which is past end!\n",
>                      elf->name, symndx);
>              return -EINVAL;
>          }
> -
> -        val = r->r_addend + elf->sym[symndx].sym->st_value;
> +        else if ( !elf->sym[symndx].sym )
> +        {
> +            dprintk(XENLOG_ERR, LIVEPATCH "%s: No symbol@%u\n",
> +                    elf->name, symndx);
> +            return -EINVAL;
> +        }
> +        else
> +            val = r->r_addend + elf->sym[symndx].sym->st_value;

I don't understand this: st_value for STN_UNDEF is going to be zero
(so far there's also no extension defined for the first entry, afaict),
so there should be no difference between hard-coding the zero and
reading the symbol table entry. Furthermore r_addend would still
need applying. And finally "val" is never being cast to a pointer, and
hence I miss the connection to whatever crash you've been
observing.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 2/2] xen/livepatch: Don't crash on encountering STN_UNDEF relocations

[Andrew Cooper]

On 14/06/17 11:11, Jan Beulich wrote:
>>>> On 13.06.17 at 22:51, <andrew.cooper3@citrix.com> wrote:
>> --- a/xen/arch/x86/livepatch.c
>> +++ b/xen/arch/x86/livepatch.c
>> @@ -170,14 +170,22 @@ int arch_livepatch_perform_rela(struct livepatch_elf *elf,
>>          uint8_t *dest = base->load_addr + r->r_offset;
>>          uint64_t val;
>>  
>> -        if ( symndx > elf->nsym )
>> +        if ( symndx == STN_UNDEF )
>> +            val = 0;
>> +        else if ( symndx > elf->nsym )
>>          {
>>              dprintk(XENLOG_ERR, LIVEPATCH "%s: Relative relocation wants symbol@%u which is past end!\n",
>>                      elf->name, symndx);
>>              return -EINVAL;
>>          }
>> -
>> -        val = r->r_addend + elf->sym[symndx].sym->st_value;
>> +        else if ( !elf->sym[symndx].sym )
>> +        {
>> +            dprintk(XENLOG_ERR, LIVEPATCH "%s: No symbol@%u\n",
>> +                    elf->name, symndx);
>> +            return -EINVAL;
>> +        }
>> +        else
>> +            val = r->r_addend + elf->sym[symndx].sym->st_value;
> I don't understand this: st_value for STN_UNDEF is going to be zero
> (so far there's also no extension defined for the first entry, afaict),
> so there should be no difference between hard-coding the zero and
> reading the symbol table entry. Furthermore r_addend would still
> need applying. And finally "val" is never being cast to a pointer, and
> hence I miss the connection to whatever crash you've been
> observing.

elf->sym[0].sym is the NULL pointer.

->st_value dereferences it.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 2/2] xen/livepatch: Don't crash on encountering STN_UNDEF relocations

[Jan Beulich]

>>> On 14.06.17 at 12:13, <andrew.cooper3@citrix.com> wrote:
> On 14/06/17 11:11, Jan Beulich wrote:
>>>>> On 13.06.17 at 22:51, <andrew.cooper3@citrix.com> wrote:
>>> --- a/xen/arch/x86/livepatch.c
>>> +++ b/xen/arch/x86/livepatch.c
>>> @@ -170,14 +170,22 @@ int arch_livepatch_perform_rela(struct livepatch_elf 
> *elf,
>>>          uint8_t *dest = base->load_addr + r->r_offset;
>>>          uint64_t val;
>>>  
>>> -        if ( symndx > elf->nsym )
>>> +        if ( symndx == STN_UNDEF )
>>> +            val = 0;
>>> +        else if ( symndx > elf->nsym )
>>>          {
>>>              dprintk(XENLOG_ERR, LIVEPATCH "%s: Relative relocation wants 
> symbol@%u which is past end!\n",
>>>                      elf->name, symndx);
>>>              return -EINVAL;
>>>          }
>>> -
>>> -        val = r->r_addend + elf->sym[symndx].sym->st_value;
>>> +        else if ( !elf->sym[symndx].sym )
>>> +        {
>>> +            dprintk(XENLOG_ERR, LIVEPATCH "%s: No symbol@%u\n",
>>> +                    elf->name, symndx);
>>> +            return -EINVAL;
>>> +        }
>>> +        else
>>> +            val = r->r_addend + elf->sym[symndx].sym->st_value;
>> I don't understand this: st_value for STN_UNDEF is going to be zero
>> (so far there's also no extension defined for the first entry, afaict),
>> so there should be no difference between hard-coding the zero and
>> reading the symbol table entry. Furthermore r_addend would still
>> need applying. And finally "val" is never being cast to a pointer, and
>> hence I miss the connection to whatever crash you've been
>> observing.
> 
> elf->sym[0].sym is the NULL pointer.
> 
> ->st_value dereferences it.

Ah, but that is then what you want to change (unless we decide
to outright refuse STN_UNDEF, which still depends on why it's
there in the first place).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 1/2] xen/livepatch: Clean up arch relocation handling

[Konrad Rzeszutek Wilk]

On Tue, Jun 13, 2017 at 09:51:35PM +0100, Andrew Cooper wrote:
>  * Reduce symbol scope and initalisation as much as possible
>  * Annotate a fallthrough case in arm64
>  * Fix switch statement style in arm32
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> CC: Ross Lagerwall <ross.lagerwall@citrix.com>
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien.grall@arm.com>
> 
> The purpose of this patch is simply to make the following patch easier to
> review.
> ---
>  xen/arch/arm/arm32/livepatch.c | 27 ++++++++++++---------------
>  xen/arch/arm/arm64/livepatch.c | 19 +++++++------------
>  xen/arch/x86/livepatch.c       | 13 +++++--------
>  3 files changed, 24 insertions(+), 35 deletions(-)
> 
> diff --git a/xen/arch/arm/arm32/livepatch.c b/xen/arch/arm/arm32/livepatch.c
> index a7fd5e2..a328179 100644
> --- a/xen/arch/arm/arm32/livepatch.c
> +++ b/xen/arch/arm/arm32/livepatch.c
> @@ -224,21 +224,21 @@ int arch_livepatch_perform(struct livepatch_elf *elf,
>                             const struct livepatch_elf_sec *rela,
>                             bool use_rela)
>  {
> -    const Elf_RelA *r_a;
> -    const Elf_Rel *r;
> -    unsigned int symndx, i;
> -    uint32_t val;
> -    void *dest;
> +    unsigned int i;
>      int rc = 0;
>  
>      for ( i = 0; i < (rela->sec->sh_size / rela->sec->sh_entsize); i++ )
>      {
> +        unsigned int symndx;
> +        uint32_t val;
> +        void *dest;
>          unsigned char type;
> -        s32 addend = 0;
> +        s32 addend;
>  
>          if ( use_rela )
>          {
> -            r_a = rela->data + i * rela->sec->sh_entsize;
> +            const Elf_RelA *r_a = rela->data + i * rela->sec->sh_entsize;
> +
>              symndx = ELF32_R_SYM(r_a->r_info);
>              type = ELF32_R_TYPE(r_a->r_info);
>              dest = base->load_addr + r_a->r_offset; /* P */
> @@ -246,10 +246,12 @@ int arch_livepatch_perform(struct livepatch_elf *elf,
>          }
>          else
>          {
> -            r = rela->data + i * rela->sec->sh_entsize;
> +            const Elf_Rel *r = rela->data + i * rela->sec->sh_entsize;
> +
>              symndx = ELF32_R_SYM(r->r_info);
>              type = ELF32_R_TYPE(r->r_info);
>              dest = base->load_addr + r->r_offset; /* P */
> +            addend = get_addend(type, dest);
>          }
>  
>          if ( symndx > elf->nsym )
> @@ -259,13 +261,11 @@ int arch_livepatch_perform(struct livepatch_elf *elf,
>              return -EINVAL;
>          }
>  
> -        if ( !use_rela )
> -            addend = get_addend(type, dest);

This was added right after the symndx > elf->nsym check as
way to make sure we won't dereference the dest (b/c the symbol
may be outside the bounds).


> -
>          val = elf->sym[symndx].sym->st_value; /* S */
>  
>          rc = perform_rel(type, dest, val, addend);
> -        switch ( rc ) {
> +        switch ( rc )
> +        {
>          case -EOVERFLOW:
>              dprintk(XENLOG_ERR, LIVEPATCH "%s: Overflow in relocation %u in %s for %s!\n",
>                      elf->name, i, rela->name, base->name);
> @@ -275,9 +275,6 @@ int arch_livepatch_perform(struct livepatch_elf *elf,
>              dprintk(XENLOG_ERR, LIVEPATCH "%s: Unhandled relocation #%x\n",
>                      elf->name, type);
>              break;
> -
> -        default:
> -            break;
>          }
>  
>          if ( rc )
> diff --git a/xen/arch/arm/arm64/livepatch.c b/xen/arch/arm/arm64/livepatch.c
> index dae64f5..63929b1 100644
> --- a/xen/arch/arm/arm64/livepatch.c
> +++ b/xen/arch/arm/arm64/livepatch.c
> @@ -241,19 +241,16 @@ int arch_livepatch_perform_rela(struct livepatch_elf *elf,
>                                  const struct livepatch_elf_sec *base,
>                                  const struct livepatch_elf_sec *rela)
>  {
> -    const Elf_RelA *r;
> -    unsigned int symndx, i;
> -    uint64_t val;
> -    void *dest;
> -    bool_t overflow_check;
> +    unsigned int i;
>  
>      for ( i = 0; i < (rela->sec->sh_size / rela->sec->sh_entsize); i++ )
>      {
> +        const Elf_RelA *r = rela->data + i * rela->sec->sh_entsize;
> +        unsigned int symndx = ELF64_R_SYM(r->r_info);
> +        void *dest = base->load_addr + r->r_offset; /* P */
> +        bool overflow_check = true;
>          int ovf = 0;
> -
> -        r = rela->data + i * rela->sec->sh_entsize;
> -
> -        symndx = ELF64_R_SYM(r->r_info);
> +        uint64_t val;
>  
>          if ( symndx > elf->nsym )
>          {
> @@ -262,11 +259,8 @@ int arch_livepatch_perform_rela(struct livepatch_elf *elf,
>              return -EINVAL;
>          }
>  
> -        dest = base->load_addr + r->r_offset; /* P */
>          val = elf->sym[symndx].sym->st_value +  r->r_addend; /* S+A */
>  
> -        overflow_check = true;
> -
>          /* ARM64 operations at minimum are always 32-bit. */
>          if ( r->r_offset >= base->sec->sh_size ||
>              (r->r_offset + sizeof(uint32_t)) > base->sec->sh_size )
> @@ -403,6 +397,7 @@ int arch_livepatch_perform_rela(struct livepatch_elf *elf,
>  
>          case R_AARCH64_ADR_PREL_PG_HI21_NC:
>              overflow_check = false;
> +            /* Fallthrough. */
>          case R_AARCH64_ADR_PREL_PG_HI21:
>              ovf = reloc_insn_imm(RELOC_OP_PAGE, dest, val, 12, 21,
>                                   AARCH64_INSN_IMM_ADR);

OK, that part is fine.
> diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c
> index dd50dd1..7917610 100644
> --- a/xen/arch/x86/livepatch.c
> +++ b/xen/arch/x86/livepatch.c
> @@ -161,16 +161,14 @@ int arch_livepatch_perform_rela(struct livepatch_elf *elf,
>                                  const struct livepatch_elf_sec *base,
>                                  const struct livepatch_elf_sec *rela)
>  {
> -    const Elf_RelA *r;
> -    unsigned int symndx, i;
> -    uint64_t val;
> -    uint8_t *dest;
> +    unsigned int i;
>  
>      for ( i = 0; i < (rela->sec->sh_size / rela->sec->sh_entsize); i++ )
>      {
> -        r = rela->data + i * rela->sec->sh_entsize;
> -
> -        symndx = ELF64_R_SYM(r->r_info);
> +        const Elf_RelA *r = rela->data + i * rela->sec->sh_entsize;
> +        unsigned int symndx = ELF64_R_SYM(r->r_info);
> +        uint8_t *dest = base->load_addr + r->r_offset;
> +        uint64_t val;
>  
>          if ( symndx > elf->nsym )
>          {
> @@ -179,7 +177,6 @@ int arch_livepatch_perform_rela(struct livepatch_elf *elf,
>              return -EINVAL;
>          }
>  
> -        dest = base->load_addr + r->r_offset;
>          val = r->r_addend + elf->sym[symndx].sym->st_value;
>  
>          switch ( ELF64_R_TYPE(r->r_info) )

And this is fine too.

> -- 
> 2.1.4
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 1/2] xen/livepatch: Clean up arch relocation handling

[Jan Beulich]

>>> On 14.06.17 at 15:44, <konrad.wilk@oracle.com> wrote:
> On Tue, Jun 13, 2017 at 09:51:35PM +0100, Andrew Cooper wrote:
>> --- a/xen/arch/arm/arm32/livepatch.c
>> +++ b/xen/arch/arm/arm32/livepatch.c
>> @@ -224,21 +224,21 @@ int arch_livepatch_perform(struct livepatch_elf *elf,
>>                             const struct livepatch_elf_sec *rela,
>>                             bool use_rela)
>>  {
>> -    const Elf_RelA *r_a;
>> -    const Elf_Rel *r;
>> -    unsigned int symndx, i;
>> -    uint32_t val;
>> -    void *dest;
>> +    unsigned int i;
>>      int rc = 0;
>>  
>>      for ( i = 0; i < (rela->sec->sh_size / rela->sec->sh_entsize); i++ )
>>      {
>> +        unsigned int symndx;
>> +        uint32_t val;
>> +        void *dest;
>>          unsigned char type;
>> -        s32 addend = 0;
>> +        s32 addend;
>>  
>>          if ( use_rela )
>>          {
>> -            r_a = rela->data + i * rela->sec->sh_entsize;
>> +            const Elf_RelA *r_a = rela->data + i * rela->sec->sh_entsize;
>> +
>>              symndx = ELF32_R_SYM(r_a->r_info);
>>              type = ELF32_R_TYPE(r_a->r_info);
>>              dest = base->load_addr + r_a->r_offset; /* P */
>> @@ -246,10 +246,12 @@ int arch_livepatch_perform(struct livepatch_elf *elf,
>>          }
>>          else
>>          {
>> -            r = rela->data + i * rela->sec->sh_entsize;
>> +            const Elf_Rel *r = rela->data + i * rela->sec->sh_entsize;
>> +
>>              symndx = ELF32_R_SYM(r->r_info);
>>              type = ELF32_R_TYPE(r->r_info);
>>              dest = base->load_addr + r->r_offset; /* P */
>> +            addend = get_addend(type, dest);
>>          }
>>  
>>          if ( symndx > elf->nsym )
>> @@ -259,13 +261,11 @@ int arch_livepatch_perform(struct livepatch_elf *elf,
>>              return -EINVAL;
>>          }
>>  
>> -        if ( !use_rela )
>> -            addend = get_addend(type, dest);
> 
> This was added right after the symndx > elf->nsym check as
> way to make sure we won't dereference the dest (b/c the symbol
> may be outside the bounds).

But symndx isn't being used here.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 2/2] xen/livepatch: Don't crash on encountering STN_UNDEF relocations

[Konrad Rzeszutek Wilk]

On Wed, Jun 14, 2017 at 04:24:00AM -0600, Jan Beulich wrote:
> >>> On 14.06.17 at 12:13, <andrew.cooper3@citrix.com> wrote:
> > On 14/06/17 11:11, Jan Beulich wrote:
> >>>>> On 13.06.17 at 22:51, <andrew.cooper3@citrix.com> wrote:
> >>> --- a/xen/arch/x86/livepatch.c
> >>> +++ b/xen/arch/x86/livepatch.c
> >>> @@ -170,14 +170,22 @@ int arch_livepatch_perform_rela(struct livepatch_elf 
> > *elf,
> >>>          uint8_t *dest = base->load_addr + r->r_offset;
> >>>          uint64_t val;
> >>>  
> >>> -        if ( symndx > elf->nsym )
> >>> +        if ( symndx == STN_UNDEF )
> >>> +            val = 0;
> >>> +        else if ( symndx > elf->nsym )
> >>>          {
> >>>              dprintk(XENLOG_ERR, LIVEPATCH "%s: Relative relocation wants 
> > symbol@%u which is past end!\n",
> >>>                      elf->name, symndx);
> >>>              return -EINVAL;
> >>>          }
> >>> -
> >>> -        val = r->r_addend + elf->sym[symndx].sym->st_value;
> >>> +        else if ( !elf->sym[symndx].sym )
> >>> +        {
> >>> +            dprintk(XENLOG_ERR, LIVEPATCH "%s: No symbol@%u\n",
> >>> +                    elf->name, symndx);
> >>> +            return -EINVAL;
> >>> +        }
> >>> +        else
> >>> +            val = r->r_addend + elf->sym[symndx].sym->st_value;
> >> I don't understand this: st_value for STN_UNDEF is going to be zero
> >> (so far there's also no extension defined for the first entry, afaict),
> >> so there should be no difference between hard-coding the zero and
> >> reading the symbol table entry. Furthermore r_addend would still
> >> need applying. And finally "val" is never being cast to a pointer, and
> >> hence I miss the connection to whatever crash you've been
> >> observing.
> > 
> > elf->sym[0].sym is the NULL pointer.
> > 
> > ->st_value dereferences it.
> 
> Ah, but that is then what you want to change (unless we decide
> to outright refuse STN_UNDEF, which still depends on why it's
> there in the first place).

That the !elf->sym[0].sym is very valid case.
And in that context the 'val=r->r_addend' makes sense.

And from an EFI spec, the relocations can point to the SHN_UNDEF area (why
would it I have no clue) - but naturally we can't mess with that.

But I am curious as Jan about this - and whether this is something that
could be constructed with a test-case?

Thanks.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 1/2] xen/livepatch: Clean up arch relocation handling

[Andrew Cooper]

On 14/06/17 15:02, Jan Beulich wrote:
>>>> On 14.06.17 at 15:44, <konrad.wilk@oracle.com> wrote:
>> On Tue, Jun 13, 2017 at 09:51:35PM +0100, Andrew Cooper wrote:
>>> --- a/xen/arch/arm/arm32/livepatch.c
>>> +++ b/xen/arch/arm/arm32/livepatch.c
>>> @@ -224,21 +224,21 @@ int arch_livepatch_perform(struct livepatch_elf *elf,
>>>                             const struct livepatch_elf_sec *rela,
>>>                             bool use_rela)
>>>  {
>>> -    const Elf_RelA *r_a;
>>> -    const Elf_Rel *r;
>>> -    unsigned int symndx, i;
>>> -    uint32_t val;
>>> -    void *dest;
>>> +    unsigned int i;
>>>      int rc = 0;
>>>  
>>>      for ( i = 0; i < (rela->sec->sh_size / rela->sec->sh_entsize); i++ )
>>>      {
>>> +        unsigned int symndx;
>>> +        uint32_t val;
>>> +        void *dest;
>>>          unsigned char type;
>>> -        s32 addend = 0;
>>> +        s32 addend;
>>>  
>>>          if ( use_rela )
>>>          {
>>> -            r_a = rela->data + i * rela->sec->sh_entsize;
>>> +            const Elf_RelA *r_a = rela->data + i * rela->sec->sh_entsize;
>>> +
>>>              symndx = ELF32_R_SYM(r_a->r_info);
>>>              type = ELF32_R_TYPE(r_a->r_info);
>>>              dest = base->load_addr + r_a->r_offset; /* P */
>>> @@ -246,10 +246,12 @@ int arch_livepatch_perform(struct livepatch_elf *elf,
>>>          }
>>>          else
>>>          {
>>> -            r = rela->data + i * rela->sec->sh_entsize;
>>> +            const Elf_Rel *r = rela->data + i * rela->sec->sh_entsize;
>>> +
>>>              symndx = ELF32_R_SYM(r->r_info);
>>>              type = ELF32_R_TYPE(r->r_info);
>>>              dest = base->load_addr + r->r_offset; /* P */
>>> +            addend = get_addend(type, dest);
>>>          }
>>>  
>>>          if ( symndx > elf->nsym )
>>> @@ -259,13 +261,11 @@ int arch_livepatch_perform(struct livepatch_elf *elf,
>>>              return -EINVAL;
>>>          }
>>>  
>>> -        if ( !use_rela )
>>> -            addend = get_addend(type, dest);
>> This was added right after the symndx > elf->nsym check as
>> way to make sure we won't dereference the dest (b/c the symbol
>> may be outside the bounds).
> But symndx isn't being used here.

Indeed.  r->r_offset (and therefore dest) has no direct bearing on symndx.

Having said that, there is no sanity check that r->r_offset is within
base->load_addr + sec->sh_size in arm32, whereas both arm64 and x86
appear to do this check.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 2/2] xen/livepatch: Don't crash on encountering STN_UNDEF relocations

[Andrew Cooper]

On 14/06/17 15:18, Konrad Rzeszutek Wilk wrote:
> On Wed, Jun 14, 2017 at 04:24:00AM -0600, Jan Beulich wrote:
>>>>> On 14.06.17 at 12:13, <andrew.cooper3@citrix.com> wrote:
>>> On 14/06/17 11:11, Jan Beulich wrote:
>>>>>>> On 13.06.17 at 22:51, <andrew.cooper3@citrix.com> wrote:
>>>>> --- a/xen/arch/x86/livepatch.c
>>>>> +++ b/xen/arch/x86/livepatch.c
>>>>> @@ -170,14 +170,22 @@ int arch_livepatch_perform_rela(struct livepatch_elf 
>>> *elf,
>>>>>          uint8_t *dest = base->load_addr + r->r_offset;
>>>>>          uint64_t val;
>>>>>  
>>>>> -        if ( symndx > elf->nsym )
>>>>> +        if ( symndx == STN_UNDEF )
>>>>> +            val = 0;
>>>>> +        else if ( symndx > elf->nsym )
>>>>>          {
>>>>>              dprintk(XENLOG_ERR, LIVEPATCH "%s: Relative relocation wants 
>>> symbol@%u which is past end!\n",
>>>>>                      elf->name, symndx);
>>>>>              return -EINVAL;
>>>>>          }
>>>>> -
>>>>> -        val = r->r_addend + elf->sym[symndx].sym->st_value;
>>>>> +        else if ( !elf->sym[symndx].sym )
>>>>> +        {
>>>>> +            dprintk(XENLOG_ERR, LIVEPATCH "%s: No symbol@%u\n",
>>>>> +                    elf->name, symndx);
>>>>> +            return -EINVAL;
>>>>> +        }
>>>>> +        else
>>>>> +            val = r->r_addend + elf->sym[symndx].sym->st_value;
>>>> I don't understand this: st_value for STN_UNDEF is going to be zero
>>>> (so far there's also no extension defined for the first entry, afaict),
>>>> so there should be no difference between hard-coding the zero and
>>>> reading the symbol table entry. Furthermore r_addend would still
>>>> need applying. And finally "val" is never being cast to a pointer, and
>>>> hence I miss the connection to whatever crash you've been
>>>> observing.
>>> elf->sym[0].sym is the NULL pointer.
>>>
>>> ->st_value dereferences it.
>> Ah, but that is then what you want to change (unless we decide
>> to outright refuse STN_UNDEF, which still depends on why it's
>> there in the first place).
> That the !elf->sym[0].sym is very valid case.
> And in that context the 'val=r->r_addend' makes sense.
>
> And from an EFI spec, the relocations can point to the SHN_UNDEF area (why
> would it I have no clue) - but naturally we can't mess with that.
>
> But I am curious as Jan about this - and whether this is something that
> could be constructed with a test-case?

Well - I've got a livepatch with such a relocation.  It is probably a
livepatch build tools issue, but the question is whether Xen should ever
accept such a livepatch or not (irrespective of whether this exact
relocation is permitted within the ELF spec).

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 2/2] xen/livepatch: Don't crash on encountering STN_UNDEF relocations

[Jan Beulich]

>>> Andrew Cooper <andrew.cooper3@citrix.com> 06/14/17 8:34 PM >>>
>Well - I've got a livepatch with such a relocation.  It is probably a
>livepatch build tools issue, but the question is whether Xen should ever
>accept such a livepatch or not (irrespective of whether this exact
>relocation is permitted within the ELF spec).

Since the spec explicitly mentions that case, I think we'd better support it.
But it wouldn't be the end of the world if we didn't, as presumably there
aren't that many use cases for it.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 2/2] xen/livepatch: Don't crash on encountering STN_UNDEF relocations

[Konrad Rzeszutek Wilk]

On Wed, Jun 14, 2017 at 07:33:57PM +0100, Andrew Cooper wrote:
> On 14/06/17 15:18, Konrad Rzeszutek Wilk wrote:
> > On Wed, Jun 14, 2017 at 04:24:00AM -0600, Jan Beulich wrote:
> >>>>> On 14.06.17 at 12:13, <andrew.cooper3@citrix.com> wrote:
> >>> On 14/06/17 11:11, Jan Beulich wrote:
> >>>>>>> On 13.06.17 at 22:51, <andrew.cooper3@citrix.com> wrote:
> >>>>> --- a/xen/arch/x86/livepatch.c
> >>>>> +++ b/xen/arch/x86/livepatch.c
> >>>>> @@ -170,14 +170,22 @@ int arch_livepatch_perform_rela(struct livepatch_elf 
> >>> *elf,
> >>>>>          uint8_t *dest = base->load_addr + r->r_offset;
> >>>>>          uint64_t val;
> >>>>>  
> >>>>> -        if ( symndx > elf->nsym )
> >>>>> +        if ( symndx == STN_UNDEF )
> >>>>> +            val = 0;
> >>>>> +        else if ( symndx > elf->nsym )
> >>>>>          {
> >>>>>              dprintk(XENLOG_ERR, LIVEPATCH "%s: Relative relocation wants 
> >>> symbol@%u which is past end!\n",
> >>>>>                      elf->name, symndx);
> >>>>>              return -EINVAL;
> >>>>>          }
> >>>>> -
> >>>>> -        val = r->r_addend + elf->sym[symndx].sym->st_value;
> >>>>> +        else if ( !elf->sym[symndx].sym )
> >>>>> +        {
> >>>>> +            dprintk(XENLOG_ERR, LIVEPATCH "%s: No symbol@%u\n",
> >>>>> +                    elf->name, symndx);
> >>>>> +            return -EINVAL;
> >>>>> +        }
> >>>>> +        else
> >>>>> +            val = r->r_addend + elf->sym[symndx].sym->st_value;
> >>>> I don't understand this: st_value for STN_UNDEF is going to be zero
> >>>> (so far there's also no extension defined for the first entry, afaict),
> >>>> so there should be no difference between hard-coding the zero and
> >>>> reading the symbol table entry. Furthermore r_addend would still
> >>>> need applying. And finally "val" is never being cast to a pointer, and
> >>>> hence I miss the connection to whatever crash you've been
> >>>> observing.
> >>> elf->sym[0].sym is the NULL pointer.
> >>>
> >>> ->st_value dereferences it.
> >> Ah, but that is then what you want to change (unless we decide
> >> to outright refuse STN_UNDEF, which still depends on why it's
> >> there in the first place).
> > That the !elf->sym[0].sym is very valid case.
> > And in that context the 'val=r->r_addend' makes sense.
> >
> > And from an EFI spec, the relocations can point to the SHN_UNDEF area (why
> > would it I have no clue) - but naturally we can't mess with that.
> >
> > But I am curious as Jan about this - and whether this is something that
> > could be constructed with a test-case?
> 
> Well - I've got a livepatch with such a relocation.  It is probably a
> livepatch build tools issue, but the question is whether Xen should ever
> accept such a livepatch or not (irrespective of whether this exact
> relocation is permitted within the ELF spec).

CC-ing Jamie

I would say no, as I can't find a good use-case for a relocation 
to point to the SHN_UNDEF symbol [0]. It feels to me as if somebody
would be mucking with a NULL pointer.

But perhaps if the addendum had a value it would make sense?

As in NULL + <offset>?


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 1/2] xen/livepatch: Clean up arch relocation handling

[Konrad Rzeszutek Wilk]

On Wed, Jun 14, 2017 at 07:28:39PM +0100, Andrew Cooper wrote:
> On 14/06/17 15:02, Jan Beulich wrote:
> >>>> On 14.06.17 at 15:44, <konrad.wilk@oracle.com> wrote:
> >> On Tue, Jun 13, 2017 at 09:51:35PM +0100, Andrew Cooper wrote:
> >>> --- a/xen/arch/arm/arm32/livepatch.c
> >>> +++ b/xen/arch/arm/arm32/livepatch.c
> >>> @@ -224,21 +224,21 @@ int arch_livepatch_perform(struct livepatch_elf *elf,
> >>>                             const struct livepatch_elf_sec *rela,
> >>>                             bool use_rela)
> >>>  {
> >>> -    const Elf_RelA *r_a;
> >>> -    const Elf_Rel *r;
> >>> -    unsigned int symndx, i;
> >>> -    uint32_t val;
> >>> -    void *dest;
> >>> +    unsigned int i;
> >>>      int rc = 0;
> >>>  
> >>>      for ( i = 0; i < (rela->sec->sh_size / rela->sec->sh_entsize); i++ )
> >>>      {
> >>> +        unsigned int symndx;
> >>> +        uint32_t val;
> >>> +        void *dest;
> >>>          unsigned char type;
> >>> -        s32 addend = 0;
> >>> +        s32 addend;
> >>>  
> >>>          if ( use_rela )
> >>>          {
> >>> -            r_a = rela->data + i * rela->sec->sh_entsize;
> >>> +            const Elf_RelA *r_a = rela->data + i * rela->sec->sh_entsize;
> >>> +
> >>>              symndx = ELF32_R_SYM(r_a->r_info);
> >>>              type = ELF32_R_TYPE(r_a->r_info);
> >>>              dest = base->load_addr + r_a->r_offset; /* P */
> >>> @@ -246,10 +246,12 @@ int arch_livepatch_perform(struct livepatch_elf *elf,
> >>>          }
> >>>          else
> >>>          {
> >>> -            r = rela->data + i * rela->sec->sh_entsize;
> >>> +            const Elf_Rel *r = rela->data + i * rela->sec->sh_entsize;
> >>> +
> >>>              symndx = ELF32_R_SYM(r->r_info);
> >>>              type = ELF32_R_TYPE(r->r_info);
> >>>              dest = base->load_addr + r->r_offset; /* P */
> >>> +            addend = get_addend(type, dest);
> >>>          }
> >>>  
> >>>          if ( symndx > elf->nsym )
> >>> @@ -259,13 +261,11 @@ int arch_livepatch_perform(struct livepatch_elf *elf,
> >>>              return -EINVAL;
> >>>          }
> >>>  
> >>> -        if ( !use_rela )
> >>> -            addend = get_addend(type, dest);
> >> This was added right after the symndx > elf->nsym check as
> >> way to make sure we won't dereference the dest (b/c the symbol
> >> may be outside the bounds).
> > But symndx isn't being used here.
> 
> Indeed.  r->r_offset (and therefore dest) has no direct bearing on symndx.

/me nods.
> 
> Having said that, there is no sanity check that r->r_offset is within
> base->load_addr + sec->sh_size in arm32, whereas both arm64 and x86
> appear to do this check.

True.

And the tricky part (it was to me at least) was that ARM32 is all
REL and not RELA so the opcode gets modified after the operation.

Which means it gets a bit complex to add a boundary check in
'get_addend' .

Hm, it would seem the best way is to add a

if ( r->r_offset >= base->sec->sh_size ||                               
    (r->r_offset + sizeof(uint32_t)) > base->sec->sh_size )             

that is common for use_rela and not.

Anyhow I can do that as I would want to check this
on real hardware :-)

For your patch (as is)
Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Are you OK committing it in?
> 
> ~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 2/2] xen/livepatch: Don't crash on encountering STN_UNDEF relocations

[Konrad Rzeszutek Wilk]

On Wed, Jun 14, 2017 at 12:49:21PM -0600, Jan Beulich wrote:
> >>> Andrew Cooper <andrew.cooper3@citrix.com> 06/14/17 8:34 PM >>>
> >Well - I've got a livepatch with such a relocation.  It is probably a
> >livepatch build tools issue, but the question is whether Xen should ever
> >accept such a livepatch or not (irrespective of whether this exact
> >relocation is permitted within the ELF spec).
> 
> Since the spec explicitly mentions that case, I think we'd better support it.
> But it wouldn't be the end of the world if we didn't, as presumably there
> aren't that many use cases for it.

OK. In that case:

Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>


Andrew,

Do you think it would be possible to generate an test-case for this
in arch/test/livepatch?

> 
> Jan
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 2/2] xen/livepatch: Don't crash on encountering STN_UNDEF relocations

[Andrew Cooper]

On 19/06/2017 19:30, Konrad Rzeszutek Wilk wrote:
> On Wed, Jun 14, 2017 at 12:49:21PM -0600, Jan Beulich wrote:
>>>>> Andrew Cooper <andrew.cooper3@citrix.com> 06/14/17 8:34 PM >>>
>>> Well - I've got a livepatch with such a relocation.  It is probably a
>>> livepatch build tools issue, but the question is whether Xen should ever
>>> accept such a livepatch or not (irrespective of whether this exact
>>> relocation is permitted within the ELF spec).
>> Since the spec explicitly mentions that case, I think we'd better support it.
>> But it wouldn't be the end of the world if we didn't, as presumably there
>> aren't that many use cases for it.
> OK. In that case:
>
> Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>
>
> Andrew,
>
> Do you think it would be possible to generate an test-case for this
> in arch/test/livepatch?

I can trivially cause this situation to occur with the current build
tools, but we are currently presuming a build tools bug to be the
underlying issue behind getting a STN_UNDEF relocation in the livepatch.

Given that a STN_UNDEF relocation (appears to) mean a NULL dereference
once the relocations are evaluated, I am not happy with supporting such
a case.

Therefore, I'm going to insist that we take a concrete decision as to
what to do in the hypervisor code, before adding a test case, and
advocate for excluding it outright rather than tolerating it in the
(certain?) knowledge that Xen will subsequently crash.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 2/2] xen/livepatch: Don't crash on encountering STN_UNDEF relocations

[Jan Beulich]

>>> On 20.06.17 at 01:05, <andrew.cooper3@citrix.com> wrote:
> On 19/06/2017 19:30, Konrad Rzeszutek Wilk wrote:
>> On Wed, Jun 14, 2017 at 12:49:21PM -0600, Jan Beulich wrote:
>>>>>> Andrew Cooper <andrew.cooper3@citrix.com> 06/14/17 8:34 PM >>>
>>>> Well - I've got a livepatch with such a relocation.  It is probably a
>>>> livepatch build tools issue, but the question is whether Xen should ever
>>>> accept such a livepatch or not (irrespective of whether this exact
>>>> relocation is permitted within the ELF spec).
>>> Since the spec explicitly mentions that case, I think we'd better support 
> it.
>>> But it wouldn't be the end of the world if we didn't, as presumably there
>>> aren't that many use cases for it.
>> OK. In that case:
>>
>> Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

I have to admit that I'm surprised by that, not only because of
what Andrew says below, but also because imo the patch would
imo need to be done somewhat differently, as outlined earlier
(making STN_UNDEF less of a special case).

>> Do you think it would be possible to generate an test-case for this
>> in arch/test/livepatch?
> 
> I can trivially cause this situation to occur with the current build
> tools, but we are currently presuming a build tools bug to be the
> underlying issue behind getting a STN_UNDEF relocation in the livepatch.
> 
> Given that a STN_UNDEF relocation (appears to) mean a NULL dereference
> once the relocations are evaluated, I am not happy with supporting such
> a case.

Well, quite clearly this can be of use only to produce constants,
not to produce pointers (unless chained ["expression"] relocations
are being used, where the result of one element in the chain is the
addend of the next one, albeit even then this would effectively be
a NOP relocation, so may be useful only when post-editing binaries
where the tool doesn't want to change [relocation] section sizes).

> Therefore, I'm going to insist that we take a concrete decision as to
> what to do in the hypervisor code, before adding a test case, and
> advocate for excluding it outright rather than tolerating it in the
> (certain?) knowledge that Xen will subsequently crash.

As per the explanation above, we can't tell whether Xen will
subsequently crash, as we don't know what it is that is being
relocated by such an relocation. While, as indicated before, I'd like
to see us support everything the standard mandates, I wouldn't
view it as a big problem to simply return -EOPNOTSUPP for this case
for the time being.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 1/2] xen/livepatch: Clean up arch relocation handling

[Jan Beulich]

>>> On 19.06.17 at 20:18, <konrad.wilk@oracle.com> wrote:
> On Wed, Jun 14, 2017 at 07:28:39PM +0100, Andrew Cooper wrote:
>> Having said that, there is no sanity check that r->r_offset is within
>> base->load_addr + sec->sh_size in arm32, whereas both arm64 and x86
>> appear to do this check.
> 
> True.
> 
> And the tricky part (it was to me at least) was that ARM32 is all
> REL and not RELA so the opcode gets modified after the operation.
> 
> Which means it gets a bit complex to add a boundary check in
> 'get_addend' .
> 
> Hm, it would seem the best way is to add a
> 
> if ( r->r_offset >= base->sec->sh_size ||                               
>     (r->r_offset + sizeof(uint32_t)) > base->sec->sh_size )             

Where's the uint32_t coming from here?

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 1/2] xen/livepatch: Clean up arch relocation handling

[Andrew Cooper]

On 20/06/2017 08:36, Jan Beulich wrote:
>>>> On 19.06.17 at 20:18, <konrad.wilk@oracle.com> wrote:
>> On Wed, Jun 14, 2017 at 07:28:39PM +0100, Andrew Cooper wrote:
>>> Having said that, there is no sanity check that r->r_offset is within
>>> base->load_addr + sec->sh_size in arm32, whereas both arm64 and x86
>>> appear to do this check.
>> True.
>>
>> And the tricky part (it was to me at least) was that ARM32 is all
>> REL and not RELA so the opcode gets modified after the operation.
>>
>> Which means it gets a bit complex to add a boundary check in
>> 'get_addend' .
>>
>> Hm, it would seem the best way is to add a
>>
>> if ( r->r_offset >= base->sec->sh_size ||                               
>>     (r->r_offset + sizeof(uint32_t)) > base->sec->sh_size )             
> Where's the uint32_t coming from here?

ARM32.  It's a range check that (void *)&disp is within r_offset, as it
(void *)&disp + sizeof(disp) -1

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 1/2] xen/livepatch: Clean up arch relocation handling

[Andrew Cooper]

On 20/06/2017 08:39, Andrew Cooper wrote:
> On 20/06/2017 08:36, Jan Beulich wrote:
>>>>> On 19.06.17 at 20:18, <konrad.wilk@oracle.com> wrote:
>>> On Wed, Jun 14, 2017 at 07:28:39PM +0100, Andrew Cooper wrote:
>>>> Having said that, there is no sanity check that r->r_offset is within
>>>> base->load_addr + sec->sh_size in arm32, whereas both arm64 and x86
>>>> appear to do this check.
>>> True.
>>>
>>> And the tricky part (it was to me at least) was that ARM32 is all
>>> REL and not RELA so the opcode gets modified after the operation.
>>>
>>> Which means it gets a bit complex to add a boundary check in
>>> 'get_addend' .
>>>
>>> Hm, it would seem the best way is to add a
>>>
>>> if ( r->r_offset >= base->sec->sh_size ||                               
>>>     (r->r_offset + sizeof(uint32_t)) > base->sec->sh_size )             
>> Where's the uint32_t coming from here?
> ARM32.  It's a range check that (void *)&disp is within r_offset, as it
> (void *)&disp + sizeof(disp) -1

Sorry - I meant "as is" rather than "as it".

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 1/2] xen/livepatch: Clean up arch relocation handling

[Jan Beulich]

>>> On 20.06.17 at 09:39, <andrew.cooper3@citrix.com> wrote:
> On 20/06/2017 08:36, Jan Beulich wrote:
>>>>> On 19.06.17 at 20:18, <konrad.wilk@oracle.com> wrote:
>>> On Wed, Jun 14, 2017 at 07:28:39PM +0100, Andrew Cooper wrote:
>>>> Having said that, there is no sanity check that r->r_offset is within
>>>> base->load_addr + sec->sh_size in arm32, whereas both arm64 and x86
>>>> appear to do this check.
>>> True.
>>>
>>> And the tricky part (it was to me at least) was that ARM32 is all
>>> REL and not RELA so the opcode gets modified after the operation.
>>>
>>> Which means it gets a bit complex to add a boundary check in
>>> 'get_addend' .
>>>
>>> Hm, it would seem the best way is to add a
>>>
>>> if ( r->r_offset >= base->sec->sh_size ||                               
>>>     (r->r_offset + sizeof(uint32_t)) > base->sec->sh_size )             
>> Where's the uint32_t coming from here?
> 
> ARM32.  It's a range check that (void *)&disp is within r_offset, as it
> (void *)&disp + sizeof(disp) -1

But not all ARM32 relocations fiddle with a 32-bit word. Granted all
that livepatch code currently supports are, but baking something like
this in makes future modifications more error prone.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 2/2] xen/livepatch: Don't crash on encountering STN_UNDEF relocations

[Konrad Rzeszutek Wilk]

On Tue, Jun 20, 2017 at 01:15:18AM -0600, Jan Beulich wrote:
> >>> On 20.06.17 at 01:05, <andrew.cooper3@citrix.com> wrote:
> > On 19/06/2017 19:30, Konrad Rzeszutek Wilk wrote:
> >> On Wed, Jun 14, 2017 at 12:49:21PM -0600, Jan Beulich wrote:
> >>>>>> Andrew Cooper <andrew.cooper3@citrix.com> 06/14/17 8:34 PM >>>
> >>>> Well - I've got a livepatch with such a relocation.  It is probably a
> >>>> livepatch build tools issue, but the question is whether Xen should ever
> >>>> accept such a livepatch or not (irrespective of whether this exact
> >>>> relocation is permitted within the ELF spec).
> >>> Since the spec explicitly mentions that case, I think we'd better support 
> > it.
> >>> But it wouldn't be the end of the world if we didn't, as presumably there
> >>> aren't that many use cases for it.
> >> OK. In that case:
> >>
> >> Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> I have to admit that I'm surprised by that, not only because of
> what Andrew says below, but also because imo the patch would
> imo need to be done somewhat differently, as outlined earlier
> (making STN_UNDEF less of a special case).

My line of thinking was - if the ELF spec is OK, then lets support it.

But then your point about just giving -EOPNOTSUPP is an excellent
way of "supporting" it - and better yet - we can give the system
admin an nice warning: "Fix your livepatch build-tool as your livepatch
is trying to dereference a NULL point which is unhealthy."

(or such).

Andrew you OK posting a patch like that?

> 
> >> Do you think it would be possible to generate an test-case for this
> >> in arch/test/livepatch?
> > 
> > I can trivially cause this situation to occur with the current build
> > tools, but we are currently presuming a build tools bug to be the
> > underlying issue behind getting a STN_UNDEF relocation in the livepatch.
> > 
> > Given that a STN_UNDEF relocation (appears to) mean a NULL dereference
> > once the relocations are evaluated, I am not happy with supporting such
> > a case.
> 
> Well, quite clearly this can be of use only to produce constants,
> not to produce pointers (unless chained ["expression"] relocations
> are being used, where the result of one element in the chain is the
> addend of the next one, albeit even then this would effectively be
> a NOP relocation, so may be useful only when post-editing binaries
> where the tool doesn't want to change [relocation] section sizes).
> 
> > Therefore, I'm going to insist that we take a concrete decision as to
> > what to do in the hypervisor code, before adding a test case, and
> > advocate for excluding it outright rather than tolerating it in the
> > (certain?) knowledge that Xen will subsequently crash.
> 
> As per the explanation above, we can't tell whether Xen will
> subsequently crash, as we don't know what it is that is being
> relocated by such an relocation. While, as indicated before, I'd like
> to see us support everything the standard mandates, I wouldn't
> view it as a big problem to simply return -EOPNOTSUPP for this case
> for the time being.
> 
> Jan
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 1/2] xen/livepatch: Clean up arch relocation handling

[Konrad Rzeszutek Wilk]

On Tue, Jun 20, 2017 at 01:56:28AM -0600, Jan Beulich wrote:
> >>> On 20.06.17 at 09:39, <andrew.cooper3@citrix.com> wrote:
> > On 20/06/2017 08:36, Jan Beulich wrote:
> >>>>> On 19.06.17 at 20:18, <konrad.wilk@oracle.com> wrote:
> >>> On Wed, Jun 14, 2017 at 07:28:39PM +0100, Andrew Cooper wrote:
> >>>> Having said that, there is no sanity check that r->r_offset is within
> >>>> base->load_addr + sec->sh_size in arm32, whereas both arm64 and x86
> >>>> appear to do this check.
> >>> True.
> >>>
> >>> And the tricky part (it was to me at least) was that ARM32 is all
> >>> REL and not RELA so the opcode gets modified after the operation.
> >>>
> >>> Which means it gets a bit complex to add a boundary check in
> >>> 'get_addend' .
> >>>
> >>> Hm, it would seem the best way is to add a
> >>>
> >>> if ( r->r_offset >= base->sec->sh_size ||                               
> >>>     (r->r_offset + sizeof(uint32_t)) > base->sec->sh_size )             
> >> Where's the uint32_t coming from here?
> > 
> > ARM32.  It's a range check that (void *)&disp is within r_offset, as it
> > (void *)&disp + sizeof(disp) -1
> 
> But not all ARM32 relocations fiddle with a 32-bit word. Granted all

Correct. However 'dest' (which is what get_addend operates on) at this point
is the combination of base->load_addr + r->r_offset.

It is more of making sure that what 'get_addend' operates on is
within the livepatch and not somewhere outside of it.

Then get_addend can get more fine grain approach to figuring out the
next part - that is based on the instruction whether the offset
there is OK or not (and boy the semantics for ARM32 ELF REL are
a complex beast).

> that livepatch code currently supports are, but baking something like
> this in makes future modifications more error prone.
> 
> Jan
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH for-4.9 v2] xen/livepatch: Don't crash on encountering STN_UNDEF relocations

[Andrew Cooper]

A symndx of STN_UNDEF is special, and means a symbol value of 0.  While
legitimate in the ELF standard, its existance in a livepatch is questionable
at best.  Until a plausible usecase presents itself, reject such a relocation
with -EOPNOTSUPP.

Additionally, perform a safety check on elf->sym[symndx].sym before
derefencing it, to avoid tripping over a NULL pointer when calculating val.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
CC: Ross Lagerwall <ross.lagerwall@citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>

v2:
 * Reject STN_UNDEF with -EOPNOTSUPP
---
 xen/arch/arm/arm32/livepatch.c | 17 +++++++++++++++--
 xen/arch/arm/arm64/livepatch.c | 17 +++++++++++++++--
 xen/arch/x86/livepatch.c       | 17 +++++++++++++++--
 3 files changed, 45 insertions(+), 6 deletions(-)

diff --git a/xen/arch/arm/arm32/livepatch.c b/xen/arch/arm/arm32/livepatch.c
index a328179..53fee91 100644
--- a/xen/arch/arm/arm32/livepatch.c
+++ b/xen/arch/arm/arm32/livepatch.c
@@ -254,14 +254,27 @@ int arch_livepatch_perform(struct livepatch_elf *elf,
             addend = get_addend(type, dest);
         }
 
+        if ( symndx == STN_UNDEF )
+        {
+            dprintk(XENLOG_ERR, LIVEPATCH "%s: Encountered STN_UNDEF\n",
+                    elf->name);
+            return -EOPNOTSUPP;
+        }
+
         if ( symndx > elf->nsym )
         {
             dprintk(XENLOG_ERR, LIVEPATCH "%s: Relative symbol wants symbol@%u which is past end!\n",
                     elf->name, symndx);
             return -EINVAL;
         }
-
-        val = elf->sym[symndx].sym->st_value; /* S */
+        else if ( !elf->sym[symndx].sym )
+        {
+            dprintk(XENLOG_ERR, LIVEPATCH "%s: No relative symbol@%u\n",
+                    elf->name, symndx);
+            return -EINVAL;
+        }
+        else
+            val = elf->sym[symndx].sym->st_value; /* S */
 
         rc = perform_rel(type, dest, val, addend);
         switch ( rc )
diff --git a/xen/arch/arm/arm64/livepatch.c b/xen/arch/arm/arm64/livepatch.c
index 63929b1..b033763 100644
--- a/xen/arch/arm/arm64/livepatch.c
+++ b/xen/arch/arm/arm64/livepatch.c
@@ -252,14 +252,27 @@ int arch_livepatch_perform_rela(struct livepatch_elf *elf,
         int ovf = 0;
         uint64_t val;
 
+        if ( symndx == STN_UNDEF )
+        {
+            dprintk(XENLOG_ERR, LIVEPATCH "%s: Encountered STN_UNDEF\n",
+                    elf->name);
+            return -EOPNOTSUPP;
+        }
+
         if ( symndx > elf->nsym )
         {
             dprintk(XENLOG_ERR, LIVEPATCH "%s: Relative relocation wants symbol@%u which is past end!\n",
                     elf->name, symndx);
             return -EINVAL;
         }
-
-        val = elf->sym[symndx].sym->st_value +  r->r_addend; /* S+A */
+        else if ( !elf->sym[symndx].sym )
+        {
+            dprintk(XENLOG_ERR, LIVEPATCH "%s: No relative symbol@%u\n",
+                    elf->name, symndx);
+            return -EINVAL;
+        }
+        else
+            val = elf->sym[symndx].sym->st_value + r->r_addend; /* S+A */
 
         /* ARM64 operations at minimum are always 32-bit. */
         if ( r->r_offset >= base->sec->sh_size ||
diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c
index 7917610..bfa576c 100644
--- a/xen/arch/x86/livepatch.c
+++ b/xen/arch/x86/livepatch.c
@@ -170,14 +170,27 @@ int arch_livepatch_perform_rela(struct livepatch_elf *elf,
         uint8_t *dest = base->load_addr + r->r_offset;
         uint64_t val;
 
+        if ( symndx == STN_UNDEF )
+        {
+            dprintk(XENLOG_ERR, LIVEPATCH "%s: Encountered STN_UNDEF\n",
+                    elf->name);
+            return -EOPNOTSUPP;
+        }
+
         if ( symndx > elf->nsym )
         {
             dprintk(XENLOG_ERR, LIVEPATCH "%s: Relative relocation wants symbol@%u which is past end!\n",
                     elf->name, symndx);
             return -EINVAL;
         }
-
-        val = r->r_addend + elf->sym[symndx].sym->st_value;
+        else if ( !elf->sym[symndx].sym )
+        {
+            dprintk(XENLOG_ERR, LIVEPATCH "%s: No symbol@%u\n",
+                    elf->name, symndx);
+            return -EINVAL;
+        }
+        else
+            val = r->r_addend + elf->sym[symndx].sym->st_value;
 
         switch ( ELF64_R_TYPE(r->r_info) )
         {
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH for-4.9 v2] xen/livepatch: Don't crash on encountering STN_UNDEF relocations

[Konrad Rzeszutek Wilk]

On Wed, Jun 21, 2017 at 07:13:36PM +0100, Andrew Cooper wrote:
> A symndx of STN_UNDEF is special, and means a symbol value of 0.  While
> legitimate in the ELF standard, its existance in a livepatch is questionable
> at best.  Until a plausible usecase presents itself, reject such a relocation
> with -EOPNOTSUPP.
> 
> Additionally, perform a safety check on elf->sym[symndx].sym before
> derefencing it, to avoid tripping over a NULL pointer when calculating val.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> [x86 right now, will do
arm32 tomorrow]

I naturally had to have "xen/livepatch: Clean up arch relocation handling"
on top of this.

> ---
> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> CC: Ross Lagerwall <ross.lagerwall@citrix.com>
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien.grall@arm.com>
> 
> v2:
>  * Reject STN_UNDEF with -EOPNOTSUPP
> ---
>  xen/arch/arm/arm32/livepatch.c | 17 +++++++++++++++--
>  xen/arch/arm/arm64/livepatch.c | 17 +++++++++++++++--
>  xen/arch/x86/livepatch.c       | 17 +++++++++++++++--
>  3 files changed, 45 insertions(+), 6 deletions(-)
> 
> diff --git a/xen/arch/arm/arm32/livepatch.c b/xen/arch/arm/arm32/livepatch.c
> index a328179..53fee91 100644
> --- a/xen/arch/arm/arm32/livepatch.c
> +++ b/xen/arch/arm/arm32/livepatch.c
> @@ -254,14 +254,27 @@ int arch_livepatch_perform(struct livepatch_elf *elf,
>              addend = get_addend(type, dest);
>          }
>  
> +        if ( symndx == STN_UNDEF )
> +        {
> +            dprintk(XENLOG_ERR, LIVEPATCH "%s: Encountered STN_UNDEF\n",
> +                    elf->name);
> +            return -EOPNOTSUPP;
> +        }
> +
>          if ( symndx > elf->nsym )
>          {
>              dprintk(XENLOG_ERR, LIVEPATCH "%s: Relative symbol wants symbol@%u which is past end!\n",
>                      elf->name, symndx);
>              return -EINVAL;
>          }
> -
> -        val = elf->sym[symndx].sym->st_value; /* S */
> +        else if ( !elf->sym[symndx].sym )
> +        {
> +            dprintk(XENLOG_ERR, LIVEPATCH "%s: No relative symbol@%u\n",
> +                    elf->name, symndx);
> +            return -EINVAL;
> +        }
> +        else
> +            val = elf->sym[symndx].sym->st_value; /* S */
>  
>          rc = perform_rel(type, dest, val, addend);
>          switch ( rc )
> diff --git a/xen/arch/arm/arm64/livepatch.c b/xen/arch/arm/arm64/livepatch.c
> index 63929b1..b033763 100644
> --- a/xen/arch/arm/arm64/livepatch.c
> +++ b/xen/arch/arm/arm64/livepatch.c
> @@ -252,14 +252,27 @@ int arch_livepatch_perform_rela(struct livepatch_elf *elf,
>          int ovf = 0;
>          uint64_t val;
>  
> +        if ( symndx == STN_UNDEF )
> +        {
> +            dprintk(XENLOG_ERR, LIVEPATCH "%s: Encountered STN_UNDEF\n",
> +                    elf->name);
> +            return -EOPNOTSUPP;
> +        }
> +
>          if ( symndx > elf->nsym )
>          {
>              dprintk(XENLOG_ERR, LIVEPATCH "%s: Relative relocation wants symbol@%u which is past end!\n",
>                      elf->name, symndx);
>              return -EINVAL;
>          }
> -
> -        val = elf->sym[symndx].sym->st_value +  r->r_addend; /* S+A */
> +        else if ( !elf->sym[symndx].sym )
> +        {
> +            dprintk(XENLOG_ERR, LIVEPATCH "%s: No relative symbol@%u\n",
> +                    elf->name, symndx);
> +            return -EINVAL;
> +        }
> +        else
> +            val = elf->sym[symndx].sym->st_value + r->r_addend; /* S+A */
>  
>          /* ARM64 operations at minimum are always 32-bit. */
>          if ( r->r_offset >= base->sec->sh_size ||
> diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c
> index 7917610..bfa576c 100644
> --- a/xen/arch/x86/livepatch.c
> +++ b/xen/arch/x86/livepatch.c
> @@ -170,14 +170,27 @@ int arch_livepatch_perform_rela(struct livepatch_elf *elf,
>          uint8_t *dest = base->load_addr + r->r_offset;
>          uint64_t val;
>  
> +        if ( symndx == STN_UNDEF )
> +        {
> +            dprintk(XENLOG_ERR, LIVEPATCH "%s: Encountered STN_UNDEF\n",
> +                    elf->name);
> +            return -EOPNOTSUPP;
> +        }
> +
>          if ( symndx > elf->nsym )
>          {
>              dprintk(XENLOG_ERR, LIVEPATCH "%s: Relative relocation wants symbol@%u which is past end!\n",
>                      elf->name, symndx);
>              return -EINVAL;
>          }
> -
> -        val = r->r_addend + elf->sym[symndx].sym->st_value;
> +        else if ( !elf->sym[symndx].sym )
> +        {
> +            dprintk(XENLOG_ERR, LIVEPATCH "%s: No symbol@%u\n",
> +                    elf->name, symndx);
> +            return -EINVAL;
> +        }
> +        else
> +            val = r->r_addend + elf->sym[symndx].sym->st_value;
>  
>          switch ( ELF64_R_TYPE(r->r_info) )
>          {
> -- 
> 2.1.4
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Is [PATCH for-4.9] Was:Re: [PATCH 1/2] xen/livepatch: Clean up arch relocation handling

[Konrad Rzeszutek Wilk]

On Tue, Jun 13, 2017 at 09:51:35PM +0100, Andrew Cooper wrote:
>  * Reduce symbol scope and initalisation as much as possible
>  * Annotate a fallthrough case in arm64
>  * Fix switch statement style in arm32
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> CC: Ross Lagerwall <ross.lagerwall@citrix.com>
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien.grall@arm.com>
> 
> The purpose of this patch is simply to make the following patch easier to
> review.

Jan Acked it and Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
and
Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> [on x86 right now]

> ---
>  xen/arch/arm/arm32/livepatch.c | 27 ++++++++++++---------------
>  xen/arch/arm/arm64/livepatch.c | 19 +++++++------------
>  xen/arch/x86/livepatch.c       | 13 +++++--------
>  3 files changed, 24 insertions(+), 35 deletions(-)
> 
> diff --git a/xen/arch/arm/arm32/livepatch.c b/xen/arch/arm/arm32/livepatch.c
> index a7fd5e2..a328179 100644
> --- a/xen/arch/arm/arm32/livepatch.c
> +++ b/xen/arch/arm/arm32/livepatch.c
> @@ -224,21 +224,21 @@ int arch_livepatch_perform(struct livepatch_elf *elf,
>                             const struct livepatch_elf_sec *rela,
>                             bool use_rela)
>  {
> -    const Elf_RelA *r_a;
> -    const Elf_Rel *r;
> -    unsigned int symndx, i;
> -    uint32_t val;
> -    void *dest;
> +    unsigned int i;
>      int rc = 0;
>  
>      for ( i = 0; i < (rela->sec->sh_size / rela->sec->sh_entsize); i++ )
>      {
> +        unsigned int symndx;
> +        uint32_t val;
> +        void *dest;
>          unsigned char type;
> -        s32 addend = 0;
> +        s32 addend;
>  
>          if ( use_rela )
>          {
> -            r_a = rela->data + i * rela->sec->sh_entsize;
> +            const Elf_RelA *r_a = rela->data + i * rela->sec->sh_entsize;
> +
>              symndx = ELF32_R_SYM(r_a->r_info);
>              type = ELF32_R_TYPE(r_a->r_info);
>              dest = base->load_addr + r_a->r_offset; /* P */
> @@ -246,10 +246,12 @@ int arch_livepatch_perform(struct livepatch_elf *elf,
>          }
>          else
>          {
> -            r = rela->data + i * rela->sec->sh_entsize;
> +            const Elf_Rel *r = rela->data + i * rela->sec->sh_entsize;
> +
>              symndx = ELF32_R_SYM(r->r_info);
>              type = ELF32_R_TYPE(r->r_info);
>              dest = base->load_addr + r->r_offset; /* P */
> +            addend = get_addend(type, dest);
>          }
>  
>          if ( symndx > elf->nsym )
> @@ -259,13 +261,11 @@ int arch_livepatch_perform(struct livepatch_elf *elf,
>              return -EINVAL;
>          }
>  
> -        if ( !use_rela )
> -            addend = get_addend(type, dest);
> -
>          val = elf->sym[symndx].sym->st_value; /* S */
>  
>          rc = perform_rel(type, dest, val, addend);
> -        switch ( rc ) {
> +        switch ( rc )
> +        {
>          case -EOVERFLOW:
>              dprintk(XENLOG_ERR, LIVEPATCH "%s: Overflow in relocation %u in %s for %s!\n",
>                      elf->name, i, rela->name, base->name);
> @@ -275,9 +275,6 @@ int arch_livepatch_perform(struct livepatch_elf *elf,
>              dprintk(XENLOG_ERR, LIVEPATCH "%s: Unhandled relocation #%x\n",
>                      elf->name, type);
>              break;
> -
> -        default:
> -            break;
>          }
>  
>          if ( rc )
> diff --git a/xen/arch/arm/arm64/livepatch.c b/xen/arch/arm/arm64/livepatch.c
> index dae64f5..63929b1 100644
> --- a/xen/arch/arm/arm64/livepatch.c
> +++ b/xen/arch/arm/arm64/livepatch.c
> @@ -241,19 +241,16 @@ int arch_livepatch_perform_rela(struct livepatch_elf *elf,
>                                  const struct livepatch_elf_sec *base,
>                                  const struct livepatch_elf_sec *rela)
>  {
> -    const Elf_RelA *r;
> -    unsigned int symndx, i;
> -    uint64_t val;
> -    void *dest;
> -    bool_t overflow_check;
> +    unsigned int i;
>  
>      for ( i = 0; i < (rela->sec->sh_size / rela->sec->sh_entsize); i++ )
>      {
> +        const Elf_RelA *r = rela->data + i * rela->sec->sh_entsize;
> +        unsigned int symndx = ELF64_R_SYM(r->r_info);
> +        void *dest = base->load_addr + r->r_offset; /* P */
> +        bool overflow_check = true;
>          int ovf = 0;
> -
> -        r = rela->data + i * rela->sec->sh_entsize;
> -
> -        symndx = ELF64_R_SYM(r->r_info);
> +        uint64_t val;
>  
>          if ( symndx > elf->nsym )
>          {
> @@ -262,11 +259,8 @@ int arch_livepatch_perform_rela(struct livepatch_elf *elf,
>              return -EINVAL;
>          }
>  
> -        dest = base->load_addr + r->r_offset; /* P */
>          val = elf->sym[symndx].sym->st_value +  r->r_addend; /* S+A */
>  
> -        overflow_check = true;
> -
>          /* ARM64 operations at minimum are always 32-bit. */
>          if ( r->r_offset >= base->sec->sh_size ||
>              (r->r_offset + sizeof(uint32_t)) > base->sec->sh_size )
> @@ -403,6 +397,7 @@ int arch_livepatch_perform_rela(struct livepatch_elf *elf,
>  
>          case R_AARCH64_ADR_PREL_PG_HI21_NC:
>              overflow_check = false;
> +            /* Fallthrough. */
>          case R_AARCH64_ADR_PREL_PG_HI21:
>              ovf = reloc_insn_imm(RELOC_OP_PAGE, dest, val, 12, 21,
>                                   AARCH64_INSN_IMM_ADR);
> diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c
> index dd50dd1..7917610 100644
> --- a/xen/arch/x86/livepatch.c
> +++ b/xen/arch/x86/livepatch.c
> @@ -161,16 +161,14 @@ int arch_livepatch_perform_rela(struct livepatch_elf *elf,
>                                  const struct livepatch_elf_sec *base,
>                                  const struct livepatch_elf_sec *rela)
>  {
> -    const Elf_RelA *r;
> -    unsigned int symndx, i;
> -    uint64_t val;
> -    uint8_t *dest;
> +    unsigned int i;
>  
>      for ( i = 0; i < (rela->sec->sh_size / rela->sec->sh_entsize); i++ )
>      {
> -        r = rela->data + i * rela->sec->sh_entsize;
> -
> -        symndx = ELF64_R_SYM(r->r_info);
> +        const Elf_RelA *r = rela->data + i * rela->sec->sh_entsize;
> +        unsigned int symndx = ELF64_R_SYM(r->r_info);
> +        uint8_t *dest = base->load_addr + r->r_offset;
> +        uint64_t val;
>  
>          if ( symndx > elf->nsym )
>          {
> @@ -179,7 +177,6 @@ int arch_livepatch_perform_rela(struct livepatch_elf *elf,
>              return -EINVAL;
>          }
>  
> -        dest = base->load_addr + r->r_offset;
>          val = r->r_addend + elf->sym[symndx].sym->st_value;
>  
>          switch ( ELF64_R_TYPE(r->r_info) )
> -- 
> 2.1.4
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH for-4.9 v2] xen/livepatch: Don't crash on encountering STN_UNDEF relocations

[Jan Beulich]

>>> On 21.06.17 at 20:13, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/arch/x86/livepatch.c
> +++ b/xen/arch/x86/livepatch.c
> @@ -170,14 +170,27 @@ int arch_livepatch_perform_rela(struct livepatch_elf *elf,
>          uint8_t *dest = base->load_addr + r->r_offset;
>          uint64_t val;
>  
> +        if ( symndx == STN_UNDEF )
> +        {
> +            dprintk(XENLOG_ERR, LIVEPATCH "%s: Encountered STN_UNDEF\n",
> +                    elf->name);
> +            return -EOPNOTSUPP;
> +        }
> +
>          if ( symndx > elf->nsym )

Would you mind fixing the off-by-one mistake here at once?

>          {
>              dprintk(XENLOG_ERR, LIVEPATCH "%s: Relative relocation wants symbol@%u which is past end!\n",
>                      elf->name, symndx);
>              return -EINVAL;
>          }
> -
> -        val = r->r_addend + elf->sym[symndx].sym->st_value;
> +        else if ( !elf->sym[symndx].sym )
> +        {

With this it may also be a good idea to have elf_get_sym() set
sym[0].sym (and sym[0].name) to NULL.

> +            dprintk(XENLOG_ERR, LIVEPATCH "%s: No symbol@%u\n",
> +                    elf->name, symndx);
> +            return -EINVAL;
> +        }
> +        else
> +            val = r->r_addend + elf->sym[symndx].sym->st_value;

In the spirit of the earlier code here I'd suggest omitting both "else".

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH for-4.9 v2] xen/livepatch: Don't crash on encountering STN_UNDEF relocations

[Ross Lagerwall]

On 06/21/2017 07:13 PM, Andrew Cooper wrote:
> A symndx of STN_UNDEF is special, and means a symbol value of 0.  While
> legitimate in the ELF standard, its existance in a livepatch is questionable
> at best.  Until a plausible usecase presents itself, reject such a relocation
> with -EOPNOTSUPP.
> 
> Additionally, perform a safety check on elf->sym[symndx].sym before
> derefencing it, to avoid tripping over a NULL pointer when calculating val.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Ross Lagerwall <ross.lagerwall@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH for-4.9 v2] xen/livepatch: Don't crash on encountering STN_UNDEF relocations

[Konrad Rzeszutek Wilk]

On Wed, Jun 21, 2017 at 09:26:15PM -0400, Konrad Rzeszutek Wilk wrote:
> On Wed, Jun 21, 2017 at 07:13:36PM +0100, Andrew Cooper wrote:
> > A symndx of STN_UNDEF is special, and means a symbol value of 0.  While
> > legitimate in the ELF standard, its existance in a livepatch is questionable
> > at best.  Until a plausible usecase presents itself, reject such a relocation
> > with -EOPNOTSUPP.
> > 
> > Additionally, perform a safety check on elf->sym[symndx].sym before
> > derefencing it, to avoid tripping over a NULL pointer when calculating val.
> > 
> > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> [x86 right now, will do
> arm32 tomorrow]

I did that on my Cubietruck and I made the rookie mistake of not trying
a hypervisor _without_ your changes, so I don't know if this crash
(see inline) is due to your patch or something else.

Also I messed up and made the livepatch test run every time it boots, so
now it is stuck in a loop of crashes :-(

The git tree is:

git://xenbits.xen.org/people/konradwilk/xen.git staging-4.9

Stay tuned.


U-Boot SPL 2015.04 (Mar 14 2016 - 12:00:28)
DRAM: 2048 MiB
CPU: 912000000Hz, AXI/AHB/APB: 3/2/2


U-Boot 2015.04 (Mar 14 2016 - 12:00:28) Allwinner Technology

CPU:   Allwinner A20 (SUN7I)
I2C:   ready
DRAM:  2 GiB
MMC:   SUNXI SD/MMC: 0
Setting up a 1024x768 vga console
In:    serial
Out:   vga
Err:   vga
SCSI:  SUNXI SCSI INIT
SATA link 0 timeout.
AHCI 0001.0100 32 slots 1 ports 3 Gbps 0x1 impl SATA mode
flags: ncq stag pm led clo only pmp pio slum part ccc apst 
Net:   dwmac.1c50000
starting USB...
USB0:   USB EHCI 1.00
scanning bus 0 for devices... 1 USB Device(s) found
USB1:   USB EHCI 1.00
scanning bus 1 for devices... 1 USB Device(s) found
       scanning usb for storage devices... 0 Storage Device(s) found
Hit any key to stop autoboot:  2 \b\b\b 1 \b\b\b 0 
switch to partitions #0, OK
mmc0 is current device
Scanning mmc 0:1...
Found U-Boot script /boot.scr
reading /boot.scr
1629 bytes read in 22 ms (72.3 KiB/s)
## Executing script at 43100000
reading /xen
884744 bytes read in 72 ms (11.7 MiB/s)
reading /sun7i-a20-cubietruck.dtb
30801 bytes read in 42 ms (715.8 KiB/s)
reading /vmlinuz
5662136 bytes read in 382 ms (14.1 MiB/s)
Kernel image @ 0xaea00000 [ 0x000000 - 0x11b700 ]
## Flattened Device Tree blob at aec00000
   Booting using the fdt blob at 0xaec00000
   reserving fdt memory region: addr=aec00000 size=8000
   Using Device Tree in place at aec00000, end aec0afff

Starting kernel ...

 Xen 4.9-rc
(XEN) Xen version 4.9-rc (konrad@dumpdata.com) (arm-linux-gnueabihf-gcc (Ubuntu/Linaro 5.4.0-6ubuntu1~16.04.4) 5.4.0 20160609) debug=y  Wed Jun 21 21:55:01 EDT 2017
(XEN) Latest ChangeSet: Wed Jun 21 19:13:36 2017 +0100 git:e199fd6
(XEN) Processor: 410fc074: "ARM Limited", variant: 0x0, part 0xc07, rev 0x4
(XEN) 32-bit Execution:
(XEN)   Processor Features: 00001131:00011011
(XEN)     Instruction Sets: AArch32 A32 Thumb Thumb-2 ThumbEE Jazelle
(XEN)     Extensions: GenericTimer Security
(XEN)   Debug Features: 02010555
(XEN)   Auxiliary Features: 00000000
(XEN)   Memory Model Features: 10101105 40000000 01240000 02102211
(XEN)  ISA Features: 02101110 13112111 21232041 11112131 10011142 00000000
(XEN) Using PSCI-0.1 for SMP bringup
(XEN) SMP: Allowing 2 CPUs
(XEN) Generic Timer IRQ: phys=30 hyp=26 virt=27 Freq: 24000 KHz
(XEN) GICv2: WARNING: The GICC size is too small: 0x1000 expected 0x2000
(XEN) GICv2 initialization:
(XEN)         gic_dist_addr=0000000001c81000
(XEN)         gic_cpu_addr=0000000001c82000
(XEN)         gic_hyp_addr=0000000001c84000
(XEN)         gic_vcpu_addr=0000000001c86000
(XEN)         gic_maintenance_irq=25
(XEN) GICv2: 160 lines, 2 cpus, secure (IID 0100143b).
(XEN) Using scheduler: SMP Credit Scheduler (credit)
(XEN) Allocated console ring of 16 KiB.
(XEN) VFP implementer 0x41 architecture 2 part 0x30 variant 0x7 rev 0x4
(XEN) Bringing up CPU1
(XEN) CPU 1 booted.
(XEN) Brought up 2 CPUs
(XEN) P2M: 40-bit IPA
(XEN) P2M: 3 levels with order-1 root, VTCR 0x80003558
(XEN) I/O virtualisation disabled
(XEN) build-id: d406e500724be7c1443df04d783419bc70fa75b9
(XEN) alternatives: Patching with alt table 100c1464 -> 100c1494
(XEN) *** LOADING DOMAIN 0 ***
(XEN) Loading kernel from boot module @ 00000000af600000
(XEN) Allocating 1:1 mappings totalling 512MB for dom0:
(XEN) BANK[0] 0x00000060000000-0x00000080000000 (512MB)
(XEN) Grant table range: 0x000000bfa00000-0x000000bfa6d000
(XEN) Loading zImage from 00000000af600000 to 0000000067a00000-0000000067f665b8
(XEN) Allocating PPI 16 for event channel interrupt
(XEN) Loading dom0 DTB to 0x0000000068000000-0x00000000680072e0
(XEN) Scrubbing Free RAM on 1 nodes using 2 CPUs
(XEN) ........done.
(XEN) Initial low memory virq threshold set at 0x4000 pages.
(XEN) Std. Loglevel: All
(XEN) Guest Loglevel: All
(XEN) *** Serial input -> DOM0 (type 'CTRL-a' three times to switch input to Xen)
(XEN) Freed 288kB init memory.
(XEN) d0v0: vGICD: unhandled word write 0xffffffff to ICACTIVER4
(XEN) d0v0: vGICD: unhandled word write 0xffffffff to ICACTIVER8
(XEN) d0v0: vGICD: unhandled word write 0xffffffff to ICACTIVER12
(XEN) d0v0: vGICD: unhandled word write 0xffffffff to ICACTIVER16
(XEN) d0v0: vGICD: unhandled word write 0xffffffff to ICACTIVER0
[    0.000000] Booting Linux on physical CPU 0x0
[    0.000000] Linux version 4.5.0-00001-geebcae0 (konrad@vm-konrad-ubuntu) (gcc version 4.7.4 (Ubuntu/Linaro 4.7.4-2ubuntu1) ) #3 SMP Wed Mar 30 11:32:26 EDT 2016
[    0.000000] CPU: ARMv7 Processor [410fc074] revision 4 (ARMv7), cr=10c5387d
[    0.000000] CPU: div instructions available: patching division code
[    0.000000] CPU: PIPT / VIPT nonaliasing data cache, VIPT aliasing instruction cache
[    0.000000] Machine model: Cubietech Cubietruck
[    0.000000] cma: Reserved 64 MiB at 0x7c000000
[    0.000000] Memory policy: Data cache writealloc
[    0.000000] psci: probing for conduit method from DT.
[    0.000000] psci: PSCIv0.2 detected in firmware.
[    0.000000] psci: Using standard PSCI v0.2 function IDs
[    0.000000] psci: Trusted OS migration not required
[    0.000000] Xen 4.9 support found
[    0.000000] PERCPU: Embedded 12 pages/cpu @dbba8000 s19200 r8192 d21760 u49152
[    0.000000] Built 1 zonelists in Zone order, mobility grouping on.  Total pages: 130048
[    0.000000] Kernel command line: console=hvc0 ro root=/dev/mmcblk0p2 rootwait clk_ignore_unused
[    0.000000] PID hash table entries: 2048 (order: 1, 8192 bytes)
[    0.000000] Dentry cache hash table entries: 65536 (order: 6, 262144 bytes)
[    0.000000] Inode-cache hash table entries: 32768 (order: 5, 131072 bytes)
[    0.000000] Memory: 441476K/524288K available (7354K kernel code, 875K rwdata, 3116K rodata, 736K init, 302K bss, 17276K reserved, 65536K cma-reserved, 0K highmem)
[    0.000000] Virtual kernel memory layout:
[    0.000000]     vector  : 0xffff0000 - 0xffff1000   (   4 kB)
[    0.000000]     fixmap  : 0xffc00000 - 0xfff00000   (3072 kB)
[    0.000000]     vmalloc : 0xe0800000 - 0xff800000   ( 496 MB)
[    0.000000]     lowmem  : 0xc0000000 - 0xe0000000   ( 512 MB)
[    0.000000]     pkmap   : 0xbfe00000 - 0xc0000000   (   2 MB)
[    0.000000]     modules : 0xbf000000 - 0xbfe00000   (  14 MB)
[    0.000000]       .text : 0xc0208000 - 0xc0c41b14   (10471 kB)
[    0.000000]       .init : 0xc0c42000 - 0xc0cfa000   ( 736 kB)
[    0.000000]       .data : 0xc0cfa000 - 0xc0dd4cc0   ( 876 kB)
[    0.000000]        .bss : 0xc0dd7000 - 0xc0e22ab0   ( 303 kB)
[    0.000000] SLUB: HWalign=64, Order=0-3, MinObjects=0, CPUs=2, Nodes=1
[    0.000000] Hierarchical RCU implementation.
[    0.000000] 	Build-time adjustment of leaf fanout to 32.
[    0.000000] 	RCU restricting CPUs from NR_CPUS=4 to nr_cpu_ids=2.
[    0.000000] RCU: Adjusting geometry for rcu_fanout_leaf=32, nr_cpu_ids=2
[    0.000000] NR_IRQS:16 nr_irqs:16 16
[    0.000000] Architected cp15 timer(s) running at 24.00MHz (virt).
[    0.000000] clocksource: arch_sys_counter: mask: 0xffffffffffffff max_cycles: 0x588fe9dc0, max_idle_ns: 440795202592 ns
[    0.000007] sched_clock: 56 bits at 24MHz, resolution 41ns, wraps every 4398046511097ns
[    0.000019] Switching to timer-based delay loop, resolution 41ns
[    0.002017] clocksource: timer: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns: 79635851949 ns
[    0.002651] clocksource: hstimer: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns: 12741736309 ns
[    0.003112] Console: colour dummy device 80x30
[    0.004304] console [hvc0] enabled
[    0.004346] Calibrating delay loop (skipped), value calculated using timer frequency.. 48.00 BogoMIPS (lpj=240000)
[    0.004399] pid_max: default: 32768 minimum: 301
[    0.004551] Mount-cache hash table entries: 1024 (order: 0, 4096 bytes)
[    0.004593] Mountpoint-cache hash table entries: 1024 (order: 0, 4096 bytes)
[    0.005260] CPU: Testing write buffer coherency: ok
[    0.005685] /cpus/cpu@0 missing clock-frequency property
[    0.005755] /cpus/cpu@1 missing clock-frequency property
[    0.005789] CPU0: thread -1, cpu 0, socket 0, mpidr 80000000
[    0.006131] xen:grant_table: Grant tables using version 1 layout
[    0.006220] Grant table initialized
[    0.006336] xen:events: Using FIFO-based ABI
[    0.006383] Xen: initializing cpu0
[    0.006579] Setting up static identity map for 0x60208280 - 0x60208318
(XEN) d0v1: vGICD: unhandled word write 0xffffffff to ICACTIVER0
[    0.010521] Xen: initializing cpu1
[    0.010610] CPU1: thread -1, cpu 1, socket 0, mpidr 80000001
[    0.010810] Brought up 2 CPUs
[    0.010895] SMP: Total of 2 processors activated (96.00 BogoMIPS).
[    0.010926] CPU: All CPU(s) started in SVC mode.
[    0.011954] devtmpfs: initialized
[    0.020353] VFP support v0.3: implementor 41 architecture 2 part 30 variant 7 rev 4
[    0.020873] clocksource: jiffies: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns: 19112604462750000 ns
[    0.023321] pinctrl core: initialized pinctrl subsystem
[    0.025390] NET: Registered protocol family 16
[    0.027567] DMA: preallocated 256 KiB pool for atomic coherent allocations
[    0.030610] xen:swiotlb_xen: Warning: only able to allocate 8 MB for software IO TLB
[    0.035217] software IO TLB [mem 0x7a000000-0x7a800000] (8MB) mapped at [da000000-da7fffff]
[    0.044299] No ATAGs?
[    0.046950] Serial: AMBA PL011 UART driver
[    0.084515] xen:balloon: Initialising balloon driver
[    0.085041] xen_balloon: Initialising balloon driver
[    0.085529] reg-fixed-voltage ahci-5v: could not find pctldev for node /soc@01c00000/pinctrl@01c20800/ahci_pwr_pin@1, deferring probe
[    0.085624] reg-fixed-voltage usb0-vbus: could not find pctldev for node /soc@01c00000/pinctrl@01c20800/usb0_vbus_pin@0, deferring probe
[    0.085701] reg-fixed-voltage usb1-vbus: could not find pctldev for node /soc@01c00000/pinctrl@01c20800/usb1_vbus_pin@0, deferring probe
[    0.085773] reg-fixed-voltage usb2-vbus: could not find pctldev for node /soc@01c00000/pinctrl@01c20800/usb2_vbus_pin@0, deferring probe
[    0.088754] vgaarb: loaded
[    0.089451] SCSI subsystem initialized
[    0.090238] usbcore: registered new interface driver usbfs
[    0.090370] usbcore: registered new interface driver hub
[    0.090596] usbcore: registered new device driver usb
[    0.091442] Linux video capture interface: v2.00
[    0.091547] pps_core: LinuxPPS API ver. 1 registered
[    0.091580] pps_core: Software ver. 5.3.6 - Copyright 2005-2007 Rodolfo Giometti <giometti@linux.it>
[    0.091642] PTP clock support registered
[    0.091717] EDAC MC: Ver: 3.0.0
[    0.092696] Advanced Linux Sound Architecture Driver Initialized.
[    0.094361] clocksource: Switched to clocksource arch_sys_counter
[    0.095823] simple-framebuffer bfd00000.framebuffer: framebuffer at 0xbfd00000, 0x300000 bytes, mapped to 0xe0900000
[    0.095933] simple-framebuffer bfd00000.framebuffer: format=x8r8g8b8, mode=1024x768x32, linelength=4096
[    0.110692] Console: switching to colour frame buffer device 128x48
[    0.124528] simple-framebuffer bfd00000.framebuffer: fb0: simplefb registered!
[    0.139939] /thermal-zones/cpu_thermal/cooling-maps/map0: could not find phandle
[    0.140010] missing cooling_device property
[    0.140034] failed to build thermal zone cpu_thermal: -22
[    0.140302] NET: Registered protocol family 2
[    0.141045] TCP established hash table entries: 4096 (order: 2, 16384 bytes)
[    0.141143] TCP bind hash table entries: 4096 (order: 3, 32768 bytes)
[    0.141237] TCP: Hash tables configured (established 4096 bind 4096)
[    0.141337] UDP hash table entries: 256 (order: 1, 8192 bytes)
[    0.141400] UDP-Lite hash table entries: 256 (order: 1, 8192 bytes)
[    0.141639] NET: Registered protocol family 1
[    0.142266] RPC: Registered named UNIX socket transport module.
[    0.142337] RPC: Registered udp transport module.
[    0.142363] RPC: Registered tcp transport module.
[    0.142388] RPC: Registered tcp NFSv4.1 backchannel transport module.
[    0.144798] futex hash table entries: 512 (order: 3, 32768 bytes)
[    0.157286] squashfs: version 4.0 (2009/01/31) Phillip Lougher
[    0.158577] NFS: Registering the id_resolver key type
[    0.158683] Key type id_resolver registered
[    0.158710] Key type id_legacy registered
[    0.160111] Block layer SCSI generic (bsg) driver version 0.4 loaded (major 249)
[    0.160181] io scheduler noop registered
[    0.160212] io scheduler deadline registered
[    0.160274] io scheduler cfq registered (default)
[    0.165688] sun7i-a20-pinctrl 1c20800.pinctrl: initialized sunXi PIO driver
[    0.178331] xen:xen_evtchn: Event-channel device installed
[    0.275929] Serial: 8250/16550 driver, 4 ports, IRQ sharing disabled
[    0.279002] msm_serial: driver initialized
[    0.279251] STMicroelectronics ASC driver initialized
[    0.280577] [drm] Initialized drm 1.1.0 20060810
[    0.292040] loop: module loaded
[    0.292131] Invalid max_queues (4), will use default max: 2.
[    0.296470] libphy: Fixed MDIO Bus: probed
[    0.296856] tun: Universal TUN/TAP device driver, 1.6
[    0.296892] tun: (C) 1999-2004 Max Krasnyansky <maxk@qualcomm.com>
[    0.299459] sun7i-dwmac 1c50000.ethernet: no regulator found
[    0.299606] sun7i-dwmac 1c50000.ethernet: no reset control found
[    0.299645]  Ring mode enabled
[    0.299668]  No HW DMA feature register supported
[    0.299692]  Normal descriptors
[    0.299717]  TX Checksum insertion supported
[    0.300398] sun7i-dwmac 1c50000.ethernet eth0: No MDIO subnode found
[    0.304732] libphy: stmmac: probed
[    0.304785] eth0: PHY ID 001cc915 at 0 IRQ POLL (stmmac-0:00) active
[    0.304818] eth0: PHY ID 001cc915 at 1 IRQ POLL (stmmac-0:01)
[    0.305708] xen_netfront: Initialising Xen virtual ethernet driver
[    0.305854] pegasus: v0.9.3 (2013/04/25), Pegasus/Pegasus II USB Ethernet driver
[    0.305971] usbcore: registered new interface driver pegasus
[    0.306096] usbcore: registered new interface driver asix
[    0.306177] usbcore: registered new interface driver ax88179_178a
[    0.306259] usbcore: registered new interface driver cdc_ether
[    0.306355] usbcore: registered new interface driver smsc75xx
[    0.306450] usbcore: registered new interface driver smsc95xx
[    0.306541] usbcore: registered new interface driver net1080
[    0.306623] usbcore: registered new interface driver cdc_subset
[    0.306705] usbcore: registered new interface driver zaurus
[    0.306852] usbcore: registered new interface driver cdc_ncm
[    0.307226] ehci_hcd: USB 2.0 'Enhanced' Host Controller (EHCI) Driver
[    0.307273] ehci-pci: EHCI PCI platform driver
[    0.307382] ehci-platform: EHCI generic platform driver
[    0.307899] ehci-omap: OMAP-EHCI Host Controller driver
[    0.308078] ehci-orion: EHCI orion driver
[    0.308241] SPEAr-ehci: EHCI SPEAr driver
[    0.308383] tegra-ehci: Tegra EHCI driver
[    0.308674] usbcore: registered new interface driver usb-storage
[    0.310393] mousedev: PS/2 mouse device common for all mice
[    0.313021] sunxi-rtc 1c20d00.rtc: rtc core: registered rtc-sunxi as rtc0
[    0.313084] sunxi-rtc 1c20d00.rtc: RTC enabled
[    0.313827] i2c /dev entries driver
[    0.317636] usbcore: registered new interface driver uvcvideo
[    0.317696] USB Video Class driver (1.1.1)
[    0.317727] gspca_main: v2.14.0 registered
[    0.319558] sunxi-wdt 1c20c90.watchdog: Watchdog enabled (timeout=16 sec, nowayout=0)
[    0.320289] device-mapper: uevent: version 1.0.3
[    0.320926] device-mapper: ioctl: 4.34.0-ioctl (2015-10-28) initialised: dm-devel@redhat.com
[    0.322218] sdhci: Secure Digital Host Controller Interface driver
[    0.322276] sdhci: Copyright(c) Pierre Ossman
[    0.324254] sunxi-mmc 1c0f000.mmc: Got CD GPIO
[    0.364640] sunxi-mmc 1c0f000.mmc: base:0xe08a6000 irq:25
[    0.365874] sunxi-mmc 1c12000.mmc: allocated mmc-pwrseq
[    0.401560] mmc0: host does not support reading read-only switch, assuming write-enable
[    0.404064] mmc0: new high speed SDHC card at address b368
[    0.404623] sunxi-mmc 1c12000.mmc: base:0xe08a8000 irq:26
[    0.404939] sdhci-pltfm: SDHCI platform and OF driver helper
[    0.405943] usbcore: registered new interface driver usbhid
[    0.405988] usbhid: USB HID core driver
[    0.411246] nf_conntrack version 0.5.0 (7922 buckets, 31688 max)
[    0.412351] ip_tables: (C) 2000-2006 Netfilter Core Team
[    0.412556] NET: Registered protocol family 17
[    0.412940] Key type dns_resolver registered
[    0.413661] ThumbEE CPU extension supported.
[    0.413728] Registering SWP/SWPB emulation handler
[    0.414252] mmcblk0: mmc0:b368       29.8 GiB 
[    0.420494]  mmcblk0: p1 p2 p3
[    0.423755] sunxi-mmc 1c12000.mmc: smc 1 err, cmd 8, RTO !!
[    0.434014] mmc1: queuing unknown CIS tuple 0x80 (2 bytes)
[    0.435648] mmc1: queuing unknown CIS tuple 0x80 (3 bytes)
[    0.437277] mmc1: queuing unknown CIS tuple 0x80 (3 bytes)
[    0.440218] mmc1: queuing unknown CIS tuple 0x80 (7 bytes)
[    0.456847] mmc1: new high speed SDIO card at address 0001
[    0.484441] ahci-sunxi 1c18000.sata: controller can't do PMP, turning off CAP_PMP
[    0.484540] ahci-sunxi 1c18000.sata: SSS flag set, parallel bus scan disabled
[    0.484605] ahci-sunxi 1c18000.sata: AHCI 0001.0100 32 slots 1 ports 3 Gbps 0x1 impl platform mode
[    0.484654] ahci-sunxi 1c18000.sata: flags: ncq sntf stag pm led clo only pio slum part ccc 
[    0.486107] scsi host0: ahci-sunxi
[    0.486525] ata1: SATA max UDMA/133 mmio [mem 0x01c18000-0x01c18fff] port 0x100 irq 31
[    0.489765] sunxi-rtc 1c20d00.rtc: setting system clock to 2017-06-22 15:20:06 UTC (1498144806)
[    0.495462] vcc3v0: disabling
[    0.495538] vcc5v0: disabling
[    0.495565] usb0-vbus: disabling
[    0.495588] usb1-vbus: disabling
[    0.495615] usb2-vbus: disabling
[    0.495653] clk: Not disabling unused clocks
[    0.495693] ALSA device list:
[    0.495714]   No soundcards found.
[    0.806376] ata1: SATA link down (SStatus 0 SControl 300)
[    0.807825] EXT4-fs (mmcblk0p2): couldn't mount as ext3 due to feature incompatibilities
[    0.808866] EXT4-fs (mmcblk0p2): couldn't mount as ext2 due to feature incompatibilities
[    0.811218] EXT4-fs (mmcblk0p2): INFO: recovery required on readonly filesystem
[    0.811276] EXT4-fs (mmcblk0p2): write access will be enabled during recovery
[    2.391356] random: nonblocking pool is initialized
[    4.905760] EXT4-fs (mmcblk0p2): recovery complete
[    5.405235] EXT4-fs (mmcblk0p2): mounted filesystem with ordered data mode. Opts: (null)
[    5.405359] VFS: Mounted root (ext4 filesystem) readonly on device 179:2.
[    5.409436] devtmpfs: mounted
[    5.410290] Freeing unused kernel memory: 736K (c0c42000 - c0cfa000)
Mount failed for selinuxfs on /sys/fs/selinux:  No such file or directory
[    5.916629] init: plymouth-upstart-bridge main process (69) terminated with status 1
[    5.916823] init: plymouth-upstart-bridge main process ended, respawning
[    5.960826] init: plymouth-upstart-bridge main process (80) terminated with status 1
[    5.961012] init: plymouth-upstart-bridge main process ended, respawning
[    5.997035] init: plymouth-upstart-bridge main process (83) terminated with status 1
[    5.997113] init: plymouth-upstart-bridge main process ended, respawning
[    6.034802] init: ureadahead main process (73) terminated with status 5
 * Stopping Send an event to indicate plymouth is up^[[74G[ OK ]
 * Starting Mount filesystems on boot^[[74G[ OK ]
 * Starting Populate /dev filesystem^[[74G[ OK ]
 * Stopping Populate /dev filesystem^[[74G[ OK ]
 * Starting Fix-up /sys/kernel/debug filesystem^[[74G[ OK ]
 * Stopping Fix-up /sys/kernel/debug filesystem^[[74G[ OK ]
 * Starting Populate and link to /run filesystem^[[74G[ OK ]
 * Stopping Populate and link to /run filesystem^[[74G[ OK ]
 * Stopping Track if upstart is running in a container^[[74G[ OK ]
 * Starting Initialize or finalize resolvconf^[[74G[ OK ]
 * Starting set console keymap^[[74G[ OK ]
 * Starting Signal sysvinit that virtual filesystems are mounted^[[74G[ OK ]
 * Starting Signal sysvinit that virtual filesystems are mounted^[[74G[ OK ]
 * Starting Bridge udev events into upstart^[[74G[ OK ]
 * Stopping set console keymap^[[74G[ OK ]
 * Starting Signal sysvinit that remote filesystems are mounted^[[74G[ OK ]
 * Starting device node and kernel event manager^[[74G[ OK ]
 * Starting load modules from /etc/modules^[[74G[ OK ]
 * Starting cold plug devices^[[74G[ OK ]
 * Starting log initial device creation^[[74G[ OK ]
 * Stopping load modules from /etc/modules^[[74G[ OK ]
 * Starting set console font^[[74G[ OK ]
 * Stopping set console font^[[74G[ OK ]
 * Starting userspace bootsplash^[[74G[ OK ]
 * Stopping userspace bootsplash^[[74G[ OK ]
 * Starting Send an event to indicate plymouth is up^[[74G[ OK ]
 * Stopping Send an event to indicate plymouth is up^[[74G[ OK ]
[    8.704502] brcmfmac: brcmf_sdio_htclk: HT Avail timeout (1000000): clkctl 0x50
 * Starting Signal sysvinit that the rootfs is mounted^[[74G[ OK ]
 * Starting configure network device security^[[74G[ OK ]
 * Stopping cold plug devices^[[74G[ OK ]
 * Starting configure network device security^[[74G[ OK ]
 * Stopping log initial device creation^[[74G[ OK ]
[    9.714497] brcmfmac: brcmf_sdio_htclk: HT Avail timeout (1000000): clkctl 0x50
 * Starting Mount network filesystems^[[74G[ OK ]
 * Starting Clean /tmp directory^[[74G[ OK ]
 * Stopping Mount network filesystems^[[74G[ OK ]
 * Stopping Clean /tmp directory^[[74G[ OK ]
 * Stopping Read required files in advance (for other mountpoints)^[[74G[ OK ]
 * Starting configure network device^[[74G[ OK ]
 * Starting Signal sysvinit that local filesystems are mounted^[[74G[ OK ]
 * Starting configure network device security^[[74G[ OK ]
 * Stopping Mount filesystems on boot^[[74G[ OK ]
 * Starting Failsafe Boot Delay^[[74G[ OK ]
 * Starting Load gator driver module and launch gator daemon^[[74G[ OK ]
 * Starting flush early job output to logs^[[74G[ OK ]
 * Starting Load gator driver module and launch gator daemon^[[74G[^[[31mfail^[[39;49m]
 * Starting Mount network filesystems^[[74G[ OK ]
 * Stopping flush early job output to logs^[[74G[ OK ]
 * Starting configure network device^[[74G[ OK ]
 * Stopping Failsafe Boot Delay^[[74G[ OK ]
 * Starting System V initialisation compatibility^[[74G[ OK ]
No sensors found!
Make sure you loaded all the kernel drivers you need.
Try sensors-detect to find out which these are.
 * Starting configure virtual network devices^[[74G[ OK ]
No sensors found!
Make sure you loaded all the kernel drivers you need.
Try sensors-detect to find out which these are.
 * Starting system logging daemon^[[74G[ OK ]
 * Stopping Mount network filesystems^[[74G[ OK ]
 * Setting sensors limits       ^[[80G \r^[[74G[ OK ]
 * Starting Bridge file events into upstart^[[74G[ OK ]
 * Starting Bridge socket events into upstart^[[74G[ OK ]
 * Setting up X socket directories...       ^[[80G \r^[[74G[ OK ]
 * Stopping System V initialisation compatibility^[[74G[ OK ]
 * Starting System V runlevel compatibility^[[74G[ OK ]
 * Starting Restore Sound Card State^[[74G[ OK ]
 * Starting save kernel messages^[[74G[ OK ]
 * Starting regular background program processing daemon^[[74G[ OK ]
 * Stopping Restore Sound Card State^[[74G[ OK ]
 * Starting OpenSSH server^[[74G[ OK ]
 * Stopping Restore Sound Card State^[[74G[ OK ]
 * Stopping save kernel messages^[[74G[ OK ]
 * Loading cpufreq kernel modules...       ^[[80G \r^[[74G[ OK ]
 ID                                     | status
----------------------------------------+------------
Failed to get status of xen_hello_world.
Error 2: No such file or directory
Uploading xen_hello_world.livepatch... (XEN) livepatch.c:413: livepatch: xen_hello_world: Loaded .note.gnu.build-id at 00a04000
(XEN) livepatch.c:413: livepatch: xen_hello_world: Loaded .text at 00a02000
(XEN) livepatch.c:413: livepatch: xen_hello_world: Loaded .rodata at 00a04024
(XEN) livepatch.c:413: livepatch: xen_hello_world: Loaded .altinstructions at 00a0404a
(XEN) livepatch.c:413: livepatch: xen_hello_world: Loaded .altinstr_replacement at 00a04058
(XEN) livepatch.c:413: livepatch: xen_hello_world: Loaded .rodata.str1.4 at 00a0405c
(XEN) livepatch.c:413: livepatch: xen_hello_world: Loaded .rodata.str at 00a040d0
(XEN) livepatch.c:413: livepatch: xen_hello_world: Loaded .bug_frames.1 at 00a040e4
(XEN) livepatch.c:413: livepatch: xen_hello_world: Loaded .livepatch.hooks.unload at 00a040f4
(XEN) livepatch.c:413: livepatch: xen_hello_world: Loaded .livepatch.hooks.load at 00a04100
(XEN) livepatch.c:413: livepatch: xen_hello_world: Loaded .livepatch.depends at 00a04108
(XEN) livepatch.c:413: livepatch: xen_hello_world: Loaded .livepatch.funcs at 00a03000
(XEN) livepatch_elf.c:330: livepatch: xen_hello_world: Symbol resolved:  => 0xa04000 (.note.gnu.build-id)
(XEN) livepatch_elf.c:330: livepatch: xen_hello_world: Symbol resolved:  => 0xa02000 (.text)
(XEN) livepatch_elf.c:330: livepatch: xen_hello_world: Symbol resolved:  => 0xa04024 (.rodata)
(XEN) livepatch_elf.c:330: livepatch: xen_hello_world: Symbol resolved:  => 0xa0404a (.altinstructions)
(XEN) livepatch_elf.c:330: livepatch: xen_hello_world: Symbol resolved:  => 0xa04058 (.altinstr_replacement)
(XEN) livepatch_elf.c:330: livepatch: xen_hello_world: Symbol resolved:  => 0xa0405c (.rodata.str1.4)
(XEN) livepatch_elf.c:330: livepatch: xen_hello_world: Symbol resolved:  => 0xa040d0 (.rodata.str)
(XEN) livepatch_elf.c:330: livepatch: xen_hello_world: Symbol resolved:  => 0xa040e4 (.bug_frames.1)
(XEN) livepatch_elf.c:330: livepatch: xen_hello_world: Symbol resolved:  => 0xa040f4 (.livepatch.hooks.unload)
(XEN) livepatch_elf.c:330: livepatch: xen_hello_world: Symbol resolved:  => 0xa04100 (.livepatch.hooks.load)
(XEN) livepatch_elf.c:330: livepatch: xen_hello_world: Symbol resolved:  => 0xa04108 (.livepatch.depends)
(XEN) livepatch_elf.c:330: livepatch: xen_hello_world: Symbol resolved:  => 0xa03000 (.livepatch.funcs)
(XEN) livepatch_elf.c:330: livepatch: xen_hello_world: Symbol resolved:  => 0xa03034 (.bss)
(XEN) livepatch_elf.c:306: livepatch: xen_hello_world: Absolute symbol: xen_hello_world_func.c => 00000000
(XEN) livepatch_elf.c:330: livepatch: xen_hello_world: Symbol resolved: $a => 0xa02000 (.text)
(XEN) livepatch_elf.c:330: livepatch: xen_hello_world: Symbol resolved: $a => 0xa04058 (.altinstr_replacement)
(XEN) livepatch_elf.c:330: livepatch: xen_hello_world: Symbol resolved: .LC0 => 0xa0405c (.rodata.str1.4)
(XEN) livepatch_elf.c:330: livepatch: xen_hello_world: Symbol resolved: $d => 0xa0405c (.rodata.str1.4)
(XEN) livepatch_elf.c:306: livepatch: xen_hello_world: Absolute symbol: xen_hello_world.c => 00000000
(XEN) livepatch_elf.c:330: livepatch: xen_hello_world: Symbol resolved: $a => 0xa02020 (.text)
(XEN) livepatch_elf.c:330: livepatch: xen_hello_world: Symbol resolved: apply_hook => 0xa02020 (.text)
(XEN) livepatch_elf.c:330: livepatch: xen_hello_world: Symbol resolved: .LC0 => 0xa04068 (.rodata.str1.4)
(XEN) livepatch_elf.c:330: livepatch: xen_hello_world: Symbol resolved: revert_hook => 0xa02048 (.text)
(XEN) livepatch_elf.c:330: livepatch: xen_hello_world: Symbol resolved: .LC1 => 0xa0407c (.rodata.str1.4)
(XEN) livepatch_elf.c:330: livepatch: xen_hello_world: Symbol resolved: hi_func => 0xa02070 (.text)
(XEN) livepatch_elf.c:330: livepatch: xen_hello_world: Symbol resolved: .LANCHOR0 => 0xa03034 (.bss)
(XEN) livepatch_elf.c:330: livepatch: xen_hello_world: Symbol resolved: .LANCHOR1 => 0xa04024 (.rodata)
(XEN) livepatch_elf.c:330: livepatch: xen_hello_world: Symbol resolved: .LC2 => 0xa04090 (.rodata.str1.4)
(XEN) livepatch_elf.c:330: livepatch: xen_hello_world: Symbol resolved: check_fnc => 0xa020b4 (.text)
(XEN) livepatch_elf.c:330: livepatch: xen_hello_world: Symbol resolved: .LC3 => 0xa040b0 (.rodata.str1.4)
(XEN) livepatch_elf.c:330: livepatch: xen_hello_world: Symbol resolved: $d => 0xa020f0 (.text)
(XEN) livepatch_elf.c:330: livepatch: xen_hello_world: Symbol resolved: .L2\x021 => 0xa040d0 (.rodata.str)
(XEN) livepatch_elf.c:330: livepatch: xen_hello_world: Symbol resolved: .L3\x021 => 0xa040e2 (.rodata.str)
(XEN) livepatch_elf.c:330: livepatch: xen_hello_world: Symbol resolved: $d => 0xa040e4 (.bug_frames.1)
(XEN) livepatch_elf.c:330: livepatch: xen_hello_world: Symbol resolved: $a => 0xa020f4 (.text)
(XEN) livepatch_elf.c:330: livepatch: xen_hello_world: Symbol resolved: $d => 0xa02108 (.text)
(XEN) livepatch_elf.c:330: livepatch: xen_hello_world: Symbol resolved: $d => 0xa04024 (.rodata)
(XEN) livepatch_elf.c:330: livepatch: xen_hello_world: Symbol resolved: __func__.8535 => 0xa04024 (.rodata)
(XEN) livepatch_elf.c:330: livepatch: xen_hello_world: Symbol resolved: __func__.8539 => 0xa0402c (.rodata)
(XEN) livepatch_elf.c:330: livepatch: xen_hello_world: Symbol resolved: hello_world_patch_this_fnc => 0xa04038 (.rodata)
(XEN) livepatch_elf.c:330: livepatch: xen_hello_world: Symbol resolved: $d => 0xa040f4 (.livepatch.hooks.unload)
(XEN) livepatch_elf.c:330: livepatch: xen_hello_world: Symbol resolved: $d => 0xa04100 (.livepatch.hooks.load)
(XEN) livepatch_elf.c:330: livepatch: xen_hello_world: Symbol resolved: $d => 0xa03000 (.livepatch.funcs)
(XEN) livepatch_elf.c:330: livepatch: xen_hello_world: Symbol resolved: $d => 0xa04068 (.rodata.str1.4)
(XEN) livepatch_elf.c:330: livepatch: xen_hello_world: Symbol resolved: $d => 0xa03034 (.bss)
(XEN) livepatch_elf.c:330: livepatch: xen_hello_world: Symbol resolved: cnt => 0xa03034 (.bss)
(XEN) livepatch_elf.c:330: livepatch: xen_hello_world: Symbol resolved: $d => 0xa0404a (.altinstructions)
(XEN) livepatch_elf.c:330: livepatch: xen_hello_world: Symbol resolved: $d => 0xa040d0 (.rodata.str)
(XEN) livepatch_elf.c:330: livepatch: xen_hello_world: Symbol resolved: livepatch_load_data_hi_func => 0xa04100 (.livepatch.hooks.load)
(XEN) livepatch_elf.c:330: livepatch: xen_hello_world: Symbol resolved: xen_hello_world => 0xa02000 (.text)
(XEN) livepatch_elf.c:301: livepatch: xen_hello_world: Undefined symbol resolved: xen_extra_version => 0x240040
(XEN) livepatch_elf.c:330: livepatch: xen_hello_world: Symbol resolved: livepatch_unload_data_check_fnc => 0xa040f4 (.livepatch.hooks.unload)
(XEN) livepatch_elf.c:330: livepatch: xen_hello_world: Symbol resolved: livepatch_load_data_apply_hook => 0xa04104 (.livepatch.hooks.load)
(XEN) livepatch_elf.c:330: livepatch: xen_hello_world: Symbol resolved: livepatch_unload_data_revert_hook => 0xa040fc (.livepatch.hooks.unload)
(XEN) livepatch_elf.c:301: livepatch: xen_hello_world: Undefined symbol resolved: printk => 0x24f250
(XEN) livepatch_elf.c:330: livepatch: xen_hello_world: Symbol resolved: livepatch_xen_hello_world => 0xa03000 (.livepatch.funcs)
(XEN) livepatch_elf.c:330: livepatch: xen_hello_world: Symbol resolved: livepatch_unload_data_hi_func => 0xa040f8 (.livepatch.hooks.unload)
(XEN) CPU1: Unexpected Trap: Data Abort
(XEN) ----[ Xen-4.9-rc  arm32  debug=y   Not tainted ]----
(XEN) CPU:    1
(XEN) PC:     0026b768 arch_livepatch_perform+0x160/0x43c
(XEN) CPSR:   8000001a MODE:Hypervisor
(XEN)      R0: 43fcfd04 R1: 40020808 R2: 40020818 R3: 00000000
(XEN)      R4: 00000000 R5: 00000002 R6: 00a0404a R7: 00a0404a
(XEN)      R8: 00000000 R9: fff0f000 R10:01ffffff R11:43fcfc9c R12:00000003
(XEN) HYP: SP: 43fcfc64 LR: 00000001
(XEN) 
(XEN)   VTCR_EL2: 80003558
(XEN)  VTTBR_EL2: 00010000bfb1c000
(XEN) 
(XEN)  SCTLR_EL2: 30cd187f
(XEN)    HCR_EL2: 000000000038663f
(XEN)  TTBR0_EL2: 00000000bdfea000
(XEN) 
(XEN)    ESR_EL2: 94000021
(XEN)  HPFAR_EL2: 000000000001c810
(XEN)      HDFAR: 00a0404a
(XEN)      HIFAR: 38c40080
(XEN) 
(XEN) Xen stack trace from sp=43fcfc64:
(XEN)    94000021 43fcfc7c 0024f27c 43fcfcac 43fcfc84 43fcfccc 00000006 43fcfd04
(XEN)    00000008 00000002 00a03000 1001c000 00000000 43fcfca4 0026ba58 43fcfccc
(XEN)    0021be04 43fcfccc 00000240 40020690 00000025 00000002 00000240 40020690
(XEN)    00000025 43fcfdcc 0021a78c 1001dc07 00a03000 0028b97c 0028b62c 00a04000
(XEN)    00000014 00a02000 40020c48 43fcfd88 0025ad70 43fcfd0c 00259438 00000002
(XEN)    40020730 0000226c 1001c000 400207b8 40020a10 00000046 400209e8 400209f8
(XEN)    00000023 5f6e6578 6c6c6568 6f775f6f 00646c72 00000000 600f001a 0031a384
(XEN)    00000002 43fcfdbc 00000000 00000000 00000000 0000000a 00000000 43fcfd74
(XEN)    0025fcb4 4003d000 00078baf 00000f7d 00400000 00000000 00000088 00000088
(XEN)    00000088 f8baf004 b6f8c004 027175e0 43fcfe40 43fcfdcc 0025b7c0 0031a580
(XEN)    43fcfec8 00000000 0031b614 43fcff58 0029624c 0031a580 002e1f00 0031a580
(XEN)    00000000 43fcfeec 0023b9d4 43fcfe54 00218b08 00000000 b6f8c004 43fcfe18
(XEN)    00000000 43fcfe08 00000002 f6bc7600 00000000 00000000 00000000 43fcfe14
(XEN)    0025fcb4 4003d000 0007bbb6 0007ba00 00262c1c 6000009a 00000030 00000030
(XEN)    00000030 000002c8 fbbb62c8 43fcfec8 00000002 43fcfebc 00218b08 0000001b
(XEN)    0000000f 00000000 00000000 b6f90004 00000000 00000010 00000000 0000226c
(XEN)    00000000 b6f8d004 00000000 00000000 00000000 00000000 b6fab210 00000000
(XEN)    00013064 00008660 b6fab210 00000000 00013014 00008660 00000000 0000226c
(XEN)    000130f4 00000018 b6f9b9cf 00000000 00000001 00000001 00000000 00000018
(XEN)    b6f615ac 4a000ea1 0023b030 43fcff58 00000000 bec869d0 db56e3c0 d9b7e000
(XEN) Xen call trace:
(XEN)    [<0026b768>] arch_livepatch_perform+0x160/0x43c (PC)
(XEN)    [<00000001>] 00000001 (LR)
(XEN)    [<0026ba58>] arch_livepatch_perform_rel+0x14/0x24
(XEN)    [<0021be04>] livepatch_elf_perform_relocs+0x13c/0x174
(XEN)    [<0021a78c>] livepatch_op+0x450/0x162c
(XEN)    [<0023b9d4>] do_sysctl+0x9a4/0xaa0
(XEN)    [<00267034>] do_trap_guest_sync+0x1234/0x1770
(XEN)    [<0026b200>] entry.o#return_from_trap+0/0x4
(XEN) 
(XEN) 
(XEN) ****************************************
(XEN) Panic on CPU 1:
(XEN) CPU1: Unexpected Trap: Data Abort
(XEN) 
(XEN) ****************************************
(XEN) 
(XEN) Reboot in five seconds...


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH for-4.9 v2] xen/livepatch: Don't crash on encountering STN_UNDEF relocations

[Konrad Rzeszutek Wilk]

On Thu, Jun 22, 2017 at 11:27:50AM -0400, Konrad Rzeszutek Wilk wrote:
> On Wed, Jun 21, 2017 at 09:26:15PM -0400, Konrad Rzeszutek Wilk wrote:
> > On Wed, Jun 21, 2017 at 07:13:36PM +0100, Andrew Cooper wrote:
> > > A symndx of STN_UNDEF is special, and means a symbol value of 0.  While
> > > legitimate in the ELF standard, its existance in a livepatch is questionable
> > > at best.  Until a plausible usecase presents itself, reject such a relocation
> > > with -EOPNOTSUPP.
> > > 
> > > Additionally, perform a safety check on elf->sym[symndx].sym before
> > > derefencing it, to avoid tripping over a NULL pointer when calculating val.
> > > 
> > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> > 
> > Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> [x86 right now, will do
> > arm32 tomorrow]
> 
> I did that on my Cubietruck and I made the rookie mistake of not trying
> a hypervisor _without_ your changes, so I don't know if this crash
> (see inline) is due to your patch or something else.
> 
> Also I messed up and made the livepatch test run every time it boots, so
> now it is stuck in a loop of crashes :-(
> 
> The git tree is:
> 
> git://xenbits.xen.org/people/konradwilk/xen.git staging-4.9
> 
> Stay tuned.

And I see the same thing with b38b147 (that is the top of 'origin/staging').

So time to dig in.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH for-4.9 v2] xen/livepatch: Don't crash on encountering STN_UNDEF relocations

[Konrad Rzeszutek Wilk]

On Thu, Jun 22, 2017 at 12:10:46PM -0400, Konrad Rzeszutek Wilk wrote:
> On Thu, Jun 22, 2017 at 11:27:50AM -0400, Konrad Rzeszutek Wilk wrote:
> > On Wed, Jun 21, 2017 at 09:26:15PM -0400, Konrad Rzeszutek Wilk wrote:
> > > On Wed, Jun 21, 2017 at 07:13:36PM +0100, Andrew Cooper wrote:
> > > > A symndx of STN_UNDEF is special, and means a symbol value of 0.  While
> > > > legitimate in the ELF standard, its existance in a livepatch is questionable
> > > > at best.  Until a plausible usecase presents itself, reject such a relocation
> > > > with -EOPNOTSUPP.
> > > > 
> > > > Additionally, perform a safety check on elf->sym[symndx].sym before
> > > > derefencing it, to avoid tripping over a NULL pointer when calculating val.
> > > > 
> > > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> > > 
> > > Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > > Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> [x86 right now, will do
> > > arm32 tomorrow]
> > 
> > I did that on my Cubietruck and I made the rookie mistake of not trying
> > a hypervisor _without_ your changes, so I don't know if this crash
> > (see inline) is due to your patch or something else.
> > 
> > Also I messed up and made the livepatch test run every time it boots, so
> > now it is stuck in a loop of crashes :-(
> > 
> > The git tree is:
> > 
> > git://xenbits.xen.org/people/konradwilk/xen.git staging-4.9
> > 
> > Stay tuned.
> 
> And I see the same thing with b38b147 (that is the top of 'origin/staging').
> 
> So time to dig in.

/me blushes.

I compiled the hypervisor and the livepatches on a cross compiler.
arm-linux-gnueabi-gcc (Ubuntu/Linaro 5.4.0-6ubuntu1~16.04.4) 5.4.0 20160609


But if I compile both on the Cubietruck (natively) it all works nicely.
gcc (Ubuntu/Linaro 4.8.2-19ubuntu1) 4.8.2

So:

Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> [x86, arm32]

for both of the patches. Sorry for the alarm.

Julien, would you be OK with these two going in 4.9? Please?

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH for-4.9 v2] xen/livepatch: Don't crash on encountering STN_UNDEF relocations

[Konrad Rzeszutek Wilk]

On Thu, Jun 22, 2017 at 12:33:57PM -0400, Konrad Rzeszutek Wilk wrote:
> On Thu, Jun 22, 2017 at 12:10:46PM -0400, Konrad Rzeszutek Wilk wrote:
> > On Thu, Jun 22, 2017 at 11:27:50AM -0400, Konrad Rzeszutek Wilk wrote:
> > > On Wed, Jun 21, 2017 at 09:26:15PM -0400, Konrad Rzeszutek Wilk wrote:
> > > > On Wed, Jun 21, 2017 at 07:13:36PM +0100, Andrew Cooper wrote:
> > > > > A symndx of STN_UNDEF is special, and means a symbol value of 0.  While
> > > > > legitimate in the ELF standard, its existance in a livepatch is questionable
> > > > > at best.  Until a plausible usecase presents itself, reject such a relocation
> > > > > with -EOPNOTSUPP.
> > > > > 
> > > > > Additionally, perform a safety check on elf->sym[symndx].sym before
> > > > > derefencing it, to avoid tripping over a NULL pointer when calculating val.
> > > > > 
> > > > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> > > > 
> > > > Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > > > Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> [x86 right now, will do
> > > > arm32 tomorrow]
> > > 
> > > I did that on my Cubietruck and I made the rookie mistake of not trying
> > > a hypervisor _without_ your changes, so I don't know if this crash
> > > (see inline) is due to your patch or something else.
> > > 
> > > Also I messed up and made the livepatch test run every time it boots, so
> > > now it is stuck in a loop of crashes :-(
> > > 
> > > The git tree is:
> > > 
> > > git://xenbits.xen.org/people/konradwilk/xen.git staging-4.9
> > > 
> > > Stay tuned.
> > 
> > And I see the same thing with b38b147 (that is the top of 'origin/staging').
> > 
> > So time to dig in.
> 
> /me blushes.
> 
> I compiled the hypervisor and the livepatches on a cross compiler.
> arm-linux-gnueabi-gcc (Ubuntu/Linaro 5.4.0-6ubuntu1~16.04.4) 5.4.0 20160609
> 
> 
> But if I compile both on the Cubietruck (natively) it all works nicely.
> gcc (Ubuntu/Linaro 4.8.2-19ubuntu1) 4.8.2
> 
> So:
> 
> Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> [x86, arm32]
> 
> for both of the patches. Sorry for the alarm.


Jan,

Do you recall perchance this thread: http://www.mail-archive.com/xen-devel@lists.xen.org/msg80633.html

I am thinking to ressurect it but to follow the same spirit as here,
that is return -ENOTSUPPO if the sh_addralign is not the correct
value.

> 
> Julien, would you be OK with these two going in 4.9? Please?

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH for-4.9 v2] xen/livepatch: Don't crash on encountering STN_UNDEF relocations

[Jan Beulich]

>>> On 22.06.17 at 19:05, <konrad.wilk@oracle.com> wrote:
> Do you recall perchance this thread: 
> http://www.mail-archive.com/xen-devel@lists.xen.org/msg80633.html 

Vaguely.

> I am thinking to ressurect it but to follow the same spirit as here,
> that is return -ENOTSUPPO if the sh_addralign is not the correct
> value.

Let's see how that looks.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel