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.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.
> ... 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
