[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [xen master] xen/livepatch: Fix .altinstructions safety checks
commit e74360e4ba4a6b6827a44f8b1b22a0ec4311694a Author: Andrew Cooper <andrew.cooper3@xxxxxxxxxx> AuthorDate: Thu Apr 13 20:56:15 2023 +0100 Commit: Andrew Cooper <andrew.cooper3@xxxxxxxxxx> CommitDate: Wed Apr 19 15:55:30 2023 +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@xxxxxxxxxx> Reviewed-by: Ross Lagerwall <ross.lagerwall@xxxxxxxxxx> --- xen/common/livepatch.c | 68 ++++++++++++++++++++++++++++++++++++++++---- xen/include/xen/elfstructs.h | 2 ++ 2 files changed, 64 insertions(+), 6 deletions(-) diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c index d385f882c6..d89a904bd4 100644 --- a/xen/common/livepatch.c +++ b/xen/common/livepatch.c @@ -808,28 +808,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->start && instr >= region->end) || - (replacement < region->start && replacement >= region->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#master
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |