Re: [Xen-devel] [PATCH v6 3/6] livepatch: NOP if func->new_addr is zero.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 19.09.16 at 18:11, <konrad.wilk@xxxxxxxxxx> wrote:
> On Mon, Sep 19, 2016 at 02:59:32AM -0600, Jan Beulich wrote:
>> >>> On 16.09.16 at 17:29, <konrad.wilk@xxxxxxxxxx> wrote:
>> > @@ -31,11 +30,11 @@ void arch_livepatch_revive(void)
>> >  
>> >  int arch_livepatch_verify_func(const struct livepatch_func *func)
>> >  {
>> > -    /* No NOP patching yet. */
>> > -    if ( !func->new_size )
>> > +    /* If NOPing only do up to maximum amount we can put in the ->opaque. 
>> > */
>> > +    if ( !func->new_addr && func->new_size > sizeof(func->opaque) )
>> >          return -EOPNOTSUPP;
>> >  
>> > -    if ( func->old_size < PATCH_INSN_SIZE )
>> > +    if ( func->old_size < ARCH_PATCH_INSN_SIZE )
>> >          return -EINVAL;
>> 
>> Is that indeed a requirement when NOPing? You can easily NOP out
>> just a single byte on x86. Or shouldn't in that case old_size == new_size
>> anyway? In which case the comment further down stating that new_size
> 
> The original intent behind .old_size was to guard against patching
> functions that were less than our relative jump. 
> 
> (The tools end up computing the .old_size as the size of the whole function
> which is fine).
> 
> But with this NOPing support, you are right - we could have now an
> function that is say 4 bytes long and we only need to NOP three bytes
> out of it (the last instruction I assume would be 'ret').
> 
> So perhaps this check needs just needs an 'else if' , like so:
> 
> int arch_livepatch_verify_func(const struct livepatch_func *func)
> {
>     /* If NOPing.. */
>     if ( !func->new_addr )
>     {
>         /* Only do up to maximum amount we can put in the ->opaque. */
>         if ( func->new_size > sizeof(func->opaque) )
>             return -EOPNOTSUPP;
> 
>         /* One instruction for 'ret' and the other to NOP. */
>         if ( func->old_size < 2 )
>             return -EINVAL;
>     }
>     else if ( func->old_size < ARCH_PATCH_INSN_SIZE )
>         return -EINVAL;
> 
>     return 0;
> }

Except that I wouldn't use 2, to not exclude patching out some
single byte in the middle of a function, without regard to what the
function's actual size is.

>> can be zero would also be wrong.
>> 
>> > @@ -43,23 +42,36 @@ 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;
>> > -
>> > -    BUILD_BUG_ON(PATCH_INSN_SIZE > sizeof(func->opaque));
>> > -    BUILD_BUG_ON(PATCH_INSN_SIZE != (1 + sizeof(val)));
>> > +    uint8_t insn[sizeof(func->opaque)];
>> > +    unsigned int len;
>> >  
>> >      old_ptr = func->old_addr;
>> > -    memcpy(func->opaque, old_ptr, PATCH_INSN_SIZE);
>> > +    len = livepatch_insn_len(func);
>> > +    if ( !len )
>> > +        return;
>> > +
>> > +    memcpy(func->opaque, old_ptr, len);
>> > +    if ( func->new_addr )
>> > +    {
>> > +        int32_t val;
>> > +
>> > +        BUILD_BUG_ON(ARCH_PATCH_INSN_SIZE != (1 + sizeof(val)));
>> > +
>> > +        insn[0] = 0xe9;
>> > +        val = func->new_addr - func->old_addr - ARCH_PATCH_INSN_SIZE;
>> > +
>> > +        memcpy(&insn[1], &val, sizeof(val));
>> > +    }
>> > +    else
>> > +        add_nops(&insn, len);
>> >  
>> > -    *old_ptr++ = 0xe9; /* Relative jump */
>> 
>> Are you btw intentionally getting rid of this comment? And with the
> 
> Not at all. Just missed it.
>> NOP addition here, perhaps worth dropping the _jmp from the
>> function name (and its revert counterpart)?
> 
> Ooh, good idea. But I think it maybe better as a seperate patch (as it
> also touches the ARM code).

That's in the other series, isn't it?

Jan


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