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

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

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