Re: [Xen-devel] [PATCH v2 3/5] xen/livepatch/ARM32: Don't load and crash on livepatches loaded with wrong alignment.

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 26/07/2017 20:47, Konrad Rzeszutek Wilk wrote:
> diff --git a/xen/arch/arm/arm64/livepatch.c b/xen/arch/arm/arm64/livepatch.c
> index 2247b925a0..7b36210ccd 100644
> --- a/xen/arch/arm/arm64/livepatch.c
> +++ b/xen/arch/arm/arm64/livepatch.c
> @@ -86,6 +86,12 @@ bool arch_livepatch_symbol_deny(const struct livepatch_elf 
> *elf,
>      return false;
>  }
>  
> +bool arch_livepatch_verify_alignment(const struct livepatch_elf_sec *sec)
> +{

Semantically, "verify_alignment" implies "the alignment is correct", but
the return value is the opposite.

I'd recommend inverting the sense of these functions, returning true for
x86/arm64, and == 0 for arm32...

> +    /* Unaligned access on ARM 64 is OK. */
> +    return false;
> +}
> +
>  enum aarch64_reloc_op {
>      RELOC_OP_NONE,
>      RELOC_OP_ABS,
> diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c
> index 406eb910cc..b3cbdac9b7 100644
> --- a/xen/arch/x86/livepatch.c
> +++ b/xen/arch/x86/livepatch.c
> @@ -148,6 +148,12 @@ bool arch_livepatch_symbol_deny(const struct 
> livepatch_elf *elf,
>      return false;
>  }
>  
> +bool arch_livepatch_verify_alignment(const struct livepatch_elf_sec *sec)
> +{
> +    /* Unaligned access on x86 is fine. */
> +    return false;
> +}
> +
>  int arch_livepatch_perform_rel(struct livepatch_elf *elf,
>                                 const struct livepatch_elf_sec *base,
>                                 const struct livepatch_elf_sec *rela)
> diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
> index 40ff6b3a27..13d8f25a4b 100644
> --- a/xen/common/livepatch.c
> +++ b/xen/common/livepatch.c
> @@ -472,6 +472,13 @@ static int check_section(const struct livepatch_elf *elf,
>          return -EINVAL;
>      }
>  
> +    if ( arch_livepatch_verify_alignment(sec) )
> +    {
> +        dprintk(XENLOG_ERR, LIVEPATCH "%s: %s is not aligned properly!\n",
> +               elf->name, sec->name);

It also means this would read as

if ( !arch_livepatch_verify_alignment(sec) )
{
    "$blah not properly aligned"

which is also the usual way around to think.

(Also, bool functions and int functions really do have opposite success
cases, and should be kept that way.)

~Andrew

> +        return -EINVAL;
> +    }
> +
>      return 0;
>  }
>  


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