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 04:28:59PM +0200, Jan Beulich wrote:
> On 23.04.2024 16:26, Roger Pau Monné wrote:
> > 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?
> 
> Perhaps. "not resolving" isn't quite true when "force" is true, and warnings
> would also better not be issued with XENLOG_ERR.

I was assuming that printing as XENLOG_ERR level would still be OK -
even if bypassed because of the usage of --force.  The "not resolving"
part should indeed go away. Maybe this is better:

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

Thanks, Roger.