[PATCH for-4.9 v3 0/3] Fixes for livepatching

[PATCH for-4.9 v3 0/3] Fixes for livepatching

[Andrew Cooper]

Andrew Cooper (3):
  xen/livepatch: Clean up arch relocation handling
  xen/livepatch: Use zeroed memory allocations for arrays
  xen/livepatch: Don't crash on encountering STN_UNDEF relocations

 xen/arch/arm/arm32/livepatch.c | 41 +++++++++++++++++++++++++----------------
 xen/arch/arm/arm64/livepatch.c | 33 ++++++++++++++++++++-------------
 xen/arch/x86/livepatch.c       | 27 ++++++++++++++++++---------
 xen/common/livepatch.c         |  4 ++--
 xen/common/livepatch_elf.c     |  4 ++--
 5 files changed, 67 insertions(+), 42 deletions(-)

-- 
2.1.4


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

[PATCH for-4.9 v3 1/3] 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>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 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 for-4.9 v3 2/3] xen/livepatch: Use zeroed memory allocations for arrays

[Andrew Cooper]

Each of these arrays is sparse.  Use zeroed allocations to cause uninitialised
array elements to contain deterministic values, most importantly for the
embedded pointers.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
CC: Ross Lagerwall <ross.lagerwall@citrix.com>

* new in v3
---
 xen/common/livepatch.c     | 4 ++--
 xen/common/livepatch_elf.c | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index df67a1a..66d532d 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -771,8 +771,8 @@ static int build_symbol_table(struct payload *payload,
         }
     }
 
