| Message ID | 20230415022229.3475033-1-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/livepatch: Fix .altinstructions safety checks | expand |
On 15.04.2023 04:22, Andrew Cooper wrote: > 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. Or .init.text, which may be worth making explicit (perhaps also in the respective code comment). Or am I misremembering and livepatch blobs, unlike e.g. Linux modules, don't support the concept of .init.* sections? > Any non-zero sized > replacements must be fully contained within .altinstr_replacement. Yes, but if they're all zero-sized, in principle no .altinstr_replacement section could be there. Not sure though whether that's worth supporting as a special case. Furthermore, ... > 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> > > 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. ... if .altinstr_replacement is retained right now anyway, isn't it legal to fold it with another section (e.g. .text) while linking? > --- 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. Since this is generic code, perhaps drop "bytes"? (Or else use "instruction 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. "will" or "may"? And especially if indeed "will", is it really worth mentioning this here in this way, posing a fair risk of the comment going stale entirely unnoticed? > + */ > + 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. Who is "they ... both" here? Surely it doesn't matter here whether "orig_len" is zero. Jan
On 17/04/2023 11:01 am, Jan Beulich wrote: > On 15.04.2023 04:22, Andrew Cooper wrote: >> 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. > Or .init.text, which may be worth making explicit (perhaps also in the > respective code comment). Or am I misremembering and livepatch blobs, > unlike e.g. Linux modules, don't support the concept of .init.* sections? Here, we're talking strictly about the .alt* and .text of the livepatch itself. I suppose it would be nice if the safety check / other one-time hooks could live in a local .init.text for the livepatch itself, but we don't have this concept at the moment. > >> Any non-zero sized >> replacements must be fully contained within .altinstr_replacement. > Yes, but if they're all zero-sized, in principle no .altinstr_replacement > section could be there. Not sure though whether that's worth supporting > as a special case. This is discussed in the source code comment. Right now, all zero-length replacements reference the altinstr_replacement section, so the section is present even if it's not got any data in it. If this changes in the future, we can accommodate. > Furthermore, ... > >> 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> >> >> 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. > ... if .altinstr_replacement is retained right now anyway, isn't it legal > to fold it with another section (e.g. .text) while linking? Why would it be? We're auditing what the current tools actually produce, not any arbitrary theoretical one. > >> --- 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. > Since this is generic code, perhaps drop "bytes"? (Or else use "instruction > bytes"?) Technically bytes is ok here, but yeah - it will be slightly better without. >> + * 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. > "will" or "may"? And especially if indeed "will", is it really worth mentioning > this here in this way, posing a fair risk of the comment going stale entirely > unnoticed? Hmm. Thinking about it, I expect that it's not actually always the start. The code uses pushsection/ref/{repl bytes}/popsection, so for an empty replacement it will probably reference the end of the previously replacement. I should tweak the comment, but the logic is fine. All I check is that [repl, size) is entirely within altinstr_replacement, with no special case at 0. > >> + */ >> + 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. > Who is "they ... both" here? Surely it doesn't matter here whether "orig_len" > is zero. I haven't explicitly rejected it, but an orig_len of 0 is an error. "they" is repl_len and altinstr_replacement. And FYI, I need to repost this as part of a 3-patch series in order to not break the ARM build. ~Andrew
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);
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.cooper3@citrix.com> --- CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> CC: Ross Lagerwall <ross.lagerwall@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(-)