Re: [Xen-devel] [PATCH v4 6/9] livepatch: Add parsing for the symbol+0x<offset>

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

On Wed, Aug 24, 2016 at 03:08:01AM -0600, Jan Beulich wrote:
> >>> On 24.08.16 at 04:22, <konrad.wilk@xxxxxxxxxx> wrote:
> > --- a/xen/common/livepatch.c
> > +++ b/xen/common/livepatch.c
> > @@ -237,13 +237,34 @@ static const char *livepatch_symbols_lookup(unsigned 
> > long addr,
> >  static int resolve_old_address(struct livepatch_func *f,
> >                                 const struct livepatch_elf *elf)
> >  {
> > +    const char *s;
> > +    char *plus = NULL;
> 
> Pointless initializer.

We need that otherwise this part (which is at the bottom of this function):

    if ( plus )
    {
        *plus = '+';
        f->old_addr += offset;
    }


May be invoked for symbols that that don't have the '+' in them.
> 
> > +    unsigned long offset = 0;
> > +
> >      if ( f->old_addr )
> >          return 0;
> >  
> > -    f->old_addr = (void *)symbols_lookup_by_name(f->name);
> > +    s = f->name;
> 
> Otoh this could become s'es initializer.

<nods>
> 
> > +    /* +<offset> */
> > +    plus = strchr(f->name, '+');
> 
> And I think you should prefer using the local variable here.

<nods>

> 
> Furthermore you're losing const here - does f->name really point
> to memory that doesn't get mapped r/o?

Yes.

The 'struct livepatch_func' contains the the ->opaque array of 31 bytes
(from which we use 5 bytes) which the hypervisor uses to stash the original
instructions.
> 
> > +    if ( plus )
> > +    {
> > +        const char *endp = NULL;
> 
> Pointless initializer again (or else ...
> 
> > +        offset = simple_strtoul(plus + 1, &endp, 16);
> > +
> > +        if ( *endp != '\0' )
> 
> ... the deref here couldn't be unconditional).
> 
> > +            return -EINVAL;
> > +
> > +        /* So that symbol lookup works. */
> > +        *plus = '\0';
> > +        s = f->name;
> 
> Why? f->name didn't change afaict.

Ugh. Stale code.
> 
> Overall - are you sure you want to disallow symbol names containing
> + characters? I.e. you don't want to add support for some form of
> quoting?

Can you actually have + in a function or object?

Or are you asking in terms of the '\0' overwritting the '+' ?
We put it back later on so that the symbol still has + in it:

 274     if ( plus )                                                            
     
 275     {                                                                      
     
 276         *plus = '+';                                                       
     
 277         f->old_addr += offset;                                             
     
 278     }                                           

I will change the comment from 'So that symbol lookup works.' to
'Temporary mutilation of name so that symbol lookup works.'

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