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.
Jan
