kernel-hardening - Re: [PATCH v4 13/17] arm64: preserve x18 when CPU is suspended

Follow @Openwall on Twitter for new release announcements and other news

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

Date: Tue, 05 Nov 2019 16:04:47 +0109
From: Marc Zyngier <maz@...nel.org>
To: Sami Tolvanen <samitolvanen@...gle.com>
Cc: Nick Desaulniers <ndesaulniers@...gle.com>, Will Deacon
 <will@...nel.org>, Catalin Marinas <catalin.marinas@....com>, Steven Rostedt
 <rostedt@...dmis.org>, Masami Hiramatsu <mhiramat@...nel.org>, Ard
 Biesheuvel <ard.biesheuvel@...aro.org>, Dave Martin <dave.martin@....com>,
 Kees Cook <keescook@...omium.org>, Laura Abbott <labbott@...hat.com>, Mark
 Rutland <mark.rutland@....com>, Jann Horn <jannh@...gle.com>, Miguel Ojeda
 <miguel.ojeda.sandonis@...il.com>, Masahiro Yamada
 <yamada.masahiro@...ionext.com>, clang-built-linux
 <clang-built-linux@...glegroups.com>, Kernel Hardening
 <kernel-hardening@...ts.openwall.com>, linux-arm-kernel
 <linux-arm-kernel@...ts.infradead.org>, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4 13/17] arm64: preserve x18 when CPU is suspended

On 2019-11-05 01:11, Sami Tolvanen wrote:
> On Mon, Nov 4, 2019 at 1:59 PM Nick Desaulniers
> <ndesaulniers@...gle.com> wrote:
>>
>> On Mon, Nov 4, 2019 at 1:38 PM Sami Tolvanen 
>> <samitolvanen@...gle.com> wrote:
>> >
>> > On Mon, Nov 4, 2019 at 5:20 AM Marc Zyngier <maz@...nel.org> 
>> wrote:
>> > > >  ENTRY(cpu_do_suspend)
>> > > >       mrs     x2, tpidr_el0
>> > > > @@ -73,6 +75,9 @@ alternative_endif
>> > > >       stp     x8, x9, [x0, #48]
>> > > >       stp     x10, x11, [x0, #64]
>> > > >       stp     x12, x13, [x0, #80]
>> > > > +#ifdef CONFIG_SHADOW_CALL_STACK
>> > > > +     str     x18, [x0, #96]
>> > > > +#endif
>> > >
>> > > Do we need the #ifdefery here? We didn't add that to the KVM 
>> path,
>> > > and I'd feel better having a single behaviour, specially when
>> > > NR_CTX_REGS is unconditionally sized to hold 13 regs.
>> >
>> > I'm fine with dropping the ifdefs here in v5 unless someone 
>> objects to this.
>>
>> Oh, yeah I guess it would be good to be consistent.  Rather than 
>> drop
>> the ifdefs, would you (Marc) be ok with conditionally setting
>> NR_CTX_REGS based on CONFIG_SHADOW_CALL_STACK, and doing so in KVM?
>> (So 3 ifdefs, rather than 0)?
>>
>> Without any conditionals or comments, it's not clear why x18 is 
>> being
>> saved and restored (unless git blame survives, or a comment is added
>> in place of the ifdefs in v6).
>
> True. Clearing the sleep state buffer in cpu_do_resume is also
> pointless without CONFIG_SHADOW_CALL_STACK, so if the ifdefs are
> removed, some kind of an explanation is needed there.

I can't imagine the overhead being noticeable, and I certainly value
minimizing the testing space. Sticking a comment there should be
enough for people hacking on this to understand that this isn't
entirely dead code.

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...

Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.