Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 21 Sep 2020 10:03:48 -0500
From:   Josh Poimboeuf <jpoimboe@redhat.com>
To:     Julien Thierry <jthierry@redhat.com>
Cc:     linux-kernel@vger.kernel.org, peterz@infradead.org, mbenes@suse.cz,
        raphael.gault@arm.com, benh@kernel.crashing.org
Subject: Re: [PATCH 1/3] objtool: check: Fully validate the stack frame
Message-ID: <20200921150348.egfsznskrpebuyri@treble>
References: <20200915081204.9204-1-jthierry@redhat.com>
 <20200915081204.9204-2-jthierry@redhat.com>
 <20200918205641.6nvnsib2paqa6xyn@treble>
 <9a40f898-8f6a-2ba0-1954-0d06be5e2ef8@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <9a40f898-8f6a-2ba0-1954-0d06be5e2ef8@redhat.com>
Precedence: bulk

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