Re: [PATCH v3 3/4] livepatch: refuse to resolve symbols that belong to init sections

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

On Tue, Apr 23, 2024 at 03:44:42PM +0200, Jan Beulich wrote:
> On 23.04.2024 15:12, 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.
> > 
> > Since __init_begin can alias other symbols (like _erodata for example)
> > allow the force flag to override the check and resolve the symbol anyway.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> 
> In principle, as promised (and just to indicate earlier concerns were
> addressed, as this is meaningless for other purposes)
> Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
> However, ...
> 
> > @@ -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 && !force )
> > +            {
> > +                printk(XENLOG_ERR LIVEPATCH
> > +                       "%s: symbol %s is in init section, not resolving\n",
> > +                       elf->name, elf->sym[i].name);
> > +                rc = -ENXIO;
> > +                break;
> > +            }
> 
> ... wouldn't it make sense to still warn in this case when "force" is set?

Pondered it, I was thinking that a user would first run without
--force, and use the option as a result of seeing the first failure.

However if there is more than one check that's bypassed, further ones
won't be noticed, so:

            else if ( st_value >= (uintptr_t)&__init_begin &&
                      st_value <  (uintptr_t)&__init_end )
            {
                printk(XENLOG_ERR LIVEPATCH
                       "%s: symbol %s is in init section, not resolving\n",
                       elf->name, elf->sym[i].name);
                if ( !force )
                {
                    rc = -ENXIO;
                    break;
                }
            }

Would be OK then?

Thanks, Roger.