Re: [PATCH] livepatch: refuse to resolve symbols that belong to init sections

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.pau@xxxxxxxxxx>
> ---
>  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