Re: [Xen-devel] [PATCH v2 2/2] x86/traps: Dump instruction stream in show_execution_state()

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 11.02.16 at 11:52, <andrew.cooper3@xxxxxxxxxx> wrote:
> v2: Deal with %rip wrapping.  In such a case, insert dashes into printed 
> line.

I don't think this deals with wrapping now, since ...

> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -114,6 +114,56 @@ boolean_param("ler", opt_ler);
>  #define stack_words_per_line 4
>  #define ESP_BEFORE_EXCEPTION(regs) ((unsigned long *)regs->rsp)
>  
> +static void show_code(const struct cpu_user_regs *regs)
> +{
> +    unsigned char insns_before[8], insns_after[16];
> +    unsigned int i, missing_before, missing_after;
> +
> +    if ( guest_mode(regs) )
> +        return;
> +
> +    /*
> +     * This dance with {insns,missing}_{before,after} is to ensure that, if
> +     * %rip -8/+16 wraps around a bounday, we read a non-wrapped regs->rip
> +     * pointer, and calculate which bytes were not read so they may be
> +     * replaced with dashes in the printed output.
> +     */
> +    missing_before = __copy_from_user(
> +        insns_before, (void __user *)regs->rip - 8, 
> ARRAY_SIZE(insns_before));
> +    missing_after = __copy_from_user(
> +        insns_after, (void __user *)regs->rip, ARRAY_SIZE(insns_after));

... iirc __copy_from_user() doesn't range check the addresses.
Also reading the leading bytes is done in kind of a strange way: It'll
read initial bytes (farther away from RIP) and perhaps not read
later ones (closer to RIP), albeit clearly the ones closer are of more
interest. In the extreme case, where RIP is only a few bytes into a
page following an unmapped one, no leading bytes would be printed
at all despite some actually being readable.

Avoiding actual wrapping could be easily done by extending the
guest_mode() check above to also range check RIP against the
hypervisor image boundaries (post-boot this could even be limited
further, but perhaps using the full XEN_VIRT_{START,END} range is
the better route with xSplice in mind.

And then there's a literal 8 left in the first function invocation
above, which imo would better also be ARRAY_SIZE(insns_before)
(albeit with the comment above this invocation is going to change
anyway).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel