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

From: Jan Beulich <jbeulich@suse.com>
To: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
	Ross Lagerwall <ross.lagerwall@citrix.com>,
	Xen-devel <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH] xen/livepatch: Fix .altinstructions safety checks
Date: Mon, 17 Apr 2023 12:01:41 +0200	[thread overview]
Message-ID: <dd6615fb-dc2d-9885-e3a3-9cf0954f57d3@suse.com> (raw)
In-Reply-To: <20230415022229.3475033-1-andrew.cooper3@citrix.com>

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