| Message ID | 20230417121357.3738919-4-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/livepatch: Fix .altinstructions safety checks | expand |
On 17.04.2023 14:13, Andrew Cooper wrote: > --- a/xen/common/livepatch.c > +++ b/xen/common/livepatch.c > @@ -803,28 +803,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. Didn't you agree that "will" is not really true, and it's at best "may", but then also doesn't really matter here in the first place (suggesting that the sentence might best be dropped, to avoid drawing attention to something that might at best confuse the reader as to its relevance)? Jan
On 17/04/2023 1:35 pm, Jan Beulich wrote: > On 17.04.2023 14:13, Andrew Cooper wrote: >> --- a/xen/common/livepatch.c >> +++ b/xen/common/livepatch.c >> @@ -803,28 +803,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. > Didn't you agree that "will" is not really true, and it's at best "may", but > then also doesn't really matter here in the first place (suggesting that the > sentence might best be dropped, to avoid drawing attention to something that > might at best confuse the reader as to its relevance)? Only that "will be at 0" wasn't actually true. Right now, the repl reference *will* be somewhere in altinstr_replacement. It is discussed here because it is what the check enforces. As an implementation detail, it is of course free to change in the future if needs be. ~Andrew
On 17.04.2023 15:37, Andrew Cooper wrote: > On 17/04/2023 1:35 pm, Jan Beulich wrote: >> On 17.04.2023 14:13, Andrew Cooper wrote: >>> --- a/xen/common/livepatch.c >>> +++ b/xen/common/livepatch.c >>> @@ -803,28 +803,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. >> Didn't you agree that "will" is not really true, and it's at best "may", but >> then also doesn't really matter here in the first place (suggesting that the >> sentence might best be dropped, to avoid drawing attention to something that >> might at best confuse the reader as to its relevance)? > > Only that "will be at 0" wasn't actually true. Oh, right - I'm sorry for the noise. Jan > Right now, the repl reference *will* be somewhere in > altinstr_replacement. It is discussed here because it is what the check > enforces. > > As an implementation detail, it is of course free to change in the > future if needs be. > > ~Andrew
> From: Andrew Cooper <andrew.cooper3@citrix.com> > Sent: Monday, April 17, 2023 1:13 PM > To: Xen-devel <xen-devel@lists.xenproject.org> > Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; Ross Lagerwall <ross.lagerwall@citrix.com> > Subject: [PATCH v2 3/3] 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>
diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c index c10ab1f374e0..004b5a436569 100644 --- a/xen/common/livepatch.c +++ b/xen/common/livepatch.c @@ -803,28 +803,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 3124469faeb4..eb6b87a823a8 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
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> --- CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> CC: Ross Lagerwall <ross.lagerwall@citrix.com> v2: * Rebase over prior patches to keep the ARM build working * Tweak commit message and comments for clarity 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 | 68 ++++++++++++++++++++++++++++++++---- xen/include/xen/elfstructs.h | 2 ++ 2 files changed, 64 insertions(+), 6 deletions(-)