Re: [Xen-devel] [PATCH v3 7/9] livepatch: NOP if func->new_[addr, size] is zero.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 14.08.16 at 23:52, <konrad.wilk@xxxxxxxxxx> wrote:
> The NOP functionality will NOP any of the code at
> the 'old_addr' or at 'name' if the 'new_addr' and 'new_size'
> are both zero. The purpose of this is to NOP out calls, such as:
> 
>  e9 <4-bytes-offset>

Except that E9 is JMP; CALL is E8.

> (5 byte insn), or on ARM a 4 byte insn for branching.
> 
> We need the EIP of where we need to the NOP, and that can
> be provided via the `old_addr` or `name`.
> 
> If the `old_addr` is provided we will NOP
> 5 instructions (on x86) at that location.
> 
> If `name` is provided with the symbol+0x<offset/<len>
> we make sure that <len> is 5 (on x86) and upon retrieving the
> EIP based on `name` will NOP that location.

So why does this need to be restricted to 5-byte (on x86) code
blocks? I.e. what's wrong with NOP-ing out other code.

> @@ -46,18 +42,27 @@ int arch_livepatch_verify_func(const struct 
> livepatch_func *func)
>  
>  void arch_livepatch_apply_jmp(struct livepatch_func *func)
>  {
> -    int32_t val;
>      uint8_t *old_ptr;
> +    uint8_t insn[PATCH_INSN_SIZE];
>  
>      BUILD_BUG_ON(PATCH_INSN_SIZE > sizeof(func->opaque));
> -    BUILD_BUG_ON(PATCH_INSN_SIZE != (1 + sizeof(val)));
>  
>      old_ptr = func->old_addr;
>      memcpy(func->opaque, old_ptr, PATCH_INSN_SIZE);
>  
> -    *old_ptr++ = 0xe9; /* Relative jump */
> -    val = func->new_addr - func->old_addr - PATCH_INSN_SIZE;
> -    memcpy(old_ptr, &val, sizeof(val));
> +    if ( func->new_size )
> +    {
> +        int32_t val;
> +
> +        BUILD_BUG_ON(PATCH_INSN_SIZE != (1 + sizeof(val)));
> +
> +        insn[0] = 0xe9;
> +        val = func->new_addr - func->old_addr - PATCH_INSN_SIZE;
> +        memcpy(&insn[1], &val, sizeof(val));
> +    } else

Style.

> --- a/xen/common/livepatch.c
> +++ b/xen/common/livepatch.c
> @@ -561,11 +561,15 @@ static int prepare_payload(struct payload *payload,
>              return -EOPNOTSUPP;
>          }
>  
> -        if ( !f->new_addr || !f->new_size )
> +        /* If both are zero then we are NOPing. */
> +        if ( (!f->new_addr || !f->new_size) )

Comment and condition contradict one another. And you're adding
unnecessary parentheses.

Jan


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