linux-kernel - Re: [RFC PATCH 1/2] x86: Fix rbp saving in pt

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 7 Jan 2011 15:26:21 +0100
From:	Frederic Weisbecker <fweisbec@...il.com>
To:	Jan Beulich <JBeulich@...ell.com>
Cc:	"H. Peter Anvin" <a.p.zijlstra@...llo.nl>,
	Ingo Molnar <mingo@...e.hu>,
	Stephane Eranian <eranian@...gle.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Arnaldo Carvalho de Melo <acme@...hat.com>,
	Soeren Sandmann Pedersen <sandmann@...hat.com>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH 1/2] x86: Fix rbp saving in pt_regs on irq entry

On Fri, Jan 07, 2011 at 07:45:53AM +0000, Jan Beulich wrote:
> >>> On 06.01.11 at 18:12, Frederic Weisbecker <fweisbec@...il.com> wrote:
> > On Thu, Jan 06, 2011 at 04:58:54PM +0000, Jan Beulich wrote:
> >> >>> On 06.01.11 at 17:54, Frederic Weisbecker <fweisbec@...il.com> wrote:
> >> > On Thu, Jan 06, 2011 at 04:39:39PM +0000, Jan Beulich wrote:
> >> >> >>> On 06.01.11 at 17:22, Frederic Weisbecker <fweisbec@...il.com> wrote:
> >> >> > On Thu, Jan 06, 2011 at 04:10:55PM +0000, Jan Beulich wrote:
> >> >> >> >>> On 06.01.11 at 16:45, Frederic Weisbecker <fweisbec@...il.com> wrote:
> >> >> >> > Before we had:
> >> >> >> > 
> >> >> >> > 
> >> >> >> > 	leaveq
> >> >> >> > 
> >> >> >> > 	CFI_RESTORE             rbp
> >> >> >> > 	CFI_DEF_CFA_REGISTER    rsp
> >> >> >> > 	CFI_ADJUST_CFA_OFFSET   -8
> >> >> >> > 
> >> >> >> > So CFI_RESTORE means rbp has now the value of the base frame of
> >> >> >> > the calling frame (the base frame pointer of the interrupted proc) ?
> >> >> >> 
> >> >> >> No - all it means is that %rbp now has its original (caller or
> >> >> >> interrupted procedure) value again (i.e. an unwinder should not
> >> >> >> try to read it from the stack [or other previously recorded
> >> >> >> location] anymore).
> >> >> >> 
> >> >> >> > And what follows means that rsp-8 points to the return address?
> >> >> >> 
> >> >> >> No - .cfi_def_cfa_register says which register serves as the frame
> >> >> >> pointer, and .cfi_adjust_cfa_offset says to adjust the offset from
> >> >> >> the frame pointer to the top [or bottom] of frame. At any time
> >> >> >> 
> >> >> >> 	CFA = cfa_register + cfa_offset
> >> >> >> 
> >> >> >> with CFA being what all locations on the stack are expressed
> >> >> >> relative to.
> >> >> > 
> >> >> > Ok.
> >> >> > 
> >> >> > So here rsp points to pt_regs::r11
> >> >> > 
> >> >> > I don't understand why locations relative to the stack must be
> >> >> > expressed here by taking rsp - 8 as a base.
> >> >> 
> >> >> Nothing says rsp-8. The annotations merely say to set the base
> >> >> register to rsp and to *adjust* the offset by -8 (after all, that's
> >> >> what the leaveq instruction does).
> >> > 
> >> > Ah! So CFA acts like a virtual frame base pointer right?
> >> 
> >> Correct.
> > 
> > Ah great. I was starting to prepare for the case you come to stab me :)
> > 
> > So what do you think about that:
> > 
> >         leaveq
> > 
> >         CFI_RESTORE             rbp
> >         CFI_DEF_CFA_REGISTER    rsp
> >         CFI_ADJUST_CFA_OFFSET   -8
> > 
> >        /* we did not save rbx, restore only from ARGOFFSET */
> >        addq $8, %rsp
> >        CFI_ADJUST_CFA_OFFSET   -16
> > 
> > 
> > Does that look correct to you? We increased rsp to start recovering
> > the regs from the right place, but the frame pointer of the current
> > proc must stay what it was.
> 
> As you hinted in your subsequent reply - it's -8 here (that's
> why the directive is named *adjust*; there are other
> directives allowing to *set* an offset).

Ok, I'll respin with a proper patch then.
 
> > Now I don't understand how this is all useful as this is not a normal
> > proc but an interruption. We can't get back the return address from
> > the CFA. Or am I missing something?
> 
> Unwind annotations, when written correctly, allow unwinding
> through all kinds of execution flows, including interrupts or
> exceptions as well as including stack switches.

Hmm I see, I guess this is handled through the movq_cfi things we have,
so that the unwinder can ignore the whole part with the saved registers
after which we can find the instruction pointer (considered as the return
address) saved by the hardware.

Fine, thanks for your explanations!
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/