From: Michael Ellerman <mpe@ellerman•id.au>
To: xiujianfeng <xiujianfeng@huawei•com>,
Nicholas Piggin <npiggin@gmail•com>,
benh@kernel•crashing.org, christophe.leroy@csgroup•eu,
mark.rutland@arm•com, paulus@samba•org, tglx@linutronix•de
Cc: linuxppc-dev@lists•ozlabs.org, linux-hardening@vger•kernel.org,
linux-kernel@vger•kernel.org
Subject: Re: [PATCH -next] powerpc: add support for syscall stack randomization
Date: Thu, 12 May 2022 23:17:04 +1000 [thread overview]
Message-ID: <87pmki7uwf.fsf@mpe.ellerman.id.au> (raw)
In-Reply-To: <a1dcd50b-0819-df54-a963-ebb0551e3356@huawei.com>
xiujianfeng <xiujianfeng@huawei•com> writes:
> 在 2022/5/10 17:23, Nicholas Piggin 写道:
>> Excerpts from Xiu Jianfeng's message of May 5, 2022 9:19 pm:
>>> Add support for adding a random offset to the stack while handling
>>> syscalls. This patch uses mftb() instead of get_random_int() for better
>>> performance.
>>
...
>>
>>> @@ -405,6 +407,7 @@ interrupt_exit_user_prepare_main(unsigned long ret, struct pt_regs *regs)
>>>
>>> /* Restore user access locks last */
>>> kuap_user_restore(regs);
>>> + choose_random_kstack_offset(mftb() & 0xFF);
>>>
>>> return ret;
>>> }
>> So this seems to be what x86 and s390 do, but why are we choosing a
>> new offset for every interrupt when it's only used on a syscall?
>> I would rather you do what arm64 does and just choose the offset
>> at the end of system_call_exception.
> thanks for you suggestion, will do in v2.
>>
>> I wonder why the choose is separated from the add? I guess it's to
>> avoid a data dependency for stack access on an expensive random
>> function, so that makes sense (a comment would be nice in the
>> generic code).
>>
>> I don't actually know if mftb() is cheaper here than a RNG. It
>> may not be conditioned all that well either. I would be tempted
> #if defined(__powerpc64__) && (defined(CONFIG_PPC_CELL) ||
> defined(CONFIG_E500))
> #define mftb() ({unsigned long rval; \
> asm volatile( \
> "90: mfspr %0, %2;\n" \
> ASM_FTR_IFSET( \
> "97: cmpwi %0,0;\n" \
> " beq- 90b;\n", "", %1) \
> : "=r" (rval) \
> : "i" (CPU_FTR_CELL_TB_BUG), "i" (SPRN_TBRL) :
> "cr0"); \
> rval;})
> #elif defined(CONFIG_PPC_8xx)
> #define mftb() ({unsigned long rval; \
> asm volatile("mftbl %0" : "=r" (rval)); rval;})
> #else
> #define mftb() ({unsigned long rval; \
> asm volatile("mfspr %0, %1" : \
> "=r" (rval) : "i" (SPRN_TBRL));
> rval;})
> #endif /* !CONFIG_PPC_CELL */
>
> there are 3 implementations of mftb() in
> arch/powerpc/include/asm/vdso/timebase.h,
>
> the last two cases have only one instruction, It's obviously cheaper
> than get_random_int,
Just because it's one instruction doesn't mean it's obviously cheaper.
On some CPUs mftb takes 10s of cycles, and can also stall the pipeline.
But looking at get_random_u32() it does look pretty complicated, it
takes a lock and so on. It's also silly to call get_random_u32() for
4-bits of randomness.
My initial impression was that mftb() is too predictable to be useful
against a determined attacker. But looking closer I see that
choose_random_kstack_offset() xor's the value we pass with the existing
value. So that makes me less worried about using mftb().
We could additionally call choose_random_kstack_offset(get_random_int())
less regularly, eg. during context switch. But I guess that's too
infrequent to actually make any difference.
But limiting it to 4-bits of randomness seems insufficient. It seems
like we should allow the full 6 (10) bits, and anyone turning this
option on should probably also consider increasing their stack size.
Also did you check the help text about stack-protector under
HAVE_ARCH_RANDOMIZE_KSTACK_OFFSET?
cheers
next prev parent reply other threads:[~2022-05-12 13:17 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-05-05 11:19 [PATCH -next] powerpc: add support for syscall stack randomization Xiu Jianfeng
2022-05-10 9:23 ` Nicholas Piggin
2022-05-10 16:19 ` Kees Cook
2022-05-11 8:36 ` xiujianfeng
2022-05-12 13:03 ` Michael Ellerman
2022-05-11 8:34 ` xiujianfeng
2022-05-12 13:17 ` Michael Ellerman [this message]
2022-05-16 7:29 ` xiujianfeng
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87pmki7uwf.fsf@mpe.ellerman.id.au \
--to=mpe@ellerman$(echo .)id.au \
--cc=benh@kernel$(echo .)crashing.org \
--cc=christophe.leroy@csgroup$(echo .)eu \
--cc=linux-hardening@vger$(echo .)kernel.org \
--cc=linux-kernel@vger$(echo .)kernel.org \
--cc=linuxppc-dev@lists$(echo .)ozlabs.org \
--cc=mark.rutland@arm$(echo .)com \
--cc=npiggin@gmail$(echo .)com \
--cc=paulus@samba$(echo .)org \
--cc=tglx@linutronix$(echo .)de \
--cc=xiujianfeng@huawei$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox