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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 19.09.16 at 19:02, <konrad.wilk@xxxxxxxxxx> wrote:
> On Mon, Sep 19, 2016 at 10:31:23AM -0600, Jan Beulich wrote:
>> >>> 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.
> 
> Uh-uh.
> 
> The _new_size_ determines how many bytes to NOP (in the context of
> this patch). The old_size (where we check to be at min 2) is a safety
> valve to make sure we don't NOP something outside the function.

Well, all this looks a little fishy to me: I don't see the relation to
functions at all here. Patching can be done anywhere - at the start
of a function, in its middle, at the end, or - in extreme cases - even
spanning function boundaries.

So perhaps all you really want then (without altering that basic
concept) is new_size <= old_size?

>> >> 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?
> 
> It expands the existing ones. Right now in 'staging' branch we have an
> arch/arm/livepatch.c which has these functions in it.
> 
> Granted nothing compiles them, so I could break it in this patch.
> 
> But I already cobbled up the patch so may as well use it?

Oh, sure.

Jan


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