-    symtab = xmalloc_array(struct livepatch_symbol, nsyms);
-    strtab = xmalloc_array(char, strtab_len);
+    symtab = xzalloc_array(struct livepatch_symbol, nsyms);
+    strtab = xzalloc_array(char, strtab_len);
 
     if ( !strtab || !symtab )
     {
diff --git a/xen/common/livepatch_elf.c b/xen/common/livepatch_elf.c
index c4a9633..b69e271 100644
--- a/xen/common/livepatch_elf.c
+++ b/xen/common/livepatch_elf.c
@@ -52,7 +52,7 @@ static int elf_resolve_sections(struct livepatch_elf *elf, const void *data)
     int rc;
 
     /* livepatch_elf_load sanity checked e_shnum. */
-    sec = xmalloc_array(struct livepatch_elf_sec, elf->hdr->e_shnum);
+    sec = xzalloc_array(struct livepatch_elf_sec, elf->hdr->e_shnum);
     if ( !sec )
     {
         dprintk(XENLOG_ERR, LIVEPATCH"%s: Could not allocate memory for section table!\n",
@@ -225,7 +225,7 @@ static int elf_get_sym(struct livepatch_elf *elf, const void *data)
     /* No need to check values as elf_resolve_sections did it. */
     nsym = symtab_sec->sec->sh_size / symtab_sec->sec->sh_entsize;
 
-    sym = xmalloc_array(struct livepatch_elf_sym, nsym);
+    sym = xzalloc_array(struct livepatch_elf_sym, nsym);
     if ( !sym )
     {
         dprintk(XENLOG_ERR, LIVEPATCH "%s: Could not allocate memory for symbols\n",
-- 
2.1.4


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

Re: [PATCH for-4.9 v3 2/3] xen/livepatch: Use zeroed memory allocations for arrays

[Konrad Rzeszutek Wilk]

On Thu, Jun 22, 2017 at 07:15:28PM +0100, Andrew Cooper wrote:
> Each of these arrays is sparse.  Use zeroed allocations to cause uninitialised
> array elements to contain deterministic values, most importantly for the
> embedded pointers.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

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

Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
[x86 and ARM32]
> CC: Ross Lagerwall <ross.lagerwall@citrix.com>
> 
> * new in v3
> ---
>  xen/common/livepatch.c     | 4 ++--
>  xen/common/livepatch_elf.c | 4 ++--
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
> index df67a1a..66d532d 100644
> --- a/xen/common/livepatch.c
> +++ b/xen/common/livepatch.c
> @@ -771,8 +771,8 @@ static int build_symbol_table(struct payload *payload,
>          }
>      }
>  
> -    symtab = xmalloc_array(struct livepatch_symbol, nsyms);
> -    strtab = xmalloc_array(char, strtab_len);
> +    symtab = xzalloc_array(struct livepatch_symbol, nsyms);
> +    strtab = xzalloc_array(char, strtab_len);
>  
>      if ( !strtab || !symtab )
>      {
> diff --git a/xen/common/livepatch_elf.c b/xen/common/livepatch_elf.c
> index c4a9633..b69e271 100644
> --- a/xen/common/livepatch_elf.c
> +++ b/xen/common/livepatch_elf.c
> @@ -52,7 +52,7 @@ static int elf_resolve_sections(struct livepatch_elf *elf, const void *data)
>      int rc;
>  
>      /* livepatch_elf_load sanity checked e_shnum. */
> -    sec = xmalloc_array(struct livepatch_elf_sec, elf->hdr->e_shnum);
> +    sec = xzalloc_array(struct livepatch_elf_sec, elf->hdr->e_shnum);
>      if ( !sec )
>      {
>          dprintk(XENLOG_ERR, LIVEPATCH"%s: Could not allocate memory for section table!\n",
> @@ -225,7 +225,7 @@ static int elf_get_sym(struct livepatch_elf *elf, const void *data)
>      /* No need to check values as elf_resolve_sections did it. */
>      nsym = symtab_sec->sec->sh_size / symtab_sec->sec->sh_entsize;
>  
> -    sym = xmalloc_array(struct livepatch_elf_sym, nsym);
> +    sym = xzalloc_array(struct livepatch_elf_sym, nsym);
>      if ( !sym )
>      {
>          dprintk(XENLOG_ERR, LIVEPATCH "%s: Could not allocate memory for symbols\n",
> -- 
> 2.1.4
> 

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

Re: [PATCH for-4.9 v3 2/3] xen/livepatch: Use zeroed memory allocations for arrays

[Ross Lagerwall]

On 06/22/2017 07:15 PM, Andrew Cooper wrote:
> Each of these arrays is sparse.  Use zeroed allocations to cause uninitialised
> array elements to contain deterministic values, most importantly for the
> embedded pointers.
> 
> 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

[PATCH for-4.9 v3 3/3] 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, fix an off-by-one error while range checking symndx, and 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>

v3:
 * Fix off-by-one error
v2:
 * Reject STN_UNDEF with -EOPNOTSUPP
---
 xen/arch/arm/arm32/livepatch.c | 14 +++++++++++++-
 xen/arch/arm/arm64/livepatch.c | 14 +++++++++++++-
 xen/arch/x86/livepatch.c       | 14 +++++++++++++-
 3 files changed, 39 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/arm32/livepatch.c b/xen/arch/arm/arm32/livepatch.c
index a328179..41378a5 100644
--- a/xen/arch/arm/arm32/livepatch.c
+++ b/xen/arch/arm/arm32/livepatch.c
@@ -254,12 +254,24 @@ int arch_livepatch_perform(struct livepatch_elf *elf,
             addend = get_addend(type, dest);
         }
 
-        if ( symndx > elf->nsym )
+        if ( symndx == STN_UNDEF )
+        {
+            dprintk(XENLOG_ERR, LIVEPATCH "%s: Encountered STN_UNDEF\n",
+                    elf->name);
+            return -EOPNOTSUPP;
+        }
+        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;
         }
+        else if ( !elf->sym[symndx].sym )
+        {
+            dprintk(XENLOG_ERR, LIVEPATCH "%s: No relative symbol@%u\n",
+                    elf->name, symndx);
+            return -EINVAL;
+        }
 
         val = elf->sym[symndx].sym->st_value; /* S */
 
diff --git a/xen/arch/arm/arm64/livepatch.c b/xen/arch/arm/arm64/livepatch.c
index 63929b1..2247b92 100644
--- a/xen/arch/arm/arm64/livepatch.c
+++ b/xen/arch/arm/arm64/livepatch.c
@@ -252,12 +252,24 @@ int arch_livepatch_perform_rela(struct livepatch_elf *elf,
         int ovf = 0;
         uint64_t val;
 
-        if ( symndx > elf->nsym )
+        if ( symndx == STN_UNDEF )
+        {
+            dprintk(XENLOG_ERR, LIVEPATCH "%s: Encountered STN_UNDEF\n",
+                    elf->name);
+            return -EOPNOTSUPP;
+        }
+        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;
         }
+        else if ( !elf->sym[symndx].sym )
+        {
+            dprintk(XENLOG_ERR, LIVEPATCH "%s: No relative symbol@%u\n",
+                    elf->name, symndx);
+            return -EINVAL;
+        }
 
         val = elf->sym[symndx].sym->st_value +  r->r_addend; /* S+A */
 
diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c
index 7917610..406eb91 100644
--- a/xen/arch/x86/livepatch.c
+++ b/xen/arch/x86/livepatch.c
@@ -170,12 +170,24 @@ 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 )
+        {
+            dprintk(XENLOG_ERR, LIVEPATCH "%s: Encountered STN_UNDEF\n",
+                    elf->name);
+            return -EOPNOTSUPP;
+        }
+        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;
         }
+        else if ( !elf->sym[symndx].sym )
+        {
+            dprintk(XENLOG_ERR, LIVEPATCH "%s: No symbol@%u\n",
+                    elf->name, symndx);
+            return -EINVAL;
+        }
 
         val = r->r_addend + elf->sym[symndx].sym->st_value;
 
-- 
2.1.4


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

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

[Stefano Stabellini]

On Thu, 22 Jun 2017, 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, fix an off-by-one error while range checking symndx, and 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>
> 
> v3:
>  * Fix off-by-one error
> v2:
>  * Reject STN_UNDEF with -EOPNOTSUPP

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
>  xen/arch/arm/arm32/livepatch.c | 14 +++++++++++++-
>  xen/arch/arm/arm64/livepatch.c | 14 +++++++++++++-
>  xen/arch/x86/livepatch.c       | 14 +++++++++++++-
>  3 files changed, 39 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/arm/arm32/livepatch.c b/xen/arch/arm/arm32/livepatch.c
> index a328179..41378a5 100644
> --- a/xen/arch/arm/arm32/livepatch.c
> +++ b/xen/arch/arm/arm32/livepatch.c
> @@ -254,12 +254,24 @@ int arch_livepatch_perform(struct livepatch_elf *elf,
>              addend = get_addend(type, dest);
>          }
>  
> -        if ( symndx > elf->nsym )
> +        if ( symndx == STN_UNDEF )
> +        {
> +            dprintk(XENLOG_ERR, LIVEPATCH "%s: Encountered STN_UNDEF\n",
> +                    elf->name);
> +            return -EOPNOTSUPP;
> +        }
> +        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;
>          }
> +        else if ( !elf->sym[symndx].sym )
> +        {
> +            dprintk(XENLOG_ERR, LIVEPATCH "%s: No relative symbol@%u\n",
> +                    elf->name, symndx);
> +            return -EINVAL;
> +        }
>  
>          val = elf->sym[symndx].sym->st_value; /* S */
>  
> diff --git a/xen/arch/arm/arm64/livepatch.c b/xen/arch/arm/arm64/livepatch.c
> index 63929b1..2247b92 100644
> --- a/xen/arch/arm/arm64/livepatch.c
> +++ b/xen/arch/arm/arm64/livepatch.c
> @@ -252,12 +252,24 @@ int arch_livepatch_perform_rela(struct livepatch_elf *elf,
>          int ovf = 0;
>          uint64_t val;
>  
> -        if ( symndx > elf->nsym )
> +        if ( symndx == STN_UNDEF )
> +        {
> +            dprintk(XENLOG_ERR, LIVEPATCH "%s: Encountered STN_UNDEF\n",
> +                    elf->name);
> +            return -EOPNOTSUPP;
> +        }
> +        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;
>          }
> +        else if ( !elf->sym[symndx].sym )
> +        {
> +            dprintk(XENLOG_ERR, LIVEPATCH "%s: No relative symbol@%u\n",
> +                    elf->name, symndx);
> +            return -EINVAL;
> +        }
>  
>          val = elf->sym[symndx].sym->st_value +  r->r_addend; /* S+A */
>  
> diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c
> index 7917610..406eb91 100644
> --- a/xen/arch/x86/livepatch.c
> +++ b/xen/arch/x86/livepatch.c
> @@ -170,12 +170,24 @@ 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 )
> +        {
> +            dprintk(XENLOG_ERR, LIVEPATCH "%s: Encountered STN_UNDEF\n",
> +                    elf->name);
> +            return -EOPNOTSUPP;
> +        }
> +        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;
>          }
> +        else if ( !elf->sym[symndx].sym )
> +        {
> +            dprintk(XENLOG_ERR, LIVEPATCH "%s: No symbol@%u\n",
> +                    elf->name, symndx);
> +            return -EINVAL;
> +        }
>  
>          val = r->r_addend + elf->sym[symndx].sym->st_value;
>  
> -- 
> 2.1.4
> 

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

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

[Konrad Rzeszutek Wilk]

On Thu, Jun 22, 2017 at 07:15:29PM +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, fix an off-by-one error while range checking symndx, and 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>

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

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

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

[Jan Beulich]

>>> On 22.06.17 at 20:15, <andrew.cooper3@citrix.com> 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, fix an off-by-one error while range checking symndx, and 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: Jan Beulich <jbeulich@suse.com>
with two remarks:

> --- a/xen/arch/x86/livepatch.c
> +++ b/xen/arch/x86/livepatch.c
> @@ -170,12 +170,24 @@ 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 )
> +        {
> +            dprintk(XENLOG_ERR, LIVEPATCH "%s: Encountered STN_UNDEF\n",
> +                    elf->name);
> +            return -EOPNOTSUPP;
> +        }
> +        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;
>          }
> +        else if ( !elf->sym[symndx].sym )

Neither of the two "else" is really necessary, and elsewhere we've
been telling people to avoid such.

> +        {
> +            dprintk(XENLOG_ERR, LIVEPATCH "%s: No symbol@%u\n",

Symbol tables can grow large, and for large numbers I generally
find hex representation preferable of dec. Otoh the other
(pre-existing) message uses dec too ...

Jan


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

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

[Andrew Cooper]

On 23/06/17 10:50, Jan Beulich wrote:
>>>> On 22.06.17 at 20:15, <andrew.cooper3@citrix.com> 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, fix an off-by-one error while range checking symndx, and 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: Jan Beulich <jbeulich@suse.com>
> with two remarks:
>
>> --- a/xen/arch/x86/livepatch.c
>> +++ b/xen/arch/x86/livepatch.c
>> @@ -170,12 +170,24 @@ 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 )
>> +        {
>> +            dprintk(XENLOG_ERR, LIVEPATCH "%s: Encountered STN_UNDEF\n",
>> +                    elf->name);
>> +            return -EOPNOTSUPP;
>> +        }
>> +        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;
>>          }
>> +        else if ( !elf->sym[symndx].sym )
> Neither of the two "else" is really necessary, and elsewhere we've
> been telling people to avoid such.

I see two logically different scenarios.

Per the style, if I were to use fully separate if() statements, I'd need
a newline between each.  This expands the code, and separates a chain of
logically-related checks.

IMO, its better to keep logically related checks more obviously
together, while I would definitely agree that unrelated chains (which
could in principle be if/else like this) should be separated.

>
>> +        {
>> +            dprintk(XENLOG_ERR, LIVEPATCH "%s: No symbol@%u\n",
> Symbol tables can grow large, and for large numbers I generally
> find hex representation preferable of dec. Otoh the other
> (pre-existing) message uses dec too ...

I'll stay consistent with everything else.

~Andrew

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

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

[Ross Lagerwall]

On 06/22/2017 07:15 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, fix an off-by-one error while range checking symndx, and 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 v3 3/3] xen/livepatch: Don't crash on encountering STN_UNDEF relocations

[Julien Grall]

Hi Andrew,

I am a bit confused, the title says "PATCH for-4.9 v3 3/3". I haven't 
been CCed on the first two patches. Does it mean you are only looking at 
this patch to be in 4.9?

Cheers,

On 22/06/17 19:15, 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, fix an off-by-one error while range checking symndx, and 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>
>
> v3:
>  * Fix off-by-one error
> v2:
>  * Reject STN_UNDEF with -EOPNOTSUPP
> ---
>  xen/arch/arm/arm32/livepatch.c | 14 +++++++++++++-
>  xen/arch/arm/arm64/livepatch.c | 14 +++++++++++++-
>  xen/arch/x86/livepatch.c       | 14 +++++++++++++-
>  3 files changed, 39 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/arm/arm32/livepatch.c b/xen/arch/arm/arm32/livepatch.c
> index a328179..41378a5 100644
> --- a/xen/arch/arm/arm32/livepatch.c
> +++ b/xen/arch/arm/arm32/livepatch.c
> @@ -254,12 +254,24 @@ int arch_livepatch_perform(struct livepatch_elf *elf,
>              addend = get_addend(type, dest);
>          }
>
> -        if ( symndx > elf->nsym )
> +        if ( symndx == STN_UNDEF )
> +        {
> +            dprintk(XENLOG_ERR, LIVEPATCH "%s: Encountered STN_UNDEF\n",
> +                    elf->name);
> +            return -EOPNOTSUPP;
> +        }
> +        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;
>          }
> +        else if ( !elf->sym[symndx].sym )
> +        {
> +            dprintk(XENLOG_ERR, LIVEPATCH "%s: No relative symbol@%u\n",
> +                    elf->name, symndx);
> +            return -EINVAL;
> +        }
>
>          val = elf->sym[symndx].sym->st_value; /* S */
>
> diff --git a/xen/arch/arm/arm64/livepatch.c b/xen/arch/arm/arm64/livepatch.c
> index 63929b1..2247b92 100644
> --- a/xen/arch/arm/arm64/livepatch.c
> +++ b/xen/arch/arm/arm64/livepatch.c
> @@ -252,12 +252,24 @@ int arch_livepatch_perform_rela(struct livepatch_elf *elf,
>          int ovf = 0;
>          uint64_t val;
>
> -        if ( symndx > elf->nsym )
> +        if ( symndx == STN_UNDEF )
> +        {
> +            dprintk(XENLOG_ERR, LIVEPATCH "%s: Encountered STN_UNDEF\n",
> +                    elf->name);
> +            return -EOPNOTSUPP;
> +        }
> +        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;
>          }
> +        else if ( !elf->sym[symndx].sym )
> +        {
> +            dprintk(XENLOG_ERR, LIVEPATCH "%s: No relative symbol@%u\n",
> +                    elf->name, symndx);
> +            return -EINVAL;
> +        }
>
>          val = elf->sym[symndx].sym->st_value +  r->r_addend; /* S+A */
>
> diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c
> index 7917610..406eb91 100644
> --- a/xen/arch/x86/livepatch.c
> +++ b/xen/arch/x86/livepatch.c
> @@ -170,12 +170,24 @@ 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 )
> +        {
> +            dprintk(XENLOG_ERR, LIVEPATCH "%s: Encountered STN_UNDEF\n",
> +                    elf->name);
> +            return -EOPNOTSUPP;
> +        }
> +        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;
>          }
> +        else if ( !elf->sym[symndx].sym )
> +        {
> +            dprintk(XENLOG_ERR, LIVEPATCH "%s: No symbol@%u\n",
> +                    elf->name, symndx);
> +            return -EINVAL;
> +        }
>
>          val = r->r_addend + elf->sym[symndx].sym->st_value;
>
>

