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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

Can fix on commit.