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 .text. Any non-zero sized
replacements must be fully contained within .altinstr_replacement.
Fixes: f8a10174e8b1 ("xsplice: Add support for alternatives")
Signed-off-by: Andrew Cooper <andrew.coop...@citrix.com>
---
CC: Konrad Rzeszutek Wilk <konrad.w...@oracle.com>
CC: Ross Lagerwall <ross.lagerw...@citrix.com>
As a further observation, .altinstr_replacement shouldn't survive beyond its
use in apply_alternatives(), but the disp32 relative references (for x86 at
least) in alt_instr force .altinstr_replacement to be close to the payload
while being applied.
---
xen/common/livepatch.c | 66 ++++++++++++++++++++++++++++++++++++++----
1 file changed, 60 insertions(+), 6 deletions(-)
diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index 784fbd92e913..020a9648d5ba 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -803,28 +803,82 @@ 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 bytes.
+ *
+ * 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 be the start of 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+%#lx\n",
+ elf->name, orig, a->orig_len, payload->text_addr,
payload->text_size);
+ return -EINVAL;
+ }
- if ( (instr < region->start && instr >= region->end) ||
- (replacement < region->start && replacement >= region->end) )
+ /*
+ * repl must be fully within .altinstr_replacement, even if they
+ * 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+%#lx\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);
--
2.30.2