-- 
Julien Grall

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

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

[Andrew Cooper]

On 23/06/17 14:32, Julien Grall wrote:
> Hi Andrew,
>
> I am a bit confused, the title says "PATCH for-4.9 v3 3/3". I haven't
> been CCed on the first two patches. Does it mean you are only looking
> at this patch to be in 4.9?

Sorry - I messed up the CC lists.  The correctness of this patch does
depend on the previous two, so all 3 are looking for inclusion.

~Andrew

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

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

[Julien Grall]

Hi,

On 23/06/17 14:33, Andrew Cooper wrote:
> On 23/06/17 14:32, Julien Grall wrote:
>> Hi Andrew,
>>
>> I am a bit confused, the title says "PATCH for-4.9 v3 3/3". I haven't
>> been CCed on the first two patches. Does it mean you are only looking
>> at this patch to be in 4.9?
>
> Sorry - I messed up the CC lists.  The correctness of this patch does
> depend on the previous two, so all 3 are looking for inclusion.

Given that we don't have livepatch testing in osstest how much test have 
we done on those 3 patches?

Cheers,

-- 
Julien Grall

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

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

[Andrew Cooper]

On 23/06/17 14:43, Julien Grall wrote:
> Hi,
>
> On 23/06/17 14:33, Andrew Cooper wrote:
>> On 23/06/17 14:32, Julien Grall wrote:
>>> Hi Andrew,
>>>
>>> I am a bit confused, the title says "PATCH for-4.9 v3 3/3". I haven't
>>> been CCed on the first two patches. Does it mean you are only looking
>>> at this patch to be in 4.9?
>>
>> Sorry - I messed up the CC lists.  The correctness of this patch does
>> depend on the previous two, so all 3 are looking for inclusion.
>
> Given that we don't have livepatch testing in osstest how much test
> have we done on those 3 patches?

