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

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

On Mon, Aug 15, 2016 at 04:59:52AM -0600, Jan Beulich wrote:
> >>> On 14.08.16 at 23:52, <konrad.wilk@oracle.com> 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.

It can most certainly nop variable sizes. I will have to update
the design to make it clear that if 'new_addr' is zero then we will
NOP (and the .new_size will determine the amount of NOPs to sprinkle).

Let me do that along with your comments. Thanks!
> 
> > @@ -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@lists.xen.org
https://lists.xen.org/xen-devel