2020-09-15 08:26:55

by Julien Thierry

Subject: [PATCH 3/3] objtool: check: Make SP memory operation match PUSH/POP semantics

Architectures without PUSH/POP instructions will always access the stack
though memory operations (SRC/DEST_INDIRECT). Make those operations have
the same effect on the CFA as PUSH/POP, with no stack pointer
modification.

Signed-off-by: Julien Thierry <[email protected]>
---
tools/objtool/check.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)

diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index f45991c2db41..7ff87fa3caec 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -2005,6 +2005,13 @@ static int update_cfi_state(struct instruction *insn, struct cfi_state *cfi,
break;

case OP_SRC_REG_INDIRECT:
+ if (!cfi->drap && op->dest.reg == cfa->base) {
+
+ /* mov disp(%rsp), %rbp */
+ cfa->base = CFI_SP;
+ cfa->offset = cfi->stack_size;
+ }
+
if (cfi->drap && op->src.reg == CFI_BP &&
op->src.offset == cfi->drap_offset) {

@@ -2026,6 +2033,11 @@ static int update_cfi_state(struct instruction *insn, struct cfi_state *cfi,
/* mov disp(%rbp), %reg */
/* mov disp(%rsp), %reg */
restore_reg(cfi, op->dest.reg);
+ } else if (op->src.reg == CFI_SP &&
+ op->src.offset == regs[op->dest.reg].offset + cfi->stack_size) {
+
+ /* mov disp(%rsp), %reg */
+ restore_reg(cfi, op->dest.reg);
}

break;
@@ -2103,6 +2115,11 @@ static int update_cfi_state(struct instruction *insn, struct cfi_state *cfi,
/* mov reg, disp(%rsp) */
save_reg(cfi, op->src.reg, CFI_CFA,
op->dest.offset - cfi->cfa.offset);
+ } else if (op->dest.reg == CFI_SP) {
+
+ /* mov reg, disp(%rsp) */
+ save_reg(cfi, op->src.reg, CFI_CFA,
+ op->dest.offset - cfi->stack_size);
}

break;
--
2.21.3

2020-09-18 21:45:06

by Josh Poimboeuf

Subject: Re: [PATCH 3/3] objtool: check: Make SP memory operation match PUSH/POP semantics

On Tue, Sep 15, 2020 at 09:12:04AM +0100, Julien Thierry wrote:
> Architectures without PUSH/POP instructions will always access the stack
> though memory operations (SRC/DEST_INDIRECT). Make those operations have
> the same effect on the CFA as PUSH/POP, with no stack pointer
> modification.
>
> Signed-off-by: Julien Thierry <[email protected]>
> ---
> tools/objtool/check.c | 17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
>
> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
> index f45991c2db41..7ff87fa3caec 100644
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -2005,6 +2005,13 @@ static int update_cfi_state(struct instruction *insn, struct cfi_state *cfi,
> break;
>
> case OP_SRC_REG_INDIRECT:
> + if (!cfi->drap && op->dest.reg == cfa->base) {

&& op->dest.reg == CFI_BP ?

> +
> + /* mov disp(%rsp), %rbp */
> + cfa->base = CFI_SP;
> + cfa->offset = cfi->stack_size;
> + }
> +
> if (cfi->drap && op->src.reg == CFI_BP &&
> op->src.offset == cfi->drap_offset) {
>
> @@ -2026,6 +2033,11 @@ static int update_cfi_state(struct instruction *insn, struct cfi_state *cfi,
> /* mov disp(%rbp), %reg */
> /* mov disp(%rsp), %reg */
> restore_reg(cfi, op->dest.reg);
> + } else if (op->src.reg == CFI_SP &&

An empty line above the else would help readability.

> + op->src.offset == regs[op->dest.reg].offset + cfi->stack_size) {
> +
> + /* mov disp(%rsp), %reg */
> + restore_reg(cfi, op->dest.reg);

> }
>
> break;
> @@ -2103,6 +2115,11 @@ static int update_cfi_state(struct instruction *insn, struct cfi_state *cfi,
> /* mov reg, disp(%rsp) */
> save_reg(cfi, op->src.reg, CFI_CFA,
> op->dest.offset - cfi->cfa.offset);
> + } else if (op->dest.reg == CFI_SP) {

Same here.

> +
> + /* mov reg, disp(%rsp) */
> + save_reg(cfi, op->src.reg, CFI_CFA,
> + op->dest.offset - cfi->stack_size);
> }
>
> break;
> --
> 2.21.3
>

--
Josh

2020-09-21 10:34:54

by Julien Thierry

Subject: Re: [PATCH 3/3] objtool: check: Make SP memory operation match PUSH/POP semantics

On 9/18/20 10:43 PM, Josh Poimboeuf wrote:
> On Tue, Sep 15, 2020 at 09:12:04AM +0100, Julien Thierry wrote:
>> Architectures without PUSH/POP instructions will always access the stack
>> though memory operations (SRC/DEST_INDIRECT). Make those operations have
>> the same effect on the CFA as PUSH/POP, with no stack pointer
>> modification.
>>
>> Signed-off-by: Julien Thierry <[email protected]>
>> ---
>> tools/objtool/check.c | 17 +++++++++++++++++
>> 1 file changed, 17 insertions(+)
>>
>> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
>> index f45991c2db41..7ff87fa3caec 100644
>> --- a/tools/objtool/check.c
>> +++ b/tools/objtool/check.c
>> @@ -2005,6 +2005,13 @@ static int update_cfi_state(struct instruction *insn, struct cfi_state *cfi,
>> break;
>>
>> case OP_SRC_REG_INDIRECT:
>> + if (!cfi->drap && op->dest.reg == cfa->base) {
>
> && op->dest.reg == CFI_BP ?
>

Does it matter? My unstandig was that the register used to point to the
CFA is getting overwritten, so we need to fallback to something known
which is the offset from the stack pointer.

Was that not the case?

>> +
>> + /* mov disp(%rsp), %rbp */
>> + cfa->base = CFI_SP;
>> + cfa->offset = cfi->stack_size;
>> + }
>> +
>> if (cfi->drap && op->src.reg == CFI_BP &&
>> op->src.offset == cfi->drap_offset) {
>>
>> @@ -2026,6 +2033,11 @@ static int update_cfi_state(struct instruction *insn, struct cfi_state *cfi,
>> /* mov disp(%rbp), %reg */
>> /* mov disp(%rsp), %reg */
>> restore_reg(cfi, op->dest.reg);
>> + } else if (op->src.reg == CFI_SP &&
>
> An empty line above the else would help readability.
>
>> + op->src.offset == regs[op->dest.reg].offset + cfi->stack_size) {
>> +
>> + /* mov disp(%rsp), %reg */
>> + restore_reg(cfi, op->dest.reg);
>
>> }
>>
>> break;
>> @@ -2103,6 +2115,11 @@ static int update_cfi_state(struct instruction *insn, struct cfi_state *cfi,
>> /* mov reg, disp(%rsp) */
>> save_reg(cfi, op->src.reg, CFI_CFA,
>> op->dest.offset - cfi->cfa.offset);
>> + } else if (op->dest.reg == CFI_SP) {
>
> Same here.
>

I'll add those.

>> +
>> + /* mov reg, disp(%rsp) */
>> + save_reg(cfi, op->src.reg, CFI_CFA,
>> + op->dest.offset - cfi->stack_size);
>> }
>>
>> break;
>> --
>> 2.21.3
>>
>

--
Julien Thierry

2020-09-21 15:14:04

by Josh Poimboeuf

Subject: Re: [PATCH 3/3] objtool: check: Make SP memory operation match PUSH/POP semantics

On Mon, Sep 21, 2020 at 11:31:28AM +0100, Julien Thierry wrote:
> > > diff --git a/tools/objtool/check.c b/tools/objtool/check.c
> > > index f45991c2db41..7ff87fa3caec 100644
> > > --- a/tools/objtool/check.c
> > > +++ b/tools/objtool/check.c
> > > @@ -2005,6 +2005,13 @@ static int update_cfi_state(struct instruction *insn, struct cfi_state *cfi,
> > > break;
> > > case OP_SRC_REG_INDIRECT:
> > > + if (!cfi->drap && op->dest.reg == cfa->base) {
> >
> > && op->dest.reg == CFI_BP ?
> >
>
> Does it matter? My unstandig was that the register used to point to the CFA
> is getting overwritten, so we need to fallback to something known which is
> the offset from the stack pointer.
>
> Was that not the case?

Maybe. I was wondering if it would be possible to overwrite the stack
pointer, like 'mov disp(%rsp), %rsp', which could be possible in asm.

Though I suppose the below code would be harmless, since the CFA
base/offset would already be CFI_SP/cfi->stack_size respectively.

Still, no harm in making the condition more precise.

> > > +
> > > + /* mov disp(%rsp), %rbp */
> > > + cfa->base = CFI_SP;
> > > + cfa->offset = cfi->stack_size;
> > > + }

--
Josh