There is testing in OSSTest.

I've manually run each of the scenarios, including with my livepatch
which has a STN_UNDEF relocation.

I don't know what testing Konrad has done.

~Andrew

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

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

[Julien Grall]

On 23/06/17 14:45, Andrew Cooper wrote:
> On 23/06/17 14:43, Julien Grall wrote:
>> Hi,
>>
>> On 23/06/17 14:33, Andrew Cooper wrote:
>>> On 23/06/17 14:32, Julien Grall wrote:
>>>> Hi Andrew,
>>>>
>>>> I am a bit confused, the title says "PATCH for-4.9 v3 3/3". I haven't
>>>> been CCed on the first two patches. Does it mean you are only looking
>>>> at this patch to be in 4.9?
>>>
>>> Sorry - I messed up the CC lists.  The correctness of this patch does
>>> depend on the previous two, so all 3 are looking for inclusion.
>>
>> Given that we don't have livepatch testing in osstest how much test
>> have we done on those 3 patches?
>
> There is testing in OSSTest.

Oh yes sorry. But only for amd64 and i386. No arm32 nor arm64 test.

>
> I've manually run each of the scenarios, including with my livepatch
> which has a STN_UNDEF relocation.
>
> I don't know what testing Konrad has done.

Cheers,

-- 
Julien Grall

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

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

