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

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 11/02/16 12:52, Jan Beulich wrote:
>>>> On 11.02.16 at 13:12, <andrew.cooper3@citrix.com> wrote:
>> On 11/02/16 11:16, Jan Beulich wrote:
>>>>>> On 11.02.16 at 11:52, <andrew.cooper3@citrix.com> wrote:
>>>> --- 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.
>> I think in this specific case, it might be best to hand roll some asm
>> using rep movs and the direction flag, with manual fault handling.
> That's certainly an option.

It is turning out to be much nicer.

>
>>> 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.
>> I would like, where possible, to avoid omitting the instruction stream
>> if Xen is outside of its expected boundaries.
> Which is because of ...? What useful information do you think can
> be gained from the actual instruction when the mere fact of being
> outside the boundaries is a bug?

Wherever %rip is pointing, the code under %rip is directly relevant to
the exact values of the registers and stack dump printed.

It will be obvious from the numeric value of %rip whether it is bad
(also, whether symbol information is found), and making it work for all
cases is easier than restricting to the Xen-only case.

~Andrew