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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan