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 <[email protected]>
> ---
> CC: Konrad Rzeszutek Wilk <[email protected]>
> CC: Ross Lagerwall <[email protected]>
>
> 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
