Re: [PATCH v1 6/9] livepatch: Initial ARM64 support.

[Julien Grall <julien.grall@arm.com>]

Hi Konrad,

On 17/08/2016 19:57, Konrad Rzeszutek Wilk wrote:
>>> +void arch_livepatch_apply_jmp(struct livepatch_func *func)
>>> +{
>>> +    uint32_t insn;
>>> +    uint32_t *old_ptr;
>>> +    uint32_t *new_ptr;
>>> +
>>> +    BUILD_BUG_ON(PATCH_INSN_SIZE > sizeof(func->opaque));
>>> +    BUILD_BUG_ON(PATCH_INSN_SIZE != sizeof(insn));
>>> +
>>> +    ASSERT(vmap_of_xen_text);
>>> +
>>> +    /* Save old one. */
>>> +    old_ptr = func->old_addr;
>>> +    memcpy(func->opaque, old_ptr, PATCH_INSN_SIZE);
>>> +
>>> +    if ( func->new_size )
>>> +        insn = aarch64_insn_gen_branch_imm((unsigned long)func->old_addr,
>>> +                                           (unsigned long)func->new_addr,
>>> +                                           AARCH64_INSN_BRANCH_NOLINK);
>>
>> The function aarch64_insn_gen_branch_imm will fail to create the branch if
>> the difference between old_addr and new_addr is greater than 128MB.
>>
>> In this case, the function will return AARCH64_BREAK_FAULT which will crash
>> Xen when the instruction is executed.
>>
>> I cannot find any sanity check in the hypervisor code. So are we trusting
>> the payload?
>
> Ugh. This is a thorny one. I concur we need to check this - and better of
> do it when we load the payload to make sure the distance is correct.
>
> And that should also be on x86, so will spin of a seperate patch.
>
> And in here add an ASSERT that the insn != AARCH64_BREAK_FAULT

It sounds good to me.

[...]

>>> +
>>> +    modify_xen_mappings(start, start + pages * PAGE_SIZE, flags);
>>> +
>>> +    return 0;
>>>  }
>>>
>>>  void __init arch_livepatch_init(void)
>>>  {
>>> +    void *start, *end;
>>> +
>>> +    start = (void *)LIVEPATCH_VMAP;
>>> +    end = start + MB(2);
>>
>> Could you define the in config.h? So in the future we can rework more easily
>> the memory layout.
>
> LIVEPATCH_VMAP_START and LIVEPATCH_VMAP_END ?

Something like that.

>
>>
>>> +
>>> +    vm_init_type(VMAP_XEN, start, end);
>>>  }
>>>
>>>  /*
>>> diff --git a/xen/arch/arm/livepatch.h b/xen/arch/arm/livepatch.h
>>> new file mode 100644
>>> index 0000000..8c8d625
>>> --- /dev/null
>>> +++ b/xen/arch/arm/livepatch.h
>>
>> I am not sure why this header is living in arch/arm/ and not
>> include/asm-arm/
>>
>>> @@ -0,0 +1,28 @@
>>> +/*
>>> + * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved.
>>> + *
>>> + */
>>> +
>>> +#ifndef __XEN_ARM_LIVEPATCH_H__
>>> +#define __XEN_ARM_LIVEPATCH_H__
>>> +
>>> +/* On ARM32,64 instructions are always 4 bytes long. */
>>> +#define PATCH_INSN_SIZE 4
>>> +
>>> +/*
>>> + * The va of the hypervisor .text region. We need this as the
>>> + * normal va are write protected.
>>> + */
>>> +extern void *vmap_of_xen_text;
>>> +
>>> +#endif /* __XEN_ARM_LIVEPATCH_H__ */
>>> +
>>> +/*
>>> + * Local variables:
>>> + * mode: C
>>> + * c-file-style: "BSD"
>>> + * c-basic-offset: 4
>>> + * tab-width: 4
>>> + * indent-tabs-mode: nil
>>> + * End:
>>> + */
>>> diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c
>>> index 06c67bc..e3f3f37 100644
>>> --- a/xen/arch/x86/livepatch.c
>>> +++ b/xen/arch/x86/livepatch.c
>>> @@ -15,10 +15,12 @@
>>>
>>>  #define PATCH_INSN_SIZE 5
>>>
>>> -void arch_livepatch_quiesce(void)
>>> +int arch_livepatch_quiesce(void)
>>>  {
>>>      /* Disable WP to allow changes to read-only pages. */
>>>      write_cr0(read_cr0() & ~X86_CR0_WP);
>>> +
>>> +    return 0;
>>>  }
>>>
>>>  void arch_livepatch_revive(void)
>>> diff --git a/xen/common/Kconfig b/xen/common/Kconfig
>>> index 51afa24..2fc76b6 100644
>>> --- a/xen/common/Kconfig
>>> +++ b/xen/common/Kconfig
>>> @@ -222,7 +222,7 @@ endmenu
>>>  config LIVEPATCH
>>>  	bool "Live patching support (TECH PREVIEW)"
>>>  	default n
>>> -	depends on X86 && HAS_BUILD_ID = "y"
>>> +	depends on (X86 || ARM_64) && HAS_BUILD_ID = "y"
>>>  	---help---
>>>  	  Allows a running Xen hypervisor to be dynamically patched using
>>>  	  binary patches without rebooting. This is primarily used to binarily
>>> diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
>>> index 9c45270..af9443d 100644
>>> --- a/xen/common/livepatch.c
>>> +++ b/xen/common/livepatch.c
>>> @@ -682,7 +682,7 @@ static int prepare_payload(struct payload *payload,
>>>                                    sizeof(*region->frame[i].bugs);
>>>      }
>>>
>>> -#ifndef CONFIG_ARM
>>> +#ifndef CONFIG_ARM_32
>>
>> I would prefer if you use CONFIG_ALTERNATIVE rather than CONFIG_ARM_32.
>
> That is not exposed on x86 thought.
>
> I can expose HAVE_ALTERNATIVE on x86 and use that instead.

True. I would like to avoid using  CONFIG_ARM_* in the common code as it 
is a call to forget to remove the #ifdef when ARM32 will gain 
alternative patching.

You are the maintainer of this code, so it is your call :).

Regards,

-- 
Julien Grall

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