linux-kernel - Re: [PATCH 4/5] x86: entry_64.S: always allocate complete "struct 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:	Mon, 4 Aug 2014 05:03:42 +0200
From:	Denys Vlasenko <vda.linux@...glemail.com>
To:	Frederic Weisbecker <fweisbec@...il.com>
Cc:	Denys Vlasenko <dvlasenk@...hat.com>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Oleg Nesterov <oleg@...hat.com>,
	"H. Peter Anvin" <hpa@...or.com>,
	Andy Lutomirski <luto@...capital.net>, X86 ML <x86@...nel.org>,
	Alexei Starovoitov <ast@...mgrid.com>,
	Will Drewry <wad@...omium.org>,
	Kees Cook <keescook@...omium.org>
Subject: Re: [PATCH 4/5] x86: entry_64.S: always allocate complete "struct pt_regs"

On Sat, Aug 2, 2014 at 1:19 AM, Frederic Weisbecker <fweisbec@...il.com> wrote:
> On Fri, Aug 01, 2014 at 04:48:17PM +0200, Denys Vlasenko wrote:
>>
>>  /* 0(%rsp): ~(interrupt number) */
>>       .macro interrupt func
>> -     /* reserve pt_regs for scratch regs and rbp */
>> -     subq $ORIG_RAX-RBP, %rsp
>> -     CFI_ADJUST_CFA_OFFSET ORIG_RAX-RBP
>> -     cld
>> -     /* start from rbp in pt_regs and jump over */
>> -     movq_cfi rdi, (RDI-RBP)
>> -     movq_cfi rsi, (RSI-RBP)
>> -     movq_cfi rdx, (RDX-RBP)
>> -     movq_cfi rcx, (RCX-RBP)
>> -     movq_cfi rax, (RAX-RBP)
>> -     movq_cfi  r8,  (R8-RBP)
>> -     movq_cfi  r9,  (R9-RBP)
>> -     movq_cfi r10, (R10-RBP)
>> -     movq_cfi r11, (R11-RBP)
>> -
>> -     /* Save rbp so that we can unwind from get_irq_regs() */
>> -     movq_cfi rbp, 0
>
> Hmm SAVEE_C_REGS below doesn't seem to save rbp like we did before.
> Perhaps it's implicitely saved somewhere?
>
>> -
>> -     /* Save previous stack value */
>> -     movq %rsp, %rsi
>
> Also rsp isn't saved in %rsi like before. Maybe
> that's because we already save it in rdi? Makes sense since
> now arg1 == rsp. More on that later.
>
>> -
>> -     leaq -RBP(%rsp),%rdi    /* arg1 for handler */
>> -     testl $3, CS-RBP(%rsi)
>> +     ALLOC_PTREGS_ON_STACK
>> +     SAVE_C_REGS
>> +     movq %rsp, %rdi /* arg1 for handler */
>> +     testl $3, CS(%rsp)
>>       je 1f
>>       SWAPGS
>> -     /*
>> +1:   /*
>>        * irq_count is used to check if a CPU is already on an interrupt stack
>>        * or not. While this is essentially redundant with preempt_count it is
>>        * a little cheaper to use a separate counter in the PDA (short of
>>        * moving irq_enter into assembly, which would be too much work)
>>        */
>> -1:   incl PER_CPU_VAR(irq_count)
>> +     incl PER_CPU_VAR(irq_count)
>>       cmovzq PER_CPU_VAR(irq_stack_ptr),%rsp
>> -     CFI_DEF_CFA_REGISTER    rsi
>> +     CFI_DEF_CFA_REGISTER    rdi
>>
>>       /* Store previous stack value */
>> -     pushq %rsi
>> +     pushq %rdi
>
> So you push rdi instead...
>
>>       CFI_ESCAPE      0x0f /* DW_CFA_def_cfa_expression */, 6, \
>>                       0x77 /* DW_OP_breg7 */, 0, \
>>                       0x06 /* DW_OP_deref */, \
>> -                     0x08 /* DW_OP_const1u */, SS+8-RBP, \
>> +                     0x08 /* DW_OP_const1u */, SS+8, \
>>                       0x22 /* DW_OP_plus */
>>       /* We entered an interrupt context - irqs are off: */
>>       TRACE_IRQS_OFF
>> -
>>       call \func
>>       .endm
>>
>> @@ -749,10 +719,9 @@ ret_from_intr:
>>
>>       /* Restore saved previous stack */
>>       popq %rsi
>
> And then you pop to rsi. Ok that indeed works but perhaps we should keep it symetrical
> just for clarity? Any reason why we can't reuse rdi here?

I changed this entire area in v2: basically, I will not change the logic,
but will add comments explaining what are we doing here, and why.
(Some minor code changes will be done, not affecting the logic).

While we are at it, what this  CFI_ESCAPE thing does here?
As usual, it has no comment :/

-- 
vda
--
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/