Ping?
There was some discussion on whether we need to handle such empty
sections, but I think we settled that it's necessary.
Thanks, Roger.
On Thu, Mar 17, 2022 at 12:08:53PM +0100, Roger Pau Monne wrote:
> A side effect of ignoring such sections is that symbols belonging to
> them won't be resolved, and that could make relocations belonging to
> other sections that reference those symbols fail.
>
> For example it's likely to have an empty .altinstr_replacement with
> symbols pointing to it, and marking the section as ignored will
> prevent the symbols from being resolved, which in turn will cause any
> relocations against them to fail.
>
> In order to solve this do not ignore sections with 0 size, only ignore
> sections that don't have the SHF_ALLOC flag set.
>
> Special case such empty sections in move_payload so they are not taken
> into account in order to decide whether a livepatch can be safely
> re-applied after a revert.
>
> Fixes: 98b728a7b2 ('livepatch: Disallow applying after an revert')
> Signed-off-by: Roger Pau Monné <roger....@citrix.com>
> ---
> xen/common/livepatch.c | 16 +++++++++++-----
> xen/include/xen/livepatch_elf.h | 2 +-
> 2 files changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
> index be2cf75c2d..abc1cae136 100644
> --- a/xen/common/livepatch.c
> +++ b/xen/common/livepatch.c
> @@ -300,9 +300,6 @@ static int move_payload(struct payload *payload, struct
> livepatch_elf *elf)
> * and .shstrtab. For the non-relocate we allocate and copy these
> * via other means - and the .rel we can ignore as we only use it
> * once during loading.
> - *
> - * Also ignore sections with zero size. Those can be for example:
> - * data, or .bss.
> */
> if ( livepatch_elf_ignore_section(elf->sec[i].sec) )
> offset[i] = UINT_MAX;
> @@ -361,8 +358,17 @@ static int move_payload(struct payload *payload, struct
> livepatch_elf *elf)
> else if ( elf->sec[i].sec->sh_flags & SHF_WRITE )
> {
> buf = rw_buf;
> - rw_buf_sec = i;
> - rw_buf_cnt++;
> + if ( elf->sec[i].sec->sh_size )
> + {
> + /*
> + * Special handling of RW empty regions: do not account
> for
> + * them in order to decide whether a patch can safely be
> + * re-applied, but assign them a load address so symbol
> + * resolution and relocations work.
> + */
> + rw_buf_sec = i;
> + rw_buf_cnt++;
> + }
> }
> else
> buf = ro_buf;
> diff --git a/xen/include/xen/livepatch_elf.h b/xen/include/xen/livepatch_elf.h
> index 9ad499ee8b..5b1ec469da 100644
> --- a/xen/include/xen/livepatch_elf.h
> +++ b/xen/include/xen/livepatch_elf.h
> @@ -48,7 +48,7 @@ int livepatch_elf_perform_relocs(struct livepatch_elf *elf);
>
> static inline bool livepatch_elf_ignore_section(const Elf_Shdr *sec)
> {
> - return !(sec->sh_flags & SHF_ALLOC) || sec->sh_size == 0;
> + return !(sec->sh_flags & SHF_ALLOC);
> }
> #endif /* __XEN_LIVEPATCH_ELF_H__ */
>
> --
> 2.34.1
>