[Konrad Rzeszutek Wilk]

On Fri, Jun 23, 2017 at 02:45:22PM +0100, Andrew Cooper wrote:
> On 23/06/17 14:43, Julien Grall wrote:
> > Hi,
> >
> > On 23/06/17 14:33, Andrew Cooper wrote:
> >> On 23/06/17 14:32, Julien Grall wrote:
> >>> Hi Andrew,
> >>>
> >>> I am a bit confused, the title says "PATCH for-4.9 v3 3/3". I haven't
> >>> been CCed on the first two patches. Does it mean you are only looking
> >>> at this patch to be in 4.9?
> >>
> >> Sorry - I messed up the CC lists.  The correctness of this patch does
> >> depend on the previous two, so all 3 are looking for inclusion.
> >
> > Given that we don't have livepatch testing in osstest how much test
> > have we done on those 3 patches?
> 
> There is testing in OSSTest.

Hurray hurray hurray!
> 
> I've manually run each of the scenarios, including with my livepatch
> which has a STN_UNDEF relocation.
>
> I don't know what testing Konrad has done.

I run a version of the same tests that are in OSSTest (basically an earlier
version of the Perl code) and I have done it on x86 and on ARM32.

And I also run the standalone OSSTest (on x86)

And then I also do a livepatch using the livepatch-build-tools on x86 to
patch some silly function.

So from a testing perspective these patches have been tested very exhaustively.

> 
> ~Andrew

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

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

[Julien Grall]

On 23/06/17 15:35, Konrad Rzeszutek Wilk wrote:
> On Fri, Jun 23, 2017 at 02:45:22PM +0100, Andrew Cooper wrote:
>> On 23/06/17 14:43, Julien Grall wrote:
>>> Hi,
>>>
>>> On 23/06/17 14:33, Andrew Cooper wrote:
>>>> On 23/06/17 14:32, Julien Grall wrote:
>>>>> Hi Andrew,
>>>>>
>>>>> I am a bit confused, the title says "PATCH for-4.9 v3 3/3". I haven't
>>>>> been CCed on the first two patches. Does it mean you are only looking
>>>>> at this patch to be in 4.9?
>>>>
>>>> Sorry - I messed up the CC lists.  The correctness of this patch does
>>>> depend on the previous two, so all 3 are looking for inclusion.
>>>
>>> Given that we don't have livepatch testing in osstest how much test
>>> have we done on those 3 patches?
>>
>> There is testing in OSSTest.
>
> Hurray hurray hurray!
>>
>> I've manually run each of the scenarios, including with my livepatch
>> which has a STN_UNDEF relocation.
>>
>> I don't know what testing Konrad has done.
>
> I run a version of the same tests that are in OSSTest (basically an earlier
> version of the Perl code) and I have done it on x86 and on ARM32.
>
> And I also run the standalone OSSTest (on x86)
>
> And then I also do a livepatch using the livepatch-build-tools on x86 to
> patch some silly function.
>
> So from a testing perspective these patches have been tested very exhaustively.

Well it has not been tested on ARM64 :). I am about to do that.

Cheers,

-- 
Julien Grall

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

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

[Konrad Rzeszutek Wilk]

