Re: [PATCH 2/3] xen/livepatch: do Xen build-id check earlier

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 20/09/2024 10:36 am, Roger Pau Monne wrote:
> The check against the expected Xen build ID should be done ahead of attempting
> to apply the alternatives contained in the livepatch.
>
> If the CPUID in the alternatives patching data is out of the scope of the
> running Xen featureset the BUG() in _apply_alternatives() will trigger thus
> bringing the system down.  Note the layout of struct alt_instr could also
> change between versions.  It's also possible for struct exception_table_entry
> to have changed format, hence possibly leading to other errors.
>
> Move the Xen build ID check to be done ahead of any processing of the 
> livepatch
> payload sections.
>
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> ---
>  xen/common/livepatch.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
> index cea47ffe4c84..3e4fce036a1c 100644
> --- a/xen/common/livepatch.c
> +++ b/xen/common/livepatch.c
> @@ -767,6 +767,11 @@ static int prepare_payload(struct payload *payload,
>      if ( rc )
>          return rc;
>  
> +    /* Perform the Xen build-id check ahead of doing any more processing. */
> +    rc = xen_build_id_dep(payload);
> +    if ( rc )
> +        return rc;
> +

While a step in the right direction, I think this needs to be moved far
earlier.  Even here, it's behind the processing of the livepatch func
state, which is something that can also change like alt_instr.

The buildid checks need to be as early as possible.  Looking through the
logic (which doesn't have great names/splits), I'd say the buildid
checks want to be between livepatch_elf_load() (which parses the
structure of the ELF), and move_payload() (which starts copying it into
place).

That would involve moving check_special_sections() too, but I think it's
the right thing to do.

Absolutely nothing good can come from continuing to process/setup a
livepatch identified as "not for this hypervisor".

~Andrew