xen/arch/arm/arm32/livepatch.c | 27 ++++++++++---- xen/arch/arm/arm64/livepatch.c | 23 ++++++++++-- xen/arch/x86/livepatch.c | 23 ++++++++++-- xen/common/livepatch.c | 13 ++++++- xen/common/livepatch_elf.c | 62 +++++++++++++++++++++++++++++++-- xen/include/xen/livepatch.h | 6 ++-- xen/include/xen/livepatch_elf.h | 4 ++- 7 files changed, 139 insertions(+), 19 deletions(-)
In a livepatch payload relocations will refer to included functions. If
that function happens to be a replacement for an existing Xen function, the
relocations on the livepatch payload will use the newly introduced symbol,
rather than the old one. This is usually fine, but if the result of the
relocation is stored for later use (for example in the domain struct),
usages of this address will lead to a page-fault once the livepatch is
reverted.
Implement a second pass over relocations once the list of replaced
functions has been loaded, and fixup any references to replaced functions
to use the old symbol address instead of the new one. There are some
sections that must be special cased to continue using the new symbol
address, as those instances must reference the newly added livepatch
content (for example the alternative patch sites).
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
xen/arch/arm/arm32/livepatch.c | 27 ++++++++++----
xen/arch/arm/arm64/livepatch.c | 23 ++++++++++--
xen/arch/x86/livepatch.c | 23 ++++++++++--
xen/common/livepatch.c | 13 ++++++-
xen/common/livepatch_elf.c | 62 +++++++++++++++++++++++++++++++--
xen/include/xen/livepatch.h | 6 ++--
xen/include/xen/livepatch_elf.h | 4 ++-
7 files changed, 139 insertions(+), 19 deletions(-)
diff --git a/xen/arch/arm/arm32/livepatch.c b/xen/arch/arm/arm32/livepatch.c
index 134d07a175bb..8c12099d6a1d 100644
--- a/xen/arch/arm/arm32/livepatch.c
+++ b/xen/arch/arm/arm32/livepatch.c
@@ -224,7 +224,7 @@ static int perform_rel(unsigned char type, void *dest, uint32_t val,
int arch_livepatch_perform(struct livepatch_elf *elf,
const struct livepatch_elf_sec *base,
const struct livepatch_elf_sec *rela,
- bool use_rela)
+ bool fixup, bool use_rela)
{
unsigned int i;
int rc = 0;
@@ -282,7 +282,20 @@ int arch_livepatch_perform(struct livepatch_elf *elf,
return -EINVAL;
}
- val = elf->sym[symndx].sym->st_value; /* S */
+ if ( fixup )
+ {
+ val = livepatch_elf_use_old_addr(elf,
+ elf->sym[symndx].sym->st_value);
+ if ( val == elf->sym[symndx].sym->st_value )
+ /* Don't re-apply if the symbol value hasn't changed. */
+ continue;
+ dprintk(XENLOG_DEBUG, LIVEPATCH
+ "%s: Using old address for symbol %s at reloc %s + %#x\n",
+ elf->name, elf->sym[symndx].name, base->name,
+ dest - base->addr);
+ }
+ else
+ val = elf->sym[symndx].sym->st_value; /* S */
rc = perform_rel(type, dest, val, addend);
switch ( rc )
@@ -307,16 +320,18 @@ int arch_livepatch_perform(struct livepatch_elf *elf,
int arch_livepatch_perform_rel(struct livepatch_elf *elf,
const struct livepatch_elf_sec *base,
- const struct livepatch_elf_sec *rela)
+ const struct livepatch_elf_sec *rela,
+ bool fixup)
{
- return arch_livepatch_perform(elf, base, rela, false);
+ return arch_livepatch_perform(elf, base, rela, fixup, false);
}
int arch_livepatch_perform_rela(struct livepatch_elf *elf,
const struct livepatch_elf_sec *base,
- const struct livepatch_elf_sec *rela)
+ const struct livepatch_elf_sec *rela
+ bool fixup)
{
- return arch_livepatch_perform(elf, base, rela, true);
+ return arch_livepatch_perform(elf, base, rela, fixup, true);
}
/*
diff --git a/xen/arch/arm/arm64/livepatch.c b/xen/arch/arm/arm64/livepatch.c
index e135bd5bf99a..bdbe611832fb 100644
--- a/xen/arch/arm/arm64/livepatch.c
+++ b/xen/arch/arm/arm64/livepatch.c
@@ -233,14 +233,16 @@ static int reloc_insn_imm(enum aarch64_reloc_op op, void *dest, u64 val,
int arch_livepatch_perform_rel(struct livepatch_elf *elf,
const struct livepatch_elf_sec *base,
- const struct livepatch_elf_sec *rela)
+ const struct livepatch_elf_sec *rela,
+ bool fixup)
{
return -ENOSYS;
}
int arch_livepatch_perform_rela(struct livepatch_elf *elf,
const struct livepatch_elf_sec *base,
- const struct livepatch_elf_sec *rela)
+ const struct livepatch_elf_sec *rela,
+ bool fixup)
{
unsigned int i;
@@ -279,7 +281,22 @@ int arch_livepatch_perform_rela(struct livepatch_elf *elf,
return -EINVAL;
}
- val = elf->sym[symndx].sym->st_value + r->r_addend; /* S+A */
+ if ( fixup )
+ {
+ val = livepatch_elf_use_old_addr(elf,
+ elf->sym[symndx].sym->st_value);
+ if ( val == elf->sym[symndx].sym->st_value )
+ /* Don't re-apply if the symbol value hasn't changed. */
+ continue;
+ dprintk(XENLOG_DEBUG, LIVEPATCH
+ "%s: Using old address for symbol %s at reloc %s + %#lx\n",
+ elf->name, elf->sym[symndx].name, base->name,
+ r->r_offset);
+ }
+ else
+ val = elf->sym[symndx].sym->st_value; /* S */
+
+ val += r->r_addend; /* S + A */
/* ARM64 operations at minimum are always 32-bit. */
if ( r->r_offset >= base->sec->sh_size ||
diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c
index be40f625d206..2944fdb9f91e 100644
--- a/xen/arch/x86/livepatch.c
+++ b/xen/arch/x86/livepatch.c
@@ -243,7 +243,8 @@ bool arch_livepatch_symbol_deny(const struct livepatch_elf *elf,
int arch_livepatch_perform_rel(struct livepatch_elf *elf,
const struct livepatch_elf_sec *base,
- const struct livepatch_elf_sec *rela)
+ const struct livepatch_elf_sec *rela,
+ bool fixup)
{
printk(XENLOG_ERR LIVEPATCH "%s: SHT_REL relocation unsupported\n",
elf->name);
@@ -252,7 +253,8 @@ int arch_livepatch_perform_rel(struct livepatch_elf *elf,
int arch_livepatch_perform_rela(struct livepatch_elf *elf,
const struct livepatch_elf_sec *base,
- const struct livepatch_elf_sec *rela)
+ const struct livepatch_elf_sec *rela,
+ bool fixup)
{
unsigned int i;
@@ -289,7 +291,22 @@ int arch_livepatch_perform_rela(struct livepatch_elf *elf,
return -EINVAL;
}
- val = r->r_addend + elf->sym[symndx].sym->st_value;
+ if ( fixup )
+ {
+ val = livepatch_elf_use_old_addr(elf,
+ elf->sym[symndx].sym->st_value);
+ if ( val == elf->sym[symndx].sym->st_value )
+ /* Don't re-apply if the symbol value hasn't changed. */
+ continue;
+ dprintk(XENLOG_DEBUG, LIVEPATCH
+ "%s: Using old address for symbol %s at reloc %s + %#lx\n",
+ elf->name, elf->sym[symndx].name, base->name,
+ r->r_offset);
+ }
+ else
+ val = elf->sym[symndx].sym->st_value;
+
+ val += r->r_addend;
switch ( ELF64_R_TYPE(r->r_info) )
{
diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index 9a0df5363b59..6bffd1954469 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -741,6 +741,17 @@ static int prepare_payload(struct payload *payload,
if ( rc )
return rc;
}
+
+ /*
+ * Do a second pass over the relocations and adjust those that use a
+ * replacement symbol to continue using the old address. Otherwise
+ * storing those relocated addresses would lead to crashes when the
+ * livepatch is reverted if there are stored references that point to
+ * the reverted symbols.
+ */
+ rc = livepatch_elf_perform_relocs(elf, true);
+ if ( rc )
+ return rc;
}
LIVEPATCH_ASSIGN_MULTI_HOOK(elf, payload->load_funcs, payload->n_load_funcs, ELF_LIVEPATCH_LOAD_HOOKS);
@@ -1164,7 +1175,7 @@ static int load_payload_data(struct payload *payload, void *raw, size_t len)
if ( rc )
goto out;
- rc = livepatch_elf_perform_relocs(&elf);
+ rc = livepatch_elf_perform_relocs(&elf, false);
if ( rc )
goto out;
diff --git a/xen/common/livepatch_elf.c b/xen/common/livepatch_elf.c
index 25ce1bd5a0ad..f6d4c8584718 100644
--- a/xen/common/livepatch_elf.c
+++ b/xen/common/livepatch_elf.c
@@ -358,7 +358,29 @@ int livepatch_elf_resolve_symbols(struct livepatch_elf *elf)
return rc;
}
-int livepatch_elf_perform_relocs(struct livepatch_elf *elf)
+Elf_Addr livepatch_elf_use_old_addr(const struct livepatch_elf *elf,
+ Elf_Addr new)
+{
+ const struct livepatch_elf_sec *replacements =
+ livepatch_elf_sec_by_name(elf, ELF_LIVEPATCH_FUNC);
+ struct livepatch_func *f;
+ unsigned int i;
+
+ if ( !replacements )
+ {
+ ASSERT_UNREACHABLE();
+ return new;
+ }
+
+ f = replacements->addr;
+ for ( i = 0; i < replacements->sec->sh_size / sizeof(*f); i++ )
+ if ( (Elf_Addr)f[0].new_addr == new )
+ return (Elf_Addr)f[0].old_addr;
+
+ return new;
+}
+
+int livepatch_elf_perform_relocs(struct livepatch_elf *elf, bool fixup)
{
struct livepatch_elf_sec *r, *base;
unsigned int i;
@@ -381,6 +403,40 @@ int livepatch_elf_perform_relocs(struct livepatch_elf *elf)
base = &elf->sec[r->sec->sh_info];
+ if ( fixup )
+ {
+ static const char *const ignore[] = {
+ /* Do not fixup the replacement coordinates. */
+ ELF_LIVEPATCH_FUNC,
+ /*
+ * alternatives and exception table fixups need to unconditionally
+ * reference the new functions, or otherwise they won't be applied
+ * correctly.
+ */
+ ".altinstructions",
+ ".alt_call_sites",
+ ".ex_table",
+ /*
+ * livepatch hooks use the old function addresses: if there's a need
+ * for the hooks to call the newly added function replacements a
+ * non-aliased duplicate must be added.
+ */
+ };
+ unsigned int j;
+
+ /* Bug frames must use the payload addresses. */
+ if ( !strncmp(base->name, ".bug_frames.", 12) )
+ continue;
+
+ /* If doing fixups ignore some special sections. */
+ for ( j = 0; j < ARRAY_SIZE(ignore); j++ )
+ if ( !strcmp(base->name, ignore[j]) )
+ break;
+
+ if ( j != ARRAY_SIZE(ignore) )
+ continue;
+ }
+
/* Don't relocate non-allocated sections. */
if ( !(base->sec->sh_flags & SHF_ALLOC) )
continue;
@@ -410,9 +466,9 @@ int livepatch_elf_perform_relocs(struct livepatch_elf *elf)
}
if ( r->sec->sh_type == SHT_RELA )
- rc = arch_livepatch_perform_rela(elf, base, r);
+ rc = arch_livepatch_perform_rela(elf, base, r, fixup);
else /* SHT_REL */
- rc = arch_livepatch_perform_rel(elf, base, r);
+ rc = arch_livepatch_perform_rel(elf, base, r, fixup);
if ( rc )
break;
diff --git a/xen/include/xen/livepatch.h b/xen/include/xen/livepatch.h
index d074a5bebecc..84091ea9c33e 100644
--- a/xen/include/xen/livepatch.h
+++ b/xen/include/xen/livepatch.h
@@ -75,10 +75,12 @@ bool arch_livepatch_symbol_deny(const struct livepatch_elf *elf,
const struct livepatch_elf_sym *sym);
int arch_livepatch_perform_rel(struct livepatch_elf *elf,
const struct livepatch_elf_sec *base,
- const struct livepatch_elf_sec *rela);
+ const struct livepatch_elf_sec *rela,
+ bool fixup);
int arch_livepatch_perform_rela(struct livepatch_elf *elf,
const struct livepatch_elf_sec *base,
- const struct livepatch_elf_sec *rela);
+ const struct livepatch_elf_sec *rela,
+ bool fixup);
enum va_type {
LIVEPATCH_VA_RX, /* .text */
LIVEPATCH_VA_RW, /* .data */
diff --git a/xen/include/xen/livepatch_elf.h b/xen/include/xen/livepatch_elf.h
index 842111e14518..acd98b10d8da 100644
--- a/xen/include/xen/livepatch_elf.h
+++ b/xen/include/xen/livepatch_elf.h
@@ -46,7 +46,9 @@ int livepatch_elf_load(struct livepatch_elf *elf, void *data);
void livepatch_elf_free(struct livepatch_elf *elf);
int livepatch_elf_resolve_symbols(struct livepatch_elf *elf);
-int livepatch_elf_perform_relocs(struct livepatch_elf *elf);
+int livepatch_elf_perform_relocs(struct livepatch_elf *elf, bool fixup);
+Elf_Addr livepatch_elf_use_old_addr(const struct livepatch_elf *elf,
+ Elf_Addr new);
static inline bool livepatch_elf_ignore_section(const Elf_Shdr *sec)
{
--
2.49.0
On Wed, Jul 16, 2025 at 5:00 PM Roger Pau Monne <roger.pau@citrix.com> wrote: > > In a livepatch payload relocations will refer to included functions. If > that function happens to be a replacement for an existing Xen function, the > relocations on the livepatch payload will use the newly introduced symbol, > rather than the old one. This is usually fine, but if the result of the > relocation is stored for later use (for example in the domain struct), > usages of this address will lead to a page-fault once the livepatch is > reverted. > > Implement a second pass over relocations once the list of replaced > functions has been loaded, and fixup any references to replaced functions > to use the old symbol address instead of the new one. There are some > sections that must be special cased to continue using the new symbol > address, as those instances must reference the newly added livepatch > content (for example the alternative patch sites). > Would it be possible to fix this at build time instead by generating the payload such that relocations to changed functions always point at the Xen function rather than the included function? This seems preferable to me rather than making the runtime code more complicated. Of course, neither approach would fix all cases - if the function is completely new calling it would still page-fault after reverting the live patch - so the usual care needs to be taken when creating live patches. Ross
On Mon, Jul 21, 2025 at 04:51:33PM +0100, Ross Lagerwall wrote: > On Wed, Jul 16, 2025 at 5:00 PM Roger Pau Monne <roger.pau@citrix.com> wrote: > > > > In a livepatch payload relocations will refer to included functions. If > > that function happens to be a replacement for an existing Xen function, the > > relocations on the livepatch payload will use the newly introduced symbol, > > rather than the old one. This is usually fine, but if the result of the > > relocation is stored for later use (for example in the domain struct), > > usages of this address will lead to a page-fault once the livepatch is > > reverted. > > > > Implement a second pass over relocations once the list of replaced > > functions has been loaded, and fixup any references to replaced functions > > to use the old symbol address instead of the new one. There are some > > sections that must be special cased to continue using the new symbol > > address, as those instances must reference the newly added livepatch > > content (for example the alternative patch sites). > > > > Would it be possible to fix this at build time instead by generating the > payload such that relocations to changed functions always point at the > Xen function rather than the included function? Hm, it's kind of complicated (I'm not saying it's impossible). I think it will require renaming the replaced function symbols, and then introducing new unresolved symbols that reference the existing functions, and adjusting the relocations to point to those new symbols. Then Xen would resolve those symbols and the relocations would be calculated using the old addresses. You can not unconditionally replace all the relocations that point to replacement symbols, as some of the relocations will need to point to new vs old instances (just as this patch does, where not all relocations are adjusted to point to the old addresses). I can give it a try, but looks more complicated than the approach here, as it requires crafting extra symbols to add to the payload, and adjust relocations. > This seems preferable to me rather than making the runtime code more > complicated. > > Of course, neither approach would fix all cases - if the function is > completely new calling it would still page-fault after reverting the > live patch - so the usual care needs to be taken when creating live > patches. Indeed, storing pointers to newly introduced functions won't work, that needs care, but it should be easier to spot by code review of the livepatch. Would be nice to get an automated way to prevent such pointers from getting stored, but I can't think of a way to do it. Thanks, Roger.
On 21.07.2025 17:51, Ross Lagerwall wrote: > On Wed, Jul 16, 2025 at 5:00 PM Roger Pau Monne <roger.pau@citrix.com> wrote: >> >> In a livepatch payload relocations will refer to included functions. If >> that function happens to be a replacement for an existing Xen function, the >> relocations on the livepatch payload will use the newly introduced symbol, >> rather than the old one. This is usually fine, but if the result of the >> relocation is stored for later use (for example in the domain struct), >> usages of this address will lead to a page-fault once the livepatch is >> reverted. >> >> Implement a second pass over relocations once the list of replaced >> functions has been loaded, and fixup any references to replaced functions >> to use the old symbol address instead of the new one. There are some >> sections that must be special cased to continue using the new symbol >> address, as those instances must reference the newly added livepatch >> content (for example the alternative patch sites). >> > > Would it be possible to fix this at build time instead by generating the > payload such that relocations to changed functions always point at the > Xen function rather than the included function? How would you express the difference in the object file? If that's possible (without problematic heuristics), ... > This seems preferable to me rather than making the runtime code more > complicated. ... I agree this would be quite a bit better. Jan > Of course, neither approach would fix all cases - if the function is > completely new calling it would still page-fault after reverting the > live patch - so the usual care needs to be taken when creating live > patches. > > Ross
On 16.07.2025 18:00, Roger Pau Monne wrote: > In a livepatch payload relocations will refer to included functions. If > that function happens to be a replacement for an existing Xen function, the > relocations on the livepatch payload will use the newly introduced symbol, > rather than the old one. This is usually fine, but if the result of the > relocation is stored for later use (for example in the domain struct), > usages of this address will lead to a page-fault once the livepatch is > reverted. > > Implement a second pass over relocations once the list of replaced > functions has been loaded, and fixup any references to replaced functions > to use the old symbol address instead of the new one. There are some > sections that must be special cased to continue using the new symbol > address, as those instances must reference the newly added livepatch > content (for example the alternative patch sites). This is what I was fearing, when you first mentioned the problem (and the plan) to me. What I don't see is why you do your fixing up regardless of relocation type. Relative relocations within the payload ought to be fine to not override? At which point some of the special casing may already no longer be necessary. (Later) Except that if code uses PC-relative addressing to determine a pointer to store into some struct, that'll appear as a relative relocation type, too. But then you may have a bigger problem: When referencing and referenced code are in the same section and in the same translation unit, the assembler could avoid emitting a relocation altogether. You would see nothing to fix up ... Jan
On Wed, Jul 16, 2025 at 06:31:03PM +0200, Jan Beulich wrote: > On 16.07.2025 18:00, Roger Pau Monne wrote: > > In a livepatch payload relocations will refer to included functions. If > > that function happens to be a replacement for an existing Xen function, the > > relocations on the livepatch payload will use the newly introduced symbol, > > rather than the old one. This is usually fine, but if the result of the > > relocation is stored for later use (for example in the domain struct), > > usages of this address will lead to a page-fault once the livepatch is > > reverted. > > > > Implement a second pass over relocations once the list of replaced > > functions has been loaded, and fixup any references to replaced functions > > to use the old symbol address instead of the new one. There are some > > sections that must be special cased to continue using the new symbol > > address, as those instances must reference the newly added livepatch > > content (for example the alternative patch sites). > > This is what I was fearing, when you first mentioned the problem (and the > plan) to me. What I don't see is why you do your fixing up regardless of > relocation type. Relative relocations within the payload ought to be fine > to not override? At which point some of the special casing may already no > longer be necessary. > > (Later) Except that if code uses PC-relative addressing to determine a > pointer to store into some struct, that'll appear as a relative relocation > type, too. But then you may have a bigger problem: When referencing and > referenced code are in the same section and in the same translation unit, > the assembler could avoid emitting a relocation altogether. You would see > nothing to fix up ... The only way for the referencing and referenced code to be in the same function would be for the function to reference itself, which should be quite rare? I don't recall seeing any code in Xen where a function stores a pointer to itself. Otherwise each function is in a separate section, and hence references to functions should always use a relocation. Thanks, Roger.
On 22.07.2025 17:02, Roger Pau Monné wrote: > On Wed, Jul 16, 2025 at 06:31:03PM +0200, Jan Beulich wrote: >> On 16.07.2025 18:00, Roger Pau Monne wrote: >>> In a livepatch payload relocations will refer to included functions. If >>> that function happens to be a replacement for an existing Xen function, the >>> relocations on the livepatch payload will use the newly introduced symbol, >>> rather than the old one. This is usually fine, but if the result of the >>> relocation is stored for later use (for example in the domain struct), >>> usages of this address will lead to a page-fault once the livepatch is >>> reverted. >>> >>> Implement a second pass over relocations once the list of replaced >>> functions has been loaded, and fixup any references to replaced functions >>> to use the old symbol address instead of the new one. There are some >>> sections that must be special cased to continue using the new symbol >>> address, as those instances must reference the newly added livepatch >>> content (for example the alternative patch sites). >> >> This is what I was fearing, when you first mentioned the problem (and the >> plan) to me. What I don't see is why you do your fixing up regardless of >> relocation type. Relative relocations within the payload ought to be fine >> to not override? At which point some of the special casing may already no >> longer be necessary. >> >> (Later) Except that if code uses PC-relative addressing to determine a >> pointer to store into some struct, that'll appear as a relative relocation >> type, too. But then you may have a bigger problem: When referencing and >> referenced code are in the same section and in the same translation unit, >> the assembler could avoid emitting a relocation altogether. You would see >> nothing to fix up ... > > The only way for the referencing and referenced code to be in the same > function would be for the function to reference itself, which should > be quite rare? I don't recall seeing any code in Xen where a function > stores a pointer to itself. > > Otherwise each function is in a separate section, and hence references > to functions should always use a relocation. Oh, right, I forgot about -ffunction-sections being in use. Jan
© 2016 - 2025 Red Hat, Inc.