[-- Attachment #1: Type: text/plain, Size: 1891 bytes --]

On Fri, Jun 23, 2017 at 03:36:51PM +0100, Julien Grall wrote:
> 
> 
> On 23/06/17 15:35, Konrad Rzeszutek Wilk wrote:
> > On Fri, Jun 23, 2017 at 02:45:22PM +0100, Andrew Cooper wrote:
> > > On 23/06/17 14:43, Julien Grall wrote:
> > > > Hi,
> > > > 
> > > > On 23/06/17 14:33, Andrew Cooper wrote:
> > > > > On 23/06/17 14:32, Julien Grall wrote:
> > > > > > Hi Andrew,
> > > > > > 
> > > > > > I am a bit confused, the title says "PATCH for-4.9 v3 3/3". I haven't
> > > > > > been CCed on the first two patches. Does it mean you are only looking
> > > > > > at this patch to be in 4.9?
> > > > > 
> > > > > Sorry - I messed up the CC lists.  The correctness of this patch does
> > > > > depend on the previous two, so all 3 are looking for inclusion.
> > > > 
> > > > Given that we don't have livepatch testing in osstest how much test
> > > > have we done on those 3 patches?
> > > 
> > > There is testing in OSSTest.
> > 
> > Hurray hurray hurray!
> > > 
> > > I've manually run each of the scenarios, including with my livepatch
> > > which has a STN_UNDEF relocation.
> > > 
> > > I don't know what testing Konrad has done.
> > 
> > I run a version of the same tests that are in OSSTest (basically an earlier
> > version of the Perl code) and I have done it on x86 and on ARM32.
> > 
> > And I also run the standalone OSSTest (on x86)
> > 
> > And then I also do a livepatch using the livepatch-build-tools on x86 to
> > patch some silly function.
> > 
> > So from a testing perspective these patches have been tested very exhaustively.
> 
> Well it has not been tested on ARM64 :). I am about to do that.

/me facepalm.

I really need to get myself a working ARM64 box that is not expensive.


Also attached is the poor-man livepatch_test.perl script that mirrors
what OSSTest does.

[Do adjust the path, it is /usr/lib/debug, but it should be /usr/lib/debug/livepatch
with Xen 4.9]


