Re: [PATCH] xen/livepatch: Fix .altinstructions safety checks

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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@xxxxxxxxxx>
>> ---
>> CC: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
>> CC: Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>
>>
>> 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