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....@citrix.com>
Reviewed-by: Andrew Cooper <andrew.coop...@citrix.com>
> 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.
Can fix on commit.
