| Message ID | 20240412080722.75971-1-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | livepatch: refuse to resolve symbols that belong to init sections | expand |
On 12/04/2024 9:07 am, 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. > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > --- > xen/common/livepatch_elf.c | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > > diff --git a/xen/common/livepatch_elf.c b/xen/common/livepatch_elf.c > index 45d73912a3cd..25b81189c590 100644 > --- a/xen/common/livepatch_elf.c > +++ b/xen/common/livepatch_elf.c > @@ -8,6 +8,9 @@ > #include <xen/livepatch_elf.h> > #include <xen/livepatch.h> > > +/* Needed to check we don't resolve against init section symbols. */ > +extern char __init_begin[], __init_end[]; > + The livepatching side of this is fine, and definitely an improvement. However, this is the 3rd set of externs for these symbols. Could I talk you into doing a prerequisite patch which introduces <xen/sections.h> and moves the externs out of {arm,x86}/setup.c ? This will (subsequently - not to interfere with this change) allow us to clean up kernel.h, and fix the nonsense that currently is __read_mostly being in cache.h (which is causing problems for RISCV/PPC). Also judging by kernel.h, they want a /* SAF-0-safe */ tag too for MISRA. > const struct livepatch_elf_sec * > livepatch_elf_sec_by_name(const struct livepatch_elf *elf, > const char *name) > @@ -310,6 +313,20 @@ 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 ) I think you want just < here. A label at __init_end is the start of the next section. ~Andrew
On 12.04.2024 12:59, Andrew Cooper wrote: > On 12/04/2024 9:07 am, Roger Pau Monne wrote: >> @@ -310,6 +313,20 @@ 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 ) > > I think you want just < here. A label at __init_end is the start of the > next section. May be, but not "is". It could be a reference to __init_end itself, for example. Similarly st_value == __init_begin could be a reference to the section end of the earlier section. I'm afraid the checks here are always going to be fuzzy, and hence the description needs to discuss the policy used as to what is permitted and what is not. Jan
diff --git a/xen/common/livepatch_elf.c b/xen/common/livepatch_elf.c index 45d73912a3cd..25b81189c590 100644 --- a/xen/common/livepatch_elf.c +++ b/xen/common/livepatch_elf.c @@ -8,6 +8,9 @@ #include <xen/livepatch_elf.h> #include <xen/livepatch.h> +/* Needed to check we don't resolve against init section symbols. */ +extern char __init_begin[], __init_end[]; + const struct livepatch_elf_sec * livepatch_elf_sec_by_name(const struct livepatch_elf *elf, const char *name) @@ -310,6 +313,20 @@ 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 ) + { + printk(XENLOG_ERR LIVEPATCH + "%s: symbol %s is in init section, not resolving\n", + elf->name, elf->sym[i].name); + rc = -ENXIO; + break; + } + dprintk(XENLOG_DEBUG, LIVEPATCH "%s: Undefined symbol resolved: %s => %#"PRIxElfAddr"\n", elf->name, elf->sym[i].name, st_value); break;
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. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- xen/common/livepatch_elf.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)