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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Sep 23, 2024 at 12:04:30PM +0100, Andrew Cooper wrote:
> 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.

My plan would be to move check_special_sections() ahead and expand its
logic to also check that the expected buildid matches the running
hypervisor one.

Thanks, Roger.