Re: [PATCH 1/3] objtool: check: Fully validate the stack frame

[Josh Poimboeuf]

On Mon, Sep 21, 2020 at 11:31:23AM +0100, Julien Thierry wrote:
> > > +++ b/tools/objtool/arch/x86/include/cfi_regs.h
> > > @@ -22,4 +22,8 @@
> > >   #define CFI_RA			16
> > >   #define CFI_NUM_REGS		17
> > > +#define CFA_SIZE	16
> > 
> > If I remember correctly, CFA (stolen from DWARF) is "Caller Frame
> > Address".  It's the stack address of the caller, before the call.
> > 
> 
> Ok, so maybe I'm mixing Call Frame and Stack Frame (frame pointer + return
> address).
> 
> > I get the feeling CFA_SIZE is the wrong name, because CFA is an address,
> > and its size isn't 16 bytes.  But I'm not quite sure what this is
> > supposed to represent.  Is it supposed to be the size of the frame
> > pointer + return address?  Isn't that always going to be 16 bytes for
> > both arches?
> > 
> 
> For arm64 and x86_64 it is. Maybe it is a bit early to consider it might
> differ for other arches (e.g. 32bit arches?).

I'd rather not consider other arches yet.  Even in the 32-bit case it
wouldn't necessarily have to be an arch-specific value since it would
presumably be 'size(long) * 2'.

> 
> > >   static bool has_valid_stack_frame(struct insn_state *state)
> > >   {
> > >   	struct cfi_state *cfi = &state->cfi;
> > > -	if (cfi->cfa.base == CFI_BP && cfi->regs[CFI_BP].base == CFI_CFA &&
> > > -	    cfi->regs[CFI_BP].offset == -16)
> > > +	if (cfi->cfa.base == CFI_BP && cfi->cfa.offset >= CFA_SIZE &&
> > 
> > Why '>=' rather than '=='?
> > 
> 
> Because on arm64 the stack frame is not necessarily the first thing put on
> the stack by the callee. The callee is free to create the stack frame where
> it wants (on its part of the stack of course) as long as it appropriately
> sets the frame pointer before making a call.
> 
> You could have something like:
> 
> +------------+
> |            |
> |            |
> +------------+----> f1() called
> |            |
> | some callee|
> | saved regs |
> |            |
> +------------+
> |     RA     |
> +------------+
> |     BP/FP  |
> +------------+----> Set new BP/FP value

I see, thanks.

-- 
Josh