Mailing List Archive

commit 1601fa5ac1652ff1d4903160f14c44fae2c668b8
Author: Andrew Cooper <andrew.cooper3@citrix.com>
AuthorDate: Thu Apr 13 20:56:15 2023 +0100
Commit: Andrew Cooper <andrew.cooper3@citrix.com>
CommitDate: Tue Apr 9 12:56:55 2024 +0100

xen/livepatch: Fix .altinstructions safety checks

The prior check has && vs || mixups, making it tautologically false and thus
providing no safety at all. There are boundary errors too.

First start with a comment describing how the .altinstructions and
.altinstr_replacement sections interact, and perform suitable cross-checking.

Second, rewrite the alt_instr loop entirely from scratch. Origin sites have
non-zero size, and must be fully contained within the livepatches .text
section(s). Any non-zero sized replacements must be fully contained within
the .altinstr_replacement section.

Fixes: f8a10174e8b1 ("xsplice: Add support for alternatives")
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Ross Lagerwall <ross.lagerwall@citrix.com>
(cherry picked from commit e74360e4ba4a6b6827a44f8b1b22a0ec4311694a)
---
xen/common/livepatch.c | 69 +++++++++++++++++++++++++++++++++++++++-----
xen/include/xen/elfstructs.h | 2 ++
2 files changed, 64 insertions(+), 7 deletions(-)

diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index 28c09ddf58..352639c47f 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -816,29 +816,84 @@ static int prepare_payload(struct payload *payload,
if ( sec )
{
#ifdef CONFIG_HAS_ALTERNATIVE
+ /*
+ * (As of April 2023), Alternatives are formed of:
+ * - An .altinstructions section with an array of struct alt_instr's.
+ * - An .altinstr_replacement section containing instructions.
+ *
+ * An individual alt_instr contains:
+ * - An orig reference, pointing into .text with a nonzero length
+ * - A repl reference, pointing into .altinstr_replacement
+ *
+ * It is legal to have zero-length replacements, meaning it is legal
+ * for the .altinstr_replacement section to be empty too. An
+ * implementation detail means that a zero-length replacement's repl
+ * reference will still be in the .altinstr_replacement section.
+ */
+ const struct livepatch_elf_sec *repl_sec;
struct alt_instr *a, *start, *end;

if ( !section_ok(elf, sec, sizeof(*a)) )
return -EINVAL;

+ /* Tolerate an empty .altinstructions section... */
+ if ( sec->sec->sh_size == 0 )
+ goto alt_done;
+
+ /* ... but otherwise, there needs to be something to alter... */
+ if ( payload->text_size == 0 )
+ {
+ printk(XENLOG_ERR LIVEPATCH "%s Alternatives provided, but no .text\n",
+ elf->name);
+ return -EINVAL;
+ }
+
+ /* ... and something to be altered to. */
+ repl_sec = livepatch_elf_sec_by_name(elf, ".altinstr_replacement");
+ if ( !repl_sec )
+ {
+ printk(XENLOG_ERR LIVEPATCH "%s .altinstructions provided, but no .altinstr_replacement\n",
+ elf->name);
+ return -EINVAL;
+ }
+
start = sec->load_addr;
end = sec->load_addr + sec->sec->sh_size;

for ( a = start; a < end; a++ )
{
- const void *instr = ALT_ORIG_PTR(a);
- const void *replacement = ALT_REPL_PTR(a);
+ const void *orig = ALT_ORIG_PTR(a);
+ const void *repl = ALT_REPL_PTR(a);
+
+ /* orig must be fully within .text. */
+ if ( orig < payload->text_addr ||
+ a->orig_len > payload->text_size ||
+ orig + a->orig_len > payload->text_addr + payload->text_size )
+ {
+ printk(XENLOG_ERR LIVEPATCH
+ "%s Alternative orig %p+%#x outside payload text %p+%#zx\n",
+ elf->name, orig, a->orig_len,
+ payload->text_addr, payload->text_size);
+ return -EINVAL;
+ }

- if ( (instr < region->text_start && instr >= region->text_end) ||
- (replacement < region->text_start &&
- replacement >= region->text_end) )
+ /*
+ * repl must be fully within .altinstr_replacement, even if the
+ * replacement and the section happen to both have zero length.
+ */
+ if ( repl < repl_sec->load_addr ||
+ a->repl_len > repl_sec->sec->sh_size ||
+ repl + a->repl_len > repl_sec->load_addr + repl_sec->sec->sh_size )
{
- printk(XENLOG_ERR LIVEPATCH "%s Alt patching outside payload: %p\n",
- elf->name, instr);
+ printk(XENLOG_ERR LIVEPATCH
+ "%s Alternative repl %p+%#x outside .altinstr_replacement %p+%#"PRIxElfWord"\n",
+ elf->name, repl, a->repl_len,
+ repl_sec->load_addr, repl_sec->sec->sh_size);
return -EINVAL;
}
}
apply_alternatives(start, end);
+ alt_done:;
#else
printk(XENLOG_ERR LIVEPATCH "%s: We don't support alternative patching\n",
elf->name);
diff --git a/xen/include/xen/elfstructs.h b/xen/include/xen/elfstructs.h
index 3124469fae..eb6b87a823 100644
--- a/xen/include/xen/elfstructs.h
+++ b/xen/include/xen/elfstructs.h
@@ -563,6 +563,7 @@ typedef struct {
#if defined(ELFSIZE) && (ELFSIZE == 32)
#define PRIxElfAddr PRIx32
#define PRIuElfWord PRIu32
+#define PRIxElfWord PRIx32

#define Elf_Ehdr Elf32_Ehdr
#define Elf_Phdr Elf32_Phdr
@@ -591,6 +592,7 @@ typedef struct {
#elif defined(ELFSIZE) && (ELFSIZE == 64)
#define PRIxElfAddr PRIx64
#define PRIuElfWord PRIu64
+#define PRIxElfWord PRIx64

#define Elf_Ehdr Elf64_Ehdr
#define Elf_Phdr Elf64_Phdr
--
generated by git-patchbot for /home/xen/git/xen.git#stable-4.17