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....@citrix.com>
>>>>
>>>> In principle, as promised (and just to indicate earlier concerns were
>>>> addressed, as this is meaningless for other purposes)
>>>> Acked-by: Jan Beulich <jbeul...@suse.com>
>>>> 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
