On 12/04/2024 9:07 am, Roger Pau Monne wrote:
> Livepatch payloads containing symbols that belong to init sections can only
> lead to page faults later on, as by the time the livepatch is loaded init
> sections have already been freed.
>
> Refuse to resolve such symbols and return an error instead.
>
> Note such resolutions are only relevant for symbols that point to undefined
> sections (SHN_UNDEF), as that implies the symbol is not in the current payload
> and hence must either be a Xen or a different livepatch payload symbol.
>
> Signed-off-by: Roger Pau Monné <roger....@citrix.com>
> ---
> xen/common/livepatch_elf.c | 17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
>
> diff --git a/xen/common/livepatch_elf.c b/xen/common/livepatch_elf.c
> index 45d73912a3cd..25b81189c590 100644
> --- a/xen/common/livepatch_elf.c
> +++ b/xen/common/livepatch_elf.c
> @@ -8,6 +8,9 @@
> #include <xen/livepatch_elf.h>
> #include <xen/livepatch.h>
>
> +/* Needed to check we don't resolve against init section symbols. */
> +extern char __init_begin[], __init_end[];
> +
The livepatching side of this is fine, and definitely an improvement.
However, this is the 3rd set of externs for these symbols. Could I talk
you into doing a prerequisite patch which introduces <xen/sections.h>
and moves the externs out of {arm,x86}/setup.c ?
This will (subsequently - not to interfere with this change) allow us to
clean up kernel.h, and fix the nonsense that currently is __read_mostly
being in cache.h (which is causing problems for RISCV/PPC).
Also judging by kernel.h, they want a /* SAF-0-safe */ tag too for MISRA.
> const struct livepatch_elf_sec *
> livepatch_elf_sec_by_name(const struct livepatch_elf *elf,
> const char *name)
> @@ -310,6 +313,20 @@ int livepatch_elf_resolve_symbols(struct livepatch_elf
> *elf)
> break;
> }
> }
> + /*
> + * Ensure not an init symbol. Only applicable to Xen symbols, as
> + * livepatch payloads don't have init sections or equivalent.
> + */
> + else if ( st_value >= (uintptr_t)&__init_begin &&
> + st_value <= (uintptr_t)&__init_end )
I think you want just < here. A label at __init_end is the start of the
next section.
~Andrew
