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

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

On 11/02/16 11:16, Jan Beulich wrote:
>>>> On 11.02.16 at 11:52, <andrew.cooper3@citrix.com> 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.

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.

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

~Andrew