[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v3 3/4] livepatch: refuse to resolve symbols that belong to init sections
On 23.04.2024 17:03, Roger Pau Monné wrote: > 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; > } > } I'd be okay with this; the livepatch maintainers will have the ultimate say. Jan
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |