Re: [Xen-devel] [PATCH v3 07/18] livepatch: ARM/x86: Check displacement of old_addr and new_addr

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 11.09.16 at 22:35, <konrad.wilk@xxxxxxxxxx> wrote:
> If the distance is too great we are in trouble - as our relocation

I think you mean "large" or big" here.

> distance can surely be clipped, or still have a valid width - but
> cause an overflow of distance.
> 
> On various architectures the maximum displacement for a unconditional
> branch/jump varies. ARM32 is +/- 32MB, ARM64 is +/- 128MB while x86
> for 32-bit relocations is +/- 2G.
> 
> Note: On x86 we could use the 64-bit jmpq instruction which
> would provide much bigger displacement to do a jump, but we would
> still have issues with the new function not being able to reach
> any of the old functions (as all the relocations would assume 32-bit
> displacement).

With jmpq you mean the indirect variant? That additionally
would require a register or memory location you can clobber
to load/store the address into.

> --- a/xen/include/xen/livepatch.h
> +++ b/xen/include/xen/livepatch.h
> @@ -12,6 +12,7 @@ struct livepatch_elf_sym;
>  struct xen_sysctl_livepatch_op;
>  
>  #include <xen/elfstructs.h>
> +#include <xen/errno.h> /* For -ENOSYS or -EOVERFLOW */
>  #ifdef CONFIG_LIVEPATCH
>  
>  /*
> @@ -66,16 +67,32 @@ int arch_livepatch_secure(const void *va, unsigned int 
> pages, enum va_type types
>  void arch_livepatch_init(void);
>  
>  #include <public/sysctl.h> /* For struct livepatch_func. */
> -#include <asm/livepatch.h> /* For PATCH_INSN_SIZE. */
> +#include <asm/livepatch.h> /* For LIVEPATCH_ARCH_RANGE and PATCH_INSN_SIZE */
>  int arch_livepatch_verify_func(const struct livepatch_func *func);
>  
> -static inline size_t arch_livepatch_insn_len(const struct livepatch_func 
> *func)
> +static inline
> +unsigned int arch_livepatch_insn_len(const struct livepatch_func *func)
>  {
>      if ( !func->new_addr )
>          return func->new_size;
>  
>      return PATCH_INSN_SIZE;
>  }
> +
> +static inline int arch_livepatch_verify_distance(const struct livepatch_func 
> *func)

Just like mentioned earlier for arch_livepatch_insn_len() I don't
think the arch prefix is a good choice when the function isn't
arch-specific.

> +{
> +    long offset;
> +    long range = (long)LIVEPATCH_ARCH_RANGE;
> +
> +    if ( !func->new_addr ) /* Ignore NOPs. */
> +        return 0;
> +
> +    offset = ((long)func->old_addr - (long)func->new_addr);

Please avoid casts when they're not really needed.

Jan


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