[-- Attachment #2: livepatch_test.perl --]
[-- Type: text/plain, Size: 2927 bytes --]

#!/usr/bin/perl

use Data::Dumper;

my @livepatch_files = ("xen_hello_world.livepatch", "xen_replace_world.livepatch",
			"xen_bye_world.livepatch", "xen_nop.livepatch");

my @livepatch_tests = (
	{cmd => "xen-livepatch list", rc => 0},
	{cmd => "xl info | grep xen_extra | grep -q \"Hello World\"", rc => 256},
	{cmd => "xen-livepatch revert xen_hello_world", rc => 256},
	{cmd => "xen-livepatch load xen_hello_world.livepatch", rc => 0},
	{cmd => "xen-livepatch load xen_hello_world.livepatch", rc => 256},
	{cmd => "xen-livepatch list | grep -q xen_hello_world", rc => 0},
	{cmd => "xl info | grep xen_extra | grep -q \"Hello World\"", rc => 0},
	{cmd => "xen-livepatch revert xen_hello_world", rc => 0},
	{cmd => "xl info | grep xen_extra | grep -q \"Hello World\"", rc => 256},
	{cmd => "xen-livepatch unload xen_hello_world", rc => 0},
	{cmd => "xen-livepatch unload xen_hello_world", rc => 256},
	{cmd => "xl info | grep xen_extra | grep -q \"Hello World\"", rc => 256},
	{cmd => "xen-livepatch load xen_hello_world.livepatch", rc => 0},
	{cmd => "xen-livepatch load xen_bye_world.livepatch", rc => 0},
	{cmd => "xl info | grep xen_extra | grep -q \"Bye World\"", rc => 0},
	{cmd => "xen-livepatch upload xen_replace xen_replace_world.livepatch", rc => 0},
	{cmd => "xen-livepatch replace xen_replace", rc => 0},
	{cmd => "xen-livepatch apply xen_hello_world", rc => 256},
	{cmd => "xen-livepatch apply xen_bye_world", rc => 256},
	{cmd => "xl info | grep xen_extra | grep -q \"Hello Again Wor\"", rc => 0},
	{cmd => "xen-livepatch apply xen_replace", rc => 0},
	{cmd => "xen-livepatch revert xen_replace", rc => 0},
	{cmd => "xen-livepatch unload xen_replace", rc => 0},
	{cmd => "xen-livepatch unload xen_hello_world", rc => 0},
	{cmd => "xen-livepatch unload xen_bye_world", rc => 0},
	{cmd => "xen-livepatch list | grep -q xen", rc => 256},
	# If running this under Xen 4.4, or 5.5 it will fail.
	#{cmd => "[ `xl info| grep \"xen_m\" | grep or | sed s/.*:// | uniq | wc -l` == 2 ]", rc => 0},
	{cmd => "xen-livepatch load xen_nop.livepatch", rc => 0},
	{cmd => "xen-livepatch revert xen_nop", rc => 0},
	{cmd => "xen-livepatch apply xen_nop", rc => 0},
	{cmd => "[ `xl info| grep \"xen_m\" | grep or | sed s/.*:// | uniq | wc -l` == 1 ]", rc => 0},
	{cmd => "xen-livepatch unload xen_nop", rc => 256},
	{cmd => "xen-livepatch revert xen_nop", rc => 0},
	{cmd => "xen-livepatch unload xen_nop", rc => 0},
	);

chdir("/usr/lib/debug") or die "cannot change: $!\n";

foreach my $file (@livepatch_files)
{
	if ( ! -f $file ) {
		die "$file is missing!\n";
	}
}
print "Have $#livepatch_tests test-cases\n";
my $rc=0;
for my $i ( 0 .. $#livepatch_tests ) {
	my $expected_rc = $livepatch_tests[$i]{rc};
	my $cmd = $livepatch_tests[$i]{cmd};
	print "Executing: '$cmd:' ..";
	my $rc=system($cmd);
	if ( $rc != $expected_rc ) {
		print "FAILED (got $rc, expected: $expected_rc)\n";
		exit $rc
	}
	print ".. OK!\n";
}
exit $rc

[-- Attachment #3: Type: text/plain, Size: 127 bytes --]

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

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

[Julien Grall]

Hi Konrad,

On 06/23/2017 03:46 PM, Konrad Rzeszutek Wilk wrote:
> On Fri, Jun 23, 2017 at 03:36:51PM +0100, Julien Grall wrote:
>>
>>
>> On 23/06/17 15:35, Konrad Rzeszutek Wilk wrote:
>>> On Fri, Jun 23, 2017 at 02:45:22PM +0100, Andrew Cooper wrote:
>>>> On 23/06/17 14:43, Julien Grall wrote:
>>>>> Hi,
>>>>>
>>>>> On 23/06/17 14:33, Andrew Cooper wrote:
>>>>>> On 23/06/17 14:32, Julien Grall wrote:
>>>>>>> Hi Andrew,
>>>>>>>
>>>>>>> I am a bit confused, the title says "PATCH for-4.9 v3 3/3". I haven't
>>>>>>> been CCed on the first two patches. Does it mean you are only looking
>>>>>>> at this patch to be in 4.9?
>>>>>>
>>>>>> Sorry - I messed up the CC lists.  The correctness of this patch does
>>>>>> depend on the previous two, so all 3 are looking for inclusion.
>>>>>
>>>>> Given that we don't have livepatch testing in osstest how much test
>>>>> have we done on those 3 patches?
>>>>
>>>> There is testing in OSSTest.
>>>
>>> Hurray hurray hurray!
>>>>
>>>> I've manually run each of the scenarios, including with my livepatch
>>>> which has a STN_UNDEF relocation.
>>>>
>>>> I don't know what testing Konrad has done.
>>>
>>> I run a version of the same tests that are in OSSTest (basically an earlier
>>> version of the Perl code) and I have done it on x86 and on ARM32.
>>>
>>> And I also run the standalone OSSTest (on x86)
>>>
>>> And then I also do a livepatch using the livepatch-build-tools on x86 to
>>> patch some silly function.
>>>
>>> So from a testing perspective these patches have been tested very exhaustively.
>>
>> Well it has not been tested on ARM64 :). I am about to do that.
> 
> /me facepalm.
> 
> I really need to get myself a working ARM64 box that is not expensive.
> 
> 
> Also attached is the poor-man livepatch_test.perl script that mirrors
> what OSSTest does.

I have got an error when executing the script after applying the "nop" 
livepatch:

Executing: 'xen-livepatch apply xen_nop:' ..Applying xen_nop... completed
.. OK!
Executing: '[ `xl info| grep "xen_m" | grep or | sed s/.*:// | uniq | wc 
-l` == 1 ]:' ..sh: 1: [: 1: unexpected operator
FAILED (got 512, expected: 0)

But this looks like a script error than livepatch. Although, I was not 
able to spot the error in the script.

Cheers,

-- 
Julien Grall

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

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

[Konrad Rzeszutek Wilk]

On Sat, Jun 24, 2017 at 06:28:16PM +0100, Julien Grall wrote:
> Hi Konrad,
> 
> On 06/23/2017 03:46 PM, Konrad Rzeszutek Wilk wrote:
> > On Fri, Jun 23, 2017 at 03:36:51PM +0100, Julien Grall wrote:
> > > 
> > > 
> > > On 23/06/17 15:35, Konrad Rzeszutek Wilk wrote:
> > > > On Fri, Jun 23, 2017 at 02:45:22PM +0100, Andrew Cooper wrote:
> > > > > On 23/06/17 14:43, Julien Grall wrote:
> > > > > > Hi,
> > > > > > 
> > > > > > On 23/06/17 14:33, Andrew Cooper wrote:
> > > > > > > On 23/06/17 14:32, Julien Grall wrote:
> > > > > > > > Hi Andrew,
> > > > > > > > 
> > > > > > > > I am a bit confused, the title says "PATCH for-4.9 v3 3/3". I haven't
> > > > > > > > been CCed on the first two patches. Does it mean you are only looking
> > > > > > > > at this patch to be in 4.9?
> > > > > > > 
> > > > > > > Sorry - I messed up the CC lists.  The correctness of this patch does
> > > > > > > depend on the previous two, so all 3 are looking for inclusion.
> > > > > > 
> > > > > > Given that we don't have livepatch testing in osstest how much test
> > > > > > have we done on those 3 patches?
> > > > > 
> > > > > There is testing in OSSTest.
> > > > 
> > > > Hurray hurray hurray!
> > > > > 
> > > > > I've manually run each of the scenarios, including with my livepatch
> > > > > which has a STN_UNDEF relocation.
> > > > > 
> > > > > I don't know what testing Konrad has done.
> > > > 
> > > > I run a version of the same tests that are in OSSTest (basically an earlier
> > > > version of the Perl code) and I have done it on x86 and on ARM32.
> > > > 
> > > > And I also run the standalone OSSTest (on x86)
> > > > 
> > > > And then I also do a livepatch using the livepatch-build-tools on x86 to
> > > > patch some silly function.
> > > > 
> > > > So from a testing perspective these patches have been tested very exhaustively.
> > > 
> > > Well it has not been tested on ARM64 :). I am about to do that.
> > 
> > /me facepalm.
> > 
> > I really need to get myself a working ARM64 box that is not expensive.
> > 
> > 
> > Also attached is the poor-man livepatch_test.perl script that mirrors
> > what OSSTest does.
> 
> I have got an error when executing the script after applying the "nop"
> livepatch:
> 
> Executing: 'xen-livepatch apply xen_nop:' ..Applying xen_nop... completed
> .. OK!
> Executing: '[ `xl info| grep "xen_m" | grep or | sed s/.*:// | uniq | wc -l`
> == 1 ]:' ..sh: 1: [: 1: unexpected operator
> FAILED (got 512, expected: 0)

