Re: [PATCH v3 1/5] xen/livepatch: drop load_addr Elf section field

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Sep 26, 2024 at 12:04:06PM +0100, Andrew Cooper wrote:
> On 26/09/2024 11:14 am, Roger Pau Monne wrote:
> > The Elf loading logic will initially use the `data` section field to stash a
> > pointer to the temporary loaded data (from the buffer allocated in
> > livepatch_upload(), which is later relocated and the new pointer stashed in
> > `load_addr`.
> >
> > Remove this dual field usage and use an `addr` uniformly.  Initially data 
> > will
> > point to the temporary buffer, until relocation happens, at which point the
> > pointer will be updated to the relocated address.
> >
> > This avoids leaking a dangling pointer in the `data` field once the 
> > temporary
> > buffer is freed by livepatch_upload().
> >
> > Note the `addr` field cannot retain the const attribute from the previous
> > `data`field, as there's logic that performs manipulations against the loaded
> > sections, like applying relocations or sorting the exception table.
> >
> > No functional change intended.
> >
> > Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> 
> Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> 
> > diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
> > index df41dcce970a..7e6bf58f4408 100644
> > --- a/xen/common/livepatch.c
> > +++ b/xen/common/livepatch.c
> > @@ -371,18 +371,21 @@ static int move_payload(struct payload *payload, 
> > struct livepatch_elf *elf)
> >  
> >              ASSERT(offset[i] != UINT_MAX);
> >  
> > -            elf->sec[i].load_addr = buf + offset[i];
> > +            buf += offset[i];
> >  
> >              /* Don't copy NOBITS - such as BSS. */
> >              if ( elf->sec[i].sec->sh_type != SHT_NOBITS )
> >              {
> > -                memcpy(elf->sec[i].load_addr, elf->sec[i].data,
> > +                memcpy(buf, elf->sec[i].addr,
> >                         elf->sec[i].sec->sh_size);
> >                  dprintk(XENLOG_DEBUG, LIVEPATCH "%s: Loaded %s at %p\n",
> > -                        elf->name, elf->sec[i].name, 
> > elf->sec[i].load_addr);
> > +                        elf->name, elf->sec[i].name, buf);
> >              }
> >              else
> > -                memset(elf->sec[i].load_addr, 0, elf->sec[i].sec->sh_size);
> > +                memset(buf, 0, elf->sec[i].sec->sh_size);
> > +
> > +            /* Replace the temporary buffer with the relocated one. */
> > +            elf->sec[i].addr = buf;
> 
> I'd suggest /* Update sec[] to refer to its final location. */
> 
> Replace is technically the memcpy() above, and "relocate" means
> something else in ELF terms.

Sure, no strong opinion.

> Can fix on commit.

Thanks!