From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH v2 2/2] x86/traps: Dump instruction stream in show_execution_state() Date: Thu, 11 Feb 2016 12:12:30 +0000 Message-ID: <56BC7AAE.2060004@citrix.com> References: <1455187972-30566-1-git-send-email-andrew.cooper3@citrix.com> <1455187972-30566-2-git-send-email-andrew.cooper3@citrix.com> <56BC7BA402000078000D0F31@prv-mh.provo.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <56BC7BA402000078000D0F31@prv-mh.provo.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: Xen-devel List-Id: xen-devel@lists.xenproject.org On 11/02/16 11:16, Jan Beulich wrote: >>>> On 11.02.16 at 11:52, 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