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é
In principle, as promised (and just to indicate earlier concerns were
addressed, as this is meaningless for other purposes)
Acked-by: Jan Beulich
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
Re: [PATCH v3 3/4] livepatch: refuse to resolve symbols that belong to init sections
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é
> >>
> >> In principle, as promised (and just to indicate earlier concerns were
> >> addressed, as this is meaningless for other purposes)
> >> Acked-by: Jan Beulich
> >> 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;
}
}
Thanks, Roger.
Re: [PATCH v3 3/4] livepatch: refuse to resolve symbols that belong to init sections
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é
>>
>> In principle, as promised (and just to indicate earlier concerns were
>> addressed, as this is meaningless for other purposes)
>> Acked-by: Jan Beulich
>> 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
Re: [PATCH v3 3/4] livepatch: refuse to resolve symbols that belong to init sections
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é
>
> In principle, as promised (and just to indicate earlier concerns were
> addressed, as this is meaningless for other purposes)
> Acked-by: Jan Beulich
> 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?
Thanks, Roger.
Re: [PATCH v3 3/4] livepatch: refuse to resolve symbols that belong to init sections
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é
In principle, as promised (and just to indicate earlier concerns were
addressed, as this is meaningless for other purposes)
Acked-by: Jan Beulich
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?
Jan
