[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v3 1/5] xen/livepatch: drop load_addr Elf section field
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!
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |