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

[Jan Beulich <jbeulich@xxxxxxxx>]

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