Yeah I only get that when I execute it from the serial console.
When I SSH it works fine.

> 
> But this looks like a script error than livepatch. Although, I was not able
> to spot the error in the script.

Neither could I. I do also have an earlier variant of this testing script
in bash (with less test-cases).
> 
> Cheers,
> 
> -- 
> Julien Grall

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

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

[Julien Grall]

On 26/06/17 02:02, Konrad Rzeszutek Wilk wrote:
> On Sat, Jun 24, 2017 at 06:28:16PM +0100, Julien Grall wrote:
>> Hi Konrad,
>>
>> On 06/23/2017 03:46 PM, Konrad Rzeszutek Wilk wrote:
>>> On Fri, Jun 23, 2017 at 03:36:51PM +0100, Julien Grall wrote:
>>>>
>>>>
>>>> On 23/06/17 15:35, Konrad Rzeszutek Wilk wrote:
>>>>> On Fri, Jun 23, 2017 at 02:45:22PM +0100, Andrew Cooper wrote:
>>>>>> On 23/06/17 14:43, Julien Grall wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> On 23/06/17 14:33, Andrew Cooper wrote:
>>>>>>>> On 23/06/17 14:32, Julien Grall wrote:
>>>>>>>>> Hi Andrew,
>>>>>>>>>
>>>>>>>>> I am a bit confused, the title says "PATCH for-4.9 v3 3/3". I haven't
>>>>>>>>> been CCed on the first two patches. Does it mean you are only looking
>>>>>>>>> at this patch to be in 4.9?
>>>>>>>>
>>>>>>>> Sorry - I messed up the CC lists.  The correctness of this patch does
>>>>>>>> depend on the previous two, so all 3 are looking for inclusion.
>>>>>>>
>>>>>>> Given that we don't have livepatch testing in osstest how much test
>>>>>>> have we done on those 3 patches?
>>>>>>
>>>>>> There is testing in OSSTest.
>>>>>
>>>>> Hurray hurray hurray!
>>>>>>
>>>>>> I've manually run each of the scenarios, including with my livepatch
>>>>>> which has a STN_UNDEF relocation.
>>>>>>
>>>>>> I don't know what testing Konrad has done.
>>>>>
>>>>> I run a version of the same tests that are in OSSTest (basically an earlier
>>>>> version of the Perl code) and I have done it on x86 and on ARM32.
>>>>>
>>>>> And I also run the standalone OSSTest (on x86)
>>>>>
>>>>> And then I also do a livepatch using the livepatch-build-tools on x86 to
>>>>> patch some silly function.
>>>>>
>>>>> So from a testing perspective these patches have been tested very exhaustively.
>>>>
>>>> Well it has not been tested on ARM64 :). I am about to do that.
>>>
>>> /me facepalm.
>>>
>>> I really need to get myself a working ARM64 box that is not expensive.
>>>
>>>
>>> Also attached is the poor-man livepatch_test.perl script that mirrors
>>> what OSSTest does.
>>
>> I have got an error when executing the script after applying the "nop"
>> livepatch:
>>
>> Executing: 'xen-livepatch apply xen_nop:' ..Applying xen_nop... completed
>> .. OK!
>> Executing: '[ `xl info| grep "xen_m" | grep or | sed s/.*:// | uniq | wc -l`
>> == 1 ]:' ..sh: 1: [: 1: unexpected operator
>> FAILED (got 512, expected: 0)
>
> Yeah I only get that when I execute it from the serial console.
> When I SSH it works fine.
>
>>
>> But this looks like a script error than livepatch. Although, I was not able
>> to spot the error in the script.
>
> Neither could I. I do also have an earlier variant of this testing script
> in bash (with less test-cases).

I am happy with this patch series to be backported in Xen 4.9:

Release-acked-by: Julien Grall <julien.grall@arm.com>

Can they be backported today please?

Cheers,

-- 
Julien Grall

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