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

["Roger Pau Monné" <roger.pau@citrix.com>]

On Fri, Apr 19, 2024 at 11:10:33AM +0100, Andrew Cooper wrote:
> On 19/04/2024 11:02 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.
> >
> > Do not allow to resolve symbols that point to __init_begin, as that address is
> > also unmapped.  On the other hand, __init_end is not unmapped, and hence allow
> > resolutions against it.
> >
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, although ...
> 
> > ---
> > Changes since v1:
> >  - Fix off-by-one in range checking.
> > ---
> >  xen/common/livepatch_elf.c | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> >
> > diff --git a/xen/common/livepatch_elf.c b/xen/common/livepatch_elf.c
> > index 45d73912a3cd..a67101eadc02 100644
> > --- a/xen/common/livepatch_elf.c
> > +++ b/xen/common/livepatch_elf.c
> > @@ -4,6 +4,7 @@
> >  
> >  #include <xen/errno.h>
> >  #include <xen/lib.h>
> > +#include <xen/sections.h>
> >  #include <xen/symbols.h>
> >  #include <xen/livepatch_elf.h>
> >  #include <xen/livepatch.h>
> > @@ -310,6 +311,21 @@ 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 normally vertically the (casts) in cases like this for improved
> legibility.  Happy to fold on commit.

Did this, but reverted afterwards because I wasn't sure whether it
would go against the coding style.  Aligning would be my preference
also.

Thanks, Roger.